Обсуждение: libpq: Fix lots of discrepancies in PQtrace

After initially trying to add trace support for
StartupMessage/SSLRequest/GSSENCRequest[1] I realized there were many
more cases where PQtrace was not correctly tracing messages, or not
even tracing them at all. These patches fix all of the issues that I
was able to find.

0001 is some cleanup after f4b54e1ed9
0002 does some preparatory changes for 0004 & 0007

All the others improve the tracing, and apart from 0004 and 0007
depending on 0002, none of these patches depend on each other.
Although you could argue that 0007 and 0008 depend on 0006, because
without 0006 the code added by 0007 and 0008 won't ever really be
executed.

To test you can add a PQreset(conn) call to the start of the
test_cancel function in:
src/test/modules/libpq_pipeline/libpq_pipeline.c.

And then run:
ninja -C build all install-quiet &&
build/src/test/modules/libpq_pipeline/libpq_pipeline cancel
'port=5432' -t test.trace

And then look at the top of test.trace

[1]: https://www.postgresql.org/message-id/CAGECzQTTN5aGqtDaRifJXPyd_O5qHWQcOxsHJsDSVNqMugGNEA%40mail.gmail.com

On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
> 0001 is some cleanup after f4b54e1ed9

Oops.  I'll plan on committing this after the 17beta2 release freeze is
lifted.

-- 
nathan

On Fri, Jun 21, 2024 at 04:01:55PM -0500, Nathan Bossart wrote:
> On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
>> 0001 is some cleanup after f4b54e1ed9
> 
> Oops.  I'll plan on committing this after the 17beta2 release freeze is
> lifted.

Committed 0001.

-- 
nathan

On 2024-Jun-26, Nathan Bossart wrote:

> On Fri, Jun 21, 2024 at 04:01:55PM -0500, Nathan Bossart wrote:
> > On Fri, Jun 21, 2024 at 11:22:05AM +0200, Jelte Fennema-Nio wrote:
> >> 0001 is some cleanup after f4b54e1ed9
> > 
> > Oops.  I'll plan on committing this after the 17beta2 release freeze is
> > lifted.
> 
> Committed 0001.

Thanks, Nathan.  I'm holding myself responsible for the rest ... will
handle soon after the branch.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The problem with the future is that it keeps turning into the present"
(Hobbes)

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.

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

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