msg283569 - (view) |
Author: Armin Rigo (arigo) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2017-01-14 07:02 |
Hoping to tag in less than 48 hours...!
|
msg285470 - (view) |
Author: Armin Rigo (arigo) * |
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
|
msg367371 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2020-04-27 03:35 |
As far as I can tell from the commit history associated with this issue this seems to have not been a bug, so I'm closing it. If anyone disagrees, please reopen :)
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:40 | admin | set | github: 73192 |
2020-04-27 03:35:58 | zach.ware | set | status: open -> closed
nosy:
+ zach.ware messages:
+ msg367371
resolution: not a bug stage: needs patch -> resolved |
2017-01-16 08:07:37 | python-dev | set | messages:
+ msg285543 |
2017-01-14 08:50:29 | arigo | set | messages:
+ msg285470 |
2017-01-14 07:02:13 | larry | set | messages:
+ msg285461 |
2017-01-13 11:17:07 | arigo | set | messages:
+ msg285384 |
2017-01-13 10:36:07 | arigo | set | messages:
+ msg285380 |
2017-01-13 10:25:52 | arigo | set | messages:
+ msg285376 |
2017-01-12 08:36:14 | Gian-Carlo Pascutto | set | nosy:
+ Gian-Carlo Pascutto messages:
+ msg285295
|
2017-01-12 07:40:47 | python-dev | set | nosy:
+ python-dev messages:
+ msg285292
|
2017-01-11 10:23:31 | arigo | set | messages:
+ msg285207 |
2017-01-11 05:37:43 | larry | set | messages:
+ msg285188 |
2017-01-07 16:06:18 | palaviv | set | nosy:
+ palaviv
|
2017-01-06 21:22:11 | larry | set | messages:
+ msg284852 |
2017-01-02 09:37:02 | lemburg | set | nosy:
+ lemburg messages:
+ msg284478
|
2017-01-01 23:18:16 | larry | set | nosy:
+ larry
|
2017-01-01 23:09:50 | berker.peksag | set | versions:
+ Python 3.5, Python 3.6, Python 3.7 messages:
+ msg284435
components:
+ Library (Lib) type: behavior stage: needs patch |
2016-12-20 09:26:54 | arigo | set | messages:
+ msg283675 |
2016-12-18 22:10:22 | arigo | set | messages:
+ msg283570 |
2016-12-18 21:20:40 | ned.deily | set | nosy:
+ ghaering, benjamin.peterson, berker.peksag
|
2016-12-18 21:18:59 | arigo | create | |