Add a dedicated cancel_wait_timeout #833
Merged
Conversation
This file contains 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
Cancellations and regular queries are quite different. So it makes sense to have different (default) timeouts for cancellation requests than for queries. While I already think the default query_wait_timeout of 120 seconds we have is excessively long, it doesn't make sense at all to have such a long timeout by default for cancellation requests that are not sent. So this introduced a new `cancel_wait_timeout` which is set to 10 seconds by default. If the cancellation request cannot be forwarded within that time it's probably better to drop it. One reason why it's important to relatively quickly close cancellation requests when they are not being handled is because libpq only allows the client to send a cancel in a blocking way. So there's no possibility to bail out after a certain amount of seconds on the client side, if PgBouncer keeps the connection to the client open. Both me and Marco noticed this when testing my PgBouncer peering PR (pgbouncer#666), when an entry in the `[peers]` list pointed to an unreachable host. That meant that pgbouncer would keep the cancel connection open, and even though the query had finished the `psql` would not display the result because it was stuck waiting for a result to the cancel request. To avoid race conditions of cancellations that already forwarded, but not yet answered cancellation requests are not timed out. See pgbouncer#717 for details on such race conditions.
JelteF
force-pushed
the
dedicated-cancel-timeout
branch
from
2622325
to
c09756e
Compare
JelteF
changed the title
petere
reviewed
Apr 25, 2023
| @@ -266,6 +266,12 @@ auth_file = /etc/pgbouncer/userlist.txt | |||
| ;; failure. (default: 120) | |||
| ;query_wait_timeout = 120 | |||
|
|
|||
| ;; Dangerous. Client connection is closed if the cancellation request | |||
There was a problem hiding this comment.
I wonder if the "Dangerous" is really appropriate here, or was it just copied from the query_ settings?
There was a problem hiding this comment.
Disabling it is definitely dangerous, but yeah it was mostly copied.
petere
approved these changes
Apr 25, 2023
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.
Cancellations and regular queries are quite different. So it makes sense
to have different (default) timeouts for cancellation requests than for
queries. While I already think the default query_wait_timeout of 120
seconds we have is excessively long, it doesn't make sense at all to
have such a long timeout by default for cancellation requests that are
not sent. So this introduced a new
cancel_wait_timeoutwhich is set to10 seconds by default. If the cancellation request cannot be forwarded
within that time it's probably better to drop it.
One reason why it's important to relatively quickly close cancellation
requests when they are not being handled is because libpq only allows
the client to send a cancel in a blocking way. So there's no possibility
to bail out after a certain amount of seconds on the client side, if
PgBouncer keeps the connection to the client open. Both me and Marco
noticed this when testing my PgBouncer peering PR (#666), when an entry
in the
[peers]list pointed to an unreachable host. That meant thatpgbouncer would keep the cancel connection open, and even though the
query had finished the
psqlwould not display the result because itwas stuck waiting for a result to the cancel request.
To avoid race conditions of cancellations that already forwarded, but
not yet answered cancellation requests are not timed out. See #717 for
details on such race conditions.