Add `load_balance_hosts` parameter to db config #736

dpirotte · 2022-07-05T15:14:30Z

PGBouncer's default behavior is to round-robin between comma-separated
hosts on each new connection attempt. This is desirable when all hosts
are similar, such as load balancing over multiple replicas, but
undesirable when only one host out of the list is expected to
successfully connect and login. In this scenario, every other connection
attempt would hit an invalid host and fail.

The host_strategy parameter controls this host selection behavior. The
default behavior is labeled round_robin and a new strategy called
last_successful is introduced. This new strategy instructs PGBouncer
to prefer the host with most recent successful connections. When that
host fails, new connections will round-robin through the list until a
successful connection occurs.

JelteF · 2022-08-11T07:38:58Z

src/loader.c

 	int pool_size = -1;
 	int min_pool_size = -1;
 	int res_pool_size = -1;
 	int max_db_connections = -1;
 	int dbname_ofs;
 	int pool_mode = POOL_INHERIT;
+	int host_strategy = ROUND_ROBIN;


Suggested change

int host_strategy = ROUND_ROBIN;

enum HostStrategy host_strategy = ROUND_ROBIN;

JelteF

Thank you for the contribution, it looks useful, postgres connection strings work in a similar way so it makes sense to support that as well.

I left some comments. And can you please rebase/merge this PR because there's a merge conflict.

JelteF · 2022-08-11T07:42:44Z

src/loader.c

@@ -236,6 +241,11 @@ bool parse_database(void *base, const char *name, const char *connstr)
 			res_pool_size = atoi(val);
 		} else if (strcmp("max_db_connections", key) == 0) {
 			max_db_connections = atoi(val);
+		} else if (strcmp("host_strategy", key) == 0) {


This needs an entry in config.md to describe its usage.

Documentation is in. Feedback welcome.

JelteF · 2022-08-11T07:48:55Z

src/objects.c

+		if (!server->pool->last_connect_failed && server->pool->db->host_strategy == LAST_SUCCESSFUL)
+			server->pool->rrcounter = server->pool->last_successful_rrcounter;


The current PR seems to have the desired effect based on the tests you wrote, but I have the feeling the same could probably be achieved without the two new fields you added: server->rrcounter and pool->last_successful_rrcounter.

How about we simply increase the pool->rr_counter whenever a connection attempt fails.

Thanks for the feedback. I simplified the logic as you suggest and it looks better to me.

dpirotte · 2022-08-29T14:07:13Z

@JelteF Touching base. Happy to incorporate any additional feedback on the new implementation.

goksgie · 2022-10-22T14:43:12Z

src/loader.c

@@ -299,6 +309,7 @@ bool parse_database(void *base, const char *name, const char *connstr)
 	db->res_pool_size = res_pool_size;
 	db->pool_mode = pool_mode;
 	db->max_db_connections = max_db_connections;
+	db->host_strategy = host_strategy;


if the host_strategy of a database can change, should we tag it as changed?

That's a good call. Yes, I think we should tag it so behavior changes after a reload. I will make that change. Thanks for the feedback!

host_strategy is now reloadable.

show databases/show pools also now include host_strategy in their output, which makes this testable and is useful on its own.

dpirotte · 2022-11-07T15:14:18Z

@petere @JelteF I rebased this PR and made @goksgie's suggested change above.

Please let me know if anything else needs to happen to get this into a mergeable state.

JelteF · 2022-11-17T17:36:40Z

I think the code in the PR is fine now (apart from the merge conflict that occured due to another merged PR). But after playing with these changes a bit I found one other issue, and I'm wondering how useful this feature is without also addressing this issue:
server_login_retry creates a significant pause anytime a single node cannot be connected to.

@dpirotte How are you using this change, that it is useful in its current form?

JelteF · 2023-06-05T15:43:57Z

Hi @dpirotte, github notified me that you rebased this on a branch of your fork. I think a rebased version of this can be merged pretty easily. Could you still explain how you use this in practice? Because when I played with it, another bug in pgbouncer was causing serious issues for me.

dpirotte · 2023-06-05T17:44:06Z

Hi @JelteF. Yeah, I’m working on rebasing this and another patch on top of 1.19. The code works but I haven’t updated to your new (much improved!) test framework.

The purpose here is to support faster failovers in environments where 1/N hosts in the list will be the primary and (N-1)/N hosts will be replicas and where DNS updates are potentially too slow. RDS/Aurora is one such environment, and I believe some Patroni configurations are as well.

Given the following configurations:

; pg-primary is a CNAME to whichever of pg-0 or pg-1 is the primary
current = host=pg-primary.cluster.local connect_query=‘select disconnect_self() if pg_is_in_recovery()’…
proposed = host=pg-0.cluster.local,pg-1.cluster.local host_strategy=last_successful connect_query=‘select disconnect_self() if pg_is_in_recovery()’…

After a failover, current may take some time to point to the correct new primary, i.e. due to DNS caching. To speed up failovers, proposed takes the full list of potential primaries so pgbouncer can iterate through until it finds the primary. (And, to your earlier comment, this does encourage a low value for server_login_retry.)

A key part here is the connect_query, which asserts that pgbouncer is only ever talking to a writable database. Without host_strategy, using the full list of potential primaries means that (N-1)/N new connections will fail and wait for server_login_retry before trying another host. With host_strategy=last_successful, we only fail connections until a new primary is found, and direct all new connections there until the next failure. This minimizes failed connection noise and time waiting for server_login_retry.

I have another patch that adds lightweight target_session_attrs support albeit only on PG 14+ hosts where the server informs the client of its status as a primary or standby. This replaces the connect_query above, but is not a replacement for host_strategy. One could imagine having three hosts (primary and two replicas) where you want host_strategy=last_successful for the primary db connection string but still want to load balance on the replica connection string with host_strategy=round_robin.

Another option would be to keep the round-robin behavior and make target_session_attrs loop through hosts and gracefully disconnect until it finds one matching the requested state. I prefer having both configuration options so that target_session_attrs=primary host_strategy=last_successful is opening/closing as few connections on replicas as possible.

I’ll bump once the tests are ported to pytest and we’ll go from there.

PGBouncer's default behavior is to round-robin between comma-separated hosts on each new connection attempt. This is desirable when all hosts are similar, such as load balancing over multiple replicas, but undesirable when only one host out of the list is expected to successfully connect and login. In this scenario, every other connection attempt would hit an invalid host and fail. The `host_strategy` parameter controls this host selection behavior. The default behavior is labeled `round_robin` and a new strategy called `last_successful` is introduced. This new strategy instructs PGBouncer to prefer the host with most recent successful connections. When that host fails, new connections will round-robin through the list until a successful connection occurs.

@JelteF

In order to maintain behavior that connections always start from the left-most host in the host list, increment rrcounter either before choosing a host (in the case of last_successful) or after choosing a host (in the case of round_robin). (This incorporates PR feedback from @JelteF.)

Previously, host_strategy could only be set at pgbouncer startup, which is unintuitive and inconsistent with other per-database configuration. Also, add host_strategy to both `show databases` and `show pools` output for visibility into the current setting, and to facilitate testing that the configuration properly reloads.

dpirotte · 2023-06-07T21:33:20Z

@JelteF Rebased host_strategy with pytests if you want to take a look while I sort out the macOS build.

Previously, the host_strategy tests used 127.0.0.2,127.0.0.1 had a "bad IP" (127.0.0.2) as the first entry to force the first backend server connection to fail and verify that `host_strategy=last_successful` directs new connections to the second "good" entry. MacOS doesn't treat 127.0.0.2 in the same manner as Linux by default, so we need a different way to fail the first host in the list. Switching to an unresolvable DNS hostname works fine on both Linux and MacOS.

JelteF · 2023-06-08T12:35:23Z

doc/config.md

@@ -1060,6 +1060,20 @@ they are logged but ignored otherwise.
 Set the pool mode specific to this database. If not set,
 the default `pool_mode` is used.

+### host_strategy


I think it would make sense to change the name of the option to load_balance_hosts. This option got added to libpq for PG16 (by me). It seems nice to use consistently named options across projects. The last_successful option name should then be changed to disable. And lets change round_robin to round-robin, to be consistent with other arguments like verify-full and read-write.

PS. I especially think consistency is nice here, because you mentioned you also want to add target_session_attrs to PgBouncer. Since target_session_attrs and load_balance_hosts can be used nicely together, I think it's especially useful to have the same names for both Postgres and PgBouncer.

Btw, adding a random as an option would be a nice future improvement to make sure PgBouncer supports the same as libpq. But that should definitely be another follow up PR. We can merge this PR easily without the random functionality.

I think it would make sense to change the name of the option to load_balance_hosts. This option got added to libpq for PG16 (by me).

Ha, good call. I noticed that you contributed this feature in the release notes. +100 to consistent naming with libpq. I'll take care of that.

I prefer the initial naming. It seems clear. load_balance_hosts seems to be a misnomer, since no "load" and no "balancing" is involved?

I prefer the initial naming. It seems clear.

I think the initial name was a fine name too. But I think being in line with naming of libpq, jdbc and npgsql is a big advantage of the load_balance_hosts naming.

load_balance_hosts seems to be a misnomer, since no "load" and no "balancing" is involved?

The "load" is the number of connection that are made by pgbouncer to the database, and those connections are "balanced" across the different servers in a round-robin fashion (or random fashion when implemented in the future). And you can enable this balancing by using disable which will select a working host and send all traffic there.

IMHO, the old naming describes how pgbouncer chooses hosts. The new naming describes what the user will see happening to their setup by changing the setting. I think I personally like names that describe the effects instead of the method.

I incorrectly ported these tests from bash to pytest and so they were not actually verifying that subsequent connections were reusing the last successful host in the list. The tests now verify behavior correctly.

This feature does almost the same thing as the upcoming libpq param called `load_balance_hosts`, so use that name for consistency and rename the configuration options accordingly (`last_successful` => `disable`)

dpirotte · 2023-06-08T21:40:29Z

@JelteF Updated naming per discussion above.

The macOS build passes now. The mingw{32,64} builds failed a couple times but seem to pass now, so those might have been random failures. The most recent build failures look like Cirrus issues: Failed to start an instance: INTERNAL: API response null(0): An error has occurred. Failed to connect to /34.122.100.68:443.

JelteF · 2023-06-08T22:05:49Z

test/test.ini

@@ -30,6 +30,9 @@ authdb = port=6666 host=127.0.0.1 dbname=p1 auth_user=pswcheck

 hostlist1 = port=6666 host=127.0.0.1,::1 dbname=p0 user=bouncer
 hostlist2 = port=6666 host=127.0.0.1,127.0.0.1 dbname=p0 user=bouncer
+hostlist_good_first = port=6666 host=127.0.0.1,127.0.0.3 dbname=p0 user=bouncer load_balance_hosts=disable
+hostlist_bad_first = port=6666 host=unresolvable-hostname,127.0.0.1 dbname=p0 user=bouncer load_balance_hosts=disable
+load_balance_hosts_update = port=6666 host=127.0.0.1,127.0.0.3 dbname=p0 user=bouncer load_balance_hosts=disable


The load_balance_hosts_update database isn't used (afaict). Did you have a test in mind for it?

Lost in translation from bash tests to pytest

JelteF · 2023-06-14T14:23:01Z

doc/config.md

+When a comma-separated list is specified in `host`, `load_balance_hosts` controls
+which entry is chosen for a new connection.


It would be great if this same logic would be used when receiving multiple IP adresses from DNS. The easiest way to test this is to create lines in /etc/host. Libpq its load_balance_hosts version does this too. So if we don't implement this, we should at least make it explicit in the docs that this setting currently doesn't impact DNS based load balancing (but that this might change in the future).

Yeah. I'm wondering if this feature also includes #448 or at least to have it in mind.

Interesting, I didn't see that PR before (I guess I didn't go more than 2 years into PR history when joining the project). Right now #448 is completely distinct from this PR:

