Thread

RE: libpq debug log

Aya Iwata (Fujitsu) <iwata.aya@fujitsu.com> — 2020-11-18T00:54:53Z

Hi Alvaro san,

Thank you for your email.
I will review this updated patch and I will let you know when I complete.
Please wait a moment.  

Best regards,
Aya Iwata

> -----Original Message-----
> From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Sent: Tuesday, October 27, 2020 1:23 AM
> To: Iwata, Aya/岩田 彩 <iwata.aya@fujitsu.com>
> Cc: pgsql-hackers@postgresql.org; tgl@sss.pgh.pa.us;
> robertmhaas@gmail.com; pchampion@pivotal.io; jdoty@pivotal.io;
> raam.soft@gmail.com; Nagaura, Ryohei/永浦 良平
> <nagaura.ryohei@fujitsu.com>; nagata@sraoss.co.jp;
> peter.eisentraut@2ndquadrant.com; 'Kyotaro HORIGUCHI'
> <horiguchi.kyotaro@lab.ntt.co.jp>; Jamison, Kirk/ジャミソン カーク
> <k.jamison@fujitsu.com>
> Subject: Re: libpq debug log
> 
> Hello Aya Iwata
> 
> I've been hacking at this patch again.  There were a few things I wasn't too
> clear about, so I reordered the code and renamed the routines to try to make it
> easier to follow.
> 
> One thing I didn't like very much is that all the structures and enums were
> exposed to the world in libq-int.h.  Because some enum members have
> pretty generic names, I didn't like that much, so I moved the whole thing to
> fe-misc.c, and renamed the structs.  Also, the arrays don't take space unless
> PQtrace() is called.  (This is not what I was talking about in my previous
> message.  The idea I was trying to explain in my previous message cannot
> possibly work, so I abandoned it.)
> 
> I also renamed functions to make it clear which handles frontend and which
> handles backend.  With that, it was pretty obvious that we had an "reset BE
> message" in the routine to handle FE, and some clearing of FE in the code that
> handles BE.  I fixed things in a way that I think makes the most sense.
> 
> I noticed that it's theoretically possible to have a FE message so large (more
> than MAXPGPATH pieces) that it would overrun the array; so I added a "print
> message" call after adding one piece, to avoid this.  Also, why MAXPGPATH?
> I added a new symbol MAX_FRONTEND_MSGS for this purpose.
> 
> There are some things still to do:
> 
> 0. I added a XXX comment to pqFlush.  Because we're storing messages in
> fe_msgs that point to the libpq buffer, is it possible to end up with trace
> messages that are pointing into outBuffer bytes that are already sent, and
> perhaps even overwritten with newer bytes?  Maybe not, but it's unclear.
> Should we do pqLogFrontendMsg() preventively to avoid this problem?
> 
> 1. Is the handling of protocol 2 correct?  Since it's completely separate from
> protocol 3, I have not even looked at what it produces.
> But the fact that pqLogMsgByte1 completely ignores the "commsource"
> argument makes me suspect that it's not correct.
> 1a. How do we even test protocol 2 handling?
> 
> 2. We need a mode to suppress print of time; this would be useful to be able to
> test libpq at a low level.  Maybe instead of time we can print a
> monotonically-increasing packet sequence number.  With this, we could
> easily add tests for libpq itself.  What user interface do we want for this?
> Maybe we can add an "bits32 flags" parameter to PQtrace(), with one bit for
> this use.
> 
> 3. COPY ... (FORMAT BINARY) emits "invalid protocol" ... not good.
> 
> 4. Even in text format, COPY output is not very useful.  How can we improve
> that?
> 
> 5. Error messages are still printing the terminating zero byte.  I suggest that
> it should be suppressed.
> 
> 6. Let's redefine pqTraceMaybeBreakLine() (I renamed from
> pqLogLineBreak):  If message is complete, print a newline; if message is not
> complete, print a " ".  Then, remove the whitespace after printing each
> element.  This should lead to lines that don't have an useless " "
> at the end.
> 
> 7. Why does it make sense to call pqTraceMaybeBreakLine when
> commsource=FROM_FRONTEND?