Skip to content

sqlite: report zero changes for read-only statements#62128

Open
zerone0x wants to merge 1 commit intonodejs:mainfrom
zerone0x:fix/sqlite-readonly-changes
Open

sqlite: report zero changes for read-only statements#62128
zerone0x wants to merge 1 commit intonodejs:mainfrom
zerone0x:fix/sqlite-readonly-changes

Conversation

@zerone0x
Copy link

@zerone0x zerone0x commented Mar 6, 2026

Read-only statements (e.g. SELECT) should always report 0 for both
changes and lastInsertRowid. Previously, running a SELECT after
an INSERT would leak the change count from the INSERT because
sqlite3_changes64() returns the count from the most recent write
statement on the connection.

Use sqlite3_stmt_readonly() to detect read-only statements and
return 0 instead of querying the connection-level counters.

const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE test (id INTEGER PRIMARY KEY, name TEXT)');
db.prepare('INSERT INTO test (name) VALUES (?)').run('foo');

const result = db.prepare('SELECT * FROM test').run();
// Before: { changes: 1, lastInsertRowid: 1 }
// After:  { changes: 0, lastInsertRowid: 0 }

Fixes: #59764

Read-only statements (e.g. SELECT) should always report 0 for both
changes and lastInsertRowid. Previously, running a SELECT after an
INSERT would leak the change count from the INSERT because
sqlite3_changes64() returns the count from the most recent write
statement on the connection.

Use sqlite3_stmt_readonly() to detect read-only statements and
return 0 instead of querying the connection-level counters.

Fixes: nodejs#59764

Co-Authored-By: Claude <noreply@anthropic.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Mar 6, 2026
Copy link
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't block in case I am a lone voice here, but I am -0.99 on this.

The kicker here is that "statements that impact sqlite3_changes" and "statements for which sqlite3_stmt_readonly is false" are not synonymous. There are a massive group of statements that are not INSERT/UPDATE/DELETE statements that the API will not mark as read-only, so this will result in a scenario where some statements that don't impact sqlite3_changes will return a zero rowcount, and others will return the rowcount of the last relevant query, which I would argue is worse.

I stand by my original comment at #59764 (comment): it shouldn't be a mystery to the user as to what sort of query they're executing, and that will directly determine whether the rowcount has any meaning. So long as this behaviour is documented, I don't see any great advantage to trying to hack around it.

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 89.65%. Comparing base (a06e789) to head (9c6fb01).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62128      +/-   ##
==========================================
- Coverage   89.66%   89.65%   -0.01%     
==========================================
  Files         676      676              
  Lines      206348   206351       +3     
  Branches    39527    39523       -4     
==========================================
- Hits       185019   185012       -7     
- Misses      13464    13468       +4     
- Partials     7865     7871       +6     
Files with missing lines Coverage Ξ”
src/node_sqlite.cc 80.82% <100.00%> (+0.02%) ⬆️

... and 33 files with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:sqlite read-only statements may report nonzero change counts

3 participants