Add client user connection limit by AndrewJackson2020 · Pull Request #1137 · pgbouncer/pgbouncer

AndrewJackson2020 · 2024-08-09T03:18:47Z

This PR adds a user level client connection limit. I believe that currently user connections can only be limited by how many people are given connections to the database backends but there is no way to limit the number of connections to pgbouncer itself. Combined with the global connection limit this means that one user could use up all connections on the pgbouncer instance. This PR proposes a fix to this problem.

Related to #1136 (because it partially fixes it)

JelteF

This seems like a good solution to the described problem. A similar setting would probably be useful per database, but that doesn't need to be in this PR.

Some things that should be changed about the PR:

Github Actions should be green (fix C warning and fix formatting)
A test should be added (test/test_limits.py seems like a good place to add one)
The new option should be documented in doc/config.md

AndrewJackson2020 · 2024-08-09T15:28:04Z

This seems like a good solution to the described problem. A similar setting would probably be useful per database, but that doesn't need to be in this PR.

Some things that should be changed about the PR:iss
1. Github Actions should be green (fix C warning and fix formatting)

2. A test should be added (`test/test_limits.py` seems like a good place to add one)

3. The new option should be documented in `doc/config.md`

@JelteF I believe the 3 points above have been addressed, can I get another review?

Also I opened #1138 to address this issue from the database side.

One question: should the values of max clients and current clients connected be added to the output of show users?

eulerto · 2024-08-09T17:33:54Z

I didn't review this PR yet but I noticed that you did lots of changes in the documentation that is not related to this feature. If you want to propose a whitespace cleanup, create a separate PR. We generally use /* comment */ instead of // comment.

BTW, I'm fine with this feature. It is equivalent to CREATE ROLE ... CONNECTION LIMIT.

AndrewJackson2020 · 2024-08-09T17:38:00Z

I didn't review this PR yet but I noticed that you did lots of changes in the documentation that is not related to this feature. If you want to propose a whitespace cleanup, create a separate PR. We generally use /* comment */ instead of // comment.

BTW, I'm fine with this feature. It is equivalent to CREATE ROLE ... CONNECTION LIMIT.

Apologies about that, my editor eliminates trailing whitespace default. Will revert momentarily.

AndrewJackson2020 · 2024-08-09T18:12:24Z

I didn't review this PR yet but I noticed that you did lots of changes in the documentation that is not related to this feature. If you want to propose a whitespace cleanup, create a separate PR. We generally use /* comment */ instead of // comment.

BTW, I'm fine with this feature. It is equivalent to CREATE ROLE ... CONNECTION LIMIT.

I reverted the whitespace removal and changed the comment style to conform with the rest of the codebase.

JelteF

Thanks! That looks a lot better already. I left a review just now on your other #1138 PR. Most of the feedback there should also be applied here. Other than that it looks pretty good.

I still have want to do some manual tests of both PRs though

AndrewJackson2020 · 2024-08-11T04:23:10Z

@JelteF Made all of the documentation changes and added a test of the stats command in the unit tests. Also split the unit test out into 2 separate ones: one that is meant to deny a connection and one that is meant to accept a connection, I feel this is a bit cleaner.

A few additional thoughts:

Should this PR and Add db client connection limit #1138 be merged because they are so similar and have similar feedback?
Should the name of the variables be changed from max_user_client_connections/max_user_connections to max_user_client_connections/max_user_server_connections?
Should the tests of the stats command be moved to test/test_admin.py or is it fine as is?

JelteF · 2024-08-18T09:05:50Z

1. Should this PR and [Add db client connection limit #1138](https://github.com/pgbouncer/pgbouncer/pull/1138) be merged because they are so similar and have similar feedback?

Let's keep them separate

2. Should the name of the variables be changed from max_user_client_connections/max_user_connections to max_user_client_connections/max_user_server_connections?

Let's keep them as is. I don't think the better naming (because I agree it is a bit confusing now) is worth breaking backwards compatibility of the config setting names.

3. Should the tests of the stats command  be moved to test/test_admin.py or is it fine as is?

It's fine as is imho.

JelteF

Sorry for the delay in reviewing. I think this is almost good now, some final suggested changes.

AndrewJackson2020 · 2024-09-19T13:58:04Z

@JelteF Pushed a new commit that fixes a bunch of the issues the issues that you mentioned.

Found that the auth pass through issue that you mentioned in #1138 applies to this PR as well. Implemented a somewhat hacky fix to this. Would appreciate your feedback on this or if you have any alternative ideas.

Also expanded unit test coverage for stats users and auth pass through users. One area that isnt covered is PAM but looking through the unit tests I dont think there is any test coverage of PAM currently so I think adding that here might be out of scope for this PR though it would be very beneficial.

Also I believe that the macOS failures are flaky tests but I'm not sure. I have no local dev environment for macOS so its kind of hard for me to figure out what is happening there.

AndrewJackson2020 · 2024-09-23T12:13:52Z

@JelteF Fixed the macOS tests by scaling back the run_with_config usage as described in #1138.

Let me know if this solution works for you or if it needs to be reworked more.

AndrewJackson2020 · 2024-10-04T16:05:12Z

@JelteF Anything else I can do here to help this PR get merged?

JelteF · 2024-10-04T18:40:54Z

No, it's blocked on me finding some time to review PgBouncer PRs again. Busy busy.

JelteF · 2024-10-07T20:48:32Z

+	if (!client->login_user_credentials->global_user)
+		return true;
+
+	if (!client->user_connection_counted) {


I agree this is not an elegant solution. But given the amount of counting bugs found in earlier versions, I prefer this way because this one is obviously correct and will stay correct in the future. I added a check for user_connection_counted in the decrement path too.

AndrewJackson2020 · 2024-10-26T14:10:59Z

@JelteF anything else you want done with this PR before merging?

JelteF · 2024-10-28T22:02:17Z

I don't remember why I didn't press merge yet, maybe because CI was failing for unrelated reasons.

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.

This PR adds a user level client connection limit. I believe that currently user connections can only be limited by how many people are given connections to the database backends but there is no way to limit the number of connections to pgbouncer itself. Combined with the global connection limit this means that one user could use up all connections on the pgbouncer instance. This PR proposes a fix to this problem. Related to pgbouncer#1136 (because it partially fixes it)

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.

AndrewJackson2020 mentioned this pull request Aug 9, 2024

Client Connection Limits Per User/DB #1136

Open

JelteF requested changes Aug 9, 2024

View reviewed changes

Comment thread include/bouncer.h Outdated

Comment thread include/bouncer.h Outdated

Comment thread include/bouncer.h Outdated

Comment thread src/client.c Outdated

AndrewJackson2020 mentioned this pull request Aug 9, 2024

Add db client connection limit #1138

Merged

JelteF requested changes Aug 10, 2024

View reviewed changes

AndrewJackson2020 requested a review from JelteF August 15, 2024 22:12

JelteF reviewed Aug 18, 2024

View reviewed changes

Comment thread src/objects.c

CommanderKeynes added 18 commits August 18, 2024 13:54

Add client user connection limit

ce357a0

Changed comments around user connection limits

be16da2

Remove extraneous space

e891e78

Add documentation

fe97c57

Format

80de339

Move variable declaration

9439a9f

Add option to etc/pgbouncer.ini

c03e41d

Whatever

38489da

Uncrustify

fc025f8

Add tests

0f778d4

Uncrustify

614da19

Attempt fix

6cb5696

Uncrustify

72da2dd

Added back trailing whitespaces

a9184e9

Changed comment style

8bb1360

Changeg comment style

bbdfab2

Whatever

624e9e9

Uncrustify

504fc61

JelteF requested changes Sep 17, 2024

View reviewed changes

Comment thread doc/config.md Outdated

Comment thread src/admin.c Outdated

Comment thread src/client.c

Andrew Jackson and others added 7 commits September 18, 2024 17:13

fix auth pass throufh issue

f227fb7

format python

fb7cf28

C format

7ad0eb1

Merge branch 'master' into user_client_limit

c15f3cf

attempt to fix windows

26d4370

fix test

28f8ed3

clean up tests

9013040

Andrew Jackson and others added 2 commits September 21, 2024 20:39

remove test specific configs

aabecba

Merge branch 'master' into user_client_limit

f876fb5

JelteF approved these changes Oct 7, 2024

View reviewed changes

Comment thread src/client.c Outdated

JelteF added 2 commits October 7, 2024 22:42

Remove out of date comment.

a56fbb2

Use user_connection_counted when disconnecting too

147f6c7

JelteF reviewed Oct 7, 2024

View reviewed changes

Merge branch 'master' into user_client_limit

3eca84e

AndrewJackson2020 mentioned this pull request Oct 7, 2024

Attempt to fix inconsistent error #1152

Merged

Merge branch 'master' into user_client_limit

2d7283d

JelteF merged commit 585a630 into pgbouncer:master Oct 28, 2024

AndrewJackson2020 deleted the user_client_limit branch October 28, 2024 22:05

JelteF mentioned this pull request Dec 3, 2024

Allow setting per-user connection limits to unlimited, improve logging #204

Closed

AndrewJackson2020 mentioned this pull request Jan 2, 2025

Fix bug when decrementing the user connection count #1238

Merged

Conversation

AndrewJackson2020 commented Aug 9, 2024 • edited by JelteF Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelteF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJackson2020 commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eulerto commented Aug 9, 2024

Uh oh!

AndrewJackson2020 commented Aug 9, 2024

Uh oh!

AndrewJackson2020 commented Aug 9, 2024

Uh oh!

JelteF left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJackson2020 commented Aug 11, 2024

Uh oh!

Uh oh!

JelteF commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JelteF left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJackson2020 commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJackson2020 commented Sep 23, 2024

Uh oh!

AndrewJackson2020 commented Oct 4, 2024

Uh oh!

JelteF commented Oct 4, 2024

Uh oh!

Uh oh!

JelteF Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJackson2020 commented Oct 26, 2024

Uh oh!

JelteF commented Oct 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AndrewJackson2020 commented Aug 9, 2024 •

edited by JelteF

Loading

AndrewJackson2020 commented Aug 9, 2024 •

edited

Loading

JelteF commented Aug 18, 2024 •

edited

Loading

AndrewJackson2020 commented Sep 19, 2024 •

edited

Loading

JelteF Oct 7, 2024 •

edited

Loading