sqlite: mark as release candidate

[mcollina]

Mark the SQLite module as release candidate and remove the experimental warning.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (ce2ec3d) to head (c02731d).
⚠️ Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61262      +/-   ##
==========================================
- Coverage   88.54%   88.53%   -0.01%     
==========================================
  Files         704      704              
  Lines      208734   208756      +22     
  Branches    40271    40277       +6     
==========================================
+ Hits       184823   184830       +7     
- Misses      15932    15936       +4     
- Partials     7979     7990      +11     

Files with missing lines	Coverage Δ
lib/sqlite.js	100.00% <ø> (ø)

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

[cjihrig]

LGTM. I still think there should be a definitive decision made about an async API before stabilizing it though because the presence or lack of an async API could influence the sync API.

[louwers]

What does this mean exactly? Are breaking changes still possible?

We still need to flip the default of defensive mode #60217. Created a PR.

[tpoisseau]

The API looks good for a release candidate.

[mcollina]

LGTM. I still think there should be a definitive decision made about an async API before stabilizing it though because the presence or lack of an async API could influence the sync API.

@cjihrig Is there an issue to track this?

I personally don't think we need an Async API for the generic case. However, something like https://github.com/mcollina/sqlite-pool to provide concurrent read transactions might be interesting to support. https://www.sqlite.org/lang_transaction.html

Having said that, the API to support this would need to be significantly different.

[mcollina]

What does this mean exactly? Are breaking changes still possible?

@louwers yes, breaking changes would still be possible but we'd try to avoid them unless there are major usability issues.

[louwers]

I don't know how much weight my vote counts, but I would definitely wait until the async API (#59109) has landed before marking it as a RC.

I personally think DatabaseSync should be renamed to Database, because synchronous should be the default since 9 out of 10 times it will be more performant than using the (future) asynchronous API.

[mcollina]

Moving to stabilizing what we have does not prevent new features to be added. Keeping api experimental forever is not really good for the project.

[cjihrig]

The tracking issue is at #54307.

I personally think DatabaseSync should be renamed to Database, because synchronous should be the default since 9 out of 10 times it will be more performant than using the (future) asynchronous API.

This is the type of thing I meant. The reason the name DatabaseSync was chosen was to follow the naming convention used in other Node core modules. If there is going to be an async API, it would be nice to keep those conventions and make the two APIs as similar as reasonable. If there is not going to be a dedicated async API, then yes, it probably makes sense to just name it Database. Personally, I originally thought we should have an async API, but I've changed my mind about that recently.

[geeksilva97]

LGTM. I still think there should be a definitive decision made about an async API before stabilizing it though because the presence or lack of an async API could influence the sync API.

@cjihrig Is there an issue to track this?

I personally don't think we need an Async API for the generic case. However, something like https://github.com/mcollina/sqlite-pool to provide concurrent read transactions might be interesting to support. https://www.sqlite.org/lang_transaction.html

Having said that, the API to support this would need to be significantly different.

The issue is: #54307.

I took longer than I expected, but I've been working on it. I love the reference to the sqlite-pool, it's something I added due to the nature of sqlite with concurrent operations.

I should, finally, have a PR for review by the end of this week

[cjihrig]

I think SQLite is different, the sync version is the primary and recommended use case.

I think trying to maintain consistency within core is a good thing. And, with all due respect, that is just your opinion. While I do agree with you, I have seen plenty of people complain about node:sqlite blocking the event loop.

[louwers]

I think trying to maintain consistency within core is a good thing. And, with all due respect, that is just your opinion.

We could test it. I think for a lot of queries, the serialization overhead to and from a worker thread is larger than running the query in the main thread. Guiding people to use the recommended API (in my opinion) is more important than consistency. Is it really more consistent though? Is there another class with a Sync postfix?

I am not a fan of the API where you have separate DatabaseAync, StatementAsync, SQLTagStoreAsync classes (by the way, why is the existing SQLTagStore not SQLTagStoreSync?). I don't know if a StatementAsync class even makes sense (@geeksilva97 probably thought about this) because it cannot have any correspondence to a SQLite prepared statement, because those can only be used on one thread (in multi-thread mode), and presumably the thread it gets run on is only determined when it is run. What does DatabaseAsync.prepare() do?

Maybe we could rename DatabaseSync to Database and and allow retrieving a handle to a database pool with an API that fits async better:

const { Database } from 'node:sqlite';

