sqlite: report zero changes for read-only statements

[zerone0x]

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

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[Renegade334]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.65%. Comparing base (a06e789) to head (9c6fb01).
⚠️ Report is 469 commits behind head on main.

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.

[zerone0x]

Thanks, that makes sense. I agree this approach is not correct: sqlite3_changes64() reports connection-level state for the most recent INSERT/UPDATE/DELETE, while sqlite3_stmt_readonly() only answers whether the prepared statement might write. Using read-only status would make run() return 0 for some statements that do not affect changes while still leaking the previous DML count for others, which is worse than preserving SQLite's documented behavior.\n\nI do not see a robust surgical fix that can produce per-statement row counts without reimplementing SQL classification and risking divergence from SQLite semantics. Closing this PR rather than patching around it.

[zerone0x]

Closing as invalid per the review discussion above.