Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Support connection load balancing in libpq
- 7f5b19817eaf 16.0 cited
-
Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2024-11-20T15:51:12Z
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` to have clients dynamically connect to the primary or `psql postgresql:// user@pg.database.com/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
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-16T12:03:44Z
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
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2025-02-24T15:38:31Z
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 >
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2025-02-24T16:07:33Z
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 >> >
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2025-02-24T18:06:59Z
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: > 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 >>> >>
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2025-05-17T13:41:43Z
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: > 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: > >> 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 >>>> >>>
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Navrotskiy Artem <bozaro@yandex.ru> — 2025-06-24T19:32:07Z
<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><div><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" <x4mmm@yandex-team.ru>:</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:andrewjackson947@gmail.com" rel="noopener noreferrer">andrewjackson947@gmail.com</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-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94" rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-b05b74d2a97d7f74d4311ba1702d732f0df1b101c6ac99c146b51215174cf3ffR94</a><br />[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/NpgsqlConnector.cs#L986" rel="noopener noreferrer">https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/NpgsqlConnector.cs#L986</a><br />[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-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660" rel="noopener noreferrer">https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e061b9d4cdae9c8922ded05753a629d70f2ac1de1d4f6d5a4aeb7f68R1660</a></p></blockquote><div> </div><div> </div><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">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>
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2025-08-15T23:43:20Z
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
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Evgeny Kuzin <evgeny.kuzin@outlook.com> — 2026-03-05T16:07:47Z
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/AM9PR09MB49008B02CDF003054D5D4E00977DA@AM9PR09MB4900.eurprd09.prod.outlook.com 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`> 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`> 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
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2026-03-05T21:15:26Z
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.
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrey Borodin <x4mmm@yandex-team.ru> — 2026-03-06T06:44:20Z
> 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.
-
Re: Add Option To Check All Addresses For Matching target_session_attr
Andrew Jackson <andrewjackson947@gmail.com> — 2026-03-13T01:29:44Z
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 <x4mmm@yandex-team.ru> wrote: > > > > > 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. >