Thread

RE: libpq debug log

Aya Iwata (Fujitsu) <iwata.aya@fujitsu.com> — 2021-03-19T07:38:57Z

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>&gt;</literal> for messages from client to server,
> +      and <literal>&lt;</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