sqlite: cache column names in StatementSync::All() and Get()#62793
sqlite: cache column names in StatementSync::All() and Get()#62793araujogui wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
There was a problem hiding this comment.
Pull request overview
This PR extends StatementSync’s existing column-name caching (previously used by Iterate()) to also cover StatementSync::All() and StatementSync::Get(), aiming to improve single-row query performance by avoiding repeated V8 string creation for column keys.
Changes:
- Update
StatementExecutionHelper::All()/Get()to accept aStatementSync*so they can access the statement’s cached column-name machinery. - Use
StatementSync::GetCachedColumnNames()inAll()andGet()when returning row objects (non-array mode). - Update call sites (including
SQLTagStore) to use the new helper signatures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/node_sqlite.h |
Adjusts StatementExecutionHelper function signatures to take StatementSync*. |
src/node_sqlite.cc |
Implements column-name caching usage in All()/Get() and updates helper call sites accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sqlite3_stmt* stmt = sync_stmt->statement_; | ||
| int num_cols = sqlite3_column_count(stmt); | ||
| LocalVector<Value> rows(isolate); | ||
| LocalVector<Value> row_values(isolate); | ||
| LocalVector<Name> row_keys(isolate); | ||
|
|
||
| if (!return_arrays && num_cols > 0) { | ||
| if (!sync_stmt->GetCachedColumnNames(&row_keys)) { | ||
| return MaybeLocal<Value>(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
In StatementExecutionHelper::All(), num_cols and the cached row_keys are computed before the first sqlite3_step(). SQLite can auto-reprepare a statement during sqlite3_step() (see SQLITE_STMTSTATUS_REPREPARE), which can change the column count/names; in that case ExtractRowValues() and Object::New(..., num_cols) may use stale num_cols/keys and can lead to a key/value length mismatch (potentially OOB reads in release builds). Consider initializing num_cols/column names only after the first SQLITE_ROW, and/or tracking the reprepare counter during the loop so you can refresh num_cols and rebuild row_keys if the statement was reprepared mid-iteration.
There was a problem hiding this comment.
Hmm that makes sense but I feel it wasn't right before neither. Also this doesn't seem possible in a sync api
CC @nodejs/sqlite
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62793 +/- ##
==========================================
- Coverage 89.69% 89.67% -0.02%
==========================================
Files 706 706
Lines 218270 218250 -20
Branches 41781 41780 -1
==========================================
- Hits 195772 195719 -53
- Misses 14398 14450 +52
+ Partials 8100 8081 -19
🚀 New features to boost your workflow:
|
|
Baseline: New: |
| static v8::MaybeLocal<v8::Value> All(Environment* env, | ||
| DatabaseSync* db, | ||
| sqlite3_stmt* stmt, | ||
| StatementSync* stmt, |
There was a problem hiding this comment.
nit: inconsistency in variable naming we are using sync_stmt for StatementSync
| static v8::MaybeLocal<v8::Value> Get(Environment* env, | ||
| DatabaseSync* db, | ||
| sqlite3_stmt* stmt, | ||
| StatementSync* stmt, |
Cache columns names in
StatementSync::All()andStatementSync::Get()too. Currently, it's cached only onStatementSync::Iterate()This improves single-row query performance (~10%) while leaving multi-row throughput essentially unchanged.