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
