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

Hi

pá 29. 12. 2023 v 18:29 odesílatel Jelte Fennema-Nio <me(at)jeltef(dot)nl> napsal:

> Currently the only way to set GUCs from a client is by using SET
> commands or set them in the StartupMessage. I think it would be very
> useful to be able to change settings using a message at the protocol
> level. For the following reasons:
>
> 1. Protocol messages are much easier to inspect for connection poolers
> than queries
> 2. It paves the way for GUCs that can only be set using a protocol
> message (and not using SET).
> 3. Being able to change GUCs while in an aborted transaction.
> 4. Have an easy way to use the value contained in a ParameterStatus
> message as the value for a GUC
>
> I attached a patch that adds a new protocol message to set a GUC
> value. There's definitely still some more work that needs to be done
> (docs for new libpq APIs, protocol version bump, working protocol
> version negotiation). But the core functionality is working. At this
> point I'd mainly like some feedback on the general idea.
>
> The sections below go into more detail on each of the reasons I mentioned
> above:
>
> (1) PgBouncer does not inspect query strings, to avoid having to
> write/maintain a SQL parser (even a partial one). But that means that
> clients cannot configure any behaviour of PgBouncer for their session.
> A few examples of useful things to configure would be:
> a. allow changing between session and transaction pooling on the same
> connection.
> b. intercepting changes to search_path, and routing different schemas
> to different machines (useful for Citus its schema based sharding).
> c. intercepting changing of pgbouncer.sharding_key, and route to
> different machines based on this value.
>
> (2) There are currently multiple threads ongoing that propose very
> similar protocol changes for very different purposes. Essentially all
> of them boil down to sending a protocol message to the server to
> change some other protocol behaviour. And the reason why they cannot
> use GUCs, is because the driver and/or connection pooler need to know
> what the setting is and be able to choose it without a user running
> some SQL suddenly changing the value. The threads I'm talking about
> are: Choosing specific types that use binary format for encoding [1].
> Changing what GUCs are reported to the client using ParameterStatus
> (a.k.a configurable GUC_REPORT) [2]. Changing the compression method
> that is used to compress messages[3].
>
> Another benefit could be to allow a connection pooler to configure
> certain settings to not be changeable with SQL. For instance if a
> pooler could ensure that a client couldn't later change
> session_authorization, it could use session_authorization to set the
> user and then multiplex client connections from different users over
> the same connection to the database.
>
> (3) For psql it's useful to be able to control what messages it gets a
> ParameterStatus for, even when the transaction is in aborted state.
> Because that way it could decide what parameters status updates to
> request based on the prompt it needs to display. And the prompt can be
> changed even during an aborted transaction.
>
> (4) PgBouncer uses the value contained in the ParameterStatus message
> to correctly set some GUCs back to their expected value. But to do
> this you currently need to use a SET query, which in turn requires
> quoting the value using SQL quoting . This wouldn't be so bad, except
> that GUC_LIST_QUOTE exists. Parameters with GUC_LIST_QUOTE have each
> item in the list returned **double** quoted, and then those double
> quoted items separated by commas. But to be able to correctly set
> them, they need to be given each separately **single** quoted and
> separated by commas. Doing that would require a lot of parsing logic
> to replace double quotes with single quotes correctly. For now
> pgbouncer only handles the empty string case correctly, for the
> situations where the difference between double and single quotes
> matters[4].
>
> [1]:
> https://www.postgresql.org/message-id/flat/CA%2BTgmoZyAh%2BhdN8zvHeN40n9vTstw8K1KjuWdgDuAMMbFAZqHg%40mail.gmail.com#e3a603bbc091e796148a2d660a4a1c1f
> [2]:
> https://www.postgresql.org/message-id/flat/CAFj8pRBFU-WzzQhNrwRHn67N0Ug8a9-0-9BOo69PPtcHiBDQMA(at)mail(dot)gmail(dot)com
> [3]:
> https://www.postgresql.org/message-id/flat/AB607155-8FED-4C8C-B702-205B33884CBB%40yandex-team.ru#961c695d190cdccb3975a157b22ce9d8
> [4]:
> https://github.com/pgbouncer/pgbouncer/blob/fb468025d61e1ffdc6dbc819558f45414e0a176e/src/varcache.c#L172-L183
>
> P.S. I included authors and some reviewers of the threads I mentioned
> for 2 in the CC. Since this patch is meant to be a generic protocol
> change that could be used by all of them.
>

+1

Pavel