FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sqlite: expose prepared statement statistics by geeksilva97 · Pull Request #64541 · nodejs/node · GitHub

/ node Public

sqlite: expose prepared statement statistics - #64541

Merged
nodejs-github-bot merged 7 commits into
nodejs:mainfrom
geeksilva97:sqlite-statement-statistics
Aug 13, 2026
ConversationCommits (7)ChecksFiles changed
Merged

sqlite: expose prepared statement statistics#64541
nodejs-github-bot merged 7 commits into
nodejs:mainfrom
geeksilva97:sqlite-statement-statistics

Conversation

geeksilva97 commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #64540

nodejs-github-bot commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

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 force-pushed the sqlite-statement-statistics branch 2 times, most recently from f5e8fcb to 2195e97 Compare July 16, 2026 18:42
geeksilva97 marked this pull request as ready for review July 16, 2026 18:43

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 Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.30%. Comparing base (d996610) to head (7d21604).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 90.24% 0 Missing and 4 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #64541      +/-   ##
==========================================
- Coverage   90.30%   90.30%   -0.01%     
==========================================
  Files         751      751              
  Lines      249116   249152      +36     
  Branches    47043    47065      +22     
==========================================
+ Hits       224974   224986      +12     
- Misses      15509    15556      +47     
+ Partials     8633     8610      -23
Files with missing lines Coverage Δ
src/node_sqlite.h 83.33% <ø> (ø)
src/node_sqlite.cc 81.63% <90.24%> (+0.16%) ⬆️

... and 31 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 added the wip Issues and PRs that are still a work in progress. label Jul 16, 2026
geeksilva97 force-pushed the sqlite-statement-statistics branch 2 times, most recently from bbbbeec to bef2511 Compare July 16, 2026 21:12
araujogui reviewed Jul 16, 2026
View reviewed changes
Comment thread src/node_sqlite.h
geeksilva97 force-pushed the sqlite-statement-statistics branch from bef2511 to fd5654c Compare July 17, 2026 16:25
geeksilva97 removed the wip Issues and PRs that are still a work in progress. label Jul 17, 2026

geeksilva97 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Please @nodejs/sqlite , share your thoughts

araujogui suggested changes Jul 23, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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},

araujogui Jul 23, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

araujogui Jul 23, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What do u suggest? A CHECK or something?

geeksilva97 Aug 11, 2026 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 3, 2026
geeksilva97 force-pushed the sqlite-statement-statistics branch from fd5654c to b0a266e Compare August 11, 2026 12:44

geeksilva97 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

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

trivikr requested changes Aug 12, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Outdated
Comment thread doc/api/sqlite.md Outdated
Comment thread doc/api/sqlite.md Outdated
sqlite: expose prepared statement statistics
e75cedb 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
sqlite: consolidate map lookup
6f2a5fc 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
sqlite: add method to rest statuses
33c7091 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
sqlite: handle older sqlite versions
849d773 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
sqlite: fix stale column name cache in resetStats()
5ff1f16 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
fixup: make linter happy
5a86587 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
fixup: rebase
7d21604 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
geeksilva97 force-pushed the sqlite-statement-statistics branch from 46d8994 to 7d21604 Compare August 12, 2026 21:20
trivikr added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Aug 13, 2026
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2026

nodejs-github-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/75807/

nodejs-github-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/75808/

trivikr added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 13, 2026
nodejs-github-bot merged commit f46e96f into nodejs:main Aug 13, 2026
79 checks passed

nodejs-github-bot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Landed in f46e96f

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 13, 2026
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
sqlite: expose prepared statement statistics
7d28c98 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
PR-URL: #64541
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 25, 2026
sqlite: expose prepared statement statistics
72c37b1 
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
PR-URL: #64541
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
github-actions Bot mentioned this pull request Aug 25, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
trivikr trivikr approved these changes
Renegade334 Awaiting requested review from Renegade334
+2 more reviewers
louwers louwers left review comments
araujogui araujogui requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sqlite: expose prepared statement stats

7 participants

Back | FazBrowse Home | New Git URL