SQLite incorrect row count for UPDATE

[montanalow]

BPO	35398
Nosy	@berkerpeksag, @lysnikolaou, @tirkarthi, @erlend-aasland
PRs	bpo-35398: Improve DDL statement detection for SQLite #10913

^{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 2018-12-04.02:27:45.503>
labels = ['3.8', '3.7', 'library']
title = 'SQLite incorrect row count for UPDATE'
updated_at = <Date 2021-08-11.21:01:29.425>
user = 'https://bugs.python.org/MontanaLow'

bugs.python.org fields:

activity = <Date 2021-08-11.21:01:29.425>
actor = 'erlendaasland'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2018-12-04.02:27:45.503>
creator = 'Montana Low'
dependencies = []
files = []
hgrepos = []
issue_num = 35398
keywords = ['patch']
message_count = 4.0
messages = ['331006', '331069', '331090', '399417']
nosy_count = 7.0
nosy_names = ['ghaering', 'zzzeek', 'berker.peksag', 'lys.nikolaou', 'xtreak', 'Montana Low', 'erlendaasland']
pr_nums = ['10913']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue35398'
versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

[montanalow]

SQLite driver returns an incorrect row count (-1) for UPDATE statements that begin with a comment.

Downstream Reference:
sqlalchemy/sqlalchemy#4396

Test Case:

import sqlite3

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

cursor = conn.cursor()

cursor.execute("""
CREATE TABLE foo (
    id INTEGER NOT NULL,
    updated_at DATETIME,
    PRIMARY KEY (id)
)
""")

cursor.execute("""
/* watermarking bug */
INSERT INTO foo (id, updated_at) VALUES (?, ?)
""", [1, None])


cursor.execute("""
    UPDATE foo SET updated_at=? WHERE foo.id = ?
""", ('2018-12-02 14:55:57.169785', 1))

assert cursor.rowcount == 1

cursor.execute("""
    /* watermarking bug */
    UPDATE foo SET updated_at=? WHERE foo.id = ?
""", ('2018-12-03 14:55:57.169785', 1))

assert cursor.rowcount == 1

[tirkarthi]

Did some debugging here. If I am understanding this correctly the rowcount is set at

cpython/Modules/_sqlite/cursor.c

Line 574 in b8e689a

if (self->statement->is_dml) {

. It checks for is_dml flag that is set here

cpython/Modules/_sqlite/statement.c

Line 78 in b8e689a

self->is_dml = 0;

The part to set is_dml skips space, tabs and newline and checks for the first set of characters that is not skipped to be insert, update, delete or replace and in this case the first set of characters to be matched will be "/* watermarking */". Thus with comment not matching, is_dml is not set and -1 is set for the rowcount.

[montanalow]

I took a stab at patch. It fixes the issue for me, as proven via the Test Case.

[erlend-aasland]

See also gh-81040.

[erlend-aasland]

Now that we require SQLite 3.7.15, we can safely use sqlite3_stmt_readonly to determine if we should update .rowcount or not. The API returns true for SELECT queries, which is also the case today. The API is pretty conservative, and returns false for cases where a query might modify the database. In those cases, we may end up with .rowcount being zero. That is perfectly fine.

I see no reason not to use sqlite3_stmt_readonly to solve this.

EDIT:

sqlite3_changes support INSERT, UPDATE, and DELETE statements only. sqlite3_stmt_readonly encompasses a lot more than that. As long as we are using sqlite3_changes to update .rowcount, we simply cannot use sqlite3_stmt_readonly, because SQLite does not reset the number returned by sqlite3_changes; for unsupported statements, you may end up with sqlite3_changes returning the change count for a previous statement.

If we'd used something like sqlite3_total_changes64, we could safely use sqlite3_stmt_readonly, because in the worst cases we'd end up with a .rowcount of zero, and that is perfectly fine.

[erlend-aasland]

I see no reason not to use sqlite3_stmt_readonly to solve this.

@animalize made me change my mind about that, so I made a competing PR: gh-93623

Thanks for pushing, Ma Lin.