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@jeltef.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]:
/messages/by-id/CA+TgmoZyAh+hdN8zvHeN40n9vTstw8K1KjuWdgDuAMMbFAZqHg@mail.gmail.com
[2]:
/messages/by-id/CAFj8pRBFU-WzzQhNrwRHn67N0Ug8a9-0-9BOo69PPtcHiBDQMA@mail.gmail.com
[3]:
/messages/by-id/AB607155-8FED-4C8C-B702-205B33884CBB@yandex-team.ru
[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

On Fri, 2023-12-29 at 18:29 +0100, Jelte Fennema-Nio wrote:

2. It paves the way for GUCs that can only be set using a protocol
message (and not using SET).

That sounds useful for GUCs that can interfere with the client, such as
client_encoding or the proposed GUC in you referred to at [1].

There's definitely still some more work that needs to be done
(docs for new libpq APIs, protocol version bump, working protocol
version negotiation).

That is my biggest concern right now: what will new clients connecting
to old servers do?

If the version is bumped, should we look around for other unrelated
protocol changes to make at the same time? Do we want a more generic
form of negotiation?

Regards,
Jeff Davis

Jelte Fennema-Nio <me@jeltef.nl> writes:

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

Unless you somehow forbid clients from setting GUCs in the old way,
exactly how will that help a pooler?

2. It paves the way for GUCs that can only be set using a protocol
message (and not using SET).

This is assuming facts not in evidence. Why would we want such a thing?

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.

4. Have an easy way to use the value contained in a ParameterStatus
message as the value for a GUC

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.

On the whole, this feels like you are trying to force some things
into the GUC model that should not be there. I do perceive that
there are things that could be protocol-level variables, but
trying to say they are a kind of GUC seems like it will create
more problems than it solves.

regards, tom lane

On Fri, 2023-12-29 at 13:38 -0500, Tom Lane wrote:

2. It paves the way for GUCs that can only be set using a protocol
message (and not using SET).

This is assuming facts not in evidence.  Why would we want such a
thing?

The problem came up during the binary_formats GUC discussion: it
doesn't really make sense to change that with a SQL query, and doing so
can cause strange things to happen.

We already have the issue with client_encoding and binary format COPY,
so arguably it's not worth trying to solve it. But protocol-only GUCs
was one idea that came up.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

On Fri, 2023-12-29 at 13:38 -0500, Tom Lane wrote:

This is assuming facts not in evidence.  Why would we want such a
thing?

The problem came up during the binary_formats GUC discussion: it
doesn't really make sense to change that with a SQL query, and doing so
can cause strange things to happen.
We already have the issue with client_encoding and binary format COPY,
so arguably it's not worth trying to solve it. But protocol-only GUCs
was one idea that came up.

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.

regards, tom lane

On Fri, 29 Dec 2023 at 19:32, Jeff Davis <pgsql@j-davis.com> wrote:

On Fri, 2023-12-29 at 18:29 +0100, Jelte Fennema-Nio wrote:

There's definitely still some more work that needs to be done
(docs for new libpq APIs, protocol version bump, working protocol
version negotiation).

That is my biggest concern right now: what will new clients connecting
to old servers do?

If the version is bumped, should we look around for other unrelated
protocol changes to make at the same time? Do we want a more generic
form of negotiation?

This is not that big of a deal. Since it's only an addition of a new
message type, it's completely backwards compatible with the current
protocol version. i.e. as long as a client just doesn't send it when
the server reports an older protocol version everything works fine.
The protocol already has version negotiation built in and the server
implements it in a reasonable way. The only problem is that no-one
bothered to implement the client side of it in libpq, because it
wasn't necessary yet since 3.x only had a single minor version.

Patch v20230911-0003[5]/messages/by-id/attachment/150192/v20230911-0003-allow-to-connect-to-server-with-major-protocol-versi.patch from thread [3]/messages/by-id/AB607155-8FED-4C8C-B702-205B33884CBB@yandex-team.ru implements client side
handling in (imho) a sane way. I think it probably still needs some
small tweaks and discussion on if this is the exact user facing API
that we want. But there's no big hurdles implementation or protocol
wise to make the next libpq client backwards compatible with old
servers. I think it's worth merging something like that patch anyway,
because that's necessary for pretty much any protocol changes we would
like to do. After that there's pretty much no downside to bumping
minor versions of the protocol anymore, so we could even do it every
release if needed. Thus I don't think it's necessary to bulk any
protocol changes together.

[3]: /messages/by-id/AB607155-8FED-4C8C-B702-205B33884CBB@yandex-team.ru
[5]: /messages/by-id/attachment/150192/v20230911-0003-allow-to-connect-to-server-with-major-protocol-versi.patch

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.

On Fri, 29 Dec 2023 at 19:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:

2. It paves the way for GUCs that can only be set using a protocol
message (and not using SET).

This is assuming facts not in evidence.

How about instead of talking about protocol-only GUCs (which are
indeed currently a theoretical concept), we start considering this
patch as a way to modify parameters for protocol extensions. Protocol
extension parameters (starting with _pq_.) can currently only be
configured using the StartupMessage and afterwards there is no way to
modify them.

I believe [1], [2] and [3] from my initial email could all use such
protocol extension parameters, if those parameters could be changed
after the initial startup.

How about instead of talking about protocol-only GUCs (which are
indeed currently a theoretical concept), we start considering this
patch as a way to modify parameters for protocol extensions. Protocol
extension parameters (starting with _pq_.) can currently only be
configured using the StartupMessage and afterwards there is no way to
modify them.

I believe [1], [2] and [3] from my initial email could all use such
protocol extension parameters, if those parameters could be changed
after the initial startup.

What if we allowed the client to send `ParameterStatus` messages to
the server to reconfigure protocol extensions that were requested at
startup? Such processing would be independent of the transaction
lifecycle since protocol-level options aren't related to transactions.
Any errors in the set value would be handled with an `ErrorResponse`
(including if an extension was not reconfigurable after connection
startup), and success with a new `ReadyForQuery` message. The actual
effect of an extension change must be delayed until after the
ReadyForQuery has been transmitted, though I don't know if that is
feasible or if we would need to instead implicitly assume changes were
successful and just close the connection on error. We wouldn't need
to bump the protocol version since a client wouldn't (shouldn't) send
these messages unless it successfully negotiated the relevant protocol
extension(s) at startup.

On Sat, 30 Dec 2023 at 16:05, Jacob Burroughs
<jburroughs@instructure.com> wrote:

What if we allowed the client to send `ParameterStatus` messages to
the server to reconfigure protocol extensions that were requested at
startup?

Sending ParameterStatus from the frontend is not possible, because
Sync already claimed the 'S' message type on the frontend side. (and
even if that weren't the case I think the name ParameterStatus is not
descriptive of what the message would do when you'd send it from the
frontend.

Such processing would be independent of the transaction
lifecycle since protocol-level options aren't related to transactions.
Any errors in the set value would be handled with an `ErrorResponse`
(including if an extension was not reconfigurable after connection
startup), and success with a new `ReadyForQuery` message.

If we only consider modifying protocol extension parameters with
ParameterSet, then I think I like the idea of reusing ReadyForQuery
instead of introducing ParamaterSetComplete. I like that it implicitly
makes clear that you should not send ParameterStatus while you already
have an open pipeline for protocol extension parameters. If we go this
approach, we should probably explicitly disallow sending ParameterSet
while a pipeline.

However, if we also allow using ParameterSet to change regular runtime
parameters, then I think this approach makes less sense. Because then
you might want to batch regular runtime parameter together with other
actions in a pipeline, and use the expected "ignore everything after
the first error"

The actual
effect of an extension change must be delayed until after the
ReadyForQuery has been transmitted, though I don't know if that is
feasible or if we would need to instead implicitly assume changes were
successful and just close the connection on error.

I'm trying to think about how a client would want to handle a failure
when changing a protocol extension. Is there really an action it can
reasonably take at that point? I guess it might depend on the exact
extension, but I do think that in many cases closing the connection is
the only reasonable response. Maybe the server should even close the
connection with a FATAL error when it receives a ParameterSet for a
protocol extension but it fails to apply it.

We wouldn't need
to bump the protocol version since a client wouldn't (shouldn't) send
these messages unless it successfully negotiated the relevant protocol
extension(s) at startup.

I think I'd still prefer to bump the minor version, even if it's just
so that we've done it for the first time and we fixed all the libpq
issues with it. But I also think it makes sense from a versioning
perspective, if there's new message types that can be sent by the
client, which do not correspond to a protocol extension, then I think
the only reasonable thing is to update the version number. Otherwise
you'd basically need to define the ParameterSet message to be a part
of every new protocol extension that you would add. That seems more
confusing than saying that version 3.1 supports this new ParameterSet
message type.

On Sat, 30 Dec 2023 at 00:07, Jelte Fennema-Nio <me@jeltef.nl> wrote:

On Fri, 29 Dec 2023 at 19:32, Jeff Davis <pgsql@j-davis.com> wrote:

That is my biggest concern right now: what will new clients connecting
to old servers do?

This is not that big of a deal. Since it's only an addition of a new
message type, it's completely backwards compatible with the current
protocol version. i.e. as long as a client just doesn't send it when
the server reports an older protocol version everything works fine.
The protocol already has version negotiation built in and the server
implements it in a reasonable way. The only problem is that no-one
bothered to implement the client side of it in libpq, because it
wasn't necessary yet since 3.x only had a single minor version.

Patch v20230911-0003[5] from thread [3] implements client side
handling in (imho) a sane way.

Okay I updated this patchset to start with better handling of the
NegotiateProtocolVersion packet. The implementation is quite different
from [5] after all, but the core idea is the same. It also allows the
connection to continue to work in case of a missing protocol
extension, which is necessary for the libpq compression patchset[6].
In case the protocol extension is a requirement, the client can still
choose to disconnect by checking the return value of the newly added
PQunsupportedProtocolExtensions function.

I also fixed the tests of my final patch, but haven't changed the
behaviour of it in any of the suggested ways.

[3]: /messages/by-id/AB607155-8FED-4C8C-B702-205B33884CBB@yandex-team.ru

On Fri, Dec 29, 2023 at 1:38 PM 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?

I agree that for this to work out we need the things that you can set
this way to be able to be set in only this way. But I'm also a huge
fan of the idea -- if done correctly, it would solve the problem of an
end client sneaking SET ROLE or SET SESSION AUTHORIZATION past the
pooler, which is a huge issue that we really ought to address.

2. It paves the way for GUCs that can only be set using a protocol
message (and not using SET).

This is assuming facts not in evidence. Why would we want such a thing?

See above.

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 agree with these complaints.

4. Have an easy way to use the value contained in a ParameterStatus
message as the value for a GUC

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.

Not sure about this one. It seems unwarranted to introduce an
accusation of laziness when someone is finally making the effort to
address what is IMV a pretty serious deficiency in our current
implementation, but I have no educated opinion about what if anything
ought to be done about GUC_LIST_QUOTE or how that relates to the
present effort.

On the whole, this feels like you are trying to force some things
into the GUC model that should not be there. I do perceive that
there are things that could be protocol-level variables, but
trying to say they are a kind of GUC seems like it will create
more problems than it solves.

This is not a bad thought. If we made the things that were settable
with this mechanism distinct from the set of things that are settable
as GUCs, that might work out better. For example, it completely the
objection to (3). But I'm not 100% sure, either.

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

On Tue, 2 Jan 2024 at 18:51, Robert Haas <robertmhaas@gmail.com> wrote:

On the whole, this feels like you are trying to force some things
into the GUC model that should not be there. I do perceive that
there are things that could be protocol-level variables, but
trying to say they are a kind of GUC seems like it will create
more problems than it solves.

This is not a bad thought. If we made the things that were settable
with this mechanism distinct from the set of things that are settable
as GUCs, that might work out better. For example, it completely the
objection to (3). But I'm not 100% sure, either.

It seems like the general sentiment in the thread is that protocol
parameters are different enough from GUCs that they should be handled
separately. And I think I agree. Partially because of the
transactional reasons mentioned upthread, but also because allowing to
change defaults of protocol parameters in postgresql.conf seems like a
really bad idea. If the client does not specify the protocol
parameter, then they almost certainly want whatever value the default
has been for ages. Giving a DBA the ability to change that seems like
a recipe to accidentally break many clients.

It does cause some new questions for this patchset though:
- 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?
- If I create a debug protocol parameter, what designs should it
inherit from GUCs? e.g. parsing and input validation sounds useful.
And where should it be stored e.g. protocol_params.c?
- 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?

4. Have an easy way to use the value contained in a ParameterStatus
message as the value for a GUC

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.

Not sure about this one. It seems unwarranted to introduce an
accusation of laziness when someone is finally making the effort to
address what is IMV a pretty serious deficiency in our current
implementation, but I have no educated opinion about what if anything
ought to be done about GUC_LIST_QUOTE or how that relates to the
present effort.

To clarify, the main thing that I want to do is take the value from
ParameterStatus and somehow, without having to escape it, set that
value for that GUC for a different session. As explained above, I now
think that this newly proposed protocol message is a bad fit for this.
But I still think that that is not a weird thing to want.

The current situation is that you get a value in ParameterStatus, but
before it's useful you first need to do some escaping. And to know how
to escape it requires you to maintain a hardcoded list of GUCs that
are GUC_LIST_QUOTE (which might change in the next Postgres release).
I see two options to address this:

1. Add another protocol message that sets GUCs instead of protocol
parameters (which would behave just like SET, i.e. it would be
transactional)
2. Support preparing "SET search_path = $1" and then bind a single
string to it. i.e. have this PSQL command not fail with a syntax
error:

SET search_path = $1; \bind '"user", public';

ERROR: 42601: syntax error at or near "$1"
LINE 1: SET search_path = $1;

I'll take a look at implementing option 2, since I have a feeling
that's less likely to be controversial.

What if we created a new guc flag `GUC_PROTOCOL_EXTENSION` (or a
better name), used that for any of the GUCs options that should *only*
be set via a `ParameterSet` protocol message, and then prevent
changing those through SET/RESET/RESET ALL (but I don't see a reason
to prevent reading them through SHOW). (I would imagine most/all
`GUC_PROTOCOL_EXTENSION` GUCs would also set `GUC_NOT_IN_SAMPLE`,
`GUC_DISALLOW_IN_FILE`, and maybe `GUC_NO_SHOW_ALL`.). Looking back at
use cases in the original message of this thread, I would imagine at
least the "configurable GUC report" and "protocol compression" would
likely want to be flagged with `GUC_PROTOCOL_EXTENSION` since both
would seem like things that would require low-level client
involvement/support when they change.

I was already drafting this email before the last one in the thread
came through, but I think this proposal addresses a few things from
it:

- 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?

`protcocol_managed_params` would serve as the example/test parameter
in this patch series

- If I create a debug protocol parameter, what designs should it
inherit from GUCs? e.g. parsing and input validation sounds useful.
And where should it be stored e.g. protocol_params.c?

I'm thinking using flag(s) on GUCs would get useful mechanics without
needing to implement an entirely separate param system. It appears
there are existing flags that cover almost everything we would want
except for actually marking a GUC as a protocol extension.

- 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 think it would be reasonable for a client to track its own
ParameterSets if it has a reason to care about them, since presumably
it needs to do something differently after setting them anyways?
Though I could see an argument for sending `ParameterStatus` if we
want that to serve as an "ack" so a client could wait until it had
active confirmation from a server that a new parameter value was
applied when necessary.

To clarify, the main thing that I want to do is take the value from
ParameterStatus and somehow, without having to escape it, set that
value for that GUC for a different session. As explained above, I now
think that this newly proposed protocol message is a bad fit for this.
But I still think that that is not a weird thing to want.

Now, for the possibly nutty part of my proposal, what if we added a
GUC string list named `protcocol_managed_params` that had the
`GUC_PROTOCOL_EXTENSION` flag set, which would be a list of GUC names
to treat as if `GUC_PROTOCOL_EXTENSION` is set on them within the
context of the session. If a client wanted to use `ParameterSet` to
manage a non-`GUC_PROTOCOL_EXTENSION` managed parameter, it would
first issue a `ParameterSet` to add the parameter to the
`protcocol_managed_params` list, and then issue a `ParameterSet` to
actually set the parameter. If for some reason (e.g. some pgbouncer
use cases) the client then wanted the parameter to be settable
"normally" it would issue a third `ParameterSet` to remove the
parameter from `protcocol_managed_params`. Regarding transactional
semantics, I *think* it would be reasonable to specify that changes
made through `ParameterSet` are not transactional and that
`protcocol_managed_params` itself cannot be changed while a
transaction is active. That way within a given transaction a given
parameter has *only* transactional semantics or has *only*
nontransactional semantics, which I think avoids the potential
consistency problems? I think this would both address the pgbouncer
use case where it wants to be able to reflect a ParameterStatus
message back to the server while being agnostic to its contents while
also addressing the "SET ROLE"/"SET SESSION AUTHORIZATION" issue: a
pooler would just add `session_authorization` to the
`protcocol_managed_params` list and then it would have full control
over the user by not passing along `ParameterSet` messages setting the
user from the client. (I think it would generally be reasonable to
still allow setting the role within the restricted
session_authorization role, but that would be a pooler decision not a
PG one.)

On Wed, 3 Jan 2024 at 00:43, Jacob Burroughs <jburroughs@instructure.com> wrote:

What if we...

Great suggestions! Attached is a v3 version of the patchset that
implements all of this, including documentation.

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

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

Robert Haas <robertmhaas@gmail.com> writes:

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.

I'd not been following along for the last few days, but I agree that
we don't want to make it apply to any GUC at all.

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.

There is a lot of infrastructure we'll have to re-invent if
we make this completely independent of GUCs, notably:
* a way to establish the initial/default value
* a way to display the active value

So my thought was that this should be implemented as an (unchangeable)
flag bit for a GUC variable, GUC_PROTOCOL_ONLY or something like that,
and then we would refuse SQL-based set attempts on that. The behavior
would end up being very much like PGC_BACKEND variables, in that we
could allow all the existing setting methods to work to establish
a session's initial value; but after that, it can only change within
that session via a protocol message from the client. With that
rule, it's okay for the protocol message to be nontransactional since
there's no interaction with transactions.

regards, tom lane

On Fri, Jan 5, 2024 at 11:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

There is a lot of infrastructure we'll have to re-invent if
we make this completely independent of GUCs, notably:
* a way to establish the initial/default value
* a way to display the active value

So my thought was that this should be implemented as an (unchangeable)
flag bit for a GUC variable, GUC_PROTOCOL_ONLY or something like that,
and then we would refuse SQL-based set attempts on that. The behavior
would end up being very much like PGC_BACKEND variables, in that we
could allow all the existing setting methods to work to establish
a session's initial value; but after that, it can only change within
that session via a protocol message from the client. With that
rule, it's okay for the protocol message to be nontransactional since
there's no interaction with transactions.

Maybe, but it seems like it might be complicated to make that work
with the existing GUC code. GUCs are fundamentally transactional, I
think.

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