On Fri, 29 Dec 2023 at 19:38, Tom Lane <t...@sss.pgh.pa.us> wrote: >> Jelte Fennema-Nio <m...@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 <t...@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.
