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

Don't evict connections to achieve min_pool_size #648

Merged
merged 2 commits into from
Aug 15, 2022

Conversation

waltercacau
Copy link
Contributor

@waltercacau waltercacau commented Oct 21, 2021

This should prevent us from evicting older idle connections unnecessarily if the max connection limit for the user or db has been reached.

This should address issue: #647

I have ran test/test.sh and all tests are passing (including two newly added ones).
I have also tested locally reverting the checks I added to janitor and verified that the newly added tests fail.

…ction limits before creating new connections to ensure the pool has a minimum size.

This should prevent us from evicting older idle connections unnecessarily if the max connection limit for the user or db has been reached.
@petere
Copy link
Member

petere commented Mar 16, 2022

I wonder whether a different way of structuring this might be possible. I don't like that we have to repeat the whole logic of checking the parameters in your check_max_connection_limits(). (Also, by the way, the return value of that function is not obvious. What does "true" or "false" as a return from "check" mean?) Perhaps we could add an argument to launch_new_connection() that specifies whether we want to run the "evict" code.

The tests are nice, but they each add >=7 seconds to the total run time, which seems quite a lot. Is there a way to get that shorter?

1) removed check_max_connection_limits and instead added a new parameter
   to launch_new_connection that allows eviction behavior to be turned on/off.
2) adjust tests to run with sleep value of 1s instead of 7s. Also I was
   able to rerun the adjusted tests against pgbouncer built from master
   and confirm the tests failed (this detecting the issue).
@waltercacau
Copy link
Contributor Author

Hi Peter, thanks for the review!

I have modified the PR to use your suggested approach (new parameter to launch_new_connection).

Also I adjusted the tests parameters so they would only need a sleep wait of 1 second. I confirmed that the new tests fail using a pgbouncer built before my changes (thus they are able to detect the issue).

@pgbouncer pgbouncer deleted a comment from mmontagna Aug 12, 2022
@pgbouncer pgbouncer deleted a comment from atanner27 Aug 12, 2022
@JelteF JelteF changed the title Checking whether the db or the user of a pool has hit their max connection limits before creating new connections to ensure the pool has a minimum size. Don't evict connections to achieve min_pool_size Aug 15, 2022
@JelteF JelteF merged commit 9ba0394 into pgbouncer:master Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants