Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

execute("begin immediate") throwing OperationalError #72704

Closed
fschulze mannequin opened this issue Oct 24, 2016 · 35 comments
Closed

execute("begin immediate") throwing OperationalError #72704

fschulze mannequin opened this issue Oct 24, 2016 · 35 comments
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@fschulze
Copy link
Mannequin

fschulze mannequin commented Oct 24, 2016

BPO 28518
Nosy @jaraco, @ned-deily, @bitdancer, @socketpair, @berkerpeksag, @serhiy-storchaka, @animalize, @zhangyangyu, @palaviv, @fschulze
PRs
  • bpo-28518: Start a transaction implicitly before a DML statement #245
  • [3.6] bpo-28518: Start a transaction implicitly before a DML statement (#245) #318
  • [Do Not Merge] Sample of CPython life with blurb. #703
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • 28518.patch
  • sqlite-explicit-begin.patch
  • sqlite-explicit-begin2.patch
  • sqlite-ddl-dml.patch
  • sqlite-ddl-dml-2.patch
  • sqlite-ddl-dml-3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-02-26.16:09:49.161>
    created_at = <Date 2016-10-24.11:05:18.283>
    labels = ['3.7', 'type-bug', 'library']
    title = 'execute("begin immediate") throwing OperationalError'
    updated_at = <Date 2017-03-31.16:36:23.953>
    user = 'https://github.com/fschulze'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:23.953>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-26.16:09:49.161>
    closer = 'berker.peksag'
    components = ['Library (Lib)']
    creation = <Date 2016-10-24.11:05:18.283>
    creator = 'fschulze'
    dependencies = []
    files = ['45639', '45644', '45923', '46079', '46354', '46397']
    hgrepos = []
    issue_num = 28518
    keywords = ['patch']
    message_count = 35.0
    messages = ['279301', '279305', '279808', '281626', '281633', '281638', '281639', '281691', '281700', '281704', '281709', '281724', '281735', '281738', '281739', '282463', '283345', '283398', '284268', '284373', '284392', '284397', '284538', '284585', '285618', '285893', '286084', '286087', '286122', '286300', '286308', '286525', '288599', '290380', '290391']
    nosy_count = 14.0
    nosy_names = ['ghaering', 'jaraco', 'ned.deily', 'r.david.murray', 'socketpair', 'berker.peksag', 'serhiy.storchaka', 'malin', 'xiang.zhang', 'palaviv', 'Big Stone', 'fschulze', 'callidomus', 'C\xc3\xa9dric Bellegarde']
    pr_nums = ['245', '318', '703', '552']
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28518'
    versions = ['Python 3.6', 'Python 3.7']

    @fschulze
    Copy link
    Mannequin Author

    fschulze mannequin commented Oct 24, 2016

    Using:

    conn = sqlite3.connect(':memory:', isolation_level='IMMEDIATE')
    conn.execute('begin immediate')

    Throws:

    sqlite3.OperationalError: cannot start a transaction within a transaction

    This didn't happen in previous versions and the conn.in_transaction attribute is False right before the call to execute, so this situation doesn't seem to be detectable upfront for backward compatibility.

    @fschulze fschulze mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Oct 24, 2016
    @zhangyangyu
    Copy link
    Member

    Looks like commit 284676cf2ac8 in bpo-10740 introduces this. Haven't read through the thread yet.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Oct 31, 2016

    I looked into this error and I think the problem is the sqlite3_stmt_readonly check in _pysqlite_query_execute (cursor.c line 517):

        if (self->connection->begin_statement && !sqlite3_stmt_readonly(self->statement->st) && !self->statement->is_ddl) {
            if (sqlite3_get_autocommit(self->connection->db)) {
                result = _pysqlite_connection_begin(self->connection);
                if (!result) {
                    goto error;
                }
                Py_DECREF(result);
            }
        }

    As you can see we check if the current statement is not readonly and if so we start a transaction. The problem is that sqlite3_stmt_readonly return false for the "begin immediate" statement. When this happens we open a transaction with _pysqlite_connection_begin and then executing the "begin immediate".

    I think this is a error in sqlite as the documentation says:
    "ransaction control statements such as BEGIN, COMMIT, ROLLBACK, SAVEPOINT, and RELEASE cause sqlite3_stmt_readonly() to return true,"
    (https://www.sqlite.org/c3ref/stmt_readonly.html)

    @fschulze
    Copy link
    Mannequin Author

    fschulze mannequin commented Nov 24, 2016

    How do we proceed from here? The Python 3.6.0rc1 is looming. If this is a bug in sqlite, then we need a workaround. We can't wait for a fix in sqlite, because it's not bundled with Python and without the fix Python 3.6 breaks with older sqlite versions. I'd like to help, but don't know how.

    @bitdancer
    Copy link
    Member

    I'm going to mark this as a release blocker because it is a regression. Ned may or may not agree. What is needed is for someone to propose a patch.

    @serhiy-storchaka
    Copy link
    Member

    Sorry, I'm not experienced with sqlite3, but in what circumstances conn.execute('begin immediate') makes sense? Could you please provide a larger example where it is needed?

    @ned-deily
    Copy link
    Member

    Also, if there is indeed a suspected error in the underlying library, has the issue been reported to the SQLite project?

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Nov 25, 2016

    I searched in the sqlite tickets from something related to this issue and I couldn't find anything. I wanted to check the sqlite-users mailing list to see if anyone had already reported it but there is a problem in gmane archives.

    I tried to find a good way to solve this problem but I couldn't. I am adding a patch that should solve this problem but it is for sure not a good way :(.

    @berkerpeksag
    Copy link
    Member

    I haven't had a time to investigate this yet, but I don't think there is an issue with SQLite itself.

    Note that pysqlite behaves the same way since version 2.8.0 and I couldn't find a similar report in their issue tracker.

    Like Serhiy, I'm also a little puzzled with the reproducer. I think if you want to start an explicit transaction, the preferred way would be to set isolation_level to None. Otherwise, the sqlite3 module will start one implicitly for you (or perhaps I still don't understand PEP-249's transaction model 100% correctly :))

    (If we decide that this is not a regression, we should add a test case to cover this use case.)

    @fschulze
    Copy link
    Mannequin Author

    fschulze mannequin commented Nov 25, 2016

    Ok, I reread https://docs.python.org/3/library/sqlite3.html#controlling-transactions several times now. I think it's confusing.

    I want explicit transaction handling, but isolation_level = None turns on "autocommit mode". Unfortunately I don't have suggestions on how to improve the documentation.

    For us, isolation_level = None and the explicit begin immediate seems to be the correct way and it works with all Python versions, including 3.6.0.

    So, I would say this is "not a bug" in Python, but my understanding of the sqlite3 module is flawed. The documentation could be better, but I don't know how to make it better.

    @berkerpeksag
    Copy link
    Member

    There is a patch in bpo-8145 to improve that part of the documentation. It would be great if you could take a look at it :)

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Nov 25, 2016

    There are already a few test's that start a transaction explicitly:

    In dbapi.SqliteOnConflictTests CheckOnConflictRollbackWithExplicitTransaction, CheckOnConflictAbortRaisesWithExplicitTransactions which set isolation_level = None.

    On the other hand in transaction.TransactionalDDL.CheckTransactionalDDL the test do not set the isolation_level to None but start a transaction explicitly. The test pass because sqlite3_stmt_readonly return true for "begin" and "begin deferred".

    I think that if we say that in order to start a transaction implicitly the isolation_level needs to be None CheckTransactionalDDL should be changed.

    @serhiy-storchaka
    Copy link
    Member

    Here is a patch that allows executing "begin immediate" explicitly with any isolation_level. I don't know what behavior is more preferable.

    @BigStone
    Copy link
    Mannequin

    BigStone mannequin commented Nov 25, 2016

    Comment kindly provided by D. Richard Hipp himself:
    """""
    I don't have a login for the python bug tracker so I cannot comment
    there. But I think I see the problem. This is as Aviv Polivoda
    remarks at https://bugs.python.org/issue28518#msg279808

    I think this is a error in sqlite as the documentation says:
    "ransaction control statements such as BEGIN, COMMIT, ROLLBACK,
    SAVEPOINT, and RELEASE cause sqlite3_stmt_readonly() to return true,"

    Except it is not a bug in SQLite, but rather an ambiguity in the
    documentation. In his quote, Aviv omitted the second clause from the
    documentation: "since the statements themselves do not actually
    modify the database but rather they control the timing of when other
    statements modify the database." (Full text here:
    https://www.sqlite.org/c3ref/stmt_readonly.html)

    For a plain BEGIN statement, there are no changes to the database
    file, so sqlite3_stmt_readonly() does indeed return TRUE. But for a
    BEGIN IMMEDIATE statement, there are database changes, because the
    extra IMMEDIATE keyword causes the statement to go ahead and start
    transaction "immediately".

    So technically, the documentation is correct, though I can certainly
    understand that it is misleading and ambiguous as currently worded.
    Clarification will be checked in shortly and will appear in the 3.16.0
    release.

    Note also that behavior of sqlite3_stmt_readonly() has not changed
    since that interface was first added for the 3.7.5 release on
    2011-02-01. So this is not an SQLite regression. Rather, I presume
    that Python has recently started using the sqlite3_stmt_readonly()
    interface in a new way.

    --
    D. Richard Hipp
    drh@sqlite.org
    """""

    @BigStone
    Copy link
    Mannequin

    BigStone mannequin commented Nov 25, 2016

    rewording of sqlite documentation:
    http://www.sqlite.org/src/info/a4205a83e4ed977a

    @ned-deily
    Copy link
    Member

    Thanks everyone for investigating this, in particular Big Stone for the reference to the SQLite project and especially Richard Hipp for once again responding quickly to issues we've brought up. While it would be good to get this resolved soon one way or another (doc and/or code changes), it doesn't sound like it needs to be a release blocker for 3.6.0 so I'm lowering its priority.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Dec 15, 2016

    I think that adding Serhiy patch in addition to the documentation improvement in issue bpo-8145 would be the best way to proceed. This will insure no regression problems.
    We can add a warning when someone try to use BEGIN, ROLLBACK, SAVEPOINT, and RELEASE without changing the isolation_level to None.

    @serhiy-storchaka
    Copy link
    Member

    Here is a little simplified version. No need to handle a single "begin".

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Dec 29, 2016

    Issue bpo-29003 seems to be related to this one. I think that they can be solved the same way as done in Serhiy patch but I would like to suggest a different approach.
    I suggest changing the check for DDL statement with a check for DML statement. We actually should start a transaction only for DML statements (https://docs.python.org/3/library/sqlite3.html#controlling-transactions) so all other statements (DDL, DCL and TCL) should not start a transaction.
    The attached patch solve both this issue and issue bpo-29003.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Dec 31, 2016

    I have no idea about how to fix it, but I would like to suggest that add back this test which was removed in commit 284676cf2ac8 :

    file: /Lib/sqlite3/test/transactions.py
    def CheckVacuum(self):
    self.cur.execute("create table test(i)")
    self.cur.execute("insert into test(i) values (5)")
    self.cur.execute("vacuum")

    Thank you for your efforts! regards.

    @berkerpeksag
    Copy link
    Member

    I only did a quick review, but Aviv's latest patch looks reasonable to me. Thanks!

    And yes, we can add CheckVacuum back if it's not already tested.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Dec 31, 2016

    Adding the vacuum test can't actually happen because the change in commit 284676cf2ac8 changed the API and we don't commit before non DML statements. I opened a issue that will clarify the docs (bpo-29121).

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Jan 3, 2017

    I'm trying to catch up with you.
    Does it mean, if I want execute VACUUM, I have to commit manually? Like this:

    if conn.in_transaction:
        conn.commit()
    conn.execute("vacuum")

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Jan 3, 2017

    Yes. If a transaction is open you will need to explicitly commit before doing vacuum.
    I am not sure if that was intentional or not. From quick look in the sqlite source code there are few things that cannot happen from a transaction:

    1. VACUUM
    2. ATTACH
    3. DETACH
    4. BEGIN
    5. Few PRAGMAs (temp_store, synchronous)

    All of the above worked with implicit commit before commit 284676cf2ac8 but now has to call commit explicitly.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Jan 17, 2017

    some comments on Aviv Palivoda's patch.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Jan 20, 2017

    Uploading a new patch with fixes from Ma Lin comments.
    Two points:

    1. Should we add the VACUUM with a explicit commit? Maybe there should be an implicit commit before VACUUM?
    2. Should a SELECT start a transaction? I think it should according to PEP-249. There is a open issue on the case (bpo-9924). Should we just change this on this patch?

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Jan 23, 2017

    If the answer is (no, no) , the behavior strictly follows the doc changes in commit 284676cf2ac8.
    Anyway, I'm not a deep user of SQLite, I can't give further advices. :(

    @berkerpeksag
    Copy link
    Member

    1. Should we add the VACUUM with a explicit commit? Maybe there should
      be an implicit commit before VACUUM?

    VACUUM is often an expensive operation so I think people should need to explicitly handle it anyway.

    1. Should a SELECT start a transaction? I think it should according to
      PEP-249. There is a open issue on the case (bpo-9924). Should we just
      change this on this patch?

    Let's discuss that in bpo-9924 :) Last time I look at it there was some backward compatibility concern so we might able to fix it only in 3.7.

    @palaviv
    Copy link
    Mannequin

    palaviv mannequin commented Jan 23, 2017

    Removed opening a transaction on select. I will argue for that in bpo-9924 after this is resolved.

    @CdricBellegarde
    Copy link
    Mannequin

    CdricBellegarde mannequin commented Jan 26, 2017

    Adding a commit doesn't fix the issue here:
    sql.commit()
    sql.execute('VACUUM')

    And on run:
    cannot VACUUM from within a transaction

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Jan 26, 2017

    You can set isolation_level = None in sqlite3.connect() parameters, then sqlite3 module will not begin a transaction implicitly.

    Another way is disabling auto-begin-transaction temporarily:
    sql.isolation_level = None
    sql.execute('VACUUM')
    sql.isolation_level = '' # <- note that this is the default value of isolation_level

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Jan 31, 2017

    Let me give a summary.

    Before 3.6.0 (before 284676cf2ac8):
    implicit begin: st in (INSERT, UPDATE, DELETE, REPLACE)
    implicit commit: st not in (SELECT, INSERT, UPDATE, DELETE, REPLACE)

    In 3.6.0 (after 284676cf2ac8):
    implicit begin: (not sqlite3_stmt_readonly(st)) and (not in (CREATE, DROP, REINDEX))
    implicit commit: no

    With Palivoda's sqlite-ddl-dml-3.patch:
    implicit begin: st in (INSERT, UPDATE, DELETE, REPLACE)
    implicit commit: no
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~

    Some possible subsequent steps base on this issue:

    1, remove sqlite3_stmt_readonly(), see bpo-29355.
    Base on sqlite-ddl-dml-3.patch, only change a single line of code:
    + if (self->is_dml) {

    • if (!sqlite3_stmt_readonly(self->statement->st)) {
      Then restore the behavior before 3.6.0, this is even better than using sqlite3_stmt_readonly().

    2, enhance backward compatibility.
    Please read msg284585, msg286217, msg286293 in a row.
    If we implicitly commit before executing some statements (and print a warning), we can improve compatibility largely, the cost is almost free.

    3, docments enhancement.
    see bpo-8145, bpo-29121.

    @berkerpeksag
    Copy link
    Member

    Thank you everyone! This should be fixed now.

    @berkerpeksag
    Copy link
    Member

    New changeset 76995ca by Berker Peksag in branch '3.6':
    bpo-28518: Start a transaction implicitly before a DML statement (#245) (#318)
    76995ca

    @berkerpeksag
    Copy link
    Member

    New changeset 4a926ca by Berker Peksag in branch 'master':
    bpo-28518: Start a transaction implicitly before a DML statement (#245)
    4a926ca

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants