Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

On Fri, May 24, 2024 at 6:09 PM Jelte Fennema-Nio <me(at)jeltef(dot)nl> wrote:
> Separating it from the GUC infrastructure will mean we need to
> duplicate a lot of the infrastructure. Assuming we don't care about
> SHOW or pg_settings (which I agree are not super important), the
> things that we would want for protocol parameters to have that guc.c
> gives us for free are:
> 1. Reporting the value of the parameter to the client (done using
> ParameterStatus)
> 2. Parsing and validating of the input, bool, int, enum, etc, but also
> check_hook and assign_hook.
> 3. Logic in all connection poolers to change GUC values to the
> client's expected values whenever a server connection is handed off to
> a client
> 4. Permission checking, if we want some protocol extensions to only be
> configurable by a highly privileged user
>
> All of those things would have to be duplicated/re-implemented if we
> make protocol parameters their own dedicated thing. Doing that work
> seems like a waste of time to me, and would imho add much more
> complexity than the proposed 65 lines of code in 0011.

I think that separating (1) and (3) is going to make us happy rather
than sad. For example, if a client speaks to an intermediate pooler
which speaks to a server, the client-pooler connection could have
different values from the pooler-server connection, and then if
everything is done via ParameterStatus messages it's actually more
complicated for the pooler, which will have to edit ParameterStatus
messages as they pass through, and possibly also create new ones out
of nothing. If we separate things, the pooler can always pass
ParameterStatus messages unchanged, and only has to worry about the
separate infrastructure for handling these new things.

Said differently, I'd agree with you if (a) ParameterStatus weren't so
dubiously designed and (b) all poolers were going to want every
protocol parameter to match on the input side and the output side. And
maybe in practice (b) will happen, but I want to leave the door open
to cases where it doesn't, and as soon as that happens, I think this
becomes a hassle, whereas separate mechanisms don't really hurt much.
As you and I discussed a bit in person last week, two for loops rather
than one in the pooler isn't really that big of a deal. IMHO, that's a
small price to pay for an increased chance of not boxing ourselves
into a corner depending on how these parameters end up getting used in
practice.

As for (2) and (4), I agree there's some duplication, but I think it's
quite minor. We have existing facilities for parsing integers and
booleans that are reused by a lot of code already, and this is just
one more place that can use them. That infrastructure is not
GUC-specific. The permissions-checking stuff isn't either. The vast
bulk of the GUC infrastructure is concerned with (1) allowing for GUCs
to be set from many different sources and (2) handling their
transactional nature. We don't need or want either of those
characteristics here, so a lot of that complexity just isn't needed.

--
Robert Haas
EDB: http://www.enterprisedb.com