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 <alvhe...@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
