Skip to content

Commit

Permalink
SQLite: Fix missing RELEASE of savepoinst in transactionSync().
Browse files Browse the repository at this point in the history
`ROLLBACK TO` does not release the savepoint, just rolls back to the point immediately after the savepoint was created. We must additionally release it.

This never caused a problem becaues:
1. `transactionSync()` always runs inside an implicit transaction. Committing or rolling back the implicit transaction implicitly releases all the savepoints.
2. It turns out SQLite allows savepoints with the same name as previous ones. The name always refers to the most-recent savepoint by that name which hasn't been released. So, the "leaked" savepoint didn't prevent a new savepoint with the same name from being created later.

Note that the async version of `transaction()` does release savepoints properly.

I also added a TODO to take advantage of the fact that savepoints can have the same name...
  • Loading branch information
kentonv committed Oct 10, 2024
1 parent 7458c40 commit ed88b15
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/workerd/api/actor-state.c++
Original file line number Diff line number Diff line change
Expand Up @@ -642,13 +642,19 @@ jsg::JsRef<jsg::JsValue> DurableObjectStorage::transactionSync(
uint depth = transactionSyncDepth++;
KJ_DEFER(--transactionSyncDepth);

// TODO(perf): SQLite actually allows multiple savepoints with the same name. The name refers
// to the most-recent of these savepoints. This means we don't actually have to append the
// depth to each savepoint name like I originally thought. We should refactor this -- and use
// prepared statements.

sqlite.run(SqliteDatabase::TRUSTED, kj::str("SAVEPOINT _cf_sync_savepoint_", depth));
return js.tryCatch([&]() {
auto result = callback(js);
sqlite.run(SqliteDatabase::TRUSTED, kj::str("RELEASE _cf_sync_savepoint_", depth));
return kj::mv(result);
}, [&](jsg::Value exception) -> jsg::JsRef<jsg::JsValue> {
sqlite.run(SqliteDatabase::TRUSTED, kj::str("ROLLBACK TO _cf_sync_savepoint_", depth));
sqlite.run(SqliteDatabase::TRUSTED, kj::str("RELEASE _cf_sync_savepoint_", depth));
js.throwException(kj::mv(exception));
});
} else {
Expand Down

0 comments on commit ed88b15

Please sign in to comment.