SQLite Async API

[geeksilva97]

Closes #54307

This PR implements an async API for node:sqlite module. So far, it contains a very minimal implementation of exec method, misses some tests, docs and refactoring but it is good enough to share the whole theory I have for it; with that, anybody can share thoughts about it.

• Docs
• Tests

---

Design

On C++ land, I plan to have the Database class determine whether the operations will be asynchronous.

Public API

For the public API, I plan to have classes such as Database, Statement, etc., as counterparts to' DatabaseSync', ' StatementSync', and so on.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[geeksilva97]

Concurrency Control

For concurrency control, SQLite provides a few options. Multi-threaded seems to be a good fit.

Multi-thread. In this mode, SQLite can be safely used by multiple threads provided that no single database connection nor any object derived from database connection, such as a prepared statement, is used in two or more threads at the same time.

We just need a way to guarantee that no two threads will be using connection or statements at the same time. Since we have a threadpool, hence no control over which thread a work will be executed, seems like we need a mutex. Please share your thoughts.

[geeksilva97]

We just need a way to guarantee that no two threads will be using connection or statements at the same time. Since we have a threadpool, hence no control over which thread a work will be executed, seems like we need a mutex. Please share your thoughts.

I wonder if, for the first version of this API if the serialized mode is good enough

Serialized. In serialized mode, API calls to affect or use any SQLite database connection or any object derived from such a database connection can be made safely from multiple threads. The effect on an individual object is the same as if the API calls had all been made in the same order from a single thread. The name "serialized" arises from the fact that SQLite uses mutexes to serialize access to each object.

[codecov]

Codecov Report

❌ Patch coverage is 81.73258% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (08d966c) to head (5bec78e).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	82.53%	49 Missing and 42 partials ⚠️
src/node_sqlite.h	40.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59109      +/-   ##
==========================================
- Coverage   88.54%   88.50%   -0.05%     
==========================================
  Files         703      703              
  Lines      208291   208690     +399     
  Branches    40170    40244      +74     
==========================================
+ Hits       184430   184691     +261     
- Misses      15865    15961      +96     
- Partials     7996     8038      +42     

Files with missing lines	Coverage Δ
src/node_sqlite.h	81.81% <40.00%> (+1.42%)	⬆️
src/node_sqlite.cc	79.56% <82.53%> (-0.37%)	⬇️

... and 41 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]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

[geeksilva97]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

better-sqlite lets the developer decide, suggesting worker threads https://github.com/WiseLibs/better-sqlite3/blob/master/docs/threads.md

[louwers]

I would consider not adding any async APIs, and instead

1. Allow passing threading mode on DatabaseSync construction.
2. Allow setting the threading mode at runtime.
3. Add an example to the documentation how to use SQLite with worker threads.

The overhead of serializing queries and results probably makes this somewhat of an advanced use case. Defaulting to async for all queries is (probably) a mistake for most applications. Instead it is something you might do for a few expensive queries. I could be wrong though.

[geeksilva97]

I would consider not adding any async APIs, and instead

1. Allow passing threading mode on DatabaseSync construction.
2. Allow setting the threading mode at runtime.
3. Add an example to the documentation how to use SQLite with worker threads.

The overhead of serializing queries and results probably makes this somewhat of an advanced use case. Defaulting to async for all queries is (probably) a mistake for most applications. Instead it is something you might do for a few expensive queries. I could be wrong though.

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async. Like fs and fs/promises

[PhilipTrauner]

having implemented worker thread offloading i can state that there's a significant amount of gnarly lifecycle plumbing involved, especially if one wants to support transactions
if some of that plumbing could live within node.js itself it would be much appreciated
not sure where a line would have to be drawn be to support more advanced use cases (e.g. 1 writer, <n> reader setups) but i'm eager to test stuff out!

[geeksilva97]

having implemented worker thread offloading i can state that there's a significant amount of gnarly lifecycle plumbing involved, especially if one wants to support transactions if some of that plumbing could live within node.js itself it would be much appreciated not sure where a line would have to be drawn be to support more advanced use cases (e.g. 1 writer, <n> reader setups) but i'm eager to test stuff out!

Yeah. I will build a solution based on the authorizer, and let's see what you think about it.

[louwers]

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async.

