public inbox for [email protected]
help / color / mirror / Atom feedAdd Option To Check All Addresses For Matching target_session_attr
12+ messages / 4 participants
[nested] [flat]
* Add Option To Check All Addresses For Matching target_session_attr
@ 2024-11-20 15:51 Andrew Jackson <[email protected]>
0 siblings, 2 replies; 12+ messages in thread
From: Andrew Jackson @ 2024-11-20 15:51 UTC (permalink / raw)
To: pgsql-hackers
Hi,
I was attempting to set up a high availability system using DNS and
target_session_attrs. I was using a DNS setup similar to below and was
trying to use the connection strings `psql postgresql://
[email protected]/db_name?target_session=read-write` to have clients
dynamically connect to the primary or `psql postgresql://
[email protected]/db_name?target_session=read-only` to have clients
connect to a read replica.
The problem that I found with this setup is that if libpq is unable to get
a matching target_session_attr on the first connection attempt it does not
consider any further addresses for the given host. This patch is designed
to provide an option that allows libpq to look at additional addresses for
a given host if the target_session_attr check fails for previous addresses.
Would appreciate any feedback on the applicability/relevancy of the goal
here or the implementation.
Example DNS setup
________________________________
Name | Type | Record
______________|______|___________
pg.database.com | A | ip_address_1
pg.database.com | A | ip_address_2
pg.database.com | A | ip_address_3
pg.database.com | A | ip_address_4
Attachments:
[text/x-patch] postgres.patch (2.5K, 3-postgres.patch)
download | inline diff:
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 51083dcfd8..865eb74fd7 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -365,6 +365,11 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Load-Balance-Hosts", "", 8, /* sizeof("disable") = 8 */
offsetof(struct pg_conn, load_balance_hosts)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
+
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
NULL, NULL, 0}
@@ -4030,11 +4035,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4085,11 +4090,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4703,6 +4708,7 @@ freePGconn(PGconn *conn)
free(conn->rowBuf);
free(conn->target_session_attrs);
free(conn->load_balance_hosts);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 08cc391cbd..c51ec28234 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -427,6 +427,7 @@ struct pg_conn
char *target_session_attrs; /* desired session properties */
char *require_auth; /* name of the expected auth method */
char *load_balance_hosts; /* load balance over hosts */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-02-16 12:03 Andrey Borodin <[email protected]>
parent: Andrew Jackson <[email protected]>
1 sibling, 2 replies; 12+ messages in thread
From: Andrey Borodin @ 2025-02-16 12:03 UTC (permalink / raw)
To: Andrew Jackson <[email protected]>; Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; +Cc: pgsql-hackers
Hi Andrew!
cc Jelte, I suspect he might be interested.
> On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]> wrote:
>
> Would appreciate any feedback on the applicability/relevancy of the goal here or the implementation.
Thank you for raising the issue. Following our discussion in Discord I'm putting my thoughts to list.
Context
A DNS record might return several IPs. Consider we have a connection string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to 3.3.3.3,4.4.4.4.
If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and 3.3.3.3 responded).
If we enable libpq load balancing some random 2 IPs will be probed.
IMO it's a bug, at least when load balancing is enabled. Let's consider if we can change default behavior here. I suspect we can't do it for "load_balance_hosts=disable". And even for load balancing this might be too unexpected change for someone.
Further I only consider proposal not as a bug fix, but as a feature.
In Discord we have surveyed some other drivers.
pgx treats all IPs as different servers [1]. npgsql goes through all IPs one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc Dave and Vladimir, if they would like to provide some input).
Review
The patch needs a rebase. It's trivial, so please fine attached. The patch needs real commit message, it's not trivial :)
We definitely need to adjust tests [0]. We need to change 004_load_balance_dns.pl so that it tests target_session_attrs too.
Some documentation would be nice.
I do not like how this check is performed
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
Let's make it like load balancing is done [4].
Finally, let's think about naming alternatives for "check_all_addrs".
I think that's enough for a first round of the review. If it's not a bug, but a feature - it's a very narrow window to get to 18. But we might be lucky...
Thank you!
Best regards, Andrey Borodin.
[0] https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...
[1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
[2] https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
[3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
[4] https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
Attachments:
[application/octet-stream] v2-0001-Add-option-to-check-all-IPs.patch (2.9K, 2-v2-0001-Add-option-to-check-all-IPs.patch)
download | inline diff:
From e13764ae84555034fedc430e189e056ea5af2d10 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 14 Feb 2025 23:13:55 +0500
Subject: [PATCH v2] Add option to check all IPs
---
src/interfaces/libpq/fe-connect.c | 25 +++++++++++++++----------
src/interfaces/libpq/libpq-int.h | 1 +
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85d1ca2864..11c9e0cf4c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -372,6 +372,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -4326,11 +4330,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4381,11 +4385,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5002,6 +5006,7 @@ freePGconn(PGconn *conn)
free(conn->load_balance_hosts);
free(conn->scram_client_key);
free(conn->scram_server_key);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2546f9f8a5..a96d8ce482 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -432,6 +432,7 @@ struct pg_conn
char *load_balance_hosts; /* load balance over hosts */
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-02-24 15:38 Andrew Jackson <[email protected]>
parent: Andrey Borodin <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Jackson @ 2025-02-24 15:38 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers
Hi,
Thank you for the review!
Review Response
- Made a first pass at a real commit message
- Fixed the condition on the if statement to use strcmp
- Added a test suite in the files `src/interfaces/libpq/t/
006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/
007_load_balance_dns_check_all_addrs.pl` which checks the
target_session_attrs as when used with and without load balancing.
Regarding the name of the variable itself I am definitely open to opinions
on this. I didn't put too much thought initially and just chose
`check_all_addrs`. I feel like given that it modifies the behaviour of
`target_session_attrs` ideally it should reference that in the name but
that would make that variable name very long: something akin to
`target_session_attrs_check_all_addrs`.
Context
I tested some drivers as well and found that pgx, psycopg, and
rust-postgres all traverse every IP address when looking for a matching
target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq
and terminate additional attempts after the first failure. Given this it
seems like there is a decent amount of fragmentation in the ecosystem as to
how exactly to implement this feature. I believe some drivers choose to
traverse all addresses because they have users target the same use case
outlined above.
Thanks again,
Andrew Jackson
On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin <[email protected]> wrote:
> Hi Andrew!
>
> cc Jelte, I suspect he might be interested.
>
> > On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]>
> wrote:
> >
> > Would appreciate any feedback on the applicability/relevancy of the goal
> here or the implementation.
>
> Thank you for raising the issue. Following our discussion in Discord I'm
> putting my thoughts to list.
>
>
> Context
>
> A DNS record might return several IPs. Consider we have a connection
> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to
> 3.3.3.3,4.4.4.4.
> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and
> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and
> 3.3.3.3 responded).
>
> If we enable libpq load balancing some random 2 IPs will be probed.
>
> IMO it's a bug, at least when load balancing is enabled. Let's consider if
> we can change default behavior here. I suspect we can't do it for
> "load_balance_hosts=disable". And even for load balancing this might be too
> unexpected change for someone.
>
> Further I only consider proposal not as a bug fix, but as a feature.
>
> In Discord we have surveyed some other drivers.
> pgx treats all IPs as different servers [1]. npgsql goes through all IPs
> one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc
> Dave and Vladimir, if they would like to provide some input).
>
>
> Review
>
> The patch needs a rebase. It's trivial, so please fine attached. The patch
> needs real commit message, it's not trivial :)
>
> We definitely need to adjust tests [0]. We need to change
> 004_load_balance_dns.pl so that it tests target_session_attrs too.
>
> Some documentation would be nice.
>
> I do not like how this check is performed
> + if (conn->check_all_addrs
> && conn->check_all_addrs[0] == '1')
> Let's make it like load balancing is done [4].
>
> Finally, let's think about naming alternatives for "check_all_addrs".
>
> I think that's enough for a first round of the review. If it's not a bug,
> but a feature - it's a very narrow window to get to 18. But we might be
> lucky...
>
> Thank you!
>
>
> Best regards, Andrey Borodin.
>
> [0]
> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...
> [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
> [2]
> https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
> [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
> [4]
> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
>
Attachments:
[text/x-patch] v3-0001-Add-option-to-check-all-addrs-for-target_session.patch (15.1K, 3-v3-0001-Add-option-to-check-all-addrs-for-target_session.patch)
download | inline diff:
From 358605cff594ba08b14edbcf497cab59834f8110 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 14 Feb 2025 23:13:55 +0500
Subject: [PATCH v3] Add option to check all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server if the servers
session_attr does not match the request target_session_attrs
no futher address is considered. This behaviour is extremely
inconvenient in environments where the user is attempting to
implement a high availability setup without having to modify
DNS records or a proxy server config.
This PR adds a client side option called check_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 25 ++--
src/interfaces/libpq/libpq-int.h | 1 +
.../libpq/t/006_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/007_load_balance_dns_check_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 306 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/006_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index c49e975b082..3eb993bda0a 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2373,6 +2373,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</para>
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-check-all-addrs" xreflabel="check_all_addrs">
+ <term><literal>check_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are checked when trying to make a connection
+ when attempting to find a connection with a matching <xref linkend="libpq-connect-target-session-attr"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attr"/> do not check
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attr"/> proceed
+ to check any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85d1ca2864f..42055a078e3 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -372,6 +372,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -4326,11 +4330,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4381,11 +4385,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5002,6 +5006,7 @@ freePGconn(PGconn *conn)
free(conn->load_balance_hosts);
free(conn->scram_client_key);
free(conn->scram_server_key);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2546f9f8a50..a96d8ce4825 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -432,6 +432,7 @@ struct pg_conn
char *load_balance_hosts; /* load balance over hosts */
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
diff --git a/src/interfaces/libpq/t/006_target_session_attr_dns.pl b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
new file mode 100644
index 00000000000..914f6c472f4
--- /dev/null
+++ b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
new file mode 100644
index 00000000000..d3405598e67
--- /dev/null
+++ b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expeect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} check_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received at least one connection on node1");
+ }
+ if ($standby_expeect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_standby_occurrences == 0, "received at least one connection on node1");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary2_occurrences == 0, "received at least one connection on node1");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.47.2
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-02-24 16:07 Andrew Jackson <[email protected]>
parent: Andrew Jackson <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Jackson @ 2025-02-24 16:07 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers
Looks like this needed another rebase to account for the oauth commit.
Rebase attached.
On Mon, Feb 24, 2025 at 9:38 AM Andrew Jackson <[email protected]>
wrote:
> Hi,
>
> Thank you for the review!
>
> Review Response
>
> - Made a first pass at a real commit message
> - Fixed the condition on the if statement to use strcmp
> - Added a test suite in the files `src/interfaces/libpq/t/
> 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/
> 007_load_balance_dns_check_all_addrs.pl` which checks the
> target_session_attrs as when used with and without load balancing.
>
> Regarding the name of the variable itself I am definitely open to opinions
> on this. I didn't put too much thought initially and just chose
> `check_all_addrs`. I feel like given that it modifies the behaviour of
> `target_session_attrs` ideally it should reference that in the name but
> that would make that variable name very long: something akin to
> `target_session_attrs_check_all_addrs`.
>
> Context
>
> I tested some drivers as well and found that pgx, psycopg, and
> rust-postgres all traverse every IP address when looking for a matching
> target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq
> and terminate additional attempts after the first failure. Given this it
> seems like there is a decent amount of fragmentation in the ecosystem as to
> how exactly to implement this feature. I believe some drivers choose to
> traverse all addresses because they have users target the same use case
> outlined above.
>
> Thanks again,
> Andrew Jackson
>
> On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin <[email protected]>
> wrote:
>
>> Hi Andrew!
>>
>> cc Jelte, I suspect he might be interested.
>>
>> > On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]>
>> wrote:
>> >
>> > Would appreciate any feedback on the applicability/relevancy of the
>> goal here or the implementation.
>>
>> Thank you for raising the issue. Following our discussion in Discord I'm
>> putting my thoughts to list.
>>
>>
>> Context
>>
>> A DNS record might return several IPs. Consider we have a connection
>> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to
>> 3.3.3.3,4.4.4.4.
>> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and
>> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and
>> 3.3.3.3 responded).
>>
>> If we enable libpq load balancing some random 2 IPs will be probed.
>>
>> IMO it's a bug, at least when load balancing is enabled. Let's consider
>> if we can change default behavior here. I suspect we can't do it for
>> "load_balance_hosts=disable". And even for load balancing this might be too
>> unexpected change for someone.
>>
>> Further I only consider proposal not as a bug fix, but as a feature.
>>
>> In Discord we have surveyed some other drivers.
>> pgx treats all IPs as different servers [1]. npgsql goes through all IPs
>> one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc
>> Dave and Vladimir, if they would like to provide some input).
>>
>>
>> Review
>>
>> The patch needs a rebase. It's trivial, so please fine attached. The
>> patch needs real commit message, it's not trivial :)
>>
>> We definitely need to adjust tests [0]. We need to change
>> 004_load_balance_dns.pl so that it tests target_session_attrs too.
>>
>> Some documentation would be nice.
>>
>> I do not like how this check is performed
>> + if (conn->check_all_addrs
>> && conn->check_all_addrs[0] == '1')
>> Let's make it like load balancing is done [4].
>>
>> Finally, let's think about naming alternatives for "check_all_addrs".
>>
>> I think that's enough for a first round of the review. If it's not a bug,
>> but a feature - it's a very narrow window to get to 18. But we might be
>> lucky...
>>
>> Thank you!
>>
>>
>> Best regards, Andrey Borodin.
>>
>> [0]
>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...
>> [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
>> [2]
>> https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
>> [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
>> [4]
>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
>>
>
Attachments:
[text/x-patch] v4-0001-Add-option-to-check-all-addrs-for-target_session.patch (15.1K, 3-v4-0001-Add-option-to-check-all-addrs-for-target_session.patch)
download | inline diff:
From f2a3b1e51824da7b4a3eb096ed2215d64e617abd Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 14 Feb 2025 23:13:55 +0500
Subject: [PATCH v4] Add option to check all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server if the servers
session_attr does not match the request target_session_attrs
no futher address is considered. This behaviour is extremely
inconvenient in environments where the user is attempting to
implement a high availability setup without having to modify
DNS records or a proxy server config.
This PR adds a client side option called check_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 25 ++--
src/interfaces/libpq/libpq-int.h | 1 +
.../libpq/t/006_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/007_load_balance_dns_check_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 306 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/006_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ddb3596df83..c5d29d7a0f9 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2483,6 +2483,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-check-all-addrs" xreflabel="check_all_addrs">
+ <term><literal>check_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are checked when trying to make a connection
+ when attempting to find a connection with a matching <xref linkend="libpq-connect-target-session-attr"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attr"/> do not check
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attr"/> proceed
+ to check any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d5051f5e820..bd0265c553e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -373,6 +373,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* OAuth v2 */
{"oauth_issuer", NULL, NULL, NULL,
@@ -4362,11 +4366,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4417,11 +4421,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5047,6 +5051,7 @@ freePGconn(PGconn *conn)
free(conn->oauth_client_id);
free(conn->oauth_client_secret);
free(conn->oauth_scope);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index f36f7f19d58..27f162c1c29 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -432,6 +432,7 @@ struct pg_conn
char *load_balance_hosts; /* load balance over hosts */
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
diff --git a/src/interfaces/libpq/t/006_target_session_attr_dns.pl b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
new file mode 100644
index 00000000000..914f6c472f4
--- /dev/null
+++ b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
new file mode 100644
index 00000000000..d3405598e67
--- /dev/null
+++ b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expeect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} check_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received at least one connection on node1");
+ }
+ if ($standby_expeect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_standby_occurrences == 0, "received at least one connection on node1");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary2_occurrences == 0, "received at least one connection on node1");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.47.2
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-02-24 18:06 Andrew Jackson <[email protected]>
parent: Andrew Jackson <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Jackson @ 2025-02-24 18:06 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers
The previous patch had a mistake in a reference in the documentation. This
should fix it.
On Mon, Feb 24, 2025 at 10:07 AM Andrew Jackson <[email protected]>
wrote:
> Looks like this needed another rebase to account for the oauth commit.
> Rebase attached.
>
> On Mon, Feb 24, 2025 at 9:38 AM Andrew Jackson <[email protected]>
> wrote:
>
>> Hi,
>>
>> Thank you for the review!
>>
>> Review Response
>>
>> - Made a first pass at a real commit message
>> - Fixed the condition on the if statement to use strcmp
>> - Added a test suite in the files `src/interfaces/libpq/t/
>> 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/
>> 007_load_balance_dns_check_all_addrs.pl` which checks the
>> target_session_attrs as when used with and without load balancing.
>>
>> Regarding the name of the variable itself I am definitely open to
>> opinions on this. I didn't put too much thought initially and just chose
>> `check_all_addrs`. I feel like given that it modifies the behaviour of
>> `target_session_attrs` ideally it should reference that in the name but
>> that would make that variable name very long: something akin to
>> `target_session_attrs_check_all_addrs`.
>>
>> Context
>>
>> I tested some drivers as well and found that pgx, psycopg, and
>> rust-postgres all traverse every IP address when looking for a matching
>> target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq
>> and terminate additional attempts after the first failure. Given this it
>> seems like there is a decent amount of fragmentation in the ecosystem as to
>> how exactly to implement this feature. I believe some drivers choose to
>> traverse all addresses because they have users target the same use case
>> outlined above.
>>
>> Thanks again,
>> Andrew Jackson
>>
>> On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin <[email protected]>
>> wrote:
>>
>>> Hi Andrew!
>>>
>>> cc Jelte, I suspect he might be interested.
>>>
>>> > On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]>
>>> wrote:
>>> >
>>> > Would appreciate any feedback on the applicability/relevancy of the
>>> goal here or the implementation.
>>>
>>> Thank you for raising the issue. Following our discussion in Discord I'm
>>> putting my thoughts to list.
>>>
>>>
>>> Context
>>>
>>> A DNS record might return several IPs. Consider we have a connection
>>> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to
>>> 3.3.3.3,4.4.4.4.
>>> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and
>>> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and
>>> 3.3.3.3 responded).
>>>
>>> If we enable libpq load balancing some random 2 IPs will be probed.
>>>
>>> IMO it's a bug, at least when load balancing is enabled. Let's consider
>>> if we can change default behavior here. I suspect we can't do it for
>>> "load_balance_hosts=disable". And even for load balancing this might be too
>>> unexpected change for someone.
>>>
>>> Further I only consider proposal not as a bug fix, but as a feature.
>>>
>>> In Discord we have surveyed some other drivers.
>>> pgx treats all IPs as different servers [1]. npgsql goes through all IPs
>>> one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc
>>> Dave and Vladimir, if they would like to provide some input).
>>>
>>>
>>> Review
>>>
>>> The patch needs a rebase. It's trivial, so please fine attached. The
>>> patch needs real commit message, it's not trivial :)
>>>
>>> We definitely need to adjust tests [0]. We need to change
>>> 004_load_balance_dns.pl so that it tests target_session_attrs too.
>>>
>>> Some documentation would be nice.
>>>
>>> I do not like how this check is performed
>>> + if
>>> (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
>>> Let's make it like load balancing is done [4].
>>>
>>> Finally, let's think about naming alternatives for "check_all_addrs".
>>>
>>> I think that's enough for a first round of the review. If it's not a
>>> bug, but a feature - it's a very narrow window to get to 18. But we might
>>> be lucky...
>>>
>>> Thank you!
>>>
>>>
>>> Best regards, Andrey Borodin.
>>>
>>> [0]
>>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...
>>> [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
>>> [2]
>>> https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
>>> [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
>>> [4]
>>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
>>>
>>
Attachments:
[text/x-patch] v5-0001-Add-option-to-check-all-addrs-for-target_session.patch (15.1K, 3-v5-0001-Add-option-to-check-all-addrs-for-target_session.patch)
download | inline diff:
From 521a11a0de319bf5657f735c09484a5f9aff3230 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 14 Feb 2025 23:13:55 +0500
Subject: [PATCH v5] Add option to check all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server if the servers
session_attr does not match the request target_session_attrs
no futher address is considered. This behaviour is extremely
inconvenient in environments where the user is attempting to
implement a high availability setup without having to modify
DNS records or a proxy server config.
This PR adds a client side option called check_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 25 ++--
src/interfaces/libpq/libpq-int.h | 1 +
.../libpq/t/006_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/007_load_balance_dns_check_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 306 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/006_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index ddb3596df83..4bbffa504e0 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2483,6 +2483,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-check-all-addrs" xreflabel="check_all_addrs">
+ <term><literal>check_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are checked when trying to make a connection
+ when attempting to find a connection with a matching <xref linkend="libpq-connect-target-session-attrs"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> do not check
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> proceed
+ to check any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d5051f5e820..bd0265c553e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -373,6 +373,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* OAuth v2 */
{"oauth_issuer", NULL, NULL, NULL,
@@ -4362,11 +4366,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4417,11 +4421,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5047,6 +5051,7 @@ freePGconn(PGconn *conn)
free(conn->oauth_client_id);
free(conn->oauth_client_secret);
free(conn->oauth_scope);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index f36f7f19d58..27f162c1c29 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -432,6 +432,7 @@ struct pg_conn
char *load_balance_hosts; /* load balance over hosts */
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
diff --git a/src/interfaces/libpq/t/006_target_session_attr_dns.pl b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
new file mode 100644
index 00000000000..914f6c472f4
--- /dev/null
+++ b/src/interfaces/libpq/t/006_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
new file mode 100644
index 00000000000..d3405598e67
--- /dev/null
+++ b/src/interfaces/libpq/t/007_load_balance_dns_check_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expeect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} check_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received at least one connection on node1");
+ }
+ if ($standby_expeect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_standby_occurrences == 0, "received at least one connection on node1");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary2_occurrences == 0, "received at least one connection on node1");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.47.2
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-05-17 13:41 Andrew Jackson <[email protected]>
parent: Andrew Jackson <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Jackson @ 2025-05-17 13:41 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers
The attached updated patch fixes the merge conflicts and updates the
numbering of the test files.
On Mon, Feb 24, 2025 at 12:06 PM Andrew Jackson <[email protected]>
wrote:
> The previous patch had a mistake in a reference in the documentation. This
> should fix it.
>
> On Mon, Feb 24, 2025 at 10:07 AM Andrew Jackson <
> [email protected]> wrote:
>
>> Looks like this needed another rebase to account for the oauth commit.
>> Rebase attached.
>>
>> On Mon, Feb 24, 2025 at 9:38 AM Andrew Jackson <
>> [email protected]> wrote:
>>
>>> Hi,
>>>
>>> Thank you for the review!
>>>
>>> Review Response
>>>
>>> - Made a first pass at a real commit message
>>> - Fixed the condition on the if statement to use strcmp
>>> - Added a test suite in the files `src/interfaces/libpq/t/
>>> 006_target_session_attr_dns.pl` and `src/interfaces/libpq/t/
>>> 007_load_balance_dns_check_all_addrs.pl` which checks the
>>> target_session_attrs as when used with and without load balancing.
>>>
>>> Regarding the name of the variable itself I am definitely open to
>>> opinions on this. I didn't put too much thought initially and just chose
>>> `check_all_addrs`. I feel like given that it modifies the behaviour of
>>> `target_session_attrs` ideally it should reference that in the name but
>>> that would make that variable name very long: something akin to
>>> `target_session_attrs_check_all_addrs`.
>>>
>>> Context
>>>
>>> I tested some drivers as well and found that pgx, psycopg, and
>>> rust-postgres all traverse every IP address when looking for a matching
>>> target_session_attrs. Asyncpg and psycopg2 on the other hand follow libpq
>>> and terminate additional attempts after the first failure. Given this it
>>> seems like there is a decent amount of fragmentation in the ecosystem as to
>>> how exactly to implement this feature. I believe some drivers choose to
>>> traverse all addresses because they have users target the same use case
>>> outlined above.
>>>
>>> Thanks again,
>>> Andrew Jackson
>>>
>>> On Sun, Feb 16, 2025 at 6:03 AM Andrey Borodin <[email protected]>
>>> wrote:
>>>
>>>> Hi Andrew!
>>>>
>>>> cc Jelte, I suspect he might be interested.
>>>>
>>>> > On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]>
>>>> wrote:
>>>> >
>>>> > Would appreciate any feedback on the applicability/relevancy of the
>>>> goal here or the implementation.
>>>>
>>>> Thank you for raising the issue. Following our discussion in Discord
>>>> I'm putting my thoughts to list.
>>>>
>>>>
>>>> Context
>>>>
>>>> A DNS record might return several IPs. Consider we have a connection
>>>> string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to
>>>> 3.3.3.3,4.4.4.4.
>>>> If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and
>>>> 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and
>>>> 3.3.3.3 responded).
>>>>
>>>> If we enable libpq load balancing some random 2 IPs will be probed.
>>>>
>>>> IMO it's a bug, at least when load balancing is enabled. Let's consider
>>>> if we can change default behavior here. I suspect we can't do it for
>>>> "load_balance_hosts=disable". And even for load balancing this might be too
>>>> unexpected change for someone.
>>>>
>>>> Further I only consider proposal not as a bug fix, but as a feature.
>>>>
>>>> In Discord we have surveyed some other drivers.
>>>> pgx treats all IPs as different servers [1]. npgsql goes through all
>>>> IPs one-by-one always [2]. PGJDBC are somewhat in a decision process [3]
>>>> (cc Dave and Vladimir, if they would like to provide some input).
>>>>
>>>>
>>>> Review
>>>>
>>>> The patch needs a rebase. It's trivial, so please fine attached. The
>>>> patch needs real commit message, it's not trivial :)
>>>>
>>>> We definitely need to adjust tests [0]. We need to change
>>>> 004_load_balance_dns.pl so that it tests target_session_attrs too.
>>>>
>>>> Some documentation would be nice.
>>>>
>>>> I do not like how this check is performed
>>>> + if
>>>> (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
>>>> Let's make it like load balancing is done [4].
>>>>
>>>> Finally, let's think about naming alternatives for "check_all_addrs".
>>>>
>>>> I think that's enough for a first round of the review. If it's not a
>>>> bug, but a feature - it's a very narrow window to get to 18. But we might
>>>> be lucky...
>>>>
>>>> Thank you!
>>>>
>>>>
>>>> Best regards, Andrey Borodin.
>>>>
>>>> [0]
>>>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...
>>>> [1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
>>>> [2]
>>>> https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
>>>> [3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
>>>> [4]
>>>> https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
>>>>
>>>
Attachments:
[text/x-patch] v6-0001-Add-option-to-check-all-addrs-for-target_session.patch (15.1K, 3-v6-0001-Add-option-to-check-all-addrs-for-target_session.patch)
download | inline diff:
From bb691d950739e2d6790bcd247c230a577a105aa9 Mon Sep 17 00:00:00 2001
From: CommanderKeynes <[email protected]>
Date: Sat, 17 May 2025 08:29:01 -0500
Subject: [PATCH v6] Add option to check all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server if the servers
session_attr does not match the request target_session_attrs
no futher address is considered. This behaviour is extremely
inconvenient in environments where the user is attempting to
implement a high availability setup without having to modify
DNS records or a proxy server config.
This PR adds a client side option called check_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 25 ++--
src/interfaces/libpq/libpq-int.h | 1 +
.../libpq/t/007_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/008_load_balance_dns_check_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 306 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/007_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 695fe958c3e..6f43a06ec63 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2555,6 +2555,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-check-all-addrs" xreflabel="check_all_addrs">
+ <term><literal>check_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are checked when trying to make a connection
+ when attempting to find a connection with a matching <xref linkend="libpq-connect-target-session-attrs"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> do not check
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> proceed
+ to check any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 430c0fa4442..54ed809ee7c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -389,6 +389,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* OAuth v2 */
{"oauth_issuer", NULL, NULL, NULL,
@@ -4434,11 +4438,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4489,11 +4493,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5119,6 +5123,7 @@ freePGconn(PGconn *conn)
free(conn->oauth_client_id);
free(conn->oauth_client_secret);
free(conn->oauth_scope);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index a6cfd7f5c9d..4508073efad 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -427,6 +427,7 @@ struct pg_conn
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
char *sslkeylogfile; /* where should the client write ssl keylogs */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
diff --git a/src/interfaces/libpq/t/007_target_session_attr_dns.pl b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
new file mode 100644
index 00000000000..914f6c472f4
--- /dev/null
+++ b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl b/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
new file mode 100644
index 00000000000..d3405598e67
--- /dev/null
+++ b/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expeect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} check_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received at least one connection on node1");
+ }
+ if ($standby_expeect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_standby_occurrences == 0, "received at least one connection on node1");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary2_occurrences == 0, "received at least one connection on node1");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.47.2
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-06-24 19:32 Navrotskiy Artem <[email protected]>
parent: Andrey Borodin <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Navrotskiy Artem @ 2025-06-24 19:32 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; Andrew Jackson <[email protected]>; Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; +Cc: pgsql-hackers
<div><div style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Hi Andrey!</div><div style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"> </div><div style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div>I would like to add that from my point of view the current behavior directly contradicts the documentation (<a href="https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-MULTIPLE-HOSTS"; rel="noopener noreferrer" target="_blank">https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-MULTIPLE-HOSTS</a>):</div&... style="box-sizing:border-box;margin:20px 0px 20px -2px;width:1112px"><div style="box-sizing:border-box;width:1112px"><div style="margin:0px"><div style="font-size:14px;margin-left:5px"><div><blockquote><div>When multiple hosts are specified, or when a single host name is translated to multiple addresses, all the hosts and addresses will be tried in order, until one succeeds. If none of the hosts can be reached, the connection fails. If a connection is established successfully, but authentication fails, the remaining hosts in the list are not tried.</div></blockquote></div></div></div></div></div></div><div>In my case, I want to make a single DNS name that resolves to all the hosts in the PostgreSQL cluster. This will allow you to add/remove cluster nodes without changing the service configuration.</div><div> </div><div>Now I have to change the connection string and restart the application service for each operation when adding/deleting a cluster node. Working with the PostgreSQL service and cluster is the responsibility of different teams, and I really want to separate them. The application service and PostgreSQL are the responsibility of different teams, and I really want to be able to work with them independently</div></div></div><div> </div><div>----------------</div><div>Кому: Andrew Jackson ([email protected]), Vladimir Sitnikov ([email protected]), Dave Page ([email protected]);</div><div>Копия: pgsql-hackers ([email protected]);</div><div>Тема: Add Option To Check All Addresses For Matching target_session_attr;</div><div>24.06.2025, 18:25, "Andrey Borodin" <[email protected]>:</div><blockquote><p>Hi Andrew!<br /><br />cc Jelte, I suspect he might be interested.<br /> </p><blockquote> On 20 Nov 2024, at 20:51, Andrew Jackson <<a href="mailto:[email protected]" rel="noopener noreferrer">[email protected]</a>> wrote:<br /> <br /> Would appreciate any feedback on the applicability/relevancy of the goal here or the implementation.</blockquote><p><br />Thank you for raising the issue. Following our discussion in Discord I'm putting my thoughts to list.<br /><br /><br />Context<br /><br />A DNS record might return several IPs. Consider we have a connection string with "host=A,B", A is resolved to 1.1.1.1,2.2.2.2, B to 3.3.3.3,4.4.4.4.<br />If we connect with "target_session_attrs=read-write" IPs 1.1.1.1 and 3.3.3.3 will be probed, but 2.2.2.2 and 4.4.4.4 won't (if 1.1.1.1 and 3.3.3.3 responded).<br /><br />If we enable libpq load balancing some random 2 IPs will be probed.<br /><br />IMO it's a bug, at least when load balancing is enabled. Let's consider if we can change default behavior here. I suspect we can't do it for "load_balance_hosts=disable". And even for load balancing this might be too unexpected change for someone.<br /><br />Further I only consider proposal not as a bug fix, but as a feature.<br /><br />In Discord we have surveyed some other drivers.<br />pgx treats all IPs as different servers [1]. npgsql goes through all IPs one-by-one always [2]. PGJDBC are somewhat in a decision process [3] (cc Dave and Vladimir, if they would like to provide some input).<br /><br /><br />Review<br /><br />The patch needs a rebase. It's trivial, so please fine attached. The patch needs real commit message, it's not trivial :)<br /><br />We definitely need to adjust tests [0]. We need to change 004_load_balance_dns.pl so that it tests target_session_attrs too.<br /><br />Some documentation would be nice.<br /><br />I do not like how this check is performed<br />+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')<br />Let's make it like load balancing is done [4].<br /><br />Finally, let's think about naming alternatives for "check_all_addrs".<br /><br />I think that's enough for a first round of the review. If it's not a bug, but a feature - it's a very narrow window to get to 18. But we might be lucky...<br /><br />Thank you!<br /><br /><br />Best regards, Andrey Borodin.<br /><br />[0] <a href="https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9...; rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a9... />[1] <a href="https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177"; rel="noopener noreferrer">https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177</a><br />[2] <a href="https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...; rel="noopener noreferrer">https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N... />[3] <a href="https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450"; rel="noopener noreferrer">https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450</a><br />[4] <a href="https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...; rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0... style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">-- </div><div style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Best regards,</div><div style="background-color:rgb( 255 , 255 , 255 );color:rgb( 26 , 26 , 26 );font-family:'ys text' , 'arial' , sans-serif;font-size:16px;font-style:normal;font-weight:400;text-align:start;text-decoration-color:initial;text-decoration-style:initial;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Artem Navrotskiy</div></div><div> </div>
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2025-08-15 23:43 Andrew Jackson <[email protected]>
parent: Navrotskiy Artem <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Jackson @ 2025-08-15 23:43 UTC (permalink / raw)
To: Navrotskiy Artem <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers
Hi,
Attached is the rebased patch. The use case I am targeting is exactly
what Artem describes above. Happy to make any necessary changes.
Thanks,
Andrew Jackson
Attachments:
[text/x-patch] v7-0001-Add-option-to-check-all-addrs-for-target_session.patch (15.1K, 2-v7-0001-Add-option-to-check-all-addrs-for-target_session.patch)
download | inline diff:
From b0559224bdb4fc8d72323b25ee065913c0110cfc Mon Sep 17 00:00:00 2001
From: CommanderKeynes <[email protected]>
Date: Sat, 17 May 2025 08:29:01 -0500
Subject: [PATCH] Add option to check all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server if the servers
session_attr does not match the request target_session_attrs
no futher address is considered. This behaviour is extremely
inconvenient in environments where the user is attempting to
implement a high availability setup without having to modify
DNS records or a proxy server config.
This PR adds a client side option called check_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 25 ++--
src/interfaces/libpq/libpq-int.h | 1 +
.../libpq/t/007_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/008_load_balance_dns_check_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 306 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/007_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5bf59a1985..ff3ddddd63 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2568,6 +2568,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-check-all-addrs" xreflabel="check_all_addrs">
+ <term><literal>check_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are checked when trying to make a connection
+ when attempting to find a connection with a matching <xref linkend="libpq-connect-target-session-attrs"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> do not check
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> proceed
+ to check any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a3d12931ff..a007761116 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -393,6 +393,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* OAuth v2 */
{"oauth_issuer", NULL, NULL, NULL,
@@ -4434,11 +4438,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4489,11 +4493,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (strcmp(conn->check_all_addrs, "1") == 0)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5127,6 +5131,7 @@ freePGconn(PGconn *conn)
free(conn->inBuffer);
free(conn->outBuffer);
free(conn->rowBuf);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index a701c25038..999eb3cf71 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -430,6 +430,7 @@ struct pg_conn
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
char *sslkeylogfile; /* where should the client write ssl keylogs */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
diff --git a/src/interfaces/libpq/t/007_target_session_attr_dns.pl b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
new file mode 100644
index 0000000000..914f6c472f
--- /dev/null
+++ b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary check_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write check_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any check_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby check_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only check_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl b/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
new file mode 100644
index 0000000000..d3405598e6
--- /dev/null
+++ b/src/interfaces/libpq/t/008_load_balance_dns_check_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expeect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} check_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received at least one connection on node1");
+ }
+ if ($standby_expeect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_standby_occurrences == 0, "received at least one connection on node1");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node1");
+ }else{
+ ok($node_primary2_occurrences == 0, "received at least one connection on node1");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.49.0
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2026-03-05 16:07 Evgeny Kuzin <[email protected]>
parent: Andrew Jackson <[email protected]>
1 sibling, 1 reply; 12+ messages in thread
From: Evgeny Kuzin @ 2026-03-05 16:07 UTC (permalink / raw)
To: Andrew Jackson <[email protected]>; pgsql-hackers
HI!
I just submitted a patch [1] addressing the same problem from a different angle. I wasn't aware of this earlier work until Andrey pointed me to it.
My patch takes a simpler approach: instead of adding a new check_all_addrs parameter, it just changes try_next_host to try_next_addr in the target_session_attrs mismatch handling (~12 lines changed). The rationale being that Artem's point about documentation seems valid - the docs already say "all the hosts and addresses will be tried in order, until one succeeds." The current skip-to-next-host behavior appears to contradict this.
I'm happy to coordinate - the core question seems to be:
1. Behavioral fix (my approach) - aligns with existing documentation, simpler
2. Opt-in parameter (your approach) - preserves backward compatibility explicitly
What do you think? If the community prefers backward compatibility via an explicit option, I could withdraw my patch in favor of yours. If the consensus is that this is actually a bug fix per the docs, perhaps the simpler change is better.
[1] https://www.postgresql.org/message-id/[email protected]....
Thanks,
Evgeny
________________________________
From: Andrew Jackson <[email protected]>
Sent: Wednesday, November 20, 2024 3:51 PM
To: [email protected] <[email protected]>
Subject: Add Option To Check All Addresses For Matching target_session_attr
Hi,
I was attempting to set up a high availability system using DNS and target_session_attrs. I was using a DNS setup similar to below and was trying to use the connection strings `psql postgresql://[email protected]/db_name?target_session=read-write`<http://[email protected]/db_name?target_session=read-write`> to have clients dynamically connect to the primary or `psql postgresql://[email protected]/db_name?target_session=read-only`<http://[email protected]/db_name?target_session=read-only`> to have clients connect to a read replica.
The problem that I found with this setup is that if libpq is unable to get a matching target_session_attr on the first connection attempt it does not consider any further addresses for the given host. This patch is designed to provide an option that allows libpq to look at additional addresses for a given host if the target_session_attr check fails for previous addresses.
Would appreciate any feedback on the applicability/relevancy of the goal here or the implementation.
Example DNS setup
________________________________
Name | Type | Record
______________|______|___________
pg.database.com<http://pg.database.com; | A | ip_address_1
pg.database.com<http://pg.database.com; | A | ip_address_2
pg.database.com<http://pg.database.com; | A | ip_address_3
pg.database.com<http://pg.database.com; | A | ip_address_4
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2026-03-05 21:15 Andrew Jackson <[email protected]>
parent: Evgeny Kuzin <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Jackson @ 2026-03-05 21:15 UTC (permalink / raw)
To: Evgeny Kuzin <[email protected]>; +Cc: pgsql-hackers
While I prefer your solution, I suspect that it would not be possible/worth
it to change the behavior at this point. It seems likely there is some
niche setup out there that relies on the current logic in some capacity.
I can imagine something along the lines of: someone sets up a hostname that
points to 2 IP records that each use different network infra or something
like that. In the case where you are looking for a primary you would not
want to duplicatively check the same host in this scenario because you
already confirmed that it is not the one you are looking for. With your
patch you would be increasing connection latency, probably not by that much
granted but this is the scenario that encouraged me to implement this as a
new parameter.
All that being said I'm fine with either patch and would love to see one of
them get merged. I suspect there are a lot of extraneous haproxy's that
have been set up in the wild that would be unneeded if one of our our
patches were merged and the changes proliferate to downstream drivers.
One note on your patch though: you may want to incorporate the unit tests
that I built out for this patch in yours.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2026-03-06 06:44 Andrey Borodin <[email protected]>
parent: Andrew Jackson <[email protected]>
0 siblings, 1 reply; 12+ messages in thread
From: Andrey Borodin @ 2026-03-06 06:44 UTC (permalink / raw)
To: Andrew Jackson <[email protected]>; +Cc: Navrotskiy Artem <[email protected]>; Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers; Evgeny Kuzin <[email protected]>
> On 16 Aug 2025, at 04:43, Andrew Jackson <[email protected]> wrote:
>
> Attached is the rebased patch.
I've took a look into the patch again.
The behavior and integration with the connection state machine look correct,
and the tests + docs are in good shape. Some notes:
1. Use a dedicated default "0" for check_all_addrs (not DefaultLoadBalanceHosts,
this one is used for load balancing, need more "0").
2. Guard the two strcmp(conn->check_all_addrs, "1") uses so they are safe when
conn->check_all_addrs is NULL.
3. Fix the test typos in 008 (standby_expeect_traffic and the three “on node1”
messages).
4. Parse check_all_addrs once into a bool (like load_balance_type) and use that
in the connection path for consistency and clarity.
Now about important part: is the name "check_all_addrs" good?
I've asked LLM after explaining it what the feature does. PFA attached output.
Personally, I like "try_all_addrs".
It's a bit unclear to me how randomization (load balancing) on different
addresses should work.
Best regards, Andrey Borodin.
On naming and how to configure the feature:
What the option does
It only matters when target_session_attrs is set. On a target_session_attrs
mismatch (e.g. we wanted primary but got standby), it controls:
0 (default): treat this host as failed and move to the next host.
1: stay on this host and try the next address for the same hostname.
So the name is really answering: on session-attrs mismatch, try next address
vs next host?
Is "check_all_addrs" good?
Pros: Short, easy to type, and the env var PGCHECKALLADDRS is clear.
Cons:
It doesn't mention target_session_attrs, so the link to that feature
isn't obvious.
"Check" is vague (check for what?). The important part is "try all
addresses (for this host) on mismatch."
With only check_all_addrs=1 in the conninfo, someone might not guess
it's about session attrs.
So it's usable but not ideal.
Naming alternatives
try_all_addrs
Shorter; "try" is more accurate than "check". Still doesn't tie to
target_session_attrs.
try_next_addr_on_mismatch
Very clear. Long; "mismatch" is a bit generic.
target_session_attrs_try_all_addrs
Explicitly tied to the feature. Too long for conninfo.
session_attrs_try_all_addrs
Shorter, still linked to session attrs. Slightly ambiguous ("try all"
what?).
try_all_host_addrs
Makes "addresses of current host" clear. Doesn't mention session attrs.
A reasonable compromise is try_all_addrs: same length as now, clearer verb,
and the docs can state it applies when target_session_attrs is used. If you
want the link to session attrs in the name, session_attrs_try_all_addrs is
the next best, at the cost of length.
Other ways to configure it
1) No new option (behavioral change only)
Evgeny's approach: always try next address on session-attrs mismatch. No
naming issue, but it changes default behavior and was argued against for
backward compatibility.
2) Fold into target_session_attrs
E.g. target_session_attrs=read-write,try_all_addrs or read-write+try_all.
Pro: option is clearly scoped to that feature. Con: one parameter does two
things (desired role + connection strategy); parsing and docs get more
complex.
3) Keep a separate option (current design)
A dedicated connection parameter that only has effect when
target_session_attrs is set. Pro: simple parsing, clear in conninfo, easy
to document. Con: the name should make the "only with target_session_attrs"
contract obvious (in name or docs).
4) Tie to load_balance_hosts
E.g. when load_balance_hosts=random (or disable) and target_session_attrs
is set, add a third mode like try_all_addrs. Con: mixes two concepts (load
balancing vs which address to try next on mismatch); semantics get
confusing.
Recommendation
Naming: Prefer try_all_addrs over check_all_addrs (clearer, same length). If
you want the link to session attrs in the name, use session_attrs_try_all_addrs
and accept the length.
Configuration: Keeping a separate connection parameter is the right design; the
main improvement is the name and a one-line note in the docs that it only has
effect when target_session_attrs is set. I wouldn't fold it into
target_session_attrs or into load_balance_hosts.
Made with Cursor.
Attachments:
[text/plain] check_all_addrs_variations.txt (3.1K, 2-check_all_addrs_variations.txt)
download | inline:
On naming and how to configure the feature:
What the option does
It only matters when target_session_attrs is set. On a target_session_attrs
mismatch (e.g. we wanted primary but got standby), it controls:
0 (default): treat this host as failed and move to the next host.
1: stay on this host and try the next address for the same hostname.
So the name is really answering: on session-attrs mismatch, try next address
vs next host?
Is "check_all_addrs" good?
Pros: Short, easy to type, and the env var PGCHECKALLADDRS is clear.
Cons:
It doesn't mention target_session_attrs, so the link to that feature
isn't obvious.
"Check" is vague (check for what?). The important part is "try all
addresses (for this host) on mismatch."
With only check_all_addrs=1 in the conninfo, someone might not guess
it's about session attrs.
So it's usable but not ideal.
Naming alternatives
try_all_addrs
Shorter; "try" is more accurate than "check". Still doesn't tie to
target_session_attrs.
try_next_addr_on_mismatch
Very clear. Long; "mismatch" is a bit generic.
target_session_attrs_try_all_addrs
Explicitly tied to the feature. Too long for conninfo.
session_attrs_try_all_addrs
Shorter, still linked to session attrs. Slightly ambiguous ("try all"
what?).
try_all_host_addrs
Makes "addresses of current host" clear. Doesn't mention session attrs.
A reasonable compromise is try_all_addrs: same length as now, clearer verb,
and the docs can state it applies when target_session_attrs is used. If you
want the link to session attrs in the name, session_attrs_try_all_addrs is
the next best, at the cost of length.
Other ways to configure it
1) No new option (behavioral change only)
Evgeny's approach: always try next address on session-attrs mismatch. No
naming issue, but it changes default behavior and was argued against for
backward compatibility.
2) Fold into target_session_attrs
E.g. target_session_attrs=read-write,try_all_addrs or read-write+try_all.
Pro: option is clearly scoped to that feature. Con: one parameter does two
things (desired role + connection strategy); parsing and docs get more
complex.
3) Keep a separate option (current design)
A dedicated connection parameter that only has effect when
target_session_attrs is set. Pro: simple parsing, clear in conninfo, easy
to document. Con: the name should make the "only with target_session_attrs"
contract obvious (in name or docs).
4) Tie to load_balance_hosts
E.g. when load_balance_hosts=random (or disable) and target_session_attrs
is set, add a third mode like try_all_addrs. Con: mixes two concepts (load
balancing vs which address to try next on mismatch); semantics get
confusing.
Recommendation
Naming: Prefer try_all_addrs over check_all_addrs (clearer, same length). If
you want the link to session attrs in the name, use session_attrs_try_all_addrs
and accept the length.
Configuration: Keeping a separate connection parameter is the right design; the
main improvement is the name and a one-line note in the docs that it only has
effect when target_session_attrs is set. I wouldn't fold it into
target_session_attrs or into load_balance_hosts.
Made with Cursor.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Add Option To Check All Addresses For Matching target_session_attr
@ 2026-03-13 01:29 Andrew Jackson <[email protected]>
parent: Andrey Borodin <[email protected]>
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Jackson @ 2026-03-13 01:29 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Navrotskiy Artem <[email protected]>; Vladimir Sitnikov <[email protected]>; Dave Page <[email protected]>; pgsql-hackers; Evgeny Kuzin <[email protected]>
I updated the patch in accordance with your feedback.
- The variable name has been changed to try_all_addrs, I think this
makes sense as it is shorter and coincides with the variables
try_next_addr and try_next_host even though it doesn't perfectly
convey its relationship to target_session_attrs.
- I fixed the typos and the inaccurate assertion messages in the 008_ test file
- I refactored how the try_all_addrs variable flows in the code to
coincide with how load_balance_type is handled. try_all_attrs contains
the unparsed value while try_all_addrs_type contains the parsed value
of type enum PGTryAddrType. This may be a bit overkill but it
addressed a couple of your bulletpoints above, apologies if I judged
incorrectly here.
- Changed some of the wording in the docs as it was just wrong. Also
changed all references in the docs to reference "try" instead of
"check".
With regards to the correct load balancing behavior I can think of 2 options:
1. randomly choose a host and then randomly choose an address within
that host. If the current addresses session_attr does not match
target_session_attr then move on to next address if any remains in
that host and move onto the next random host if no addresses remain.
2. Resolve all addresses in all hosts and randomly select an address
from that list.
I don't believe that any test cases exist that validate the
functionality of a combination of multiple hosts, multiple address
within each host, and target session attributes. Happy to add test
case coverage over this if it helps get this patch moving.
I was also thinking should try_all_addrs input be 0/1 or a more human
readable disable/enable or both?
On Fri, Mar 6, 2026 at 12:44 AM Andrey Borodin <[email protected]> wrote:
>
>
>
> > On 16 Aug 2025, at 04:43, Andrew Jackson <[email protected]> wrote:
> >
> > Attached is the rebased patch.
>
> I've took a look into the patch again.
>
> The behavior and integration with the connection state machine look correct,
> and the tests + docs are in good shape. Some notes:
> 1. Use a dedicated default "0" for check_all_addrs (not DefaultLoadBalanceHosts,
> this one is used for load balancing, need more "0").
> 2. Guard the two strcmp(conn->check_all_addrs, "1") uses so they are safe when
> conn->check_all_addrs is NULL.
> 3. Fix the test typos in 008 (standby_expeect_traffic and the three “on node1”
> messages).
> 4. Parse check_all_addrs once into a bool (like load_balance_type) and use that
> in the connection path for consistency and clarity.
>
> Now about important part: is the name "check_all_addrs" good?
> I've asked LLM after explaining it what the feature does. PFA attached output.
>
> Personally, I like "try_all_addrs".
>
> It's a bit unclear to me how randomization (load balancing) on different
> addresses should work.
>
>
> Best regards, Andrey Borodin.
>
Attachments:
[text/x-patch] 0003-Add-option-to-try-all-addrs-for-target_session.patch (17.0K, 2-0003-Add-option-to-try-all-addrs-for-target_session.patch)
download | inline diff:
From 33f5d0c7d62abb1ffa8a9bd8fc659f16e58ef91e Mon Sep 17 00:00:00 2001
From: CommanderKeynes <[email protected]>
Date: Sat, 17 May 2025 08:29:01 -0500
Subject: [PATCH] Add option to try all addrs for target_session.
The current behaviour of libpq with regard to searching
for a matching target_session_attrs in a list of addrs is
that after successfully connecting to a server, if the servers
session_attr does not match the request target_session_attrs
no futher address is considered in that host. This behaviour
is extremely inconvenient in environments where the user is
attempting to implement a high availability setup without having
to modify DNS records after a topology change or maintain a
proxy server layer.
This PR adds a client side option called try_all_addrs.
When set to 1 this option will tell libpq to continue checking
any remaining addresses even if there was a target_session_attrs
mismatch on one of them.
Author: Andrew Jackson
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg%40mail.gmail.com
---
doc/src/sgml/libpq.sgml | 33 +++++
src/interfaces/libpq/fe-connect.c | 42 ++++--
src/interfaces/libpq/libpq-int.h | 12 ++
.../libpq/t/007_target_session_attr_dns.pl | 129 ++++++++++++++++++
.../t/008_load_balance_dns_try_all_addrs.pl | 128 +++++++++++++++++
5 files changed, 334 insertions(+), 10 deletions(-)
create mode 100644 src/interfaces/libpq/t/007_target_session_attr_dns.pl
create mode 100644 src/interfaces/libpq/t/008_load_balance_dns_try_all_addrs.pl
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5bf59a19855..2e2f00daca4 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2568,6 +2568,39 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-try-all-addrs" xreflabel="try_all_addrs">
+ <term><literal>try_all_addrs</literal></term>
+ <listitem>
+ <para>
+ Controls whether or not all addresses within a hostname are tried when attempting to
+ make a connection with a matching <xref linkend="libpq-connect-target-session-attrs"/>.
+
+ There are two modes:
+ <variablelist>
+ <varlistentry>
+ <term><literal>0</literal> (default)</term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> do not try
+ any additional addresses and move onto the next host if one was provided.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>1</literal></term>
+ <listitem>
+ <para>
+ If a successful connection is made and that connection is found to have a
+ mismatching <xref linkend="libpq-connect-target-session-attrs"/> proceed
+ to try any additional addresses.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</para>
</sect2>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index a3d12931fff..aaac4565934 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -125,6 +125,7 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options,
#endif
#define DefaultTargetSessionAttrs "any"
#define DefaultLoadBalanceHosts "disable"
+#define DefaultTryAllAddrs "0"
#ifdef USE_SSL
#define DefaultSSLMode "prefer"
#define DefaultSSLCertMode "allow"
@@ -394,6 +395,11 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"try_all_addrs", "PGTRYALLADDRS",
+ DefaultTryAllAddrs, NULL,
+ "Try-All-Addrs", "", 1,
+ offsetof(struct pg_conn, try_all_addrs)},
+
/* OAuth v2 */
{"oauth_issuer", NULL, NULL, NULL,
"OAuth-Issuer", "", 40,
@@ -2018,6 +2024,21 @@ pqConnectOptions2(PGconn *conn)
else
conn->target_server_type = SERVER_TYPE_ANY;
+ if (conn->try_all_addrs){
+ if (strcmp(conn->try_all_addrs, "0") == 0)
+ conn->try_all_addrs_type = TRY_ALL_ADDRS_DISABLE;
+ else if (strcmp(conn->try_all_addrs, "1") == 0)
+ conn->try_all_addrs_type = TRY_ALL_ADDRS_ENABLE;
+ else {
+ conn->status = CONNECTION_BAD;
+ libpq_append_conn_error(conn, "invalid %s value: \"%s\"",
+ "try_all_addrs",
+ conn->try_all_addrs);
+ return false;
+ }
+ } else
+ conn->try_all_addrs_type = TRY_ALL_ADDRS_DISABLE;
+
if (conn->scram_client_key)
{
int len;
@@ -4434,11 +4455,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->try_all_addrs_type == TRY_ALL_ADDRS_ENABLE)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4489,11 +4510,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->try_all_addrs_type == TRY_ALL_ADDRS_ENABLE)
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5127,6 +5148,7 @@ freePGconn(PGconn *conn)
free(conn->inBuffer);
free(conn->outBuffer);
free(conn->rowBuf);
+ free(conn->try_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index a701c25038a..b1406c9dfc8 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -250,6 +250,15 @@ typedef enum
LOAD_BALANCE_RANDOM, /* Randomly shuffle the hosts */
} PGLoadBalanceType;
+/* Try address type (decoded value of try_all_addr) */
+typedef enum
+{
+ TRY_ALL_ADDRS_DISABLE = 0, /* Do not try subsequent addresses in host
+ * after target_session_attrs mismatch (default) */
+ TRY_ALL_ADDRS_ENABLE, /* Try remaining addresses in host even after
+ * target_session_attrs mismatch */
+} PGTryAddrType;
+
/* Boolean value plus a not-known state, for GUCs we might have to fetch */
typedef enum
{
@@ -430,6 +439,7 @@ struct pg_conn
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
char *sslkeylogfile; /* where should the client write ssl keylogs */
+ char *try_all_addrs; /* whether to try all ips within a host */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
@@ -534,6 +544,8 @@ struct pg_conn
PGTargetServerType target_server_type; /* desired session properties */
PGLoadBalanceType load_balance_type; /* desired load balancing
* algorithm */
+ PGTryAddrType try_all_addrs_type; /* parsed representation of try_all_addrs */
+
bool try_next_addr; /* time to advance to next address/host? */
bool try_next_host; /* time to advance to next connhost[]? */
int naddr; /* number of addresses returned by getaddrinfo */
diff --git a/src/interfaces/libpq/t/007_target_session_attr_dns.pl b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
new file mode 100644
index 00000000000..75701bcd30b
--- /dev/null
+++ b/src/interfaces/libpq/t/007_target_session_attr_dns.pl
@@ -0,0 +1,129 @@
+
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+# Cluster setup which is shared for testing both load balancing methods
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+
+my $node_primary1 = PostgreSQL::Test::Cluster->new('primary1', port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start;
+
+# Take backup from which all operations will be run
+$node_primary1->backup('my_backup');
+
+my $node_standby = PostgreSQL::Test::Cluster->new('standby', port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, 'my_backup',
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new('node1', port => $port, own_host => 1);
+$node_primary2 ->init();
+$node_primary2 ->start();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary try_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write try_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any try_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby try_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only try_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+
+$node_primary1->stop();
+
+# target_session_attrs=primary should always choose the first one.
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=primary try_all_addrs=1",
+ "target_session_attrs=primary connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_primary2->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-write try_all_addrs=1",
+ "target_session_attrs=read-write connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=any try_all_addrs=1",
+ "target_session_attrs=any connects to the first node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=standby try_all_addrs=1",
+ "target_session_attrs=standby connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+$node_standby->connect_ok(
+ "host=pg-loadbalancetest port=$port target_session_attrs=read-only try_all_addrs=1",
+ "target_session_attrs=read-only connects to the third node",
+ sql => "SELECT 'connect1'",
+ log_like => [qr/statement: SELECT 'connect1'/]);
+
+$node_primary2->stop();
+$node_standby->stop();
+
+
+done_testing();
diff --git a/src/interfaces/libpq/t/008_load_balance_dns_try_all_addrs.pl b/src/interfaces/libpq/t/008_load_balance_dns_try_all_addrs.pl
new file mode 100644
index 00000000000..9218ab785a7
--- /dev/null
+++ b/src/interfaces/libpq/t/008_load_balance_dns_try_all_addrs.pl
@@ -0,0 +1,128 @@
+# Copyright (c) 2023-2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if (!$ENV{PG_TEST_EXTRA} || $ENV{PG_TEST_EXTRA} !~ /\bload_balance\b/)
+{
+ plan skip_all =>
+ 'Potentially unsafe test load_balance not enabled in PG_TEST_EXTRA';
+}
+
+my $can_bind_to_127_0_0_2 =
+ $Config{osname} eq 'linux' || $PostgreSQL::Test::Utils::windows_os;
+
+# Checks for the requirements for testing load balancing method 2
+if (!$can_bind_to_127_0_0_2)
+{
+ plan skip_all => 'load_balance test only supported on Linux and Windows';
+}
+
+my $hosts_path;
+if ($windows_os)
+{
+ $hosts_path = 'c:\Windows\System32\Drivers\etc\hosts';
+}
+else
+{
+ $hosts_path = '/etc/hosts';
+}
+
+my $hosts_content = PostgreSQL::Test::Utils::slurp_file($hosts_path);
+
+my $hosts_count = () =
+ $hosts_content =~ /127\.0\.0\.[1-3] pg-loadbalancetest/g;
+if ($hosts_count != 3)
+{
+ # Host file is not prepared for this test
+ plan skip_all => "hosts file was not prepared for DNS load balance test";
+}
+
+$PostgreSQL::Test::Cluster::use_tcp = 1;
+$PostgreSQL::Test::Cluster::test_pghost = '127.0.0.1';
+
+my $port = PostgreSQL::Test::Cluster::get_free_port();
+local $Test::Builder::Level = $Test::Builder::Level + 1;
+my $node_primary1 = PostgreSQL::Test::Cluster->new("primary1", port => $port);
+$node_primary1->init(has_archiving => 1, allows_streaming => 1);
+
+# Start it
+$node_primary1->start();
+
+# Take backup from which all operations will be run
+$node_primary1->backup("my_backup");
+
+my $node_standby = PostgreSQL::Test::Cluster->new("standby", port => $port, own_host => 1);
+$node_standby->init_from_backup($node_primary1, "my_backup",
+ has_restoring => 1);
+$node_standby->start();
+
+my $node_primary2 = PostgreSQL::Test::Cluster->new("node1", port => $port, own_host => 1);
+$node_primary2->init();
+$node_primary2->start();
+sub test_target_session_attr {
+ my $target_session_attrs = shift;
+ my $test_num = shift;
+ my $primary1_expect_traffic = shift;
+ my $standby_expect_traffic = shift;
+ my $primary2_expect_traffic = shift;
+ # Statistically the following loop with load_balance_hosts=random will almost
+ # certainly connect at least once to each of the nodes. The chance of that not
+ # happening is so small that it's negligible: (2/3)^50 = 1.56832855e-9
+ foreach my $i (1 .. 50)
+ {
+ $node_primary1->connect_ok(
+ "host=pg-loadbalancetest port=$port load_balance_hosts=random target_session_attrs=${target_session_attrs} try_all_addrs=1",
+ "repeated connections with random load balancing",
+ sql => "SELECT 'connect${test_num}'");
+ }
+ my $node_primary1_occurrences = () =
+ $node_primary1->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_standby_occurrences = () =
+ $node_standby->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+ my $node_primary2_occurrences = () =
+ $node_primary2->log_content() =~ /statement: SELECT 'connect${test_num}'/g;
+
+ my $total_occurrences =
+ $node_primary1_occurrences + $node_standby_occurrences + $node_primary2_occurrences;
+
+ if ($primary1_expect_traffic == 1) {
+ ok($node_primary1_occurrences > 0, "received at least one connection on node primary1");
+ }else{
+ ok($node_primary1_occurrences == 0, "received no connections on node primary1");
+ }
+ if ($standby_expect_traffic == 1) {
+ ok($node_standby_occurrences > 0, "received at least one connection on node standby");
+ }else{
+ ok($node_standby_occurrences == 0, "received no connections on node standby");
+ }
+
+ if ($primary2_expect_traffic == 1) {
+ ok($node_primary2_occurrences > 0, "received at least one connection on node primary2");
+ }else{
+ ok($node_primary2_occurrences == 0, "received no connections on primary2");
+ }
+
+ ok($total_occurrences == 50, "received 50 connections across all nodes");
+}
+
+test_target_session_attr('any',
+ 1, 1, 1, 1);
+test_target_session_attr('read-only',
+ 2, 0, 1, 0);
+test_target_session_attr('read-write',
+ 3, 1, 0, 1);
+test_target_session_attr('primary',
+ 4, 1, 0, 1);
+test_target_session_attr('standby',
+ 5, 0, 1, 0);
+
+
+$node_primary1->stop();
+$node_primary2->stop();
+$node_standby->stop();
+
+done_testing();
--
2.49.0
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2026-03-13 01:29 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-11-20 15:51 Add Option To Check All Addresses For Matching target_session_attr Andrew Jackson <[email protected]>
2025-02-16 12:03 ` Andrey Borodin <[email protected]>
2025-02-24 15:38 ` Andrew Jackson <[email protected]>
2025-02-24 16:07 ` Andrew Jackson <[email protected]>
2025-02-24 18:06 ` Andrew Jackson <[email protected]>
2025-05-17 13:41 ` Andrew Jackson <[email protected]>
2025-06-24 19:32 ` Navrotskiy Artem <[email protected]>
2025-08-15 23:43 ` Andrew Jackson <[email protected]>
2026-03-06 06:44 ` Andrey Borodin <[email protected]>
2026-03-13 01:29 ` Andrew Jackson <[email protected]>
2026-03-05 16:07 ` Evgeny Kuzin <[email protected]>
2026-03-05 21:15 ` Andrew Jackson <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox