Re: libpq debug log

Iwata-san,

On Mon, Dec 21, 2020 at 5:20 PM k(dot)jamison(at)fujitsu(dot)com
<k(dot)jamison(at)fujitsu(dot)com> wrote:
>
> On Tuesday, December 15, 2020 8:12 PM, Iwata-san wrote:
> > > There are some things still to do:
> > I worked on some to do.
>
> Hi Iwata-san,
>
> Thank you for updating the patch.
> I would recommend to register this patch in the upcoming commitfest
> to help us keep track of it. I will follow the thread to provide more
> reviews too.
>
> > > 1. Is the handling of protocol 2 correct?
> > Protocol 3.0 is implemented in PostgreSQL v7.4 or later. Therefore, most
> > servers and clients today want to connect using 3.0.
> > Also, wishful thinking, I think Protocol 2.0 will no longer be supported. So I
> > didn't develop it aggressively.
> > Another reason I'm concerned about implementing it would make the code
> > less maintainable.
>
> Though I have also not tested it with 2.0 server yet, do I have to download 7.3
> version to test that isn't it? Silly question, do we still want to have this
> feature for 2.0?
> I understand that protocol 2.0 is still supported, but it is only used for
> PostgreSQL versions 7.3 and earlier, which is not updated by fixes anymore
> since we only backpatch up to previous 5 versions. However I am not sure if
> it's a good idea, but how about if we just support this feature for protocol 3.
> There are similar chunks of code in fe-exec.c, PQsendPrepare(), PQsendDescribe(),
> etc. that already do something similar, so I just thought in hindsight if we can
> do the same.
> if (PG_PROTOCOL_MAJOR(conn->pversion) >= 3)
> {
> <new code here>
> }
> else
> {
> /* Protocol 2.0 isn't supported */
> printfPQExpBuffer(&conn->errorMessage,
> libpq_gettext("function requires at least protocol version 3.0\n"));
> return 0;
> }
>
> But if it's necessary to provide this improved trace output for 2.0 servers, then ignore what
> I suggested above, and although difficult I think we should test for protocol 2.0 in older server.
>
> Some minor code comments I noticed:
> 1.
> + LOG_FIRST_BYTE, /* logging the first byte identifing the
> + * protocol message type */
>
> "identifing" should be "identifying". And closing */ should be on 3rd line.
>
> 2.
> + case LOG_CONTENTS:
> + /* Suppresses printing terminating zero byte */
>
> --> Suppress printing of terminating zero byte
>

The patch got some review comments a couple weeks ago but the patch
was still marked as "needs review", which was incorrect. Also cfbot[1]
is unhappy with this patch. So I'm switching the patch as "waiting on
author".

Regards,

[1] http://commitfest.cputube.org/

--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/