On Wed, Jun 5, 2024 at 10:06 AM Jelte Fennema-Nio <postg...@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
