feat: add support for nested transaction rollbacks via savepoints in sql #4375
Conversation
LucianBuzzo
requested review from
miguelff and
Weakky
and removed request for
a team
LucianBuzzo
force-pushed
the
lucianbuzzo/sql-nested-transactions
branch
from
6f375b8 to
b984ae4
Compare
|
|
|
This should resolve the issue described here prisma/prisma#15212 |
|
Would you consider this a breaking change compared to current behavior @LucianBuzzo? It smells a bit like that to me because current functionality would change. Agree? |
|
@janpio I think that the current behaviour is unexpected, so this would be a bug fix or new "feature". It's possible that people have made applications that rely on the current behaviour, but this is true of any bug IMO. |
|
Thanks for the explanation @LucianBuzzo, I get it now! I could also connect it to prisma/prisma#19346 then which is about the same problem. Did you find a bug report about the incorrect behavior? Optimally this issue would close that one so we have a closed bug in our release notes when we add this, that makes it clearer that this is a "breaking change" only in the context of that it fixes a bug. (Maybe you can create the issue if non exists yet!? Thanks.) (There are also the related prisma/prisma#9710 and prisma/prisma#12898, but I think they want to completely replace transactions with This should probably be documented in the future with a new "Nested interactive transactions" sub headline under https://www.prisma.io/docs/concepts/components/prisma-client/transactions#interactive-transactions? What would be good test cases for prisma/prisma? (Optimally those fail right now, but will succeed when this PR here is merged to show that this properly improves things) |
|
You can ignore all the failing "Driver Adapters" tests, and also the Vitess and MySQL on Linux ones - those are currently flaky. But these look relevant and need to be fixed: |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Thanks for the review @janpio I'll get the code issues resolved. The issue prisma/prisma#19346 describes the exact problem, I just haven't referenced it as I wanted to have this PR reviewed first. For prisma/prisma#9710 and prisma/prisma#12898 this PR will resolve those issues, as long as they are using an SQL DB. describe('some test', () => {
it('should work', async () => {
try {
await prisma.$transaction(async (tx) => {
//...
expect(x).toBe(y)
throw new Error('rollback')
})
} catch (e) {
// ignore e
}
})
})This is a similar pattern to how I discovered this issue myself - trying to test if a user is able to perform an operation when running Yates RBAC. I'll add some lines about nested transactions to the docs. As for a test case in https://github.com/prisma/prisma a simple way to do it would be to create a client extension that wraps all queries in a transaction and then test that the interactive transaction example in the docs (transferring money between accounts) works as expected. |
|
Wouldn't it then also be possible to just create a standalone test case that wraps multiple transactions, without the need to change how we run tests or use an extension? (I would assume the extension just does that automatically, but it should be possible to also express that explicitly and simpler)
Don't these suggest to use just savepoints for the actual tests, instead of normal transactions? I am still a bit unclear about the approach. |
|
From my reading of those issues, it seems that you could achieve the result they want using a nested transaction that utilises savepoints. I would let the original authors correct me if this is not the case though! |
|
And yes you could not use a client extension in the test case, I simply provided the example above as one possibility 👍 |
|
CI run is done, some more Clippy stuff to make it able to compile I guess: https://github.com/prisma/prisma-engines/actions/runs/6682077529/job/18158479734?pr=4375 (I also don't know any Rust, so unfortunately can't be of help here) |
|
(Can you do a fake PR to README.md adding a newline or some other minimal, non-intrusive change that I can merge easily? Then your commits in this PR will automatically run tests going forward - and you don't have to wait for me to click the button😆) |
LucianBuzzo
force-pushed
the
lucianbuzzo/sql-nested-transactions
branch
from
da206e7 to
a8af640
Compare
|
@janpio I've also opened a PR with a failing test case for the prisma client here prisma/prisma#21678 |
|
Note: I brought the branch into the repo, it will release the engines automatically after a while |
|
Happened, version shared here: prisma/prisma#21678 (comment) |
query-engine/connector-test-kit-rs/query-engine-tests/tests/new/interactive_tx.rs
Outdated
Show resolved
Hide resolved
LucianBuzzo
force-pushed
the
lucianbuzzo/sql-nested-transactions
branch
from
24fb9fc to
fa6eebb
Compare
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This is my first OSS contribution for a Rust project, so I'm sure I've made some stupid mistakes, but I think it should mostly work :) This change adds a mutable depth counter, that can track how many levels deep a transaction is, and uses savepoints to implement correct rollback behaviour. Previously, once a nested transaction was complete, it would be saved with `COMMIT`, meaning that even if the outer transaction was rolled back, the operations in the inner transaction would persist. With this change, if the outer transaction gets rolled back, then all inner transactions will also be rolled back. Different flavours of SQL servers have different syntax for handling savepoints, so I've had to add new methods to the `Queryable` trait for getting the commit and rollback statements. These are both parameterized by the current depth. I've additionally had to modify the `begin_statement` method to accept a depth parameter, as it will need to conditionally create a savepoint. When opening a transaction via the transaction server, you can now pass the prior transaction ID to re-use the existing transaction, incrementing the depth. Signed-off-by: Lucian Buzzo <[email protected]>
LucianBuzzo
force-pushed
the
lucianbuzzo/sql-nested-transactions
branch
from
fa6eebb to
5546288
Compare
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
This change adds support for handling rollbacks in nested transactions in SQL databases. Specifically, the inner transaction should be rolled back if the outer transaction fails. To do this we keep track of the transaction ID and transaction depth so we can re-use an existing open transaction in the underlying engine. This change also allows the use of the `$transaction` method on an interactive transaction client. depends-on: prisma/prisma-engines#4375
quaint/src/connector/transaction.rs
Outdated
| let current_depth = self.depth.load(Ordering::SeqCst); | ||
| let new_depth = current_depth + 1; | ||
| self.depth.store(new_depth, Ordering::SeqCst); |
There was a problem hiding this comment.
| let current_depth = self.depth.load(Ordering::SeqCst); | |
| let new_depth = current_depth + 1; | |
| self.depth.store(new_depth, Ordering::SeqCst); | |
| let new_depth = self.depth.fetch_add(Ordering::SeqCst) + 1; |
quaint/src/connector/transaction.rs
Outdated
| @@ -108,19 +126,88 @@ impl<'a> DefaultTransaction<'a> { | |||
|
|
|||
| #[async_trait] | |||
| impl Transaction for DefaultTransaction<'_> { | |||
| fn depth(&self) -> u32 { | |||
| self.depth.load(Ordering::SeqCst) | |||
There was a problem hiding this comment.
| self.depth.load(Ordering::SeqCst) | |
| self.depth.load(Ordering::Relaxed) |
There was a problem hiding this comment.
You don't need anything more strict than Relaxed because we're not synchronizing access to non-atomic memory using atomics, we're simply using an atomic as a thread-safe counter, and we're interested in its own total modification order, which is always guaranteed by atomics regardless of the memory ordering.
There was a problem hiding this comment.
Anything we use in between the modifications (like calling the connection methods) doesn't require external synchronization because it being Sync implies it either doesn't share memory or has all the necessary synchronization in critical sections internally.
quaint/src/connector/transaction.rs
Outdated
| } | ||
|
|
||
| async fn begin(&self) -> crate::Result<()> { | ||
| self.depth.fetch_add(1, Ordering::SeqCst); |
There was a problem hiding this comment.
| self.depth.fetch_add(1, Ordering::SeqCst); | |
| self.depth.fetch_add(1, Ordering::Relaxed); |
quaint/src/connector/transaction.rs
Outdated
| /// Commit the changes to the database and consume the transaction. | ||
| async fn commit(&self) -> crate::Result<()> { | ||
| self.gauge.decrement(); | ||
| // Perform the asynchronous operation without holding the lock |
There was a problem hiding this comment.
This can be removed, it's leftover from a previous version of the code that didn't use AtomicU32
quaint/src/connector/transaction.rs
Outdated
| self.inner.raw_cmd("COMMIT").await?; | ||
|
|
||
| self.depth.fetch_sub(1, Ordering::SeqCst); |
There was a problem hiding this comment.
| self.depth.fetch_sub(1, Ordering::SeqCst); | |
| self.depth.fetch_sub(1, Ordering::Relaxed); |
There was a problem hiding this comment.
actually, shouldn't it set the depth to zero instead?
There was a problem hiding this comment.
Yes I think you're correct, it should just get set to zero.
quaint/src/connector/transaction.rs
Outdated
| self.inner.raw_cmd("ROLLBACK").await?; | ||
|
|
||
| self.depth.fetch_sub(1, Ordering::SeqCst); |
There was a problem hiding this comment.
| self.depth.fetch_sub(1, Ordering::SeqCst); | |
| self.depth.fetch_sub(1, Ordering::Relaxed); |
quaint/src/connector/transaction.rs
Outdated
| panic!( | ||
| "No savepoint to release in transaction, make sure to call create_savepoint before release_savepoint" | ||
| ); |
There was a problem hiding this comment.
why panic rather than returning an error?
There was a problem hiding this comment.
No specific reason, I can return an error here instead.
| async fn release_savepoint(&self) -> crate::Result<()> { | ||
| let depth_val = self.depth.load(Ordering::SeqCst); | ||
|
|
||
| if depth_val == 0 { |
There was a problem hiding this comment.
This check doesn't look very reliable: you can call release_savepoint a thousand times in a row while at depth 1 and it will still be 1 here in all of those calls since it's only decremented asynchronously some time in the future. One way to make it work would be to always fetch_sub it in the beginning instead of just loading but then you'll also need to make the counter signed to allow it to become negative.
There was a problem hiding this comment.
@aqrln This check is here to help debug code issues that might occur in the future, if you do end up calling release_savepoint times, you will get a lot of SQL errors. I'm happy to switch to a signed counter and refactor this code, but I'm not sure if it's worth it - let me know what you think.
This is my first OSS contribution for a Rust project, so I'm sure I've made some stupid mistakes, but I think it should mostly work :)
This change adds a mutable depth counter, that can track how many levels deep a transaction is, and uses savepoints to implement correct rollback behaviour. Previously, once a nested transaction was complete, it would be saved with
COMMIT, meaning that even if the outer transaction was rolled back, the operations in the inner transaction would persist. With this change, if the outer transaction gets rolled back, then all inner transactions will also be rolled back.Different flavours of SQL servers have different syntax for handling savepoints, so I've had to add new methods to the
Queryabletrait for getting the commit and rollback statements. These are both parameterized by the current depth.I've additionally had to modify the
begin_statementmethod to accept a depth parameter, as it will need to conditionally create a savepoint.Client PR: prisma/prisma#21678