GeezerVison is showing github.com — sqlite3 dml statement detection does not account for CTEs · Issue #81040 · python/cpython
sqlite3 dml statement detection does not account for CTEs #81040Description
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 2019-05-08.23:08:24.384>
labels = ['3.8', 'type-bug', 'library']
title = 'sqlite3 dml statement detection does not account for CTEs'
updated_at = <Date 2021-08-11.21:02:17.366>
user = 'https://github.com/coleifer' bugs.python.org fields:
activity = <Date 2021-08-11.21:02:17.366>
actor = 'erlendaasland'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-05-08.23:08:24.384>
creator = 'coleifer'
dependencies = []
files = ['48319', '48320']
hgrepos = []
issue_num = 36859
keywords = ['patch']
message_count = 9.0
messages = ['341949', '341956', '341958', '341961', '341962', '386713', '386716', '386721', '399418']
nosy_count = 5.0
nosy_names = ['berker.peksag', 'malin', 'xtreak', 'coleifer', 'erlendaasland']
pr_nums = ['13216', '24492']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36859'
versions = ['Python 3.8'] Activitycoleifer commented on May 8, 2019 MannequinAuthor More actions 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:
|
self->is_dml = 0; |
|
for (p = sql_cstr; *p != 0; p++) { |
|
switch (*p) { |
|
case ' ': |
|
case '\r': |
|
case '\n': |
|
case '\t': |
|
continue; |
|
} |
|
|
|
self->is_dml = (PyOS_strnicmp(p, "insert", 6) == 0) |
|
|| (PyOS_strnicmp(p, "update", 6) == 0) |
|
|| (PyOS_strnicmp(p, "delete", 6) == 0) |
|
|| (PyOS_strnicmp(p, "replace", 7) == 0); |
|
break; |
|
} |
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)]coleifer commented on May 9, 2019 MannequinAuthor More actions 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. coleifer commented on May 9, 2019 MannequinAuthor More actions 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. coleifer commented on May 9, 2019 MannequinAuthor More actions 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. tirkarthi commented on May 9, 2019 erlend-aasland commented on Feb 9, 2021 I believe #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. coleifer commented on Feb 9, 2021 MannequinAuthor More actions 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. erlend-aasland commented on Feb 9, 2021 Thanks, Charles. I'll give it a shot and see if get can provoke a response :) erlend-aasland commented on Aug 11, 2021 erlend-aasland commented on Jun 21, 2022 I'm reopening this, because this issue materialises into two problems:
erlend-aasland commented on Jul 17, 2022
2. rowcount is not updated as expected for CTE's.
Suggesting to address the rowcount issue by improving the accuracy of the rowcount docs. We can then close this issue. erlend-aasland commented on Jul 22, 2022 Rowcount docs are updated in the following branches:
Considering this as resolved.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|