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

On Fri, Jan 5, 2024 at 10:14 AM Jelte Fennema-Nio <me@jeltef.nl> wrote:
> New patchset attached, where I split up the patches in smaller logical units.
> Note that the first 4 patches in this series are not making any
> protocol changes. All they do is set up infrastructure in the code
> that allows us to make protocol changes in the future.
>
> I hope that those 4 should all be fairly uncontroversial, especially
> patch 1 seems a no-brainer to me. Note that this infrastructure would
> be needed for any patchset that introduces protocol changes.
>
> The 5th only bumps the version
>
> The 6th introduces the _pq_.protocol_managed_parms protocol parameter
>
> The 7th adds the new ParameterSet protocol message

Apologies in advance if this sounds harsh but ... I don't like this
design. I have two main complaints.

First, I don't see a reason to bump the protocol version. The whole
reason for adding support for protocol options (_pq_.whatever) is so
that we wouldn't have to change the protocol version to add new
message types. At some point we may want to make a change that's large
enough that we have to do that, or a large enough number of small
changes that it seems worthwhile, but as long as we can add new
features without bumping the protocol version, it seems advantageous
to avoid doing so. It seems easier to reason about and less likely to
break older clients.

Second, I don't really like the idea of selectively turning GUCs into
protocol-managed parameters. I think there are a relatively small
number of things that we'd ever want to manage on a protocol level,
but this design would force us to make it work for any GUC whatsoever.
That seems like it adds a lot of complexity for not much benefit. If
somebody makes a random GUC into a protocol-managed parameter and then
somebody updates postgresql.conf and then the config file is reloaded,
I guess we need to refuse to adopt the new value of that parameter?
That doesn't seem like a lot of fun to implement. What about the fact
that GUCs are transactional and protocol-managed parameters maybe
shouldn't be? We can dodge a lot of complexity here, I think, if we
just put the things into this new mechanism that have a clear reason
to be there.

To answer a few questions from upthread (MHO):

> - Since we currently don't have any protocol parameters. How do I test
> it? Should I add a debug protocol parameter specifically for this
> purpose? Or should my tests  just always error at the moment?

I think we should start by picking one or two protocol-managed
parameters that we want to add, and then adding those in a way that is
distinct from the GUC system. I don't think we should add an abstract
system divorced from any particular application.

> - How do you get the value of a protocol parameter status? Do we
> expect the client to keep track of what it has sent? Do we always send
> a ParameterStatus message whenever it is changed? Or should we add a
> ParamaterGet protocol message too?

I would expect that the client would have full control over these
values and so the configured value would always be the default (which
should be non-configurable to avoid ambiguity) unless the client set
it to something else (in which case the client should know the value).
So I would think that we'd only need a message to set parameter
values, not one to get them.

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