From: | "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com> |
---|---|
To: | 'Alvaro Herrera' <alvherre(at)alvh(dot)no-ip(dot)org>, "iwata(dot)aya(at)fujitsu(dot)com" <iwata(dot)aya(at)fujitsu(dot)com> |
Cc: | 'Tom Lane' <tgl(at)sss(dot)pgh(dot)pa(dot)us>, 'Kyotaro Horiguchi' <horikyota(dot)ntt(at)gmail(dot)com>, "k(dot)jamison(at)fujitsu(dot)com" <k(dot)jamison(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: libpq debug log |
Date: | 2021-03-17 02:09:32 |
Message-ID: | TYAPR01MB29905780CF643499034A7A06FE6A9@TYAPR01MB2990.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)alvh(dot)no-ip(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2021-03-17 02:28:00 | Re: Index Skip Scan (new UniqueKeys) |
Previous Message | Thomas Munro | 2021-03-17 02:08:01 | Re: Consider Parallelism While Planning For REFRESH MATERIALIZED VIEW |