RE: libpq debug log - Mailing list pgsql-hackers

Hi Tsunakawa san,

Thank you for your review. I update patch to v29.

Output is following. It is fine.
```
2021-03-19 07:21:09.917302  >   4   Terminate
2021-03-19 07:21:09.961494  >   155 Query    "CREATE TABLESPACE regress_tblspacewith LOCATION
'/home/iwata/pgsql/postgres/src/test/regress/testtablespace'WITH (some_nonexistent_parameter = true);" 
2021-03-19 07:21:09.962657  <   124 ErrorResponse    S "ERROR" V "ERROR" C "22023" M "unrecognized parameter
"some_nonexistent_parameter""F "reloptions.c" L "1456" R "parseRelOptionsInternal" \x00 "Z" 
2021-03-19 07:21:09.962702  <   5   ReadyForQuery    I
2021-03-19 07:21:09.962767  >   144 Query    "CREATE TABLESPACE regress_tblspacewith LOCATION
'/home/iwata/pgsql/postgres/src/test/regress/testtablespace'WITH (random_page_cost = 3.0);" 
2021-03-19 07:21:09.967056  <   22  CommandComplete  "CREATE TABLESPACE"
2021-03-19 07:21:09.967092  <   5   ReadyForQuery    I
2021-03-19 07:21:09.967148  >   81  Query    "SELECT spcoptions FROM pg_tablespace WHERE spcname =
'regress_tblspacewith';"
2021-03-19 07:21:09.970338  <   35  RowDescription   1 "spcoptions" 1213 5 1009 65535 -1 0
2021-03-19 07:21:09.970402  <   32  DataRow  1 22 '{random_page_cost=3.0}'
2021-03-19 07:21:09.970420  <   13  CommandComplete  "SELECT 1"
2021-03-19 07:21:09.970431  <   5   ReadyForQuery    I
2021-03-19 07:21:09.970558  >   42  Query    "DROP TABLESPACE regress_tblspacewith;"
2021-03-19 07:21:09.971500  <   20  CommandComplete  "DROP TABLESPACE"
2021-03-19 07:21:09.971561  <   5   ReadyForQuery    I
```

> -----Original Message-----
> From: Tsunakawa, Takayuki/綱川 貴之 <tsunakawa.takay@fujitsu.com>
> Sent: Friday, March 19, 2021 11:37 AM


> Thanks to removing the static arrays, the code got much smaller.  I'm happy
> with this result.

Thank you so much. Your advice was very helpful in refactoring the patch.

> (1)
> +      (<literal>></literal> for messages from client to server,
> +      and <literal><</literal> for messages from server to client),
>
> I think this was meant to say "> or <".  So, maybe you should remove "," at
> the end of the first line, and replace "and" with "or".

I fixed.

>
>
> (2)
> +        case 8 :    /* GSSENCRequest or SSLRequest */
> +            /* These messsage does not reach here. */
>
> messsage does -> messages do

I fixed.

> (3)
> +    fprintf(f, "\tAuthentication");
>
> Why don't you move this \t in each message type to the end of:
>
> +    fprintf(conn->Pfdebug, "%s\t%s\t%d", timestr, prefix, length);

I fixed.

>
> Plus, don't we say in the manual that fields (timestamp, direction, length,
> message type, and message content) are separated by a tab?

Sure. I added following documentation;
+ Non-message contents fields (timestamp, direction, length and message type)
+ are separated by a tab. Message contents are separated by a space.

> Also, the code doesn't seem to separate the message type and content with a
> tab.

I fixed to output a tab at the end of message types.

> (4)
> Where did the processing for unknown message go in
> pqTraceOutputMessage()?  Add default label?


> (5)
> +        case 16:    /* CancelRequest */
> +            fprintf(conn->Pfdebug,
> "%s\t>\t%d\tCancelRequest", timestr, length);
>
> I think this should follow pqTraceOutputMessage().  That is, each case label
> only print the message type name in case other message types are added in
> the future.

Sure. I fixed pqTraceOutputNoTypeByteMessage() following to pqTraceOutputMessage() style.


Regards,
Aya Iwata