This PR does not implement the disable mode for DNS load balancing. It still uses round robin even if disable is used.

This PR does not implement the random mode. PR add server_shuffle_hosts #448 implements random mode for DNS load balancing only, not for host lists (certainly because host lists were not available in PgBouncer in the time that PR was written).

Th design of the config option would allow for implementing both 1 and 2 though. I think 2 isn't required to implement for this PR. A completely missing option isn't very confusing. But I think 1 is quite confusing because the existing option does not apply to all the places that you would expect, so preferably this PR would implement 1 too. If that's hard for some reason, then I think a note in the docs is also sufficient.

JelteF · 2023-06-14T14:24:19Z

doc/config.md

+
+disable
+:   A new connection continues using the same host entry until a connection
+    fails, after which the next host entry is chosen.


It would be good to mention that it is recommended to set server_login_retry lower than the default to ensure fast retries, because of issue #872

JelteF reviewed Aug 11, 2022

View reviewed changes

dpirotte force-pushed the host-strategy branch from 6087208 to 5c93a29 Compare August 17, 2022 04:09

goksgie reviewed Oct 22, 2022

View reviewed changes

dpirotte force-pushed the host-strategy branch from b35c9a6 to 5d3104f Compare November 7, 2022 01:48

dpirotte and others added 5 commits June 7, 2023 14:57

Document new host_strategy db option

748b13e

Rewrite host_strategy tests in pytest

e0007ab

dpirotte force-pushed the host-strategy branch from 97a45d5 to e0007ab Compare June 7, 2023 16:58

Appease flake8 linter by offering a newline

8ee8abb

JelteF reviewed Jun 8, 2023

View reviewed changes

dpirotte added 2 commits June 8, 2023 15:03

Fix host_strategy tests

46f6d0b

I incorrectly ported these tests from bash to pytest and so they were not actually verifying that subsequent connections were reusing the last successful host in the list. The tests now verify behavior correctly.

Rename host_strategy => load_balance_hosts

5f5463e

This feature does almost the same thing as the upcoming libpq param called `load_balance_hosts`, so use that name for consistency and rename the configuration options accordingly (`last_successful` => `disable`)

dpirotte changed the title ~~Add host_strategy parameter to db config~~ Add load_balance_hosts parameter to db config Jun 8, 2023

JelteF reviewed Jun 8, 2023

View reviewed changes

Re-add test to verify load_balance_hosts is reloadable

657a815

Lost in translation from bash tests to pytest

JelteF reviewed Jun 14, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `load_balance_hosts` parameter to db config #736

Add `load_balance_hosts` parameter to db config #736

dpirotte commented Jul 5, 2022

JelteF Aug 11, 2022

JelteF left a comment •

edited

JelteF Aug 11, 2022

dpirotte Aug 17, 2022

JelteF Aug 11, 2022 •

edited

dpirotte Aug 17, 2022

dpirotte commented Aug 29, 2022

goksgie Oct 22, 2022

dpirotte Oct 31, 2022

dpirotte Nov 7, 2022

dpirotte commented Nov 7, 2022

JelteF commented Nov 17, 2022

JelteF commented Jun 5, 2023

dpirotte commented Jun 5, 2023

dpirotte commented Jun 7, 2023

JelteF Jun 8, 2023

JelteF Jun 8, 2023

dpirotte Jun 8, 2023

petere Jul 10, 2023

JelteF Jul 11, 2023

dpirotte commented Jun 8, 2023

JelteF Jun 8, 2023

JelteF Jun 14, 2023 •

edited

eulerto Jun 16, 2023

JelteF Jun 16, 2023

JelteF Jun 14, 2023

	int host_strategy = ROUND_ROBIN;
	enum HostStrategy host_strategy = ROUND_ROBIN;

		if (!server->pool->last_connect_failed && server->pool->db->host_strategy == LAST_SUCCESSFUL)
		server->pool->rrcounter = server->pool->last_successful_rrcounter;

		When a comma-separated list is specified in `host`, `load_balance_hosts` controls
		which entry is chosen for a new connection.

Add load_balance_hosts parameter to db config #736

Are you sure you want to change the base?

Add load_balance_hosts parameter to db config #736

Conversation

dpirotte commented Jul 5, 2022

Choose a reason for hiding this comment

JelteF left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JelteF Aug 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpirotte commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpirotte commented Nov 7, 2022

JelteF commented Nov 17, 2022

JelteF commented Jun 5, 2023

dpirotte commented Jun 5, 2023

dpirotte commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpirotte commented Jun 8, 2023

Choose a reason for hiding this comment

JelteF Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `load_balance_hosts` parameter to db config #736

Add `load_balance_hosts` parameter to db config #736

JelteF left a comment •

edited

JelteF Aug 11, 2022 •

edited

JelteF Jun 14, 2023 •

edited