Repository navigation
execute("begin immediate") throwing OperationalError #72704
Description
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']
Activity
fschulze commentedon Oct 24, 2016
Using:
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.
zhangyangyu commentedon Oct 24, 2016
Looks like commit 284676cf2ac8 in bpo-10740 introduces this. Haven't read through the thread yet.
palaviv commentedon 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):
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 commentedon 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 commentedon Nov 24, 2016
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 commentedon Nov 24, 2016
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 commentedon Nov 24, 2016
Also, if there is indeed a suspected error in the underlying library, has the issue been reported to the SQLite project?
palaviv commentedon 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 commentedon Nov 25, 2016
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 commentedon 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 = 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.
berkerpeksag commentedon Nov 25, 2016
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 commentedon 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:
palaviv commentedon 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:
All of the above worked with implicit commit before commit 284676cf2ac8 but now has to call commit explicitly.
animalize commentedon Jan 17, 2017
some comments on Aviv Palivoda's patch.
palaviv commentedon Jan 20, 2017
Uploading a new patch with fixes from Ma Lin comments.
Two points:
animalize commentedon 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 commentedon Jan 23, 2017
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.
palaviv commentedon Jan 23, 2017
Removed opening a transaction on select. I will argue for that in bpo-9924 after this is resolved.
CdricBellegarde commentedon 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 commentedon 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 commentedon 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) {
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 commentedon Feb 26, 2017
Thank you everyone! This should be fixed now.
berkerpeksag commentedon Mar 24, 2017
New changeset 76995ca by Berker Peksag in branch '3.6':
bpo-28518: Start a transaction implicitly before a DML statement (#245) (#318)
76995ca
berkerpeksag commentedon Mar 24, 2017
New changeset 4a926ca by Berker Peksag in branch 'master':
bpo-28518: Start a transaction implicitly before a DML statement (#245)
4a926ca