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

On Fri, Mar 8, 2024 at 6:47 AM Jelte Fennema-Nio <me@jeltef.nl> wrote:
> 1. 0001-0005 are needed for any protocol change, and hopefully
> shouldn't require much discussion

I feel bad arguing about the patches that you think are a slam-dunk,
but I find myself disagreeing with the design choices.

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 don't think it would be catastrophic if this got committed or
anything -- it could always be changed later if the need arose -- but
I wanted to mention that I had a reason for not doing it, and I'm
still not particularly convinced that we should do it.

I'm really unhappy with 0002-0004. Before
ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed, any parameter included in
the startup message that wasn't in a short, hard-coded list was
treated as a request to set a GUC. 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. 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. Because if we implement what this patch wants to do in this
way, basically consuming the entire namespace that
ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed created in on shot, and then
later we want the sort of thing that I'm postulating, we'll have to
manufacture another new namespace for that need.

And it seems to me that there are other ways we could do this. For
example, suppose we introduced just one new protocol parameter; for
the sake of argument, I'll call it _pq_.protocol_set. If the client
sends this parameter and the server accepts it, then the client knows
that the server supports a new protocol message ProtocolSetParameter,
which is the only way to set GUCs in the new PROTOCOL_EXTENSION
category. New libpq functions with names like, I don't know,
PQsetProtocolParameter(), are added to send such messages, and they
return an error if there are network problems or whatever, or if the
server didn't accept the _pq_.protocol_set option at startup time.

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

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