sqlite3 dml statement detection does not account for CTEs

[coleifer]

BPO	36859
Nosy	@berkerpeksag, @animalize, @tirkarthi, @coleifer, @erlend-aasland
PRs	bpo-36859: Use sqlite3_stmt_readonly API when possible to determine if statement is DML.K #13216 [WIP] gh-81040: use sqlite3_stmt_readonly() to detect DML statements #24492
Files	36859.patch 36859-2.patch: Working patch that retains backwards-compatibility.

^{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']

[coleifer]

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:

cpython/Modules/_sqlite/statement.c

Lines 78 to 93 in fc662ac

	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]

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]

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]

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]

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

[erlend-aasland]

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]

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]

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

[erlend-aasland]

See also gh-79579 (bpo-35398)

[erlend-aasland]

As of the recent discussion on Discourse, I'm closing this as superseded by the proposed solution in gh-83638.

TL;DR: We cannot change the behaviour because of backwards compatibility. The proposed fix is to introducing a new, PEP 249 compliant way of controlling transactions. The old transaction handling (isolation_level) will be deprecated.

[erlend-aasland]

I'm reopening this, because this issue materialises into two problems:

• The implicit transaction handling is not as expected (in_transaction).
  Superseded by: Add an autocommit property to sqlite3.Connection with a PEP 249 compliant manual commit mode and migrate #83638
• rowcount is not updated as expected for CTE's.

[erlend-aasland]

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]

Rowcount docs are updated in the following branches:

• main: gh-81040: Improve sqlite3.Cursor.rowcount docs #94940
• 3.11: [3.11] gh-81040: Improve sqlite3.Cursor.rowcount docs (GH-94940) #95124
• 3.10: [3.10] gh-81040: Improve sqlite3.Cursor.rowcount docs (GH-94940) #95125

Considering this as resolved.