Skip to content

Lazily initialize _cf_KV sqlite table.#2515

Merged
kentonv merged 2 commits intomainfrom
kenton/lazy-init-sqlite-kv
Aug 18, 2024
Merged

Lazily initialize _cf_KV sqlite table.#2515
kentonv merged 2 commits intomainfrom
kenton/lazy-init-sqlite-kv

Conversation

@kentonv
Copy link
Copy Markdown
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
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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