digital-fabric / extralite

Ruby on SQLite
http://www.rubydoc.info/gems/extralite
MIT License
247 stars 7 forks source link

Implement Database#transaction #42

Closed noteflakes closed 6 months ago

noteflakes commented 6 months ago

Fixes #41.

fractaledmind commented 6 months ago

This is wonderful. One important detail is giving consumers the ability to change the default transaction mode for all transactions, because the better default for web apps is IMMEDIATE, to ensure that busy exceptions aren't immediately thrown when trying to upgrade a transaction with a write (since in a web app, like a Rails app, it is essentially guaranteed that a transaction will have a write, and you don't want immediate busy exceptions, but rather to attempt to queue up that transaction).

the sqlite3-ruby gem allows for a default_transaction_mode option when initializing the Database instance,^1 which is then used in the transaction method.^2

noteflakes commented 6 months ago

One important detail is giving consumers the ability to change the default transaction mode for all transactions, because the better default for web apps is IMMEDIATE...

On its face this is reasonable, but the sqlite3 docs state:

The default transaction behavior is DEFERRED.

Which means that doing transactions by executing a BEGIN TRANSACTION statement, you'll need to be explicit if you want it to be IMMEDIATE. While I undestand the utility of such a feature, I think it's better to follow the basic SQL API and mark immediate/exclusive transactions as such explicitly.

Another consideration is that I'd like to keep the Database API as small as possible. You can always patch Database#transaction to your liking (for my own use, I find it sometimes useful to be able to nest calls to Database#transaction, which you can't do with the current implementation).

Also note that we don't necessarily strive to have full comptability with sqlite3-ruby. Another way this implementation differs from sqlite3-ruby is that it expects a block to be given. The sqlite3-ruby implementation can be called without a block.

fractaledmind commented 6 months ago

I completely understand not trying to have full compatibility with sqlite3-ruby, I was just providing the context of their implementation. And yes, I could patch the method, but I then need to track all upstream changes to ensure my patch stays correct.

Given that we aren't generating BEGIN TRANSACTION strings anyway, but always an explicit BEGIN #{mode} TRANSACTION string, I would still argue that having some way to globally set the mode is useful.

Alternatively, there is a case that the better default for Ruby uses of SQLite would be :immediate. The performance difference is minimal and only a factor for read-only transactions, which are the non-standard case. Plus, the unexpected behavior of getting busy exceptions when you know that the transaction didn't take longer to connect than the timeout is very frustrating. I would be fine with having no global mode setter, but just changing the default. But, I really don't see a strong reason to make :deferred the default with no global mode setter. It guarantees poor user-experience in a multi-threaded environment under load for minimal, if any, benefit.

noteflakes commented 6 months ago

I would still argue that having some way to globally set the mode is useful.

So would you prefer having a global setting i.e. Extralite::Database.default_transaction_mode= or per-database i.e. db.default_transaction_mode=?

fractaledmind commented 6 months ago

Honestly, as I wrote that previous message, I convinced myself that a global setter doesn't feel right. I think just changing the argument default to :immediate is really the win here.

That being said, if you want to adhere to SQLite's defaults, I think setting it per db makes sense. Tho allowing it to be set via the initializer would be nicer than having to initialize then call a setter.

noteflakes commented 6 months ago

I think just changing the argument default to :immediate is really the win here.

I agree. I've changed the default mode to immediate, will merge the PR shortly.