> On 16 Aug 2025, at 04:43, Andrew Jackson <[email protected]> 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.
On naming and how to configure the feature:
What the option does
It only matters when target_session_attrs is set. On a target_session_attrs
mismatch (e.g. we wanted primary but got standby), it controls:
0 (default): treat this host as failed and move to the next host.
1: stay on this host and try the next address for the same hostname.
So the name is really answering: on session-attrs mismatch, try next address
vs next host?
Is "check_all_addrs" good?
Pros: Short, easy to type, and the env var PGCHECKALLADDRS is clear.
Cons:
It doesn't mention target_session_attrs, so the link to that feature
isn't obvious.
"Check" is vague (check for what?). The important part is "try all
addresses (for this host) on mismatch."
With only check_all_addrs=1 in the conninfo, someone might not guess
it's about session attrs.
So it's usable but not ideal.
Naming alternatives
try_all_addrs
Shorter; "try" is more accurate than "check". Still doesn't tie to
target_session_attrs.
try_next_addr_on_mismatch
Very clear. Long; "mismatch" is a bit generic.
target_session_attrs_try_all_addrs
Explicitly tied to the feature. Too long for conninfo.
session_attrs_try_all_addrs
Shorter, still linked to session attrs. Slightly ambiguous ("try all"
what?).
try_all_host_addrs
Makes "addresses of current host" clear. Doesn't mention session attrs.
A reasonable compromise is try_all_addrs: same length as now, clearer verb,
and the docs can state it applies when target_session_attrs is used. If you
want the link to session attrs in the name, session_attrs_try_all_addrs is
the next best, at the cost of length.
Other ways to configure it
1) No new option (behavioral change only)
Evgeny's approach: always try next address on session-attrs mismatch. No
naming issue, but it changes default behavior and was argued against for
backward compatibility.
2) Fold into target_session_attrs
E.g. target_session_attrs=read-write,try_all_addrs or read-write+try_all.
Pro: option is clearly scoped to that feature. Con: one parameter does two
things (desired role + connection strategy); parsing and docs get more
complex.
3) Keep a separate option (current design)
A dedicated connection parameter that only has effect when
target_session_attrs is set. Pro: simple parsing, clear in conninfo, easy
to document. Con: the name should make the "only with target_session_attrs"
contract obvious (in name or docs).
4) Tie to load_balance_hosts
E.g. when load_balance_hosts=random (or disable) and target_session_attrs
is set, add a third mode like try_all_addrs. Con: mixes two concepts (load
balancing vs which address to try next on mismatch); semantics get
confusing.
Recommendation
Naming: Prefer try_all_addrs over check_all_addrs (clearer, same length). If
you want the link to session attrs in the name, use session_attrs_try_all_addrs
and accept the length.
Configuration: Keeping a separate connection parameter is the right design; the
main improvement is the name and a one-line note in the docs that it only has
effect when target_session_attrs is set. I wouldn't fold it into
target_session_attrs or into load_balance_hosts.
Made with Cursor.
