Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sqlite: enable common flags #57621

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

sqlite: enable common flags #57621

wants to merge 2 commits into from
+147 −11

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Mar 25, 2025 •
edited
Loading

This PR enables flags that are common to other sqlite players in Node.js ecosystem:

This is related to #56476, even though it does not enable the RBU extension.

I see this as a good step toward the stabilization since it will make it easier for people from other dependencies.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Mar 25, 2025
@geeksilva97
Copy link
Contributor Author

Not sure whether this will be a problem, but this increases the binary size by 600KB.

@geeksilva97 geeksilva97 force-pushed the enable-common-sqlite-flags branch from e4b93b9 to 9590dee Compare March 25, 2025 17:53
cjihrig
cjihrig reviewed Mar 25, 2025
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Not saying whether or not we should land this, but https://github.com/nodejs/node/blob/main/deps/sqlite/unofficial.gni should be updated, and there should be at least one test for each new API to prevent regressions.

@geeksilva97
Copy link
Contributor Author

Not saying whether or not we should land this, but https://github.com/nodejs/node/blob/main/deps/sqlite/unofficial.gni should be updated, and there should be at least one test for each new API to prevent regressions.

Good point. Also, I think it would be good to have some benchmarks on sqlite implementations.

@geeksilva97 geeksilva97 force-pushed the enable-common-sqlite-flags branch from 9590dee to 0c1ec86 Compare March 25, 2025 19:22
@geeksilva97
Copy link
Contributor Author

Not saying whether or not we should land this, but https://github.com/nodejs/node/blob/main/deps/sqlite/unofficial.gni should be updated, and there should be at least one test for each new API to prevent regressions.

@cjihrig , what's the difference between .gyp and .gni file?

@geeksilva97 geeksilva97 force-pushed the enable-common-sqlite-flags branch from 0c1ec86 to 31c5aa5 Compare March 25, 2025 19:28
@cjihrig
Copy link
Contributor

cjihrig commented Mar 25, 2025

what's the difference between .gyp and .gni file?

They are for two different build systems. The .gyp file is used by the official build. The .gni file is used by the unofficial GN build.

@geeksilva97
Copy link
Contributor Author

what's the difference between .gyp and .gni file?

They are for two different build systems. The .gyp file is used by the official build. The .gni file is used by the unofficial GN build.

Thank you!

@cjihrig
Copy link
Contributor

cjihrig commented Mar 25, 2025

even though it does not enable the RBU extension.

Why not? And why not the Geopoly extension?

@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Mar 25, 2025 •
edited
Loading

even though it does not enable the RBU extension.

Why not? And why not the Geopoly extension?

For RBU, I didn't see it in better-sqlite3 or node-sqlite3. Geopoly seems like a good extension to have.

@geeksilva97
Copy link
Contributor Author

geeksilva97 commented Mar 25, 2025 •
edited
Loading

even though it does not enable the RBU extension.

Why not? And why not the Geopoly extension?

For RBU, I didn't see it in better-sqlite3 or node-sqlite3. Geopoly seems like a good extension to have.

On the other hand, the issue mentions the SQLite Amalgamation. I think we could have RBU enabled.

The amalgamation contains everything you need to integrate SQLite into a larger project

@codecov Codecov
Copy link

codecov bot commented Mar 25, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (1b5b019) to head (c22f75e).
Report is 16 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57621      +/-   ##
==========================================
+ Coverage   90.22%   90.23%   +0.01%     
==========================================
  Files         630      630              
  Lines      185054   185055       +1     
  Branches    36254    36219      -35     
==========================================
+ Hits       166963   166985      +22     
- Misses      11032    11037       +5     
+ Partials     7059     7033      -26

see 45 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.

TheOneTheOnlyJJ
TheOneTheOnlyJJ reviewed Mar 26, 2025
View reviewed changes
Copy link

@TheOneTheOnlyJJ TheOneTheOnlyJJ left a comment

Choose a reason for hiding this comment

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

Small typo

@geeksilva97 geeksilva97 force-pushed the enable-common-sqlite-flags branch from 31c5aa5 to 3f64dfa Compare March 26, 2025 13:49
geeksilva97
geeksilva97 commented Mar 26, 2025
View reviewed changes
test/parallel/test-sqlite.js
test('rbu is enabled', (t) => {
const db = new DatabaseSync(':memory:');
t.assert.deepStrictEqual(
db.prepare('SELECT sqlite_compileoption_used(\'SQLITE_ENABLE_RBU\') as rbu_enabled;').get(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is more straightforward because RBU is just a way to handle bulk updates from one database to another. The process needs more tools (either sqlitediff or C/C++ implementation).

https://www.sqlite.org/rbu.html#database_contents

Copy link

Choose a reason for hiding this comment

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

Are you thinking what I'm thinking? 😂
Maybe sqlitediff could itself also be packaged in Node.js, with a JS API exposing it gracefully.
I do not have the time to work on this, but it could definitely be investigated.

cjihrig
cjihrig reviewed Mar 26, 2025
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

One comment, but changes LGTM.

@geeksilva97 @cjihrig
sqlite: apply review change
c22f75e 
Co-authored-by: Colin Ihrig <[email protected]>
TheOneTheOnlyJJ
TheOneTheOnlyJJ reviewed Mar 26, 2025
View reviewed changes
test/parallel/test-sqlite.js
);
});

test('fts3 parenthesis', (t) => {
Copy link

Choose a reason for hiding this comment

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

This is inconsistent with the rest, missing is enabled in its name.

@TheOneTheOnlyJJ TheOneTheOnlyJJ mentioned this pull request Mar 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@TheOneTheOnlyJJ TheOneTheOnlyJJ TheOneTheOnlyJJ left review comments

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file. 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.
4 participants
@geeksilva97 @nodejs-github-bot @cjihrig @TheOneTheOnlyJJ