The sqlite3 context manager does not work with isolation_level=None

[bitdancer]

BPO	16958
Nosy	@loewis, @bitdancer, @Kodiologist, @corona10, @coleifer, @erlend-aasland

^{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 = None
created_at = <Date 2013-01-14.00:46:44.408>
labels = ['extension-modules', 'type-bug', 'library']
title = 'The sqlite3 context manager does not work with isolation_level=None'
updated_at = <Date 2022-03-29.16:07:14.645>
user = 'https://github.com/bitdancer'

bugs.python.org fields:

activity = <Date 2022-03-29.16:07:14.645>
actor = 'corona10'
assignee = 'ghaering'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'Library (Lib)']
creation = <Date 2013-01-14.00:46:44.408>
creator = 'r.david.murray'
dependencies = []
files = []
hgrepos = []
issue_num = 16958
keywords = []
message_count = 8.0
messages = ['179909', '179912', '179913', '219866', '248822', '348633', '349507', '415999']
nosy_count = 9.0
nosy_names = ['loewis', 'ghaering', 'r.david.murray', 'nagylzs', 'aymeric.augustin', 'Kodiologist', 'corona10', 'coleifer', 'erlendaasland']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16958'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']

[bitdancer]

Its operation is also not particularly intuitive if isolation_level is not None, so its documentation needs some clarification.

Currently the transaction manager does nothing on enter, and does a commit or rollback on exit, depending on whether or not there was an exception inside the with block. With isolation_level set to None, the sqlite3 library is in autocommit mode, so changes will get committed immediately inside the with, which is simply broken.

If isolation_level is not None, then the behavior of the transaction manager depends heavily on what happens inside the with block. If the with block contains only the defined DQL statements (insert, update, delete, replace) and select statements, then things will work as expected. However, if another statement (such as a CREATE TABLE or a PRAGMA) is included in the with block, an intermediate commit will be done and a new transaction started.

I propose to do two things to fix this issue: explain the above in the transactions manager docs, and have the context manager check to see if we are in isolation_level None, and if so, issue a begin (and then document that as well).

One question is, can the fix be backported? It will change the behavior of code that doesn't throw an error, but most such code won't be doing what the author expected (run the with block inside a transaction...in pure autocommit mode the transaction manager is a no-op). One place code could break is if someone figured out this issue and worked around it by explicitly starting a transaction before (or after) entering the with block. In this case they would now get an error that a transaction cannot be started inside another. I would think this is unlikely...the more obvious workaround would be to write a custom transaction manager, so I suspect that that is what is actually in the field. But that's a (hopeful :) guess.

A fix for this problem would be to use 'savepoint' instead of 'begin' if the sqlite3 version supports it (it is apparently supported as of 3.6.8).

So, I'd like to see the fix, conditionally using SAVEPOINT, (once it written and tested) applied to all active python versions, but am open to the argument that it shouldn't be.

[loewis]

"changes will get committed immediately inside the with, which is simply broken"

What do you mean by that?
A. Changes ought to be committed immediately, but are not; it is broken, and changes must be committed immediately.

• or -
  B. What actually happens is that changes are committed immediately, and sqlite is incorrect in doing so.

Your discussion suggests B; in this case, I disagree that there is a bug. In auto-commit mode, it should really auto-commit, regardless of context managers. The context manager documentation doesn't claim otherwise.

[bitdancer]

B, yes.

So you would view the connection context manager acting as an actual transaction manager as a new feature? Would you be OK with adding that feature to the existing context manager in 3.4 (since currently the context manager is a noop in autocommit mode), or do you think we need to create a new context manager for this? Or do we do as the issue that sparked this (bpo-8145) suggested, and just document how to create your own?

[aymericaugustin]

• Thesis *

I belive that using the connection as a context manager is an inadequate API for controlling transactions because it's very likely to result in subtly broken code.

As a consequence, my recommendation would be to deprecate this API.

• Argumentation *

If you nest a naive transaction context manager (BEGIN / COMMIT / ROLLBACK), you'll get very lousy transaction semantics. Look at this example:

with connection:   # outer transaction

    with connection:   # inner transaction
        do_X_in_db()

    do_Y_in_db()

