Re: libpq: Fix lots of discrepancies in PQtrace

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Jelte Fennema-Nio <postgres(at)jeltef(dot)nl>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: libpq: Fix lots of discrepancies in PQtrace
Date: 2024-06-27 05:39:21
Message-ID: Znz7CSktKAVbm4Ck@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, Jun 26, 2024 at 10:02:08PM +0200, Jelte Fennema-Nio wrote:
> On Wed, 26 Jun 2024 at 18:36, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > Thanks, Nathan. I'm holding myself responsible for the rest ... will
> > handle soon after the branch.
>
> Sounds great. Out of curiosity, what is the backpatching policy for
> something like this? Honestly most of these patches could be
> considered bugfixes in PQtrace, so backpatching might make sense. OTOH
> I don't think PQtrace is used very much in practice, so backpatching
> might carry more risk than it's worth.

0001 getting on HEAD after the feature freeze as a cleanup piece
cleanup is no big deal. That's cosmetic, still OK.

Looking at the whole, the rest of the patch set qualifies as a new
feature, even if they're aimed at closing existing gaps.
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?

+ AuthResponseType current_auth_response;
I'd recommend to document what this flag is here for, with a comment.
--
Michael

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2024-06-27 05:43:54 Re: walsender.c comment with no context is hard to understand
Previous Message Philippe BEAUDOIN 2024-06-27 05:34:32 Adminpack removal