Maintain `min_pool_size` for pools with forced user (even if no client conns) #947

fschmager · 2023-09-15T12:19:28Z

Continuation/Restart of PR #437 (shoutout to @barqshasbite) to address issue #426.

The default behaviour of PGBouncer requires that clients be connected to a pool before it is initialized. There is no way to toggle this behaviour to ensure that a pool is initialized immediately by PGBouncer.

This means that min_pool_size will only be respected if clients are connected. If all clients disconnect, then the pool can drop below the min_pool_size setting to 0 (server_lifetime will eventually close all connections). This can cause performance problems when a client finally reconnects since all of the pool connections have to be recreated.

This PR changes things so that PgBouncer (creates and) maintains pools that have user connections and, this is new, for those pools that have force users.

This is changing behavior as min_pool_size will now be enforced even if there are no client connection.

fschmager · 2023-09-15T12:22:57Z

src/janitor.c

+	 * Creating new pools to enable `min_pool_size` enforcement even if there are no active clients
+	 * (min_pool_size_requires_clients=0).
+	 *
+	 * If clients never connect there won't be a pool to maintain the min_pool_size on, which means we have to
+	 * proactively create a pool, so that it can be maintained.
+	 *
+	 * We are doing this only for forced users to reduce the risk of creating connections in unexpected ways, where
+	 * there are many users. Are all database/user combos even known ahead of time?
+	*/
+	statlist_for_each_safe(item, &database_list, tmp) {
+		db = container_of(item, PgDatabase, head);
+		if (database_min_pool_size_without_clients(db) &&
+			database_min_pool_size(db) > 0 &&
+			db->forced_user != NULL) {
+			get_pool(db, db->forced_user);
+		}
+	}


I believe this is the crucial part. Creating pools to maintain the min_pool_size. Pools needs a database and a user. If we don't have a user we can't create a pool.
Are all user/database combinations known or are there ways to configure users externally? Do we want that? Is doing this just for forced users sufficient and clear (unsurprising) enough?

For my use case, enabling this for forced users is sufficient.

I believe this is the crucial part. Creating pools to maintain the min_pool_size. Pools needs a database and a user. If we don't have a user we can't create a pool.

I agree.

Are all user/database combinations known or are there ways to configure users externally?

Using auth_query, users can be dynamically loaded. So no, not all combinations are known.

Do we want that? Is doing this just for forced users sufficient and clear (unsurprising) enough?

I think preferably we'd have a way to specify what pools (i.e. database+user combinations) should always be maintained. Something like this (suggests for better config name welcome):

mydb = min_pool_size=3 ensure_pools_exist_for=postgres,jelte,fschmager

We could even have that option default to the forced user if the forced user is specified. Which I think would make sense as a default. So this would already automatically create the pool (and thus maintain the min_pool_size):

mydb = min_pool_size=4 user=fschmager

I was being conservative to maintain back-compat. The "maintain pools for forced users"-feature should be an opt-in as not to change existing behavior.

A blunt global setting to opt in might just do the trick (name up for debate):

create_pools_for_forced_users = 1

I don't know enough about how the community is using pgbounce. I just know my own use case, and for me "global opt-in + forced users == create & maintain pool" is exactly what I need. Feels like the most straightforward and least surprising path.

If we want to make it more complex:

Do we need an opt-out on a per-pool basis?

Do we need a way to maintain pools for non-forced users?

Maybe this is our increment. I add the global opt-in to maintain min_pool_size for dbs with forced users. If we see that folks want pool-specific opt-out or opt-in for non-forced users we can add that later depending on demand.

I was being conservative to maintain back-compat.

Yeah I understood that, but I don't think it's worth the extra config in this case (we already have way too many of them). Automatically maintaining a pool for forced users makes a lot of sense to me. As long as you don't set min_pool_size the behaviour will be the same. And if you do set min_pool_size I think the only reasonable thing is to actually maintain that min_pool_size from startup.

I think it would be useful to specify what pools to create for non-forced users, but I don't think it's strictly necessary for this PR.

There's one thing that I think should be considered though: What happens when PgBouncer starts a little earlier than postgres and thus the janitor cannot initiate the connections. But then once postgres is started a client connects. There might be some PgBouncer timeouts in play that we should ignore in that case, to actually retry opening a connection.

if you do set min_pool_size I think the only reasonable thing is to actually maintain that min_pool_size from startup

I agree, this was exactly my assumption. I think we can improve the situation with documentation of min_pool_size. Adding the caveat that this only works for forced users, especially on the global min_pool_size end. The term "forced users" might not be widely know and might need a bit of word smithing.

I'd be more than happy to drop the setting flags.

There might be some PgBouncer timeouts in play that we should ignore in that case, to actually retry opening a connection.

Let me think out loud to explain what I understand.

no "forced pool" (for want of a better word) scenario:
PgBouncer starts up, client connects, but postgres isn't available yet. Connection timeout might kick in, error, no pool created, no pool needs to be maintained.

"forced pool" scenario:
PgBouncer starts up and tries to create backend connection for "forced pools". It will do that every 1/3sec. If postgres isn't available.
The operation fails and will get retried 1/3sec later.
Is what you are concerned about the connection-creation timeout being higher than 1/3sec and connection-creation attempts will pile up? Or is pool maintenance single threaded and waiting for a connection will just delay the next run of pool-maintenance? Or is there a problem around clients trying to get a backend connection while the maintenance is trying to create such a connection?

While we work through that to see if anything needs to change around timetouts, I'm going ahead with simplifying the code by removing the additional settings and adjusting the docs.

src/server.c

test/test.ini

fschmager · 2023-09-28T15:26:37Z

@JelteF I'm struggling with the tests. The Linux (Debian/Ubuntu) CFLAGS:-O0 -g ENABLE_VALGRIND:yes check keeps failing. What is that check doing differently that might explain the failed tests?

What's the release cadence? I'd love for this to get out soon. Don't want to miss the boat.

JelteF · 2023-09-28T19:44:08Z

Generally it slows tests down more, because it's running with valgrind (and without optimizations).

JelteF · 2023-09-28T19:44:43Z

Release cadance is irregular, but roughly every 2-4 months.

JelteF · 2023-09-28T21:46:16Z

I think it's because with the config used in the min_pool_size test we have many forced user connections. By setting min_pool_size globally, it starts opening connections for all of those. I think it would be best to update the test, and not set min_pool_size globally at all, but only on a specific database. Just like you're doing for your new test. i.e. replace the old min_pool_size test with a min_pool_size_without_forced_user test.

fschmager · 2023-09-29T14:43:23Z

test/test_limits.py

@@ -37,9 +38,32 @@ async def test_pool_size(pg, bouncer):

 @pytest.mark.asyncio
 async def test_min_pool_size(pg, bouncer):
-    bouncer.admin("set min_pool_size = 3")


The test has been changed to not set the global min_pool_size anymore. Caused too many connection creations in a number of pools overloading the github test runner.
Rather this test check various combinations with individual pools.

fschmager · 2023-09-29T14:45:09Z

I hadn't thought of the existing test setting min_pool_size globally. That was the ticket! I removed the global setting of min_pool_size to get the tests to run. We have pretty good test coverage of min_pool_size on a per-pool basis, but no test for global min_pool_size.

JelteF

Looks good to me. I left some final suggestions on comment/docs improvements.

doc/config.md

src/janitor.c

test/test_limits.py

Add min_pool_size_requires_clients setting

0542c2e

fschmager commented Sep 15, 2023

View reviewed changes

fschmager marked this pull request as ready for review September 15, 2023 12:44

fschmager added 2 commits September 15, 2023 13:11

fmt

7d60b3c

simplify, maintain any pool that has min_pool_size and forced user

751a778

fschmager changed the title ~~Add min_pool_size_requires_clients setting~~ Maintain min_pool_size for pools with forced user (even if no client conns) Sep 15, 2023

mcampo-dd approved these changes Sep 15, 2023

View reviewed changes

fschmager added 2 commits September 19, 2023 11:29

trigger GitHub actions

94c1ef3

wait a little longer

fdaf121

JelteF mentioned this pull request Sep 21, 2023

Add 'min_pool_size_requires_clients' setting. #437

Closed

revert longer waits

824c817

replaced global min_pool_size tests with more targeted one

5508ac1

fschmager commented Sep 29, 2023

View reviewed changes

JelteF approved these changes Sep 29, 2023

View reviewed changes

docs/format

122d052

JelteF merged commit dd7a06f into pgbouncer:master Oct 3, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain `min_pool_size` for pools with forced user (even if no client conns) #947

Maintain `min_pool_size` for pools with forced user (even if no client conns) #947

fschmager commented Sep 15, 2023 •

edited

fschmager Sep 15, 2023

JelteF Sep 15, 2023

fschmager Sep 15, 2023

fschmager Sep 15, 2023

JelteF Sep 15, 2023 •

edited

fschmager Sep 15, 2023

fschmager commented Sep 28, 2023 •

edited

JelteF commented Sep 28, 2023

JelteF commented Sep 28, 2023

JelteF commented Sep 28, 2023 •

edited

fschmager Sep 29, 2023

fschmager commented Sep 29, 2023

JelteF left a comment

Maintain min_pool_size for pools with forced user (even if no client conns) #947

Maintain min_pool_size for pools with forced user (even if no client conns) #947

Conversation

fschmager commented Sep 15, 2023 • edited

fschmager Sep 15, 2023

Choose a reason for hiding this comment

JelteF Sep 15, 2023

Choose a reason for hiding this comment

fschmager Sep 15, 2023

Choose a reason for hiding this comment

fschmager Sep 15, 2023

Choose a reason for hiding this comment

JelteF Sep 15, 2023 • edited

Choose a reason for hiding this comment

fschmager Sep 15, 2023

Choose a reason for hiding this comment

fschmager commented Sep 28, 2023 • edited

JelteF commented Sep 28, 2023

JelteF commented Sep 28, 2023

JelteF commented Sep 28, 2023 • edited

fschmager Sep 29, 2023

Choose a reason for hiding this comment

fschmager commented Sep 29, 2023

JelteF left a comment

Choose a reason for hiding this comment

Maintain `min_pool_size` for pools with forced user (even if no client conns) #947

Maintain `min_pool_size` for pools with forced user (even if no client conns) #947

fschmager commented Sep 15, 2023 •

edited

JelteF Sep 15, 2023 •

edited

fschmager commented Sep 28, 2023 •

edited

JelteF commented Sep 28, 2023 •

edited