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

add a setting to allow cancelations to bypass any pool limit checks #543

merged 3 commits into from May 26, 2021


Copy link

@thanodnl thanodnl commented Oct 22, 2020

As described in #245 this is the first patch in a series of patches to improve the cancelation of a backend when pgbouncer is involved.

In this patch we introduce the cancel_bypass_pool_size setting. This setting is interpreted as a boolean and should be set to either 0 or 1. When set to 1 a queued cancelation inside pgbouncer will always allow the creation of a new connection. Even if the creation of a new connection would normally not be allowed due to violation of pool sizes.

Since pgbouncer will use new connections first for cancelations and postgres doesn't count a cancelation request towards the max_connection setting on the database, bypassing any pool size limits should be safe. However, since this can introduce more connections to be opened I have chosen to make the setting off by default. Even though I think running with this setting off could cause issues if your application issues cancelations under load. Overtime, when we see no negative sideeffects of this bypass we could re-evaluate if the setting should be turned on by default.

The current implementation is to allow (force) a new connection when we have cancelation requests queued. With a limit of 2 * the current pool size. As suggested on the PR, this allows for every connection that can be established to receive a cancelation while keeping an upper bound on the number of outgoing connections.

Copy link

petere commented Feb 9, 2021

It seems to me that this ought to be the default behavior, no option necessary. Thoughts?

Copy link
Contributor Author

thanodnl commented Feb 9, 2021

You make a good point, I feel it should be default behaviour.

I created the patch defensively as it changes production behaviour and would actually allow pg_bouncer to exceed the upper limit of connections. To not change anyone's installation I decided to make it configurable.

Should I remove the option?

Edit: to clarify the above statement

would actually allow pg_bouncer to exceed the upper limit of connections

This is only true in the strict sense of the word. it can and will create more connections outgoing. These will solely be used to process the cancelations. As far as I can tell, the cancelation happens in postgres before the connection limit is enforced. Meaning, pgbouncer opening an extra connection on top of the configured connection ought to be no problem.

Copy link

petere commented Feb 18, 2021

I have talked about this with some people. I think there are concerns on both sides: Allowing an unbounded number of connections on the one hand, and adding more things you need to configure on the other.

How about a compromise: We allow additional cancel connections up to the pool size. So if the pool size is 50, we allow another 50 connections for canceling. That way, each live connection should get a good chance to get its cancel request through, but we don't allow an unbounded number of connections.

Copy link
Contributor Author

I agree with both concerns of configuration at its unbound nature. I like your suggestion of limiting the connections. Let me change the PR to incorporate those changes.

I will also try and find some scenarios where it can actually open 2 new connections at the same time for cancelation. I vaguely remember only seeing 1 extra connection, even during bursts of cancelations. However, this might be depending on the network conditions.

@thanodnl thanodnl force-pushed the add-cancelation-pool-size-bypass branch from e9ec349 to 90eea05 Compare March 25, 2021 14:27
Copy link
Contributor Author

I have removed the configuration and added a limit of twice the current pool size. This makes the change significantly easier as well.

I don't think it would have allowed more connections already compared to pool size + 1 due to the check at the top of the function:


Lines 1138 to 1141 in e4beeab

if (!statlist_empty(&pool->new_server_list)) {
log_debug("launch_new_connection: already progress");

Although it is hard to proof for me.

With the extra check there is some more certainty we have a bounded number of connections that pgbouncer has open at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants