feat: add support for nested transaction rollbacks via savepoints in sql

[LucianBuzzo]

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.

Client PR: https://github.com/prisma/prisma/pull/21678

[CLAassistant]

All committers have signed the CLA.

[LucianBuzzo]

This should resolve the issue described here https://github.com/prisma/prisma/issues/15212

[LucianBuzzo]

@miguelff @Weakky If you have a moment could you take a look a this? If it works as I think it does ( 🤞 ) it's going to fix a lot of headaches for me and my team! TIA 🙏

[janpio]

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?

[LucianBuzzo]

@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. As an example of how I consider this a bug: the documentation for transactions shows an example where a transfer between two accounts I rolled back, but it's not explained that this will not work if the transaction is nested. If you try to make behaviour like this and couple it with something like the RLS example from the client extensions, it will fail and the user has to do a lot of debugging to find out why.

[janpio]

Thanks for the explanation @LucianBuzzo, I get it now!

I could also connect it to https://github.com/prisma/prisma/issues/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 https://github.com/prisma/prisma/issues/9710 and https://github.com/prisma/prisma/issues/12898, but I think they want to completely replace transactions with BEGIN/COMMIT with savepoints - which this PR does not do. Do you agree that there is no overlap?)

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)

[janpio]

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:

• Lots of formatting things: https://github.com/prisma/prisma-engines/actions/runs/6553297366/job/18151456208?pr=4375
• Some Rust things: https://github.com/prisma/prisma-engines/actions/runs/6553297349/job/18151455513?pr=4375

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #4375 will not alter performance

_{Comparing LucianBuzzo:lucianbuzzo/sql-nested-transactions (3a8bf35) with main (09cb2c4)}

Summary

✅ 11 untouched benchmarks

[LucianBuzzo]

Thanks for the review @janpio I'll get the code issues resolved.

The issue https://github.com/prisma/prisma/issues/19346 describes the exact problem, I just haven't referenced it as I wanted to have this PR reviewed first. For https://github.com/prisma/prisma/issues/9710 and https://github.com/prisma/prisma/issues/12898 this PR will resolve those issues, as long as they are using an SQL DB. To get integration tests in an isolated transaction, you would simply need to run your test cases inside an interactive transaction:

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.

[janpio]

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)

For prisma/prisma#9710 and prisma/prisma#12898 this PR will resolve those issues, as long as they are using an SQL DB.

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.

[LucianBuzzo]

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!

[LucianBuzzo]

And yes you could not use a client extension in the test case, I simply provided the example above as one possibility 👍

[janpio]

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)

[janpio]

(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]

@janpio Here's a PR for the Quaint README https://github.com/prisma/prisma-engines/pull/4399

[LucianBuzzo]

@janpio I've also opened a PR with a failing test case for the prisma client here https://github.com/prisma/prisma/pull/21678

[Jolg42]

Note: I brought the branch into the repo, it will release the engines automatically after a while https://github.com/prisma/prisma-engines/tree/integration/sql-nested-transactions https://buildkite.com/prisma/test-prisma-engines/builds/21364

[janpio]

Happened, version shared here: https://github.com/prisma/prisma/pull/21678#issuecomment-1789207079

[LucianBuzzo]

@janpio @Jolg42 I think that this commit https://github.com/prisma/prisma-engines/pull/4375/commits/5d2cc0ae8c06690013edaaeb07978e66ca704070 should allow us to send an existing transaction ID to the engine in the case of a nested transaction. I'm not sure how to test this behaviour in this repo - any tips on how to do this, or an existing test I could use as an example?

[janpio]

Via GitHub and CI we would need to update our local branch with your change in your remote/fork branch, and then use the published version to Npm again in prisma/prisma I guess.

Locally, you can probably wire things together more efficiently. Build locally, use that engine in the prisma/prisma tests.

[Jolg42]

Note that in our CI pipeline here, tests are failing now, which means no build can be released until the issue(s) are fixed.

[LucianBuzzo]

@Jolg42 It looks like this same set of tests failing here are also failing on main https://github.com/prisma/prisma-engines/actions/runs/6787958315/job/18451926022

[Jolg42]

True! https://github.com/prisma/prisma-engines/actions/workflows/query-engine-driver-adapters.yml?query=branch%3Amain I hope these are gone soon (some work needs to happen) 🤞🏼 Because it's a distraction to have these always failing at the moment.

Sorry, I was not specific, Buildkite tests must pass to be able to get a release. The other tests on GitHub action can fail and they would not block a release.

I did the following locally to recreate a fresh branch inside our repo to get Buildkite running:

git branch --delete integration/sql-nested-transactions
gh pr checkout 4375
git checkout -b integration/sql-nested-transactions
git push --set-upstream origin integration/sql-nested-transactions --force

So let's check how it goes now in https://github.com/prisma/prisma-engines/pull/4405

[LucianBuzzo]

@Jolg42 It seems that the tx_id is still not being passed correctly into the engine, the only issue I could find after going through this was here https://github.com/prisma/prisma-engines/pull/4375/commits/a795da28cbd337db0b48a4f91662ba0e481c9ebf though it looks like this is only for tracing? Any ideas why this might be happening? The next place I'm going to look is in the prisma client code, at the requests being sent.

UPDATE: It looks like the transaction id is being shared correctly, but it's not picking up the correct transaction depth when the nested transaction runs - more debugging!

[LucianBuzzo]

@Jolg42 Some success! I've been able to get the TX server to re-use existing connections and correctly use the transaction depth to set savepoints. Check out the test case that I think shows the system working correctly https://github.com/prisma/prisma-engines/pull/4375/files#diff-722fc48a94cd85632c0a8a33ce7c5bacd69048e15cc27f6f3523b25b7b26d765R461 I still feel like this is kind of messy, but fingers-crossed it works!

[LucianBuzzo]

@Jolg42 @janpio I cleaned up this PR and got it up to date, now I have some more tests failing - are any of these expected to have issues? If not could you give me some pointers on how to resolve the failures?

[janpio]

Only the vitess_8_0 is expected to be flaky and failing most of the time right now. The Driver Adapters I do not understand what is going on. Don't think that is because of the PR. Related though: Formatting for sure, MongoDB also - failures aretransaction related.

[LucianBuzzo]

@janpio Sorry for the slow response - the mongo and formatting tests are now fixed, and I'll dig into the driver adapter issues next

[janpio]

As all the main tests are passing, I pushed this to a local branch and opened a draft PR: https://github.com/prisma/prisma-engines/pull/4506 This should now be published as an integration version again and open a PR in prisma/prisma - so we can see if the tests pass.

[janpio]

And here is the PR with the test results: https://github.com/prisma/prisma/pull/22161

1. Driver Adapter tests are failing as expected for now
2. One other failure for MongoDB (in 3 node versions): https://github.com/prisma/prisma/actions/runs/7038173013/job/19169529537#step:8:478

FAIL issues/21552-datetime-representation-compatibility/tests.ts (5.886 s)
  ● issues.21552-datetime-representation-compatibility (provider=mongodb) › can read back DateTime created via native connector

    SyntaxError: Unexpected token 'C', "Committing"... is not valid JSON
        at JSON.parse (<anonymous>)

      33 |       })
      34 |
    > 35 |       const writtenRecord = JSON.parse(seedOutput.stdout)
         |                                  ^
      36 |
      37 |       const readRecord = await prisma.test.findUnique({
      38 |         where: {

      at Object.<anonymous> (issues/21552-datetime-representation-compatibility/tests.ts:35:34)

That is pretty cool! (And a confusing failure, I hope you can make more sense of it)

🍾