Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow SQL interface to be enabled without "experimental" compat flag. #2666

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Sep 5, 2024

Instead, it can be enabled in the workerd config.

In production, it'll be enabled implicitly if the new SQLite-based storage engine is in use. But workerd running locally always uses SQLite, even when testing code intended to run against non-SQLite namespaces in production, so we need a config option for workerd.

Also enable PITR and abort() APIs without "experimental".

Point-in-time recovery is only supported for SQLite-backed DOs in production; PITR methods will throw for non-SQLite namespaces.

abort() will work for all DOs. I think we've generally agreed this should be made public, but it's especially needed with the PITR APIs since you need a way to restart the object immediately after calling onNextSessionRestoreBookmark().

@kentonv kentonv requested review from a team as code owners September 5, 2024 22:33
@kentonv kentonv requested a review from dom96 September 5, 2024 22:33
Instead, it can be enabled in the workerd config.

In production, it'll be enabled implicitly if the new SQLite-based storage engine is in use. But `workerd` running locally always uses SQLite, even when testing code intended to run against non-SQLite namespaces in production, so we need a config option for workerd.
Point-in-time recovery is only supported for SQLite-backed DOs in production; PITR methods will throw for non-SQLite namespaces.

abort() will work for all DOs. I think we've generally agreed this should be made public, but it's especially needed with the PITR APIs since you need a way to restart the object immediately after calling `onNextSessionRestoreBookmark()`.
@@ -579,6 +579,14 @@ struct Worker {
# pinned to memory forever, so we provide this flag to change the default behavior.
#
# Note that this is only supported in Workerd; production Durable Objects cannot toggle eviction.

enableSql @4 :Bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand how this field would be in a union with ephemeralLocal but it does seem like enableSql would be compatible with preventEviction and uniqueKey. Are we modeling the config this way because those combinations haven't been tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it appears we are manually copying this file over here(workers-sdk)? 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

This field isn't a member of a union. The union ended on line 574.

Also, it appears we are manually copying this file over here(workers-sdk)? 😕

Manual copying is a pretty normal way to share schemas. It's not a big deal, you just overwrite the copy with the updated version if and when you want to change workers-sdk to use the new field.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad!

@kentonv
Copy link
Member Author

kentonv commented Sep 6, 2024

After some discussion I think I'm going to update this PR so that instead of making storage.sql "optional", it will always be present, but its methods will throw exceptions if the DO is not backed by SQL.

@kentonv
Copy link
Member Author

kentonv commented Sep 6, 2024

Updated as described: Now storage.sql is always there, but the methods throw exceptions if the object isn't SQLite-backed.

Having `storage.sql` be optional is potentially annoying for two reasons:
- TypeScript will force people to check if it's present, even though apps that have configured it should be able to expect it's always present.
- We'd like to provide a more detailed error message telling people how to configure SQL.

So, this commit changes things so `storage.sql` is always present, but its methods will throw exceptions if the DO isn't SQLite-backed.
@kentonv kentonv merged commit ca6a0f7 into main Sep 9, 2024
13 checks passed
@kentonv kentonv deleted the kenton/enable-sql branch September 9, 2024 14:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants