kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.71k stars 271 forks source link

[feature] Tell `acquireConnection` whether the connection is for a transaction or not #322

Closed cdcarson closed 1 year ago

cdcarson commented 1 year ago

Great library. Thanks. Taking the liberty to ping @jacobwgillespie since I'm mentioning kysely-planetscale below.

I'm using PlanetScale with their serverless driver. Based on a question I asked of them, it seems that it may be more efficient to maintain a singleton PlanetScale connection in a certain scope, e.g. over the lifecycle of a worker. The PlanetScale driver allows for this -- creating one long-lived connection for non-transactional queries and one short-lived connection for each transaction.

I've looked at kysely-planetscale to see if that's doable. I don't think it is, either in that library or in Kysely itself. The acquireConnection interface doesn't pass any parameters.

It looks like this could be possible, though. AFAICT, acquireConnection only gets called (albeit indirectly) in two places:

If that's the case it seems doable to pass a boolean isTransaction to acquireConnection. Folks who are using regular database connections could ignore it; folks like me could use it. I'm aware that you probably want keep the API clean, but I don't think this is an edge case or a case limited to one particular vendor.

Let me know what you think. Happy to help out if you like the idea. Thanks.

igalklebanov commented 1 year ago

Hey 👋

You can run multiple queries on a single connection using:

await db.connection().execute(async (db) => {
  await db.selectFrom('person').selectAll().execute()

  // ...
})

You could probably wrap your entire serverless function with this, pass the callback's db argument in the context.

**HACK** You can also obtain a Kysely instance bound to a single connection:

const connection = await db.connection().execute(async (db) => db)

await connection.selectFrom('person').selectAll().execute()

// ...

Thoughts against adding isTransaction prop:

If we do add such a thing, it should not be a primitive argument, but a field in an optional options object.

jacobwgillespie commented 1 year ago

This might be a hack, but could we do this in kysely-planetscale by re-using a global instance of the @planetscale/database connection? The connection is currently acquired here: https://github.com/depot/kysely-planetscale/blob/c93a194ab5d250885e05c4dfc00ea7ccb4c01353/src/index.ts#L109. I imagine that could be configured to re-use a global connection if it exists.

Transactions already re-acquire a new connection (https://github.com/depot/kysely-planetscale/blob/c93a194ab5d250885e05c4dfc00ea7ccb4c01353/src/index.ts#L141), so within that driver we do have the ability to distinguish between transactions and other queries. I believe it'd be possible to have transactions establish a new short-lived connection but have everything else use a global one.

jacobwgillespie commented 1 year ago

@cdcarson I've added an experimental useSharedConnection config option in kysley-planetscale v1.3.0: https://github.com/depot/kysely-planetscale/pull/12. Let me know if that works for you, if you see any performance impact I'd be interested to hear about it.

const db = new Kysely<DB>({
  dialect: new PlanetScaleDialect({
    url: '...',
    useSharedConnection: true,
  }),
})
cdcarson commented 1 year ago

@igalklebanov Thanks for the reply. Totally understand not wanting to expand the footprint in this way. I'll leave it up to you whether you want to close this issue.

@jacobwgillespie Thanks! That looks good, but at first glance I'm not exactly clear on how to use it -- i.e. do I create a new Kysely when I need to do a transaction? Anyway, I'll take a closer look later today at the PR and get back to you there.

As far as performance: I'm not at a point where I can actually test this against a real world situation (early days, no users yet.) I've been meaning to figure out a way to test a "warm" connection vs a new one, running the same queries over various intervals. I'll let you know. Thanks!

igalklebanov commented 1 year ago

I'll close this for now.

Please do comment in the future if the experiment @jacobwgillespie implemented doesn't work out, and a different approach is blocked due to a limitation of kysely's API.

jacobwgillespie commented 1 year ago

at first glance I'm not exactly clear on how to use it

@cdcarson it's all automatic, you should be able to use a single Kysely like normal, the driver will handle whether things are transactions or not on its own. For now it's experimental, so you'll need to set the new useSharedConnection: true dialect option, but if it ends up working well that could just become the default in a future release.