sqlite: add virtual table support via createModule() - #65787
sqlite: add virtual table support via createModule()#65787TrevorBurnham wants to merge 2 commits into
Conversation
|
Review requested:
|
7862ab0 to
69f939a
Compare
Expose SQLite's virtual table API through a new `database.createModule(name, options)` method, wrapping `sqlite3_create_module_v2()`. This enables read-only virtual tables backed by JavaScript data sources, usable either as an eponymous table (`SELECT * FROM module_name`) or via `CREATE VIRTUAL TABLE t USING module_name`. Hidden columns pass parameters using table-valued function syntax (`SELECT * FROM module_name(param1, param2)`). `options` accepts `columns`, `rows`, `directOnly`, and `useBigIntArguments`. Column types are validated against INTEGER, TEXT, REAL, BLOB, and ANY, and column names are quoted when building the `sqlite3_declare_vtab()` schema. Rebased from nodejs#61544, which was opened by byteforge38 and became inactive. Changes on top of that work: - xColumn reports the value each hidden column was constrained to, rather than NULL. SQLite treats xBestIndex's `omit` as a hint, so it may recheck a constraint it already handed to xFilter; against NULL that recheck rejected every row, and `gs(1, 3) WHERE start = 1` returned no rows. - xBestIndex lowers estimatedCost as it consumes constraints. With a constant cost the planner was free to pick the unconstrained plan and recheck afterwards, so a correlated parameter such as `FROM t, gs(t.a, t.a + 1)` also returned no rows. - Violations of the iteration protocol report a SQLite error instead of calling PropagateJSError with no JavaScript exception pending. That left `.all()` returning undefined and `exec()` reporting success. - xBestIndex passes the constrained hidden-column indices to xFilter through idxStr rather than an int bitmask, which previously aliased for parameter indices at or above the width of an int. - xFilter, xNext, and xColumn take a CallbackDepthGuard. Without it close() from inside rows(), an iterator's next(), or a row getter finalized the statement that SQLite was still stepping, crashing the process. - xClose calls the iterator's return() method so generator `finally` blocks run when SQLite stops stepping early, as it does for LIMIT or a `break` out of a for...of loop. It is skipped while tearing down from ~StatementSync or ~DatabaseSync, which run from garbage collection callbacks where JavaScript cannot be executed; an abandoned generator does not run `finally` in JavaScript either. It is also skipped when an error is already pending, so that error still reaches the caller. - VirtualTableModule holds a BaseObjectWeakPtr<DatabaseSync> to match UserDefinedFunction instead of a raw pointer. - createModule() rejects being called from an authorizer callback. - Documents that values yielded by rows() follow the usual conversion rules, so a number is stored as REAL and a BigInt as INTEGER even when a column declares INTEGER, since virtual tables do not apply column affinity to the values they return. Refs: nodejs#61544 Refs: nodejs#63826 Fixes: nodejs#61539 Co-authored-by: byteforge38 <stormcraft318@gmail.com> Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
69f939a to
89b3a3a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65787 +/- ##
==========================================
+ Coverage 90.13% 90.17% +0.03%
==========================================
Files 769 771 +2
Lines 261645 265529 +3884
Branches 49671 50432 +761
==========================================
+ Hits 235831 239432 +3601
- Misses 16845 17000 +155
- Partials 8969 9097 +128
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| schema_sql += ")"; | ||
|
|
There was a problem hiding this comment.
An options getter can close the database after the initial state check, causing createModule() to segfault.
Please add this check after reading all options and column definitions
| // Options and column getters may have closed the database. | |
| THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); | |
Also add a regression test expecting ERR_INVALID_STATE for this case.
| // Re-throw so that an error already pending when SQLite unwound into | ||
| // xClose still reaches the caller, and so that a throwing `finally` is not | ||
| // silently discarded. | ||
| if (try_catch.HasCaught() && !try_catch.HasTerminated()) { | ||
| mod->PropagateJSError(); |
There was a problem hiding this comment.
When iterator cleanup throws, xClose() sets the SQLite error suppression flag even though SQLite ignores its return value. This leaves the flag set and silently swallows the next unrelated SQL error.
We should re-throw cleanup exceptions directly.
| // Re-throw so that an error already pending when SQLite unwound into | |
| // xClose still reaches the caller, and so that a throwing `finally` is not | |
| // silently discarded. | |
| if (try_catch.HasCaught() && !try_catch.HasTerminated()) { | |
| mod->PropagateJSError(); | |
| // Re-throw cleanup exceptions directly. SQLite ignores xClose's return | |
| // value, so there is no corresponding SQLite error to suppress. Setting | |
| // ignore_next_sqlite_error_ here would suppress an unrelated later error. | |
| if (try_catch.HasCaught() && !try_catch.HasTerminated()) { |
Please add a regression test for this.
Address review feedback from trivikr. createModule() re-checks that the database is still open after reading the options bag and the column definitions. A property getter there runs user JavaScript, so close() could release the connection before sqlite3_create_module_v2() received it, and SQLite dereferenced the null handle. Seven other entry points in this file already carry the same re-check, and test-sqlite-options-getter-reentry.js covers the hazard for nine APIs; createModule() was the one that skipped it. xClose no longer calls PropagateJSError when the iterator's return() throws. SQLite discards xClose's return value, so no SQLite error is generated for the suppression flag to consume; it stayed set until the next unrelated statement, where prepare() returned undefined instead of throwing ERR_SQLITE_ERROR. A SQLite error that does follow the cleanup throw now surfaces instead of being suppressed in favor of the JavaScript error. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com> Assisted-by: Claude Opus 5
Fixes #61539
This PR is a continuation of #61544 by @byteforge38, which became inactive.
It exposes SQLite's virtual table API through a new
database.createModule(name, options)method, wrappingsqlite3_create_module_v2().This PR enables read-only virtual tables backed by JavaScript data sources. A registered module can be used two ways:
SELECT * FROM module_name).CREATE VIRTUAL TABLE t USING module_name.Hidden columns pass parameters via table-valued function syntax (
SELECT * FROM module_name(param1, param2)).optionsacceptscolumns,rows,directOnly, anduseBigIntArguments.Note: column affinity
A virtual table doesn't apply column affinity to the values
xColumnreturns, which makes declared types behave differently than they do on an ordinary table:This follows from the conversion rules discussed in #63826: A
numberis bound asREAL, abigintasINTEGER. Value-based coercion would make it impossible to yield666.0into aREALcolumn, andbigintalready gives callers explicit control.Coercing to the declared type would be a different mechanism, driven by explicit intent rather than by guessing from the value, and it would leave
REALcolumns alone. But SQLite core doesn't do this for virtual tables (generate_seriesreturns integers because it callssqlite3_result_int64, not via affinity), and it adds per-cell cost.I've documented the current behavior rather than changing it, since coercing to the declared type is behavior SQLite core doesn't have for virtual tables.