Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Initial lock support #118

Merged
merged 3 commits into from
Sep 10, 2021
Merged

Initial lock support #118

merged 3 commits into from
Sep 10, 2021

Conversation

nappa85
Copy link
Contributor

@nappa85 nappa85 commented Aug 30, 2021

Depends on SeaQL/sea-query#117, adds lock support to select statements

@tyt2y3 tyt2y3 force-pushed the master branch 10 times, most recently from 71f7f40 to 9a1b771 Compare September 3, 2021 15:15
@nappa85
Copy link
Contributor Author

nappa85 commented Sep 7, 2021

Are there blockers that deny merging this PR?

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 10, 2021

Sorry was busy. Yes given SeaQuery is upgraded it now can be merged.

I think these methods belongs to the QuerySelect trait (in query/helper.rs) where other Selects can use them too.

I am also thinking of whether we can provide a higher level abstraction over some common scenario?

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 10, 2021

Ok for moving the methods.
I think what's missing is the transactions management, so start a transaction optionally with different isolation levels, committing it, reverting it, etc...

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 10, 2021

I think transaction is highly driver specific. I don't think we can cleanly wrap and generalizes over different database+driver. Perhaps we can start by exposing some of the underlying types and methods from SQLx.

https://docs.rs/sqlx/0.5.7/sqlx/trait.Connection.html#method.transaction

@tyt2y3 tyt2y3 merged commit c3084e4 into SeaQL:master Sep 10, 2021
@nappa85
Copy link
Contributor Author

nappa85 commented Sep 10, 2021

It can be a starting point, if we're able to maintain sea-orm typing.
What I mean is, sqlx Connection::transaction takes an fn(&mut Transaction), we would need to wrap it up so inside the function we can still use our DatabaseConnection type.

And it's kind of an overkill IMHO, Transaction is only a sugar wrapper over a normal connection, where a START TRANSACTION has been executed.

I would prefere an approach like this: https://docs.rs/mysql_async/0.28.0/mysql_async/struct.Conn.html#method.start_transaction
where Transaction is simply a tiny wrapper over Connection and can be used in any method that thakes &Conn because it impl Deref<Conn>

About isolation levels, mysql and postgres are quite similars:
https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html
https://www.postgresql.org/docs/9.5/sql-set-transaction.html

Sqlite has transactions too, it has a little different dialect, like BEGIN [TRANSACTION] instead of START TRANSACTION, and isolation levels have to be set using the PRAGMA keyword, like https://www.sqlite.org/pragma.html#pragma_read_uncommitted

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 10, 2021

Thank you for the investigation. Since this PR is closed, I think it's better we move the discussion to a different thread to follow up.

@nappa85 nappa85 mentioned this pull request Sep 10, 2021
@tyt2y3 tyt2y3 linked an issue Sep 18, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for row locking?
2 participants