TL;DR: Should we deprecate the sqlite3 context manager? Should we introduce a new SAVEPOINT based context manager?
The sqlite3 context manager has been the source of confusion for years[1].
Many people have assumed that __enter__ created a new transaction, and that __exit__ always committed/rolled back. This is not the case. A new transaction is not implicitly started, and changes are only implicitly committed/rolled back, if there is an open transaction in __exit__.
In order to try to reduce the confusion, we recently updated the docs[2], however other solutions was also discussed on the issue:
Deprecating the current context manager; this got a +1 from the original pysqlite author, Gerhard.
Adding a better context manager, based on SAVEPOINT/RELEASE.
Iām +1 to both ideas, but I do not think we should remove the current context manager. One nice thing with a SAVEPOINT based context manager, is that it would be possible to nest it.
If that deprecation is in favor of explicit API(s) offering a context manager(s) whoās expected behaviors are self explanatory from their API name that seems like a good idea.
Adding new context managers that do what people seem to actually need/want seems good, but should be done via explicit APIs. Not by replacing the old context manager behavior after the deprecation cycle.
with dbthingy.savepoint_context() as db_savepoint:
...
with dbthingy.new_transaction_context() as db_txn:
...
for example may be clear to users. (Donāt blindly take my naming advice there, those are just examples to see the explicit is better than implicit idea)
Outright replacing the existing proposed-deprecated context manager behavior with another behavior is a problem because code will exist that needs to be compatible across all versions and this scenario will lead to a cognitive burden making sense of code. It is better to require explicitness as the realization that the non-explicit CM behavior was surprising could wind applying to any chosen new behavior as well.
savepoint_context() should be pretty self-explanatory. Possibly also simply savepoint(), but I would prefer the former bco. the explicitness.
Yep. I think altering the behaviour of the existing context manager is a no-go. Just deprecate it and donāt touch it anymore.
IMO, the timeline of posts in the linked issue shows that the need is there; the issue has been alive since 2013, with the latest post arriving just before the bpo migration.
It doesnāt need to be a context manager, but a properly implemented context manager could help reduce boilerplate code when dealing with transactions. Using context managers to manage resources is a well known idiom. Extending that idiom to include database transaction handling may make sense[1]:
open a transaction
execute a series of database statements
commit if no problems occurred; roll back if not
Instead of writing a try...except...else where you explicitly roll back and commit, you could reduce boilerplate code by using a transaction manager.
If we are to provide a savepoint context manager, there is one issue to resolve, apart from the API name: should sqlite3 automatically create savepoint names, should we require the user to supply savepoint names, or should we allow a mix?
Of course, the context manager is better in many cases. I was suggesting an alternate way of working with savepoints.
Sorry for not being clear.
I canāt see a way names to be helpful with a context manager (or method) API. It would allow mixing up which savepoint refers to what, which might get confusing. (And you can always execute a SAVEPOINT command manually if you donāt want contexts.)
One exception is debugging. It might be useful to specify a prefix like in tempfile, so a nice name shows up in logs.
Good points. I guess there is no perfect path here.
As for an alternative way of working with savepoints, Iād recommend just executing them manually instead of adding a non-context-manager wrapper for them.
IMO, weāre best off by keeping the API simple: sqlite3 automatically creates savepoint names. If, in the future, a need for custom savepoint names arise, we can add that to the existing API.
As a happy user of the existing context manager that would never even contemplate turning on auto commit mode or mixing schema changes with my regular runtime SQL, Iād prefer not to see my existing code broken by a programmatic deprecation warning (it isnāt clear from the above if it is programmatic or documented deprecation that is being proposed).
Adding new & improved APIs would make sense though.
If feasible it would also be nice to programmatically deprecate the cases involving implicit commits, but Iām not sure if the relevant state lives in the Python code or the underlying C library, so Iām not sure how practical that would be.
Given that the existing context manager only breaks if you misuse it, Iād be actively opposed to breaking working code for no good reason.
Making the existing context manager break more obviously in the cases that are broken anyway would make a lot of sense though (hence the suggestion of detecting those cases, and emitting first a deprecation warning, and eventually an actual error).
This is the standard PEP 249 (DB-API 2.0) logic, so itās not surprising that people expect this behavior. However, database modules are free to implement the details in different ways, as long as the main behavior of the context manager is implemented (ie. an exception causes everything done in the context manager to be rolled back; no exception causes everything to be committed).
I donāt think deprecating the logic is a good idea, since that would mean deprecating part of the PEP 249 standard implemented by the sqlite3 module.
I also donāt understand what the problem is with sqlite3 not implicitly starting a transaction upon __enter__ or not committing anything in __exit__ when there is nothing to commit (there is no open transaction). As long as everything happening within the context manager behaves as expected, the logic used by sqlite3 seems fine w/r to PEP 249. I donāt know the sqlite3 enough to be able to tell, whether this is the case or not.
Adding new context managers is always an option, e.g. the one based on SAVEPOINTs, but at the same time, those are not PEP 249 standards, so implementations would diverge from the standard and make it harder to change to other DB-API backends.
I agree. I have also used the context manager with no problems for years.
Yes, I understand that in more advanced cases it may be confusing, and if we could fix those cases (or even just warn/error if we see them happening) that would be good. But itās a bit extreme to break working code just because people doing something more advanced can make mistakes misinterpreting the behaviour.
Thanks for your input, everyone! FYI, Iāve already addressed the confusing (for some) behaviour by trying to make the docs more explicit and clear. Hopefully, clearer docs will adjust peopleās expectations, and as long as the observed behaviour aligns with the expectations, we are all good.
I think this thread shows that there is no concensus regarding the two proposed changes. For the record, Iām fine with leaving things as they are, regarding the context manager [1]
Regarding expectations and observed behaviour; thereās an open PR regarding improving the clarity of the sqlite3 extension moduleās implicit transaction handling, and how it deviates from PEP 249. Feel free to take a look:
