Обсуждение: Add new protocol message to change GUCs for usage with future protocol-only GUCs
Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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@mail.gmail.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.
Вложения
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Pavel Stehule
Дата:
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]: 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@mail.gmail.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
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
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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] from thread [3] 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]: https://www.postgresql.org/message-id/flat/AB607155-8FED-4C8C-B702-205B33884CBB%40yandex-team.ru#961c695d190cdccb3975a157b22ce9d8 [5]: https://www.postgresql.org/message-id/attachment/150192/v20230911-0003-allow-to-connect-to-server-with-major-protocol-versi.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Burroughs
Дата:
> 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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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]: https://www.postgresql.org/message-id/flat/AB607155-8FED-4C8C-B702-205B33884CBB%40yandex-team.ru#961c695d190cdccb3975a157b22ce9d8
Вложения
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
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
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Burroughs
Дата:
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.)
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Вложения
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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
Вложения
- v4-0005-Bump-protocol-version-to-3.1.patch
- v4-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v4-0004-Prepare-server-code-for-addition-of-protocol-exte.patch
- v4-0003-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v4-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v4-0006-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v4-0007-Add-protcol-message-to-change-protocol-extension-.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
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
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
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
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Jan 2024 at 18:08, Robert Haas <robertmhaas@gmail.com> wrote: > 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. They are not fundamentally transactional afaict based on the changes that were needed so far. It makes sense too, because e.g. SIGHUP should change the GUC value if the config changed no matter if the current transaction aborts or succeeds. Based on my experience when writing the current set of patches I think the GUC infrastructure fits quite well with protocol extension parameters. When you add a new flag bit it feels very natural (whatever you may call this flag GUC_PROTOCOL_ONLY, GUC_PROTOCOL_EXTENSION, or something else).
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, Jan 5, 2024 at 12:20 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > They are not fundamentally transactional afaict based on the changes > that were needed so far. It makes sense too, because e.g. SIGHUP > should change the GUC value if the config changed no matter if the > current transaction aborts or succeeds. Well, AtEOXact_GUC either reverts or puts back changes to GUC values that have happened in that (sub)transaction, depending on whether the (sub)transaction committed or aborted. To make that work, there's a "stack" of GUC values for any given setting. For a non-transactional value, we wouldn't have all that infrastructure... -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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. 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. While I agree that it's not strictly necessary, I also feel that you think the a minor protocol version bump a much bigger deal than it is (afaict). The protocol is designed in such a way that bumping the minor version can be done without any problems. There is no possibility of breaking older clients, because the server will silently downgrade to the version that the client asks for. I would be okay not doing the actual bump, but I think at least having the infrastructure in libpq to support a future bump would be quite useful (i.e. patch 0002 and 0003). > > - 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. Based on my short experience writing these patches, even for testing/debugging it's quite useful to be able to do SHOW _pq_.some_protocol_parameter I think that's a major benefit of re-using the GUC system.
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, Jan 5, 2024 at 11:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> 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. I think it'd be quite simple. As I said, it's just a small variation on how some GUCs already work. The only thing that's really transactional is SQL-driven updates, which'd be disallowed for this class of variables. (After consuming a little more caffeine, I wonder if the class ought to be defined by a new PGC_XXX context value, rather than a flag bit.) regards, tom lane
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, Jan 5, 2024 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think it'd be quite simple. As I said, it's just a small variation > on how some GUCs already work. The only thing that's really > transactional is SQL-driven updates, which'd be disallowed for this > class of variables. Well, I know better than to tell you something is hard if you think it's easy. :-) -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
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.
Вложения
- v5-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v5-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v5-0005-Bump-protocol-version-to-3.1.patch
- v5-0004-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v5-0003-Prepare-server-code-for-addition-of-protocol-exte.patch
- v5-0006-Add-protocol-message-to-change-parameters.patch
- v5-0009-Add-tests-for-ParameterSet-and-_pq_.protocol_mana.patch
- v5-0008-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v5-0007-Add-GUC-contexts-for-protocol-extensions.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
v6 attached with the following changes: 1. Fixed rebase conflicts with master 2. removed PGC_S_PROTOCOL (but kept PGC_PROTOCOL and PGC_SU_PROTOCOL). This extra source level was not needed. And after some more testing I realized this extra source level even caused problems, since protocol messages could not override values set by SET commands anymore. 3. Added a new patch (0010) with a protocol parameter to configure which GUCs are GUC_REPORT. This is partially to show that the GUC interface makes sense for protocol parameters, but also because this would be an extremely useful feature for connection poolers. And [2] would be able to use this too. 4. Don't error, but only warn, if a GUC provided to _pq_.protocol_managed_params is unknown. It seemed useful to be able to specify GUCs in this list that not all Postgres versions support in the StartupMessage, without having to guess what postgres version you're going to connect to. [2]: https://www.postgresql.org/message-id/flat/CAFj8pRBFU-WzzQhNrwRHn67N0Ug8a9-0-9BOo69PPtcHiBDQMA@mail.gmail.com
Вложения
- v6-0003-Prepare-server-code-for-addition-of-protocol-exte.patch
- v6-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v6-0004-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v6-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v6-0005-Bump-protocol-version-to-3.1.patch
- v6-0006-Add-protocol-message-to-change-parameters.patch
- v6-0008-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v6-0009-Add-tests-for-ParameterSet-and-_pq_.protocol_mana.patch
- v6-0010-Add-_pq_.report_parameters-protocol-extension.patch
- v6-0007-Add-GUC-contexts-for-protocol-extensions.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Mon, Jan 8, 2024 at 5:52 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > 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. Could you explain why you think that the protocol version bump is necessary? > > 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". I don't know whether that limit helps anything or not, and you haven't really said why you imposed it. Personally, I think that the login role should be changeable via a protocol message and make it just as if we'd logged in using the selected role originally, except that a further protocol message can change it again (when not in transaction). SET ROLE and SET SESSION AUTHORIZATION would behave in accordance with the idea that it was the originally selected role. Then, a connected client can't distinguish between being directly connected to the database in a session created for that role and being connected to a pooler which has used this protocol message to create a session that is effectively for that role. With your design, the client can see behavioral differences between those cases. I agree that client_encoding feels like a protocol parameter rather than a GUC as we know them today. How to get there with reasonable impact on backward compatibility, I'm not sure. I'm still afraid that trying to allow this kind of nail-down for a broad range of GUCs (even if not all) is going to be messy. But I'm also plenty willing to listen to contrary opinions. I hear yours, but I wonder what others think? Tom particularly. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 10 Jan 2024 at 22:12, Robert Haas <robertmhaas@gmail.com> wrote: > Could you explain why you think that the protocol version bump is necessary? Patch 0006 adds a new protocol message to the protocol. Somehow the client needs to be told that the server understands that message. Using the protocol version seems like the best/simplest/cleanest way to do that to me. In theory we could add a dedicated protocol extension parameter (e.g. _pq_.enable_protocol_level_set) that the client would need to set to true before it would be able to use ParameterSet. But that just sounds like introducing unnecessary complexity to me. Bumping the protocol version carries exactly the same level of risk as adding new protocol extension parameters. Both will always allow old clients to connect to the newer server. And both also allow a new client to connect to an old server just fine as well, as long as that server has ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed (which was introduced in PG11.0 and was backpatched to all then supported PG versions). > > 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". > > I don't know whether that limit helps anything or not, and you haven't > really said why you imposed it. The main reason I did this is to make sure that the required context can only be hardened, not softened. e.g. it would be very bad if PGC_POSTMASTER GUCs could suddenly be changed with a protocol message. So it was more meant as fixing a bug, than really reducing the number of GUCs this has an impact on significantly. > I'm still afraid that > trying to allow this kind of nail-down for a broad range of GUCs (even > if not all) is going to be messy. But I'm also plenty willing to > listen to contrary opinions. I hear yours, but I wonder what others > think? Tom particularly. I wouldn't mind heavily reducing the GUCs that can be nailed-down like this. For my usecase (connection pooling) I only really care about it being possible to nail-down session_authorization. Honestly, I care more about patch 0010 than patch 0008. Patch 0008 simply seemed like the easiest way to demonstrate the ParameterSet message.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 10 Jan 2024 at 23:53, Jelte Fennema-Nio <me@jeltef.nl> wrote: > Honestly, I care more about patch 0010 than patch 0008. Patch 0008 > simply seemed like the easiest way to demonstrate the ParameterSet > message. So to be clear, if you consider 0008 the most controversial/risky part of this patchset (which it sounds like that's the case). I'd be fine with removing that for now. IMHO the first 7 patches would be very useful on their own, because they unblock any other patches that want to introduce protocol changes (examples of those are 0008 and 0010). Do you think that is a good idea? I could fairly easily modify the tests in 0009 to remove any things related to 0008.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 11 Jan 2024 at 17:20, Jelte Fennema-Nio <me@jeltef.nl> wrote: > So to be clear, if you consider 0008 the most controversial/risky part > of this patchset (which it sounds like that's the case). I'd be fine > with removing that for now. I haven't removed 0008 yet, since I'd like some feedback first if that makes sense. But I did add two new patches in the middle of the patchset (which shift the later patch numbers by 2): 0007: Adds a new \parameterset meta-command to psql to be able to more easily test the new ParameterSet message 0008: Shows warning in psql if the server is not supporting the newest protocol version.
Вложения
- v7-0005-Bump-protocol-version-to-3.1.patch
- v7-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v7-0003-Prepare-server-code-for-addition-of-protocol-exte.patch
- v7-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v7-0004-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v7-0008-psql-Warn-about-unsupported-protocol-features.patch
- v7-0006-Add-protocol-message-to-change-parameters.patch
- v7-0007-psql-Add-parameterset-meta-command.patch
- v7-0009-Add-GUC-contexts-for-protocol-extensions.patch
- v7-0010-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v7-0011-Add-tests-for-ParameterSet-and-_pq_.protocol_mana.patch
- v7-0012-Add-_pq_.report_parameters-protocol-extension.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Tue, Jan 16, 2024 at 8:43 AM Jelte Fennema-Nio <me@jeltef.nl> wrote: > I haven't removed 0008 yet, since I'd like some feedback first if that > makes sense. But I did add two new patches in the middle of the > patchset (which shift the later patch numbers by 2): > > 0007: Adds a new \parameterset meta-command to psql to be able to more > easily test the new ParameterSet message > > 0008: Shows warning in psql if the server is not supporting the newest > protocol version. Sorry for not getting back to this right away; there are quite a few threads competing for my attention. I think it's flat-out not viable to bump the protocol version to 3.1 any time in the next few years. NegotiateProtocolVerison has existed since ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed (2017-11-21, 11.0, 10.2, 9.6.7, 9.5.11, 9.4.16, 9.3.21), but libpq didn't handle it until bbf9c282ce92272ed7bf6771daf0f9efa209e61b (2022-11-17, 16.0) -- and even that handling doesn't really seem like what we want, because it looks like it will reject anything where the protocol version doesn't match exactly, rather than downgrading. To fix that, I think we need some parts of what you've got in 0002, where we have an earliest and a latest minor protocol version that we'll accept, and the server is allowed to downgrade from the latest thing we support, just as long as they don't try to downgrade below the earliest thing we support. But I think we would want to have those changes in all supported branches before we could think of requesting 3.1 or higher by default. Imagine that in v17 we both added server support for protocol version 3.1 and also adopted your 0001. Then, a v17 libpq would fail to connect to a v16 or earlier PostgreSQL instance. In effect, it would be a complete wire compatibility break. There's no way that such a patch is going to be acceptable. If I were to commit a patch from you or anyone else that does that, many angry people would show up to yell at both of us. So unless I'm misunderstanding the situation, 0001 is pretty much dead on arrival for now and for quite a while to come. That doesn't necessarily mean that we couldn't *optionally* request 3.1, e.g. controlled by a connection keyword. I would imagine that the user would write e.g. 'user=rhaas password=banana protocolroles=true' and libpq would say "oh, because the user wanted protocolroles=true I need to request at least 3.1" -- but if that weren't there, the server would still request only 3.0 and nothing would break. Also, I'm pretty doubtful that we want PQunsupportedProtocolExtensions(). That seems like allowing the client to have too much direct visibility into what's happening at the protocol level. That kind of interface might make sense if we were trying to support unknown protocol extensions from third parties, but for stuff in core, I think there should be specific APIs that relate to specific features e.g. you call PQsetWireProtocolRole(char *whatever) and it returns success or failure according to whether that capability is available without telling you how that's negotiated on the wire. So in summary, I think parts of 0002 are a good idea, but 0001 is not realistic. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Tue, 16 Jan 2024 at 21:36, Robert Haas <robertmhaas@gmail.com> wrote: > Sorry for not getting back to this right away; there are quite a few > threads competing for my attention. No worries, I know it's a busy time. > But I think we would want to have those changes in all supported > branches before we could think of requesting 3.1 or higher by default. > Imagine that in v17 we both added server support for protocol version > 3.1 and also adopted your 0001. Then, a v17 libpq would fail to > connect to a v16 or earlier PostgreSQL instance. In effect, it would > be a complete wire compatibility break. There's no way that such a > patch is going to be acceptable. If I were to commit a patch from you > or anyone else that does that, many angry people would show up to yell > at both of us. So unless I'm misunderstanding the situation, 0001 is > pretty much dead on arrival for now and for quite a while to come. It's understandable you're worried about breaking clients, but afaict **you are actually misunderstanding the situation**. I totally agree that we cannot bump the protocol version without also merging 0002 (that's why the version bump is in patch 0005 not patch 0001). But 0002 does **not** need to be backported to all supported branches. The only libpq versions that can ever receive a NegotiateVersionProtocol are ones that request 3.1, and since none of the pre-17 libpq versions ever request 3.1 there's no need for them to be able to handle NegotiateVersionProtocol. If you try out the patchset, you will see that you can connect with psql16 to postgres17 and psql17 to postgres16. Both without any problems. The only time when you will get problems is if you connect to a server from before these versions that you mentioned (2017-11-21, 11.0, 10.2, 9.6.7, 9.5.11, 9.4.16, 9.3.21) > Also, I'm pretty doubtful that we want > PQunsupportedProtocolExtensions(). I definitely think we should include this API. As a client author (and even user), you might want to know what features are supported by the server you are connected to. That way you can avoid calling functions that would otherwise fail. This is also the reason why PQprotocolVersion() and PQserverVersion() exist. IMHO PQunsupportedProtocolExtensions() is simply a natural addition to those already existing feature-discovery APIs. I'll move the addition of PQunsupportedProtocolExtensions() to a separate patch though, since I do agree that it's a separate item from the rest of 0002. > That seems like allowing the client > to have too much direct visibility into what's happening at the > protocol level. That kind of interface might make sense if we were > trying to support unknown protocol extensions from third parties, but > for stuff in core, I think there should be specific APIs that relate > to specific features e.g. you call PQsetWireProtocolRole(char > *whatever) and it returns success or failure according to whether that > capability is available without telling you how that's negotiated on > the wire. I think we have a very different idea of what is a desirable API for client authors that use libpq to build their clients. libpq its API is pretty low level, so I think it makes total sense for client authors to know what protocol extension parameters exist. It seems totally acceptable for me to have them call PQsetParameter directly: PQsetParameter("_pq_.protocol_roles", "true") PQsetParameter("_pq_.report_parameters", "role,search_path") Otherwise we need to introduce **two** new functions for every protocol extension that is going to be introduced, a blocking and a non-blocking one (e.g. PQsetWireProtocolRole() and PQsendSetWireProtocolRole()). And that seems like unnecessary API bloat to me. To be clear, I think it would probably make sense for client authors to expose functions like that for the users of the client. But I think libpq should not add an API surface that can easily be avoided (e.g. there's also no function to begin a transaction, even though pretty much every client exposes one).
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Tue, Jan 16, 2024 at 9:21 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > It's understandable you're worried about breaking clients, but afaict > **you are actually misunderstanding the situation**. I totally agree > that we cannot bump the protocol version without also merging 0002 > (that's why the version bump is in patch 0005 not patch 0001). But > 0002 does **not** need to be backported to all supported branches. The > only libpq versions that can ever receive a NegotiateVersionProtocol > are ones that request 3.1, and since none of the pre-17 libpq versions > ever request 3.1 there's no need for them to be able to handle > NegotiateVersionProtocol. OK, yeah, fuzzy thinking on my part. > I think we have a very different idea of what is a desirable API for > client authors that use libpq to build their clients. libpq its API is > pretty low level, so I think it makes total sense for client authors > to know what protocol extension parameters exist. It seems totally > acceptable for me to have them call PQsetParameter directly: > > PQsetParameter("_pq_.protocol_roles", "true") > PQsetParameter("_pq_.report_parameters", "role,search_path") > > Otherwise we need to introduce **two** new functions for every > protocol extension that is going to be introduced, a blocking and a > non-blocking one (e.g. PQsetWireProtocolRole() and > PQsendSetWireProtocolRole()). And that seems like unnecessary API > bloat to me. I think it's hard to say for sure what API is going to work well here, because we just don't have much experience with this. I do agree that we want to avoid API bloat. However, I also think that the reason why the API you're proposing here looks good in this case is because libpq itself doesn't really need to do anything differently for these parameters. It doesn't actually really change anything about the protocol; it only nails down the server behavior in a way that can't be changed. Another current request is to have a way to have certain data types always be sent in binary format, specified by OID. Do we want that to be written as PQsetParameter("always_binary_format", "123,456,789,101112") or do we want it to maybe look more like PQalwaysBinaryFormat(int count, Oid *stuff)? Or, another example, say we want to set client_to_server_compression_method=lz4. It's very difficult for me to believe that libpq should be doing strcmp() against the proposed protocol parameter settings and thus realizing that it needs to start compressing ... especially since there might be compression parameters (like level or degree of parallelism) that the client needs and the server doesn't. Also, I never intended for _pq_ to become a user-visible namespace that people would have to care about; that was just a convention that I adopted to differentiate setting a protocol parameter from setting a GUC. I think it's a mistake to make that string something users have to worry about directly. It wouldn't begin and end with an underscore if it were intended to be user-visible. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 17 Jan 2024 at 14:39, Robert Haas <robertmhaas@gmail.com> wrote: > I think it's hard to say for sure what API is going to work well here, > because we just don't have much experience with this. Agreed, but I strongly believe PQunsupportedProtocolExtensions() is useful regardless of the API choice. > I also think that the reason why > the API you're proposing here looks good in this case is because libpq > itself doesn't really need to do anything differently for these > parameters. It doesn't actually really change anything about the > protocol; it only nails down the server behavior in a way that can't > be changed. Another current request is to have a way to have certain > data types always be sent in binary format, specified by OID. Do we > want that to be written as PQsetParameter("always_binary_format", > "123,456,789,101112") or do we want it to maybe look more like > PQalwaysBinaryFormat(int count, Oid *stuff)? Or, another example, say > we want to set client_to_server_compression_method=lz4. I think from libpq's perspective there are two categories of protocol extension parameters: 1. parameters that change behaviour in a way that does not matter to libpq 2. parameters that change in such a way that libpq needs to change its behaviour too (by parsing or sending messages differently than it normally would). _pq_.protocol_roles, _pq_.report_parameters, and (afaict) even _pq_.always_binary_format would fall into category 1. But _pq_.client_to_server_compression_method would fall into category 2, because libpq should start to compress the messages that it is sending. I think if you look at it like that, then using PQsetParameter for parameters in category 1 makes sense. And indeed you'd likely want a dedicated API for each parameter in category 2, and probably have PQsetParameter error for these parameters. In any case it seems like something that can be decided on a case by case basis. However, to make this future proof, I think it might be best if PQsetParameter would error for protocol extension parameters that it does not know about. > Also, I never intended for _pq_ to become a user-visible namespace > that people would have to care about I agree that forcing Postgres users to learn about this prefix is probably unwanted. But requiring client authors to learn about the prefix seems acceptable to me.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Peter Smith
Дата:
2024-01 Commitfest. Hi, This patch has a CF status of "Needs Review", but it seems like there were some CFbot test failures last time it was run [1]. Please have a look and post an updated version if necessary. ====== [1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4736 Kind Regards, Peter Smith.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Mon, 22 Jan 2024 at 04:31, Peter Smith <smithpb2250@gmail.com> wrote: > Hi, This patch has a CF status of "Needs Review", but it seems like > there were some CFbot test failures last time it was run [1]. Please > have a look and post an updated version if necessary. Ah yes, I had noticed that a while back and fixed the problem in v7 of my patchset but it seems a rebase conflict has caused the cfbot not to run tests on that version at all. Attached is a rebased version together with following small changes: - Rename PQunsupportedProtocolExtensions to PQunsupportedProtocolExtensionParameters - Add PQunsupportedProtocolExtensionParameters in its own patch
Вложения
- v8-0005-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v8-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v8-0003-libpq-Add-PQunsupportedProtocolExtensionParameter.patch
- v8-0004-Prepare-server-code-for-addition-of-protocol-exte.patch
- v8-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v8-0006-Bump-protocol-version-to-3.1.patch
- v8-0008-psql-Add-parameterset-meta-command.patch
- v8-0007-Add-protocol-message-to-change-parameters.patch
- v8-0009-psql-Warn-about-unsupported-protocol-features.patch
- v8-0010-Add-GUC-contexts-for-protocol-extensions.patch
- v8-0011-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v8-0012-Add-tests-for-ParameterSet-and-_pq_.protocol_mana.patch
- v8-0013-Add-_pq_.report_parameters-protocol-extension.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
Fixed a rebase conflict.
Вложения
- v9-0005-libpq-Include-minor-version-number-in-PQprotocolV.patch
- v9-0002-libpq-Handle-NegotiateProtocolVersion-message-mor.patch
- v9-0003-libpq-Add-PQunsupportedProtocolExtensionParameter.patch
- v9-0004-Prepare-server-code-for-addition-of-protocol-exte.patch
- v9-0001-libpq-Remove-instance-of-hardcoded-protocol-versi.patch
- v9-0009-psql-Warn-about-unsupported-protocol-features.patch
- v9-0007-Add-protocol-message-to-change-parameters.patch
- v9-0008-psql-Add-parameterset-meta-command.patch
- v9-0006-Bump-protocol-version-to-3.1.patch
- v9-0010-Add-GUC-contexts-for-protocol-extensions.patch
- v9-0012-Add-tests-for-ParameterSet-and-_pq_.protocol_mana.patch
- v9-0011-Add-_pq_.protocol_managed_params-protocol-extensi.patch
- v9-0013-Add-_pq_.report_parameters-protocol-extension.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
Fixed some conflicts again. To summarize the current state of this patchset, it consists of 5 different parts that could be merged separately: 1. 0001-0005 are needed for any protocol change, and hopefully shouldn't require much discussion 2. 0006-0009 introduce a new protocol message that can be used to update GUCs. I think we might want some discussion on the design of the protocol message, but I think the behaviour has all feedback incorporated. 3. 0010 Adds GUC contexts for protocol extensions, in the way that Tom Lane suggested. 4. 0011 Adds a way to mark some GUCs as only changeable using protocol messages, there's still some discussion needed on this (I can remove this from the patchset if that makes it easier). 5. 0012 Adds some additional tests for some of the previous features 6. 0013 Add a protocol parameter to control which GUCs have GUC_REPORT. Quite straightforward imho.
Вложения
- v10-0005-libpq-Include-minor-version-number-in-PQprotocol.patch
- v10-0004-Prepare-server-code-for-addition-of-protocol-ext.patch
- v10-0003-libpq-Add-PQunsupportedProtocolExtensionParamete.patch
- v10-0001-libpq-Remove-instance-of-hardcoded-protocol-vers.patch
- v10-0002-libpq-Handle-NegotiateProtocolVersion-message-mo.patch
- v10-0007-Add-protocol-message-to-change-parameters.patch
- v10-0008-psql-Add-parameterset-meta-command.patch
- v10-0006-Bump-protocol-version-to-3.1.patch
- v10-0009-psql-Warn-about-unsupported-protocol-features.patch
- v10-0010-Add-GUC-contexts-for-protocol-extensions.patch
- v10-0012-Add-tests-for-ParameterSet-and-_pq_.protocol_man.patch
- v10-0013-Add-_pq_.report_parameters-protocol-extension.patch
- v10-0011-Add-_pq_.protocol_managed_params-protocol-extens.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
Fixed some conflicts again, as well as adding a connection option to choose the requested protocol version (as discussed in[1]). This new connection option is not useful when connecting to any of the supported postgres versions. But it can be useful when connecting to PG versions before 9.3. Or when connecting to connection poolers or other databases that implement the postgres protocol but do not support the NegotiateProtocolVersion message. [1]: https://www.postgresql.org/message-id/flat/CAGECzQRrHn52yEX%2BFc6A9uvVbwRCxjU82KNuBirwFU5HRrNxqA%40mail.gmail.com#835914cbd55c56b36e8e7691cb743a18
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 13 Mar 2024 at 19:18, Jelte Fennema-Nio <me@jeltef.nl> wrote: > > Fixed some conflicts again, as well as adding a connection option to > choose the requested protocol version now with attachments...
Вложения
- v11-0004-Prepare-server-code-for-addition-of-protocol-ext.patch
- v11-0001-libpq-Remove-instance-of-hardcoded-protocol-vers.patch
- v11-0003-libpq-Add-PQunsupportedProtocolExtensionParamete.patch
- v11-0002-libpq-Handle-NegotiateProtocolVersion-message-mo.patch
- v11-0005-libpq-Include-minor-version-number-in-PQprotocol.patch
- v11-0006-libpq-Add-max_protocol_version-connection-option.patch
- v11-0009-psql-Add-parameterset-meta-command.patch
- v11-0008-Add-protocol-message-to-change-parameters.patch
- v11-0011-Add-GUC-contexts-for-protocol-extensions.patch
- v11-0012-Add-_pq_.protocol_managed_params-protocol-extens.patch
- v11-0007-Bump-protocol-version-to-3.1.patch
- v11-0010-psql-Warn-about-unsupported-protocol-features.patch
- v11-0014-Add-_pq_.report_parameters-protocol-extension.patch
- v11-0013-Add-tests-for-ParameterSet-and-_pq_.protocol_man.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, Mar 8, 2024 at 6:47 AM Jelte Fennema-Nio <me@jeltef.nl> wrote: > 1. 0001-0005 are needed for any protocol change, and hopefully > shouldn't require much discussion I feel bad arguing about the patches that you think are a slam-dunk, but I find myself disagreeing with the design choices. Regarding 0001, I considered making this change as part of ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed but decided against it, because it seemed like it was making the assumption that we always wanted to initiate new connections with the latest protocol version that we know how to accept, and I wasn't sure that would always be true. I don't think it would be catastrophic if this got committed or anything -- it could always be changed later if the need arose -- but I wanted to mention that I had a reason for not doing it, and I'm still not particularly convinced that we should do it. I'm really unhappy with 0002-0004. Before ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed, any parameter included in the startup message that wasn't in a short, hard-coded list was treated as a request to set a GUC. That left no room for any other type of protocol modification, so that commit carved out an exception, where when we something that starts with _pq_., it's assumed to be setting some other kind of thing, not a GUC. But 0004 throws that out the window and decides, nope, those are just GUCs, too. Even if we don't have a specific reason today why we'd ever want such a thing, it seems short-sighted to give up on the possibility that in the future we will. Because if we implement what this patch wants to do in this way, basically consuming the entire namespace that ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed created in on shot, and then later we want the sort of thing that I'm postulating, we'll have to manufacture another new namespace for that need. And it seems to me that there are other ways we could do this. For example, suppose we introduced just one new protocol parameter; for the sake of argument, I'll call it _pq_.protocol_set. If the client sends this parameter and the server accepts it, then the client knows that the server supports a new protocol message ProtocolSetParameter, which is the only way to set GUCs in the new PROTOCOL_EXTENSION category. New libpq functions with names like, I don't know, PQsetProtocolParameter(), are added to send such messages, and they return an error if there are network problems or whatever, or if the server didn't accept the _pq_.protocol_set option at startup time. With this kind of design, you're just consuming one element of the _pq_ namespace, and the next person who wants to do something can consume one more element, and we'll be able to go on for a very long time without running out of room. This is really how I intended this mechanism to be used, and the only real downside I see as compared to what you've done is that you can't set the protocol GUCs in the startup packet, but must set them afterward via separate messages. If that's a problem, then the proposal I just outline needs modification ... but no matter what we do exactly, I don't want the very first protocol extension we ever add to eat up all of _pq_. I intended that to support decades worth of extensibility, not just one patch. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 14 Mar 2024 at 16:45, Robert Haas <robertmhaas@gmail.com> wrote: > I feel bad arguing about the patches that you think are a slam-dunk, > but I find myself disagreeing with the design choices. No worries, thanks a lot for responding. I'm happy to discuss this design further. I didn't necessarily mean these patches were a slam-dunk. I mainly meant that these first few patches were not specific to any protocol change, but are changes we should agree on before any change to the protocol is possible at all. Based on your response, we currently disagree on a bunch of core things. I'll first try to summarize my view on (future) protocol changes and why I think the current core design in this patchset is the correct path forward, and then go into some details inline in your response below: In my view there can be, **by definition,** only two general types of protocol changes: 1. A "simple protocol change": This is one that requires agreement by both the client and server, that they understand the new message types involved in this change. e.g. the ParameterSet message proposal (this message type is either supported or it's not) 2. A "parameterized protocol change": This requires the same as 1, but should also allow some parameterization from the client, e.g. for the compression proposal, the client should specify what compression algorithm the server should use to compress data when sending it to the client. Client and Server can agree that a "simple protocol change" is supported by both advertising a minimum minor protocol version. And for a "parameterized protocol change" the client can send a _pq_ parameter in the startup packet. So, new _pq_ parameters should only ever be introduced for parameterized protocol changes. They are not meant to advertise support, they are meant to configure protocol features. For a non-configurable protocol feature, we'd simply bump the protocol version. And since _pq_ parameters thus always control some setting at the session level, we can simply store it as a GUC, because that's how we store all our parameters at a session level. > Regarding 0001, I considered making this change as part of > ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed but decided against it, > because it seemed like it was making the assumption that we always > wanted to initiate new connections with the latest protocol version > that we know how to accept, and I wasn't sure that would always be > true. I think given the automatic downgrade supported by the NegotiateProtocolVersion, there's no real down-side to requesting the newest version by default. The only downside I can see is when connecting to other applications (i.e. non PostgreSQL servers) that implement the postgres protocol, but don't implement NegotiateProtocolVersion. But for that I added the max_protocol_version connection option in 0006 (of my latest patchset). > I'm really unhappy with 0002-0004. Just to be clear, (afaict) your argument below seems to only really be about 0004, not about 0002 or 0003. Was there anything about 0002 & 0003 that you were unhappy with? 0002 & 0003 are not dependent an 0004 imho. Because even when not making _pq_ parameters map to GUCs, we'd still need to change libpq to not instantly close the connection whenever a _pq_ parameter (or new protocol version) is not supported by the server (which is what 0002 & 0003 do). > That left no room for any other > type of protocol modification, so that commit carved out an exception, > where when we something that starts with _pq_., it's assumed to be > setting some other kind of thing, not a GUC. Okay, our interpretation is very different here. From my perspective introducing a non-GUC namespace is NOT AT ALL the benefit of the _pq_ prefix. The main benefit (imho) is that it allows sending an "optional" parameter (i.e GUC) in the startup packet. So, one where if the server doesn't recognize it the connection attempt still succeeds. If you specify a normal GUC in the connection parameters and the server doesn't know about it, the server will close the connection. So, to be able to send a GUC that depends on the protocol and/or server version in an optional way, you'd need to wait for an extra network roundtrip until the server tells you what protocol and/or server version they are. > But 0004 throws that out > the window and decides, nope, those are just GUCs, too. Even if we > don't have a specific reason today why we'd ever want such a thing, it > seems short-sighted to give up on the possibility that in the future > we will. Since I believe a _pq_ parameter should only be used to control settings at the session level. I don't think it would be short-sighted to give-up on the possibility to store them as anything else as GUCs. Because in the many years that we've had GUCs, we've been able to store all session settings using that infrastructure. BUT PLEASE NOTE: I don't think we are giving up on the thing you're describing (see explanation in final part of this email) > With this kind of design, you're just consuming one element of the > _pq_ namespace, and the next person who wants to do something can > consume one more element, and we'll be able to go on for a very long > time without running out of room. This is really how I intended this > mechanism to be used, and the only real downside I see as compared to > what you've done is that you can't set the protocol GUCs in the > startup packet, but must set them afterward via separate messages. If > that's a problem, then the proposal I just outline needs modification I very much think that's a problem. This would mean an extra roundtrip at connection startup. Which, as I described above, is to me the whole benefit of the _pq_ namespace. > ... but no matter what we do exactly, I don't want the very first > protocol extension we ever add to eat up all of _pq_. I intended that > to support decades worth of extensibility, not just one patch. This seems to be the core of your argument. But honestly, I don't understand this logic at all. Why do you think that assigning _pq_ parameters to GUCs **for the ones that match an existing GUC** would have such a far reaching effect into the future. There's only a handful of _pq_ parameters being proposed on the mailinglist at the moment. Even if we implement all of those as GUCs, and in the future, we'd want a _pq_ parameter that does not map to GUC (which I personally doubt will ever be the case). Then we can simply change the server code like so and do something special for that parameter: } + else if (strcmp(nameptr, "_pq_.some_non_guc_parameter") == 0) + { + // Do something with the parameter + } else if (strncmp(nameptr, "_pq_.", 5) == 0 && !find_option(nameptr, false, true, ERROR)) { /* * We report unkown protocol extensions using the * NegotiateProtocolVersion message instead of erroring */ This would be completely backwards compatible afaict, because _pq_.some_non_guc_parameter would not have been a GUC before. So the only thing that would have happened if you sent it, is that you would get back that the server doesn't support it in the NegotiateProtocolVersion packet (just like what is happening currently since ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed). SO TO BE VERY CLEAR: (afaict) interpreting _pq_ parameters as GUCs right now does not limit our ability to do something differently for new _pq_ parameters that we introduce in the future.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Mar 14, 2024 at 1:54 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > In my view there can be, **by definition,** only two general types of > protocol changes: > 1. A "simple protocol change": This is one that requires agreement by > both the client and server, that they understand the new message types > involved in this change. e.g. the ParameterSet message proposal (this > message type is either supported or it's not) > 2. A "parameterized protocol change": This requires the same as 1, but > should also allow some parameterization from the client, e.g. for the > compression proposal, the client should specify what compression > algorithm the server should use to compress data when sending it to > the client. You seem to be supposing here that all protocol changes will consist of adding new message types. 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?). > Client and Server can agree that a "simple protocol change" is > supported by both advertising a minimum minor protocol version. And > for a "parameterized protocol change" the client can send a _pq_ > parameter in the startup packet. > > So, new _pq_ parameters should only ever be introduced for > parameterized protocol changes. They are not meant to advertise > support, they are meant to configure protocol features. For a > non-configurable protocol feature, we'd simply bump the protocol > version. And since _pq_ parameters thus always control some setting at > the session level, we can simply store it as a GUC, because that's how > we store all our parameters at a session level. 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. > I think given the automatic downgrade supported by the > NegotiateProtocolVersion, there's no real down-side to requesting the > newest version by default. The only downside I can see is when > connecting to other applications (i.e. non PostgreSQL servers) that > implement the postgres protocol, but don't implement > NegotiateProtocolVersion. But for that I added the > max_protocol_version connection option in 0006 (of my latest > patchset). You might be right. This is a minor point that's not worth arguing about too much. > > I'm really unhappy with 0002-0004. > > Just to be clear, (afaict) your argument below seems to only really be > about 0004, not about 0002 or 0003. Was there anything about 0002 & > 0003 that you were unhappy with? 0002 & 0003 are not dependent an 0004 > imho. Because even when not making _pq_ parameters map to GUCs, we'd > still need to change libpq to not instantly close the connection > whenever a _pq_ parameter (or new protocol version) is not supported > by the server (which is what 0002 & 0003 do). I completely agree that we need to avoid slamming the connection shut. What I don't agree with is taking the list of protocol extensions that the server knows about and putting it into an array of strings that the user can see. I don't want the user to know or care so much about what's happening at the wire protocol level. 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. Just as a for instance, I had a thought that we might accumulate a few new message types controlled by protocol options (ParameterSet, AlwaysSendTypeInBinaryFormat, whatever else) while keeping the protocol version as 3.0, and then eventually bump the protocol version to 3.1 where all of that would be mandatory functionality. So the protocol parameters wouldn't be specified any more when using 3.1, but they would be specified when talking to older 3.0 servers. That difference shouldn't be visible to the user. The user should only know the result of the negotiation. > Okay, our interpretation is very different here. From my perspective > introducing a non-GUC namespace is NOT AT ALL the benefit of the _pq_ > prefix. The main benefit (imho) is that it allows sending an > "optional" parameter (i.e GUC) in the startup packet. So, one where if > the server doesn't recognize it the connection attempt still succeeds. > If you specify a normal GUC in the connection parameters and the > server doesn't know about it, the server will close the connection. 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. > > ... but no matter what we do exactly, I don't want the very first > > protocol extension we ever add to eat up all of _pq_. I intended that > > to support decades worth of extensibility, not just one patch. > > This seems to be the core of your argument. But honestly, I don't > understand this logic at all. Why do you think that assigning _pq_ > parameters to GUCs **for the ones that match an existing GUC** would > have such a far reaching effect into the future. There's only a > handful of _pq_ parameters being proposed on the mailinglist at the > moment. Even if we implement all of those as GUCs, and in the future, > we'd want a _pq_ parameter that does not map to GUC (which I > personally doubt will ever be the case). Then we can simply change the > server code like so and do something special for that parameter: 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: 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. 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. 2. Clarity of meaning across versions. Let's say we add a protocol option in the future that expands the message-type byte into a two-byte word. Failure of the two sides to agree on the value of this protocol option is, fairly obviously, a catastrophe. I assume that if we actually did something like this, there's a fair chance it would be protocol version 4.0 rather than an option to 3.whatever, but it's a good example of something that might someday need to be changed that is not just a new message type and about which the communicating parties must absolutely agree. Let's say we do as you propose and have a GUC wide_message_types = {true | false}. Now, what happens when a sneaky user of an older libpq, which does not know about this option, tries to connect to a newer server? 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. 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. And we're already using client_encoding, which is a GUC, for something that really ought not to have been handled that way ... because to take the connection pooler example again, there's no reason -- other than bad wire-protocol design -- why the encoding being used between the client and the pooler needs to match the encoding being used between the pooler and the server. But in my view, this isn't evidence that we should continue to muddy the distinction between things that ought to be protocol parameters and things that ought to be GUCs; rather, it's evidence of the need to make the distinction between the two as crisp as we possibly can. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Peter Eisentraut
Дата:
On 14.03.24 21:33, Robert Haas wrote: > You seem to be supposing here that all protocol changes will consist > of adding new message types. While I think that will be a common > pattern, I do not think it will be a universal one. As an additional data point, the column encryption patch that is currently on hiatus [0] uses a protocol extension to change the format of existing message types. [0]: https://www.postgresql.org/message-id/flat/89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com > 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. It appears there are several different perspectives about this. My intuition was that a protocol version change indicates something that we eventually want all client libraries to support. Whereas protocol extensions are truly opt-in. For example, if we didn't have the ParameterStatus message and someone wanted to add it, then this ought to be a protocol version change, so that we eventually get everyone to adopt it. But, for example, the column encryption feature is not something I'd expect all client libraries to implement, so it can be opt-in. (I believe we have added a number of new protocol messages since the beginning of the 3.0 protocol, without any version change, so "new protocol message" might not always be a good example to begin with.) I fear that if we prefer protocol extensions over version increases, then we'd get a very fragmented landscape of different client libraries supporting different combinations of things.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
Ugh, I seem to have somehow missed this response completely. On Thu, 14 Mar 2024 at 21:33, Robert Haas <robertmhaas@gmail.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.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 4 Apr 2024 at 14:50, Peter Eisentraut <peter@eisentraut.org> wrote: > It appears there are several different perspectives about this. My > intuition was that a protocol version change indicates something that we > eventually want all client libraries to support. Whereas protocol > extensions are truly opt-in. > > For example, if we didn't have the ParameterStatus message and someone > wanted to add it, then this ought to be a protocol version change, so > that we eventually get everyone to adopt it. > > But, for example, the column encryption feature is not something I'd > expect all client libraries to implement, so it can be opt-in. I think that is a good way of deciding between version bump vs protocol extension parameter. But I'd like to make one clarification/addition to this logic, if the protocol feature already requires opt-in from the client because of how the feature works, then there's no need for a protocol extension parameter. e.g. if you're column-encryption feature would require the client to send a ColumnDecrypt message before the server would exhibit any behaviour relating to the column-encryption feature, then the client can simply "support" the feature by never sending the ColumnDecrypt message. Thus, in such cases a protocol extension parameter would not be necessary, even if the feature is considered opt-in. > (I believe we have added a number of new protocol messages since the > beginning of the 3.0 protocol, without any version change, so "new > protocol message" might not always be a good example to begin with.) Personally, I feel like this is something we should change. IMHO, we should get to a state where protocol minor version bumps are so low-risk that we can do them whenever we add message types. Right now there are basically multiple versions of the 3.0 protocol, which is very confusing to anyone implementing it. Different servers implementing the 3.0 protocol without the NegotiateVersion message is a good example of that. > I fear that if we prefer protocol extensions over version increases, > then we'd get a very fragmented landscape of different client libraries > supporting different combinations of things. +1
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
Attached is a rebased patchset
Вложения
- v12-0003-libpq-Add-PQunsupportedProtocolExtensionParamete.patch
- v12-0001-libpq-Remove-instance-of-hardcoded-protocol-vers.patch
- v12-0002-libpq-Handle-NegotiateProtocolVersion-message-mo.patch
- v12-0005-libpq-Include-minor-version-number-in-PQprotocol.patch
- v12-0004-Prepare-server-code-for-addition-of-protocol-ext.patch
- v12-0008-Add-protocol-message-to-change-parameters.patch
- v12-0007-Bump-protocol-version-to-3.1.patch
- v12-0010-psql-Warn-about-unsupported-protocol-features.patch
- v12-0009-psql-Add-parameterset-meta-command.patch
- v12-0006-libpq-Add-max_protocol_version-connection-option.patch
- v12-0012-Add-_pq_.protocol_managed_params-protocol-extens.patch
- v12-0014-Add-_pq_.report_parameters-protocol-extension.patch
- v12-0011-Add-GUC-contexts-for-protocol-extensions.patch
- v12-0013-Add-tests-for-ParameterSet-and-_pq_.protocol_man.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Dave Cramer
Дата:
On Thu, 4 Apr 2024 at 12:45, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Thu, 4 Apr 2024 at 14:50, Peter Eisentraut <peter@eisentraut.org> wrote:
> It appears there are several different perspectives about this. My
> intuition was that a protocol version change indicates something that we
> eventually want all client libraries to support. Whereas protocol
> extensions are truly opt-in.
>
> For example, if we didn't have the ParameterStatus message and someone
> wanted to add it, then this ought to be a protocol version change, so
> that we eventually get everyone to adopt it.
>
> But, for example, the column encryption feature is not something I'd
> expect all client libraries to implement, so it can be opt-in.
I think that is a good way of deciding between version bump vs
protocol extension parameter. But I'd like to make one
clarification/addition to this logic, if the protocol feature already
requires opt-in from the client because of how the feature works, then
there's no need for a protocol extension parameter. e.g. if you're
column-encryption feature would require the client to send a
ColumnDecrypt message before the server would exhibit any behaviour
relating to the column-encryption feature, then the client can simply
"support" the feature by never sending the ColumnDecrypt message.
Thus, in such cases a protocol extension parameter would not be
necessary, even if the feature is considered opt-in.
> (I believe we have added a number of new protocol messages since the
> beginning of the 3.0 protocol, without any version change, so "new
> protocol message" might not always be a good example to begin with.)
Personally, I feel like this is something we should change. IMHO, we
should get to a state where protocol minor version bumps are so
low-risk that we can do them whenever we add message types. Right now
there are basically multiple versions of the 3.0 protocol, which is
very confusing to anyone implementing it. Different servers
implementing the 3.0 protocol without the NegotiateVersion message is
a good example of that.
Totally agree.
> I fear that if we prefer protocol extensions over version increases,
> then we'd get a very fragmented landscape of different client libraries
> supporting different combinations of things.
+1
Dave
+1
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Apr 4, 2024 at 8:50 AM Peter Eisentraut <peter@eisentraut.org> wrote: > It appears there are several different perspectives about this. My > intuition was that a protocol version change indicates something that we > eventually want all client libraries to support. Whereas protocol > extensions are truly opt-in. Hmm. This doesn't seem like a bad way to make a distinction to me, except that I fear it would be mushy in practice. For example: > For example, if we didn't have the ParameterStatus message and someone > wanted to add it, then this ought to be a protocol version change, so > that we eventually get everyone to adopt it. > > But, for example, the column encryption feature is not something I'd > expect all client libraries to implement, so it can be opt-in. I agree that column encryption might not necessarily be supported by all client libraries, but equally, the ParameterStatus message is just for the benefit of the client. A client that doesn't care about the contents of such a message is free to ignore it, and would be better off if it weren't sent in the first place; it's just extra bytes on the wire that aren't needed for anything. So why would we want to force everyone to adopt that, if it didn't exist already? I also wonder how the protocol negotiation for column encryption is actually going to work. What are the actual wire protocol changes that are needed? What does the server need to know from the client, or the client from the server, about what is supported? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Apr 4, 2024 at 12:45 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > 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. Often? I kind of hope that the protocol starts to evolve a bit more than it has, but I don't want a continuous stream of changes. That will be very hard to test and verify correctness, and a hassle for drivers to keep up with, and a mess for compatibility. > 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? Yeah, something like that. > 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. Sure, if we introduce it based on the protocol version then we don't need anything else. > 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. I guess that's a fair point. But I'm worried that if we expose too much of the internals, we won't be able to change things later. > > 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. Hmm. I guess that is a problem. > 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 really intended the _pq_ prefix as a way of taking something out of the GUC namespace, not as a part of the GUC namespace that users would see. And I'm reluctant to go back on that. If we want to make pg_protocol.${NAME} mean a wire protocol parameter, well maybe there's something to that idea [insert caveats here]. But doesn't _pq_ look like something that was intended to be internal? That's certainly how I intended it. > > 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. Wouldn't libpq already know what value it last set? Or is this needed because it doesn't know what the other side's default is? > 2. By using PGC_PROTOCOL to indicate that those GUCs can only be > changed using ParameterSet Hmm, OK. I guess if the PGC_PROTOCOL flag makes it so that the GUC can only be set using ParameterSet, and it also makes them non-transactional, then it's fine. So to be clear, I can't set these in postgresql.conf, or postgresql.auto.conf, or via ALTER $ANYTHING, or via SET, or in any other way than by sending ParameterStatus messages. And when I send a ParameterStatus message, it doesn't matter if I'm in a good transaction, an aborted transaction, or no transaction at all, and the setting change takes effect regardless of that and regardless of any subsequent rollbacks. Is that right? I feel like maybe it's not, because you seem to be thinking that you'd also set these in the startup packet, at least... -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Apr 4, 2024 at 1:10 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > Attached is a rebased patchset We should keep talking about this, but I think we're far too close to the wire at this point to think about committing anything for v17 at this point. These are big changes, they haven't been thoroughly reviewed by anyone AFAICT, and we don't have consensus on what we ought to be doing. I know that's probably not what you want to hear, but realistically, I think that's the only reasonable decision at this point. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 16:04, Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Apr 4, 2024 at 1:10 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > > Attached is a rebased patchset > > We should keep talking about this, but I think we're far too close to > the wire at this point to think about committing anything for v17 at > this point. These are big changes, they haven't been thoroughly > reviewed by anyone AFAICT, and we don't have consensus on what we > ought to be doing. I know that's probably not what you want to hear, > but realistically, I think that's the only reasonable decision at this > point. Agreed on not considering this for PG17 nor this commitfest anymore. I changed the commit fest entry accordingly.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 16:02, Robert Haas <robertmhaas@gmail.com> wrote: > Often? > > I kind of hope that the protocol starts to evolve a bit more than it > has, but I don't want a continuous stream of changes. That will be > very hard to test and verify correctness, and a hassle for drivers to > keep up with, and a mess for compatibility. I definitely think protocol changes require a lot more scrutiny than many other things, given their hard/impossible to change nature. But I do think that we shouldn't be at all averse to the act of bumping the protocol version itself. If we have a single small protocol change in one release, then imho it's no problem to bump the protocol version. Bumping the version should be painless. So we shouldn't be inclined to push an already agreed upon protocol change to the next release, because there are some more protocol changes in the pipeline that won't make it in the current one. I don't think this would be any harder for drivers to keep up with, then if we'd bulk all changes together. If driver developers only want to support changes in bulk changes, they can simply choose not to support 3.1 at all if they want and wait until 3.2 to support everything in bulk then. > > 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? > > Yeah, something like that. > ... > > > 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. > > I guess that's a fair point. But I'm worried that if we expose too > much of the internals, we won't be able to change things later. I'll take a look at redesigning the protocol parameter stuff. To work with dedicated functions instead. > I really intended the _pq_ prefix as a way of taking something out of > the GUC namespace, not as a part of the GUC namespace that users would > see. And I'm reluctant to go back on that. If we want to make > pg_protocol.${NAME} mean a wire protocol parameter, well maybe there's > something to that idea [insert caveats here]. But doesn't _pq_ look > like something that was intended to be internal? That's certainly how > I intended it. I agree that _pq_ does look internal and doesn't clearly indicate that it's a protocol related change. But sadly the _pq_ prefix is the only one that doesn't error in startup packets, waiting another 5 years until pg_protocol is allowed in the startup packet doesn't seem like a reasonable solution either. How about naming the GUCs pg_protocol.${NAME}, but still requiring the _pq_ prefix in the StartupPacket. That way only client libraries would have to see this internal prefix and they could remap it someway. I see two options for that: 1. At the server replace the _pq_ prefix with pg_protocol. So _pq_.${NAME} would map to pg_protocol.${name} 2. At the server replace the _pq_.pg_protocol prefix with pg_protocol. So _pq_.pg_protocol.${NAME} would map to pg_protocol.${name}. I guess you prefer option 2, because that would still leave lots of space to do something with the rest of the _pq_ space, i.e. _pq_.magic_pixie_dust can still be used for something different than a GUC. Bikeshedding: I think I prefer protocol.${NAME} over pg_protocol.${NAME}, it's shorter and it seems obvious that protocol is the postgres protocol in this context. This should be a fairly simple change to make. > Wouldn't libpq already know what value it last set? Or is this needed > because it doesn't know what the other side's default is? libpq could/should indeed know this, but for debugging/testing purposes it is quite useful to have a facility to read the server side value. I think defaults should always be whatever was happening if the parameter wasn't specified before, so knowing the server default is not something the client needs to worry about (i.e. the default is defined as part of the protocol spec). > Hmm, OK. I guess if the PGC_PROTOCOL flag makes it so that the GUC can > only be set using ParameterSet, and it also makes them > non-transactional, then it's fine. So to be clear, I can't set these > in postgresql.conf, or postgresql.auto.conf, or via ALTER $ANYTHING, > or via SET, or in any other way than by sending ParameterStatus > messages. And when I send a ParameterStatus message, it doesn't matter > if I'm in a good transaction, an aborted transaction, or no > transaction at all, and the setting change takes effect regardless of > that and regardless of any subsequent rollbacks. Is that right? > > I feel like maybe it's not, because you seem to be thinking that you'd > also set these in the startup packet, at least... Setting PGC_PROTOCOL gucs would be allowed in the startup packet, which is fine afaict because that's also something that's part of the protocol level and is thus fully controlled by client libraries and poolers) But other than that: Yes, conf files, ALTER, and SET cannot change these GUCs.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Dave Cramer
Дата:
On Fri, 5 Apr 2024 at 12:09, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Fri, 5 Apr 2024 at 16:02, Robert Haas <robertmhaas@gmail.com> wrote:
> Often?
>
> I kind of hope that the protocol starts to evolve a bit more than it
> has, but I don't want a continuous stream of changes. That will be
> very hard to test and verify correctness, and a hassle for drivers to
> keep up with, and a mess for compatibility.
I definitely think protocol changes require a lot more scrutiny than
many other things, given their hard/impossible to change nature.
But I do think that we shouldn't be at all averse to the act of
bumping the protocol version itself. If we have a single small
protocol change in one release, then imho it's no problem to bump the
protocol version. Bumping the version should be painless. So we
shouldn't be inclined to push an already agreed upon protocol change
to the next release, because there are some more protocol changes in
the pipeline that won't make it in the current one.
I don't think this would be any harder for drivers to keep up with,
then if we'd bulk all changes together. If driver developers only want
to support changes in bulk changes, they can simply choose not to
support 3.1 at all if they want and wait until 3.2 to support
everything in bulk then.
> > 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?
>
> Yeah, something like that.
> ...
>
> > 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.
>
> I guess that's a fair point. But I'm worried that if we expose too
> much of the internals, we won't be able to change things later.
I'll take a look at redesigning the protocol parameter stuff. To work
with dedicated functions instead.
+1
> I really intended the _pq_ prefix as a way of taking something out of
> the GUC namespace, not as a part of the GUC namespace that users would
> see. And I'm reluctant to go back on that. If we want to make
> pg_protocol.${NAME} mean a wire protocol parameter, well maybe there's
> something to that idea [insert caveats here]. But doesn't _pq_ look
> like something that was intended to be internal? That's certainly how
> I intended it.
Is this actually used in practice? If so, how ?
I agree that _pq_ does look internal and doesn't clearly indicate that
it's a protocol related change. But sadly the _pq_ prefix is the only
one that doesn't error in startup packets, waiting another 5 years
until pg_protocol is allowed in the startup packet doesn't seem like a
reasonable solution either.
How about naming the GUCs pg_protocol.${NAME}, but still requiring the
_pq_ prefix in the StartupPacket. That way only client libraries would
have to see this internal prefix and they could remap it someway. I
see two options for that:
1. At the server replace the _pq_ prefix with pg_protocol. So
_pq_.${NAME} would map to pg_protocol.${name}
2. At the server replace the _pq_.pg_protocol prefix with pg_protocol.
So _pq_.pg_protocol.${NAME} would map to pg_protocol.${name}.
I guess you prefer option 2, because that would still leave lots of
space to do something with the rest of the _pq_ space, i.e.
_pq_.magic_pixie_dust can still be used for something different than a
GUC.
Bikeshedding: I think I prefer protocol.${NAME} over
pg_protocol.${NAME}, it's shorter and it seems obvious that protocol
is the postgres protocol in this context.
This should be a fairly simple change to make.
> Wouldn't libpq already know what value it last set? Or is this needed
> because it doesn't know what the other side's default is?
libpq could/should indeed know this, but for debugging/testing
purposes it is quite useful to have a facility to read the server side
value. I think defaults should always be whatever was happening if the
parameter wasn't specified before, so knowing the server default is
not something the client needs to worry about (i.e. the default is
defined as part of the protocol spec).
> Hmm, OK. I guess if the PGC_PROTOCOL flag makes it so that the GUC can
> only be set using ParameterSet, and it also makes them
> non-transactional, then it's fine. So to be clear, I can't set these
> in postgresql.conf, or postgresql.auto.conf, or via ALTER $ANYTHING,
> or via SET, or in any other way than by sending ParameterStatus
> messages. And when I send a ParameterStatus message, it doesn't matter
> if I'm in a good transaction, an aborted transaction, or no
> transaction at all, and the setting change takes effect regardless of
> that and regardless of any subsequent rollbacks. Is that right?
>
> I feel like maybe it's not, because you seem to be thinking that you'd
> also set these in the startup packet, at least...
Setting PGC_PROTOCOL gucs would be allowed in the startup packet,
which is fine afaict because that's also something that's part of the
protocol level and is thus fully controlled by client libraries and
poolers) But other than that: Yes, conf files, ALTER, and SET cannot
change these GUCs.
+1
Dave
Dave Cramer <davecramer@gmail.com> writes: > On Fri, 5 Apr 2024 at 12:09, Jelte Fennema-Nio <me@jeltef.nl> wrote: >> Setting PGC_PROTOCOL gucs would be allowed in the startup packet, >> which is fine afaict because that's also something that's part of the >> protocol level and is thus fully controlled by client libraries and >> poolers) But other than that: Yes, conf files, ALTER, and SET cannot >> change these GUCs. > +1 I don't buy that argument, actually. libpq, and pretty much every other client AFAIK, has provisions to let higher code levels insert random options into the startup packet. So to make this work libpq would have to filter or at least inspect such options, which is logic that doesn't exist and doesn't seem nice to need. The other problem with adding these things in the startup packet is that when you send that packet, you don't know what the server version is and hence don't know if it will take these options. What's so bad about insisting that these options must be sent in a separate message? regards, tom lane
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, Apr 5, 2024 at 12:09 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > But I do think that we shouldn't be at all averse to the act of > bumping the protocol version itself. If we have a single small > protocol change in one release, then imho it's no problem to bump the > protocol version. Bumping the version should be painless. So we > shouldn't be inclined to push an already agreed upon protocol change > to the next release, because there are some more protocol changes in > the pipeline that won't make it in the current one. I think I half-agree with this. Let's say we all agree that the world will end unless we make wire protocol changes A and B, and for whatever reason we also agree that these changes should be handled via a protocol version bump rather than any other method, but only the patch for A is sufficiently stable by the end of the release cycle. Then we commit A and bump the protocol version and the next release we do the same for B, hopefully before the world ends. In this sense this is just like CATALOG_VERSION_NO or XLOG_PAGE_MAGIC. We don't postpone commits because they'd require bumping those values; we just bump the values when it's necessary. But on the other hand, I find it a bit hard to believe that the statement "Bumping the version should be painless" will ever correspond to reality. Since we've not done a hard wire protocol break in a very long time, I assume that we would not want to start now. But that also means that when the PG version with protocol version 3.5 comes out, that server is going to have to be compatible with 3.4, 3.3, 3.2, 3.1, and 3.0. How will we test that it really is? We could test against libpqs from older server versions, but that quickly becomes awkward, because it means that there won't be tests that run as part of a regular build, but it'll have to all be done in the buildfarm or CI with something like the cross-version upgrade tests we already have. Maybe we'd be better off adding a libpq connection option that forces the use of a specific minor protocol version, but then we'll need backward-compatibility code in libpq basically forever. But maybe we need that anyway to avoid older and newer servers being unable to communicate. Plus, you've got all of the consequences for non-core drivers, which have to both add support for the new wire protocol - if they don't want to seem outdated and eventually obsolete - and also test that they're still compatible with all supported server versions. Connection poolers have the same set of problems. The whole thing is almost a hole with no bottom. Keeping up with core changes in this area could become a massive undertaking for lots and lots of people, some of whom may be the sole maintainer of some important driver that now needs a whole bunch of work. I'm not sure how much it improves things if we imagine adding feature flags to the existing protocol versions, rather than whole new protocol versions, but at least it cuts down on the assumption that adopting new features is mandatory, and that such features are cumulative. If a driver wants to support TDE but not protocol parameters or protocol parameters but not TDE, who are we to say no? If in supporting those things we bump the protocol version to 3.2, and then 3.3 fixes a huge performance problem, are drivers going to be required to add support for features they don't care about to get the performance fixes? I see some benefit in bumping the protocol version for major changes, or for changes that we have an important reason to make mandatory, or to make previously-optional features for which support has become in practical terms universal part of the base feature set. But I'm very skeptical of the idea that we should just handle as many things as possible via a protocol version bump. We've been avoiding protocol version bumps like the plague since forever, and swinging all the way to the other extreme doesn't sound like the right idea to me. > How about naming the GUCs pg_protocol.${NAME}, but still requiring the > _pq_ prefix in the StartupPacket. That way only client libraries would > have to see this internal prefix and they could remap it someway. I > see two options for that: > 1. At the server replace the _pq_ prefix with pg_protocol. So > _pq_.${NAME} would map to pg_protocol.${name} > 2. At the server replace the _pq_.pg_protocol prefix with pg_protocol. > So _pq_.pg_protocol.${NAME} would map to pg_protocol.${name}. > > I guess you prefer option 2, because that would still leave lots of > space to do something with the rest of the _pq_ space, i.e. > _pq_.magic_pixie_dust can still be used for something different than a > GUC. I'm not sure what I think about this. Do we need these new GUCs to be both PGC_PROTOCOL *and also* live in a separate namespace? I see the need for the former pretty clearly: if these kinds of things are to be part of the GUC system (which wasn't my initial bias, but whatever) then they need to have some important behavioral differences from other GUCs and so we need a flag to signal that. But what problem are we solving by also giving them special-looking names, and are we sure we wouldn't rather solve that problem some other way? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 18:30, Dave Cramer <davecramer@gmail.com> wrote: >> > I really intended the _pq_ prefix as a way of taking something out of >> > the GUC namespace, not as a part of the GUC namespace that users would >> > see. And I'm reluctant to go back on that. If we want to make >> > pg_protocol.${NAME} mean a wire protocol parameter, well maybe there's >> > something to that idea [insert caveats here]. But doesn't _pq_ look >> > like something that was intended to be internal? That's certainly how >> > I intended it. > > > Is this actually used in practice? If so, how ? No, it's not used for anything at the moment. This whole thread is basically about trying to agree on how we want to make protocol changes in the future in a somewhat standardized way. But using the tools available that we have to not break connecting to old postgres servers: ProtocolVersionNegotation messages, minor version numbers, and _pq_ parameters in the startup message. All of those have so far been completely theoretical and have not appeared in any client-server communication.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 18:43, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't buy that argument, actually. libpq, and pretty much every > other client AFAIK, has provisions to let higher code levels insert > random options into the startup packet. So to make this work libpq > would have to filter or at least inspect such options, which is > logic that doesn't exist and doesn't seem nice to need. libpq actually doesn't support doing this (only by putting them in the "options" parameter, but _pq_ parameters would not be allowed there), but indeed many other clients do allow this and indeed likely don't have logic to filter/disallow _pq_ prefixed parameters. This seems very easy to address though: Only parse _pq_ options when protocol version 3.1 is requested by the client, and otherwise always report them as "not supported". Then clients upgrading to 3.1, they should filter/disallow _pq_ parameters to be arbitrarily set. I don't think that's hard/not nice to add, it's literally a prefix check for the "_pq_." string. > The other problem with adding these things in the startup packet > is that when you send that packet, you don't know what the server > version is and hence don't know if it will take these options. (imho) the whole point of the _pq_ options is that they don't trigger an error when they are requested by the client, but not supported by the server. So I don't understand your problem here. > What's so bad about insisting that these options must be sent in a > separate message? To not require an additional roundtrip waiting for the server to respond.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Dave Cramer
Дата:
Plus, you've got all of the consequences for non-core drivers, which
have to both add support for the new wire protocol - if they don't
want to seem outdated and eventually obsolete - and also test that
they're still compatible with all supported server versions.
Connection poolers have the same set of problems. The whole thing is
almost a hole with no bottom. Keeping up with core changes in this
area could become a massive undertaking for lots and lots of people,
some of whom may be the sole maintainer of some important driver that
now needs a whole bunch of work.
We already have this in many places. Headers or functions change and extensions have to fix their code.
catalog changes force drivers to change their code.
This argument blocks any improvement to the protocol. I don't think it's unreasonable to expect maintainers to keep up.
We could make it easier by having a specific list for maintainers, but that doesn't change the work.
I'm not sure how much it improves things if we imagine adding feature
flags to the existing protocol versions, rather than whole new
protocol versions, but at least it cuts down on the assumption that
adopting new features is mandatory, and that such features are
cumulative. If a driver wants to support TDE but not protocol
parameters or protocol parameters but not TDE, who are we to say no?
If in supporting those things we bump the protocol version to 3.2, and
then 3.3 fixes a huge performance problem, are drivers going to be
required to add support for features they don't care about to get the
performance fixes? I see some benefit in bumping the protocol version
for major changes, or for changes that we have an important reason to
make mandatory, or to make previously-optional features for which
support has become in practical terms universal part of the base
feature set. But I'm very skeptical of the idea that we should just
handle as many things as possible via a protocol version bump. We've
been avoiding protocol version bumps like the plague since forever,
and swinging all the way to the other extreme doesn't sound like the
right idea to me.
+1 for not swinging too far here. But I don't think it should be a non starter.
Dave
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 18:48, Robert Haas <robertmhaas@gmail.com> wrote: > Maybe we'd be better off adding a libpq connection > option that forces the use of a specific minor protocol version, but > then we'll need backward-compatibility code in libpq basically > forever. But maybe we need that anyway to avoid older and newer > servers being unable to communicate. I think this would be good because it makes testing easy and, like you said, I think we'll need this backward-compatibility code in libpq anyway to be able to connect to old servers. To have even better and more realistic test coverage though, I think we might also want to actually test new libpq against old postgres servers and vice-versa in a build farm animal though. > Plus, you've got all of the consequences for non-core drivers, which > have to both add support for the new wire protocol - if they don't > want to seem outdated and eventually obsolete - and also test that > they're still compatible with all supported server versions. I think for clients/drivers, the work would generally be pretty minimal. For almost all proposed changes, clients can "support" the protocol version update by simply not using the new features, e.g. a client can "support" the ParameterSet feature, by simply never sending the ParameterSet message. So binding it to a protocol version bump doesn't make it any harder for that client to support that protocol version. I'm not saying that is the case for all protocol changes, but based on what's being proposed so far that's definitely a very common theme. Overall, I think this is something to discuss for each protocol change in isolation: i.e. how to make supporting the new feature as painless as possible for clients/drivers. > Connection poolers have the same set of problems. For connection poolers this is indeed a bigger hassle, because they at least need to be able to handle all the new message types that a client can send and maybe do something special for them. But I think if we're careful to keep connection poolers in mind when designing the features themselves then I think this isn't necessarily a problem. And probably indeed for the features that we think are hard for connection poolers to implement, we should be using protocol extension parameter feature flags. But I think a lot of protocol would be fairly trivial for a connection pooler to support. > The whole thing is > almost a hole with no bottom. Keeping up with core changes in this > area could become a massive undertaking for lots and lots of people, > some of whom may be the sole maintainer of some important driver that > now needs a whole bunch of work. I agree with Dave here, if you want to benefit from new features there's some expectation to keep up with the changes. But to be clear, we'd still support old protocol versions too. So we wouldn't break connecting using those clients, they simply wouldn't benefit from some of the new features. I think that's acceptable. > I'm not sure how much it improves things if we imagine adding feature > flags to the existing protocol versions, rather than whole new > protocol versions, but at least it cuts down on the assumption that > adopting new features is mandatory, and that such features are > cumulative. If a driver wants to support TDE but not protocol > parameters or protocol parameters but not TDE, who are we to say no? > If in supporting those things we bump the protocol version to 3.2, and > then 3.3 fixes a huge performance problem, are drivers going to be > required to add support for features they don't care about to get the > performance fixes? I think there's an important trade-off here. On one side we don't want to make maintainers of clients/poolers do lots of work to support features they don't care about. And on the other side it seems quite useful to limit the amount of feature combinations that are used it the wild (both for users and for us) e.g. the combinations of backwards compatibility testing you were talking about would explode if every protocol change was a feature flag. I think this trade-off is something we should be deciding on based on the specific protocol change. But if work needed to "support" the feature is "minimal" (to-be-defined exactly what we consider minimal), I think making it part of a protocol version bump is reasonable. > I see some benefit in bumping the protocol version > for major changes, or for changes that we have an important reason to > make mandatory, or to make previously-optional features for which > support has become in practical terms universal part of the base > feature set. But I'm very skeptical of the idea that we should just > handle as many things as possible via a protocol version bump. We've > been avoiding protocol version bumps like the plague since forever, > and swinging all the way to the other extreme doesn't sound like the > right idea to me. I think there's two parts to a protocol version bump: 1. The changes that cause us to consider a protocol bump 2. The actual act of bumping the protocol version I think 1 is a thing we should be careful about every time (especially regarding impact on clients/poolers). But 2 shouldn't be something that we should consider dangerous/scary. I think that every change that we make to the protocol (no matter how minor or backwards compatible it is), should be accompanied with a protocol version bump. This isn't what has happened in the past, and it makes it quite hard to understand what "supporting" a specific protocol version actually means. e.g. PgBouncer currently supports protocol 3.0, but doesn't support the NegotiateProtocolVersion message (I'm working on fixing that). To take it to the extreme: I think we should get to a state, where if we bump the protocol version at the client and server side without actually making any protocol changes, everything should continue to work fine. If we'd do that right now, then libpq wouldn't be able to connect to old postgres versions anymore. > I'm not sure what I think about this. Do we need these new GUCs to be > both PGC_PROTOCOL *and also* live in a separate namespace? I see the > need for the former pretty clearly: if these kinds of things are to be > part of the GUC system (which wasn't my initial bias, but whatever) > then they need to have some important behavioral differences from > other GUCs and so we need a flag to signal that. But what problem are > we solving by also giving them special-looking names, and are we sure > we wouldn't rather solve that problem some other way? Clients might want to allow the user of the client to change regular parameters using ParameterSet (e.g. so that a connection pooler can intercept those ParameterSet messages and change its own behaviour if the parameter name is pgbouncer.pool_mode). But they wouldn't want a user to set any parameters that change the wire-protocol this way. And because an old client might connect to a new server a simple hard-coded list of parameters at the client side is not sufficient. I can see two ways around this: 1. Using a well-known prefix or namespace for parameters that change the wire protocol. (exact prefix to be bikeshedded on) 2. Using a hard-coded list at the client AND disallow changing PGC_PROTOCOL parameters at the server if the negotiated protocol version is lower than the version this parameter was introduced in AND bump the protocol version whenever we add a new PGC_PROTOCOL parameter. I think 1 is easier to implement at the client side, as it only requires a prefix comparison instead of keeping track of a list.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Peter Eisentraut
Дата:
On 05.04.24 14:55, Robert Haas wrote: > I also wonder how the protocol negotiation for column encryption is > actually going to work. What are the actual wire protocol changes that > are needed? What does the server need to know from the client, or the > client from the server, about what is supported? I have just posted an updated patch for that: [0] The protocol changes can be inspected in the diffs for doc/src/sgml/protocol.sgml src/backend/access/common/printtup.c src/interfaces/libpq/fe-protocol3.c There are various changes, including new messages, additional fields in existing messages, and some more flag bits in existing fields. It all works, so I don't have any requests or anything in this thread, but it would be good to get some feedback if I'm using this wrong. AFAICT, that patch was the first public one that ever tried to make use of the protocol extension facility, so I was mainly guessing about the intended way to use this. [0]: https://www.postgresql.org/message-id/f63fe170-cef2-4914-be00-ef9222456505%40eisentraut.org
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > I think for clients/drivers, the work would generally be pretty > minimal. For almost all proposed changes, clients can "support" the > protocol version update by simply not using the new features, ... I mean, I agree if a particular protocol version bump does nothing other than signal the presence of some optional, ignorable feature, then it doesn't cause a problem if we force clients to support it. But that seems a bit like saying that eating wild mushrooms is fine because some of them aren't poisonous. The point is that if we roll out two protocol changes, A and B, each of which requires the client to make some change in order to work with the newer protocol version, then using version numbers as the gating mechanism requires that the client can't support the newer of those two changes without also supporting the older one. Using feature flags doesn't impose that constraint, which I think is a plus. > I think there's an important trade-off here. On one side we don't want > to make maintainers of clients/poolers do lots of work to support > features they don't care about. And on the other side it seems quite > useful to limit the amount of feature combinations that are used it > the wild (both for users and for us) e.g. the combinations of > backwards compatibility testing you were talking about would explode > if every protocol change was a feature flag. This is a good point, and I agree. It's a little hard to discuss this in the abstract. I wouldn't want to have feature flags that say: 1. I support rows containing an extra-large number of bytes. 2. I support rows containing an extra-large number of columns. 3. I support queries returning an extra-large number of rows. It is quite likely that there might be bugs that only manifest with particular combinations of those flags; knowing that someone who supports (3) must also support (1) and (2) seems like it would make everyone's life easier. But on the other hand, I wouldn't mind having feature flags that say: 1. I support transparent column encryption. 2. I support letting a connection pooler lock down the session role in a way that can't be reversed from SQL. 3. I support compression. It's still not theoretically impossible that those features could interact with each other in some way, but it seems a lot less likely. Here, I think a driver ought to be able to choose any subset of these things and support only the ones they care about, without having to worry about the server deciding to do something for which the driver is unprepared. Also, in a case like this, if there are bugs that only occur in certain combinations, we have to test for and fix those anyway, because even if a driver supports all of those features, they're not all going to be used for every connection. > I think there's two parts to a protocol version bump: > 1. TI ahe changes that cause us to consider a protocol bump > 2. The actual act of bumping the protocol version > > I think 1 is a thing we should be careful about every time (especially > regarding impact on clients/poolers). But 2 shouldn't be something > that we should consider dangerous/scary. I think that every change > that we make to the protocol (no matter how minor or backwards > compatible it is), should be accompanied with a protocol version bump. > This isn't what has happened in the past, and it makes it quite hard > to understand what "supporting" a specific protocol version actually > means. e.g. PgBouncer currently supports protocol 3.0, but doesn't > support the NegotiateProtocolVersion message (I'm working on fixing > that). I'm generally not a fan of giving things version numbers and then not changing the version numbers when you change the thing, but I find myself reluctant to apply that principle to this case. I think it's bad that we keep adding functions to libpq and sometimes changing the behavior of existing functions and never, ever bumping the libpq .so version. I've seen that cause real, practical problems. It means for example that you can't make an RPM depend on libpq.so.5 and expect that to do anything meaningful -- every version going back forever is version 5, even if it doesn't contain the functions (or other behavior changes) that are needed for some program compiled against a newer version of PostgreSQL to work. But the wire protocol changes very slowly, and I think that is a difference that actually matters quite a bit here. Broadly speaking, I can use a psq > To take it to the extreme: I think we should get to a state, where if > we bump the protocol version at the client and server side without > actually making any protocol changes, everything should continue to > work fine. If we'd do that right now, then libpq wouldn't be able to > connect to old postgres versions anymore. I think I agree with this, but it seems like a bootstrapping problem and nothing more. > > > I'm not sure what I think about this. Do we need these new GUCs to be > > both PGC_PROTOCOL *and also* live in a separate namespace? I see the > > need for the former pretty clearly: if these kinds of things are to be > > part of the GUC system (which wasn't my initial bias, but whatever) > > then they need to have some important behavioral differences from > > other GUCs and so we need a flag to signal that. But what problem are > > we solving by also giving them special-looking names, and are we sure > > we wouldn't rather solve that problem some other way? > > Clients might want to allow the user of the client to change regular > parameters using ParameterSet (e.g. so that a connection pooler can > intercept those ParameterSet messages and change its own behaviour if > the parameter name is pgbouncer.pool_mode). But they wouldn't want a > user to set any parameters that change the wire-protocol this way. And > because an old client might connect to a new server a simple > hard-coded list of parameters at the client side is not sufficient. > > I can see two ways around this: > 1. Using a well-known prefix or namespace for parameters that change > the wire protocol. (exact prefix to be bikeshedded on) > 2. Using a hard-coded list at the client AND disallow changing > PGC_PROTOCOL parameters at the server if the negotiated protocol > version is lower than the version this parameter was introduced in AND > bump the protocol version whenever we add a new PGC_PROTOCOL > parameter. > > I think 1 is easier to implement at the client side, as it only > requires a prefix comparison instead of keeping track of a list. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
[ Hit send too early, sorry. ] On Mon, Apr 15, 2024 at 1:43 PM Robert Haas <robertmhaas@gmail.com> wrote: > But the wire protocol changes very slowly, and I think that is a > difference that actually matters quite a bit here. Broadly speaking, I > can use a psq ...a psql that I just built today to talk to a server from many years ago, and everything is fine. Sure, there are some marginal wire protocol changes around the edges, but not in places that are going to really affect the ability of psql to communicate. But I wouldn't try to run a psql built against 16 with a libpq even one major release old, and the other direction (older psql, newer libpq) also carries some (albeit fewer) risks. So the two situations aren't really entirely comparable, I feel. I don't quite know what to make of that as a practical matter: surely it can't be right to use protocol version 3.0 to refer to a bunch of different things. But at the same time, surely we don't want clients to start panicking and bailing out when everything would have been fine. > > To take it to the extreme: I think we should get to a state, where if > > we bump the protocol version at the client and server side without > > actually making any protocol changes, everything should continue to > > work fine. If we'd do that right now, then libpq wouldn't be able to > > connect to old postgres versions anymore. > > I think I agree with this, but it seems like a bootstrapping problem > and nothing more. That is, once we figure out how we want backward compatibility to work in general, I think we'll probably get pretty close to the state you want here pretty quickly. > > Clients might want to allow the user of the client to change regular > > parameters using ParameterSet (e.g. so that a connection pooler can > > intercept those ParameterSet messages and change its own behaviour if > > the parameter name is pgbouncer.pool_mode). But they wouldn't want a > > user to set any parameters that change the wire-protocol this way. And > > because an old client might connect to a new server a simple > > hard-coded list of parameters at the client side is not sufficient. > > > > I can see two ways around this: > > 1. Using a well-known prefix or namespace for parameters that change > > the wire protocol. (exact prefix to be bikeshedded on) > > 2. Using a hard-coded list at the client AND disallow changing > > PGC_PROTOCOL parameters at the server if the negotiated protocol > > version is lower than the version this parameter was introduced in AND > > bump the protocol version whenever we add a new PGC_PROTOCOL > > parameter. > > > > I think 1 is easier to implement at the client side, as it only > > requires a prefix comparison instead of keeping track of a list. I'm unconvinced that we should let ParameterSet change non-PGC_PROTOCOL GUCs. The pooler can agree on a list of protocol GUCs with the end client that differs from what the server agreed with the pooler - e.g., it can add pgbouncer.pool_mode to the list. But for truly non-protocol GUCs, we have a lot of ways to set those already. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Mon, 15 Apr 2024 at 19:43, Robert Haas <robertmhaas@gmail.com> wrote: > > On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > > I think for clients/drivers, the work would generally be pretty > > minimal. For almost all proposed changes, clients can "support" the > > protocol version update by simply not using the new features, ... > > I mean, I agree if a particular protocol version bump does nothing > other than signal the presence of some optional, ignorable feature, > then it doesn't cause a problem if we force clients to support it. But > that seems a bit like saying that eating wild mushrooms is fine > because some of them aren't poisonous. The point is that if we roll > out two protocol changes, A and B, each of which requires the client > to make some change in order to work with the newer protocol version, > then using version numbers as the gating mechanism requires that the > client can't support the newer of those two changes without also > supporting the older one. Using feature flags doesn't impose that > constraint, which I think is a plus. I think we're in agreement here, i.e. it depends on the situation if a feature flag or version bump is more appropriate. I think the guidelines could be as follows: 1. For protocol changes that require "extremely minimal" work from clients & poolers: bump the protocol version. 2. For "niche" features that require some work from clients and/or poolers: use a protocol parameter feature flag. 3. For anything in between, let's discuss on the thread for that specific protocol change on the tradeoffs. On Mon, 15 Apr 2024 at 19:52, Robert Haas <robertmhaas@gmail.com> wrote: > surely it can't be right to use protocol > version 3.0 to refer to a bunch of different things. But at the same > time, surely we don't want clients to start panicking and bailing out > when everything would have been fine. I feel like the ProtocolVersionNegotiation should make sure people don't panic and bail out. And if not, then feature flags won't help with this either. Because clients would then still bail out if some feature is not supported. > I'm unconvinced that we should let ParameterSet change > non-PGC_PROTOCOL GUCs. The pooler can agree on a list of protocol GUCs > with the end client that differs from what the server agreed with the > pooler - e.g., it can add pgbouncer.pool_mode to the list. But for > truly non-protocol GUCs, we have a lot of ways to set those already. I feel like you're glossing over something fairly important here. How exactly would the client know about pgbouncer.pool_mode? Are you envisioning a list of GUCs which can be changed using ParameterSet, which the server then sends to the client during connection startup (using presumably some new protocol message)? If so, then I feel this same problem still exists. How would the client know which of those GUCs change wire-protocol behaviour and which don't? It still would need a hardcoded list (now including pgbouncer.pool_mode and maybe more) of things that a user is allowed to change using ParameterSet. So I think a well-known prefix would still be applicable. To be clear, imho the well-known prefix discussion is separate from the discussion about whether Postgres should throw an ERROR when ParameterSet is used to change any non-PGC_PROTOCOL GUC. I'd be fine with disallowing that if that seems better/safer/clearer to you (although I'd love to hear your exact concerns about this). But I'd still want a well-known prefix for protocol parameters. Because that prefix is not for the benefit of the server, it's for the benefit of the client and pooler. So the client/pooler can error if any dangerous GUC is being changed, because the server would accept it and change the wire-protocol accordingly.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Dave Cramer
Дата:
On Mon, 15 Apr 2024 at 15:38, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Mon, 15 Apr 2024 at 19:43, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio <me@jeltef.nl> wrote:
> > I think for clients/drivers, the work would generally be pretty
> > minimal. For almost all proposed changes, clients can "support" the
> > protocol version update by simply not using the new features, ...
>
> I mean, I agree if a particular protocol version bump does nothing
> other than signal the presence of some optional, ignorable feature,
> then it doesn't cause a problem if we force clients to support it. But
> that seems a bit like saying that eating wild mushrooms is fine
> because some of them aren't poisonous. The point is that if we roll
> out two protocol changes, A and B, each of which requires the client
> to make some change in order to work with the newer protocol version,
> then using version numbers as the gating mechanism requires that the
> client can't support the newer of those two changes without also
> supporting the older one. Using feature flags doesn't impose that
> constraint, which I think is a plus.
I think we're in agreement here, i.e. it depends on the situation if a
feature flag or version bump is more appropriate. I think the
guidelines could be as follows:
1. For protocol changes that require "extremely minimal" work from
clients & poolers: bump the protocol version.
2. For "niche" features that require some work from clients and/or
poolers: use a protocol parameter feature flag.
3. For anything in between, let's discuss on the thread for that
specific protocol change on the tradeoffs.
My first thought here is that all of the above is subjective and we will end up discussing all of the above.
No real argument just an observation.
On Mon, 15 Apr 2024 at 19:52, Robert Haas <robertmhaas@gmail.com> wrote:
> surely it can't be right to use protocol
> version 3.0 to refer to a bunch of different things. But at the same
> time, surely we don't want clients to start panicking and bailing out
> when everything would have been fine.
I feel like the ProtocolVersionNegotiation should make sure people
don't panic and bail out. And if not, then feature flags won't help
with this either. Because clients would then still bail out if some
feature is not supported.
I don't think a client should ever bail out. They may not support something but IMO bailing out is not an option.
Dave
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Mon, 15 Apr 2024 at 21:47, Dave Cramer <davecramer@gmail.com> wrote: >> On Mon, 15 Apr 2024 at 19:52, Robert Haas <robertmhaas@gmail.com> wrote: >> > surely it can't be right to use protocol >> > version 3.0 to refer to a bunch of different things. But at the same >> > time, surely we don't want clients to start panicking and bailing out >> > when everything would have been fine. >> >> I feel like the ProtocolVersionNegotiation should make sure people >> don't panic and bail out. And if not, then feature flags won't help >> with this either. Because clients would then still bail out if some >> feature is not supported. > > I don't think a client should ever bail out. They may not support something but IMO bailing out is not an option. On Thu, 18 Apr 2024 at 21:01, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Apr 18, 2024 at 1:49 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > IMHO that means that we should also bump the protocol version for this > > change, because it's changing the wire protocol by adding a new > > parameter format code. And it does so in a way that does not depend on > > the new protocol extension. > > I think we're more or less covering the same ground we did on the > other thread here -- in theory I don't love the fact that we never > bump the protocol version when we change stuff, but in practice if we > start bumping it every time we do anything I think it's going to just > break a bunch of stuff without any real benefit. (the second quoted message comes from Peter his column encryption thread, but responding here to keep this discussion in one place) I really don't understand what exactly you're worried about. What do you expect will break when bumping the protocol version? As Dave said, clients should never bail out due to protocol version differences. When the server supports a lower version than the client, the client should disable certain features because it gets the ProtocolVersionNegotiation message. This is also true if we don't bump the version. Negotiating a lower version actually makes it clearer for the client what features to disable. Using the reported postgres version for this might not, because a connection pooler in the middle might not support the features that the client wants and thus throw an error (e.g. due to the client sending unknown messages) even if the backing Postgres server would support these features. Not to mention non-postgresql servers that implement the PostgreSQL protocol (of which there are more and more). When the server supports a higher version, the client never even notices this because the server will silently accept and only enable the features of the lower version. So this could never cause breakage, as from the client's perspective the server didn't bump their protocol version. So, I don't understand why you seem to view bumping the protocol version with so much negativity. We're also bumping PG versions every year. Afaik people only like that, partially because it's immediately clear that certain features (e.g. MERGE) are not supported when connecting to older servers. To me the same is true for bumping the protocol version. There are no downsides to bumping it, only upsides.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Apr 18, 2024 at 3:34 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > I really don't understand what exactly you're worried about. What do > you expect will break when bumping the protocol version? As Dave said, > clients should never bail out due to protocol version differences. Sure, and I should never forget to take out the trash or mow the lawn. > So, I don't understand why you seem to view bumping the protocol > version with so much negativity. We're also bumping PG versions every > year. Afaik people only like that, partially because it's immediately > clear that certain features (e.g. MERGE) are not supported when > connecting to older servers. To me the same is true for bumping the > protocol version. There are no downsides to bumping it, only upsides. I see it the exact opposite way around. If we go with Peter's approach, every driver that supports his feature will work perfectly, and every driver that doesn't will work exactly as it does today. The risk of breaking anything is as near to zero as human developers can reasonably hope to achieve. Nobody who doesn't care about the feature will have to lift a single finger, today, tomorrow, or ever. That's absolutely brilliant. If we instead go with your approach, then anyone who wants to support 3.2 when it materializes will have to also support 3.1, which means they have to support this feature. That's not a terrible burden, but it's not a necessary one either. Also, even just 3.1 is going to break something for somebody. There's just no way that we've left the protocol version unchanged for this long and the first change we make doesn't cause some collateral damage. Sure, those are minor downsides in the grand scheme of things. But AFAICS the only downside of Peter's approach that you've alleged is that doesn't involve bumping the version number. Of course, if bumping the version number is an intrinsic good, then no further justification is required, but I don't buy that. I do not believe that users or maintainers will throw us a pizza party when they find out that we've changed the version number. There's no reason for anyone to be happy about that for its own sake. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 18 Apr 2024 at 22:17, Robert Haas <robertmhaas@gmail.com> wrote: > If we go with Peter's approach, every driver that supports his feature > will work perfectly, and every driver that doesn't will work exactly > as it does today. The risk of breaking anything is as near to zero as > human developers can reasonably hope to achieve. Nobody who doesn't > care about the feature will have to lift a single finger, today, > tomorrow, or ever. That's absolutely brilliant. > > If we instead go with your approach, then anyone who wants to support > 3.2 when it materializes will have to also support 3.1, which means > they have to support this feature. To clarify: My proposed approach is to use a protocol extension parameter for to enable the new messages that the server can send (like Peter is doing now). And **in addition to that** gate the new Bind format type behind a feature switch. There is literally nothing clients will have to do to "support" that feature (except for requesting a higher version protocol). Your suggestion of not bumping the version but still allowing the new format type on version 3.0 doesn't have any advantage afaict, except secretly hiding from any pooler in the middle that such a format type might be sent. > Also, even just 3.1 is going to break > something for somebody. There's just no way that we've left the > protocol version unchanged for this long and the first change we make > doesn't cause some collateral damage. Sure, but the exact same argument holds for protocol extension parameters. We've never set them, so they are bound to break something the first time. My whole point is that once we bite that bullet, the next protocol parameters and protocol version bumps won't cause such breakage. > Sure, those are minor downsides in the grand scheme of things. But > AFAICS the only downside of Peter's approach that you've alleged is > that doesn't involve bumping the version number. Of course, if bumping > the version number is an intrinsic good, then no further justification > is required, but I don't buy that. I do not believe that users or > maintainers will throw us a pizza party when they find out that we've > changed the version number. There's no reason for anyone to be happy > about that for its own sake. As a connection pooler maintainer I would definitely love it if every protocol change required either a protocol version parameter or a protocol version bump. That way I can easily check every release if the protocol changed by looking at two things, instead of diffing the protocol docs for some tiny "supposedly irrelevant" change was made.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 18 Apr 2024 at 23:36, Jelte Fennema-Nio <me@jeltef.nl> wrote: > To clarify: My proposed approach is to use a protocol extension > parameter for to enable the new messages that the server can send > (like Peter is doing now). And **in addition to that** gate the new > Bind format type behind a feature switch. ugh, correction: gate the new Bind format type behind a **protocol bump**
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Apr 18, 2024 at 5:36 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > To clarify: My proposed approach is to use a protocol extension > parameter for to enable the new messages that the server can send > (like Peter is doing now). And **in addition to that** gate the new > Bind format type behind a feature switch. There is literally nothing > clients will have to do to "support" that feature (except for > requesting a higher version protocol). Your suggestion of not bumping > the version but still allowing the new format type on version 3.0 > doesn't have any advantage afaict, except secretly hiding from any > pooler in the middle that such a format type might be sent. That's a fair point, but I'm still not seeing much practical advantage. It's unlikely that a client is going to set a random bit in a format parameter for no reason. > As a connection pooler maintainer I would definitely love it if every > protocol change required either a protocol version parameter or a > protocol version bump. That way I can easily check every release if > the protocol changed by looking at two things, instead of diffing the > protocol docs for some tiny "supposedly irrelevant" change was made. Perhaps this is the root of our disagreement, or at least part of it. I completely agree that it is important for human beings to be able to understand whether, and how, the wire protocol has changed from one release to another. I think it would be useful to document that, and maybe some agreement to start actually bumping the version number would come out of that, either immediately or eventually. But I don't think bumping the protocol version first is going to help anything. If you know that something has changed at least one time in the release, you still have to figure out what it was, and whether there were any more of them that, presumably, would not bump the protocol version because there would be no good reason to do that more than once per major release. Not only that, but it's entirely possible that someone could fail to realize that they were supposed to bump the protocol version, or have some reason not to do it in a particular instance, so even if there are no bumps at all in a particular release cycle, that doesn't prove that there are no changes that you would have liked to know about. Said differently, I think bumping the protocol version should be, first and foremost, a way of telling the computer on the end of the connection something that it needs to know. There is a separate problem of making sure that human maintainers know what they need to know, and I think we're doing that quite poorly right now, but I think you might be conflating those two problems a bit. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Mon, 22 Apr 2024 at 16:26, Robert Haas <robertmhaas@gmail.com> wrote: > That's a fair point, but I'm still not seeing much practical > advantage. It's unlikely that a client is going to set a random bit in > a format parameter for no reason. I think you're missing an important point of mine here. The client wouldn't be "setting a random bit in a format parameter for no reason". The client would decide it is allowed to set this bit, because the PG version it connected to supports column encryption (e.g. PG18). But this completely breaks protocol and application layer separation. It doesn't seem completely outside of the realm of possibility for a pooler to gather some statistics on the amount of Bind messages that use text vs binary query parameters. That's very easily doable now, while looking only at the protocol layer. If a client then sets the new format parameter bit, this pooler could then get confused and close the connection. > Perhaps this is the root of our disagreement, or at least part of it. > I completely agree that it is important for human beings to be able to > understand whether, and how, the wire protocol has changed from one > release to another. I think this is partially the reason for our disagreement, but I think there are at least two other large reasons: 1. I strongly believe minor protocol version bumps after the initial 3.1 one can be made painless for clients/poolers (so the ones to 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not having to worry about breaking TLS 1.2 communication. Once clients and poolers implement version negotiation support for 3.1, there's no reason for version negation support to work for 3.0 and 3.1 to then suddenly break on the 3.2 bump. To be clear, I'm talking about the act of bumping the version here, not the actual protocol changes. So assuming zero/near-zero client implementation effort for the new features (like never setting the newly supported bit in a format parameter), then bumping the protocol version for these new features can never have negative consequences. 2. I very much want to keep a clear split between the protocol layer and the application layer of our communication. And these layers merge whenever (like you say) "the wire protocol has changed from one release to another", but no protocol version bump or protocol extension is used to indicate that. When that happens the only way for a client to know what valid wire protocol messages are according to the server, is by checking the server version. This completely breaks the separation between layers. So, while checking the server version indeed works for direct client to postgres communication, it starts to break down whenever you put a pooler inbetween (as explained in the example earlier in this email). And it breaks down even more when connecting to servers that implement the Postgres wire protocol, but are not postgres at all, like CockroachDB. Right now libpq and other postgres drivers can be used to talk to these other servers and poolers, but if we start mixing protocol and application layer stuff then eventually that will stop being the case. Afaict from your responses, you disagree with 1. However, it's not at all clear to me what exact problems you're worried about. It sounds like you don't know either, and it's more that you're worried about things breaking for not yet known reasons. I hoped to take away/reduce those worries using some arguments in a previous email (quoted below), but you didn't respond to those arguments, so I'm not sure if they were able to change your mind. On Thu, 18 Apr 2024 at 21:34, Jelte Fennema-Nio <me@jeltef.nl> wrote: > When the server supports a lower version than the client, the client > should disable certain features because it gets the > ProtocolVersionNegotiation message. This is also true if we don't bump > the version. Negotiating a lower version actually makes it clearer for > the client what features to disable. Using the reported postgres > version for this might not, because a connection pooler in the middle > might not support the features that the client wants and thus throw an > error (e.g. due to the client sending unknown messages) even if the > backing Postgres server would support these features. Not to mention > non-postgresql servers that implement the PostgreSQL protocol (of > which there are more and more). > > When the server supports a higher version, the client never even > notices this because the server will silently accept and only enable > the features of the lower version. So this could never cause breakage, > as from the client's perspective the server didn't bump their protocol > version.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Mon, Apr 22, 2024 at 5:19 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > On Mon, 22 Apr 2024 at 16:26, Robert Haas <robertmhaas@gmail.com> wrote: > > That's a fair point, but I'm still not seeing much practical > > advantage. It's unlikely that a client is going to set a random bit in > > a format parameter for no reason. > > I think you're missing an important point of mine here. The client > wouldn't be "setting a random bit in a format parameter for no > reason". The client would decide it is allowed to set this bit, > because the PG version it connected to supports column encryption > (e.g. PG18). But this completely breaks protocol and application layer > separation. I can't see what the problem is here. If the client is connected to a database that contains encrypted columns, and its response to seeing an encrypted column is to set this bit, that's fine and nothing should break. If a client doesn't know about encrypted columns and sets that bit at random, that will break things, and formally I think that's a risk, because I don't believe we document anywhere that you shouldn't set unused bits in the format mask. But practically, it's not likely. (And also, maybe we should document that you shouldn't do that.) > It doesn't seem completely outside of the realm of possibility for a > pooler to gather some statistics on the amount of Bind messages that > use text vs binary query parameters. That's very easily doable now, > while looking only at the protocol layer. If a client then sets the > new format parameter bit, this pooler could then get confused and > close the connection. Right, this is the kind of risk I was worried about. I think it's similar to my example of a client setting an unused bit for no reason and breaking everything. Here, you've hypothesized a pooler that tries to interpret the bit and just errors out when it sees something it doesn't understand. I agree that *formally* this is enough to justify bumping the protocol version, but I think *practically* it isn't, because the incompatibility is so minor as to inconvenience almost nobody, whereas changing the protocol version affects everybody. Let's consider a hypothetical country much like Canada except that there are three official languages rather than two: English, French, and Robertish. Robertish is just like English except that the meanings of the words cabbage and rutabaga are reversed. Shall we mandate that all signs in the country be printed in three languages rather than two? Formally, we ought, because the substantial minority of our hypothetical country that proudly speaks Robertish as their mother tongue will not want to feel that they are second class citizens. But practically, there are very few situations where the differences between the two languages are going to inconvenience anyone. Indeed, the French speakers might be a bit put out if English is effectively represented twice on every sign while their mother tongue is there only once. Of course, people are entitled to organize their countries politically in any way that works for the people who live in them, but as a practical matter, English and Robertish are mutually intelligible. And so here. If someone codes a connection pooler in the way you suppose, then it will break. But, first of all, they probably won't do that, both because it's not particularly likely that someone wants to gather that particular set of statistics and also because erroring out seems like an overreaction. And secondly, let's imagine that we do bump the protocol version and think about whether and how that solves the problem. A client will request from the pooler a version 3.1 connection and the pooler will say, sorry, no can do, I only understand 3.0. So the client will now say, oh ok, no problem, I'm going to refrain from setting that parameter format bit. Cool, right? Well, no, not really. First, now the client application is probably broken. If the client is varying its behavior based on the server's protocol version, that must mean that it cares about accessing encrypted columns, and that means that the bit in question is not an optional feature. So actually, the fact that the pooler can force the client to downgrade hasn't fixed anything at all. Second, if the connection pooler were written to do something other than close the connection, like say mask out the one bit that it knows how to deal with or have an "unknown" bucket to count values that it doesn't recognize, then it wouldn't have needed to care about the protocol version in the first place. It would have been better off not even knowing, because then it wouldn't have forced a downgrade onto the client application for no real reason. Throwing an error wasn't a wrong decision on the part of the person writing the pooler, but there are other things they could have done that would have been less brittle. Third, applications, drivers, and connection poolers now all need to worry about handling downgrades smoothly. If a connection pooler requests a v3.1 connection to the server and gets v3.0, it had better make sure that it only advertises 3.0 to the client. If the client requests v3.0, the pooler had better make sure to either request v3.0 from the server. Or alternatively, the pooler can be prepared to translate between 3.0 and 3.1 wherever that's needed, in either direction. But it's not at all clear what that would look like for something like TCE. Will the pooler arrange to encrypt parameters destined for encrypted tables if the client doesn't do so? Will it arrange to decrypt values coming from encrypted tables if the client doesn't understand encryption? It's possible someone will code that sort of thing, but I bet a lot of people won't bother. In general, I think we'll quickly end up with a bunch of different protocol versions -- say, 3.0 through 3.4 -- but people will thoroughly test with only one or two of them and support for the others will either be buggy because it wasn't tested or work anyway because the differences didn't really matter in the first place. > 1. I strongly believe minor protocol version bumps after the initial > 3.1 one can be made painless for clients/poolers (so the ones to > 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not > having to worry about breaking TLS 1.2 communication. Once clients and > poolers implement version negotiation support for 3.1, there's no > reason for version negation support to work for 3.0 and 3.1 to then > suddenly break on the 3.2 bump. To be clear, I'm talking about the act > of bumping the version here, not the actual protocol changes. So > assuming zero/near-zero client implementation effort for the new > features (like never setting the newly supported bit in a format > parameter), then bumping the protocol version for these new features > can never have negative consequences. I do like the idea of being able to introduce new versions without breaking things, but I think that if the TLS folks bumped the protocol version for something as minor as what we're talking about here, there would quickly be so many TLS versions that the result would be unmanageable. I suspect that they either never make small changes and batch everything up for the next rev, or they slip small changes into existing protocol versions as I propose that we do here. I have zero objection to bumping the protocol version when there is a real question of mutual intelligibility, and zero objection to trying to reduce friction around version bumps. But my current view, which I reserve the right to revise at a later time, is that a change that 99.99+% of people can safely ignore is not a sufficient reason for a version bump. > 2. I very much want to keep a clear split between the protocol layer > and the application layer of our communication. And these layers merge > whenever (like you say) "the wire protocol has changed from one > release to another", but no protocol version bump or protocol > extension is used to indicate that. When that happens the only way for > a client to know what valid wire protocol messages are according to > the server, is by checking the server version. This completely breaks > the separation between layers. So, while checking the server version > indeed works for direct client to postgres communication, it starts to > break down whenever you put a pooler inbetween (as explained in the > example earlier in this email). And it breaks down even more when > connecting to servers that implement the Postgres wire protocol, but > are not postgres at all, like CockroachDB. Right now libpq and other > postgres drivers can be used to talk to these other servers and > poolers, but if we start mixing protocol and application layer stuff > then eventually that will stop being the case. In practice, it's already the case. If such databases don't share code with PostgreSQL, it seems impossible that the replication subprotocol works in any meaningful way. It seems very likely that there are other dark corners of the protocol where things don't work either. And TCE will be another one, but bumping the protocol version doesn't fix that. I kind of feel bad arguing so much about this - I don't think the urge to bump the protocol version when we change the protocol is a bad one in concept. And it sounds like you've done more work with software that cares about the protocol outside of PostgreSQL itself than I have. So maybe you're right and I'm all wet. But I can't understand why you don't see practical problems with frequent version bumps. It's not just about the one-time effort of getting everything that doesn't currently understand how to negotiate a version to do so. It's about how everyone acts on that information, or doesn't, and whether the end result of all of those individual decisions is better or worse for the community as a whole. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Champion
Дата:
On Mon, Apr 22, 2024 at 2:20 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > 1. I strongly believe minor protocol version bumps after the initial > 3.1 one can be made painless for clients/poolers (so the ones to > 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not > having to worry about breaking TLS 1.2 communication. Apologies for focusing on a single portion of your argument, but this claim in particular stuck out to me. To my understanding, IETF worried a _lot_ about breaking TLS 1.2 implementations with the TLS 1.3 change, to the point that TLS 1.3 clients and servers advertise themselves as TLS 1.2 and sneak the actual version used into a TLS extension (roughly analogous to the _pq_ stuff). I vaguely recall that the engineering work done for that update was pretty far from painless. --Jacob
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Tue, 23 Apr 2024 at 17:03, Robert Haas <robertmhaas@gmail.com> wrote: > If a client doesn't know about encrypted columns and sets that > bit at random, that will break things, and formally I think that's a > risk, because I don't believe we document anywhere that you shouldn't > set unused bits in the format mask. But practically, it's not likely. > (And also, maybe we should document that you shouldn't do that.) Currently Postgres errors out when you set anything other than 0 or 1 in the format field. And it's already documented that these are the only allowed values: "Each must presently be zero (text) or one (binary)." > Let's consider a hypothetical country much like Canada except that > there are three official languages rather than two: English, French, > and Robertish. I don't really understand the point you are trying to make with this analogy. Sure an almost equivalent unused language maybe shouldn't be put on the signs, but having two different names (aka protocol versions) for English and Robertish seems quite useful to avoid confusion. > And so here. If someone codes a connection pooler in the way you > suppose, then it will break. But, first of all, they probably won't do > that, both because it's not particularly likely that someone wants to > gather that particular set of statistics and also because erroring out > seems like an overreaction. Is it really that much of an overreaction? Postgres happily errors on any weirdness in the protocol including unknown format codes. Why wouldn't a pooler/proxy in the middle be allowed to do the same? The pooler has no clue what the new format means, maybe it requires some session state to be passed on to the server to be interpreted correctly. The pooler would be responsible for syncing that session state but might not have synced it because it doesn't know that the format code requires that. An example of this would be a compressed value of which the compression algorithm is configured using a GUC. Thus the pooler being strict in what it accepts would be a good way of notifying the client that the pooler needs to be updated (or the feature not used). But I do agree that it seems quite unlikely that a pooler would implement it like that though. However, a pooler logging a warning seems not that crazy. And in that case the connection might not be closed, but the logs would be flooded. > And secondly, let's imagine that we do > bump the protocol version and think about whether and how that solves > the problem. A client will request from the pooler a version 3.1 > connection and the pooler will say, sorry, no can do, I only > understand 3.0. So the client will now say, oh ok, no problem, I'm > going to refrain from setting that parameter format bit. Cool, right? > > Well, no, not really. First, now the client application is probably > broken. If the client is varying its behavior based on the server's > protocol version, that must mean that it cares about accessing > encrypted columns, and that means that the bit in question is not an > optional feature. So actually, the fact that the pooler can force the > client to downgrade hasn't fixed anything at all. It seems quite a lot nicer if the client can safely fallback to not writing data using the new format code, instead of getting an error from the pooler/flooding the pooler logs/having the pooler close the connection. > Third, applications, drivers, and connection poolers now all need to > worry about handling downgrades smoothly. If a connection pooler > requests a v3.1 connection to the server and gets v3.0, it had better > make sure that it only advertises 3.0 to the client. This seems quite straightforward to solve from a pooler perspective: 1. Connect to the server first to find out what the maximum version is that it support 2. If a client asks for a higher version, advertise the server version > If the client > requests v3.0, the pooler had better make sure to either request v3.0 > from the server. Or alternatively, the pooler can be prepared to > translate between 3.0 and 3.1 wherever that's needed, in either > direction. But it's not at all clear what that would look like for > something like TCE. Will the pooler arrange to encrypt parameters > destined for encrypted tables if the client doesn't do so? Will it > arrange to decrypt values coming from encrypted tables if the client > doesn't understand encryption? This is a harder problem, and indeed one I hadn't considered much. From a pooler perspective I really would like to be able to have just one pool of connections to the server all initially connected with the 3.1 protocol and use those 3.1 server connections for clients that use the 3.1 protocol but also for clients that use the 3.0 protocol. Creating separate pools of server connections for different protocol versions is super annoying to manage for operators (e.g. how should these pools be sized). One of the reasons I'm proposing the ParameterSet message is to avoid this problem for protocol parameters e.g. PgBouncer could then set TCE=off on server connection with TCE=on, just before it hands it to a client with TCE=off. I can think of a few ways of solving this issue for protocol versions: 1. Introduce a ProtocolVersionSet message, which could be send on handoff to downgrade/upgrade the protocol version at the postgres side 2. Feature-flag all protocol changes behind protocol parameters, so ParameterSet can be used to enable/disable everything 3. Feature-flag most protocol changes using protocol parameters, but allow protocol changes to be made using a version bump when the server responds the exact same way to all messages a client can send using the previous protocol version. So don't allow a protocol version bump to add extra fields to existing messages, nor introduce new message types that are not sent as a 1-to-1 response to a new message type sent by the client, nor send existing message types more often than was expected in the previous protocol version. I would prefer the 3rd option. 1 seems strange when considering protocol changes that only impact the handshake, such as lengthening the cancel key[1], also it requires adding yet another message type. And 2 seems an overly strict version of 3. [1]: https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi > It's possible someone will code that > sort of thing, but I bet a lot of people won't bother. In general, I > think we'll quickly end up with a bunch of different protocol versions > -- say, 3.0 through 3.4 -- but people will thoroughly test with only > one or two of them and support for the others will either be buggy > because it wasn't tested or work anyway because the differences didn't > really matter in the first place. I think this is overly pessimistic. I'm pretty sure clients and poolers will want to support multiple protocol versions, to be able to talk to old clients/servers. I do think we should make supporting multiple versions as easy as possible though. > In practice, it's already the case. If such databases don't share code > with PostgreSQL, it seems impossible that the replication subprotocol > works in any meaningful way. I think the fact that the replication subprotocol is gated behind the "replication=true" StartupMessage parameter makes it very easy to check for support. > It seems very likely that there are other > dark corners of the protocol where things don't work either. And TCE > will be another one, but bumping the protocol version doesn't fix > that. To be clear, gating the new TCE format code behind a protocol version bump is **not only useful detect non-support for TCE** in such other servers. But it can also be used to detect that this other server actually **does support TCE**. If the postgresql server version is used to indicate such support, then these non-Postgres servers now need to pretend that they actually are Postgres servers by sending the same version number in the server_version GUC ParameterStatus message. > But I can't understand > why you don't see practical problems with frequent version bumps. It's > not just about the one-time effort of getting everything that doesn't > currently understand how to negotiate a version to do so. It's about > how everyone acts on that information, or doesn't, and whether the end > result of all of those individual decisions is better or worse for the > community as a whole. I do see practical problems. But I see the exact same practical problems when encoding new protocol feature support in the postgres server version number instead of the protocol version number. But encoding protocol feature support in the server version introduces other issues, such as not being able to detect that some non-Postgres server supports TCE.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Tue, 23 Apr 2024 at 19:39, Jacob Champion <jacob.champion@enterprisedb.com> wrote: > > On Mon, Apr 22, 2024 at 2:20 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > > 1. I strongly believe minor protocol version bumps after the initial > > 3.1 one can be made painless for clients/poolers (so the ones to > > 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not > > having to worry about breaking TLS 1.2 communication. > > Apologies for focusing on a single portion of your argument, but this > claim in particular stuck out to me. To my understanding, IETF worried > a _lot_ about breaking TLS 1.2 implementations with the TLS 1.3 > change, to the point that TLS 1.3 clients and servers advertise > themselves as TLS 1.2 and sneak the actual version used into a TLS > extension (roughly analogous to the _pq_ stuff). I vaguely recall that > the engineering work done for that update was pretty far from > painless. My bad... I guess TLS 1.3 was a bad example, due to it changing the handshake itself so significantly.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 5 Apr 2024 at 18:30, Dave Cramer <davecramer@gmail.com> wrote: > On Fri, 5 Apr 2024 at 12:09, Jelte Fennema-Nio <me@jeltef.nl> wrote: >> I'll take a look at redesigning the protocol parameter stuff. To work >> with dedicated functions instead. > > +1 It's been a while, but I now actually took the time to look into this. And I ran into a problem that I'd like to get some feedback on before continuing the implementation: If we're not setting the protocol parameter in the StartupMessage, there's currently no way for us to know if the protocol parameter is supported by the server. If protocol parameters were unchangable then that would be fine, but the whole point of introducing ParameterSet is to make it possible to change protocol parameters on an existing connection. Having the function SupportsProtocolCompression return false, even though you can enable compression just fine, only because we didn't ask for compression when connecting seems quite silly and confusing. I see five ways around this problem and would love some feedback on which you think is best (or if you can think of any other/better ones): 1. Have protocol parameters always be GUC_REPORT, so that the presence of a ParameterStatus message during connection startup can be used as a way of detecting support for the protocol parameter. 2. Make libpq always send each known protocol parameter in the StartupMessage to check for their support, even if the connection string does not contain the related parameters (set them to their default value then). Then the non-presence of the parameter in the NegotiateProtocolVersion message can be used reliably to determine support for the feature. We could even disallow changing a protocol parameter at the server side using ParameterSet if it was not requested in the StartupMessage. 3. Very similar to 1, but require explicit user input in the connection string to request the feature on connection startup by having the user explicitly provide its default value. If it's not requested on connection startup assume its unsupported and disallow usage of the feature (even if the server might actually support it). 4. Make SupportsProtocolCompression return a tri-state, SUPPORTED, UNSUPPORTED, UNKNOWN. If it's UNKNOWN people could send a ParameterSet message themselves to check for feature support after connection startup. We could even recognize this and change the state that SupportProtocolCompression function to return SUPPORTED/UNSUPPORTED on future calls according to the server response. 5. Basically the same as 4 but automatically send a ParameterSet message internally when calling SupportsProtocolCompression and the state is UNKNOWN, so we'd only ever return SUPPORTED or UNSUPPORTED. The above options are listed in my order of preference, below some reasoning why: 1 and 2 would increase the bandwidth used during connection handshake slightly for each protocol parameter that we would add, but they have the best user experience IMHO. I slightly prefer 1 over 2 because there is another argument to be made for always having protocol parameters be GUC_REPORT: these parameters change what message types a client can send or receive. So it makes sense to me to have the server tell the client what the current value of such a parameter is. This might not be a strong argument though, because this value would only ever change due to user interaction. But still, one might imagine scenarios where the value that the client sent is not exactly what the server would set the parameter to on receiving that value from the client. e.g. for protocol compression, maybe the client sends a list of prefered compression methods and the server would send a ParameterStatus containing only the specific compression method that it will use when sending messages to the client. 3 seems also an acceptable option to me. While having slightly worse user experience than 2, it allows the user of the client to make the decision if the extra bandwidth during connection startup is worth it to be able to enable the feature later. 4 assumes that we want people to be able to trigger sending ParameterSet messages for every protocol parameter. I'm not sure we'd want to give that ability in all cases. 5 would require SupportsProtocolCompression to also have a non-blocking version, which bloats our API more than I'd like. Also as a user you wouldn't be able to know if SupportsProtocolCompression will do a network request or not. PS. This is only a problem for feature detection for features relying on protocol parameters, feature-support relying on protocol version bumps are easy to detect based on the NegotiateProtocolVersion message.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, May 16, 2024 at 5:22 AM Jelte Fennema-Nio <me@jeltef.nl> wrote: > If we're not setting the protocol parameter in the StartupMessage, > there's currently no way for us to know if the protocol parameter is > supported by the server. If protocol parameters were unchangable then > that would be fine, but the whole point of introducing ParameterSet is > to make it possible to change protocol parameters on an existing > connection. Having the function SupportsProtocolCompression return > false, even though you can enable compression just fine, only because > we didn't ask for compression when connecting seems quite silly and > confusing. You're probably not going to like this answer, but I feel like this is another sign that you're trying to use the protocol extensibility facilities in the wrong way. In my first reply to the thread, I proposed having the client send _pq_.protocol_set=1 in that startup message. If the server accepts that message, then you can send whatever set of message types are associated with that option, which could include messages to list known settings, as well as messages to set them. Alternatively, if we used a wire protocol bump for this, you could request version 3.1 and everything that I just said still applies. In other words, I feel that if you had adopted the design that I proposed back in March, or some variant of it, the problem you're having now wouldn't exist. IMHO, we need to negotiate the language that we're going to use to communicate before we start communicating. We should find out which protocol version we're using, and what protocol options are accepted, based on sending a StartupMessage and receiving a reply. Then, after that, having established a common language, we can do whatever. I think you're trying to meld those two steps into one, which is understandable from the point of view of saving a round trip, but I just don't see it working out well. What we can do in the startup message is extremely limited, because any startup messages we generate can't break existing servers, and also because of the concerns I raised earlier about leaving room for more extension in the future. Once we get past the startup message negotiation, the sky's the limit! -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 16 May 2024 at 17:29, Robert Haas <robertmhaas@gmail.com> wrote: > You're probably not going to like this answer, but I feel like this is > another sign that you're trying to use the protocol extensibility > facilities in the wrong way. In my first reply to the thread, I > proposed having the client send _pq_.protocol_set=1 in that startup > message. If the server accepts that message, then you can send > whatever set of message types are associated with that option, which > could include messages to list known settings, as well as messages to > set them. Alternatively, if we used a wire protocol bump for this, you > could request version 3.1 and everything that I just said still > applies. In other words, I feel that if you had adopted the design > that I proposed back in March, or some variant of it, the problem > you're having now wouldn't exist. I don't really understand the benefit of your proposal over option 2 that I proposed. Afaict you're proposing that for e.g. compression we first set _pq_.supports_compression=1 in the StartupMessage and use that to do feature detection, and then after we get the response we send ParameterSet("compression", "gzip"). To me this is pretty much identical to option 2, except that it introduces an extra round trip for no benefit (as far as I can see). Why not go for option 2 and send _pq_.compression=gzip in the StartupMessage directly. > IMHO, we need to negotiate the language that we're going to use to > communicate before we start communicating. We should find out which > protocol version we're using, and what protocol options are accepted, > based on sending a StartupMessage and receiving a reply. Then, after > that, having established a common language, we can do whatever. I > think you're trying to meld those two steps into one, which is > understandable from the point of view of saving a round trip, but I > just don't see it working out well. I think not increasing the number of needed round trips in the startup of a connection is extremely important. I think it's so important that I honestly don't think we should merge a protocol change that introduces an extra round trip without a VERY good reason, and this round trip should only be needed when actually using the feature. > What we can do in the startup > message is extremely limited, because any startup messages we generate > can't break existing servers, and also because of the concerns I > raised earlier about leaving room for more extension in the future. > Once we get past the startup message negotiation, the sky's the limit! Sure, what we can do in the StartupMessage is extremely limited, but what it does allow is passing arbitrary key value pairs to the server. But by only using _pq_.feature_name=1, we're effectively only using the key part of the key value pair. Limiting ourselves even more, by throwing half of our communication channel away, seems like a bad idea to me. But maybe I'm just not understanding the problem you're seeing with using the value too.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, May 16, 2024 at 12:09 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > I don't really understand the benefit of your proposal over option 2 > that I proposed. Afaict you're proposing that for e.g. compression we > first set _pq_.supports_compression=1 in the StartupMessage and use > that to do feature detection, and then after we get the response we > send ParameterSet("compression", "gzip"). To me this is pretty much > identical to option 2, except that it introduces an extra round trip > for no benefit (as far as I can see). Why not go for option 2 and send > _pq_.compression=gzip in the StartupMessage directly. Ugh, it's so hard to communicate clearly about this stuff. I didn't really have any thought that we'd ever try to handle something as complicated as compression using ParameterSet. I tend to agree that for compression I'd like to see the startup packet contain more than _pq_.compression=1, but I'm not sure what would happen after that exactly. If the client asks for _pq_.compression=lz4 and the server tells the client that it doesn't understand _pq_.compression at all, then everybody's on the same page: no compression. But, if the server understands the option but isn't OK with the proposed value, what happens then? Does it send a NegotiateCompressionType message after the NegotiateProtocolVersion, for example? That seems like it could lead to the client having to be prepared for a lot of NegotiateX messages somewhere down the road. I think at some point in the past we had discussed having the client list all the algorithms it supported in the argument to _pq_.compression, and then the server would respond with the algorithm it wanted use, or maybe a list of algorithms that it could allow, and then we'd go from there. But I'm not entirely sure if that's the right idea, either. Changing compression algorithms in mid-stream is tricky, too. If I tell the server "hey, turn on server-to-client compression!" then I need to be able to identify where exactly that happens. Any messages already sent by the server and not yet processed by me, or any messages sent after that but before the server handles my request, are going to be uncompressed. Then, at some point, I'll start getting compressed data. If the compressed data is framed inside some message type created for that purpose, like I get a CompressedMessage message and then I decompress to get the actual message, this is simpler to manage. But even then, it's tricky if the protocol shifts. If I tell the server, you know what, gzip was a bad choice, I want lz4, I'll need to know where the switch happens to be able to decompress properly. I don't know if we want to support changing compression algorithms in mid-stream. I don't think there's any reason we can't, but it might be a bunch of work for something that nobody really cares about. Not sure. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Burroughs
Дата:
On Thu, May 16, 2024 at 6:57 AM Robert Haas <robertmhaas@gmail.com> wrote: > > Ugh, it's so hard to communicate clearly about this stuff. I didn't > really have any thought that we'd ever try to handle something as > complicated as compression using ParameterSet. I tend to agree that > for compression I'd like to see the startup packet contain more than > _pq_.compression=1, but I'm not sure what would happen after that > exactly. If the client asks for _pq_.compression=lz4 and the server > tells the client that it doesn't understand _pq_.compression at all, > then everybody's on the same page: no compression. But, if the server > understands the option but isn't OK with the proposed value, what > happens then? Does it send a NegotiateCompressionType message after > the NegotiateProtocolVersion, for example? That seems like it could > lead to the client having to be prepared for a lot of NegotiateX > messages somewhere down the road. > > I think at some point in the past we had discussed having the client > list all the algorithms it supported in the argument to > _pq_.compression, and then the server would respond with the algorithm > it wanted use, or maybe a list of algorithms that it could allow, and > then we'd go from there. But I'm not entirely sure if that's the right > idea, either. As currently implemented [1], the client sends the server the list of all compression algorithms it is willing to accept, and the server can use one of them. If the server knows what `_pq_.compression` means but doesn't actually support any compression, it will both send the client its empty list of supported algorithms and just never send any compressed messages, and everyone involved will be (relatively) happy. There is a libpq function that a client can use to check what compression is in use if a client *really* doesn't want to continue with the conversation without compression, but 99% of the time I can't see why a client wouldn't prefer to continue using a connection with whatever compression the server supports (or even none) without more explicit negotiation. (Unlike TLS, where automagically picking between using and not using TLS has strange security implications and effects, compression is a convenience feature for everyone involved.) > Changing compression algorithms in mid-stream is tricky, too. If I > tell the server "hey, turn on server-to-client compression!" then I > need to be able to identify where exactly that happens. Any messages > already sent by the server and not yet processed by me, or any > messages sent after that but before the server handles my request, are > going to be uncompressed. Then, at some point, I'll start getting > compressed data. If the compressed data is framed inside some message > type created for that purpose, like I get a CompressedMessage message > and then I decompress to get the actual message, this is simpler to > manage. But even then, it's tricky if the protocol shifts. If I tell > the server, you know what, gzip was a bad choice, I want lz4, I'll > need to know where the switch happens to be able to decompress > properly. > > I don't know if we want to support changing compression algorithms in > mid-stream. I don't think there's any reason we can't, but it might be > a bunch of work for something that nobody really cares about. Not > sure. As the protocol layer is currently designed [1], it explicitly makes it very easy to change/restart compression streams, specifically for this use case (and in particular for the general connection pooler use case). Compressed data is already framed in a `CompressedData` message, and that message has a header byte that corresponds to an enum value for which algorithm is currently in use. Any time the compression stream was restarted by the sender, the first `CompressedData` message will set that byte, and then the client will restart its decompression stream with the chosen algorithm from that point. For `CompressedData` messages that continue using the already-established stream, the byte is simply set to 0. (This is also how the "each side sends a list" form of negotiation is able to work without additional round trips, as the `CompressedData` framing itself communicates which compression algorithm has been selected.) [1] https://www.postgresql.org/message-id/CACzsqT5-7xfbz%2BSi35TBYHzerNX3XJVzAUH9AewQ%2BPp13fYBoQ%40mail.gmail.com -- Jacob Burroughs | Staff Software Engineer E: jburroughs@instructure.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 16 May 2024 at 18:57, Robert Haas <robertmhaas@gmail.com> wrote: > Ugh, it's so hard to communicate clearly about this stuff. I didn't > really have any thought that we'd ever try to handle something as > complicated as compression using ParameterSet. Okay, then it's definitely very hard to communicate clearly about this. Because being able to re-configure compression using ParameterSet is exactly the type of thing I want to be able to do in PgBouncer. Otherwise a connection from PgBouncer to Postgres cannot be handed off to any client, because its compression state cannot be changed on the fly to the state that a client expects, if the first one wants lz4 compression, and a second one wants zstd compression. There's no way the same connection can be reused for both unless you decompress and recompress in pgbouncer, which is expensive to do. Being able to reconfigure the stream to compress the messages in the expected form is much cheaper. > Does it send a NegotiateCompressionType message after > the NegotiateProtocolVersion, for example? That seems like it could > lead to the client having to be prepared for a lot of NegotiateX > messages somewhere down the road. Like Jacob explains, you'd want to allow the client to provide a list of options in order of preference. And then have the server respond with a ParameterStatus message saying what it ended up being. So no new NegotiateXXX messages are needed, as long as we make sure any _pq_.xxx falls back to reporting its default value on failure. This is exactly why I said I prefer option 1 of the options I listed, because we need _pq_.xxx messages to report their current value to the client. To be clear, this is not special to compression. This applies to ALL proposed protocol parameters. The server should fall back to some "least common denominator" if it doesn't understand (part of) the value for the protocol parameter that's provided by the client, possibly falling back to disabling the protocol extension completely. > Changing compression algorithms in mid-stream is tricky, too. If I > tell the server "hey, turn on server-to-client compression!" Yes it is tricky, but it's something that it would need to support imho. And Jacob actually implemented it this way, so I feel like we're discussing a non-problem here. > I don't know if we want to support changing compression algorithms in > mid-stream. I don't think there's any reason we can't, but it might be > a bunch of work for something that nobody really cares about. Again, I guess I wasn't clear at all in my previous emails and/or commit messages. Connection poolers care **very much** about this. Poolers need to be able to re-configure any protocol parameter to be able to pool the same server connection across clients with differently configured protocol parameters. Again: This is the primary reason for me wanting to introduce the ParameterSet message.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, May 16, 2024 at 1:39 PM Jacob Burroughs <jburroughs@instructure.com> wrote: > As currently implemented [1], the client sends the server the list of > all compression algorithms it is willing to accept, and the server can > use one of them. If the server knows what `_pq_.compression` means > but doesn't actually support any compression, it will both send the > client its empty list of supported algorithms and just never send any > compressed messages, and everyone involved will be (relatively) happy. > There is a libpq function that a client can use to check what > compression is in use if a client *really* doesn't want to continue > with the conversation without compression, but 99% of the time I can't > see why a client wouldn't prefer to continue using a connection with > whatever compression the server supports (or even none) without more > explicit negotiation. (Unlike TLS, where automagically picking > between using and not using TLS has strange security implications and > effects, compression is a convenience feature for everyone involved.) This all seems sensible to me. > As the protocol layer is currently designed [1], it explicitly makes > it very easy to change/restart compression streams, specifically for > this use case (and in particular for the general connection pooler use > case). Compressed data is already framed in a `CompressedData` > message, and that message has a header byte that corresponds to an > enum value for which algorithm is currently in use. Any time the > compression stream was restarted by the sender, the first > `CompressedData` message will set that byte, and then the client will > restart its decompression stream with the chosen algorithm from that > point. For `CompressedData` messages that continue using the > already-established stream, the byte is simply set to 0. (This is > also how the "each side sends a list" form of negotiation is able to > work without additional round trips, as the `CompressedData` framing > itself communicates which compression algorithm has been selected.) OK, so you made it so that compressed data is fully self-identifying. Hence, there's no need to worry if something gets changed: the receiver, if properly implemented, can't help but notice. The only downside that I can see to this design is that you only have one byte to identify the compression algorithm, but that doesn't actually seem like a real problem at all, because I expect the number of supported compression algorithms to grow very slowly. I think it would take centuries, possibly millenia, before we started to get short of identifiers. So, cool. But, in your system, how does the client ask the server to switch to a different compression algorithm, or to restart the compression stream? -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Burroughs
Дата:
On Fri, May 17, 2024 at 3:15 AM Robert Haas <robertmhaas@gmail.com> wrote: > > OK, so you made it so that compressed data is fully self-identifying. > Hence, there's no need to worry if something gets changed: the > receiver, if properly implemented, can't help but notice. The only > downside that I can see to this design is that you only have one byte > to identify the compression algorithm, but that doesn't actually seem > like a real problem at all, because I expect the number of supported > compression algorithms to grow very slowly. I think it would take > centuries, possibly millenia, before we started to get short of > identifiers. So, cool. > > But, in your system, how does the client ask the server to switch to a > different compression algorithm, or to restart the compression stream? I was leaving the details around triggering that for this conversation and in that patch just designing the messages in a way that would allow adding the reconfiguration/restarting to be easily added in a backwards-compatible way in a future patch. I would imagine that an explicit `ParameterSet` call that sets `_pq_.connection_compression` (or whatever the implementation details turn out to be) would also trigger a compressor restart, and when restarted it would pick an algorithm/configuration based on the new value of the parameter rather than the one used at connection establishment. -- Jacob Burroughs | Staff Software Engineer E: jburroughs@instructure.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, May 17, 2024 at 1:26 PM Jacob Burroughs <jburroughs@instructure.com> wrote: > I was leaving the details around triggering that for this conversation > and in that patch just designing the messages in a way that would > allow adding the reconfiguration/restarting to be easily added in a > backwards-compatible way in a future patch. I would imagine that an > explicit `ParameterSet` call that sets `_pq_.connection_compression` > (or whatever the implementation details turn out to be) would also > trigger a compressor restart, and when restarted it would pick an > algorithm/configuration based on the new value of the parameter rather > than the one used at connection establishment. Hmm, OK, interesting. I suppose that I thought we were going to handle problems like this by just adding bespoke messages for each case (e.g. CompressionSet). I wasn't thinking it would be practical to make a message whose remit was as general as what it seems like Jelte wants to do with ParameterSet, because I'm not sure everything can be handled that simply. It's not an exact analogy, but when you want to stop the server, it's not enough to say that you want to change from the Running state to the Stopped state. You have to specify which type of shutdown should be used to make the transition. You could even have more complicated cases, where one side says "prepare to do X" and the other side eventually says "OK, I'm prepared" and the first side says "great, now activate X" and the other side eventually says "OK, it's activate, please confirm that you've also activated it from your side" and the first side eventually says "OK, I confirm that". I think the fear that we're going to run into cases where such complex handshaking is necessary is a major reason why I'm afraid of Jelte's ideas about ParameterSet: it seems much more opinionated to me than he seems to think it is. To the extent that I can wrap my head around what Jelte is proposing, and all signs point to that extent being quite limited, I suppose I was thinking of something like his option (2). That is, I assumed that a client would request all the optional features that said client might wish to use at connection startup time. But I can see how that assumption might be somewhat problematic in a connection-pooling environment. Still, at least to me, it seems better than trying to rely on GUC_REPORT. My opinion is (1) GUC_REPORT isn't a particularly well-designed mechanism so I dislike trying to double down on it and (2) trying to mix these protocol-level parameters and the transactional GUCs we have together in a single mechanism seems potentially problematic and (3) I'm still not particularly happy about the idea of making protocol parameters into GUCs in the first place. Perhaps these are all minority positions, but I can't tell you what everyone thinks, only what I think. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 17 May 2024 at 21:24, Robert Haas <robertmhaas@gmail.com> wrote: > I think the > fear that we're going to run into cases where such complex handshaking > is necessary is a major reason why I'm afraid of Jelte's ideas about > ParameterSet: it seems much more opinionated to me than he seems to > think it is. I think that fear is valid, and I agree that we might want to add a bespoke message for cases where ParameterSet is not enough. But as far as I can tell ParameterSet would at least cover all the protocol changes that have been suggested so far. Using an opinionated but limited message type for 90% of the cases and a bespoke message for the last 10% seems much better to me than having a bespoke one for each (especially if currently none of the protocol proposals fall into the last 10%). > To the extent that I can wrap my head around what Jelte is proposing, > and all signs point to that extent being quite limited, I suppose I > was thinking of something like his option (2). That is, I assumed that > a client would request all the optional features that said client > might wish to use at connection startup time. But I can see how that > assumption might be somewhat problematic in a connection-pooling > environment. To be clear, I'd also be totally fine with my option (2). I'm personally slightly leaning towards my option (1), due to the reasons listed before. But connection poolers could request all the protocol extensions at the start just fine (using the default "disabled" value) to check for support. So I think option (2) would probably be the most conservative, i.e. we could always decide that option (1) is fine in some future release. > Still, at least to me, it seems better than trying to > rely on GUC_REPORT. My opinion is (1) GUC_REPORT isn't a particularly > well-designed mechanism so I dislike trying to double down on it I agree that GUC_REPORT is not particularly well designed, currently... But even in its current form it's already a very effective mechanism for connection poolers to find out to which value a specific GUC is set to, and if something similar to patch 0014 would be merged my main gripe with GUC_REPORT would be gone. Tracking GUC settings by using ParameterSet would actually be harder, because if ParameterSet errors at Postgres then the connection pooler would have to roll back its cache of that setting. While with the GUC_REPORT response from Postgres it can simply rely on Postgres telling the pooler that the GUC has changed, even rollbacks are handled correctly this way. > and > (2) trying to mix these protocol-level parameters and the > transactional GUCs we have together in a single mechanism seems > potentially problematic I don't understand what potential problems you're worried about here. Could you clarify? > and (3) I'm still not particularly happy about > the idea of making protocol parameters into GUCs in the first place. Similar to the above: Could you clarify why you're not happy about that? > Perhaps these are all minority positions, but I can't tell you what > everyone thinks, only what I think. I'd love to hear some opinions from others on these design choices. So far it seems like we're the only two that have opinions on this (which seems hard to believe) and our opinions are clearly conflicting. And above all I'd like to move forward with this, be it my way or yours (although I'd prefer my way of course ;) )
Jelte Fennema-Nio <me@jeltef.nl> writes: > On Fri, 17 May 2024 at 21:24, Robert Haas <robertmhaas@gmail.com> wrote: >> Perhaps these are all minority positions, but I can't tell you what >> everyone thinks, only what I think. > I'd love to hear some opinions from others on these design choices. So > far it seems like we're the only two that have opinions on this (which > seems hard to believe) and our opinions are clearly conflicting. And > above all I'd like to move forward with this, be it my way or yours > (although I'd prefer my way of course ;) ) I got around to looking through this thread in preparation for next week's patch review session. I have a couple of opinions to offer: 1. Protocol versions suck. Bumping them is seldom a good answer, and should never be done if you have any finer-grained negotiation mechanism available. My aversion to this is over thirty years old: I learned that lesson from watching the GIF87-to-GIF89 transition mess. Authors of GIF-writing tools tended to take the easy way out and write "GIF89" in the header whether they were actually using any of the new version's features or not. This led to an awful lot of pictures that couldn't be read by available GIF-displaying tools, for no good reason whatsoever. The PNG committee, a couple years later, reacted to that mess by designing PNG to have no version number whatsoever, and yet be extensible in a fine-grained way. (Basically, a PNG file is made up of labeled chunks. If a reader doesn't recognize a particular chunk code, it can still tell whether the chunk is "critical" or not, and thereby decide if it must give up or can proceed while ignoring that chunk.) So overall, I have a strong preference for using the _pq_.xxx mechanism instead of a protocol version bump. I do not believe the latter has any advantage. 2. I share Robert's suspicion of equating protocol parameters with GUCs. The GUC mechanism is quite opinionated and already serves multiple masters. In particular, the fact that GUC settings are normally transactional does not play nice with the way protocol parameters need to behave. Yeah, no doubt you could add another dollop of complexity to guc.c to make parameters work differently from other GUCs, but I think it's the wrong design direction. We should handle protocol parameters with a separate mechanism. It's not, for instance, clear to me that protocol parameters should be exposed at the SQL level at all; but if we don't feel they need to be available via SHOW and pg_settings, what benefit is guc.c really bringing to the table? regards, tom lane
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jacob Champion
Дата:
On Thu, May 23, 2024 at 11:12 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > If a reader doesn't recognize a particular > chunk code, it can still tell whether the chunk is "critical" or not, > and thereby decide if it must give up or can proceed while ignoring > that chunk.) Would it be good to expand on that idea of criticality? IIRC one of Jelte's complaints earlier was that middleware has to know all the extension types anyway, to be able to figure out whether it has to do something about them or not. HTTP has the concept of hop-by-hop vs end-to-end headers for related reasons. --Jacob
Jacob Champion <jacob.champion@enterprisedb.com> writes: > Would it be good to expand on that idea of criticality? IIRC one of > Jelte's complaints earlier was that middleware has to know all the > extension types anyway, to be able to figure out whether it has to do > something about them or not. HTTP has the concept of hop-by-hop vs > end-to-end headers for related reasons. Yeah, perhaps. We'd need to figure out just which classes we need to divide protocol parameters into, and then think about a way for code to understand which class a parameter falls into even when it doesn't specifically know that parameter. That seems possible though. PNG did it with spelling rules for the chunk labels. Here, since we don't yet have any existing _pq_.xxx parameter names, we could maybe say that the names shall follow a pattern like "_pq_.class.param". (That works only if the classes are non-overlapping, an assumption not yet justified by evidence; but we could do something more complicated if we have to.) regards, tom lane
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, May 23, 2024 at 2:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I got around to looking through this thread in preparation for next > week's patch review session. I have a couple of opinions to offer: I agree with these opinions. Independently of that, I'm glad you shared them. I think part of the reason we ended up with the protocol parameters = GUCs thing is because you seemed to be concurring with that approach upthread. I think it was Jelte's idea originally, but I interpreted some of your earlier remarks to be supporting it. I'm not sure whether you've revised your opinion, or just refined it, or whether we misinterpreted your earlier remarks. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > I think part of the reason we ended up with the protocol parameters = > GUCs thing is because you seemed to be concurring with that approach > upthread. I think it was Jelte's idea originally, but I interpreted > some of your earlier remarks to be supporting it. I'm not sure whether > you've revised your opinion, or just refined it, or whether we > misinterpreted your earlier remarks. I don't recall exactly what I thought earlier, but now I think we'd be better off with separate infrastructure. guc.c is unduly complex already. Perhaps there are bits of it that could be factored out and shared, but I bet not a lot. regards, tom lane
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, May 23, 2024 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't recall exactly what I thought earlier, but now I think we'd > be better off with separate infrastructure. guc.c is unduly complex > already. Perhaps there are bits of it that could be factored out > and shared, but I bet not a lot. OK. That seems fine to me, but I bet Jelte is going to disagree. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Fri, 24 May 2024 at 15:28, Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, May 23, 2024 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I don't recall exactly what I thought earlier, but now I think we'd > > be better off with separate infrastructure. guc.c is unduly complex > > already. Perhaps there are bits of it that could be factored out > > and shared, but I bet not a lot. > > OK. That seems fine to me, but I bet Jelte is going to disagree. I indeed disagree. I think the effort needed to make guc.c handle protocol parameters is extremely little. The 0011 patch are all the changes that are needed to achieve that, and that patch only adds 65 additional lines. And only 15 of those 65 lines actually have to do anything somewhat weird, to be able to handle the transactionality discrepancy between protocol parameters and other GUCs. The other 50 lines are (imho) really clean and fit perfectly with the way guc.c is currently structured (i.e. they add PGC_PROTOCOL and PGC_SU_PROTOCOL in a really obvious way) Separating it from the GUC infrastructure will mean we need to duplicate a lot of the infrastructure. Assuming we don't care about SHOW or pg_settings (which I agree are not super important), the things that we would want for protocol parameters to have that guc.c gives us for free are: 1. Reporting the value of the parameter to the client (done using ParameterStatus) 2. Parsing and validating of the input, bool, int, enum, etc, but also check_hook and assign_hook. 3. Logic in all connection poolers to change GUC values to the client's expected values whenever a server connection is handed off to a client 4. Permission checking, if we want some protocol extensions to only be configurable by a highly privileged user All of those things would have to be duplicated/re-implemented if we make protocol parameters their own dedicated thing. Doing that work seems like a waste of time to me, and would imho add much more complexity than the proposed 65 lines of code in 0011.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 23 May 2024 at 20:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Jacob Champion <jacob.champion@enterprisedb.com> writes: > > Would it be good to expand on that idea of criticality? IIRC one of > > Jelte's complaints earlier was that middleware has to know all the > > extension types anyway, to be able to figure out whether it has to do > > something about them or not. HTTP has the concept of hop-by-hop vs > > end-to-end headers for related reasons. > > Yeah, perhaps. We'd need to figure out just which classes we need > to divide protocol parameters into, and then think about a way for > code to understand which class a parameter falls into even when > it doesn't specifically know that parameter. I think this class is so rare, that it's not worth complicating the discussion on new protocol features even more. AFAICT there is only one proposed protocol change that does not need any pooler support (apart from syncing the feature value when re-assigning the connectin): Automatic binary encoding for a list of types All others need some support from poolers, at the very least they need new message types to not error out. But in many cases more complex stuff is needed.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 23 May 2024 at 20:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Jelte Fennema-Nio <me@jeltef.nl> writes: > > On Fri, 17 May 2024 at 21:24, Robert Haas <robertmhaas@gmail.com> wrote: > >> Perhaps these are all minority positions, but I can't tell you what > >> everyone thinks, only what I think. > > > I'd love to hear some opinions from others on these design choices. So > > far it seems like we're the only two that have opinions on this (which > > seems hard to believe) and our opinions are clearly conflicting. And > > above all I'd like to move forward with this, be it my way or yours > > (although I'd prefer my way of course ;) ) > > I got around to looking through this thread in preparation for next > week's patch review session. I have a couple of opinions to offer: > > 1. Protocol versions suck. Bumping them is seldom a good answer, > and should never be done if you have any finer-grained negotiation > mechanism available. My aversion to this is over thirty years old: > I learned that lesson from watching the GIF87-to-GIF89 transition mess. > Authors of GIF-writing tools tended to take the easy way out and write > "GIF89" in the header whether they were actually using any of the new > version's features or not. This led to an awful lot of pictures that > couldn't be read by available GIF-displaying tools, for no good reason > whatsoever. The PNG committee, a couple years later, reacted to that > mess by designing PNG to have no version number whatsoever, and yet > be extensible in a fine-grained way. (Basically, a PNG file is made > up of labeled chunks. If a reader doesn't recognize a particular > chunk code, it can still tell whether the chunk is "critical" or not, > and thereby decide if it must give up or can proceed while ignoring > that chunk.) > > So overall, I have a strong preference for using the _pq_.xxx > mechanism instead of a protocol version bump. I do not believe > the latter has any advantage. I'm not necessarily super opposed to only using the _pq_.xxx mechanism. I mainly think it's silly to have a protocol version number and then never use it. And I feel some of the proposed changes don't really benefit from being able to be turned on-and-off by themselves. My rule of thumb would be: 1. Things that a modern client/pooler would always request: version bump 2. Everything else: _pq_.xxx Of the proposed changes so far on the mailing list the only 2 that would fall under 1 imho are: 1. The ParameterSet message 2. Longer than 32bit secret in BackendKeyData I also don't think the GIF situation you describe translates fully to this discussion. We have active protocol version negotiation, so if a server doesn't support protocol 3.1 a client is expected to fall back to the 3.0 protocol when communicating. Of course you can argue that a badly behaved client will fail to connect when it gets a downgrade request from the server, but that same argument can be made about a server not reporting support for a _pq_.xxx parameter that every modern client/pooler requests. So I don't think there's a practical difference in the problem you're describing. But again if I'm alone in this, then I don't
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
(pressed send to early) On Sat, 25 May 2024 at 12:39, Jelte Fennema-Nio <me@jeltef.nl> wrote: > But again if I'm alone in this, then I don't ... mind budging on this to move this decision along. Using _pq_.xxx parameters for all protocol changes would totally be acceptable to me.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Dave Cramer
Дата:
On Sat, 25 May 2024 at 06:40, Jelte Fennema-Nio <me@jeltef.nl> wrote:
On Thu, 23 May 2024 at 20:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Jelte Fennema-Nio <me@jeltef.nl> writes:
> > On Fri, 17 May 2024 at 21:24, Robert Haas <robertmhaas@gmail.com> wrote:
> >> Perhaps these are all minority positions, but I can't tell you what
> >> everyone thinks, only what I think.
>
> > I'd love to hear some opinions from others on these design choices. So
> > far it seems like we're the only two that have opinions on this (which
> > seems hard to believe) and our opinions are clearly conflicting. And
> > above all I'd like to move forward with this, be it my way or yours
> > (although I'd prefer my way of course ;) )
>
> I got around to looking through this thread in preparation for next
> week's patch review session. I have a couple of opinions to offer:
>
> 1. Protocol versions suck. Bumping them is seldom a good answer,
> and should never be done if you have any finer-grained negotiation
> mechanism available. My aversion to this is over thirty years old:
> I learned that lesson from watching the GIF87-to-GIF89 transition mess.
> Authors of GIF-writing tools tended to take the easy way out and write
> "GIF89" in the header whether they were actually using any of the new
> version's features or not. This led to an awful lot of pictures that
> couldn't be read by available GIF-displaying tools, for no good reason
> whatsoever. The PNG committee, a couple years later, reacted to that
> mess by designing PNG to have no version number whatsoever, and yet
> be extensible in a fine-grained way. (Basically, a PNG file is made
> up of labeled chunks. If a reader doesn't recognize a particular
> chunk code, it can still tell whether the chunk is "critical" or not,
> and thereby decide if it must give up or can proceed while ignoring
> that chunk.)
>
> So overall, I have a strong preference for using the _pq_.xxx
> mechanism instead of a protocol version bump. I do not believe
> the latter has any advantage.
I'm not necessarily super opposed to only using the _pq_.xxx
mechanism.
I find it interesting that up to now nobody has ever used this mechanism.
I mainly think it's silly to have a protocol version number
and then never use it. And I feel some of the proposed changes don't
really benefit from being able to be turned on-and-off by themselves.
My rule of thumb would be:
1. Things that a modern client/pooler would always request: version bump
2. Everything else: _pq_.xxx
Have to agree, why have a protocol version and then just not use it ?
Of the proposed changes so far on the mailing list the only 2 that
would fall under 1 imho are:
1. The ParameterSet message
2. Longer than 32bit secret in BackendKeyData
I also don't think the GIF situation you describe translates fully to
this discussion. We have active protocol version negotiation, so if a
server doesn't support protocol 3.1 a client is expected to fall back
to the 3.0 protocol when communicating.
Also agree. Isn't the point of having a version number to figure out what features the client wants and subsequently the server can provide?
Of course you can argue that a
badly behaved client will fail to connect when it gets a downgrade
request from the server, but that same argument can be made about a
server not reporting support for a _pq_.xxx parameter that every
modern client/pooler requests. So I don't think there's a practical
difference in the problem you're describing.
+1
But again if I'm alone in this, then I don't
I would prefer to see a well defined protocol handshaking mechanism rather than some strange _pq.xxx dance.
Dave
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Fri, May 24, 2024 at 6:09 PM Jelte Fennema-Nio <me@jeltef.nl> wrote: > Separating it from the GUC infrastructure will mean we need to > duplicate a lot of the infrastructure. Assuming we don't care about > SHOW or pg_settings (which I agree are not super important), the > things that we would want for protocol parameters to have that guc.c > gives us for free are: > 1. Reporting the value of the parameter to the client (done using > ParameterStatus) > 2. Parsing and validating of the input, bool, int, enum, etc, but also > check_hook and assign_hook. > 3. Logic in all connection poolers to change GUC values to the > client's expected values whenever a server connection is handed off to > a client > 4. Permission checking, if we want some protocol extensions to only be > configurable by a highly privileged user > > All of those things would have to be duplicated/re-implemented if we > make protocol parameters their own dedicated thing. Doing that work > seems like a waste of time to me, and would imho add much more > complexity than the proposed 65 lines of code in 0011. I think that separating (1) and (3) is going to make us happy rather than sad. For example, if a client speaks to an intermediate pooler which speaks to a server, the client-pooler connection could have different values from the pooler-server connection, and then if everything is done via ParameterStatus messages it's actually more complicated for the pooler, which will have to edit ParameterStatus messages as they pass through, and possibly also create new ones out of nothing. If we separate things, the pooler can always pass ParameterStatus messages unchanged, and only has to worry about the separate infrastructure for handling these new things. Said differently, I'd agree with you if (a) ParameterStatus weren't so dubiously designed and (b) all poolers were going to want every protocol parameter to match on the input side and the output side. And maybe in practice (b) will happen, but I want to leave the door open to cases where it doesn't, and as soon as that happens, I think this becomes a hassle, whereas separate mechanisms don't really hurt much. As you and I discussed a bit in person last week, two for loops rather than one in the pooler isn't really that big of a deal. IMHO, that's a small price to pay for an increased chance of not boxing ourselves into a corner depending on how these parameters end up getting used in practice. As for (2) and (4), I agree there's some duplication, but I think it's quite minor. We have existing facilities for parsing integers and booleans that are reused by a lot of code already, and this is just one more place that can use them. That infrastructure is not GUC-specific. The permissions-checking stuff isn't either. The vast bulk of the GUC infrastructure is concerned with (1) allowing for GUCs to be set from many different sources and (2) handling their transactional nature. We don't need or want either of those characteristics here, so a lot of that complexity just isn't needed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Sat, May 25, 2024 at 6:40 AM Jelte Fennema-Nio <me@jeltef.nl> wrote: > Of the proposed changes so far on the mailing list the only 2 that > would fall under 1 imho are: > 1. The ParameterSet message > 2. Longer than 32bit secret in BackendKeyData Yeah. I wonder how Heikki thinks he can do (2) without breaking everything. Maybe just adding an extra, optional, longer field onto the existing message and hoping that client implementations ignore the extra field? If that's not good enough, then I don't understand how that can be done without breaking compatibility in a fundamental and relatively serious way -- at which point maybe bumping the protocol version is the right thing to do. Really, what I'm strongly opposed to is not bumping the version, but rather doing things that break compatibility such that we need to bump the version. *If* we have a problem that's sufficiently serious to justify breaking compatibility anyway, then we don't really lose anything by bumping the version, and indeed we gain something. I just want us to be searching for ways to avoid breaking interoperability, rather than seeking them out. If it becomes impossible for a PG 18 (or whatever version) server to communicate with earlier servers without specifying special options, or worse yet at all, then a lot of people are going to be very sad about that. We will get a bunch of complaints and a bunch of frustrated users, and they will not be impressed by vague claims of necessity or desirability. They'll just be mad. The question for me here is not "what is the theoretically right thing to do?" but rather "what am I going to tell angry users when they demand to know why I committed the patch that broke this?". "The old way was insecure so we had to change it" might be a good enough reason for people to calm down and stop yelling at me, but "it's no use having a protocol version if we never bump it" definitely won't be. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Tue, 4 Jun 2024 at 16:47, Robert Haas <robertmhaas@gmail.com> wrote: > I think that separating (1) and (3) is going to make us happy rather > than sad. Attached is a new patchset where I keep protocol parameters completely separate from GUCs. I do think I indeed like it better this way, although I'm not entirely sure about the newly proposed NegotiateProtocolParameter message (which we discussed in person last week). I'm looking forward to hearing what you think of these newly proposed changes. Patch 1 & 2: Minor code changes with zero effect until we actually bump the protocol version or add protocol parameters. I hope these can be merged rather soon to reduce the number of patches in the patchset. Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But after bumping the version this would be a user visible API change, so I expect it requires a bit more discussion. Patch 4: Adds a new connection option, but initially all parameters that it takes have the same effect. Patch 5: Starts using the new connection option from Patch 4 Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a more complex way. (nothing changes in practice yet) Patch 7: Bump the protocol version to 3.2 (see commit message for why not bumping to 3.1) Patch 8: The main change: Infrastructure and protocol support to be able to add protocol parameters Patch 9: Adds a report_parameters protocol extension as a POC for the changes in the previous patch. See the commit messages for more details. On Tue, 4 Jun 2024 at 17:00, Robert Haas <robertmhaas@gmail.com> wrote: > Yeah. I wonder how Heikki thinks he can do (2) without breaking > everything. Maybe just adding an extra, optional, longer field onto > the existing message and hoping that client implementations ignore the > extra field? If that's not good enough, then I don't understand how > that can be done without breaking compatibility in a fundamental and > relatively serious way -- at which point maybe bumping the protocol > version is the right thing to do. FYI Heikki his patchset is here: https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi Afaict there's no way to implement this with old clients supporting the new message. So it would need to be opt-in from the client perspective, either using a version bump or a protocol parameter (e.g. large_cancel=true). IMHO a version bump would make more sense for this one. > Really, what I'm strongly opposed to is not bumping the version, but > rather doing things that break compatibility such that we need to bump > the version. *If* we have a problem that's sufficiently serious to > justify breaking compatibility anyway, then we don't really lose > anything by bumping the version, and indeed we gain something. I just > want us to be searching for ways to avoid breaking interoperability, > rather than seeking them out. If it becomes impossible for a PG 18 (or > whatever version) server to communicate with earlier servers without > specifying special options, or worse yet at all, then a lot of people > are going to be very sad about that. We will get a bunch of complaints > and a bunch of frustrated users, and they will not be impressed by > vague claims of necessity or desirability. They'll just be mad. As discussed in person last week: I think we agree on this. There is obviously some moment where it becomes acceptable e.g. libpq45 not being able to connect to PG11 (at least by default) would probably be reasonable. But currently I don't think we're there yet, given the current level of support for NegotiateProtocolVersion in the wild. While many PG servers support this message, no pooler supports it at the moment. Not being able to connect to a pooler by default is indeed going to make people quite angry. So that's why in this new version of the patchset, libpq continues to connect using the 3.0 protocol version by default. However, as soon as a user asks for a feature in their connection string that requires a protocol parameter, or when they explicitly ask for a newer protocol version to be used, then both the newest protocol version supported by the client as well as all known protocol parameters are requested. Again as discussed in person, this is meant to prevent ossification of the protocol. For servers that don't support NegotiateProtocolVersion currently, a client asking for a new protocol version is just as breaking as a client asking for an unknown protocol parameter (i.e. the server will close the connection, because it doesn't know what to do). Since sending either of these anyway is a breaking change in the StartupMessage, we might as well break it as much as possible, in the hopes that clients/servers/poolers implement NegotiateProtocolVersion correctly. Which means afterwards we can safely add new protocol parameters again, as well as safely bump the minor protocol version again without worrying about breaking the ecosystem (because now feature+version negotiation is implemented everywhere). Not bumping the version number, or not adding a protocol parameter, has the risk of people implementing NegotiateProtocolVersion in such a way that it only works for the thing that we changed the first time (i.e. only version negotiation or only feature negotiation, instead of both) > The question for me here is not "what is the theoretically right thing > to do?" but rather "what am I going to tell angry users when they > demand to know why I committed the patch that broke this?". "The old > way was insecure so we had to change it" might be a good enough reason > for people to calm down and stop yelling at me, but "it's no use > having a protocol version if we never bump it" definitely won't be. I think that is totally fair, see also my reason to bump the protocol version to 3.2 instead of 3.1 in the commit message (i.e. because PgBouncer currently mistakenly reports 3.1 as supported).
Вложения
- v13-0001-libpq-Trace-NegotiateProtocolVersion-correctly.patch
- v13-0002-Do-not-hardcode-sending-PG_PROTOCOL_LATEST-in-Ne.patch
- v13-0003-libpq-Include-minor-version-number-in-PQprotocol.patch
- v13-0004-libpq-Add-max_protocol_version-connection-option.patch
- v13-0005-Use-latest-protocol-version-in-libpq_pipeline-te.patch
- v13-0006-libpq-Handle-NegotiateProtocolVersion-message-mo.patch
- v13-0007-Bump-protocol-version-to-3.2.patch
- v13-0008-Add-infrastructure-for-protocol-parameters.patch
- v13-0009-Add-report_parameters-protocol-parameter.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Wed, Jun 5, 2024 at 10:06 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > FYI Heikki his patchset is here: > https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi > > Afaict there's no way to implement this with old clients supporting > the new message. So it would need to be opt-in from the client > perspective, either using a version bump or a protocol parameter (e.g. > large_cancel=true). IMHO a version bump would make more sense for this > one. Well, hang on. The discussion on that thread suggests that this is only going to break connections to servers that don't have NegotiateProtocolVersion. Personally, I think that's fairly OK. It's true that connections to, I guess, pre-9.3 servers will break, but there shouldn't be tons of those left around. It's not great to break connectivity to such servers, of course, but it seems kind of OK. What I really don't want is for v18 to break connections to v17 servers. That would be exponentially more disruptive. I do take your point that poolers haven't added support for NegotiateProtocolVersion yet, but I bet that will change pretty quickly once there's a benefit to doing so. I think it's a lot easier for people to install a new pooler version than a new server version, because the server has the data in it. Also, it's not like they haven't had time. But the real question here is whether we think the longer cancel key is really important enough to justify the partial compatibility break. I'm not deathly opposed to it, but I think it's debatable and I'm sure some people are going to be unhappy. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 5 Jun 2024 at 17:12, Robert Haas <robertmhaas@gmail.com> wrote: > Well, hang on. The discussion on that thread suggests that this is > only going to break connections to servers that don't have > NegotiateProtocolVersion. Yes, correct. > What > I really don't want is for v18 to break connections to v17 servers. > That would be exponentially more disruptive. Totally agreed, and that's easily achievable because of the NegotiateProtocolVersion message. We're all good on v18 to v17 connections and protocol changes, as long as we make any new behaviour depend on either a protocol parameter or a protocol version bump. > I do take your point that poolers haven't added support for > NegotiateProtocolVersion yet, but I bet that will change pretty > quickly once there's a benefit to doing so. I think at minimum we'll need a mechanism for people to connect using a StartupMessage that doesn't break existing poolers. If users have no way to connect at all to their semi-recently installed connection pooler with the newest libpq, then I expect we'll have many unhappy users. I think it's debatable whether the compatible StartupMessage should be opt-in or opt-out. I'd personally at minimum want to wait one PG release before using the incompatible StartupMessage by default, just to give pooler installs some time to catch up. > I think it's a lot easier > for people to install a new pooler version than a new server version, > because the server has the data in it. Also, it's not like they > haven't had time. I agree that it's a lot easier to upgrade poolers than it is to upgrade PG, but still people are generally hesitant to upgrade stuff in their infrastructure. And I totally agree that poolers have had the time to implement NegotiateProtocolVersion support, but I'm pretty sure many users will assign blame to libpq anyway. > But the real question here is whether we think the longer cancel key > is really important enough to justify the partial compatibility break. > I'm not deathly opposed to it, but I think it's debatable and I'm sure > some people are going to be unhappy. I think if it's an opt-in compatibility break, users won't care how important it is. It's either important enough to opt-in to this compatibility break for them, or it's not and nothing changes for them. My feeling is that we're unlikely to find a feature that's worth breaking compatibility (with poolers and pre-9.3 PGs) by default on its own. But after adding a few new protocol features, at some point together these features become worth this break. Especially, because by then 9.3 will be even older and poolers will have started to support NegotiateProtocolVersion (due to people complaining that they cannot connect with the new opt-in libpq break-compatibility flag). But if we're going to wait until we have the one super important protocol feature, then I don't see us moving forward. To summarize: I'd like to make some relatively small opt-in change(s) in PG18 that breaks compatibility (with poolers and pre-9.3 PGs). So that when we have an important enough reason to break compatibility by default, that break will be much less painful to do.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Wed, Jun 5, 2024 at 1:50 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Totally agreed, and that's easily achievable because of the > NegotiateProtocolVersion message. We're all good on v18 to v17 > connections and protocol changes, as long as we make any new behaviour > depend on either a protocol parameter or a protocol version bump. Cool. > I think at minimum we'll need a mechanism for people to connect using > a StartupMessage that doesn't break existing poolers. If users have no > way to connect at all to their semi-recently installed connection > pooler with the newest libpq, then I expect we'll have many unhappy > users. I think it's debatable whether the compatible StartupMessage > should be opt-in or opt-out. I'd personally at minimum want to wait > one PG release before using the incompatible StartupMessage by > default, just to give pooler installs some time to catch up. I agree that we need such a mechanism, but if the driving feature is cancel-key length, I expect that opt-in isn't going to work very well. Opt-in seems like it would work well with compression or transparent column encryption, but few users will specify a connection string option just to get a longer cancel key length, so the feature won't get much use if we do it that way. I won't be crushed if we decide to somehow make it opt-in, but I kind of doubt that will happen. Would we make everyone add longcancelkey=true to all their connection strings for one release and then carry that connection parameter until the heat death of the universe even though after the 1-release transition period there would be no reason to ever use it? Would we rip the parameter back out after the transition period and break existing connection strings? Would we have people write protocolversion=3.1 to opt in and then they could just keep that in the connection string without harm, or at least without harm until 3.2 comes out? I don't really like any of these options that well. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 5 Jun 2024 at 22:48, Robert Haas <robertmhaas@gmail.com> wrote: > I agree that we need such a mechanism, but if the driving feature is > cancel-key length, I expect that opt-in isn't going to work very well. > Opt-in seems like it would work well with compression or transparent > column encryption, but few users will specify a connection string > option just to get a longer cancel key length, so the feature won't > get much use if we do it that way. I know Neon wants to make use of this for their proxy (to encode some tenant_id into the key). So they might want to require people to opt-in when using their proxy. > I won't be crushed if we decide to > somehow make it opt-in, but I kind of doubt that will happen. > Would we > make everyone add longcancelkey=true to all their connection strings > for one release and then carry that connection parameter until the > heat death of the universe even though after the 1-release transition > period there would be no reason to ever use it? Would we rip the > parameter back out after the transition period and break existing > connection strings? Would we have people write protocolversion=3.1 to > opt in and then they could just keep that in the connection string > without harm, or at least without harm until 3.2 comes out? I don't > really like any of these options that well. I agree longcancelkey=true is not what we want. In my patch 0004, you can specify max_protocol_version=latest to use the latest protocol version as opt-in. This is a future proof version of protocolversion=3.1 that you're proposing, because it will automatically start using 3.2 when it comes out. So I think that solves your concern here. (although maybe it should be called latest-3.x or something, in case we ever want to add a 4.0 protocol, naming is hard) I personally quite like the max_protocol_version connection parameter. I think even for testing it is pretty useful to tell libpq what protocol version to try to connect as. It could even be accompanied with a min_protocol_version, e.g. in case you only want the connection attempt to fail when the server does not support this more secure cancel key.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Wed, Jun 5, 2024 at 5:16 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > I agree longcancelkey=true is not what we want. In my patch 0004, you > can specify max_protocol_version=latest to use the latest protocol > version as opt-in. This is a future proof version of > protocolversion=3.1 that you're proposing, because it will > automatically start using 3.2 when it comes out. So I think that > solves your concern here. (although maybe it should be called > latest-3.x or something, in case we ever want to add a 4.0 protocol, > naming is hard) > > I personally quite like the max_protocol_version connection parameter. > I think even for testing it is pretty useful to tell libpq what > protocol version to try to connect as. It could even be accompanied > with a min_protocol_version, e.g. in case you only want the connection > attempt to fail when the server does not support this more secure > cancel key. This makes some sense to me. I don't think that I believe max_protocol_version=latest is future-proof: just because I want to opt into this round of new features doesn't mean I feel the same way about the next round. But it may still be a good idea. I suppose the semantics are that we try to connect with the version specified by max_protocol_version and, if we get downgraded by the server, we abort if we end up below min_protocol_version. I like those semantics, and I think I also like having both parameters, but I'm not 100% sure of the naming. It's a funny use of "max" and "min", because the max is really what we're trying to do and the min is what we end up with, and those terms don't necessarily bring those ideas to mind. I don't have a better idea off-hand, though. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Greg Sabino Mullane
Дата:
On Wed, Jun 5, 2024 at 9:03 PM Robert Haas <robertmhaas@gmail.com> wrote:
It's a funny use of "max" and "min", because the max is really what we're trying to do and the min is what we end
up with, and those terms don't necessarily bring those ideas to mind.
requested_protocol_version and minimum_protocol_version?
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 6 Jun 2024 at 03:03, Robert Haas <robertmhaas@gmail.com> wrote: > This makes some sense to me. I don't think that I believe > max_protocol_version=latest is future-proof: just because I want to > opt into this round of new features doesn't mean I feel the same way > about the next round. But it may still be a good idea. I think for most people the only reason not to opt-in to improvements (even if they are small) is if those improvements break something else. Once the NegotiateProtocolVersion message is implemented everywhere in the ecosystem, nothing should break when going from e.g. 3.2 to 3.4. So for the majority of the people I think max_protocol_version=latest is what they'll want to use once the ecosystem has caught up. Of course there will be people that want tight control, but they can set max_protocol_version=3.2 instead. > I suppose the semantics are that we try to connect with the version > specified by max_protocol_version and, if we get downgraded by the > server, we abort if we end up below min_protocol_version. Correct > I like those > semantics, and I think I also like having both parameters, but I'm not > 100% sure of the naming. It's a funny use of "max" and "min", because > the max is really what we're trying to do and the min is what we end > up with, and those terms don't necessarily bring those ideas to mind. > I don't have a better idea off-hand, though. I borrowed this terminology from the the ssl_min_protocol_version and ssl_max_protocol_version connection options that we already have. Those basically have the same semantics as what I'm proposing here, but for the TLS protocol version instead of the Postgres protocol version. I'm also not a huge fan of the min_protocol_version and max_protocol_version names, but staying consistent with existing options seems quite nice. Looking at ssl_max_protocol_version closer though, to stay really consistent I'd have to change "latest" to be renamed to empty string (i.e. there is no max_protocol_version). I think I might prefer staying consistent over introducing an imho slightly clearer name. Another way to stay consistent would of course be also adding "latest" as an option to ssl_max_protocol_version? What do you think? I'll look into adding min_protocol_version to the patchset soonish. Some review of the existing code in the first few patches would definitely be appreciated.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Jun 6, 2024 at 5:12 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Looking at ssl_max_protocol_version closer though, to stay really > consistent I'd have to change "latest" to be renamed to empty string > (i.e. there is no max_protocol_version). I think I might prefer > staying consistent over introducing an imho slightly clearer name. > Another way to stay consistent would of course be also adding "latest" > as an option to ssl_max_protocol_version? What do you think? As I see it, the issue here is whether the default value would ever be different from the latest value. If not, then using blank to mean the latest seems fine, but if so, then I'd expect blank to mean the default version and latest to mean the latest version. > I'll look into adding min_protocol_version to the patchset soonish. > Some review of the existing code in the first few patches would > definitely be appreciated. Yeah, I understand, and I do want to do that, but keep in mind I've already spent considerable time on this patch set, way more than most others, and if I want to get it committed I'm nowhere close to being done. It's probably multiple weeks of additional work for me, and I think I've probably already spent close to a week on this, and I only work ~48 weeks a year, and there are ~300 patches in the CommitFest. Plus, right now there is no possibility of actually committing anything until after we branch. And, respectfully, I feel like there has to be some give and take here. I've been trying to give this patch set higher priority because it's in an area that I know something about and have opinions about and also because I can tell that you're kind of frustrated and I don't want you to leave the development community. But, at the same time, I don't think you've done much to help me get my patches committed, and while you have done some review of other people's patches, it doesn't seem to often be the kind of detailed, line-by-line review that is needed to get most patches committed. So I'm curious how you expect this system to scale. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Thu, 6 Jun 2024 at 18:01, Robert Haas <robertmhaas@gmail.com> wrote: > As I see it, the issue here is whether the default value would ever be > different from the latest value. If not, then using blank to mean the > latest seems fine, but if so, then I'd expect blank to mean the > default version and latest to mean the latest version. Alright, that's fair. And we already seem to follow that pattern: There's currently no connection option that has a default that's not the empty string, but still accepts the empty string as an argument. > > I'll look into adding min_protocol_version to the patchset soonish. > > Some review of the existing code in the first few patches would > > definitely be appreciated. > > Yeah, I understand, and I do want to do that, but keep in mind I've > already spent considerable time on this patch set, way more than most > others, and if I want to get it committed I'm nowhere close to being > done. It's probably multiple weeks of additional work for me, and I > think I've probably already spent close to a week on this, and I only > work ~48 weeks a year, and there are ~300 patches in the CommitFest. I very much appreciate the time you spent on this patchset so far. I mainly thought that instead of only discussing the more complex parts of the patchset, it would be nice to also actually move forward a little bit too. And the first 3 patches in this patchset are very small and imho straightforward improvements. To be clear, I'm not saying that should be all on you. I think those first three patches can be reviewed by pretty much anyone. > Plus, right now there is no possibility of actually committing > anything until after we branch. Totally fair, but even a LGTM on one of the patches would be quite nice. > And, respectfully, I feel like there > has to be some give and take here. I've been trying to give this patch > set higher priority because it's in an area that I know something > about and have opinions about and also because I can tell that you're > kind of frustrated and I don't want you to leave the development > community. Thank you for giving it a higher priority, it's definitely appreciated and noticed. > But, at the same time, I don't think you've done much to > help me get my patches committed, and while you have done some review > of other people's patches, it doesn't seem to often be the kind of > detailed, line-by-line review that is needed to get most patches > committed. So I'm curious how you expect this system to scale. Of course there's always the possibility to review more. But I don't really agree with this summary of my review activity. I did see your patches related to the incremental backup stuff. They looked interesting, but at the time from an outside perspective it didn't seem like those threads needed my reviews to progress (a bunch of people more knowledgable on the topic were already responding). So I spent my time mainly on threads where I felt I could add something useful, and often that was more on the design front than the exact code. Honestly that's what triggered this whole patchset in the first place: Adding infrastructure for protocol changes so that the several other threads that try to introduce protocol changes can actually move forward, instead of being in limbo forever. Regarding line-by-line reviews, imho I definitely do that for the smaller patches I tend to review (even if they are part of a bigger patchset). But the bigger ones I don't think line-by-line reviews are super helpful at the start, so I generally comment more on the design in those cases.
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Thu, Jun 6, 2024 at 3:27 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Of course there's always the possibility to review more. But I don't > really agree with this summary of my review activity. Nonetheless, I need to take a break from this to work on some of my own stuff. I'll circle back around to it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Wed, Jun 5, 2024 at 10:06 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > Patch 1 & 2: Minor code changes with zero effect until we actually > bump the protocol version or add protocol parameters. I hope these can > be merged rather soon to reduce the number of patches in the patchset. 0001 looks like a bug fix that can (and probably should) be committed and back-patched. What would reduce work for me is if the commit message explained why these changes are necessary and to which stable branches they should be applied and, if that's not all of them, the reason why back-patching should stop at some particular release. The change to pqTraceOutput_NegotiateProtocolVersion is easy to understand: the current code doesn't dump the message format as documented. It probably doesn't help that the documentation is missing a word -- it should say "Then, for each protocol option...". It's less obvious why the change to fe-connect.c is needed. Since most cases seem to be handled in a few common places, it would be good to mention in the commit message (or maybe a comment) why this one needs separate handling. I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST. I prefer that test the way it is; I think the intent is clearer with the existing code. > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But > after bumping the version this would be a user visible API change, so > I expect it requires a bit more discussion. I don't know if this is the right idea or not. An alternative would be to leave this alone and add PQprotocolMinorVersion(). > Patch 4: Adds a new connection option, but initially all parameters > that it takes have the same effect. Generally seems OK, but: - The commit message needs spelling and grammar checking. - dispsize 3 isn't long enough for 3.10, or more immediately, "latest". - This is storing the major protocol version in a variable called "minor" and the minor protocol version in a variable called "major". - I think PGMAXPROTOCOLVERSION needs to be added to the docs. > Patch 5: Starts using the new connection option from Patch 4 Not sure yet whether I like this approach. > Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a > more complex way. (nothing changes in practice yet) + * NegotiateProtocolVersion message. So we only want to send a only->don't + * protocol version by default. Since either of those would cause a "default. Since" => "default, since" + char *conn_string_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + char *server_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + char *supported_value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); I have some difficulty understanding how these calculations would produce different answers. + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, server version is newer than client version"); + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion message, negative protocol parameter count"); + libpq_append_conn_error(conn, "unexpected NegotiateProtocolVersion message, server supports requested features"); These messages don't seem good. First, I don't think that telling the user that there's a problem with a specific wire protocol message is very user-friendly. Second, the use of a comma to glue two semi-related thoughts together is not a great practice in English in general and is certainly something we should try to avoid in our error messages. Third, the first and last of these aren't very clear about what the problem actually is. I can only understand it from reading the code. Maybe these messages could be rephrased as "unable to negotiate protocol version: blah". Like "unable to negotiate protocol version: server requests downgrade to higher-numbered version" or "unable to negotiate protocol version: server negotiates but asks for no changes". I don't think I completely understand what's going on in this patch yet. I'm not sure that it can be committed on its own, and I think it might need more polishing, including on comments and the commit message. > Patch 7: Bump the protocol version to 3.2 (see commit message for why > not bumping to 3.1) Good commit message. The point is arguable, so putting forth your best argument is important. > Patch 8: The main change: Infrastructure and protocol support to be > able to add protocol parameters > Patch 9: Adds a report_parameters protocol extension as a POC for the > changes in the previous patch. My general impression on first looking at these patches is that a lot of the ideas make sense but that they don't seem very close to being committable. It's not very clear how these new messages integrate into the overall protocol flow. The documentation makes the negative statement that they can't be used as part of the extended query protocol, but that just begs the question of where they can be used. I think there should be an update to protocol-flow.html here. For example, consider the "Simple Query" section of that page, which begins "A simple query cycle is initiated by the frontend sending a Query message to the backend." It goes on to describe what happens afterward. A similar discussion seems to be needed here, or maybe two of them, The patch touches src/interfaces/libpq (which is good) but does not update the libpq documentation (which is bad). The documentation for NegotiateProtocolParameter is almost identical to the documentation for SetProtocolParameterComplete. I would have expected the former to include a field giving guidance about values that might be legal in the future, and the latter to include an error message, rather than just an error indicator. I wonder whether we could define 3.2 to report on all supported protocol parameters even if they weren't in the startup message, to avoid having to jam a lot of stuff we don't really care about into the startup message. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Jelte Fennema-Nio
Дата:
On Wed, 12 Jun 2024 at 19:53, Robert Haas <robertmhaas@gmail.com> wrote: > 0001 looks like a bug fix that can (and probably should) be committed > and back-patched. I moved this patch to its own thread, together with a bunch of other fixes to the libpq tracing logic: https://www.postgresql.org/message-id/flat/CAGECzQSoPHtZ4xe0raJ6FYSEiPPS%2BYWXBhOGo%2BY1YecLgknF3g%40mail.gmail.com I left the patch numbering intact though. > I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto) > > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST. > I prefer that test the way it is; I think the intent is clearer with > the existing code. Great to hear! I reverted that change to the check back to what it was now. > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But > > after bumping the version this would be a user visible API change, so > > I expect it requires a bit more discussion. > > I don't know if this is the right idea or not. An alternative would be > to leave this alone and add PQprotocolMinorVersion(). I considered that, but that makes the API a lot more annoying to use for end users when they want to compare to a version, especially when they want to include the major version too. PQprotocolVersion() >= 30001 vs PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 && PQprotocolVersionMinor() >= 1) Given that PQprotocolVersion currently has no practical use, because it always returns 3 in practice. I personally think that changing the behaviour of API in the way I suggested is the best option here. > > Patch 4: Adds a new connection option, but initially all parameters > > that it takes have the same effect. > > Generally seems OK, but: Fixed > > Patch 5: Starts using the new connection option from Patch 4 > > Not sure yet whether I like this approach. Could you explain a bit more what you don't like about it? > > Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a > > more complex way. (nothing changes in practice yet) > > + * NegotiateProtocolVersion message. So we only want to send a > > only->don't > > + * protocol version by default. Since either of those would cause a > > "default. Since" => "default, since" Fixed > I have some difficulty understanding how these calculations would > produce different answers. Oops, copy paste mistake, fixed > + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion > message, server version is newer than client version"); > + libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion > message, negative protocol parameter count"); > + libpq_append_conn_error(conn, "unexpected NegotiateProtocolVersion > message, server supports requested features"); > > These messages don't seem good. Fair enough. I changed them a bit now, do you think they are good now? > I don't think I completely understand what's going on in this patch > yet. I'm not sure that it can be committed on its own, and I think it > might need more polishing, including on comments and the commit > message. Yeah, I think it probably makes sense to combine this with 0008, because there is so much interaction between the two. For now I haven't done that yet though. > > Patch 7: Bump the protocol version to 3.2 (see commit message for why > > not bumping to 3.1) > > Good commit message. The point is arguable, so putting forth your best > argument is important. Just to clarify: do you agree with the point now, after that argument? > > Patch 8: The main change: Infrastructure and protocol support to be > > able to add protocol parameters > > Patch 9: Adds a report_parameters protocol extension as a POC for the > > changes in the previous patch. > > My general impression on first looking at these patches is that a lot > of the ideas make sense but that they don't seem very close to being > committable. I totally agree that there's definitely significant work/discussion that needs to happen before these are committable. Patch 8 was basically my first implementation of my interpretation of our in-person conversation at PGConf.dev. I mainly meant these last two patches as an initial start for further discussion, and to see if this was indeed the direction in which to progress. Sorry if I didn't make that clear. > It's not very clear how these new messages integrate into the overall > protocol flow. The documentation makes the negative statement that > they can't be used as part of the extended query protocol, but that > just begs the question of where they can be used. I think there should > be an update to protocol-flow.html here. For example, consider the > "Simple Query" section of that page, which begins "A simple query > cycle is initiated by the frontend sending a Query message to the > backend." It goes on to describe what happens afterward. A similar > discussion seems to be needed here, or maybe two of them, I changed the second paragraph a bit to hopefully clarify this. Is it indeed clearer like this? > The patch touches src/interfaces/libpq (which is good) but does not > update the libpq documentation (which is bad). I definitely did update the libpq documentation for all the new public C APIs, but it seems I forgot to add docs for the report_parameters connection option, so I fixed that now. Note: 0008 doesn't add any publicly facing libpq changes, so I don't think updating the libpq docs make sense for that patch. > The documentation for NegotiateProtocolParameter is almost identical > to the documentation for SetProtocolParameterComplete. I would have > expected the former to include a field giving guidance about values > that might be legal in the future Ugh, it seems I implemented the client side for that only half-baked and didn't include it in the docs in the message specification (only in the the report_parameters one). This is fixed now. > and the latter to include an error > message, rather than just an error indicator. I did consider an error message (and I think that is what we discussed in-person too). But during implementation a WARNING together with a simple error indicator seemed nicer since that could hook into the existing infrastructure for reporting warnings (both server and client side). e.g. you can now provide detail/context/errorcode in the warning, without having to add all those to the SetProtocolParameterComplete message. I don't feel super strongly either way though, so If you prefer the error message to be part of the SetProtocolParameterComplete message then I'm happy to change that. > I wonder whether we could define 3.2 to report on all supported > protocol parameters even if they weren't in the startup message, to > avoid having to jam a lot of stuff we don't really care about into the > startup message. I think that's a good idea. I'll try to look into doing that soonish.
Вложения
- v14-0002-Do-not-hardcode-sending-PG_PROTOCOL_LATEST-in-Ne.patch
- v14-0003-libpq-Include-minor-version-number-in-PQprotocol.patch
- v14-0004-libpq-Add-max_protocol_version-connection-option.patch
- v14-0005-Use-latest-protocol-version-in-libpq_pipeline-te.patch
- v14-0006-libpq-Handle-NegotiateProtocolVersion-message-mo.patch
- v14-0007-Bump-protocol-version-to-3.2.patch
- v14-0008-Add-infrastructure-for-protocol-parameters.patch
- v14-0009-Add-report_parameters-protocol-parameter.patch
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But > > > after bumping the version this would be a user visible API change, so > > > I expect it requires a bit more discussion. > > > > I don't know if this is the right idea or not. An alternative would be > > to leave this alone and add PQprotocolMinorVersion(). > > I considered that, but that makes the API a lot more annoying to use > for end users when they want to compare to a version, especially when > they want to include the major version too. > > PQprotocolVersion() >= 30001 > > vs > > PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 && > PQprotocolVersionMinor() >= 1) > > Given that PQprotocolVersion currently has no practical use, because > it always returns 3 in practice. I personally think that changing the > behaviour of API in the way I suggested is the best option here. Mmm, I was thinking of defining the new function to return the major and minor version in one number, while the existing function could continue to work as now. I see your point too, but I'd like to hear some other opinions before we decide, because I think it's unclear what is actually best here. And also: surely this is not the hill to die on. If it makes others a lot happier to do it one way or the other, I'm severely disinclined to spend energy arguing with those people. And, on the basis of previous experience, this is exactly the sort of question about which opinions sometimes run quite strongly. I would much rather swim with the current than get what I would myself prefer. > > > Patch 5: Starts using the new connection option from Patch 4 > > > > Not sure yet whether I like this approach. > > Could you explain a bit more what you don't like about it? I don't dislike it; I'm just not sure whether it is the best approach to testing the remainder of the patch series. Perhaps the commit message could explain more why this approach was chosen and what value we get from it. > Fair enough. I changed them a bit now, do you think they are good now? I'll try to re-review the patch set when I have a bit more time than I do at this exact moment. > > > Patch 7: Bump the protocol version to 3.2 (see commit message for why > > > not bumping to 3.1) > > > > Good commit message. The point is arguable, so putting forth your best > > argument is important. > > Just to clarify: do you agree with the point now, after that argument? Well, here again, I would like to know what other people think. It doesn't intrinsically matter to me that much what we do here, but it matters to me a lot that extensive recriminations don't ensue afterwards. > I did consider an error message (and I think that is what we discussed > in-person too). But during implementation a WARNING together with a > simple error indicator seemed nicer since that could hook into the > existing infrastructure for reporting warnings (both server and client > side). e.g. you can now provide detail/context/errorcode in the > warning, without having to add all those to the > SetProtocolParameterComplete message. I don't feel super strongly > either way though, so If you prefer the error message to be part of > the SetProtocolParameterComplete message then I'm happy to change > that. My general programming experience has been that any time I decide to include an error flag rather than an error message, I end up regretting it. It's possible that this case is, for some reason, an exception to that principle, but I feel like we need to nail down exactly what the protocol flow *and* the libpq API for these new messages is going to be before I can really have an intelligent opinion about that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs
От
Robert Haas
Дата:
On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto) > > > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST. > > I prefer that test the way it is; I think the intent is clearer with > > the existing code. > > Great to hear! I reverted that change to the check back to what it was now. I took another look at 0002 today: + if (proto > PG_PROTOCOL_LATEST) + FrontendProtocol = PG_PROTOCOL_LATEST; A few lines before this, we set FrontendProto = proto. Then we have one error check, then this. How about instead changing the earlier assignment to FrontendProto = Max(proto, PG_PROTOCOL_LATEST), or an if-statement with similar effect? I don't think it practically matters because send_message_to_frontend() looks well-equipped to handle a garbage value in FrontendProto, but it seems cleaner to set the value once and not change it than to change it and then almost immediately change it again. Also, if we do that, we could fix the comment e.g. "Set FrontendProtocol now so that ereport() knows what format to send if we fail during startup, but limit the value to the newest protocol version we actually support." And if we don't do that, then this new if-statement needs a comment of its own, and it should explain why we didn't choose to merge this with the earlier assignment. - if (PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) || - unrecognized_protocol_options != NIL) + if (PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) + || unrecognized_protocol_options != NIL) This hunk can be dropped. - pq_sendint32(&buf, PG_PROTOCOL_LATEST); + pq_sendint32(&buf, FrontendProtocol); This looks good, but the function header comment needs to be corrected, e.g. "This lets the client know that they have either requested a newer minor protocol version than we are able to speak, or at least one protocol option that we don't understand, or possibly both. FrontendProtocol has already been set to the version requested by the client or the highest version we know how to speak, whichever is older. If the highest version that we know how to speak is too old for the client, it can abandon the connection." > > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But > > > after bumping the version this would be a user visible API change, so > > > I expect it requires a bit more discussion. > > > > I don't know if this is the right idea or not. An alternative would be > > to leave this alone and add PQprotocolMinorVersion(). > > I considered that, but that makes the API a lot more annoying to use > for end users when they want to compare to a version, especially when > they want to include the major version too. > > PQprotocolVersion() >= 30001 > > vs > > PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 && > PQprotocolVersionMinor() >= 1) > > Given that PQprotocolVersion currently has no practical use, because > it always returns 3 in practice. I personally think that changing the > behaviour of API in the way I suggested is the best option here. I respect that, but I don't want to get flamed for doing something that might be controversial without anybody else endorsing it. I'll commit this if it gets some support, but not otherwise. I'm willing to commit a patch adding a new function even if nobody else votes, but not this. > > > Patch 4: Adds a new connection option, but initially all parameters > > > that it takes have the same effect. > > > > Generally seems OK, but: > > Fixed Not entirely. The documentation of the environment variable gets the name of the environment variable wrong. > > I wonder whether we could define 3.2 to report on all supported > > protocol parameters even if they weren't in the startup message, to > > avoid having to jam a lot of stuff we don't really care about into the > > startup message. > > I think that's a good idea. I'll try to look into doing that soonish. Is this something that you still intend to do? It looks to me like this would change some things as early as 0006. Other things about 0006: + * Since some old servers and poolers don't support the + * NegotiateProtocolVersion message. So we don't want to send a You could say "Some old servers ... message, so we don't" or "Some old servers ... message. So, we don't" or "Since some old servers ... message, we don't" but it's not quite grammatical the way it is. + * ask for the latest protocol version we support by default too. How about "default to the latest protocol version we support"? + for (const pg_protocol_parameter *param = KnownProtocolParameters; param->name; param++) + { + const char *value = *(char **) ((char *) conn + param->conn_connection_string_value_offset); + + if (value && value[0]) + { + needs_new_protocol_features = true; + break; + } + } I would personally prefer this if it were written with explicit tests, like param->name != NULL and value[0] != '\0' but I realize that's not everyone's preferred style. I guess I'm also a little unsure whether we need to deal with both NULL and "" here. Are they semantically different? Is it reasonable to try to prevent NULL from occurring in the first place? Or maybe it's fine the way it is. Not sure. I also happened to notice that 0009 has a typo: report_paramters. -- Robert Haas EDB: http://www.enterprisedb.com