Okay, attempt number 5 attached. The primary changes with the previous
version are:
1. Split up commits a bit differently. I think each commit now stands
on its own and is an incremental improvement that could be committed
without any of the later ones being necessary. Descriptions of why
each commit is useful can be found in the commit message. Especially
the first 3 (and even first 4) seem rather uncontroversial to me.
2. ParameterSet now works for normal backend parameters too. For
normal parameters it works just like SET does (so in a transactional
manner). For protocol extension parameters it still works too, but
there it errors when trying to change protocol extension parameters
within a transaction. Thus (imho) elegantly avoiding confusing
situations around rolling back protocol extension parameters.
3. As Tom suggested, it now uses a PGC_PROTOCOL context for the
protocol extension GUCs, instead of using a GUC_PROTOCOL_EXTENSION
flag. This definitely seems like the cleanest way of adding "protocol
only" parameters to the current GUC system.
4. _pq_.protocol_managed_params its list of GUCs can now only contain
GUCs that have the PGC_USERSET or PGC_SUSET context.
On Fri, 5 Jan 2024 at 17:28, Robert Haas <robertmhaas@gmail.com> wrote:
> First, I don't see a reason to bump the protocol version.
It's still bumping the protocol version. I think this is a necessity
with the current changeset, since ParameterSet now applies to plain
GUCs too and. As I clarified in a previous email, this does **not**
break old clients, since the server silently downgrades when an older
protocol version is requested.
> 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.
It now limits the possible GUCs that can be changed to PGC_USERSET and
PGC_SUSET. If desired, we could limit it even further by using an
allowlist of reasonable GUCs or set a flag on GUCs that can be
"upgraded" . Things that seem at least reasonable to me are "role",
"session_authorization", "client_encoding".
> 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?
This was actually really easy to do after changing to PGC_PROTOCOL.
This exact behaviour is needed for PGC_BACKEND parameters, so I simply
used that same small if statement for PGC_PROTOCOL.
If you still think we shouldn't do this, then the only other option I
can think of is to only allow GUCs with the GUC_DISALLOW_IN_FILE flag.
This would rule out adding client_encoding in the list though, but
using role and session_authorization would still be possible.