Re: libpq debug log - Mailing list pgsql-hackers
From | Masahiko Sawada |
---|---|
Subject | Re: libpq debug log |
Date | |
Msg-id | CAD21AoA+_uT0x+ArKy7OhgVW67KNasUFAfmY=uC3L=E871wE3A@mail.gmail.com Whole thread Raw |
In response to | RE: libpq debug log ("k.jamison@fujitsu.com" <k.jamison@fujitsu.com>) |
List | pgsql-hackers |
Iwata-san, On Mon, Dec 21, 2020 at 5:20 PM k.jamison@fujitsu.com <k.jamison@fujitsu.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/
pgsql-hackers by date: