python
gh-83638: Add sqlite3.Connection.autocommit for PEP 249 compliant behaviour
Resolves #83638
TODO
- [x] Wait for PEP-249 amendments to land
- [x] Reflow and reindent doc changes
- Issue: gh-83638
@zzzeek, @maggyero, I'd appreciate your review on this, if only by building and testing it, and/or taking a look at the added tests.
For external reviewers, here's some help on how to get started reviewing this PR:
- Rationale / proposed solution
- First, read up on the discussions in the linked issue, and those linked from that issue
- Are there holes or inconsistencies in the proposed solution?
- Does the proposed solution actually solve the problem?
- Implementation
- Does the implementation align with the proposed solution?
- Does the implementation align with PEP 249?
- Are there logical inconsistencies in the implementation?
- Tests:
- Do the added tests test the actual implementation?
- Could the added tests be more specific?
- Are there missing corner cases?
- Are there superfluous tests?
- How is the code coverage? Are there uncovered functions/lines/branches?
- Docs:
- Are the added docs helpful for the average user? (Too technical? Missing examples?)
- Are the added docs aligned with the actual implementation
- Are there spelling/syntax errors?
- Is the NEWS entry understandable for the average user? Does it explain the change in a concise way?
- Can the What's New entry be improved?
- Backwards compatibility
- Does the proposed solution follow PEP 387?
- Could we add more emphasis on the deprecation notices?
I'm marking this as draft, as I want to land a couple of other cleanups to the sqlite3 docs before requesting further reviews.
I need to resolve #94538 in order to proceed with this PR.
Even though that PR has landed, more AC changes are required in order to optimise the relayed call from sqlite3.connect to sqlite3.Connection.__init__. Going for the slow path for now: Py_BuildValue and PyObject_Call.
Oh, is :pep:`249`-compliant invalid ReST markup? That's a shame, it's better grammatically :(
I believe the added docs are in a pretty good shape now. I'll try to get MAL to do a review of the semantics of setting the autocommit attribute.
@malemburg, there is one corner case that just came to mind. Consider the following code:
import sqlite3
cx = sqlite3.connect(":memory:", autocommit=False)
cx.execute("create table t(t unique)")
data = [(1,), (2,), (2,), (3,)]
try:
cx.executemany("insert or rollback into t values(?)", data)
except sqlite3.IntegrityError:
pass
assert cx.in_transaction is True # this will fail
We can guard against this by checking for SQLITE_CONSTRAINT result codes in _pysqlite_query_execute() and implicitly open a new transaction if the existing one was closed by the implicit rollback. But, I don't think it is worth it; I prefer to keep the autocommit implementation as straight-forward as possible. If users want to shoot themselves in the foot, the may just as well do so.
Taking this thought a little bit further; executing INSERT OR ROLLBACK while in autocommit=False mode makes as little sense as executing any other explicit transaction control in that mode. So, let's not guard against it.
All right, I think I'm ready to land this PR soon. I'll go over it a few times more myself the next days; will probably merge it on Sunday or Monday.
Taking this thought a little bit further; executing
INSERT OR ROLLBACKwhile inautocommit=Falsemode makes as little sense as executing any other explicit transaction control in that mode. So, let's not guard against it.
This is a general problem with exposing separate transaction APIs in DB-API modules: it is often still possible to use transaction statements in the executed SQL. Doing so will usually cause problems and is discouraged. There's nothing the modules can do and usually nothing the underlying database libraries can do either. The ODBC spec is full of warnings about this.
If programmers choose to mix the APIs with SQL transaction statements, they are on thin ice. We can warn about this, but that's it.
BTW: I have started the Connection.autocommit discussion on DB-SIG. We'll have to see where things land, but there is concern about making the attribute writable. The main concern is that this may result in a network roundtrip, which would be impossible to handle in an async implementation (you can only await co-routines, not attribute access).
This is not relevant for SQLite (there is no network I/O to consider), but we may end up adding a new standard extension method e.g. .set_autocommit() to adjust the setting and only document the attribute .autocommit for querying the current state, leaving the writable semantics intentionally undocumented.
BTW: I have started the
Connection.autocommitdiscussion on DB-SIG. We'll have to see where things land, but there is concern about making the attribute writable. The main concern is that this may result in a network roundtrip, which would be impossible to handle in an async implementation (you can only await co-routines, not attribute access).This is not relevant for SQLite (there is no network I/O to consider), but we may end up adding a new standard extension method e.g.
.set_autocommit()to adjust the setting and only document the attribute.autocommitfor querying the current state, leaving the writable semantics intentionally undocumented.
Thanks, yes I'm following the discussion. I'll hold this PR a little bit further, in case the autocommit attribute ends up being read-only in PEP-249. We're far from the feature freeze, so I have no problems with leaving this open until things are settled. OTOH, we can of course merge now and just make the needed adaptions, as long as we are in the alpha phase.
If programmers choose to mix the APIs with SQL transaction statements, they are on thin ice. We can warn about this, but that's it.
+1 -- I think we should warn about this in the docs. (I'll save that for another PR.)
Would a real warning be feasible? E.g. checking the input SQL queries for the ROLLBACK, BEGIN, COMMIT, etc keywords (as relevant)? Or would that be either out of scope or not practical here?
Would a real warning be feasible? E.g. checking the input SQL queries for the
ROLLBACK,BEGIN,COMMIT, etc keywords (as relevant)? Or would that be either out of scope or not practical here?
I consider it not practical. The SQL dialect of SQLite is an evolving thing; there would always be a chance that we'd not catch a future variant of a transaction control query. Also, I really don't want to add an SQL parser to the sqlite3 module (added complexity, performance penalty, ...).
@CAM-Gerlach, thanks for your thorough review. I agree with most of your remarks. I agree it is a good thing to update the tutorial and the doctests according to the new recommendations, but I'll save that for a future PR (in time for 3.12 release of course).
As I mentioned in an earlier comment (and now on the issue), I don't think it is wise to deprecate anything (related to this feature) in 3.12. I've removed deprecation notices from the docs in this PR.
Additional comments, some of them questions on points of additional confusion, some of them things you probably want to clarify, some of them things I missed in making my suggestions or you missed in manually applying them last time, and a few other follow ups.
I probably missed some of them. Thanks for taking the time to go through all of this again!
- [ ] As I mentioned in my (seemingly lost, and now re-created) reply, I suggest
LEGACY_TRANSACTION_CONTROLfor the new name of the constant.
Yep, I agree that LEGACY_TRANSACTION_CONTROL is a lot clearer than my suggestion. I'll address it in one of the upcoming commits.
Also, I don't understand where the "AL" in "TRANSACTIONAL" came from
Hm, probably a linguistic glitch from my side, as I'm not a native speaker. Sorry 'bout that.
I'll have a look at your other comments and push an update, hopefully today. Thanks again for helping out with this; the docs are the hardest part of this PR (the code is pretty straight-forward).
@CAM-Gerlach, I think I've addressed most of your second review. I've left some of the conversations open for further comments.
Thanks for the last round of comments, @CAM-Gerlach, very helpful!
BTW, I refactored the tests by adding a decorator that got rid of the repeated setup/boiler-plate code. However, I reverted it before pushing. Though the refactored version was more elegant and less verbose, I found I missed the verbosity when reading through the test suite. It was harder to see what was actually tested, and how it was tested. I see that a lot of tests in the stdlib are written out explicitly (with lots of repeated code), and perhaps that verbosity actually is a good thing.
Here's an excerpt of the diff
- def test_autocommit_disabled(self):
- expected = [
- "SELECT 1",
- "COMMIT",
- "BEGIN",
- "ROLLBACK",
- "BEGIN",
- ]
- with memory_database(autocommit=False) as cx:
- self.assertTrue(cx.in_transaction)
- with self.check_stmt_trace(cx, expected):
- cx.execute("SELECT 1")
- cx.commit()
- cx.rollback()
-
- def test_autocommit_disabled_implicit_rollback(self):
- expected = ["ROLLBACK"]
- with memory_database(autocommit=False) as cx:
- self.assertTrue(cx.in_transaction)
- with self.check_stmt_trace(cx, expected, reset=False):
- cx.close()
+ @autocommit(False, expected=["SELECT 1", "COMMIT", "BEGIN", "ROLLBACK", "BEGIN"])
+ def test_disabled(self, cx):
+ cx.execute("SELECT 1")
+ cx.commit()
+ cx.rollback()
- def test_autocommit_enabled(self):
- expected = ["SELECT 1"]
- with memory_database(autocommit=True) as cx:
- self.assertFalse(cx.in_transaction)
- with self.check_stmt_trace(cx, expected):
- cx.execute("SELECT 1")
- cx.commit() # expect this to pass silently
- self.assertFalse(cx.in_transaction)
+ @autocommit(False, expected=["ROLLBACK"])
+ def test_disabled_implicit_rollback(self, cx):
+ cx.close()
I have no strong opinion here. I'm fine with verbose tests, but I'm also fine with adding the decorator.
I have no strong opinion here. I'm fine with verbose tests, but I'm also fine with adding the decorator.
I defer to whatever you find best, as the maintainer of this module :)
FTR, it seems like the autocommit PEP 249 discussion on db-sig is landing. MAL is suggesting to deprecate writing to the autocommit attribute, but we can do that in a follow-up PR. There is plenty of time till the next alpha.
After resolving the last open conversation, I'll land this PR. Thanks a lot to everyone involved!
Hey @erlend-aasland , just making sure—seems like at least one, and maybe two of the suggestions you intended to apply either manually or automatically aren't yet on this PR. Do you make them locally and haven't pushed yet, or was that an oversight?
Hey @erlend-aasland , just making sure—seems like at least one, and maybe two of the suggestions you intended to apply either manually or automatically aren't yet on this PR. Do you make them locally and haven't pushed yet, or was that an oversight?
Deliberately not pushed; I'd made most of the changes, except for the last (until recently) unresolved conversation, as my proposed change was part of my local branch.
Thanks so much for the hard work @erlend-aasland of implementing my feature request! I am going to make a quick review today, so can you hold the merge a little bit more?
Deliberately not pushed; I'd made most of the changes, except for the last (until recently) unresolved conversation, as my proposed change was part of my local branch.
I had figured that, up until you then applied one of the suggestions to the remote branch, which combined with your statement made me a little unsure so I wanted to double-check (since that would mean your local unpushed changes and the remote branch would be both out of sync, in addition to applying GitHub suggestions directly being a little out of character for you :joy: )