Improve connection fail fast logic #998
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 22, 2023
|
Thanks a lot for looking into it! |
JelteF
force-pushed
the
improve-connection-fail-fast
branch
from
052e7a6 to
e366f63
Compare
For a very long time, we've had logic to quickly close all clients when the a server could not be connected to. This wasn't working correctly in all cases, one of those was when an auth_user was set (and no auth_db). This was reported in pgbouncer#989. While looking closer into that issue I realized it also wouldn't handle cases where first connecting worked but then after a while it doesn't anymore, because it relied only on `wait_for_welcome`. It's not strictly necessary to kill all waiting connections when creating new server connections is not possible anymore, as long as there are still working server connections. But if those don't exist either, then we're effectively in the same state as complete refresh (except that we have cached the welcome message). Fixes pgbouncer#989 Fixes pgbouncer#985 Co-Authored-By: Anna Khanova <annakhanova@neon.tech>
JelteF
force-pushed
the
improve-connection-fail-fast
branch
from
e366f63 to
171ec14
Compare
emelsimsek
reviewed
Jan 15, 2024
| kill_pool_logins_server_error(server->pool, pkt); | ||
| else | ||
| log_server_error("S: login failed", pkt); | ||
| kill_pool_logins_server_error(server->pool, pkt); |
There was a problem hiding this comment.
Why to remove the below log?
log_server_error("S: login failed", pkt);
There was a problem hiding this comment.
We now always call kill_pool_logins_server_error and that already logs the error using:
log_warning("server login failed: %s %s", level, msg);(it's a warning not an error this time, but that seems like a not very important change)
|
Changes look fine to me. |
5 tasks
bayandin
added a commit
to neondatabase/neon
that referenced
this pull request
Feb 2, 2024
## Problem Update pgbouncer from 1.21 (and patches[0][1]) to 1.22 (which includes these patches) - [0] pgbouncer/pgbouncer#972 - [1] pgbouncer/pgbouncer#998 ## Summary of changes - Build pgbouncer 1.22.0 for neonVMs from upstream
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For a very long time, we've had logic to quickly close all clients when
the a server could not be connected to.
This wasn't working correctly in all cases, one of those was when an
auth_userwas set (and noauth_db). This was reported in #989.While looking closer into that issue I realized it also wouldn't handle
cases where first connecting worked but then after a while it doesn't
anymore, because it relied only on
wait_for_welcome. It's not strictlynecessary to kill all waiting connections when creating new server
connections is not possible anymore, as long as there are still working
server connections. But if those don't exist either, then we're
effectively in the same state as complete refresh (except that we have
cached the welcome message).
Fixes #989
Fixes #985