RE: libpq debug log

Alvaro-san,


Thank you for taking your time to take a look at an incomplete patch.  I thought I would ask you for final check for commit after Iwata-san has reflected my review comments.

I discussed with Iwata-sn your below comments.  Let me convey her opinions.  (She is now focusing on fixing the patch.)  We'd appreciate if you could tweak her completed patch.


From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> * There's no cross-check that the protocol message length word
>   corresponds to what we actually print.  I think one easy way to
>   cross-check that would be to return the "count" from the type-specific
>   routines, and verify that it matches "length" in pqTraceOutputMessage.
>   (If the separate routines are removed and the code put back in one
>   giant switch, then the "count" is already available when the switch
>   ends.)

We don't think the length check is necessary, because 1) for FE->BE, correct messages are always assembled, and 2) for BE->FE, the parsing and decomposition of messages have already checked the messages.


> * If we have separate functions for each message type, then we can have
>   that function supply the message name by itself, rather than have the
>   separate protocol_message_type_f / _b arrays that print it.

I felt those two arrays are clumsy and thought of suggesting to remove them.  But I didn't because the functions or case labels for each message type have to duplicate the printing of header fields: timestamp, message type, and length.  Maybe we can change the order of output to "timestamp length message_type content", but I kind of prefer the current order.  What do you think?  (I can understand removing the clumsy static arrays should be better than the order of output fields.)


> * If we make each type-specific function print the message name, then we
>   need to make the same function print the message length, because it
>   comes after.  So the function would have to receive the length (so
>   that it can be printed).  But I think we should still have the
>   cross-check I mentioned in my first point occur in the main
>   pqTraceOutputMessage, not the type-specific function, for fear that we
>   will forget the cross-check in one of the functions and never realize
>   that we did.

As mentioned above, we think the current structure is better for smaller and readable code.


> * I would make the pqTraceOutputInt16() function and siblings advance
>   the cursor themselves, actually.  I think something like this:
>   static int
>   pqTraceOutputInt16(const char *data, int *cursor, FILE *pfdebug)
>   {
> 	  uint16		tmp;
> 	  int			result;
> 
> 	  memcpy(&tmp, data + *cursor, 2);
> 	  *cursor += 2;
> 	  result = (int) pg_ntoh16(tmp);
> 	  fprintf(pfdebug, " #%d", result);
> 
> 	  return result;
>   }
>   (So the caller has to pass the original "data" pointer, not
>   "data+cursor").  This means the caller no longer has to do "cursor+=N"
>   at each place.  Also, you get rid of the moveStrCursor() which does
>   not look good.

That makes sense, because in fact the patch mistakenly added 4 when it should add 2.  Also, the code would become smaller.


> * I'm not fond of the idea of prefixing "#" for 16bit ints and no
>   prefix for 32bit ints.  Seems obscure and the output looks weird.
>   I would use a one-letter prefix for each type, "w" for 32-bit ints
>   (stands for "word") and "h" for 16-bit ints (stands for "half-word").
>   Message length goes unadorned.  Then you'd have lines like this
> 
> 2021-03-15 08:10:44.324296  <   RowDescription  35 h1 "spcoptions"
> w1213 h5 w1009 h65535 w-1 h0
> 2021-03-15 08:10:44.324335  <   DataRow 32 h1 w22
> '{random_page_cost=3.0}'

Yes, actually I felt something similar.  Taking a second thought, I think we don't have to have any prefix because it doesn't help users.  So we're removing the prefix.  We don't have any opinion on adding some prefix.


> * I don't like that pqTraceOutputS/H print nothing when !toServer.  I
> think they should complain.

Yes, the caller should not call the function if there's no message content.  That way, the function doesn't need the toServer argument.


	Regards
Takayuki Tsunakawa