Skip to content
This repository was archived by the owner on Mar 19, 2021. It is now read-only.

with-transaction in Sqlitex and Sqlitex.Server #87

Closed

Conversation

the-kenny
Copy link
Contributor

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
Copy link

coveralls commented Jun 19, 2019

Coverage Status

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

@mmmries
Copy link
Collaborator

mmmries commented Jun 19, 2019

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
Copy link
Contributor Author

@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
Copy link
Collaborator

mmmries commented Jun 27, 2019

@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
Copy link
Member

Sorry for the long wait - Checking right now.

@ConnorRigby
Copy link
Member

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

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
Copy link
Contributor Author

the-kenny commented Feb 20, 2020 via email

@ConnorRigby
Copy link
Member

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

@the-kenny
Copy link
Contributor Author

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

@dominicletz
Copy link
Contributor

@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
Copy link
Member

This was expanded upon by #99

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants