Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazily initialize _cf_KV sqlite table. #2515

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Aug 11, 2024

This way, if a Durable Object never actually uses the KV storage interface -- only uses SQL -- we'll never create the table and can skip preparing statements for it.

This will also make it easier for the edge runtime to clean up actors that never had data.

@kentonv kentonv requested a review from justin-mp August 11, 2024 00:27
@kentonv kentonv requested review from a team as code owners August 11, 2024 00:27
@kentonv kentonv requested review from danlapid and tewaro August 11, 2024 00:27
@@ -1857,6 +1857,10 @@ public:
auto db = kj::heap<SqliteDatabase>(*as,
kj::Path({d.uniqueKey, kj::str(idPtr, ".sqlite")}),
kj::WriteMode::CREATE | kj::WriteMode::MODIFY | kj::WriteMode::CREATE_PARENT);

// Before we do anything, make sure the database is in WAL mode.
db->run("PRAGMA journal_mode=WAL;");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test to ensure that the db is in WAL mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not at present.

Technically workerd works fine in any mode, we only enable WAL because it's better.

Storage Relay Service (what we use on the edge) absolutely requires WAL mode, so all its tests would certainly blow up with out it. But it also doesn't depend on workerd setting it; it sets WAL mode itself in internal code.

@justin-mp
Copy link
Contributor

Hmm, the //src/workerd/api:sql-test the test is unhappy:

workerd/server/server.c++:3791: debug: [ TEST ] main
(18:00:15) [2,869 / 2,870] Testing @@workerd//src/workerd/api:sql-test; 1s linux-sandbox
workerd/io/worker.c++:1925: info: uncaught exception; source = Uncaught (in promise); stack = AssertionError [ERR_ASSERTION]: The input did not match the regular expression /access to _cf_KV.key is prohibited/. Input:

'Error: no such table: _cf_KV: SQLITE_ERROR'

    at new AssertionError (node-internal:internal_assertionerror:419:15)
    at createAssertionError (node-internal:internal_assert:35:19)
    at validateThrownError (node-internal:internal_assert:552:15)
    at Module.throws (node-internal:internal_assert:97:13)
    at test (worker:267:10)
    at async DurableObjectExample.fetch (worker:1131:7)
workerd/io/io-context.c++:355: info: uncaught exception; exception = workerd/jsg/_virtual_includes/jsg/workerd/jsg/value.h:1355: failed: jsg.Error: AssertionError: The input did not match the regular expression /access to _cf_KV.key is prohibited/. Input:

'Error: no such table: _cf_KV: SQLITE_ERROR'

Did we miss a bypass or initialization somewhere?

@kentonv kentonv force-pushed the kenton/lazy-init-sqlite-kv branch from e4720b4 to 5294df8 Compare August 17, 2024 23:29
@kentonv
Copy link
Member Author

kentonv commented Aug 17, 2024

Did we miss a bypass or initialization somewhere?

No, the test was checking for precise error text that depended on the _cf_KV table existing, but now it's lazily initialized. Fixed by adding a storage.put() before the check.

In the edge runtime, WAL mode is already set elsewhere, so this statement was redundant there. And anyway, maybe someday we'll have a mode where there's no KV table.
This way, if a Durable Object never actually uses the KV storage interface -- only uses SQL -- we'll never create the table and can skip preparing statements for it.

This will also make it easier for the edge runtime to clean up actors that never had data.
@kentonv kentonv force-pushed the kenton/lazy-init-sqlite-kv branch from 5294df8 to 32bdb4b Compare August 17, 2024 23:36
@kentonv kentonv merged commit c49cfb4 into main Aug 18, 2024
9 checks passed
@kentonv kentonv deleted the kenton/lazy-init-sqlite-kv branch August 18, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants