Repository navigation
sqlite async API #54307
Description
Activity
@ronag can you add "why" to the issue? (You can copy it from the original discussion).
Reacted by Robert Nagy and Alex YangHey, @ronag . To make this happen, Would there be a
DatabaseAsync- in contrast to the DatabaseSync - class?Hey, @ronag . To make this happen, Would there be a
DatabaseAsync- in contrast to the DatabaseSync - class?This doesn't make so much sense. I think it would be better to have
Databasejust like fs that has someFunction and someFunctionSync.Just FYI, this appears to be the stance of better-sqlite3 on the topic - https://github.com/WiseLibs/better-sqlite3/blob/HEAD/docs/threads.md.
A detailed description of a use case which would benefit from offloading to a thread pool can be found here:
Reacted by Edy Silva and Matthew McEachenA detailed description of a use case which would benefit from offloading to a thread pool can be found here:
IMHO, this link clarifies the use cases and the necessity for an async API.
That said, I'd love to start working on this. What do you say, @cjihrig?
Heads up, I started working on it. I will open PR by next month to get some feedback.
Reacted by Cody Casterline, Santiago Aguilar Hernández, 0hm☘️, Alex Yang, Opportunity, Tobias Petry, Roscoe Rubin-Rottenberg and Claudio WunderHeads up, I started working on it. I will open PR by next month to get some feedback.
this is awesome. cant wait for that. let me know if you have the PR. i will defenitely test it
Reacted by Edy SilvaHeads up, I started working on it. I will open PR by next month to get some feedback.
this is awesome. cant wait for that. let me know if you have the PR. i will defenitely test it
For sure, will defintely need help 😅
1 remaining item
Picking up the discussion from #61262.
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
@cjihrig Could you elaborate on that? Because as I see it, when in multi-thread mode, the main thread is 'just another thread', so I don't know what would be inefficient about it.
Do you see any problems with this:
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
And perhaps:
const sql = db.createTagStore(); const rows = sql.all`SELECT * FROM widgets`; // sync const rows = await sql.pool.all`SELECT * FROM widgets`; // async
So, in this case,
dbanddb.poolare basically separate database connections. It's not necessarily a bad API. But, what I was referring to was the fact that async access requires a different mode of execution for SQLite (and possibly extra synchronization in Node depending on the implementation) that is less efficient than the single threaded model. I just don't want to see the synchronous performance take a hit to accommodate an async API. Other than that, I'm sure whatever the maintainers come up with will be fine.Reacted by Bart Louwers@cjihrig Technically
db.poolwould be an abstraction over the database connections on however many threads your pool has.less efficient than the single threaded model
Yes! Hence the
threadingModeflag onDatabaseconstruction. I don't think allowing synchronous on a multi-thread enabled database handle is a performance issue, but defaulting to enabling multi-thread for all connections definitely is problematic for purely synchronous use cases.Reacted by Colin Ihrig and Claudio WunderPlease read the official SQLite documentation regarding threading mode. A
threadingModeconstructor option is at least misleading, because single vs multi threading mode in the SQLite sense is a compile (and startup) time option which should always be set to multi-thread (not serialized) in nodejs, otherwise it would be unsafe to create database connections from nodejs worker threads.If one wants to provide an option to preclude the creation of a db connection pool with automatic thread pool dispatching, the option should be named to reflect exactly that.
@BurningEnlightenment Since you can override the compile time option at runtime, the way I imagine it is if you specify
threadingMode: 'single', multi-threading is not available for that database connection. Whereas you might usethreadingMode: 'searialized'if you want to build your own worker implementation and let SQLite handle the synchronization.@louwers no, you can't. To quote the relevant part of the documentation:
If single-thread mode has not been selected at compile-time or start-time, then individual database connections can be created as either multi-thread or serialized. It is not possible to downgrade an individual database connection to single-thread mode. Nor is it possible to escalate an individual database connection if the compile-time or start-time mode is single-thread.
So either the whole process is in single-threaded mode or it isn't.
Alright! So we could use
threadingMode, but then (by default) the only available options would bemultiThreadorsearialized. I wonder if there is any performance penalty for single-threaded use cases when compiling SQLite withSQLITE_THREADSAFE=2. Since SQLite does not do synchronization I suppose not?Otherwise a Node.js runtime flag to could be introduced to choose single-threaded SQLite at start time. If enabled then
threadingMode: singlehas to be chosen, and it is not available otherwise.Serialized mode isn't really useful if one follows the pool model proposed above. Serialized mode is for cases where a single sqlite database connection is used from multiple threads without external synchronization.
DatabaseSyncobjects currently can't (and shouldn't) be shared between main and worker threads, i.e. it's not useful in this case. (Is DatabaseSync even transferable btw?). And in case of the proposed.poolAPI one can design the API in a way to ensure that only ever one concurrent thread accesses a single database connection. External synchronization via API invariants is way more efficient than relying on the internal sqlite db connection mutex.Otherwise a Node.js runtime flag to could be introduced to choose single-threaded SQLite at start time. If enabled then threadingMode: single has to be chosen, and it is not available otherwise.
@louwers Note that if single threaded mode is enabled via runtime flag, you'd also have to prevent worker threads from creating db connections.
Reacted by Bart Louwers, Edy Silva and Claudio WunderAlright! So we could use
threadingMode, but then (by default) the only available options would bemultiThreadorsearialized. I wonder if there is any performance penalty for single-threaded use cases when compiling SQLite withSQLITE_THREADSAFE=2. Since SQLite does not do synchronization I suppose not?Otherwise a Node.js runtime flag to could be introduced to choose single-threaded SQLite at start time. If enabled then
threadingMode: singlehas to be chosen, and it is not available otherwise.I don't see a reason not to set SQLITE_THREADSAFE to 2. In the current implementation, there's no way to have multiple threads working on the same connection. That's how better-sqlite set it, by the way.
For an async API, the serialization can be done in a queue per connection.
Given that @geeksilva97 stopped working on it, I'd like to pick up the work. Some use cases (as has already been pointed out) can be well served by delegating db accesses to worker threads, there are some scenarios where this either isn't tenable or has considerable downsides:
- Pooling many concurrent SQLite DB connections in a similar fashion to the libuv thread pool for IO bound queries can't be easily achieved with workers as it is expensive (and currently not implemented) to move DB connections between worker threads.
- Spawning a worker thread per DB connection incurs considerable memory overhead and setup latency penalties which makes it infeasible for opening hundreds of connections to different databases. A use case for this is described in the previously mentioned kysely issue.1
Therefore, I still think it is well justified to have an async implementation which dispatches to the libuv thread pool. However, as the previous implementation attempt has shown that there is a feature set which can not (and should not) be shared with an asynchronous db connection, I'd like to go in a slightly different direction regarding the API, namely introducing separate
DatabaseandStatementclasses with a comparatively slim and focused feature set. Whereas everything introducing user callbacks into sql queries (e.g..function) is left out to avoid the complexity which arises from the then necessary synchronization. In contrast tonode-sqlite3I want to implement a batching mechanism and avoid the pseudo-parallelization non-sense, i.e. the implementation should behave similar tonode-sqlite3's serial mode, but without the inefficiency of creating a thread pool work item per query.Footnotes
The implementation in #62015 has reached MVP state and I'd like to get some feedback on whether the implementation approach is deemed acceptable before investing time in writing the documentation.
This issue has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.Reacted by Matthew McEachen, Andy Bao and Krzysztof Rosiński🪄 unstale
Provide an async API to our sqlite bindings. In order to avoid blocking the event loop on lookups that require IO.