I think the API should reflect this. If it is named Database then I think a lot of people will just go for that one, thinking fewer keystrokes! And async is better/faster! For reading files, async often makes most sense. Correct me if I am wrong, but I think for SQLite queries, most of the time, the overhead makes async the worse option, even measured in wall clock time on the main thread.

Maybe calling it something like DatabaseAsync, StatementAsync is an option?

Or maybe DatabaseSync can be renamed to Database and we can have Database.execAsync?

[louwers]

Is there a performance penalty for purely single-thread use cases?

If so, would it make sense to allow setting the threading mode at runtime?

[geeksilva97]

I don't know for sure about any performance penalti. But as discusses in the issue, due to the node.js nature, this will work fine for sync. Better-sqlite uses it.

[louwers]

Understood. Thanks! This can be closed.

If anything I think this will speed up single-threaded use cases, because apparently by default SQLite uses internal serialization. Did not test it though.

[mcollina]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

The only viable solution is not support custom functions with the async mode. Basically you would need to move the computation back to the main thread, causing a possible deadlock situation.

[geeksilva97]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.
In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.
My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.
@nodejs/sqlite would you have any clue? Thanks in advance.

The only viable solution is not support custom functions with the async mode. Basically you would need to move the computation back to the main thread, causing a possible deadlock situation.

That sounds good to me

[geeksilva97]

Here's the state of this PR by now.

• Async API delegates work to the Thread pool
• Classes don't have a Sync postfix (like zlib), this is what is causing most of the conflicts.
• Custom functions, aggregates and authorizer are not allowed, those functions require to be executed in the main thread.
• Each database instance has a queue for async tasks. With that we ensure only no connection or statement will be used in multiple threads at the same time.
• Iterators remains the same as async iterator helpers are still on stage 2 (https://github.com/tc39/proposal-async-iterator-helpers)

cc @mcollina @cjihrig

[geeksilva97]

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async.

I think the API should reflect this. If it is named Database then I think a lot of people will just go for that one, thinking fewer keystrokes! And async is better/faster! For reading files, async often makes most sense. Correct me if I am wrong, but I think for SQLite queries, most of the time, the overhead makes async the worse option, even measured in wall clock time on the main thread.

Maybe calling it something like DatabaseAsync, StatementAsync is an option?

Or maybe DatabaseSync can be renamed to Database and we can have Database.execAsync?

All of them are good options but the argument of consistency is very strong. Looking at other modules with the perspective of what is exported. Zlib exports functions with sync prefix with the function is sync and without it otherwise.

Maybe this can be a turning point?

I saw that @mcollina added the tsc-agenda tag to the PR #61262. Maybe this naming thing can be discusses as well?

[louwers]

I don't find the consistency argument very strong, because as far as I am aware there is no precedent of postfixing a class with *Sync (versus leaving it out for async). It's only functions.

I also don't think creating a separate Database class that allows asynchronous operations is a particularly good API. It makes it harder to switch between async and sync. Due to the overhead workers incur, I expect many applications will want to only use async for a subset of queries (and ideally be able to switch between one or the other without too much fuss).

In addition, we are staying quite close to the SQLite API, which I think it a good thing to do. The current DatabaseSync class has a single SQLite database connection handle, wheras the proposed (async) Database wraps complicated machinery to juggle multiple database connection handles on different threads. I think the naming should reflect this (call it a database pool or something similar) and offer a more constrained API that makes more sense for the underlying multi-threaded implementation.

I wonder what you think about this API and if it would be feasible.

[geeksilva97]

I don't find the consistency argument very strong, because as far as I am aware there is no precedent of postfixing a class with *Sync (versus leaving it out for async). It's only functions.

That's why I mentioned the perspective. If you look at what is exported, they follow this rule.

I wonder what you think about #54307 (comment) and if it would be feasible.

I like your proposal. It brings flexibility, with the async stuff under the pool attribute. I don't think we need a threadingMode though. We can keep all async under pool anyways.

If we all agree we can drop the Sync postfix - as Bun - I think we can do this.

The machinery will still have complexity because that's how it is. Every work must be splitted to run part on thread pool (SQLite stuff) part on main thread (object creation like arrays and objects, and promise resolving).

[louwers]

I don't think we need a threadingMode though.

Agreed. I thought this would make it possible to use single threaded mode for individual database connections, but this is not possible as pointed out by @BurningEnlightenment.

Glad you like it though!

The machinery will still have complexity because that's how it is.

Yes, but this API makes it clear that it not 'just' a SQLite database handle.