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 handling of transactions with explicit isolation level #3

Merged
merged 1 commit into from
Apr 26, 2023
Merged

Fix handling of transactions with explicit isolation level #3

merged 1 commit into from
Apr 26, 2023

Conversation

sds
Copy link
Contributor

@sds sds commented Apr 26, 2023

First of all, super excited for this plugin! Been wanting to use Kysely with Postgres.js for a while. Thanks for getting it started!

Looks like the current implementation doesn't support transactions with specific isolation levels. Fix that by mirroring what the vanilla Postgres driver does here: https://github.com/kysely-org/kysely/blob/a826cede4007211ec670bfe094de8acdddbbe1cb/src/dialect/postgres/postgres-driver.ts#L58-L66

@igalklebanov
Copy link
Member

igalklebanov commented Apr 26, 2023

Hey 👋

Thanks!

Could we add a test case for isolation level?

@igalklebanov igalklebanov added the bug Something isn't working label Apr 26, 2023
@sds sds changed the title Fix handling of transactions Fix handling of transactions with explicit isolation level Apr 26, 2023
@sds
Copy link
Contributor Author

sds commented Apr 26, 2023

I tried adding the tests from the upstream Kysely project, but it looks like the start transaction .../commit statements don't trigger an event that we can tap into here.

You're more familiar with the nuts and bolts of Kysely itself—is there a simple workaround? More specifically: is there a reason we're performing the transaction handling logic in the PostgresJSConnection rather than in the PostgresJSDriver, like what is done upstream?

@igalklebanov
Copy link
Member

igalklebanov commented Apr 26, 2023

I tried adding the tests from the upstream Kysely project, but it looks like the start transaction .../commit statements don't trigger an event that we can tap into here.

You're more familiar with the nuts and bolts of Kysely itself—is there a simple workaround? More specifically: is there a reason we're performing the transaction handling logic in the PostgresJSConnection rather than in the PostgresJSDriver, like what is done upstream?

Its a hack around Postgres.js's limitations (no single connection getter, transaction API not suitable for Kysely - we have no control over commit/rollback) and how Kysely is built around these parts.

The lifecycle of a transaction in Kysely ~is:
acquireConnection: connection -> beginTransaction(connection) -> userFunctionality(connection) -> commitTransaction(connection) OR rollbackTransaction(connection) -> releaseConnection(connection).

acquireConnection is used regardless of transaction scenario, returns a new Connection that has the pool.
If that connection is then used to begin a transaction, we create a new pool of 1 that is used in this Connection until commit/rollback.

I wonder if we could spy on Postgres.js' .unsafe function and assert it was called with isolation level. I'll dig around Kysely logging, maybe I could emit the query's details.

src/connection.ts Outdated Show resolved Hide resolved
@sds
Copy link
Contributor Author

sds commented Apr 26, 2023

Tests pass! Let me know if there's anything else you'd like addressed as part of this PR.

Copy link
Member

@igalklebanov igalklebanov left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@igalklebanov igalklebanov merged commit 60c47c4 into kysely-org:main Apr 26, 2023
@sds sds deleted the sds/transactions branch April 26, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants