Iwata-san, On Mon, Dec 21, 2020 at 5:20 PM k.jami...@fujitsu.com <k.jami...@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/