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.

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

lib,src,test,doc: add node:sqlite module #53752

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 7, 2024

#53264 has been open for over a month with no objections, so I am opening this PR with an initial node:sqlite module. There is other functionality that could potentially be exposed in the future, but I believe this is enough for an experimental MVP.

Fixes: #53264

Summary: Node.js now includes a built-in sqlite module (require('node:sqlite')) that becomes available when using the --experimental-sqlite flag

The following example shows the basic usage of the node:sqlite module to open
an in-memory database, write data to the database, and then read the data back.

import { DatabaseSync } from 'node:sqlite';
const database = new DatabaseSync(':memory:');

// Execute SQL statements from strings.
database.exec(`
  CREATE TABLE data(
    key INTEGER PRIMARY KEY,
    value TEXT
  ) STRICT
`);
// Create a prepared statement to insert data into the database.
const insert = database.prepare('INSERT INTO data (key, value) VALUES (?, ?)');
// Execute the prepared statement with bound values.
insert.run(1, 'hello');
insert.run(2, 'world');
// Create a prepared statement to read data from the database.
const query = database.prepare('SELECT * FROM data ORDER BY key');
// Execute the prepared statement and log the result set.
console.log(query.all());
// Prints: [ { key: 1, value: 'hello' }, { key: 2, value: 'world' } ]
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/loaders
  • @nodejs/startup
@nodejs-github-bot nodejs-github-bot added 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 7, 2024
@cjihrig cjihrig requested a review from benjamingr July 7, 2024 16:57
@RedYetiDev RedYetiDev added the experimental Issues and PRs related to experimental features. label Jul 7, 2024
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I can't review the C++ code, but API and tests LGTM for a MVP.

doc/api/sqlite.md Outdated Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Yay, happy to see this come to fruition. Let's land as experimental and iterate 🎉


Constructs a new `SQLiteDatabaseSync` instance.

### `database.close()`
Copy link
Member

Choose a reason for hiding this comment

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

It would be nifty to also support Symbol.dispose here but that can be in a follow up PR.

@benjamingr
Copy link
Member

This is a lot more feature complete and ready than I was expecting, good job Colin!

@RedYetiDev
Copy link
Member

This is adding a new feature, so shouldn't be semver-minor? (and with this kind of change, notable-change as well?)

doc/api/sqlite.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

Love it! Amazing job @cjihrig

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. experimental Issues and PRs related to experimental features. and removed experimental Issues and PRs related to experimental features. labels Jul 8, 2024
doc/api/sqlite.md Show resolved Hide resolved
doc/api/sqlite.md Show resolved Hide resolved
doc/api/sqlite.md Outdated Show resolved Hide resolved
doc/api/sqlite.md Show resolved Hide resolved
doc/api/sqlite.md Show resolved Hide resolved
doc/api/sqlite.md Outdated Show resolved Hide resolved
src/node_sqlite.cc Outdated Show resolved Hide resolved
src/node_sqlite.cc Show resolved Hide resolved
Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

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

Should we be concerned at all about the performance impact of people using these sync APIs within requests and blocking the event loop? With sync being the only option here I feel like the risk is increased.

If the concern is simplicity, I feel like only async would be better than only sync. Especially given top-level await. What reason is there to favour sync over async here?

I won't block on that concern, and in fact I think the work done so far is great. I would be happy landing what's here now. I just wanted to share my concern with going for sync first rather than async first.

@benjamingr
Copy link
Member

Should we be concerned at all about the performance impact of people using these sync APIs within requests and blocking the event loop? With sync being the only option here I feel like the risk is increased.

SQLite runs in process and is often CPU bound, we do have async APIs for some CPU intensive stuff (like in crypto) but I suspect for most people this API is the better one performance wise (that said, in some cases an async API is preferable and we should expose that as well)

@RafaelGSS RafaelGSS added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 10, 2024
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @RafaelGSS.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

@RafaelGSS
Copy link
Member

RafaelGSS commented Jul 10, 2024

@cjihrig I took the liberty to create a summary in the PR description for the notable changes section in the upcoming release. Please feel free to adjust it.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 10, 2024

Thanks @RafaelGSS.

how difficult would it be to support transferring DatabaseSync instances between worker threads, particularly :memory: instances?

@tlhunter the database could be made transferrable. I think we would want to add extra bookkeeping to clean up resources like finalizing prepared statements derived from the connection.

There is also a serialization API (not currently exposed in Node) if we just wanted to transfer the contents of the database, but I don't think that's what your question is about.

@tlhunter
Copy link
Contributor

@cjihrig I'm mostly curious about using it as a shared data storage, for example if many threads wanted to simultaneously write to a shared log.

Of course this could also be achieved by message passing and having a dedicated thread do the DB writes but it seems like sharing a database might make for cleaner patterns in app code. Theoretically writing to the same in-memory DB could even result in higher throughput assuming whatever locking mechanism sqlite has is more efficient than message passing overhead.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 10, 2024

for example if many threads wanted to simultaneously write to a shared log.

SQLite itself supports different threading modes. I believe that pattern would only be supported in serialized mode. If we made the connection transferrable, JavaScript would not allow the object to be used in multiple contexts though (transferring a prepared statement could be an option though).

You might be able to use SQLite shared cache for this in-memory use case, but I'm not 100% sure and shared cache is discouraged. Another option is to create a file-backed database in the temp directory and opening it from each thread.

@aduh95 aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jul 12, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jul 12, 2024

There are some conflicts with v22.x-staging, this would need a backport PR if we want it in 22.x.

targos pushed a commit that referenced this pull request Jul 12, 2024
PR-URL: #53752
Fixes: #53264
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
@targos
Copy link
Member

targos commented Jul 12, 2024

I found the conflicts trivial to fix. Pushed to v22.x-staging at 26a2c3a

@targos targos removed the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jul 12, 2024
@RedYetiDev
Copy link
Member

@targos @aduh95 is the appropriate label backported-to-xyz, or none because a specific backport PR was not needed? (For future reference)

Thanks!

@targos
Copy link
Member

targos commented Jul 13, 2024

none because a specific backport PR was not needed

@karlhorky
Copy link

If anyone wants to try out node:sqlite interactively in a sandbox, I've set up a CodeSandbox with @hemanth's node-nightly here:

CodeSandbox demo: https://codesandbox.io/p/devbox/node-sqlite-with-node-js-nightly-vygw6p?file=%2Findex.js

Screenshot 2024-07-14 at 12 27 04

@karimfromjordan
Copy link

I just tested this and it's very slow and appears to ship with SQLite 3.46. Is this expected in the Nightly build?

@bholmesdev
Copy link

@karimfromjordan I think that's a limitation of codesandbox. I was unable to run the sandbox on my machine, and I was also unable to get node-nightly to succeed. Luckily, I found these instructions on how to install the latest nightly release using nvm. This worked for me:

NVM_NODEJS_ORG_MIRROR=https://nodejs.org/download/nightly/ nvm i node

Running against an index.mjs file with the snippet from those PR notes ran very fast for me. Your machine may vary of course.

@karimfromjordan
Copy link

@bholmesdev I tested it locally. I ran 1000 inserts, with WAL enabled and it completes in about 1 - 1.5 secs which is really slow. better-sqlite3 finishes the same statements in 50 ms.

@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 14, 2024

This PR is about the addition of SQLite into Node.js. If you have concerns about its usage, please visit the help repo.

If you have concerns specific to the sandbox environment, please reach out to @karlhorky privately.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 14, 2024

@karimfromjordan thanks for sharing. I can confirm a slowdown, but I'm not seeing anything near 20x - more like 4x. Please note this is not optimized at all yet. When I start working on this again, I'll look into improving the perf if no one beats me to it.

@mpnkhan
Copy link

mpnkhan commented Jul 15, 2024

This is exciting. I always loved the PHP+MySql combo. And this one is one more reason for me to love nodeJS more.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 15, 2024

From a quick test, compiling SQLite in Node with the same settings used by better-sqlite3 yielded roughly the same performance. There may be other things to optimize as well, but it's good to know we aren't doing anything too awful 😄.

aduh95 pushed a commit that referenced this pull request Jul 16, 2024
PR-URL: #53752
Fixes: #53264
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
@aduh95 aduh95 mentioned this pull request Jul 16, 2024
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 16, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 17, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
RafaelGSS pushed a commit that referenced this pull request Jul 17, 2024
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. sqlite Issues and PRs related to the SQLite subsystem.