At Wed, 17 Mar 2021 02:09:32 +0000, "[email protected]"
<[email protected]> wrote in
> 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 <[email protected]>
> > * 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.
Maybe it is not for the sanity of message bytes but to check if the
logging-stuff is implemented in sync with the messages structure. If
the given message length differs from the length the logging facility
read after scanning a message bytes, it's sign of something wrong in
*the logging feature*.
> > * 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.)
+1 for removing the arrays.
> > * 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.
FWIW, that's what I suggested upthread:p So +3.
me> *I*'d want to make the output functions move the reading pointer by
me> themseves. pqTradeOutputMsg can be as simplified as the follows doing
> > * 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.
It would help when the value is "255", which is confusing between -1
(or 255) in byte or 255 in 2-byte word. Or when the value looks like
broken. I'd suggest "b"yte for 1 byte, "s"hort for 2 bytes, "l"ong for
4 bytes ('l' is confusing with '1', but anyway it is not needed)..
> > * 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.
--
Kyotaro Horiguchi
NTT Open Source Software Center
