Skip to content

sqlite: expose prepared statement statistics - #64541

Open
geeksilva97 wants to merge 4 commits into
nodejs:mainfrom
geeksilva97:sqlite-statement-statistics
Open

sqlite: expose prepared statement statistics#64541
geeksilva97 wants to merge 4 commits into
nodejs:mainfrom
geeksilva97:sqlite-statement-statistics

Conversation

@geeksilva97

@geeksilva97 geeksilva97 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Closes #64540

@nodejs-github-bot

Copy link
Copy Markdown
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++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 16, 2026
@geeksilva97
geeksilva97 force-pushed the sqlite-statement-statistics branch 2 times, most recently from f5e8fcb to 2195e97 Compare July 16, 2026 18:42
@geeksilva97
geeksilva97 marked this pull request as ready for review July 16, 2026 18:43
@bakkot

bakkot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

I'm a little uncomfortable with getters that create an object every time they're read.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (cb9bb66) to head (11ac9a0).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 89.18% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64541      +/-   ##
==========================================
+ Coverage   90.31%   90.33%   +0.01%     
==========================================
  Files         760      760              
  Lines      248532   248564      +32     
  Branches    46908    46909       +1     
==========================================
+ Hits       224467   224534      +67     
+ Misses      15505    15456      -49     
- Partials     8560     8574      +14     
Files with missing lines Coverage Δ
src/node_sqlite.h 82.60% <ø> (ø)
src/node_sqlite.cc 81.63% <89.18%> (+0.14%) ⬆️

... and 23 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.

@geeksilva97 geeksilva97 added the wip Issues and PRs that are still a work in progress. label Jul 16, 2026
@geeksilva97
geeksilva97 force-pushed the sqlite-statement-statistics branch 2 times, most recently from bbbbeec to bef2511 Compare July 16, 2026 21:12
Comment thread src/node_sqlite.h
@geeksilva97
geeksilva97 force-pushed the sqlite-statement-statistics branch from bef2511 to fd5654c Compare July 17, 2026 16:25
@geeksilva97 geeksilva97 removed the wip Issues and PRs that are still a work in progress. label Jul 17, 2026
@geeksilva97

Copy link
Copy Markdown
Contributor Author

Please @nodejs/sqlite , share your thoughts

@araujogui araujogui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Another method to reset stats would be nice to have

Comment thread src/node_sqlite.h
{"reprepare", SQLITE_STMTSTATUS_REPREPARE},
{"run", SQLITE_STMTSTATUS_RUN},
{"filterMiss", SQLITE_STMTSTATUS_FILTER_MISS},
{"filterHit", SQLITE_STMTSTATUS_FILTER_HIT},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SQLITE_STMTSTATUS_FILTER_MISS and SQLITE_STMTSTATUS_FILTER_HIT was introduced in SQLite 3.38.0.

3.37.2: https://github.com/sqlite/sqlite/blob/version-3.37.2/src/sqlite.h.in
3.38.0: https://github.com/sqlite/sqlite/blob/version-3.38.0/src/sqlite.h.in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So maybe we need to enforce a minimum SQLite version on --shared-sqlite

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do u suggest? A CHECK or something?

@geeksilva97 geeksilva97 Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added some ifdefs. Let's see what the team says

Comment thread src/node_sqlite.cc
Comment thread test/parallel/test-sqlite-statement-sync.js Outdated
Comment thread src/node_sqlite.cc Outdated
@trivikr trivikr added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 3, 2026
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
@geeksilva97
geeksilva97 force-pushed the sqlite-statement-statistics branch from fd5654c to b0a266e Compare August 11, 2026 12:44
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
@geeksilva97

Copy link
Copy Markdown
Contributor Author

Please @araujogui . Let me know if you have any other concerns

@trivikr trivikr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified against a rebuilt PR head. One issue is a reproducible native crash; the other comments correct the documented SQLite semantics.

Comment thread src/node_sqlite.cc
// sqlite3_stmt_status() resets a single counter per call, so every exposed
// counter is visited. The returned value is the pre-reset one and is unused.
for (const auto& info : kStatusMapping) {
sqlite3_stmt_status(stmt->statement_, info.sqlite_status_id, true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Resetting SQLITE_STMTSTATUS_REPREPARE desynchronizes the iterator column-name cache. After caching names, altering the schema, calling resetStats(), and altering it again, the counter returns to the cached value and iterate() reads past the stale key array; I reproduced exit code 139. Please invalidate the cache and its generation when resetting stats. For example:

 void StatementSync::InvalidateColumnNameCache() {
   cached_column_names_.clear();
+  cached_column_names_reprepare_count_ = -1;
 }
@@
   for (const auto& info : kStatusMapping) {
     sqlite3_stmt_status(stmt->statement_, info.sqlite_status_id, true);
   }
+  stmt->InvalidateColumnNameCache();

Comment thread doc/api/sqlite.md
Comment on lines +1202 to +1203
Resets every counter reported by [`statement.stat()`][] back to zero. This
method is a wrapper around [`sqlite3_stmt_status()`][] and is useful for

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SQLITE_STMTSTATUS_MEMUSED is not a counter, and SQLite ignores resetFlg for it. I verified that it remains 3856 before and after resetStats(), so this contract cannot hold.

Suggested change
Resets every counter reported by [`statement.stat()`][] back to zero. This
method is a wrapper around [`sqlite3_stmt_status()`][] and is useful for
Resets every counter reported by [`statement.stat()`][] back to zero, except
`memused`, which reports current memory usage and cannot be reset. This

Comment thread doc/api/sqlite.md
prepared statement.
* `'reprepare'` The number of times the statement has been automatically
reprepared due to schema changes or changes to bound parameters.
* `'run'` The number of times the statement has run to completion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SQLITE_STMTSTATUS_RUN increments on the first sqlite3_step(), not when execution completes; a partially consumed iterator increments it too.

Suggested change
* `'run'` The number of times the statement has run to completion.
* `'run'` The number of execution cycles started by the prepared statement.

Comment on lines +593 to +594
suite('StatementSync.prototype.resetStats()', () => {
test('returns undefined', (t) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Regression rest for the suggestion from #64541 (comment)

Suggested change
suite('StatementSync.prototype.resetStats()', () => {
test('returns undefined', (t) => {
suite('StatementSync.prototype.resetStats()', () => {
test('invalidates cached iterator column names', (t) => {
using db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE data(a); INSERT INTO data VALUES (1)');
const stmt = db.prepare('SELECT * FROM data');
db.exec('ALTER TABLE data RENAME COLUMN a TO b');
stmt.iterate().toArray();
stmt.resetStats();
db.exec('ALTER TABLE data RENAME COLUMN b TO c');
t.assert.deepStrictEqual(stmt.iterate().toArray(), [
{ __proto__: null, c: 1 },
]);
});
test('returns undefined', (t) => {

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++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

sqlite: expose prepared statement stats

6 participants