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

On Fri, 29 Dec 2023 at 19:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Jelte Fennema-Nio <me@jeltef.nl> writes:
> > 1. Protocol messages are much easier to inspect for connection poolers
> > than queries
>
> Unless you somehow forbid clients from setting GUCs in the old way,
> exactly how will that help a pooler?

A co-operating client could choose to only use this new message type
to edit GUCs such as search_path or pgbouncer.sharding_key using this
method. That's already a big improvement over the status quo, where
PgBouncer (and most other poolers) only understands GUC changes by
observing the ParameterStatus responses from Postgres. At which point
it is obviously too late to make any routing decisions, because to get
the ParamaterStatus back the pooler already needs to have forwarded
the SET command to an actual Postgres server. The same holds for any
setting changes that are specific to the pooler and postgres doesn't
even know about, such as pgbouncer.pool_mode

> > 3. Being able to change GUCs while in an aborted transaction.
>
> I'm really dubious that that's sane.  How will it interact with, for
> example, changes to the same GUC done in the now-failed transaction?
> Or for that matter, changes that happen later in the current
> transaction?  It seems like you can't even think about this unless
> you deem GUC changes made this way to be non-transactional, which
> seems very wart-y and full of consistency problems.

I think that's a fair criticism of the current patch. I do think it's
quite useful for drivers/poolers not to have to worry about their
changes to protocol settings being rolled back unexpectedly because
they made the change while the client was doing a transaction.
Particularly because it's easy for poolers to detect when a Sync is
sent without parsing queries, but not when a BEGIN is sent (PgBouncer
uses the ReadyForQuery response from the server to detect if a
transaction is open or not). But I agree that this behaviour is
fraught with problems for any non-protocol level settings.

I feel like a reasonable solution would be to make this ParameterSet
message transactional after all, but explicitly define the relevant
protocol-only GUCs to be non-transactional.

> I agree that GUC_LIST_QUOTE is a mess, but "I'm too lazy to deal
> with that" seems like a rather poor argument for instead expending
> the amount of labor involved in a protocol change.

Fair enough, honestly this is more of a bonus benefit to me.

On Fri, 29 Dec 2023 at 20:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, there's definitely an issue about what level of the client-side
> software ought to be able to set such parameters.  I'm not sure that
> "only the lowest level" is the correct answer though.  As an example,
> libpq doesn't especially care what encoding it's dealing with, nor
> (AFAIR) whether COPY data is text or binary.  The calling application
> probably cares, but then we end up needing a bunch of new plumbing to
> pass the settings through.  That's not really providing a lot of
> value-add IMO.

The value-add that I think it provides is forcing the user to use a
well defined interface when requesting behavioral changes to the
protocol. A client/pooler probably still wants to allow a user to
change these protocol level settings, but it wants to exert some
control when that happens. Clients could do so by exposing a basic
wrapper around PQparameterSet, and registering that they should parse
responses from postgres differently now. And poolers could do this by
understanding the message, and taking a relevant action (such as
enabling/disabling compression on outgoing messages). By having a well
defined interface, clients and poolers know when these protocol
related settings are being changed and can possibly even slightly
change the value before sending it to the server (e.g. by adding a few
extra GUCs to the list of GUCs that should be GUC_REPORTed because
PgBouncer wants to track them).

Achieving the same without this new ParameterSet message and
PQparameterSet might seem possible too, but then all clients and
poolers would need to implement a partial (and probably buggy) SQL
parser to check if the query that is being sent is "SET
binary_protocol = ...". And even this would not really be enough,
since a function would be able to change the relevant GUC without the
client ever sending SET ...

So, to summarize: Every piece of software in between the user writing
queries and postgres sending responses has some expectation and
requirements on what stuff looks like at the protocol level. Requiring
those intermediaries to look at the application layer (i.e. the actual
queries) to understand what to expect at the protocol layer is a
layering violation. Thus having a generic way to change protocol level
configuration options at the actual protocol level is needed to ensure
layer separation.