elixir-sqlite / sqlitex

An Elixir wrapper around esqlite. Allows access to sqlite3 databases.
https://hex.pm/packages/sqlitex
120 stars 34 forks source link

`with-transaction` in `Sqlitex` and `Sqlitex.Server` #87

Closed the-kenny closed 4 years ago

the-kenny commented 5 years ago

This adds two new functions: Sqlitex.with_transaction and Sqlitex.Server.with_transaction.

Sqlitex.with_transaction runs a given function in a transaction, automatically calling commit or rollback, the latter if an error is raised.

Sqlitex.Server.with_transaction has the same behavior, but runs in the scope of the server process. This assures no other messages can sneak into an open transaction as described in #85

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.3%) to 93.925% when pulling c7fe150638e18415deca5cfa86050cabb82465ca on the-kenny:server-with-transaction into 1049a2c8cf88cd0e12e56f1f4cbed1bd4dc3283e on elixir-sqlite:master.

mmmries commented 5 years ago

I think the only downside here is that Server will block during the entire with_transaction call. This will cause other clients to wait and it would be fairly easy to push over the 5 second default timeout for GenServer.call calls. If 2 clients both call Server.with_transaction and they each take 3 seconds to complete, the second client will get a timeout error and the Server will be unavailable to other clients for 6 seconds.

@ConnorRigby do you know if sqlite_ecto or sqlite_ecto2 make use of Server? I'm wondering if we should just deprecate the Server and keep the with_transaction function for code that is managing its own db connection? @the-kenny do you use Server in your codebase? Do you find it helpful?

the-kenny commented 5 years ago

@mmmries Slow functions are a danger here, absolutely. I should have made this more clear in the docs.

I'm not using Sqlitex in any production set up yet - I'm just evaluating different approaches of persistence and will likely go with it.

I'm also fine with deprecating Sqlitex.Server. That would also narrow down the scope of Sqlitex being a lower-level library which only wraps esqlite.

mmmries commented 5 years ago

@ConnorRigby just wanted to check in on this. Do you use Sqlitex.Server for any of the ecto integration? Would you be in favor of deprecating its usage and including a note about that in the docs for this project?

ConnorRigby commented 5 years ago

Sorry for the long wait - Checking right now.

ConnorRigby commented 5 years ago

@mmmries sqlite_ecto2 actually exclusively only uses Sqlitex.Server https://github.com/elixir-sqlite/sqlite_ecto2/blob/323c2dd9ce1ce3a0d3719ee00b985ee40e79d83e/lib/sqlite_db_connection/protocol.ex#L16

I'm okay with depreciating the Sqlitex.Server, starting with removing it from the ecto2 adapter.

dominicletz commented 4 years ago

Any movement here? I'm in need of the transactions as well. So probably just gonna work of the-kennys fork until this is either merged or otherwise moved forward.

the-kenny commented 4 years ago

Note that I also started a new project using DBConnection (the connection pool by Ecto) for SQLite: https://github.com/the-kenny/exqlite

It’s still pretty barebones but builds on a solid and tested base.

On 20. Feb 2020, at 12:51, Dominic Letz notifications@github.com wrote:

 Any movement here? I'm in need of the transactions as well. So probably just gonna work of the-kennys fork until this is either merged or otherwise moved forward.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

ConnorRigby commented 4 years ago

Sorry for the long delay. @the-kenny could you rebase this off of master?

the-kenny commented 4 years ago

@ConnorRigby merging was actually easier, sorry for not doing the rebase :)

dominicletz commented 4 years ago

@ConnorRigby now that the conflicts are gone you can use Githubs builtin "Rebase and Merge" or "Squas and merge" and you wont have those ugly merge commits. image

ConnorRigby commented 4 years ago

This was expanded upon by #99