On Thu, 27 Jun 2024 at 07:39, Michael Paquier <mich...@paquier.xyz> wrote: > Looking at the whole, the rest of the patch set qualifies as a new > feature, even if they're aimed at closing existing gaps.
Alright, seems reasonable. To be clear, I don't care at all about this being backported personally. > Particularly, you have bits of new infrastructure introduced in libpq > like the current_auth_response business in 0004, making it a new > feature by structure. > > + conn->current_auth_response = AUTH_RESP_PASSWORD; > ret = pqPacketSend(conn, PqMsg_PasswordMessage, pwd_to_send, > strlen(pwd_to_send) + 1); > + conn->current_auth_response = AUTH_RESP_NONE; > > It's a surprising approach. Future callers of pqPacketSend() and > pqPutMsgEnd() would easily miss that this flag should be set, as much > as reset. Isn't that something that should be added in input of these > functions? Yeah, I'm not entirely happy about it either. But adding an argument to pqPutMsgEnd and pqPutPacketSend would mean all the existing call sites would need to change, even though only 4 of them would care about the new argument. You could argue that it's the better solution, but it would at least greatly increase the size of the diff. Of course to reduce the diff size you could make the old functions a wrapper around a new one with the extra argument, but I couldn't think of a good name for those functions. Overall I went for the chosen approach here, because it only impacted code at the call sites for these auth packets (which are the only v3 packets in the protocol that you cannot interpret based on their contents alone). I think your worry about easily missing to set/clear the flag is not a huge problem in practice. We almost never add new authentication messages and it's only needed there. Also the clearing is not even strictly necessary for the tracing to behave correctly, but it seemed like the right thing to do. > + AuthResponseType current_auth_response; > I'd recommend to document what this flag is here for, with a comment. Oops, yeah I forgot about that. Done now.
v2-0002-libpq-Add-suppress-argument-to-pqTraceOutputNchar.patch
Description: Binary data
v2-0003-libpq-Trace-StartupMessage-SSLRequest-GSSENCReque.patch
Description: Binary data
v2-0004-libpq-Trace-frontend-authentication-challenges.patch
Description: Binary data
v2-0005-libpq-Trace-responses-to-SSLRequest-and-GSSENCReq.patch
Description: Binary data
v2-0006-libpq-Trace-all-messages-received-from-the-server.patch
Description: Binary data
v2-0007-libpq-Trace-server-Authentication-messages-in-det.patch
Description: Binary data
v2-0008-libpq-Trace-NegotiateProtocolVersion-correctly.patch
Description: Binary data
