Re: libpq: Fix lots of discrepancies in PQtrace

On Thu, 27 Jun 2024 at 07:39, Michael Paquier <michael(at)paquier(dot)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.