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

On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio <me@jeltef.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 there's an important trade-off here. On one side we don't want
> to make maintainers of clients/poolers do lots of work to support
> features they don't care about. And on the other side it seems quite
> useful to limit the amount of feature combinations that are used it
> the wild (both for users and for us) e.g. the combinations of
> backwards compatibility testing you were talking about would explode
> if every protocol change was a feature flag.

This is a good point, and I agree. It's a little hard to discuss this
in the abstract. I wouldn't want to have feature flags that say:

1. I support rows containing an extra-large number of bytes.
2. I support rows containing an extra-large number of columns.
3. I support queries returning an extra-large number of rows.

It is quite likely that there might be bugs that only manifest with
particular combinations of those flags; knowing that someone who
supports (3) must also support (1) and (2) seems like it would make
everyone's life easier. But on the other hand, I wouldn't mind having
feature flags that say:

1. I support transparent column encryption.
2. I support letting a connection pooler lock down the session role in
a way that can't be reversed from SQL.
3. I support compression.

It's still not theoretically impossible that those features could
interact with each other in some way, but it seems a lot less likely.
Here, I think a driver ought to be able to choose any subset of these
things and support only the ones they care about, without having to
worry about the server deciding to do something for which the driver
is unprepared. Also, in a case like this, if there are bugs that only
occur in certain combinations, we have to test for and fix those
anyway, because even if a driver supports all of those features,
they're not all going to be used for every connection.

> I think there's two parts to a protocol version bump:
> 1. TI ahe changes that cause us to consider a protocol bump
> 2. The actual act of bumping the protocol version
>
> I think 1 is a thing we should be careful about every time (especially
> regarding impact on clients/poolers). But 2 shouldn't be something
> that we should consider dangerous/scary. I think that every change
> that we make to the protocol (no matter how minor or backwards
> compatible it is), should be accompanied with a protocol version bump.
> This isn't what has happened in the past, and it makes it quite hard
> to understand what "supporting" a specific protocol version actually
> means. e.g. PgBouncer currently supports protocol 3.0, but doesn't
> support the NegotiateProtocolVersion message (I'm working on fixing
> that).

I'm generally not a fan of giving things version numbers and then not
changing the version numbers when you change the thing, but I find
myself reluctant to apply that principle to this case. I think it's
bad that we keep adding functions to libpq and sometimes changing the
behavior of existing functions and never, ever bumping the libpq .so
version. I've seen that cause real, practical problems. It means for
example that you can't make an RPM depend on libpq.so.5 and expect
that to do anything meaningful -- every version going back forever is
version 5, even if it doesn't contain the functions (or other behavior
changes) that are needed for some program compiled against a newer
version of PostgreSQL to work.

But the wire protocol changes very slowly, and I think that is a
difference that actually matters quite a bit here. Broadly speaking, I
can use a psq

> To take it to the extreme: I think we should get to a state, where if
> we bump the protocol version at the client and server side without
> actually making any protocol changes, everything should continue to
> work fine. If we'd do that right now, then libpq wouldn't be able to
> connect to old postgres versions anymore.

I think I agree with this, but it seems like a bootstrapping problem
and nothing more.

>
> > I'm not sure what I think about this. Do we need these new GUCs to be
> > both PGC_PROTOCOL *and also* live in a separate namespace? I see the
> > need for the former pretty clearly: if these kinds of things are to be
> > part of the GUC system (which wasn't my initial bias, but whatever)
> > then they need to have some important behavioral differences from
> > other GUCs and so we need a flag to signal that. But what problem are
> > we solving by also giving them special-looking names, and are we sure
> > we wouldn't rather solve that problem some other way?
>
> Clients might want to allow the user of the client to change regular
> parameters using ParameterSet (e.g. so that a connection pooler can
> intercept those ParameterSet messages and change its own behaviour if
> the parameter name is pgbouncer.pool_mode). But they wouldn't want a
> user to set any parameters that change the wire-protocol this way. And
> because an old client might connect to a new server a simple
> hard-coded list of parameters at the client side is not sufficient.
>
> I can see two ways around this:
> 1. Using a well-known prefix or namespace for parameters that change
> the wire protocol. (exact prefix to be bikeshedded on)
> 2. Using a hard-coded list at the client AND disallow changing
> PGC_PROTOCOL parameters at the server if the negotiated protocol
> version is lower than the version this parameter was introduced in AND
> bump the protocol version whenever we add a new PGC_PROTOCOL
> parameter.
>
> I think 1 is easier to implement at the client side, as it only
> requires a prefix comparison instead of keeping track of a list.



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