const db = new Database('...', {
  threadingMode: 'multiThread'
});
// will fail if threadingMode is 'singleThread'
const dbPool = db.pool; // DatabasePool instance

await db.pool.exec('...');  // async
db.exec('...');  // sync

Another advantage would be that is that it's easier to use both sync and async queries from the same DB handle.

[cjihrig]

Is it really more consistent though? Is there another class with a Sync postfix?

I'm not sure if there are any classes, but IMO it's definitely more consistent with, for example, the fs, child_process, and zlib APIs which use Sync to indicate synchronous and no suffix to indicate asynchronous. The reason I added the Sync on the class name is because I don't think we can design an efficient database connection that does both sync and async work. We could add Sync to the individual method names, but that seems worse.

I'll stop commenting on this discussion though.

[louwers]

Let's move the discussion to #54307

[louwers]

@Waldenesque I think you offer a valuable viewpoint. The async API not being ready should not be a blocker for stabilizing the SQLite API, but we should at least agree on what the extension point will be when and if an async API is added. For example if we should have a unified Database class for async and sync operations, or introduce a separate class.

[mike-git374]

I am requesting more attention to this other proposed API change before everyone votes to mark sqlite as release candidate: db.prepare(sql, options) Proposal 2. Breaking change is remove statement.set* functions, add db.prepare(sql, options)

As @Waldenesque said:

My suspicion is this API will eventually become the new de facto standard, so now is the last chance before "coulda woulda shouldas."

[joyeecheung]

The stability index definition suggests:

1.2 - Release candidate. Experimental features at this stage are hopefully ready to become stable. No further breaking changes are anticipated but may still occur in response to user feedback or the features' underlying specification development. We encourage user testing and feedback so that we can know that this feature is ready to be marked as stable.

So if the worry is that "there will be changes", 1.2 does not forbid that and already warns about it. Personally my understanding of the difference between 1.1. and 1.2 is "how certain you are about the likelihood of changes - >x% or < x%?" (different people might have different x, I'd say 20% is my personal threshold). If the worry is that it will encourage usage and make changes more difficult, perhaps it's worth gauging how many users are already using to make it difficult already.

Note that breaking changes even after a feature reaches stability are still possible. Just that they will have to be semver-major (in experimental stages, that's not a hard requirement, but can still be done out of caution).

[louwers]

@geeksilva97 @mcollina Should we rename DatabaseSync to Database and then merge this PR?

If you think it is a good idea to do so, would you want to make the PR or shall I do it?

Then we can create an async API that does not use a separate class.

[geeksilva97]

@geeksilva97 @mcollina Should we rename DatabaseSync to Database and then merge this PR?

If you think it is a good idea to do so, would you want to make the PR or shall I do it?

Then we can create an async API that does not use a separate class.

I think it makes sense. It will make my life a lot easier with the async stuff, too.

[Waldenesque]

I'm really struggling to decide if I should say anything or just wait. Let me try this...

@cjihrig you named this in the first place, yet you have since changed your original assumptions. @mcollina originally made the same assumption as you (start with a sync API, then optionally add an async variation of that API), yet likewise has since changed his thinking to favor what I'll term a "background pooler" rather than a "full async API." In theory I imagine there could be legitimate arguments either way for a "full async API" versus a more modest "background pooler" of some sort.

I'm a pragmatist. To me the most relevant consideration is not theoretical ideals, but rather the availability of a volunteer who is actually willing to work on something. Since @geeksilva97 has already put meaningful effort into an async API PR, I think that demonstrated reality deserves far more weight than any abstract debate over "full async API" versus more limited "background pooler." If @geeksilva97 is willing to work on a full async API implementation, why not take that path? There's no hurry for the async part, it's fine if it takes some more time to work out the exact implementation details, the main thing is just making sure it will mesh with the sync API before committing this release candidate PR.

Right now that just means removing the "Sync" suffixes, and then the sync API can move forward toward stabilization. This feels like a reasonably safe bet, because even if a different decision is made down the road to go with the simpler "background pooler" approach instead, that interface could always use some variation of "pool" or "pooler" for its naming. It seems like what @louwers is suggesting is probably a decent way forward.

So @cjihrig what do you think about this?

[cjihrig]

So @cjihrig what do you think about this?

Just to be clear, I'm not blocking anything here, nor do I plan to. I was only sharing my opinion.

My opinion is that if there are two similar/identical APIs, one sync and one async, then the synchronous one should have the Sync suffix because that is how Node has always named APIs. If there aren't going to be two similar APIs, then it doesn't matter at all. Other than that, I don't care about the naming. I don't think we need a dedicated async API, but having one is fine if someone else is doing the work.

Side note: if DatabaseSync does get renamed, I think it would be good to leave a deprecated alias in place to avoid breaking existing users until the new name exists on all supported release lines.

[Waldenesque]

@cjihrig thank you. In general I agree maintaining historical consistency with established and widely expected patterns is wise (indeed SQLite itself is pretty strict about this). However there is a counter argument...

SQLite is a unique piece of software in this field, so it's sensible to treat it uniquely. The most obvious example is the very fact that Node is shipping built-in SQLite in the first place. When this was first proposed one of the arguments against it was that Node has historically not included anything like this, leaving such support for npm addons. That objection was overridden because the benefits of built-in SQLite outweighed historical precedent. And by the way on a personal note, thank you @cjihrig for creating this since I think having SQLite built-in is sweet!

I think this "exception to the rule" should also apply to the API. This isn't just another crypto algo or compression format or something, juggled in mind alongside dozens more situationally relevant tools, only needed intermittently so consistent patterns ease usage. This is SQLite: its fans keep an explicit place for it in their mind, exercised often enough for comfortable usage without borrowing familiarity from other tools. Others copy from Stack Overflow or ChatGPT anyway, so imitating general precedent makes less sense for SQLite. I'd rather see independent standalone decisions driven by how this unique tool will actually be used, which likely means "sync, plus occasional special handling for slow queries."

In short, if the design caters to SQLite fans, they will be delighted, while historical inconsistency will almost surely be overlooked by all but a few killjoys who complain about everything anyway. If the design caters to abstract historical expectations, nobody will be delighted, yet you won't even get credit from the connoisseurs of complaint, because they're always on the lookout for their next disappointment.

I anticipate that's how this plays out. So who does everyone hope to please? People who will be happy? Or people who can never be happy?

[louwers]

has changed his thinking to favor what I'll term a "background pooler" rather than a "full async API."

Even if you would create an API that is method for method identical to DatabaseSync, but with all methods async (which would not make sense), it would still need to use a background pool.

My opinion is that if there are two similar/identical APIs, one sync and one async, then the synchronous one should have the Sync suffix because that is how Node has always named APIs.

I can understand this reasoning. The difference in this case is that this is not an API, but a class which contains multiple APIs. So that makes it a lot less clean to apply this naming convention here. Note that by this logic we should rename SQLTagStore to SQLTagStoreSync, because we probably want to have a SQLTagStore handle that uses a pool.

The main reason why I am against naming a database pool handle Database is while there will be some overlap in methods with DatabaseSync, it will be very different, and not just internally. SQLite also does not have an async API. We have stayed pretty close to the SQLite API, even referring to the SQLite docs in our docs. Going with Database for a database pool handle feels like we are trying to obscure the implementation, which I would argue is not helpful in this case.

We could also decide to just keep the name DatabaseSync, even if we end up not naming a database pool handle Database. Easier to grep anyway. 😉

[mceachen]

Like it’s been said before several times, SQLite is fundamentally a sync API. Pretending that it’s async is a footgun-rich facade—unexpectedly overlapping transactions, save points, queries, and iterations will cause a litany of surprising and hard to debug issues for your users.

An alternative async API that would provide a db handle of which the caller must then manage coherent actions (like you’d expect from, say, any other external rdbms) would properly push this complexity to the user. Has this been discussed? For large DB, each new handle may require substantial RAM for cache/mmap—that’s not a show stopper, but should be documented as it may be surprising to the uninitiated.

(FWIW I’ve helped maintain better-sqlite3 for several years and have also written a drop-in for node:sqlite to help make it easier to migrate to this API: @photostructure/sqlite. My point is that I’ve spent quality time with this API, and appreciate the time and care that has gone into node:sqlite—kudos to the contributors!)

[geeksilva97]

SQLite is fundamentally a sync API. Pretending that it’s async is a footgun-rich facade

I don't think pretending it will be async is the goal.

[mceachen]

SQLite is fundamentally a sync API. Pretending that it’s async is a footgun-rich facade

I don't think pretending it will be async is the goal.

Offense certainly wasn’t intended.

My guess is that if you asked a normal node user, (especially if they’ve interacted with if MySQL or Postgres) if an async call could block another async call, I suspect that they would be surprised. Any async api will require careful verbiage to describe the footguns.