public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrey Borodin <[email protected]>
To: Andrew Jackson <[email protected]>
To: Vladimir Sitnikov <[email protected]>
To: Dave Page <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Add Option To Check All Addresses For Matching target_session_attr
Date: Sun, 16 Feb 2025 17:03:44 +0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg@mail.gmail.com>
References: <CAKK5BkESSc69sp2TiTWHvvOHCUey0rDWXSrR9pinyRqyfamUYg@mail.gmail.com>
Hi Andrew!
cc Jelte, I suspect he might be interested.
> On 20 Nov 2024, at 20:51, Andrew Jackson <[email protected]> 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-b05b74d2a9...
[1] https://github.com/jackc/pgx/blob/master/pgconn/pgconn.go#L177
[2] https://github.com/npgsql/npgsql/blob/7f1a59fa8dc1ccc34a70154f49a768e1abf826ba/src/Npgsql/Internal/N...
[3] https://github.com/pgjdbc/pgjdbc/pull/3012#discussion_r1408069450
[4] https://github.com/postgres/postgres/commit/7f5b19817eaf38e70ad1153db4e644ee9456853e#diff-8d819454e0...
Attachments:
[application/octet-stream] v2-0001-Add-option-to-check-all-IPs.patch (2.9K, 2-v2-0001-Add-option-to-check-all-IPs.patch)
download | inline diff:
From e13764ae84555034fedc430e189e056ea5af2d10 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 14 Feb 2025 23:13:55 +0500
Subject: [PATCH v2] Add option to check all IPs
---
src/interfaces/libpq/fe-connect.c | 25 +++++++++++++++----------
src/interfaces/libpq/libpq-int.h | 1 +
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 85d1ca2864..11c9e0cf4c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -372,6 +372,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
{"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2,
offsetof(struct pg_conn, scram_server_key)},
+ {"check_all_addrs", "PGCHECKALLADDRS",
+ DefaultLoadBalanceHosts, NULL,
+ "Check-All-Addrs", "", 1,
+ offsetof(struct pg_conn, check_all_addrs)},
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
@@ -4326,11 +4330,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -4381,11 +4385,11 @@ keep_going: /* We will come back to here until there is
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
- /*
- * Try next host if any, but we don't want to consider
- * additional addresses for this host.
- */
- conn->try_next_host = true;
+ if (conn->check_all_addrs && conn->check_all_addrs[0] == '1')
+ conn->try_next_addr = true;
+ else
+ conn->try_next_host = true;
+
goto keep_going;
}
}
@@ -5002,6 +5006,7 @@ freePGconn(PGconn *conn)
free(conn->load_balance_hosts);
free(conn->scram_client_key);
free(conn->scram_server_key);
+ free(conn->check_all_addrs);
termPQExpBuffer(&conn->errorMessage);
termPQExpBuffer(&conn->workBuffer);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2546f9f8a5..a96d8ce482 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -432,6 +432,7 @@ struct pg_conn
char *load_balance_hosts; /* load balance over hosts */
char *scram_client_key; /* base64-encoded SCRAM client key */
char *scram_server_key; /* base64-encoded SCRAM server key */
+ char *check_all_addrs; /* whether to check all ips within a host or terminate on failure */
bool cancelRequest; /* true if this connection is used to send a
* cancel request, instead of being a normal
--
2.39.5 (Apple Git-154)
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add Option To Check All Addresses For Matching target_session_attr
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox