Repository navigation
sqlite3 module breaks transactions and potentially corrupts data #54949
Description
| BPO | 10740 |
|---|---|
| Nosy | @pitrou, @ned-deily, @bitdancer, @asvetlov, @florentx, @dholth, @Bluehorn, @RonnyPfannschmidt, @berkerpeksag, @JimJJewett, @zooba, @animalize, @jan-hudec, @carlwgeorge, @maggyero |
| Files |
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 2016-09-11.11:56:25.732>
created_at = <Date 2010-12-20.18:02:11.675>
labels = ['type-feature', 'library']
title = 'sqlite3 module breaks transactions and potentially corrupts data'
updated_at = <Date 2020-01-26.19:27:00.405>
user = 'https://bugs.python.org/scotturban'bugs.python.org fields:
activity = <Date 2020-01-26.19:27:00.405>
actor = 'maggyero'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-11.11:56:25.732>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2010-12-20.18:02:11.675>
creator = 'scott.urban'
dependencies = []
files = ['20118', '20119', '21417', '21418', '21419', '21479', '35498', '35780', '40252', '42251', '42316', '44437', '45949']
hgrepos = []
issue_num = 10740
keywords = ['patch']
message_count = 69.0
messages = ['124392', '124393', '124398', '124407', '128815', '132285', '132287', '132414', '132470', '132612', '136362', '144197', '207789', '207796', '207800', '209900', '209901', '209904', '209917', '209924', '216715', '216732', '216734', '216738', '216740', '219757', '219758', '219759', '219761', '219806', '219818', '219884', '220478', '220491', '220500', '220501', '220519', '220533', '220555', '220566', '220602', '220615', '221568', '248816', '248830', '248833', '248839', '248915', '248917', '249111', '262241', '262254', '262268', '262280', '262281', '262573', '274810', '274816', '274935', '275760', '283528', '283529', '283533', '283919', '283921', '360665', '360703', '360729', '360733']
nosy_count = 26.0
nosy_names = ['ghaering', 'pitrou', 'ned.deily', 'Jeremy Banks', 'r.david.murray', 'zzzeek', 'asvetlov', 'flox', 'adamtj', 'dholth', 'torsten', 'monsanto', 'scott.urban', 'aymeric.augustin', 'tshepang', 'python-dev', 'Ronny.Pfannschmidt', 'berker.peksag', 'Mark.Bucciarelli', 'Jim.Jewett', 'steve.dower', 'malin', 'bulb', 'bkircher', 'carlwgeorge', 'maggyero']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10740'
versions = ['Python 3.6']
Activity
scotturban commentedon Dec 20, 2010
The python sqlite module automatically commits open transactions
when it encounters a DDL statement. This is unnecessary; DDL is
transactional in my testing (see attached).
Attached patch addresses the issue. Patch is against 2.6.1, but
looking at Trunk in svn, it seems like the patch is needed and
would apply. One issue I could foresee is that this behavior might
depend on the sqlite version in use (I'm on 3.6.10).
Patch also allows pragma statement.
scotturban commentedon Dec 20, 2010
Here are some tests.
bitdancer commentedon Dec 20, 2010
See also bpo-8145. It would be nice if someone could sort all this out, but I'm not knowledgeable enough to do so.
For this patch, it would be a significant change it behaviour. Therefore it would have to be a new feature controlled by a flag of some sort (which is part of the reason for the reference to bpo-8145).
scotturban commentedon Dec 20, 2010
I find the way that the sqlite3 module handles transactions pretty
surprising in general, but I agree that someone who got used
to DDL not rolling back could in theory find this patch surprising.
We will apply this patch to our python build because having DDL
covered by a transactions is convenient for certain tasks. If anyone
can think of a problem with this simple patch, I'd like to hear it.
Thanks
Scott
dholth commentedon Feb 18, 2011
I want transactional DDL too. I was tremendously surprised that I could not duplicate the way sqlite3 behaves on the command line from witin pysqlite.
Instead of this patch, I would rather be able to instruct pysqlite to always begin a transaction for any kind of statement (I hear this is a requirement for DB-API compliance) and never perform an implicit commit for any reason. For example, someone on the google code project had a complaint that 'SAVEPOINT' (create a subtransaction) automatically commits because pysqlite doesn't know about it.
I tried to trick pysqlite into doing what I wanted by prepending /* update */ to every CREATE TABLE statement but it didn't seem to quite work. Instead, I would prefer a pysqlite that does not strcmp(statement) at all.
Bluehorn commentedon Mar 27, 2011
The attached patch is my take on this issue. I ran into the problem that during schema upgrades dropping a table was not rolled back. In another instance, renaming a table was not rolled back. This greatly increases the risk of data loss for our application.
Because I do not currently foresee which commands might need dropping out of a transaction and because of backwards compatibility woes, I added a new field to the sqlite3.Connection: operation_needs_transaction_callback
This function is called to decide if a running transaction should be implicitly committed (I'd consider this dangerous), if a transaction has to be started if not running (should normally always hold) or if the transaction state should be left alone.
For example, the "pragma foreign_keys = ..." is a no-op inside a transaction, therefore an implicit begin would be possibly harmful. In our application, we enable foreign keys when getting the connection out of the SQLAlchemy pool via a pool listener, which would be disabled if there is an implicit begin triggered.
The patch also adds a bunch of unit tests to cover the new behaviour.
Bluehorn commentedon Mar 27, 2011
Same patch for Python 3. In fact, this also adds a missing Py_XDECREF.
dholth commentedon Mar 28, 2011
Torsten basically you are suggesting that PRAGMA would never work at all with my 'do not strcmp() the sql at all, always begin a transaction' approach?
Bluehorn commentedon Mar 29, 2011
No. Most pragmas should still work and getting the current setting of a pragma should also work.
I was only referring to http://www.sqlite.org/pragma.html#pragma_foreign_keys
Quote:
I did not see such a restriction for other pragmas though I admit that I did not reread the list in full.
Bluehorn commentedon Mar 30, 2011
The attached patch is an updated version which adds a bit of documentation.
Bluehorn commentedon May 20, 2011
I updated the patch for upstream pysqlite2. Available at
http://code.google.com/p/pysqlite/issues/detail?id=24
Patch over there is for Python 2 (tested with our production Python 2.6).
52 remaining items
aymericaugustin commentedon Sep 7, 2016
The latest patch removes the current statement parsing and unexpected implicit commits. It looks good to me.
Unfortunately it also introduces a new kind of statement parsing that detects DDL statements and doesn't open a transaction in that case, while it should. See ghaering/pysqlite#105 for details.
As a consequence, this change will allow Django to remove one of the two special cases for transaction in SQLite:
https://github.com/django/django/blob/master/django/db/transaction.py#L159-L166
but not the other:
https://github.com/django/django/blob/271581df606b307d89c141e8b1a50ace763bea81/django/db/backends/base/base.py#L375-L404
It's still a step forward so I support merging it.
zooba commentedon Sep 8, 2016
I'm in favor of merging Berker's patch, once the string comparison is made a little more robust. Right now there's a risk of matching a prefix rather than a whole word, and potentially overrunning the buffer (unless I've forgotten how stricmp works).
python-dev commentedon Sep 11, 2016
New changeset 284676cf2ac8 by Berker Peksag in branch 'default':
Issue bpo-10740: sqlite3 no longer implicitly commit an open transaction before DDL statements
https://hg.python.org/cpython/rev/284676cf2ac8
animalize commentedon Dec 18, 2016
I'm using Python 3.6.0 RC2, I don't know if it's a bug.
When I try to run VACUUM command, an exception raised:
sqlite3.OperationalError: cannot VACUUM from within a transaction
~~~~~~~~~~~~~~~~~~~
On Python 3.5, everything is fine.
Attached file cant_vacuum.py is the full example code.
bitdancer commentedon Dec 18, 2016
It is an unexpected change in behavior and is therefore probably a bug. It will work if you set isolation_level=None. Because of that I don't think it is a release critical bug, though one could wish we'd discovered it earlier. Please open a new issue for this.
animalize commentedon Dec 18, 2016
bpo-29003 created
carlwgeorge commentedon Dec 24, 2016
While attempting to build a Python 3.6 RPM for RHEL/CentOS 6, I noticed the following warning.
*** WARNING: renaming "_sqlite3" since importing it failed: build/lib.linux-i686-3.6-pydebug/_sqlite3.cpython-36dm-i386-linux-gnu.so: undefined symbol: sqlite3_stmt_readonly
The resolution of this issue introduced usage of the sqlite3_stmt_readonly interface. That interface wasn't added to sqlite until 3.7.4 (http://www.sqlite.org/releaselog/3_7_4.html). My RPM build failed because RHEL/CentOS 6 only has sqlite 3.6.20. I understand that Python can't support old libraries forever, but can this minimum sqlite version be noted somewhere in the documentation?
bitdancer commentedon Dec 24, 2016
Please open a new issue for this request.
maggyero commentedon Jan 25, 2020
With Berker Peksag's patch merged in 2016, in the default non auto-commit mode, sqlite3 implicitly issues a BEGIN statement before any non SELECT statement not already in a transaction, EXCEPT DDL statements, for backwards compatibility reasons:
+ /* For backwards compatibility reasons, do not start a transaction if a
+ DDL statement is encountered. If anybody wants transactional DDL,
+ they can issue a BEGIN statement manually. */
+ 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);
}
}
Is there any plan to cover DDL statements as well in a future Python version and deprecate the old behaviour? That would avoid having to insert BEGIN statements manually for getting transactional DDL statements, like in psycopg2 for PostgreSQL databases.
bitdancer commentedon Jan 25, 2020
Please open a new issue for this question.
maggyero commentedon Jan 26, 2020
@Aymeric Augustin
+1. We could use this new autocommit property to enable the new full transactional mode (that is to say with transactional DDL):
To transition from the old partial transactional mode (without transactional DDL) by default to the new full transactional mode (with transactional DDL) by default, we could use the following migration strategy:
maggyero commentedon Jan 26, 2020
@r. David Murray
Done here: https://bugs.python.org/issue39457