Reset row counters when creating Query using templates #2704
Conversation
| @@ -509,7 +509,9 @@ KJ_TEST("SQLite read row counters (basic)") { | |||
| constexpr int dbRowCount = 1000; | |||
| auto insertStmt = db.prepare("INSERT INTO things (id, unindexed_int, value) VALUES (?, ?, ?)"); | |||
| for (int i = 0; i < dbRowCount; i++) { | |||
| insertStmt.run(i, i * 1000, kj::str("value", i)); | |||
| auto query = insertStmt.run(i, i * 1000, kj::str("value", i)); | |||
There was a problem hiding this comment.
As far as I can tell, this new test code only tests the Query(SqliteDatabase& db, const Regulator& regulator, Statement& statement, Params&&... bindings) (on line 404 in sqlite.h).
You've also changed Query(SqliteDatabase& db, const Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings) (from line 412). This version of the constructor is called by countRowsTouched. How come the test was passing before adding the code? I think it's because countRowsTouched makes a new implicit statement on every call, rather than reusing the statement.
Is there a test we can add that tickles the problem and is fixed by adding the resetRowCounters(); statement?
There was a problem hiding this comment.
Looking at this a bit more... why do we have the resetRowCounters() in Query(SqliteDatabase& db, const Regulator& regulator, kj::StringPtr sqlCode, Params&&... bindings) (from line 412)? We're creating a new statement from the SQL code.
There was a problem hiding this comment.
The kj::StringPtr sqlCode version always prepares a new statement from scratch hence its counts always start from zero anyway, so there was no bug in that version of the constructor.
There was a problem hiding this comment.
We don't need the reset counters in the constructor which creates the statement, only for the one that takes in a statement. I updated the code accordingly
|
Also, it's good practice to create an internal PR that updates workerd to make sure that no internal tests fail. |
src/workerd/util/sqlite.h
Outdated
| @@ -413,6 +414,7 @@ class SqliteDatabase::Query final: private ResetListener { | |||
| regulator(regulator), | |||
| ownStatement(db.prepareSql(regulator, sqlCode, 0, MULTI)), | |||
| maybeStatement(ownStatement) { | |||
| resetRowCounters(); | |||
There was a problem hiding this comment.
This line isn't needed because in this constructor the statement is always newly-constructed so the counts will already be zero.
Only the other constructor, which takes a Statement& that may have been used before, needs to reset the counters.
Yep! I had some git permissions issues so couldn't post that PR, but will send out the edgeworker PR with the deps update |
shrima-cf
force-pushed
the
shrima/fix-resetCounters-sqlite
branch
from
6fe5471 to
3756650
Compare
Reset the row counters when we create Query, so we calculate rowsRead and rowsWritten correctly.