Brayden / starbasedb

HTTP SQLite scale-to-zero database on the edge built on Cloudflare Durable Objects.
https://starbasedb.com
GNU Affero General Public License v3.0
711 stars 15 forks source link

DO has a transactions API (and we... forgot to document it... sorry) #12

Closed kentonv closed 1 month ago

kentonv commented 1 month ago

Hey folks! I read this:

https://starbasedb.com/blog/developing-acid-transaction-support-in-starbasedb/

This is really impressive engineering! But, thought you might want to know... Durable Objects has a built-in transaction API.

this.ctx.storage.transactionSync(() => {
  // do some sql here
  this.ctx.storage.sql.exec(...);

  // If the callback returns normally, the transaction is committed.
  // If an error is thrown, the transaction is rolled back.
});

It's totally our (Cloudflare's) fault that you didn't see this -- the error message thrown by BEGIN TRANSACTION fails to explain it, and it looks like we totally forgot to update the docs to cover the new transactionSync() API, or how the older async transaction() API interacts with SQL.

Using the transaction API will, of course, be much more efficient than using PITR for rollbacks.

So sorry that our documentation error led you to put so much effort into building a work-around. :/

Brayden commented 1 month ago

Wow, thanks for bringing this to our attention @kentonv!

It certainly does make our lives a bit easier getting to use something natively exposed rather than writing it ourselves. No apologies needed, super happy to hear that it exists already. Will get this updated so we can get the efficiency wins of not having to do PITR bookmarking on our end :)

Any reason why writing a query with BEGIN TRANSACTION and letting a user handle that themselves isn't natively supported like it is with SQLite? I assume something to do with file system interactions?

Awesome work for the entire Cloudflare team on putting together some amazing products to build on top of. Keep it up!

kentonv commented 1 month ago

We don't allow BEGIN TRANSACTION because under the hood the platform is creating some transactions implicitly, and we need to make sure our implicit transactions don't conflict with the application's explicit ones.

In particular, we implement a guarantee that if you perform a series of queries in sequence without any awaits in between, these queries are implicitly atomic -- either they will all be committed, or none of them will be. This guarantee helps avoid a lot of bugs that people may not even be thinking about. We implement it by implicitly wrapping in a transaction.

(This also has another benefit, which is that in SQLite, committing a series of statements in a single transaction is actually more efficient than committing them one-by-one. So, applications get that benefit automatically.)

So, because we have these implicit transactions, there would be a few problems with allowing BEGIN TRANSACTION:

Brayden commented 1 month ago

@kentonv This explanation makes a lot of sense, and lines up with some of my assumptions. Nothing beats getting to hear how the internals work in depth, though, from someone who has intimate knowledge with it. Appreciate you breaking it down and even caring enough to open this issue up to bring light to the transactionSync() API method. 🙏

I've opened this PR here if you wouldn't mind verifying this is how you'd expect it be implemented! https://github.com/Brayden/starbasedb/pull/13/files?diff=split&w=0

kentonv commented 1 month ago

Yep, that looks right.