classification
Title: 2.7.13 _sqlite more prone to "database table is locked"
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Gian-Carlo Pascutto, arigo, benjamin.peterson, berker.peksag, ghaering, larry, lemburg, palaviv, python-dev
Priority: normal Keywords:

Created on 2016-12-18 21:18 by arigo, last changed 2017-01-16 08:07 by python-dev.

Messages (16)
msg283569 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2016-12-18 21:18
2.7.13 did a small change to the sqlite commit() method, http://bugs.python.org/issue10513, which I think is causing troubles.  I noticed the problem in PyPy, which (with the same change) fails another test in Lib/sqlite3/test/regression.py, CheckTypeMapUsage(), containing this code:

        ...
        con.execute(SELECT)
        con.execute("drop table foo")
        ...

The first execute() method creates a cursor; assuming it is not promptly deleted, its mere existence causes the second execute() method to fail inside Sqlite with "OperationalError: database table is locked".  As a second step, I could reproduce the problem in CPython by changing the test like this:

        ...
        keepalive = con.execute(SELECT)    # the cursor stays alive
        con.execute("drop table foo")
        ...

The reason is that in the commit() done by the second execute(), we no longer reset all this connection's cursors before proceeding.  But depending on the operation, Sqlite may then complain that the "table is locked" by these old cursors.  In other words, this new situation introduced in 2.7.13 potentially makes a few complicated cases crash by "table is locked" on CPython, where they would work fine previously---which is bad IMHO.  About PyPy, many more cases would crash, to the point that we may have no choice but not implement this 2.7.13 change at all.
msg283570 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2016-12-18 22:10
Or maybe it would be enough to change commit() so that if Sqlite fails with "table is locked", pysqlite would reset all cursors and then try again?
msg283675 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2016-12-20 09:26
Tried that, but reverted because on Windows CheckTypeMapUsage() would fail with SQLITE_MISUSE ("ProgrammingError: database table is locked").  For now PyPy will not implement this 2.7.13 change.  I really suspect you can get the same problems on CPython in some cases, as described.
msg284435 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-01-01 23:09
Thanks for the report. I can also reproduce it with 3.5+. We may want to revert 030e100f048a for the next 2.7 and 3.5 releases if we can't come up with a solution.
msg284478 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2017-01-02 09:37
As general comment, I think you'd have to check which operation modes SQLite supports for the case of a transaction commit/rollback in the light of open cursors.

ODBC defines the following cases and each data source can specify a different behavior (https://msdn.microsoft.com/en-us/library/ms711769%28v=vs.85%29.aspx):

 * All cursors are closed, and access plans for prepared statements on that connection are deleted.

 * All cursors are closed, and access plans for prepared statements on that connection remain intact.

 * All cursors remain open, and access plans for prepared statements on that connection remain intact.

The Python DB-API does not specify any particular behavior (since this depends on the database backend), so I guess pysqlite3 should implement whatever SQLite supports per default in such cases (or make this configurable).

BTW: It is quite common for databases to support the second mode of operation:

 * All cursors are closed, and access plans for prepared statements on that connection remain intact.

since this allows pooling cursors to cache often used access plans.
msg284852 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-06 21:22
FYI I'm keeping an eye on this for possible cherry-picking into 3.5.3 final, depending on the resolution.  Reverting 030e100f048a work for me, assuming that's a reasonable solution.
msg285188 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-11 05:37
Ping.  Hoping to resolve this in time for 3.5.3, which I tag in about four days.
msg285207 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2017-01-11 10:23
If I had a say, I would indeed revert 030e100f048a (2.7 branch) and 81f614dd8136 (3.5 branch) as well as forward-port the revert to 3.6 and trunk.  Then we wait for someone that really knows why the change was done in the first place.
msg285292 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-12 07:40
New changeset dd13098a5dc2 by Benjamin Peterson in branch '2.7':
revert 030e100f048a (#29006, #10513)
https://hg.python.org/cpython/rev/dd13098a5dc2
msg285295 - (view) Author: Gian-Carlo Pascutto (Gian-Carlo Pascutto) Date: 2017-01-12 08:36
>Then we wait for someone that really knows why the change was done in the first place.

Python 2.7 had a regression compared to 2.6 where a SELECT after a COMMIT would silently return the wrong data:

http://bugs.python.org/issue23129
http://bugs.python.org/issue10513

Neither exiting with a locked error nor producing the wrong data are particularly appealing results. Did 2.6 pass those sets that caused the attempted fix to get backed out?
msg285376 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2017-01-13 10:25
Gian-Carlo is right: I can modify the 2.6 tests in the same way as I described, and then I get the same error with python2.6.  So it seems that all of 2.6 was prone to the same issue, and it was never found, but went away in 2.7 accidentally.  That seems to mean that reverting 030e100f048a was not really necessary (beyond the issue with PyPy/Jython/IronPython).

According to http://bugs.python.org/issue23129 it was not a good idea to revert 030e100f048a.  But I'm surprized that the test added with 030e100f048a continues to pass with the revertion.  Maybe we should investigate why it does.  (Again, don't rely on me for that, because I don't know sqlite.)

If 030e100f048a stays in, I'll probably figure out a hack to avoid this pitfall with PyPy.
msg285380 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2017-01-13 10:36
...hah, of course the commit dd13098a5dc2 also reverted away the new test.  That test fails.  Sorry about that, and feel free to redo that commit.  It's just one more case in which the implicit refcounting is used, but I guess as it fixes a real issue it's a good idea anyway and should be fixed first in PyPy.
msg285384 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2017-01-13 11:17
Managed to write a patch in PyPy that seems to pass all tests including the new one, including on Windows.  I know think that dd13098a5dc2 should be backed out (i.e. 030e100f048a should be kept).

Reference to the PyPy changes: https://bitbucket.org/pypy/pypy/commits/235e8a3889790042b3f148bcf04891b27f97a1fc

Maybe something similar should be added to CPython, to avoid the unexpected "database table is locked" case; but such a change should probably be done only in trunk, because the <= 2.6 experience seems to suggest it is rare enough in practice.
msg285461 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2017-01-14 07:02
Hoping to tag in less than 48 hours...!
msg285470 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2017-01-14 08:50
larry: unless someone else comments, I think now that the current status of 3.5.3 is fine enough (nothing was done in this branch, and the problem I describe and just fixed in PyPy can be left for later).

The revert dd13098a5dc2 needs to be itself reverted in the 2.7 branch.
msg285543 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-16 08:07
New changeset 74a68d86569c by Benjamin Peterson in branch '2.7':
revert dd13098a5dc2 (#29006, #10513)
https://hg.python.org/cpython/rev/74a68d86569c
History
Date User Action Args
2017-01-16 08:07:37python-devsetmessages: + msg285543
2017-01-14 08:50:29arigosetmessages: + msg285470
2017-01-14 07:02:13larrysetmessages: + msg285461
2017-01-13 11:17:07arigosetmessages: + msg285384
2017-01-13 10:36:07arigosetmessages: + msg285380
2017-01-13 10:25:52arigosetmessages: + msg285376
2017-01-12 08:36:14Gian-Carlo Pascuttosetnosy: + Gian-Carlo Pascutto
messages: + msg285295
2017-01-12 07:40:47python-devsetnosy: + python-dev
messages: + msg285292
2017-01-11 10:23:31arigosetmessages: + msg285207
2017-01-11 05:37:43larrysetmessages: + msg285188
2017-01-07 16:06:18palavivsetnosy: + palaviv
2017-01-06 21:22:11larrysetmessages: + msg284852
2017-01-02 09:37:02lemburgsetnosy: + lemburg
messages: + msg284478
2017-01-01 23:18:16larrysetnosy: + larry
2017-01-01 23:09:50berker.peksagsetversions: + Python 3.5, Python 3.6, Python 3.7
messages: + msg284435

components: + Library (Lib)
type: behavior
stage: needs patch
2016-12-20 09:26:54arigosetmessages: + msg283675
2016-12-18 22:10:22arigosetmessages: + msg283570
2016-12-18 21:20:40ned.deilysetnosy: + ghaering, benjamin.peterson, berker.peksag
2016-12-18 21:18:59arigocreate