Greg Nancarrow <gregn4...@gmail.com> writes:
> [ v19-0001-Enhance-libpq-target_session_attrs-and-add-target_se.patch ]
I started to look through this, and I find that I'm really pretty
disappointed in the direction the patch has gone of late. I think
there is no defensible reason for the choices that have been made
to have different behavior for v14-and-up servers than for older
servers. It's not necessary and it complicates life for users.
We can use pg_is_in_recovery() on every server version that has
hot standby at all, so there is no reason not to treat the
GUC_REPORT indicator as an optimization that lets us skip a
separate inquiry transaction, rather than something we have to
have to make the feature work correctly.
So I think what we ought to have is the existing read-write
vs read-only distinction, implemented as it is now by checking
"SHOW transaction_read_only" if the server fails to send that
as a GUC_REPORT value; and orthogonally to that, a primary/standby
distinction implemented by checking pg_is_in_recovery(), again
with a fast path if we got a ParameterStatus report.
I do not like the addition of target_server_type. It seems
unnecessary and confusing, particularly because you've had to
make a completely arbitrary decision about how it interacts with
target_session_attrs when both are specified. I think the
justification that "it's more like JDBC" is risible. Any user of
this will be writing C not Java.
A couple of other thoughts:
* Could we call the GUC "in_hot_standby" rather than "in_recovery"?
I think "recovery" is a poorly chosen legacy term that we ought to
avoid exposing to users more than we already have. We're stuck
with pg_is_in_recovery() I suppose, but let's not double down on
bad decisions.
* I don't think you really need a hard-wired test on v14-or-not
in the libpq code. The internal state about read-only and
hot-standby ought to be "yes", "no", or "unknown", starting in
the latter state. Receipt of ParameterStatus changes it from
"unknown" to one of the other two states. If we need to know
the value, and it's still "unknown", then we send a probe query.
We still need hard-coded version checks to know if the probe
query is safe, but those version breaks are far enough back to
be pretty well set in stone. (In the back of my mind here is
that people might well choose to back-port the GUC_REPORT marking
of transaction_read_only, and maybe even the other GUC if they
were feeling ambitious. So not having a hard-coded version
assumption where we don't particularly need it seems a good thing.)
* This can't be right can it? Too many commas.
@@ -1618,7 +1619,7 @@ static struct config_bool ConfigureNamesBool[] =
{"transaction_read_only", PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop("Sets the current transaction's read-only status."),
NULL,
- GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+ GUC_REPORT, GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE |
GUC_DISALLOW_IN_FILE
},
&XactReadOnly,
false,
(The compiler will fail to bitch about that unfortunately, since
there are more struct fields that we leave uninitialized normally.)
BTW, I think it would be worth splitting this into separate server-side
and libpq patches. It looked to me like the server side is pretty
nearly committable, modulo bikeshedding about the new GUC name. We could
get that out of the way and then have a much smaller libpq patch to argue
about.
regards, tom lane
