Thread: Re: 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]. 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

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:

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]. 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

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:

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:

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]. 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