kysely icon indicating copy to clipboard operation
kysely copied to clipboard

Mssql connection pooling issue. Multiple server round trips per query.

Open tmrclark opened this issue 1 year ago • 6 comments

The problem

I was looking through OpenTelemtry traces of an application that uses Kysely with mssql and noticed that for every user created Kysley query, 3 SQL queries get sent to the server in succession.

The first is sent when a connection is acquired from the tarn connection pool. Every time this happens, tarn calls the provided validate method, which in this case sends a query.

The seconds is the user defined query.

The third happens with the connection is released back to the connection pool. The releaseConnection method calls the PRIVATE_RELEASE_METHOD, which calls the reset method defined in tedious, which in turn executes this query.

The 2 bookending queries create additional application latency by adding 2 round trips to the database server. I am assuming that this behavior is not expected and that there should only be one round trip to the database per user defined query. If I am wrong, please let me know.

How to reproduce

I created a demo repo with steps to reproduce this with trace logging and example traces.

Possible solution

I think the easiest and safest way to address this is to allow the user to configure the pool validation function and if the tedious reset method should be called when a connection is released. The default behavior can be the same as it is now, but this gives the opportunity to opt out.

tmrclark avatar Jun 25 '24 19:06 tmrclark

@igalklebanov @koskimas Any input on ways this could be addressed beyond what I suggest? I hope to implement a fix when time allows some time this week or the next.

tmrclark avatar Jul 02 '24 11:07 tmrclark

Unfortunately I know nothing about the dialect or mssql in general. @igalklebanov is needed for this one.

koskimas avatar Jul 09 '24 08:07 koskimas

Hey 👋

Are there any risks? Does the mssql library (a higher level abstraction over tedious) allow/do something similar?

igalklebanov avatar Jul 09 '24 09:07 igalklebanov

@igalklebanov The mssql has the same pool connection validation functionality. It does not appear to ever call tedious.reset and does not perform any connection reset when releasing a connection back to the pool.

I looked into how other db drivers handle acquiring and releasing connections from a pool. The pg driver has an (undocumented?) optional verify function that is called whenever a connection is acquired. If it is not provided, there is no connection verification. It also does not do any sort of connection reseting when a connection is released to the pool.

From what I can tell mysql2 does not do connection verification or resetting.

So from what I can tell, making this change is not enabling something out of the ordinary.

tmrclark avatar Jul 09 '24 14:07 tmrclark

OK.

Let's validate like in mssql - allow to not execute validation query via configuration flag (maybe validateConnections in additional tarn options), execute validation query by default. This doesn't introduce a breaking change.

Let's split the reset change into 2 efforts:

  1. a PR that allows to not call reset via configuration flag (maybe resetConnectionOnRelease in additional tedious options?). By default, it'll call reset. This doesn't introduce a breaking change.
  2. a PR that flips the default behavior - should not call reset now. We want to be aligned with mssql here, but still support the legacy behavior. This introduces a breaking change.

igalklebanov avatar Jul 09 '24 14:07 igalklebanov

Sounds reasonable to me.

Here is a PR to add the new configs. resetConnectionOnRelease defaults to true.

tmrclark avatar Jul 09 '24 15:07 tmrclark

Extracted these options to root of config. Flipped resetConnectionsOnRelease to false by default.

This thing will hit in #1278.

igalklebanov avatar Mar 16 '25 14:03 igalklebanov