Skip to content

Add db client connection limit#1138

Merged
JelteF merged 38 commits intopgbouncer:masterfrom
AndrewJackson2020:db_client_limit
Nov 2, 2024
Merged

Add db client connection limit#1138
JelteF merged 38 commits intopgbouncer:masterfrom
AndrewJackson2020:db_client_limit

Conversation

@AndrewJackson2020
Copy link
Contributor

@AndrewJackson2020 AndrewJackson2020 commented Aug 9, 2024

Related to #1137. This PR allows the user to configure client connection limits at the database level. This prevents issues where SQL clients all connecting to a single database consume the entire connection limit defined in max_client_conn which effectively locks all other databases from being accessed.

@AndrewJackson2020 AndrewJackson2020 force-pushed the db_client_limit branch 3 times, most recently from 473c791 to dbec783 Compare August 9, 2024 19:46
AndrewJackson2020 and others added 13 commits August 10, 2024 21:04
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
This PR cleans up some instances of trailing whitespace in config.md.
Minor issue but some confusion can arise as many people configure their
code editors to remove trailing whitespace from markdown files on save.

Co-authored-by: CommanderKeynes <andrewjackson947@gmail.coma>
Today I was looking to crank the log verbosity up to
hopefully diagnose a vexing issue and had to look at the source to learn
that setting it to anything higher than 3 would be useless.
@AndrewJackson2020
Copy link
Contributor Author

Found a similar issue with this PR as with #1137 wherein the increment was not done in the correct place. This should be fixed now and added an extra assert in one of the test cases.

The implementation of #1137 is a bit more complicated than this one in that databases don't get created in multiple places like users. Because of this I did not encapsulate the limit check into a function. Let me know if you want this in a function anyways for consistency sake.

src/client.c Outdated
Comment on lines +435 to +437
if (database_max_client_connections(client->db) > 0) {
/* increment count now, so that we can decrement it safely in disconnect_client if limit was reached */
client->db->client_connection_count++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using an auth_user something is still not correct here. If you try to connect using a user that does not exist the count is not decremented correctly somehow.

auth_user does work in a bit of a weird way. See start_auth_query for some details, as well as this PR where I'm trying to fix another issue that popped up due to the confusing way in which it works: #1166

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am noticing the same thing in the #1137. Specifically I'm seeing that it logs in 4 different times initially and of those 4 times 3 logins are permanently retained. Then every additional time you log in it "logs in" 2 times and of those 2 log ins one is not decremented. Definitely something weird with this implementation as is and definitely not something we can merge in as is.

$ psql  'postgresql://maxedout3@127.0.0.1:10203/pgbouncer' -c 'show users' | grep pswcheck_not_in_auth_file
 pswcheck_not_in_auth_file |         1 |           |                    0 |                   0 |                         100 |                          0

$ psql  'postgresql://pswcheck_not_in_auth_file@127.0.0.1:10203/authdb'  -c select 1;
--
(1 row)


$ psql  'postgresql://maxedout3@127.0.0.1:10203/pgbouncer' -c 'show users' | grep pswcheck_not_in_auth_file
 pswcheck_not_in_auth_file |         1 |           |                    0 |                   0 |                         100 |                          3

$ psql  'postgresql://pswcheck_not_in_auth_file@127.0.0.1:10203/authdb'  -c select 1;
--
(1 row)


$ psql  'postgresql://maxedout3@127.0.0.1:10203/pgbouncer' -c 'show users' | grep pswcheck_not_in_auth_file
 pswcheck_not_in_auth_file |         1 |           |                    0 |                   0 |                         100 |                          4
$ psql  'postgresql://pswcheck_not_in_auth_file@127.0.0.1:10203/authdb'  -c select 1;
--
(1 row)


$ psql  'postgresql://maxedout3@127.0.0.1:10203/pgbouncer' -c 'show users' | grep pswcheck_not_in_auth_file
 pswcheck_not_in_auth_file |         1 |           |                    0 |                   0 |                         100 |                          5

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So super hacky solution but I was able to fix this by essentially adding an additional boolean variable to PgSocket that specifies whether or not that client has been contributed to the total count of clients. This ensures that the recursive set_pool calls only increment the count one time. Will push this in a bit, after that we can figure out if there is a better way of doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF Pushed the hacky solution mentioned in the last comment. Similar solution implemented for #1137. Also expanded test coverage to admin users, stat users, and auth pass through users.

Some macOS tests are failing but I believe these are due to flaky tests similar to #1137 as the tests that are failing are different between the two test runs. Its possible that I might have made some changes that impact that global state of the test running but I don't understand why it would not impact linux but would impact macOS. Hard for me to look into this further as I don't have a dev environment set up for MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JelteF I believe that I fixed the macOS tests by removing my usage of the run_with_config method. I think something about how this is implemented makes tests flakier on macOS but I do not have a development environment to recreate and try to fix this.

Either way please lmk what you think about this updated solution.

Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
AndrewJackson2020 and others added 4 commits October 7, 2024 17:02
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
@JelteF
Copy link
Member

JelteF commented Oct 28, 2024

I'm not sure what's up with this one, but every CI job seems to be failing. I seem to remember this was basically good to merge. Was there a bad merge conflict resolution or something??

@AndrewJackson2020
Copy link
Contributor Author

Bad merge on my end, apologies for that.

@AndrewJackson2020
Copy link
Contributor Author

Fixed the tests. The merge created some excessive line by line diffs that I can clean up as well.

@JelteF
Copy link
Member

JelteF commented Oct 30, 2024

Another merge conflict... I guess after I merged #1193

@AndrewJackson2020
Copy link
Contributor Author

Another merge conflict... I guess after I merged #1193

I will take care of it.

@AndrewJackson2020
Copy link
Contributor Author

Another merge conflict... I guess after I merged #1193

@JelteF Fixed the merge conflict. I think that the currently failing test is a flaky test.

@JelteF JelteF enabled auto-merge (squash) November 2, 2024 10:31
@AndrewJackson2020
Copy link
Contributor Author

Again, looks like the failure is a flaky test. I've noticed this seems to happen more on windows than other platforms.

@JelteF JelteF merged commit d82085a into pgbouncer:master Nov 2, 2024
@AndrewJackson2020 AndrewJackson2020 deleted the db_client_limit branch November 2, 2024 17:37
rajaryanece pushed a commit to rajaryanece/pgbouncer that referenced this pull request Jan 16, 2025
Related to pgbouncer#1137. This PR allows the user to configure client connection
limits at the database level. This prevents issues where SQL clients all
connecting to a single database consume the entire connection limit
defined in max_client_conn which effectively locks all other databases
from being accessed.
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