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@alvh.no-ip.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
