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

Ugh, I seem to have somehow missed this response completely.

On Thu, 14 Mar 2024 at 21:33, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> While I think that will be a common
> pattern, I do not think it will be a universal one. I do agree,
> however, that every possible variation of the protocol is either
> Boolean-valued (i.e. are we doing X or not?) or something more
> complicated (i.e. how exactly are doing X?).

Agreed

> This is definitely not how I was thinking about it. I was imagining
> that we wanted to reserve bumping the protocol version for more
> significant changes, and that we'd use _pq_ parameters for relatively
> minor new functionality whether Boolean-valued or otherwise.

Yeah, we definitely think differently here then. To me bumping the
minor protocol version shouldn't be a thing that we would need to
carefully consider. It should be easy to do, and probably done often.

> The user is entitled to
> know whether PQsetProtocolParameter() will work or not, and the user
> is entitled to know whether it has a chance of working next time if it
> didn't work this time, and when it fails, the user is entitled to a
> good error message explaining the reason for the failure. But the user
> is not entitled to know what negotiation took place over the wire to
> figure that out. They shouldn't need to know that the _pq_ namespace
> exists, and they shouldn't need to know whether we negotiated the
> availability or unavailability of PQsetProtocolParameter() using [a]
> the protocol minor version number, [b] the protocol major version
> number, [c] a protocol option called parameter_set, or [d] a protocol
> option called bananas_foster. Those are all things that might change
> in the future.

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?

I think I might agree with you that it would be nice for features that
depend on a protocol extension parameter, indeed because in the future
we might make them required and they don't have to update their logic
then. But for features only depending on the protocol version I
honestly don't see the point. A protocol version check is always going
to continue working.

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.

> But this is another example of a problem that can *easily* be fixed
> without using up the entirety of the _pq_ namespace. You can introduce
> _pq_.dont_error_on_unknown_gucs=1 or
> _pq_.dont_error_on_these_gucs='foo, bar, baz'. The distinction between
> the startup packet containing whizzbang=frob and instead containing
> _pq_.whizzbang=frob shouldn't be just whether an error is thrown if we
> don't know anything about whizzbang.

Nice idea, but that sadly won't work well in practice. Because old PG
versions don't know about _pq_.dont_error_on_unknown_gucs. So that
would mean we'd have to wait another 5 years (and probably more) until
we could set non-_pq_-prefixed GUCs safely in the startup message,
using this approach.

Two side-notes:
1. I realized there is a second benefit to using _pq_ for all GUCs
that change the protocol level that I didn't mention in my previous
email: It allows clients to assume that _pq_ prefixed GUCs will change
the wire-protocol and that they should not allow a user of the client
to set them willy-nilly. I'll go into this benefit more in the rest of
this email.
2. To clarify: I'm suggesting that a startup packet containing
_pq_.whizzbang would actually set the _pq_.whizzbang GUC, and not the
whizzbang GUC. i.e. the _pq_ prefix is not stripped-off when parsing
the startup packet.

> I guess I'm in the same position as you are -- your argument doesn't
> really make any sense to me. That also has the unfortunate
> disadvantage of making it difficult for me to explain why I don't
> agree with you, but let me just tick off a few things that I'm
> thinking about here:

Thanks for listing these thoughts. That makes it much easier to
discuss something concrete.

> 1. Connection poolers. If I'm talking to pgpool and pgpool is talking
> to the server, and pgpool and I agree to use compression, that's
> completely separate from whether pgpool and the server are using
> compression. If I have to interrogate the compression state by
> executing "SHOW some_compression_guc", then I'm going to get the wrong
> answer unless pgpool runs a full SQL parser on every command that I
> execute and intercepts the ones that touch protocol parameters. That's
> bound to be expensive an unreliable -- consider something like SELECT
> current_setting('some_compression_guc') || ' ' |
> current_setting('some_other_guc') which isn't half as pathological as
> it first looks.

Totally agreed that we shouldn't rely on poolers parsing queries.

> I want to be able to know the state of my protocol
> parameters by calling libpq functions that answer my questions
> definitively based on libpq's own internal state. libpq itself *must*
> know what compression I'm using on my connection; the server's answer
> may be different.

I think that totally makes sense that libpq should be able to answer
those questions without contacting the server, and indeed some
introspection should be added for that. But being able to introspect
what the server thinks the setting is seems quite useful too. That
still doesn't solve the problem of poolers though. How about
introducing a new ParameterGet message type too (matching the proposed
ParameterSet), so that poolers can easily parse and intercept that
message type.

> 2. Clarity of meaning across versions.
> ...
> As I see it, in your proposal, the
> client thinks they're just setting a GUC, but the server thinks we're
> completely changing up the wire protocol. Disaster ensues. From my
> point of view, the problem is created by the fact that you're mixing
> together two things which ought to be kept well-separated -- the act
> of negotiating what protocol variant we're using, on the one hand, and
> the setting of particular GUCs to particular values, on the other.

I totally agree that this is a problem that needs to be fixed. But I
feel like it is fixed by my patchset, due to two things:
1. By requiring the _pq_ prefix for GUCs that change the wire protocol
2. By using PGC_PROTOCOL to indicate that those GUCs can only be
changed using ParameterSet

This is the exact way how I protect the new \parameterset meta-command
in psql from patch 0009

+ if (strncmp("_pq_.", pset.parameterset_args[0], 5) == 0)
+ {
+ pg_log_error("\\parameterset cannot be used to change protocol
extensions parameters");
+ goto error;
+ }

> 3. Generally, and maybe this is just an expansion of the previous
> point, it feels to me like you've conflated the thing you want to do
> right now with what everybody who wants to modify the protocol will
> ever want to do in the future. It's just all GUCs, all the time! But
> the GUC model is actually a poor fit in all kinds of scenarios, which
> is why we have all kinds of other ways to configure things too, like
> connection parameters for instance. Now, to be fair, it's often useful
> to expose values that are configured through some other means as
> read-only GUCs, so the dividing line between GUCs and other things
> does get a bit sloppy.

I don't understand this argument at all. You mention specifically
connection parameters as an argument against using GUCs, but
connection parameters are also GUCs. They are GUCs with the
PGC_BACKEND GucContext. Adding a PGC_PROTOCOL GucContext like Tom
suggested is a really natural and well working extension to the
existing GUC system imho.

> rather, it's evidence of the need to
> make the distinction between the two as crisp as we possibly can.

Prefixing such options with _pq_ is my suggestion of making that
difference very crisp.