Hi Alvaro san and Tsunakawa san, Thank you for your review. I updated patch to v27.
`make check` output is following. I think it is OK.
```
2021-03-18 07:02:55.090598 < ReadyForQuery 5 I
2021-03-18 07:02:55.090672 > Terminate 4
2021-03-18 07:02:55.120492 > Query 155 "CREATE TABLESPACE
regress_tblspacewith LOCATION
'/home/iwata/pgsql/postgres/src/test/regress/testtablespace' WITH
(some_nonexistent_parameter = true);"
2021-03-18 07:02:55.121624 < ErrorResponse 124 S "ERROR" V "ERROR" C
"22023" M "unrecognized parameter "some_nonexistent_parameter"" F
"reloptions.c" L "1447" R "parseRelOptionsInternal" \x00 "Z"
2021-03-18 07:02:55.121664 < ReadyForQuery 5 I
2021-03-18 07:02:55.121728 > Query 144 "CREATE TABLESPACE
regress_tblspacewith LOCATION
'/home/iwata/pgsql/postgres/src/test/regress/testtablespace' WITH
(random_page_cost = 3.0);"
2021-03-18 07:02:55.123335 < CommandComplete 22 "CREATE TABLESPACE"
2021-03-18 07:02:55.123400 < ReadyForQuery 5 I
2021-03-18 07:02:55.123460 > Query 81 "SELECT spcoptions FROM
pg_tablespace WHERE spcname = 'regress_tblspacewith';"
2021-03-18 07:02:55.126556 < RowDescription 35 1 "spcoptions" 1213 5 1009
65535 -1 0
2021-03-18 07:02:55.126594 < DataRow 32 1 22 '{random_page_cost=3.0}'
2021-03-18 07:02:55.126607 < CommandComplete 13 "SELECT 1"
2021-03-18 07:02:55.126617 < ReadyForQuery 5 I
```
> Iwata-san,
> Why don't you submit v27 patch with the current arrays kept, and then v28
> with the arrays removed soon after?
And I will try to remove byte1 type table for v28 patch.
> From: Tsunakawa, Takayuki/綱川 貴之 <tsunakawa.ta...@fujitsu.com>
> Sent: Thursday, March 18, 2021 12:38 PM
> From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
> > In pqTraceOutputString(), you can use the return value from fprintf to
> > move the cursor -- no need to count chars.
>
> Yes, precisely, 2 bytes for the double quotes needs to be subtracted as
> follows:
>
> len = fprintf(...);
> *cursor += (len - 2);
Thank you for your advice. I changed pqTraceOutputString set cursor to fprintf
return -2.
And I removed cursor movement from that function.
> From: Tsunakawa, Takayuki/綱川 貴之 <tsunakawa.ta...@fujitsu.com>
> Sent: Thursday, March 18, 2021 11:52 AM
...
> I'll look at the comments from Alvaro-san and Horiguchi-san. Here are my
> review comments:
Thank you for your review. I am sorry previous patch contain some mistake.
> (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 you want to remove the second
> condition from the outer if.
Yes, I remove second condition.
> (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 significant 16 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.
> --------------------------------------------------
Thank you. I read document again and I add one pqTraceOutputInt32().
> (25)
> + case 8 : /* GSSENRequest or SSLRequest */
>
> GSSENRequest -> GSSENCRequest
Thank you. I fixed.
> (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.
Thank you. I fixed this. I add space anywhere in pqTraceOutput? function.
> (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.
I initialized message_type as NULL, changed `else` to `if (message_type ==
NULL)`
and move message type setup code to after setting id.
> (28)
> pqTraceOutputBinary() is only used in pqTraceOutputNchar(). Do they have
> to be separated?
I see. I merged this code to pqTraceOutputBinary().
Regards,
Aya Iwata
v27-libpq-trace-log.patch
Description: v27-libpq-trace-log.patch
