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

On Fri, 5 Apr 2024 at 12:09, Jelte Fennema-Nio <me(at)jeltef(dot)nl> wrote:

> On Fri, 5 Apr 2024 at 16:02, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Often?
> >
> > I kind of hope that the protocol starts to evolve a bit more than it
> > has, but I don't want a continuous stream of changes. That will be
> > very hard to test and verify correctness, and a hassle for drivers to
> > keep up with, and a mess for compatibility.
>
> I definitely think protocol changes require a lot more scrutiny than
> many other things, given their hard/impossible to change nature.
>
> But I do think that we shouldn't be at all averse to the act of
> bumping the protocol version itself. If we have a single small
> protocol change in one release, then imho it's no problem to bump the
> protocol version. Bumping the version should be painless. So we
> shouldn't be inclined to push an already agreed upon protocol change
> to the next release, because there are some more protocol changes in
> the pipeline that won't make it in the current one.
>
> I don't think this would be any harder for drivers to keep up with,
> then if we'd bulk all changes together. If driver developers only want
> to support changes in bulk changes, they can simply choose not to
> support 3.1 at all if they want and wait until 3.2 to support
> everything in bulk then.
>
> > > So, what approach of checking feature support are you envisioning
> > > instead? A function for every feature?
> > > Something like SupportsSetProtocolParameter, that returns an error
> > > message if it's not supported and NULL otherwise. And then add such a
> > > support function for every feature?
> >
> > Yeah, something like that.
> > ...
> >
> > > I'm also not sure why you're saying a user is not entitled to this
> > > information. We already provide users of libpq a way to see the full
> > > Postgres version, and the major protocol version. I think allowing a
> > > user to access this information is only a good thing. But I agree that
> > > providing easy to use feature support functions is a better user
> > > experience in some cases.
> >
> > I guess that's a fair point. But I'm worried that if we expose too
> > much of the internals, we won't be able to change things later.
>
> I'll take a look at redesigning the protocol parameter stuff. To work
> with dedicated functions instead.
>
+1

>
> > I really intended the _pq_ prefix as a way of taking something out of
> > the GUC namespace, not as a part of the GUC namespace that users would
> > see. And I'm reluctant to go back on that. If we want to make
> > pg_protocol.${NAME} mean a wire protocol parameter, well maybe there's
> > something to that idea [insert caveats here]. But doesn't _pq_ look
> > like something that was intended to be internal? That's certainly how
> > I intended it.
>

Is this actually used in practice? If so, how ?

>
> I agree that _pq_ does look internal and doesn't clearly indicate that
> it's a protocol related change. But sadly the _pq_ prefix is the only
> one that doesn't error in startup packets, waiting another 5 years
> until pg_protocol is allowed in the startup packet doesn't seem like a
> reasonable solution either.
>
> How about naming the GUCs pg_protocol.${NAME}, but still requiring the
> _pq_ prefix in the StartupPacket. That way only client libraries would
> have to see this internal prefix and they could remap it someway. I
> see two options for that:
> 1. At the server replace the _pq_ prefix with pg_protocol. So
> _pq_.${NAME} would map to pg_protocol.${name}
> 2. At the server replace the _pq_.pg_protocol prefix with pg_protocol.
> So _pq_.pg_protocol.${NAME} would map to pg_protocol.${name}.
>
> I guess you prefer option 2, because that would still leave lots of
> space to do something with the rest of the _pq_ space, i.e.
> _pq_.magic_pixie_dust can still be used for something different than a
> GUC.
>
> Bikeshedding: I think I prefer protocol.${NAME} over
> pg_protocol.${NAME}, it's shorter and it seems obvious that protocol
> is the postgres protocol in this context.
>
> This should be a fairly simple change to make.
>
> > Wouldn't libpq already know what value it last set? Or is this needed
> > because it doesn't know what the other side's default is?
>
> libpq could/should indeed know this, but for debugging/testing
> purposes it is quite useful to have a facility to read the server side
> value. I think defaults should always be whatever was happening if the
> parameter wasn't specified before, so knowing the server default is
> not something the client needs to worry about (i.e. the default is
> defined as part of the protocol spec).
>
> > Hmm, OK. I guess if the PGC_PROTOCOL flag makes it so that the GUC can
> > only be set using ParameterSet, and it also makes them
> > non-transactional, then it's fine. So to be clear, I can't set these
> > in postgresql.conf, or postgresql.auto.conf, or via ALTER $ANYTHING,
> > or via SET, or in any other way than by sending ParameterStatus
> > messages. And when I send a ParameterStatus message, it doesn't matter
> > if I'm in a good transaction, an aborted transaction, or no
> > transaction at all, and the setting change takes effect regardless of
> > that and regardless of any subsequent rollbacks. Is that right?
> >
> > I feel like maybe it's not, because you seem to be thinking that you'd
> > also set these in the startup packet, at least...
>
> Setting PGC_PROTOCOL gucs would be allowed in the startup packet,
> which is fine afaict because that's also something that's part of the
> protocol level and is thus fully controlled by client libraries and
> poolers) But other than that: Yes, conf files, ALTER, and SET cannot
> change these GUCs.
>
+1

Dave