Deprecate the sqlite3 context manager and/or add a SAVEPOINT based context manager

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:

  1. Deprecating the current context manager; this got a +1 from the original pysqlite author, Gerhard.
  2. 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.

See also:


  1. See gh-61162: “The sqlite3 context manager does not work with isolation_level=None” ↩︎

  2. See gh-93890 ↩︎

2 Likes

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.

3 Likes

Well said.

+1

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.

3 Likes

Does it need to always be a context manager? Methods might be helpful if you aren’t using exceptions when deciding to rollback() vs. release(). e.g.

savepoint = dbthingy.savepoint()
savepoint.rollback()

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]:

  1. open a transaction
  2. execute a series of database statements
  3. 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.


  1. if implemented properly ↩︎

Though “with a savepoint, do that” also sounds good for me (with savepoint(): pass).

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.

2 Likes

No problem, you’re fine :slight_smile:

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.

2 Likes

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.

2 Likes

Sorry, I assumed deprecation implicitly meant both documented and programmatic deprecation. I’m proposing both kinds of deprecation.

I’ll certainly consider this, but in my opinion, it feels a bit strange to deprecate a feature only partially.

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).

2 Likes

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.

1 Like

PEP 249 does not mention context managers, AFAICS. I don’t know how other DB API backends implement context managers.

Noted.

That’s a good point.

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.

1 Like

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 :slight_smile: [1]


  1. I started out +1, but ended up +0. ↩︎

MySQL-python / mysqlclient / PyMySQL had context manager that manages transaction.

I had changed its semantics. Connection.__enter__ returns itself and Connection.__exit__ closes the connection for now.

This issue comment describes why I changed the semantics.

In short, many users expected Connection.__exit__ close the connection and implemented connection leak bugs.

1 Like

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:

Ha, good point. I stand corrected :slight_smile:

Looks like this should be added as another standard DB-API extension. Ditto for context managers closing cursors.

1 Like