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

On Mon, 15 Apr 2024 at 19:43, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio <me(at)jeltef(dot)nl> wrote:
> > I think for clients/drivers, the work would generally be pretty
> > minimal. For almost all proposed changes, clients can "support" the
> > protocol version update by simply not using the new features, ...
>
> I mean, I agree if a particular protocol version bump does nothing
> other than signal the presence of some optional, ignorable feature,
> then it doesn't cause a problem if we force clients to support it. But
> that seems a bit like saying that eating wild mushrooms is fine
> because some of them aren't poisonous. The point is that if we roll
> out two protocol changes, A and B, each of which requires the client
> to make some change in order to work with the newer protocol version,
> then using version numbers as the gating mechanism requires that the
> client can't support the newer of those two changes without also
> supporting the older one. Using feature flags doesn't impose that
> constraint, which I think is a plus.

I think we're in agreement here, i.e. it depends on the situation if a
feature flag or version bump is more appropriate. I think the
guidelines could be as follows:
1. For protocol changes that require "extremely minimal" work from
clients & poolers: bump the protocol version.
2. For "niche" features that require some work from clients and/or
poolers: use a protocol parameter feature flag.
3. For anything in between, let's discuss on the thread for that
specific protocol change on the tradeoffs.

On Mon, 15 Apr 2024 at 19:52, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> surely it can't be right to use protocol
> version 3.0 to refer to a bunch of different things. But at the same
> time, surely we don't want clients to start panicking and bailing out
> when everything would have been fine.

I feel like the ProtocolVersionNegotiation should make sure people
don't panic and bail out. And if not, then feature flags won't help
with this either. Because clients would then still bail out if some
feature is not supported.

> I'm unconvinced that we should let ParameterSet change
> non-PGC_PROTOCOL GUCs. The pooler can agree on a list of protocol GUCs
> with the end client that differs from what the server agreed with the
> pooler - e.g., it can add pgbouncer.pool_mode to the list. But for
> truly non-protocol GUCs, we have a lot of ways to set those already.

I feel like you're glossing over something fairly important here. How
exactly would the client know about pgbouncer.pool_mode? Are you
envisioning a list of GUCs which can be changed using ParameterSet,
which the server then sends to the client during connection startup
(using presumably some new protocol message)? If so, then I feel this
same problem still exists. How would the client know which of those
GUCs change wire-protocol behaviour and which don't? It still would
need a hardcoded list (now including pgbouncer.pool_mode and maybe
more) of things that a user is allowed to change using ParameterSet.
So I think a well-known prefix would still be applicable.

To be clear, imho the well-known prefix discussion is separate from
the discussion about whether Postgres should throw an ERROR when
ParameterSet is used to change any non-PGC_PROTOCOL GUC. I'd be fine
with disallowing that if that seems better/safer/clearer to you
(although I'd love to hear your exact concerns about this). But I'd
still want a well-known prefix for protocol parameters. Because that
prefix is not for the benefit of the server, it's for the benefit of
the client and pooler. So the client/pooler can error if any dangerous
GUC is being changed, because the server would accept it and change
the wire-protocol accordingly.