This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: sqlite3 dml statement detection does not account for CTEs
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, coleifer, erlendaasland, malin, xtreak
Priority: normal Keywords: patch

Created on 2019-05-08 23:08 by coleifer, last changed 2022-04-11 14:59 by admin.

Files
File name Uploaded Description Edit
36859.patch coleifer, 2019-05-09 03:39
36859-2.patch coleifer, 2019-05-09 04:12 Working patch that retains backwards-compatibility.
Pull Requests
URL Status Linked Edit
PR 13216 closed coleifer, 2019-05-09 04:44
PR 24492 open erlendaasland, 2021-02-09 14:38
Messages (9)
msg341949 - (view) Author: Charles (coleifer) * Date: 2019-05-08 23:08
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)]
msg341956 - (view) Author: Charles (coleifer) * Date: 2019-05-09 03:39
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.
msg341958 - (view) Author: Charles (coleifer) * Date: 2019-05-09 03:42
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.
msg341961 - (view) Author: Charles (coleifer) * Date: 2019-05-09 04:12
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.
msg341962 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2019-05-09 04:17
CPython now accepts PRs on GitHub. Please try raising a PR as per devuguide : https://devguide.python.org/
msg386713 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-02-09 14:01
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.
msg386716 - (view) Author: Charles (coleifer) * Date: 2021-02-09 14:26
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.
msg386721 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-02-09 14:36
Thanks, Charles. I'll give it a shot and see if get can provoke a response :)
msg399418 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-08-11 21:02
See also bpo-35398
History
Date User Action Args
2022-04-11 14:59:14adminsetgithub: 81040
2021-08-11 21:02:17erlendaaslandsetmessages: + msg399418
2021-02-09 14:38:11erlendaaslandsetpull_requests: + pull_request23282
2021-02-09 14:36:32erlendaaslandsetmessages: + msg386721
2021-02-09 14:26:42coleifersetmessages: + msg386716
2021-02-09 14:01:15erlendaaslandsetmessages: + msg386713
2020-05-28 09:39:16erlendaaslandsetnosy: + erlendaasland
2019-05-09 12:35:13malinsetnosy: + malin
2019-05-09 04:44:31coleifersetstage: patch review
pull_requests: + pull_request13127
2019-05-09 04:17:35xtreaksetnosy: + xtreak, berker.peksag

messages: + msg341962
versions: + Python 3.8
2019-05-09 04:12:20coleifersetfiles: + 36859-2.patch

messages: + msg341961
2019-05-09 03:42:54coleifersetmessages: + msg341958
2019-05-09 03:39:01coleifersetfiles: + 36859.patch
keywords: + patch
messages: + msg341956
2019-05-08 23:08:24coleifercreate