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
Regards,
Kirk Jamison
