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

[feature request] ability to reserve a single connection from the pool #603

Closed
igalklebanov opened this issue May 20, 2023 · 9 comments
Closed
Labels
enhancement New feature or request

Comments

@igalklebanov
Copy link

igalklebanov commented May 20, 2023

Hey 👋

Thank you @porsager for the impressive work on this library! ❤️

People have been requesting Kysely support for postgres.js for a few months now due to its superior performance.
So I have recently published a postgres.js dialect but it comes with caveats.

Kysely was built to wrap client libraries' pools. Ideally, a dialect should get a pool on instantiation, and on-the-fly acquire connections and execute queries/transactions.

This works well with postgres.js' API as far as single query scenario goes, but doesn't work in 2 scenarios:

  1. postgres.js' transaction API is great, but Kysely needs full control over a transaction's lifecycle (execute begin and commit/rollback) for its transaction API to work. So you need to acquire a single connection and use it for multiple queries. Since postgres.js doesn't provide a connection getter, I had to eventually hack a "swap main pool with a N=1 pool for transaction" solution. So the main pool can co-exist with potentially many short-lived pools of 1. This is not ideal if the consumer does not want to exceed the main pool's max size.

  2. Kysely has a single connection API that allows consumers to use a single connection within a callback and perform multiple queries on it. Close to transactions API but without the transaction part. This is impossible to implement right now, as postgres.js doesn't provide a connection getter, and Kysely's internals do not signal this scenario (can't do same swap done in first scenario). Making changes on Kysely's side here would be potentially breaking and quite awkward (signaling single connection mode when we should already be in that mode anyway, for an external dialect).

Was wondering if it's possible and simpler to add a connection getter/reservation in postgres.js, and whether its something you'd consider a good fit for the project?

@porsager porsager changed the title [feature request] ability to acquire a single connection from the pool [feature request] ability to reserve a single connection from the pool Jun 19, 2023
@porsager
Copy link
Owner

Hey @igalklebanov ...

There's a reserve branch now at https://github.com/porsager/postgres/tree/reserve . Would be could if you could give it a go and see how it fits your need.

There's no docs yet, but the api is fairly simple (not set in stone though):

const sql = postgres(..)

const reserved = await sql.reserve()
p(await reserved`select pg_backend_pid()`)
p(await sql`select pg_backend_pid()`)

reserved.release()

Looking forward to hear if that helps your implementation out.

@igalklebanov
Copy link
Author

igalklebanov commented Jun 22, 2023

Hey 👋

Amazing stuff @porsager! 💪

I've opened a pull request with the necessary changes:
kysely-org/kysely-postgres-js#5

It seems to be passing the current set of test cases but hangs when running the "after all" hook that tries ending the entire client.

@porsager
Copy link
Owner

Very cool! I missed clearing the reserved status of the connection, hence the hanging tests. Is fixed now

@porsager
Copy link
Owner

porsager commented Jun 25, 2023

There's still some tests I'd like to write, and the documentation as well, but time isn't on my side, so I'm thinking of just including it undocumented in a release if you find everything works well on your end?

@porsager porsager added the enhancement New feature or request label Jun 26, 2023
@igalklebanov
Copy link
Author

Very cool! I missed clearing the reserved status of the connection, hence the hanging tests. Is fixed now

Looking good on my end too! 💪

There's still some tests I'd like to write, and the documentation as well, but time isn't on my side, so I'm thinking of just including it undocumented in a release if you find everything works well on your end?

You've got [🟢 ⚫ ⚫] from me.

@tim-smart
Copy link
Contributor

Planning to cut a release soon?

I can contribute some types and docs if that helps.

@tim-smart
Copy link
Contributor

#667

@joshxyzhimself
Copy link

@porsager
Copy link
Owner

Oh right - Yay 🎉 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants