RE: libpq debug log

I've not finished reviewing yet, but there seems to be many mistakes.  I'm sending second set of review comments now so you can fix them in parallel.


(8)
+	char 		id = '\0';

This initialization is not required because id will always be assigned a value shortly.


(9)
+static int
+pqTraceOutputInt32(const char *data, FILE *pfdebug)
+{
+	int			result;
+
+	memcpy(&result, data, 4);
+	result = (int) pg_ntoh32(result);
+	fputc(' ', pfdebug);
+	fprintf(pfdebug, "%d", result);
+
+	return result;
+}

fputc() and fprintf() can be merged into one fprintf() call for efficiency.

TBH, it feels strange that an "output" function returns a value.  But I understood this as a positive compromise for reducing code; without this, the caller has to do memcpy() and endianness conversion.


(10)
+/* BackendKeyData */
+static void
+pqTraceOutputK(const char *message, FILE *f)
+{
+	int	cursor = 0;
+
+	pqTraceOutputInt32(message + cursor, f);
+	cursor += 4;
+	pqTraceOutputInt32(message + cursor, f);
+}

I don't think you need to always use a cursor variable in order to align with more complex messages.  That is, you can just write:

+	pqTraceOutputInt32(message, f);
+	pqTraceOutputInt32(message + 4, f);


(11)
+		default:
+			fprintf(conn->Pfdebug, "Invalid Protocol:%c\n", id);
+			break;
+

(This is related to (7))
You can remove this default label if you exit the function before the switch statement when the message type is unknown.  Make sure to output \n in that case.


(12) pqTraceOutputB
+	for (i = 0; i < nparams; i++)
+	{
+		nbytes = pqTraceOutputInt32(message + cursor, f);
+		cursor += 4;
+		if (nbytes == -1)
+			break;
+		pqTraceOutputNchar(message + cursor, f, nbytes);
+		cursor += nbytes;
+	}

Not break but continue, because non-NULL parameters may follow a NULL parameter.


(13) pqTraceOutputB
+	pqTraceOutputInt16(message + cursor, f);
+	cursor += 4;
+	pqTraceOutputInt16(message + cursor, f);
+}

This part doesn't seem to match the following description.

----------
After the last parameter, the following fields appear:

Int16
The number of result-column format codes that follow (denoted R below). This can be zero to indicate that there are no result columns or that the result columns should all use the default format (text); or one, in which case the specified format code is applied to all result columns (if any); or it can equal the actual number of result columns of the query.

Int16[R]
The result-column format codes. Each must presently be zero (text) or one (binary).
----------


(14)
The processing for CancelRequest message is missing?


(15)
+/* CopyInResponse */
+static void
+pqTraceOutputG(const char *message, int end, FILE *f)
+{
+	int	cursor = 0;
+
+	pqTraceOutputByte1(message + cursor, f);
+	cursor++;
+
+	while (end > cursor)
+	{
+		pqTraceOutputInt16(message + cursor, f);
+		cursor += 2;
+	}
+}
+

According to the following description, for loop should be used.

----------
Int16
The number of columns in the data to be copied (denoted N below).

Int16[N]
The format codes to be used for each column. Each must presently be zero (text) or one (binary). All must be zero if the overall copy format is textual.
----------




Regards
Takayuki Tsunakawa