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

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