Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqlite3.connect(autocommit=False) breaks executescript("BEGIN; COMMIT") #112441

Closed
tom93 opened this issue Nov 27, 2023 · 5 comments
Closed

sqlite3.connect(autocommit=False) breaks executescript("BEGIN; COMMIT") #112441

tom93 opened this issue Nov 27, 2023 · 5 comments

Comments

@tom93
Copy link
Contributor

tom93 commented Nov 27, 2023

Bug report

Bug description:

Background: The autocommit parameter was added to the sqlite3 module in Python 3.12 (#83638), the default is currently LEGACY_TRANSACTION_CONTROL but it is set to change to False in a future Python release.

When autocommit is False, it breaks code that uses executescript() with transaction control. Example:

import sqlite3
con = sqlite3.connect(":memory:")
con.executescript("BEGIN TRANSACTION; CREATE TABLE t (x); INSERT INTO t VALUES(1); COMMIT;")
con.close()

This program works on all existing Python versions (2.7 - 3.13.0a1), but with autocommit=False it throws the following error:

Traceback (most recent call last):
  File "<string>", line 4, in <module>
sqlite3.OperationalError: cannot start a transaction within a transaction

So making autocommit=False the default in future will break backward compatibility.


One idea for fixing this is to change the behaviour of executescript() so that when autocommit is False, it will first execute an implicit COMMIT (as it already does when autocommit=LEGACY_TRANSACTION_CONTROL), and after it will execute BEGIN. Edit: This idea is unsuitable, because it is incompatible with the model where a transaction is always open.

@tom93 tom93 added the type-bug An unexpected behavior, bug, or error label Nov 27, 2023
@AlexWaygood AlexWaygood added topic-sqlite3 3.12 bugs and security fixes 3.13 bugs and security fixes labels Nov 27, 2023
@erlend-aasland erlend-aasland self-assigned this Nov 27, 2023
@erlend-aasland
Copy link
Contributor

This is intended behaviour; autocommit=False implies PEP-249 compliant behaviour, meaning a transaction is always open. Please read the relevant docs on how autocommit is intended to work. See also PEP-249.

https://docs.python.org/3/library/sqlite3.html#sqlite3-transaction-control-autocommit

@erlend-aasland erlend-aasland closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
@erlend-aasland erlend-aasland added invalid and removed type-bug An unexpected behavior, bug, or error 3.12 bugs and security fixes 3.13 bugs and security fixes labels Nov 27, 2023
@erlend-aasland
Copy link
Contributor

So making autocommit=False the default in future will break backward compatibility.

No. autocommit was introduced in 3.12; it was not present in earlier versions. Changing the default in the future is possible with the help of documentation, changelog/What's New entries, and deprecation warnings.

@tom93
Copy link
Contributor Author

tom93 commented Nov 27, 2023

Hi @erlend-aasland, thanks for your reply. I see now that my proposed fix is unsuitable. And I missed that apparently during the deprecation period there will be a warning unless autocommit is explicitly set to True or False (source), which should address the backward compatibility issue.


Can you please share your thoughts on the best way to use executescript() when the SQL string uses transaction control? My use case is restoring a database from a dump (generated using iterdump() or CLI .dump). I'd like to support existing and future Python versions.

Here are the options I can think of:

A. Use isolation_level=None. Issues: isolation_level will be deprecated(?), and there will be warnings if I don't set autocommit explicitly(?).

B. Use autocommit=True. Issues: requires Python 3.12, and the docs recommend autocommit=False.

So should I use sys.version_info / hasattr(sqlite3.Connection, "autocommit") to switch between A and B?

I've also come up with the following, which works with both the current default and the future default, however it's a horrible hack and will produce a warning during the deprecation period(?) because it doesn't explicitly set autocommit:

import sqlite3
dump = "BEGIN TRANSACTION; ...; COMMIT"
con = sqlite3.connect("restored.sqlite")
if con.in_transaction():
    con.execute("COMMIT")
con.executescript(dump)
con.execute("BEGIN")
...
con.close()

@erlend-aasland
Copy link
Contributor

Can you please share your thoughts on the best way to use executescript() when the SQL string uses transaction control?

For that scenario you cannot use the implicit transaction control recommended by PEP-249. You can do that either by setting autocommit=True (Python 3.12 or newer) or isolation_level=None (Python 3.11 or older). You can for example set the connect keywords conditionally:

kdws = {"isolation_level": None} if legacy else {"autocommit": True}
cx = sqlite3.connect(path, **kwds)
cx.executescript(sql_with_tx_ctrl_statements)

It's up to you how you set legacy; using sys.version_info is fine. So is hasattr.

A. Use isolation_level=None. Issues: isolation_level will be deprecated(?), and there will be warnings if I don't set autocommit explicitly(?).

Currently, isolation_level is only soft deprecated (docs only). Perhaps that will change in the future; I can't promise anything now :(

B. Use autocommit=True. Issues: [...] and the docs recommend autocommit=False.

I don't see how this is an issue; the docs recommend the default that PEP-249 recommends. If your application's requirements don't align with that recommendation, that's ok. It is ok to use autocommit=True.

@tom93
Copy link
Contributor Author

tom93 commented Nov 28, 2023

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants