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

fix(driver-adapters): support Transaction Isolation Levels in PlanetScale #4966

Closed
wants to merge 5 commits into from

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jul 23, 2024

This PR:

Explanation

We originally thought that Transaction Isolation Levels for PlanetScale worked fine in TypeScript-land, and only failed in Rust tests with the error message:

Transaction characteristics can't be changed while a transaction is in progress (errno 1568) (sqlstate 25001) during query: SET TRANSACTION ISOLATION LEVEL SERIALIZABLE

As it turns out, that wasn't the case: there were actual issues in the way we were executing SET TRANSACTION ISOLATION LEVEL ... queries.

Consider the following snippet, constructing a planetScale.Client instance:

import * as planetScale from '@planetscale/database'

async function main() {
  const databaseURL = 'mysql://root:root@localhost:33807/PRISMA_DB_NAME'
  const url = new URL('http://root:root@127.0.0.1:8085')
  url.pathname = new URL(databaseURL).pathname
  
  const client = new planetScale.Client({ url: url.toString() })

  ... // TODO
}

Before this PR, using transactions with custom isolation levels in @prisma/adapter-planetscale was virtually equivalent to:

  {
    await client.transaction(async (tx) => {
      // 1. Custom isolation level
      await client.execute('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE')

      // 2. any query within the transactional context `tx`
      const query = await tx.execute('SELECT 1') 
      console.log('[query]')
      console.dir(query, { depth: null })
    })
  }

This results in a runtime error. As written in the MySQL docs,

You can set transaction characteristics globally, for the current session, or for the next transaction only.

So, this PR changes the driver-adapters part of Query Engine to trigger the following JS logic instead:

  {
    // 1. Custom isolation level
    await client.execute('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE')

    await client.transaction(async (tx) => {
      // 2. any query within the transactional context `tx`
      const query = await tx.execute('SELECT 1')
      console.log('[query]')
      console.dir(query, { depth: null })
    })
  }

@jkomyno jkomyno requested a review from a team as a code owner July 23, 2024 13:33
@jkomyno jkomyno requested review from Druue and removed request for a team July 23, 2024 13:33
@jkomyno jkomyno added this to the 5.18.0 milestone Jul 23, 2024
@CLAassistant
Copy link

CLAassistant commented Jul 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.057MiB 2.057MiB 4.000B
Postgres (gzip) 820.543KiB 820.453KiB 93.000B
Mysql 2.027MiB 2.027MiB -7.000B
Mysql (gzip) 808.051KiB 808.035KiB 17.000B
Sqlite 1.919MiB 1.919MiB 4.000B
Sqlite (gzip) 766.193KiB 766.277KiB -86.000B

Copy link

codspeed-hq bot commented Jul 23, 2024

CodSpeed Performance Report

Merging #4966 will not alter performance

Comparing fix/planetscale-transactions (35ba9f4) with main (a6977e5)

Summary

✅ 11 untouched benchmarks

@jkomyno jkomyno requested review from SevInf and removed request for Druue July 24, 2024 08:02
@jkomyno jkomyno self-assigned this Jul 24, 2024
Copy link
Contributor

@SevInf SevInf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really not sure about that.
client.execute is not guaranteed to be executed on the same connection transaction will later start and it only works it tests because there are no concurrent requests.

And since we are not making a change on the adapter level, but on the driver itself - it is definietely not guaranteed that for any potential mysql adapter executeRaw and startTransaction are executed on the same connection.

I think the way we should do it instead is:

  • change startTransaction in driver interface to accept isolation level. This is how I originally implemented it, and then we changed it for logging' sake and I think this was a mistake.
  • now driver is responsible for both starting the transaction and setting isolation level.
  • in planetscale driver, we can now first secure the connection (IIRC, they have a session api for that), set isolation level and only then start the transaction.

@jkomyno
Copy link
Contributor Author

jkomyno commented Jul 24, 2024

client.execute is not guaranteed to be executed on the same connection transaction will later start and it only works it tests because there are no concurrent requests.

You raised a good point, but I think we can use the Connection API, without refactoring as much as you suggest:

  {
    const conn = await client.connection()
    await conn.execute('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE')
    await conn.transaction(async (tx) => {
      // ...
    })
  }

What do you think?

@jkomyno
Copy link
Contributor Author

jkomyno commented Jul 24, 2024

client.execute is not guaranteed to be executed on the same connection transaction will later start and it only works it tests because there are no concurrent requests.

You raised a good point, but I think we can use the Connection API, without refactoring as much as you suggest:

  {
    const conn = await client.connection()
    await conn.execute('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE')
    await conn.transaction(async (tx) => {
      // ...
    })
  }

What do you think?

From what I can see, implementing this means:

  • keeping this engines PR as it is
  • adjusting the @prisma/adapter-planetscale in TypeScript

@SevInf
Copy link
Contributor

SevInf commented Jul 24, 2024

Sorry, I don't quite see how that would work without refactor I am suggesting, can you clarify a bit?
We need to know what isolation level is in the startTransaction in order to do so. Which means that we need to change startTransaction signature the way I suggesting and stop issuing SET TRANSACTION ISOLATION LEVEL statements on the engine side. What am I missing?

@jkomyno
Copy link
Contributor Author

jkomyno commented Jul 24, 2024

Deprecated by #4967 and prisma/prisma#24878.

@jkomyno jkomyno closed this Jul 24, 2024
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.

[DA] Planetscale engine tests: interactive_tx
3 participants