Add Option To Check All Addresses For Matching target_session_attr

Hi Andrew!

cc Jelte, I suspect he might be interested.

On 20 Nov 2024, at 20:51, Andrew Jackson <andrewjackson947@gmail.com> 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]https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177. npgsql goes through all IPs one-by-one always [2]https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986. PGJDBC are somewhat in a decision process [3]https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450 (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]https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94. 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]https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94 == '1')
Let's make it like load balancing is done [4]https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660.

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-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94
[1]: https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
[2]: https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986
[3]: https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
[4]: https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660

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 <x4mmm@yandex-team.ru> 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 <andrewjackson947@gmail.com>
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 <andrewjackson947@gmail.com>
wrote:

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 <andrewjackson947@gmail.com>
wrote:

<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&quot; rel="noopener noreferrer" target="_blank">https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-MULTIPLE-HOSTS&lt;/a&gt;):&lt;/div&gt;&lt;div&gt;&lt;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 (andrewjackson947@gmail.com), Vladimir Sitnikov (sitnikov.vladimir@gmail.com), Dave Page (dpage@pgadmin.org);</div><div>Копия: pgsql-hackers (pgsql-hackers@postgresql.org);</div><div>Тема: Add Option To Check All Addresses For Matching target_session_attr;</div><div>24.06.2025, 18:25, "Andrey Borodin" &lt;x4mmm@yandex-team.ru&gt;:</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 &lt;<a href="mailto:andrewjackson947@gmail.com" rel="noopener noreferrer">andrewjackson947@gmail.com</a>&gt; 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-&gt;check_all_addrs &amp;&amp; conn-&gt;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-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94&quot; rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94&lt;/a&gt;&lt;br />[1] <a href="https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177&quot; rel="noopener noreferrer">https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177&lt;/a&gt;&lt;br />[2] <a href="https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986&quot; rel="noopener noreferrer">https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986&lt;/a&gt;&lt;br />[3] <a href="https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450&quot; rel="noopener noreferrer">https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450&lt;/a&gt;&lt;br />[4] <a href="https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660&quot; rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660&lt;/a&gt;&lt;/p&gt;&lt;/blockquote&gt;&lt;div&gt; &lt;/div&gt;&lt;div&gt; &lt;/div&gt;&lt;div&gt;&lt;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">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>

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

HI!
I just submitted a patch [1]/messages/by-id/AM9PR09MB49008B02CDF003054D5D4E00977DA@AM9PR09MB4900.eurprd09.prod.outlook.com Thanks, Evgeny 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]: /messages/by-id/AM9PR09MB49008B02CDF003054D5D4E00977DA@AM9PR09MB4900.eurprd09.prod.outlook.com Thanks, Evgeny
Thanks,
Evgeny

________________________________
From: Andrew Jackson <andrewjackson947@gmail.com>
Sent: Wednesday, November 20, 2024 3:51 PM
To: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
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://user@pg.database.com/db_name?target_session=read-write`<http://user@pg.database.com/db_name?target_session=read-write`&gt; to have clients dynamically connect to the primary or `psql postgresql://user@pg.database.com/db_name?target_session=read-only`<http://user@pg.database.com/db_name?target_session=read-only`&gt; 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&gt; | A | ip_address_1
pg.database.com<http://pg.database.com&gt; | A | ip_address_2
pg.database.com<http://pg.database.com&gt; | A | ip_address_3
pg.database.com<http://pg.database.com&gt; | A | ip_address_4

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.

On 16 Aug 2025, at 04:43, Andrew Jackson <andrewjackson947@gmail.com> 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.