-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
Comments
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. |
Looks like commit 284676cf2ac8 in bpo-10740 introduces this. Haven't read through the thread yet. |
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: |
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. |
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. |
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? |
Also, if there is indeed a suspected error in the underlying library, has the issue been reported to the SQLite project? |
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 :(. |
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.) |
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 For us, 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. |
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 :) |
There are already a few test's that start a transaction explicitly: In dbapi.SqliteOnConflictTests CheckOnConflictRollbackWithExplicitTransaction, CheckOnConflictAbortRaisesWithExplicitTransactions which set 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. |
Here is a patch that allows executing "begin immediate" explicitly with any isolation_level. I don't know what behavior is more preferable. |
Comment kindly provided by D. Richard Hipp himself: I think this is a error in sqlite as the documentation says: Except it is not a bug in SQLite, but rather an ambiguity in the For a plain BEGIN statement, there are no changes to the database So technically, the documentation is correct, though I can certainly Note also that behavior of sqlite3_stmt_readonly() has not changed -- |
rewording of sqlite documentation: |
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. |
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. |
Here is a little simplified version. No need to handle a single "begin". |
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 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 Thank you for your efforts! regards. |
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. |
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). |
I'm trying to catch up with you. if conn.in_transaction:
conn.commit()
conn.execute("vacuum") |
Yes. If a transaction is open you will need to explicitly commit before doing vacuum.
All of the above worked with implicit commit before commit 284676cf2ac8 but now has to call commit explicitly. |
some comments on Aviv Palivoda's patch. |
Uploading a new patch with fixes from Ma Lin comments. |
If the answer is (no, no) , the behavior strictly follows the doc changes in commit 284676cf2ac8. |
VACUUM is often an expensive operation so I think people should need to explicitly handle it anyway.
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. |
Removed opening a transaction on select. I will argue for that in bpo-9924 after this is resolved. |
Adding a commit doesn't fix the issue here: And on run: |
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: |
Let me give a summary. Before 3.6.0 (before 284676cf2ac8): In 3.6.0 (after 284676cf2ac8): With Palivoda's sqlite-ddl-dml-3.patch: Some possible subsequent steps base on this issue: 1, remove sqlite3_stmt_readonly(), see bpo-29355.
2, enhance backward compatibility. |
Thank you everyone! This should be fixed now. |
Misc/NEWS
so that it is managed by towncrier #552Note: 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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: