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.
Hi Andrew!
cc Jelte, I suspect he might be interested.
On 20 Nov 2024, at 20:51, Andrew Jackson <andrewjackson...@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]. 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-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
