From: Andrey Borodin <x4mmm@yandex-team.ru>
Message-Id: <F1096CFA-CF22-4B47-A10B-D1F837BF7AF1@yandex-team.ru>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_3698EAA5-64F4-4E0C-975A-FD9B4D7AFFD2"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: Add Option To Check All Addresses For Matching
 target_session_attr
Date: Sun, 16 Feb 2025 17:03:44 +0500
In-Reply-To: <CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg@mail.gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
To: Andrew Jackson <andrewjackson947@gmail.com>,
 Vladimir Sitnikov <sitnikov.vladimir@gmail.com>,
 Dave Page <dpage@pgadmin.org>
References: <CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/F1096CFA-CF22-4B47-A10B-D1F837BF7AF1%40yandex-team.ru>
Precedence: bulk


--Apple-Mail=_3698EAA5-64F4-4E0C-975A-FD9B4D7AFFD2
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

Hi Andrew!

cc Jelte, I suspect he might be interested.

> On 20 Nov 2024, at 20:51, Andrew Jackson <andrewjackson947@gmail.com> =
wrote:
>=20
> 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=3DA,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=3Dread-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=3Ddisable". 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] =3D=3D '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/7f5b19817eaf38e70ad1153db4e644=
ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b5121517=
4cf3ffR94
[1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
[2] =
https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf8=
26ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986
[3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
[4] =
https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644=
ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4a=
eb7f68R1660

--Apple-Mail=_3698EAA5-64F4-4E0C-975A-FD9B4D7AFFD2
Content-Disposition: attachment;
	filename=v2-0001-Add-option-to-check-all-IPs.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="v2-0001-Add-option-to-check-all-IPs.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20e13764ae84555034fedc430e189e056ea5af2d10=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Andrey=20Borodin=20<amborodin@acm.org>=0ADate:=20=
Fri,=2014=20Feb=202025=2023:13:55=20+0500=0ASubject:=20[PATCH=20v2]=20=
Add=20option=20to=20check=20all=20IPs=0A=0A---=0A=20=
src/interfaces/libpq/fe-connect.c=20|=2025=20+++++++++++++++----------=0A=
=20src/interfaces/libpq/libpq-int.h=20=20|=20=201=20+=0A=202=20files=20=
changed,=2016=20insertions(+),=2010=20deletions(-)=0A=0Adiff=20--git=20=
a/src/interfaces/libpq/fe-connect.c=20=
b/src/interfaces/libpq/fe-connect.c=0Aindex=2085d1ca2864..11c9e0cf4c=20=
100644=0A---=20a/src/interfaces/libpq/fe-connect.c=0A+++=20=
b/src/interfaces/libpq/fe-connect.c=0A@@=20-372,6=20+372,10=20@@=20=
static=20const=20internalPQconninfoOption=20PQconninfoOptions[]=20=3D=20=
{=0A=20=0A=20=09{"scram_server_key",=20NULL,=20NULL,=20NULL,=20=
"SCRAM-Server-Key",=20"D",=20SCRAM_MAX_KEY_LEN=20*=202,=0A=20=09=
offsetof(struct=20pg_conn,=20scram_server_key)},=0A+=09=
{"check_all_addrs",=20"PGCHECKALLADDRS",=0A+=09=09=
DefaultLoadBalanceHosts,=20NULL,=0A+=09=09"Check-All-Addrs",=20"",=201,=0A=
+=09offsetof(struct=20pg_conn,=20check_all_addrs)},=0A=20=0A=20=09/*=20=
Terminating=20entry=20---=20MUST=20BE=20LAST=20*/=0A=20=09{NULL,=20NULL,=20=
NULL,=20NULL,=0A@@=20-4326,11=20+4330,11=20@@=20keep_going:=09=09=09=09=09=
=09/*=20We=20will=20come=20back=20to=20here=20until=20there=20is=0A=20=09=
=09=09=09=09=09conn->status=20=3D=20CONNECTION_OK;=0A=20=09=09=09=09=09=09=
sendTerminateConn(conn);=0A=20=0A-=09=09=09=09=09=09/*=0A-=09=09=09=09=09=
=09=20*=20Try=20next=20host=20if=20any,=20but=20we=20don't=20want=20to=20=
consider=0A-=09=09=09=09=09=09=20*=20additional=20addresses=20for=20this=20=
host.=0A-=09=09=09=09=09=09=20*/=0A-=09=09=09=09=09=09=
conn->try_next_host=20=3D=20true;=0A+=09=09=09=09=09=09if=20=
(conn->check_all_addrs=20&&=20conn->check_all_addrs[0]=20=3D=3D=20'1')=0A=
+=09=09=09=09=09=09=09conn->try_next_addr=20=3D=20true;=0A+=09=09=09=09=09=
=09else=0A+=09=09=09=09=09=09=09conn->try_next_host=20=3D=20true;=0A+=0A=20=
=09=09=09=09=09=09goto=20keep_going;=0A=20=09=09=09=09=09}=0A=20=09=09=09=
=09}=0A@@=20-4381,11=20+4385,11=20@@=20keep_going:=09=09=09=09=09=09/*=20=
We=20will=20come=20back=20to=20here=20until=20there=20is=0A=20=09=09=09=09=
=09=09conn->status=20=3D=20CONNECTION_OK;=0A=20=09=09=09=09=09=09=
sendTerminateConn(conn);=0A=20=0A-=09=09=09=09=09=09/*=0A-=09=09=09=09=09=
=09=20*=20Try=20next=20host=20if=20any,=20but=20we=20don't=20want=20to=20=
consider=0A-=09=09=09=09=09=09=20*=20additional=20addresses=20for=20this=20=
host.=0A-=09=09=09=09=09=09=20*/=0A-=09=09=09=09=09=09=
conn->try_next_host=20=3D=20true;=0A+=09=09=09=09=09=09if=20=
(conn->check_all_addrs=20&&=20conn->check_all_addrs[0]=20=3D=3D=20'1')=0A=
+=09=09=09=09=09=09=09conn->try_next_addr=20=3D=20true;=0A+=09=09=09=09=09=
=09else=0A+=09=09=09=09=09=09=09conn->try_next_host=20=3D=20true;=0A+=0A=20=
=09=09=09=09=09=09goto=20keep_going;=0A=20=09=09=09=09=09}=0A=20=09=09=09=
=09}=0A@@=20-5002,6=20+5006,7=20@@=20freePGconn(PGconn=20*conn)=0A=20=09=
free(conn->load_balance_hosts);=0A=20=09free(conn->scram_client_key);=0A=20=
=09free(conn->scram_server_key);=0A+=09free(conn->check_all_addrs);=0A=20=
=09termPQExpBuffer(&conn->errorMessage);=0A=20=09=
termPQExpBuffer(&conn->workBuffer);=0A=20=0Adiff=20--git=20=
a/src/interfaces/libpq/libpq-int.h=20b/src/interfaces/libpq/libpq-int.h=0A=
index=202546f9f8a5..a96d8ce482=20100644=0A---=20=
a/src/interfaces/libpq/libpq-int.h=0A+++=20=
b/src/interfaces/libpq/libpq-int.h=0A@@=20-432,6=20+432,7=20@@=20struct=20=
pg_conn=0A=20=09char=09=20=20=20*load_balance_hosts;=20/*=20load=20=
balance=20over=20hosts=20*/=0A=20=09char=09=20=20=20*scram_client_key;=09=
/*=20base64-encoded=20SCRAM=20client=20key=20*/=0A=20=09char=09=20=20=20=
*scram_server_key;=09/*=20base64-encoded=20SCRAM=20server=20key=20*/=0A+=09=
char=20=20=20=20=20=20=20*check_all_addrs;=20=20/*=20whether=20to=20=
check=20all=20ips=20within=20a=20host=20or=20terminate=20on=20failure=20=
*/=0A=20=0A=20=09bool=09=09cancelRequest;=09/*=20true=20if=20this=20=
connection=20is=20used=20to=20send=20a=0A=20=09=09=09=09=09=09=09=09=20*=20=
cancel=20request,=20instead=20of=20being=20a=20normal=0A--=20=0A2.39.5=20=
(Apple=20Git-154)=0A=0A=

--Apple-Mail=_3698EAA5-64F4-4E0C-975A-FD9B4D7AFFD2--