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

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

Open
tmrclark opened this issue Jun 25, 2024 · 6 comments
Open
Labels
breaking change Includes breaking changes built-in dialect Related to a built-in dialect enhancement New feature or request mssql Related to MS SQL Server (MSSQL)

Comments

@tmrclark
Copy link
Contributor

tmrclark commented Jun 25, 2024

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.

@igalklebanov igalklebanov added built-in dialect Related to a built-in dialect mssql Related to MS SQL Server (MSSQL) labels Jun 26, 2024
@tmrclark
Copy link
Contributor Author

tmrclark commented Jul 2, 2024

@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.

@koskimas
Copy link
Member

koskimas commented Jul 9, 2024

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

@igalklebanov
Copy link
Member

Hey 👋

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

@tmrclark
Copy link
Contributor Author

tmrclark commented Jul 9, 2024

@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.

@igalklebanov
Copy link
Member

igalklebanov commented Jul 9, 2024

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 igalklebanov added the breaking change Includes breaking changes label Jul 9, 2024
@tmrclark
Copy link
Contributor Author

tmrclark commented Jul 9, 2024

Sounds reasonable to me.

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

@igalklebanov igalklebanov added the enhancement New feature or request label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Includes breaking changes built-in dialect Related to a built-in dialect enhancement New feature or request mssql Related to MS SQL Server (MSSQL)
Projects
None yet
Development

No branches or pull requests

3 participants