perf(session): reuse backend connections when possible#837
Conversation
When terminating gracefully and not in transaction, the ClientHandler can run a cleanup in the DbHandler and put it back to the pool. This reduces the overhead of opening new connections in the session mode.
mentels
left a comment
mentels
left a comment
There was a problem hiding this comment.
Looks good!
Leaving some comments.
| Logger.debug("ClientHandler: Performing session cleanup before termination") | ||
| {pool, db_pid, _} = data.db_connection | ||
|
|
||
| case DbHandler.attempt_cleanup(db_pid) do |
There was a problem hiding this comment.
I wonder if it wasn't be safer to have a timeout here as well.
Can we rule out a scenario in wich an "unrelated" event arrives to the DbHandler while in :waiting_cleanup and cancels the DbHandler :cleanup_timeout?
There was a problem hiding this comment.
Can we rule out a scenario in wich an "unrelated" event arrives to the DbHandler while in :waiting_cleanup and cancels the DbHandler :cleanup_timeout?
Great point!!
I should've used :state_timeout instead of :timeout which ensures this does not happen.
:state_timeout refers to the time spent in a state while :timeout refers to the time without processing messages (like in GenServer)
Will push a fix.
Other than that, I think we do a regular timeout in the gen:call just for sanity.
There was a problem hiding this comment.
Actually, there's yet another issue!
If the call fails - be it b/c of the :timeout or just b/c the gen_statem processes exists which is actually expected exec path here - the gen_statem:call exists the caller! And this will crash the client_handler.
So I guess to make it graceful we need to wrap it in the try catch OR make it async? (No, the latter sounds like a bad idea but "thinking out loud").
This makes sure we don't block other connections while we are doing the cleanup
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mentels
left a comment
mentels
left a comment
There was a problem hiding this comment.
Changes looks good but as I pointed out in the comment, I believe there's an issue.
|
|
||
| defp maybe_cleanup_db_handler(state, data) do | ||
| if state == :idle and data.mode == :session and data.db_connection != nil and | ||
| !Supavisor.Helpers.no_warm_pool_user?(data.user) do |
There was a problem hiding this comment.
🤯 takes a while to reason about this condition
| Logger.debug("ClientHandler: Performing session cleanup before termination") | ||
| {pool, db_pid, _} = data.db_connection | ||
|
|
||
| case DbHandler.attempt_cleanup(db_pid) do |
There was a problem hiding this comment.
Actually, there's yet another issue!
If the call fails - be it b/c of the :timeout or just b/c the gen_statem processes exists which is actually expected exec path here - the gen_statem:call exists the caller! And this will crash the client_handler.
So I guess to make it graceful we need to wrap it in the try catch OR make it async? (No, the latter sounds like a bad idea but "thinking out loud").
We are cutting the v2.8.10 release anyway and moving to v2.9.0 which will not be soft deploy compatible.
2 participants
When terminating gracefully and not in transaction, the ClientHandler can run a cleanup in the DbHandler and put it back to the pool. This reduces the overhead of opening new connections in the session mode, making supavisor a more performant session pooler, akin to PgBouncer.