# once in a while, you get an exception there...

With this code, when you get an exception, X will be presevred in the database, but not Y. Most likely this breaks the expectations of the "outer transaction". Now, imagine the inner transaction in deep inside a third-party library, and you understand that this API is a Gun Pointed At Feet.

Of course, you could say "don't nest", but:

• this clashes with the expected semantics of Python context managers,
• it's unreasonable to expect Python users to audit all their lower level libraries for this issue!

Now, let's look at how popular database-oriented libraires handle this.

SQLAlchemy provides an explicit begin() method: http://docs.sqlalchemy.org/en/latest/core/connections.html#sqlalchemy.engine.Connection.begin

It also provides variants for nested transactions and two-phase commits.

Django provide an all-purpose atomic() context manager:
https://docs.djangoproject.com/en/stable/topics/db/transactions/#django.db.transaction.atomic

That function takes a keyword argument, savepoint, to control whether a savepoint is emitted for nested transactions.

So it's possible to implement a safe, nestable context manager with savepoints. However:

• you need to provide more control, and as a consequence you cannot simply use the connection as a context manager anymore;
• it takes a lot of rather complex code. See Django's implementation for an example:
  https://github.com/django/django/blob/stable/1.6.x/django/db/transaction.py#L199-L372

If you ignore the cross-database compatibility stuff, you're probably still looking at over a hundred lines of very stateful code...

That's why I believe it's better to leave this up to user code, and to stop providing an API that looks good for trivial use cases but that's likely to introduce subtle transactional integrity bugs.

[ghaering]

I'm +1 on deprecating the connection manager

[vstinner]

This issue is no newcomer friendly, I remove the "easy" keyword.

[coleifer]

With isolation_level set to None, the sqlite3 library is in autocommit mode, so changes will get committed immediately inside the with, which is simply broken.

Not necessarily. When sqlite is in autocommit mode, you can still open transactions by executing a BEGIN query. In fact, that's the main reason to use isolation_level=None -- you can manage the transactions yourself.

[Kodiologist]

This bit me real bad. On Python 3.8, I wrote a program with isolation_level = None and with db: … and spent a long time figuring out why writes were so slow. Turns out that with db doesn't actually start a transaction in this case, as the documentation suggests it should. This issue is approaching the age of 10, so if there's still uncertainty about how the implementation or the interface should change, the docs should be clarified in the meantime.

I always thought the Python library turning off autocommit by default, contrary to SQLite's command-line interface, was needlessly surprising. I think it contributed to this problem because the docs about context managers seem to assume you have autocommit off.

[erlend-aasland]

I'm not sure we should change the current context manager to emit BEGIN statements on enter. That could be backwards incompatible.

I think the best solution may be to deprecate the current context manager, and possibly introduce a new SAVEPOINT based context manager that allows nesting.

cx = sqlite3.connect(":memory:")
with cx.savepoint("sp1"):
    with cx.savepoint("sp2"):
        pass

# alternatively, we could auto-generate savepoint names internally
with cx.savepoint():
    with cx.savepoint():
        pass

If we can land a new PEP-249-compliant transaction behaviour (gh-83638), the existing context manager will behave like this:

• autocommit=False; a transaction is always open, so there will be no need to execute BEGIN on __enter__; exit will work as before and commit/rollback.
• autocommit=True; transaction handling is left to SQLite's autocommit mode (or any explicit transaction handling performed by the user); the existing context manager is a no-op (this will be documented).

[erlend-aasland]

@Kodiologist:

the docs should be clarified in the meantime.

See gh-93890

[erlend-aasland]

The docs are now updated in:

• main: gh-61162: Clarify sqlite3 connection context manager docs #93890
• 3.11: [3.11] gh-61162: Clarify sqlite3 connection context manager docs (GH-93890) #94013
• 3.10: [3.10] gh-61162: Clarify sqlite3 connection context manager docs (GH-93890) #94012

[erlend-aasland]

IMO, we can close this issue now.

I think the discussion of deprecating the current context manager and/or possibly replacing it with a new SAVEPOINT-based context manager should have a wider audience than this issue alone. I therefore suggest opening a topic on Discourse, to try and gain more attention to such changes. If there is sufficient interest in following up and implementing the deprecation and/or a new context manager, I suggest we create new issue on the bug tracker, with backlinks to this discussion for reference.

[phiresky]

I just hit the same issue as @Kodiologist

Specifically what happens:

First, you use sqlite3 library like this:

db = sqlite3.connect("x.db")
db.execute("create table foo(x);") # this auto-commits, as expected. `$ sqlite3 x.db .dump` shows the table as expected.
db.execute("insert into foo values (1);") # this implicitly creates a cursor and a transaction but doesn't commit it!!
db.close()
# $ sqlite3 x.db .dump # the table is empty! why?

After a while of thinking your code is buggy you find out that sqlite3 uses isolation_level=DEFERRED by default which starts a transaction "secretly" and is different than the default of the sqlite3 REPL (and pretty much every other database engine I've seen)

So you set isolation_level=None to get the behaviour you expected. But then some time later you write this code:

db = sqlite3.connect("x.db", isolation_level=None)

db.execute("insert into foo values (1);") # this one autocommits as expected
with db:
    for i in range(1000):
        db.execute("insert into foo values (1);")
        # i would expect these all to happen in one transaction, but turns out it commits after every insert
        # (you probably won't even realize this until later just thinking sqlite itself is slow)
    db.execute("syntax error")
    # the table contains the value that you think was rolled back
    # but it turns out `with db` doesn't actually do anything when isolation_mode=None

I think this behaviour is really confusing for most people, at least those coming from SQLite in different programming langugaes as well as other db engines like PostgreSQL.

Here's my workaround:

class ConnectionFixedCtxManager(sqlite3.Connection):
    def __enter__(self) -> ConnectionFixedCtxManager:
        raise Exception(
            "don't use contextmanager on sqlite3 connection, it's broken with isolation_level=None (which is the sane default). use .transaction() instead (see https://github.com/python/cpython/issues/61162)"
        )

    def transaction(self) -> CursorFixedCtxManager:
        return CursorFixedCtxManager(self)


class CursorFixedCtxManager(ContextManager[sqlite3.Cursor]):
    _cursor: sqlite3.Cursor | None = None

    def __init__(self, conn: ConnectionFixedCtxManager):
        self._conn = conn

    def __enter__(self) -> sqlite3.Cursor:
        self._cursor = self._conn.cursor()
        self._cursor.execute("BEGIN")
        return self._cursor

    def __exit__(
        self,
        exc_type: type[BaseException] | None,
        value: BaseException | None,
        traceback: TracebackType | None,
    ) -> None:
        assert self._cursor
        if exc_type is None:
            self._conn.commit()
        else:
            self._conn.rollback()
        self._cursor.close()
        self._cursor = None

Usage:

db = sqlite3.connect(..., isolation_level=None, factory=ConnectionFixedCtxManager)

with db.transaction() as cursor:
    cursor.execute("insert into foo values (1);")

Pretty simiilar to the .savepoint() suggestion by @erlend-aasland above except I think the context manager should return a cursor to prevent mixing up the main db object with the cursor. My code doesn't do anything for nested transactions since I don't use those. The separation between connection and cursor is still not really clean, IMO all convenience methods like db.execute() should throw an error when .transaction() was called / in_transaction == True

Edit: I see there's some efforts to maybe fix this soon. Awesome!

[erlend-aasland]

The separation between connection and cursor is still not really clean, IMO all convenience methods like db.execute() should throw an error when .transaction() was called / in_transaction == True

That would probably break existing code, so that is not an option.

The docs have been made more explicit in the last weeks. Hopefully that will reduce the confusion. Also, the upcoming autocommit attribute will introduce a cleaner API that should be easier to understand, imo.

[phiresky]

It wouldn't be a breaking change if it only happens with a new .savepoint() / .transaction() method right? My idea would be that while the context manager created from that function is open, the main Connection object would basically be disabled and not usable.

Otherwise the __enter__ method should probably return None and not Connection (as the current contextmanager does) or Cursor (as my suggestion). If it returns something I'd expect what it returns to be different and statements on the main connection object to be unaffected

[erlend-aasland]

Let's move this discussion to either Discourse or a new issue.