Repository navigation
execute("begin immediate") throwing OperationalError #72704
Description
Activity
fschulze commented
on Oct 24, 2016 fschulzemannequinMannequinAuthorUsing:
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.
palaviv commented
on Oct 31, 2016 palavivmannequinMannequinI 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 commented
on Nov 24, 2016 fschulzemannequinMannequinAuthorHow 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?
palaviv commented
on Nov 25, 2016 palavivmannequinMannequinI 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.)
fschulze commented
on Nov 25, 2016 fschulzemannequinMannequinAuthorOk, 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 = Noneturns on "autocommit mode". Unfortunately I don't have suggestions on how to improve the documentation.For us,
isolation_level = Noneand the explicitbegin immediateseems 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.
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 :)
12 remaining items
animalize commented
on Jan 3, 2017 animalizemannequinMannequinI'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 commented
on Jan 3, 2017 palavivmannequinMannequinYes. 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:- VACUUM
- ATTACH
- DETACH
- BEGIN
- Few PRAGMAs (temp_store, synchronous)
All of the above worked with implicit commit before commit 284676cf2ac8 but now has to call commit explicitly.
animalize commented
on Jan 17, 2017 animalizemannequinMannequinsome comments on Aviv Palivoda's patch.
palaviv commented
on Jan 20, 2017 palavivmannequinMannequinUploading a new patch with fixes from Ma Lin comments.
Two points:animalize commented
on Jan 23, 2017 animalizemannequinMannequinIf 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. :(- 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.
- 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.
- Should we add the VACUUM with a explicit commit? Maybe there should
palaviv commented
on Jan 23, 2017 palavivmannequinMannequinRemoved opening a transaction on select. I will argue for that in bpo-9924 after this is resolved.
CdricBellegarde commented
on Jan 26, 2017 CdricBellegardemannequinMannequinAdding a commit doesn't fix the issue here:
sql.commit()
sql.execute('VACUUM')And on run:
cannot VACUUM from within a transactionanimalize commented
on Jan 26, 2017 animalizemannequinMannequinYou 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_levelanimalize commented
on Jan 31, 2017 animalizemannequinMannequinLet 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: noWith 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.- if (!sqlite3_stmt_readonly(self->statement->st)) {
Thank you everyone! This should be fixed now.
Misc/NEWSso 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: