On Mon, 28 Aug 2023 at 15:00, Pavel Stehule <pavel.steh...@gmail.com> wrote: + minServerMajor = 1600; + serverMajor = PQserverVersion(pset.db) / 100;
Instead of using the server version, we should instead use the protocol version negotiation that's provided by the NegotiateProtocolVersion message type. We should bump the requested protocol version from 3.0 to 3.1 and check that the server supports 3.1. Otherwise proxies or connection poolers might get this new message type, without knowing what to do with them. + <varlistentry id="protocol-message-formats-ReportGUC"> + <term>ReportGUC (F)</term> We'll need some docs on the "Message Flow" page too: https://www.postgresql.org/docs/current/protocol-flow.html Specifically what response is expected, if any. Another thing that should be described there is that this falls outside of the transaction flow, i.e. it's changes are not reverted on ROLLBACK. But that leaves an important consideration: What happens when an error occurs on the server during handling of this message (e.g. the GUC does not exist or an OOM is triggered). Is any open transaction aborted in that case? If not, we should have a test for that. + if (PQresultStatus(res) != PGRES_COMMAND_OK) + pg_fatal("failed to link custom variable: %s", PQerrorMessage(conn)); + PQclear(res); The tests should also change the config after running PQlinkParameterStatus/PQunlinkParameterStatus to show that the guc is reported then or not reported then. + if (!PQsendTypedCommand(conn, PqMsg_ReportGUC, 't', paramName)) + return NULL; I think we'll need some bikeshedding on what the protocol message should look like exactly. I'm not entirely sure what is the most sensible here, so please treat everything I write next as suggestions/discussion: I see that you're piggy-backing on PQsendTypedCommand, which seems nice to avoid code duplication. It has one downside though: not every type, is valid for each command anymore. One way to avoid that would be to not introduce a new command, but only add a new type that is understood by Describe and Close, e.g. a 'G' (for GUC). Then PqMsg_Describe, G would become the equivalent of what'the current patch its PqMsg_ReportGUC, 't' and PqMsg_Close, G would be the same as PqMsg_ReportGUC, 'f'. The rest of this email assumes that we're keeping your current proposal for the protocol message, so it might not make sense to address all of this feedback, in case we're still going to change the protocol: + if (is_set == 't') + { + SetGUCOptionFlag(name, GUC_REPORT); + status = "SET REPORT_GUC"; + } + else + { + UnsetGUCOptionFlag(name, GUC_REPORT); + status = "UNSET REPORT_GUC"; + } I think we should be strict about what we accept here. Any other value than 'f'/'t' for is_set should result in an error imho.
