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

On Fri, 5 Apr 2024 at 18:48, Robert Haas <robertmhaas@gmail.com> wrote:
> Maybe we'd be better off adding a libpq connection
> option that forces the use of a specific minor protocol version, but
> then we'll need backward-compatibility code in libpq basically
> forever. But maybe we need that anyway to avoid older and newer
> servers being unable to communicate.

I think this would be good because it makes testing easy and, like you
said, I think we'll need this backward-compatibility code in libpq
anyway to be able to connect to old servers. To have even better and
more realistic test coverage though, I think we might also want to
actually test new libpq against old postgres servers and vice-versa in
a build farm animal though.

> Plus, you've got all of the consequences for non-core drivers, which
> have to both add support for the new wire protocol - if they don't
> want to seem outdated and eventually obsolete - and also test that
> they're still compatible with all supported server versions.

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, e.g. a
client can "support" the ParameterSet feature, by simply never sending
the ParameterSet message. So binding it to a protocol version bump
doesn't make it any harder for that client to support that protocol
version. I'm not saying that is the case for all protocol changes, but
based on what's being proposed so far that's definitely a very common
theme. Overall, I think this is something to discuss for each protocol
change in isolation: i.e. how to make supporting the new feature as
painless as possible for clients/drivers.

> Connection poolers have the same set of problems.

For connection poolers this is indeed a bigger hassle, because they at
least need to be able to handle all the new message types that a
client can send and maybe do something special for them. But I think
if we're careful to keep connection poolers in mind when designing the
features themselves then I think this isn't necessarily a problem. And
probably indeed for the features that we think are hard for connection
poolers to implement, we should be using protocol extension parameter
feature flags. But I think a lot of protocol would be fairly trivial
for a connection pooler to support.

> The whole thing is
> almost a hole with no bottom. Keeping up with core changes in this
> area could become a massive undertaking for lots and lots of people,
> some of whom may be the sole maintainer of some important driver that
> now needs a whole bunch of work.

I agree with Dave here, if you want to benefit from new features
there's some expectation to keep up with the changes. But to be clear,
we'd still support old protocol versions too. So we wouldn't break
connecting using those clients, they simply wouldn't benefit from some
of the new features. I think that's acceptable.

> I'm not sure how much it improves things if we imagine adding feature
> flags to the existing protocol versions, rather than whole new
> protocol versions, but at least it cuts down on the assumption that
> adopting new features is mandatory, and that such features are
> cumulative. If a driver wants to support TDE but not protocol
> parameters or protocol parameters but not TDE, who are we to say no?
> If in supporting those things we bump the protocol version to 3.2, and
> then 3.3 fixes a huge performance problem, are drivers going to be
> required to add support for features they don't care about to get the
> performance fixes?

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. I think this trade-off is
something we should be deciding on based on the specific protocol
change. But if work needed to "support" the feature is "minimal"
(to-be-defined exactly what we consider minimal), I think making it
part of a protocol version bump is reasonable.

> I see some benefit in bumping the protocol version
> for major changes, or for changes that we have an important reason to
> make mandatory, or to make previously-optional features for which
> support has become in practical terms universal part of the base
> feature set. But I'm very skeptical of the idea that we should just
> handle as many things as possible via a protocol version bump. We've
> been avoiding protocol version bumps like the plague since forever,
> and swinging all the way to the other extreme doesn't sound like the
> right idea to me.

I think there's two parts to a protocol version bump:
1. The 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).

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'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.