In statement.c, there is some logic which detects whether or not an incoming statement is a DML-type. The logic, as of 2019-05-08, I am referring to is here: https://github.com/python/cpython/blob/fc662ac332443a316a120fa5287c235dc4f8739b/Modules/_sqlite/statement.c#L78-L93

To demonstrate the bug:

import sqlite3

conn = sqlite3.connect(':memory:')

conn.execute('create table kv ("key" text primary key, "value" integer)')
conn.execute('insert into kv (key, value) values (?, ?), (?, ?)',
             ('k1', 1, 'k2', 2))

assert conn.in_transaction  # Yes we are in a transaction.
conn.commit()
assert not conn.in_transaction  # Not anymore, as expected.

rc = conn.execute(
    'with c(k, v) as (select key, value + 10 from kv) '
    'update kv set value=(select v from c where k=kv.key)')

print(rc.rowcount)  # Should be 2, prints "-1".
#assert conn.in_transaction  # !!! Fails.

curs = conn.execute('select * from kv order by key;')
print(curs.fetchall())  # [('k1', 11), ('k2', 12)]

Sqlite since 3.7.11 provides sqlite3_stmt_readonly() API for determining if a prepared statement will affect the database. I made the change, removing the SQL scanning code and replacing it with:

self->is_dml = !sqlite3_stmt_readonly(self->st);

But then I see a number of test failures, mostly related to the fact that table-creation is now treated as "is_dml" with the above change.

I don't know if the above API is going to be a workable path forward, since it seems like DML statements *not* automatically starting a transaction is a behavior a lot of people may have come to depend on (whether or not it is correct).

I've attached a patch just-in-case anyone's interested.

Oh, one more thing that is actually quite important -- since BEGIN IMMEDIATE and BEGIN EXCLUSIVE "modify" the database, these statements (intended to begin a transaction) are treated as "is_dml" when using the sqlite3_stmt_readonly API.

I've got a patch working now that:

- retains complete backwards-compatibility for DDL (as well as BEGIN EXCLUSIVE/IMMEDIATE) -- tests are passing locally.
- retains previous behavior for old sqlite that do not have the sqlite3_stmt_readonly API.

I think this should be good-to-go and I've in fact merged a similar patch on my own standalone pysqlite3 package.

Also I will add that the little 'test script' I provided is working as-expected with this patch applied.

CPython now accepts PRs on GitHub. Please try raising a PR as per devuguide : https://devguide.python.org/

I believe GH-13216 would be an improvement. I see that your original branch is unavailable, Charles; would you mind if I cherry-picked it and rebased it onto master? The sqlite3 module now requires SQLite >= 3.7.15 which simplifies the change a lot.

Yeah, go for it erlendaasland - I abandoned all hope of getting it merged, and have just been maintaining my own pysqlite3 which simplifies my life greatly.

Thanks, Charles. I'll give it a shot and see if get can provoke a response :)

See also bpo-35398

messages: + msg341962
versions: + Python 3.8

messages: + msg341961