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

On Thu, 14 Mar 2024 at 16:45, Robert Haas <robertmhaas@gmail.com> wrote:
> I feel bad arguing about the patches that you think are a slam-dunk,
> but I find myself disagreeing with the design choices.

No worries, thanks a lot for responding. I'm happy to discuss this
design further. I didn't necessarily mean these patches were a
slam-dunk. I mainly meant that these first few patches were not
specific to any protocol change, but are changes we should agree on
before any change to the protocol is possible at all. Based on your
response, we currently disagree on a bunch of core things.

I'll first try to summarize my view on (future) protocol changes and
why I think the current core design in this patchset is the correct
path forward, and then go into some details inline in your response
below:

In my view there can be, **by definition,** only two general types of
protocol changes:
1. A "simple protocol change": This is one that requires agreement by
both the client and server, that they understand the new message types
involved in this change. e.g. the ParameterSet message proposal (this
message type is either supported or it's not)
2. A "parameterized protocol change": This requires the same as 1, but
should also allow some parameterization from the client, e.g. for the
compression proposal, the client should specify what compression
algorithm the server should use to compress data when sending it to
the client.

Client and Server can agree that a "simple protocol change" is
supported by both advertising a minimum minor protocol version. And
for a "parameterized protocol change" the client can send a _pq_
parameter in the startup packet.

So, new _pq_ parameters should only ever be introduced for
parameterized protocol changes. They are not meant to advertise
support, they are meant to configure protocol features. For a
non-configurable protocol feature, we'd simply bump the protocol
version. And since _pq_ parameters thus always control some setting at
the session level, we can simply store it as a GUC, because that's how
we store all our parameters at a session level.

> Regarding 0001, I considered making this change as part of
> ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed but decided against it,
> because it seemed like it was making the assumption that we always
> wanted to initiate new connections with the latest protocol version
> that we know how to accept, and I wasn't sure that would always be
> true.

I think given the automatic downgrade supported by the
NegotiateProtocolVersion, there's no real down-side to requesting the
newest version by default. The only downside I can see is when
connecting to other applications (i.e. non PostgreSQL servers) that
implement the postgres protocol, but don't implement
NegotiateProtocolVersion. But for that I added the
max_protocol_version connection option in 0006 (of my latest
patchset).

> I'm really unhappy with 0002-0004.

Just to be clear, (afaict) your argument below seems to only really be
about 0004, not about 0002 or 0003. Was there anything about 0002 &
0003 that you were unhappy with? 0002 & 0003 are not dependent an 0004
imho. Because even when not making _pq_ parameters map to GUCs, we'd
still need to change libpq to not instantly close the connection
whenever a _pq_ parameter (or new protocol version) is not supported
by the server (which is what 0002 & 0003 do).

> That left no room for any other
> type of protocol modification, so that commit carved out an exception,
> where when we something that starts with _pq_., it's assumed to be
> setting some other kind of thing, not a GUC.

Okay, our interpretation is very different here. From my perspective
introducing a non-GUC namespace is NOT AT ALL the benefit of the _pq_
prefix. The main benefit (imho) is that it allows sending an
"optional" parameter (i.e GUC) in the startup packet. So, one where if
the server doesn't recognize it the connection attempt still succeeds.
If you specify a normal GUC in the connection parameters and the
server doesn't know about it, the server will close the connection.
So, to be able to send a GUC that depends on the protocol and/or
server version in an optional way, you'd need to wait for an extra
network roundtrip until the server tells you what protocol and/or
server version they are.

> But 0004 throws that out
> the window and decides, nope, those are just GUCs, too. Even if we
> don't have a specific reason today why we'd ever want such a thing, it
> seems short-sighted to give up on the possibility that in the future
> we will.

Since I believe a _pq_ parameter should only be used to control
settings at the session level. I don't think it would be short-sighted
to give-up on the possibility to store them as anything else as GUCs.
Because in the many years that we've had GUCs, we've been able to
store all session settings using that infrastructure. BUT PLEASE NOTE:
I don't think we are giving up on the thing you're describing (see
explanation in final part of this email)

> With this kind of design, you're just consuming one element of the
> _pq_ namespace, and the next person who wants to do something can
> consume one more element, and we'll be able to go on for a very long
> time without running out of room. This is really how I intended this
> mechanism to be used, and the only real downside I see as compared to
> what you've done is that you can't set the protocol GUCs in the
> startup packet, but must set them afterward via separate messages. If
> that's a problem, then the proposal I just outline needs modification

I very much think that's a problem. This would mean an extra roundtrip
at connection startup. Which, as I described above, is to me the whole
benefit of the _pq_ namespace.

> ... but no matter what we do exactly, I don't want the very first
> protocol extension we ever add to eat up all of _pq_. I intended that
> to support decades worth of extensibility, not just one patch.

This seems to be the core of your argument. But honestly, I don't
understand this logic at all. Why do you think that assigning _pq_
parameters to GUCs **for the ones that match an existing GUC** would
have such a far reaching effect into the future. There's only a
handful of _pq_ parameters being proposed on the mailinglist at the
moment. Even if we implement all of those as GUCs, and in the future,
we'd want a _pq_ parameter that does not map to GUC (which I
personally doubt will ever be the case). Then we can simply change the
server code like so and do something special for that parameter:

                        }
+                       else if (strcmp(nameptr,
"_pq_.some_non_guc_parameter") == 0)
+                       {
+                               // Do something with the parameter
+                       }
                        else if (strncmp(nameptr, "_pq_.", 5) == 0 &&
!find_option(nameptr, false, true, ERROR))
                        {
                                /*
                                 * We report unkown protocol
extensions using the
                                 * NegotiateProtocolVersion message
instead of erroring
                                 */


This would be completely backwards compatible afaict, because
_pq_.some_non_guc_parameter would not have been a GUC before. So the
only thing that would have happened if you sent it, is that you would
get back that the server doesn't support it in the
NegotiateProtocolVersion packet (just like what is happening currently
since ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed).

SO TO BE VERY CLEAR: (afaict) interpreting _pq_ parameters as GUCs
right now does not limit our ability to do something differently for
new _pq_ parameters that we introduce in the future.