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: "'alvherre@alvh.no-ip.org'" <alvherre@alvh.no-ip.org>, "iwata.aya@fujitsu.com" <iwata.aya@fujitsu.com>
Cc: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>, 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2021-02-24T04:04:04Z
Lists: pgsql-hackers 
> From: alvherre@alvh.no-ip.org <alvherre@alvh.no-ip.org>
> I'll give this another look tomorrow, but I wanted to pass along that I prefer
> libpq-trace.{c,h} instead of libpq-logging.  I also renamed variable "pin" and
> pgindented.  I don't have any major reservations about this patch now, so I'll
> mark it ready-for-committer in case somebody else wants to have a look
> before push.
> 
> (Not sure about the use of the word "forcely")

Hi Iwata-san and Alvaro-san,

Thank you for updating the patch.
Although it's already set as "Ready for Committer", I just found minor parts
that need to be fixed.

(1) Doc: PQtraceSetFlags
+      <literal>flags</literal> contains flag bits describing the operating mode
+      of tracing.  If <literal>flags</literal> contains <literal>PQTRACE_SUPPRESS_TIMESTAMPS</literal>, 
+      then timestamp is not printed with each message. If set to 0 (default),tracing will be output with timestamp.
+      This function should be called after calling <function>PQtrace</function>.

Missing space. And can be paraphrased to: 
"If set to 0 (default), tracing with timestamp is printed."


(2)
+ * pqTraceMaybeBreakLine:
+ *             Check whether the backend message is complete. If so, print a line
+ *             break and reset the buffer. If print break line, return 1.

The 2nd & last sentence can be combined to 
 "If so, print a line break, reset the buffer, and return 1."


(3) +PQtraceSetFlags(PGconn *conn, int flags)
+	/* If PQtrace() is failed, do noting. */

"If PQtrace() failed, do nothing."


(4)
> (Not sure about the use of the word "forcely")

I think it's not necessary.


Also, I tested the flag to not print timestamp. For example,
 PQtrace(conn, trace_file);
 PQtraceSetFlags(conn, PQTRACE_SUPPRESS_TIMESTAMPS);

And it did not print the timestamp. So it worked.
It also passed all the regression tests. (although PQtrace() is not tested in existing libpq tests).


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.