RE: libpq debug log - Mailing list pgsql-hackers

I'll look at the comments from Alvaro-san and Horiguchi-san.  Here are my review comments:


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

The inner else path doesn't seem to be reached because both the outer and inner if contain the same condition.  I think
youwant to remove the second condition from the outer if. 


(24) pqTraceOutputNoTypeByteMessage
+        case 16:    /* CancelRequest */
+            fprintf(conn->Pfdebug, "%s\t>\tCancelRequest\t%d", timestr, length);
+            pqTraceOutputInt32(message, &LogCursor, conn->Pfdebug);
+            pqTraceOutputInt32(message, &LogCursor, conn->Pfdebug);
+            break;

Another int32 data needs to be output as follows:

--------------------------------------------------
Int32(80877102)
The cancel request code. The value is chosen to contain 1234 in the most significant 16 bits, and 5678 in the least
significant16 bits. (To avoid confusion, this code must not be the same as any protocol version number.) 

Int32
The process ID of the target backend.

Int32
The secret key for the target backend.
--------------------------------------------------


(25)
+        case 8 :    /* GSSENRequest or SSLRequest */

GSSENRequest -> GSSENCRequest


(26)
+static void
+pqTraceOutputByte1(const char *data, int *cursor, FILE *pfdebug)
+{
+    const char *v = data + *cursor;
+    /*

+static void
+pqTraceOutputf(const char *message, int end, FILE *f)
+{
+    int    cursor = 0;
+    pqTraceOutputString(message, &cursor, end, f);
+}

Put an empty line to separate the declaration part and execution part.


(27)
+    const char    *message_type = "UnkownMessage";
+
+    id = message[LogCursor++];
+
+    memcpy(&length, message + LogCursor , 4);
+    length = (int) pg_ntoh32(length);
+    LogCursor += 4;
+    LogEnd = length - 4;

+    /* Get a protocol type from first byte identifier */
+    if (toServer &&
+        id < lengthof(protocol_message_type_f) &&
+        protocol_message_type_f[(unsigned char)id] != NULL)
+        message_type = protocol_message_type_f[(unsigned char)id];
+    else if (!toServer &&
+        id < lengthof(protocol_message_type_b) &&
+        protocol_message_type_b[(unsigned char)id] != NULL)
+        message_type = protocol_message_type_b[(unsigned char)id];
+    else
+    {
+        fprintf(conn->Pfdebug, "Unknown message: %02x\n", id);
+        return;
+    }
+

The initial value "UnkownMessage" is not used.  So, you can initialize message_type with NULL and do like:

+     if (...)
+        ...
+    else if (...)
+        ...
+
+    if (message_type == NULL)
+    {
+        fprintf(conn->Pfdebug, "Unknown message: %02x\n", id);
+        return;
+

Plus, I think this should be done before looking at the message length.


(28)
pqTraceOutputBinary() is only used in pqTraceOutputNchar().  Do they have to be separated?


Regards
Takayuki Tsunakawa