Horton

RE: libpq debug log

k.jamison@fujitsu.com <k.jamison@fujitsu.com>

View thread View on postgr.es
From: "k.jamison@fujitsu.com" <k.jamison@fujitsu.com>
To: "iwata.aya@fujitsu.com" <iwata.aya@fujitsu.com>, "'pgsql-hackers@lists.postgresql.org'" <pgsql-hackers@lists.postgresql.org>
Date: 2020-12-21T08:20:44Z
Lists: pgsql-hackers 
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

Commits

  1. Rename PQtraceSetFlags() to PQsetTraceFlags().

  2. Suppress length of Notice/Error msgs in PQtrace regress mode

  3. Strip file names reported in error messages on Windows, too.

  4. Fix setvbuf()-induced crash in libpq_pipeline

  5. libpq_pipeline: Must strdup(optarg) to avoid crash

  6. Remove setvbuf() call from PQtrace()

  7. Initialize conn->Pfdebug to NULL when creating a connection

  8. Disable force_parallel_mode in libpq_pipeline

  9. libpq_pipeline: add PQtrace() support and tests

  10. Improve PQtrace() output format

  11. Re-simplify management of inStart in pqParseInput3's subroutines.