sqlite: add support for SQLite Session Extension

[louwers]

We were talking about adding support for the Session Extension of SQLite in #53752. This is not a run-time extension but rather a feature built in to the SQLite amalgamation. It can be enabled with a compile flag and is enabled by default on some platforms.

I have never contributed to Node.js before so I will probably need some help to bring this to a conclusion.

---

TODO:

• Make sure sessions are deleted before the database they are attached to is closed
• Consensus on API
• Documentation
• Allow custom conflict handler
• Throw with specific (documented) exception in case of conflict when applying changeset since SQLite doesn't consider this to be an error I return false when applying the changeset is aborted due to a conflict
• Allow generating a patchset as well
• Allow specifying table name when creating session (to only track that table)
• Implement Session.close()

---

Example usage:

const database1 = new DatabaseSync(':memory:');
const database2 = new DatabaseSync(':memory:');

database1.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, value TEXT)');
database2.exec('CREATE TABLE data(key INTEGER PRIMARY KEY, value TEXT)');

const session = database1.createSession();

const insert = database1.prepare('INSERT INTO data (key, value) VALUES (?, ?)');
insert.run(1, 'hello');
insert.run(2, 'world');

const changeset = session.changeset();
database2.applyChangeset(changeset);
// Now database2 contains the same data as database1 

[nodejs-github-bot]

Review requested:

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

[karimfromjordan]

Wouldn't this be solved if users could use their own SQLite build? That's already being considered according to the original issue.

[louwers]

@karimfromjordan Nope, because you need access to the C API to access this feature.

[anonrig]

Can you add the appropriate documentation as well?

[codecov]

Codecov Report

Attention: Patch coverage is 86.06965% with 28 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (7b01758) to head (092a28c).
Report is 125 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	86.93%	6 Missing and 20 partials ⚠️
src/node_sqlite.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54181      +/-   ##
==========================================
+ Coverage   88.40%   88.41%   +0.01%     
==========================================
  Files         654      654              
  Lines      187594   188034     +440     
  Branches    36089    36169      +80     
==========================================
+ Hits       165834   166259     +425     
+ Misses      15001    14999       -2     
- Partials     6759     6776      +17     

Files with missing lines	Coverage Δ
src/node_sqlite.h	70.00% <0.00%> (-7.78%)	⬇️
src/node_sqlite.cc	83.80% <86.93%> (+0.47%)	⬆️

... and 62 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[louwers]

I need to make sure sessions are deleted before the database they are attached to is closed.

I think I need the DatabaseSync hold onto the Sessions it creates. Both are BaseObjects so I need to look into how to do that.

When a Session goes out of scope the corresponding DatabaseSync needs to no longer hold onto it. Conversely I also need to make sure that all operations on Session objects are no-ops after the corresponding database is closed.

[louwers]

@anonrig I have added documentation, most of the functionality and fixed the lint issues. Could you trigger another CI run?

I want to add support for generating a patchset and then this PR is ready for review.

I have some ideas for future improvements, e.g. more fine-grained conflict handling or exposing the utilities SQLite provides for inspecting and merging changesets, but they can be added in a backward-compatible manner.

[TheOneTheOnlyJJ]

This is excellent work that many projects will surely be based on. I am curious about the eventual wrapping of the entire Sessions C API (mixed & mashed into higher-level Node functions). Does this PR leave space for that? There are a few more Sessions API functions that would be of great use in Node.

[louwers]

@TheOneTheOnlyJJ Thanks! I agree that exposing more functionality of the Session Extension in Node.js would be great. Here are some ideas:

• Wrappers for sqlite3changeset_invert() (creates a changeset that undoes the changes in another changeset) and sqlite3changeset_concat() (combines changesets).
• Allow closing a session. Right now a session is closed when it is garbage collected or when the corresponding database is closed. It is helpful to be able to manually close session in some cases.
• Allowing the contents of a changeset to be read. You probably want to reuturn a Generator for this so you don't need to read the entire changeset it in memory at once.
• Change the onConflict handler to take a function as well. You would be able to inspect the conflicting change and decide on the action (omit, replace, abort) for each conflicting change.
• A wrapper for sqlite3session_diff().
• Utilize streaming versions of the Session API (you would need another API for Node.js as well). These are helpful for reading or applying changesets that do not fit in memory.

Here is the full API for future reference: https://www.sqlite.org/session/funclist.html

Right now I first want to gather some feedback, but I'd be interested continue working on this, either in a follow-up PR or in this one. Since the SQLite integration is still under active development I think working incrementally is the preferred approach.

[TheOneTheOnlyJJ]

Good point, here's my view on them:

• Wrappers for sqlite3changeset_invert() (creates a changeset that undoes the changes in another changeset) and sqlite3changeset_concat() (combines changesets).

Yes, I was about to suggest these. There's also sqlite3session_isempty() and sqlite3session_memory_used() which are simpler (compared to the rest of the API) and could be wrapped more easily.

• Allow closing a session. Right now a session is closed when it is garbage collected or when the corresponding database is closed. It is helpful to be able to manually close session in some cases.

This should be a high priority right now, as long-running Node processes (like server back-ends) that would use Sessions will have their memory slowly but surely filled up with no way of freeing it without closing the database connection. Applications could run for months or even longer without closing a database connection, so not being able to close Sessions and free their memory would deter developers from using them. The database should, of course, still keep a list of all active sessions and delete them when it is closed, just in case not all Sessions were manually closed. From my point of view, this function should be wrapped as soon as possible and be included in this PR. Not having it surely decreases the chances of getting this merged, and it would be a shame to lose out on this.

• Allowing the contents of a changeset to be read. You probably want to reuturn a Generator for this so you don't need to read the entire changeset it in memory at once.
• Change the onConflict handler to take a function as well. You would be able to inspect the conflicting change and decide on the action (omit, replace, abort) for each conflicting change.
• A wrapper for sqlite3session_diff().

Very important functionalities, but again, they're not vital for achieving functional basic interaction with the extension. These 3 points could be bundled in a follow-up PR that handles the finer-grained data management side of the API. The changegroup could be exposed as an object with its API functions bound to it as methods.

• Utilize streaming versions of the Session API (you would need another API for Node.js as well). These are helpful for reading or applying changesets that do not fit in memory.

I've noticed this in the documentation and it's definitely a part of the API that would be crucial in large apps with large databases. But again, for the current goal of incrementally proposing a stable API that exposes the API of the extension, this is not required right now. It would also require wrapping sqlite3session_config(), which allows configuring SQLITE_SESSION_CONFIG_STRMSIZE. This could be handled after the 3 points above get sorted out.

Also, before I wrap up...

I noticed that you proposed the createSession() method as a wrapper around both sqlite3session_create() and sqlite3session_attach(), combined. Looking at the API, I'm trying to figure out if there's any potential functionality or use case where exposing these functions separately in Node would make sense. The only potential use case I see is creating Sessions in one part of the application and then attaching them somewhere else. Maybe this would be useful if using a global Session that gets created at app start and attached later in the app life cycle? But, given that a Session cannot be unattached, this is likely not going to be useful in any way, so the separation of the functions is not needed. What's you input on this, do you see any potential use cases for the splitting of these 2 functions?

[louwers]

This should be a high priority right now

Agreed. I will still add this one as part of this PR.

What's you input on this, do you see any potential use cases for the splitting of these 2 functions?

I don't see any benefits. Furthermore, I like the philosophy of better-sqlite3:

better-sqlite3's public API must be as simple as possible. Rather than calling 3 functions in a specific order, it's simpler for users to call a single function. Rather than providing many similar functions for doing similar things (e.g., "convenience functions"), there should just be one function that is already convenient by design. Sane defaults should be applied when possible. A function's minimal call signature should be as small as possible, with progressively complex customization available when needed. Function names should only be as long as necessary to convey their purpose. For any new feature, it should be easy to showcase code examples that is are so simple that they are self-explanatory.

In this vein I also oped to default to aborting on conflict, instead of requiring an explicit conflict handler to be passed like in the API of SQLite.

[TheOneTheOnlyJJ]

Agreed. I will still add this one as part of this PR.

Yes, this would be ideal. For further updates other than this, I believe it's best to wait for the final form of the node:sqlite API itself.

I don't see any benefits. Furthermore, I like the philosophy of better-sqlite3:

I was checking if any additional functionality could be squeezed out by separating the functions. Given that there is none, your decision is a sensible one and I support it.

In this vein I also oped to default to aborting on conflict, instead of requiring an explicit conflict handler to be passed like in the API of SQLite.

This is also the best sane default option, as every developer should customize this behavior according to their use case.

I'm not experienced in the internals of Node, so my contribution here can only amount to discussing the API design and smaller implementation details.

I guess the next step now (after implementing the Session close wrapper) is to wait for a maintainer to review this?

[ReillyBrogan]

I arrived here after trying to figure out why we were getting an undefined symbol error after updating to nodejs 22.12.0 on Solus.

Many Linux distributions currently build nodejs with --shared-sqlite so that it uses the system libsqlite, however most of them don't build the session extension currently which will cause the nodejs build to succeed* however it will fail upon launch with a undefined symbol: sqlite3session_delete error. The nodejs build should catch an incompatible system libsqlite and error out with an appropriate message so that this can be caught during build-time.

• I'm not sure how it's succeeding, it should fail during linking if the symbols aren't present in the libsqlite3.so.

[cjihrig]

@louwers can you update the implementation to return errors if Node is built without the session extension?

[louwers]

@cjihrig I will see what I can do.

[louwers]

@cjihrig @ReillyBrogan The docs state that:

It is the responsibility of any distribution
shipping with these options to:

* ensure that the shared dependencies available at runtime
  match what is expected by the Node.js binary. A
  mismatch may result in crashes or unexpected behavior.
* fully test that Node.js operates as expected with the
  external dependencies. There may be little or no test coverage
  within the Node.js project CI for these non-default options.

I'm going to say this works as intended...

[richardlau]

• I'm not sure how it's succeeding, it should fail during linking if the symbols aren't present in the libsqlite3.so.

On main I get link failures if building with --shared-sqlite.

[louwers]

You need to pass SQLITE_ENABLE_SESSION and SQLITE_ENABLE_PREUPDATE_HOOK as defines when compiling SQLite.

[ReillyBrogan]

You need to pass SQLITE_ENABLE_SESSION and SQLITE_ENABLE_PREUPDATE_HOOK as defines when compiling SQLite.

Thank you, I'd already figured this out though. Perhaps this should be added to the release notes for the most recent 22.x and 23.x releases? Just as an informational note for those distributions using a shared sqlite3 (unsupported as it may be).

• I'm not sure how it's succeeding, it should fail during linking if the symbols aren't present in the libsqlite3.so.

On main I get link failures if building with --shared-sqlite.

Ah, I see the issue on our side now. Since nodejs has such a heavy linking phase we were using mold to shave some time off the build, but apparently mold doesn't throw errors on undefined symbols... (rui314/mold#659). I get appropriate errors with ld and lld.

[louwers]

@ReillyBrogan I have made PR to update the changelog. #56318