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

On Tue, 4 Jun 2024 at 16:47, Robert Haas <robertmhaas@gmail.com> wrote:
> I think that separating (1) and (3) is going to make us happy rather
> than sad.

Attached is a new patchset where I keep protocol parameters completely
separate from GUCs. I do think I indeed like it better this way,
although I'm not entirely sure about the newly proposed
NegotiateProtocolParameter message (which we discussed in person last
week). I'm looking forward to hearing what you think of these newly
proposed changes.

Patch 1 & 2: Minor code changes with zero effect until we actually
bump the protocol version or add protocol parameters. I hope these can
be merged rather soon to reduce the number of patches in the patchset.
Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But
after bumping the version this would be a user visible API change, so
I expect it requires a bit more discussion.
Patch 4: Adds a new connection option, but initially all parameters
that it takes have the same effect.
Patch 5: Starts using the new connection option from Patch 4
Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a
more complex way. (nothing changes in practice yet)
Patch 7: Bump the protocol version to 3.2 (see commit message for why
not bumping to 3.1)
Patch 8: The main change: Infrastructure and protocol support to be
able to add protocol parameters
Patch 9: Adds a report_parameters protocol extension as a POC for the
changes in the previous patch.

See the commit messages for more details.

On Tue, 4 Jun 2024 at 17:00, Robert Haas <robertmhaas@gmail.com> wrote:
> Yeah. I wonder how Heikki thinks he can do (2) without breaking
> everything. Maybe just adding an extra, optional, longer field onto
> the existing message and hoping that client implementations ignore the
> extra field? If that's not good enough, then I don't understand how
> that can be done without breaking compatibility in a fundamental and
> relatively serious way -- at which point maybe bumping the protocol
> version is the right thing to do.

FYI Heikki his patchset is here:
https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi

Afaict there's no way to implement this with old clients supporting
the new message. So it would need to be opt-in from the client
perspective, either using a version bump or a protocol parameter (e.g.
large_cancel=true). IMHO a version bump would make more sense for this
one.

> Really, what I'm strongly opposed to is not bumping the version, but
> rather doing things that break compatibility such that we need to bump
> the version. *If* we have a problem that's sufficiently serious to
> justify breaking compatibility anyway, then we don't really lose
> anything by bumping the version, and indeed we gain something. I just
> want us to be searching for ways to avoid breaking interoperability,
> rather than seeking them out. If it becomes impossible for a PG 18 (or
> whatever version) server to communicate with earlier servers without
> specifying special options, or worse yet at all, then a lot of people
> are going to be very sad about that. We will get a bunch of complaints
> and a bunch of frustrated users, and they will not be impressed by
> vague claims of necessity or desirability. They'll just be mad.

As discussed in person last week: I think we agree on this. There is
obviously some moment where it becomes acceptable e.g. libpq45 not
being able to connect to PG11 (at least by default) would probably be
reasonable. But currently I don't think we're there yet, given the
current level of support for NegotiateProtocolVersion in the wild.
While many PG servers support this message, no pooler supports it at
the moment. Not being able to connect to a pooler by default is indeed
going to make people quite angry.

So that's why in this new version of the patchset, libpq continues to
connect using the 3.0 protocol version by default. However, as soon as
a user asks for a feature in their connection string that requires a
protocol parameter, or when they explicitly ask for a newer protocol
version to be used, then both the newest protocol version supported by
the client as well as all known protocol parameters are requested.

Again as discussed in person, this is meant to prevent ossification of
the protocol. For servers that don't support NegotiateProtocolVersion
currently, a client asking for a new protocol version is just as
breaking as a client asking for an unknown protocol parameter (i.e.
the server will close the connection, because it doesn't know what to
do). Since sending either of these anyway is a breaking change in the
StartupMessage, we might as well break it as much as possible, in the
hopes that clients/servers/poolers implement NegotiateProtocolVersion
correctly. Which means afterwards we can safely add new protocol
parameters again, as well as safely bump the minor protocol version
again without worrying about breaking the ecosystem (because now
feature+version negotiation is implemented everywhere). Not bumping
the version number, or not adding a protocol parameter, has the risk
of people implementing NegotiateProtocolVersion in such a way that it
only works for the thing that we changed the first time (i.e. only
version negotiation or only feature negotiation, instead of both)

> The question for me here is not "what is the theoretically right thing
> to do?" but rather "what am I going to tell angry users when they
> demand to know why I committed the patch that broke this?". "The old
> way was insecure so we had to change it" might be a good enough reason
> for people to calm down and stop yelling at me, but "it's no use
> having a protocol version if we never bump it" definitely won't be.

I think that is totally fair, see also my reason to bump the protocol
version to 3.2 instead of 3.1 in the commit message (i.e. because
PgBouncer currently mistakenly reports 3.1 as supported).