Skip to content

Commit

Permalink
Add test showing that computed SQL queries aren't cached properly.
Browse files Browse the repository at this point in the history
  • Loading branch information
kentonv committed Nov 14, 2024
1 parent 968d761 commit 92bf3b0
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 1 deletion.
21 changes: 21 additions & 0 deletions src/workerd/api/sql-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1012,6 +1012,27 @@ async function test(state) {
await state.blockConcurrencyWhile(async () => {
sql.exec(`PRAGMA foreign_keys = ON;`);
});

// Verify caching.
{
let isCached = (q) => {
let cursor = sql.exec(q);
cursor.toArray();
return cursor.reusedCachedQueryForTest;
};

// Query based on literal string is cached.
assert.equal(false, isCached('SELECT 179321'));
assert.equal(true, isCached('SELECT 179321'));
assert.equal(true, isCached('SELECT 179321'));

// Qeury based on computed string is cached.
assert.equal(false, isCached('SELECT "' + 'x'.repeat(4) + '"'));

// TODO(now): These should be true.
assert.equal(false, isCached('SELECT "' + 'x'.repeat(4) + '"'));
assert.equal(false, isCached('SELECT "' + 'x'.repeat(4) + '"'));
}
}

async function testIoStats(storage) {
Expand Down
2 changes: 2 additions & 0 deletions src/workerd/api/sql.c++
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ void SqlStorage::Cursor::initColumnNames(jsg::Lock& js, State& stateRef) {
// TODO(cleanup): Make `js.withinHandleScope` understand `jsg::JsValue` types in addition to
// `v8::Local`.
KJ_IF_SOME(cached, stateRef.cachedStatement) {
reusedCachedQuery = cached->useCount++ > 0;

KJ_IF_SOME(names, cached->cachedColumnNames) {
// Use cached copy.
columnNames = names.addRef(js);
Expand Down
14 changes: 13 additions & 1 deletion src/workerd/api/sql.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class SqlStorage final: public jsg::Object, private SqliteDatabase::Regulator {
SqliteDatabase::Statement statement;
kj::Maybe<jsg::JsRef<jsg::JsArray>> cachedColumnNames; // initialized on first exec
kj::ListLink<CachedStatement> lruLink;
uint useCount = 0;

CachedStatement(jsg::Lock& js,
SqlStorage& sqlStorage,
Expand Down Expand Up @@ -190,7 +191,7 @@ class SqlStorage::Cursor final: public jsg::Object {
double getRowsWritten();

jsg::JsArray getColumnNames(jsg::Lock& js);
JSG_RESOURCE_TYPE(Cursor) {
JSG_RESOURCE_TYPE(Cursor, CompatibilityFlags::Reader flags) {
JSG_METHOD(next);
JSG_METHOD(toArray);
JSG_METHOD(one);
Expand All @@ -210,6 +211,10 @@ class SqlStorage::Cursor final: public jsg::Object {
one(): T;
columnNames: string[];
});

if (flags.getWorkerdExperimental()) {
JSG_READONLY_PROTOTYPE_PROPERTY(reusedCachedQueryForTest, getReusedCachedQueryForTest);
}
}

JSG_ITERATOR(RowIterator, rows, jsg::JsObject, jsg::Ref<Cursor>, rowIteratorNext);
Expand All @@ -226,6 +231,10 @@ class SqlStorage::Cursor final: public jsg::Object {
tracker.trackField("columnNames", columnNames);
}

bool getReusedCachedQueryForTest() {
return reusedCachedQuery;
}

private:
struct State {
kj::Maybe<kj::Rc<CachedStatement>> cachedStatement;
Expand All @@ -251,6 +260,9 @@ class SqlStorage::Cursor final: public jsg::Object {
// flag an error if the application tries to reuse the cursor.
bool canceled = false;

// Did we reuse a query from the query cache? Tracked for testing purposes.
bool reusedCachedQuery = false;

// Reference to a weak reference that might point back to this object. If so, null it out at
// destruction. Used by Statement to invalidate past cursors when the statement is
// executed again.
Expand Down

0 comments on commit 92bf3b0

Please sign in to comment.