sqlite: expose prepared statement statistics - #64541
Conversation
|
Review requested:
|
f5e8fcb to
2195e97
Compare
|
I'm a little uncomfortable with getters that create an object every time they're read. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64541 +/- ##
==========================================
+ Coverage 90.31% 90.33% +0.01%
==========================================
Files 760 760
Lines 248532 248564 +32
Branches 46908 46909 +1
==========================================
+ Hits 224467 224534 +67
+ Misses 15505 15456 -49
- Partials 8560 8574 +14
🚀 New features to boost your workflow:
|
bbbbeec to
bef2511
Compare
bef2511 to
fd5654c
Compare
|
Please @nodejs/sqlite , share your thoughts |
araujogui
left a comment
There was a problem hiding this comment.
Another method to reset stats would be nice to have
| {"reprepare", SQLITE_STMTSTATUS_REPREPARE}, | ||
| {"run", SQLITE_STMTSTATUS_RUN}, | ||
| {"filterMiss", SQLITE_STMTSTATUS_FILTER_MISS}, | ||
| {"filterHit", SQLITE_STMTSTATUS_FILTER_HIT}, |
There was a problem hiding this comment.
SQLITE_STMTSTATUS_FILTER_MISS and SQLITE_STMTSTATUS_FILTER_HIT was introduced in SQLite 3.38.0.
3.37.2: https://github.com/sqlite/sqlite/blob/version-3.37.2/src/sqlite.h.in
3.38.0: https://github.com/sqlite/sqlite/blob/version-3.38.0/src/sqlite.h.in
There was a problem hiding this comment.
So maybe we need to enforce a minimum SQLite version on --shared-sqlite
There was a problem hiding this comment.
What do u suggest? A CHECK or something?
There was a problem hiding this comment.
I added some ifdefs. Let's see what the team says
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
fd5654c to
b0a266e
Compare
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
Signed-off-by: geeksilva97 <edigleyssonsilva@gmail.com>
|
Please @araujogui . Let me know if you have any other concerns |
trivikr
left a comment
There was a problem hiding this comment.
Verified against a rebuilt PR head. One issue is a reproducible native crash; the other comments correct the documented SQLite semantics.
| // sqlite3_stmt_status() resets a single counter per call, so every exposed | ||
| // counter is visited. The returned value is the pre-reset one and is unused. | ||
| for (const auto& info : kStatusMapping) { | ||
| sqlite3_stmt_status(stmt->statement_, info.sqlite_status_id, true); |
There was a problem hiding this comment.
Resetting SQLITE_STMTSTATUS_REPREPARE desynchronizes the iterator column-name cache. After caching names, altering the schema, calling resetStats(), and altering it again, the counter returns to the cached value and iterate() reads past the stale key array; I reproduced exit code 139. Please invalidate the cache and its generation when resetting stats. For example:
void StatementSync::InvalidateColumnNameCache() {
cached_column_names_.clear();
+ cached_column_names_reprepare_count_ = -1;
}
@@
for (const auto& info : kStatusMapping) {
sqlite3_stmt_status(stmt->statement_, info.sqlite_status_id, true);
}
+ stmt->InvalidateColumnNameCache();| Resets every counter reported by [`statement.stat()`][] back to zero. This | ||
| method is a wrapper around [`sqlite3_stmt_status()`][] and is useful for |
There was a problem hiding this comment.
SQLITE_STMTSTATUS_MEMUSED is not a counter, and SQLite ignores resetFlg for it. I verified that it remains 3856 before and after resetStats(), so this contract cannot hold.
| Resets every counter reported by [`statement.stat()`][] back to zero. This | |
| method is a wrapper around [`sqlite3_stmt_status()`][] and is useful for | |
| Resets every counter reported by [`statement.stat()`][] back to zero, except | |
| `memused`, which reports current memory usage and cannot be reset. This |
| prepared statement. | ||
| * `'reprepare'` The number of times the statement has been automatically | ||
| reprepared due to schema changes or changes to bound parameters. | ||
| * `'run'` The number of times the statement has run to completion. |
There was a problem hiding this comment.
SQLITE_STMTSTATUS_RUN increments on the first sqlite3_step(), not when execution completes; a partially consumed iterator increments it too.
| * `'run'` The number of times the statement has run to completion. | |
| * `'run'` The number of execution cycles started by the prepared statement. |
| suite('StatementSync.prototype.resetStats()', () => { | ||
| test('returns undefined', (t) => { |
There was a problem hiding this comment.
Regression rest for the suggestion from #64541 (comment)
| suite('StatementSync.prototype.resetStats()', () => { | |
| test('returns undefined', (t) => { | |
| suite('StatementSync.prototype.resetStats()', () => { | |
| test('invalidates cached iterator column names', (t) => { | |
| using db = new DatabaseSync(':memory:'); | |
| db.exec('CREATE TABLE data(a); INSERT INTO data VALUES (1)'); | |
| const stmt = db.prepare('SELECT * FROM data'); | |
| db.exec('ALTER TABLE data RENAME COLUMN a TO b'); | |
| stmt.iterate().toArray(); | |
| stmt.resetStats(); | |
| db.exec('ALTER TABLE data RENAME COLUMN b TO c'); | |
| t.assert.deepStrictEqual(stmt.iterate().toArray(), [ | |
| { __proto__: null, c: 1 }, | |
| ]); | |
| }); | |
| test('returns undefined', (t) => { |
Closes #64540