RE: libpq debug log

I'm looking at the last file libpq-trace.c.  I'll continue the review after lunch.  Below are some comments so far.


(1)
-      Enables  tracing of the client/server communication to a debugging file stream.
+      Enables tracing of the client/server communication to a debugging file
+      stream.
+      Only available when protocol version 3 or higher is used.

The last line is unnecessary now that v2 is not supported.


(2)
@@ -113,6 +114,7 @@ install: all installdirs install-lib
 	$(INSTALL_DATA) $(srcdir)/libpq-fe.h '$(DESTDIR)$(includedir)'
 	$(INSTALL_DATA) $(srcdir)/libpq-events.h '$(DESTDIR)$(includedir)'
 	$(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)'
+	$(INSTALL_DATA) $(srcdir)/libpq-trace.h '$(DESTDIR)$(includedir_internal)'
 	$(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)'
 	$(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample'

Why is this necessary?


(3) libpq-trace.h
+#ifdef __cplusplus
+extern "C"
+{

This is unnecessary because pqTraceOutputMessage() is for libpq's internal use.  This extern is for the user's C++ application to call public libpq functions.

+#include "libpq-fe.h"
+#include "libpq-int.h"

I think these can be removed by declaring the struct and function as follows:

struct pg_conn;
extern void pqTraceOutputMessage(struct pg_conn *conn, const char *message, bool toServer);

But... after doing the above, only this declaration is left in libpq-trace.h.  Why don't you put your original declaration using PGconn in libpq-int.h and remove libpq-trace.h?


(4)
+	/* Trace message only when there is first 1 byte */
+	if (conn->Pfdebug && (conn->outCount < conn->outMsgStart))
+		pqTraceOutputMessage(conn, conn->outBuffer + conn->outCount, true);

() surrounding the condition after && can be removed.


(5)
+static const char *pqGetProtocolMsgType(unsigned char c,
+										bool toServer);

This is unnecessary because the function definition precedes its only one call site.


(6)
+ * We accumulate frontend message pieces in an array as the libpq code writes
+ * them, and log the complete message when pqTraceOutputFeMsg is called.
+ * For backend, we print the pieces as soon as we receive them from the server.

The first paragraph is no longer true.  I think this entire comment is unnecessary.


(7)
+static const char *
+pqGetProtocolMsgType(unsigned char c, bool toServer)
+{
+	if (toServer == true &&
+		c < lengthof(protocol_message_type_f) &&
+		protocol_message_type_f[c] != NULL)
+		return protocol_message_type_f[c];
+
+	if (toServer == false &&
+		c < lengthof(protocol_message_type_b) &&
+		protocol_message_type_b[c] != NULL)
+		return protocol_message_type_b[c];
+
+	return "UnknownMessage:";
+}

"== true/false" can be removed.  libpq doesn't appear to use such style.

Why don't we embed this processing in pqTraceOutputMessage() because this function is short and called only once?  The added benefit would be that you can print an invalid message type byte like this:

	fprintf(..., "Unknown message: %02x\n", ...);


Regards
Takayuki Tsunakawa