Re: libpq debug log

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: iwata(dot)aya(at)jp(dot)fujitsu(dot)com
Cc: tgl(at)sss(dot)pgh(dot)pa(dot)us, robertmhaas(at)gmail(dot)com, pchampion(at)pivotal(dot)io, jdoty(at)pivotal(dot)io, raam(dot)soft(at)gmail(dot)com, k(dot)jamison(at)jp(dot)fujitsu(dot)com, nagaura(dot)ryohei(at)jp(dot)fujitsu(dot)com, pgsql-hackers(at)postgresql(dot)org, nagata(at)sraoss(dot)co(dot)jp, kommi(dot)haribabu(at)gmail(dot)com, peter(dot)eisentraut(at)2ndquadrant(dot)com
Subject: Re: libpq debug log
Date: 2019-04-09 09:57:34
Message-ID: 20190409.185734.86276033.horiguchi.kyotaro@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello. Thank you for the new patch.

At Tue, 9 Apr 2019 06:19:32 +0000, "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com> wrote in <71E660EB361DF14299875B198D4CE5423DF161BA(at)g01jpexmbkw25>
> Hi,
>
> I update patch to improve PQtrace(); output log message in one line.
> Please find my attached patch.
>
> How it changed:
> > > The basic idea being:
> > >
> > > - Each line is a whole message.
> > > - The line begins with <<< for a message received and >>> for a message
> > sent.
> > > - Strings in single quotes are those sent/received as a fixed number of
> > bytes.
> > > - Strings in double quotes are those sent/received as a string.
> > > - 4-byte integers are printed unadorned.
> > > - 2-byte integers are prefixed by #.
> > > - I guess 1-byte integers would need some other prefix, maybe @ or ##.
>
> New log output examples:
> The message sent from frontend is like this;
> 2019-04-04 02:39:51.488 UTC > Query 59 "SELECT pg_catalog.set_config('search_path', '', false)"
>
> The message sent from backend is like this;
> 2019-04-04 02:39:51.489 UTC < RowDescription 35 #1 "set_config" 0 #0 25 #65535 -1 #0

I had a brief look on this.

+/* protocol message name */
+static char *command_text_b[] = {

Couldn't the name be more descriptive? The comment just above
doesn't seem consistent with the variable. The tables are very
sparse. I think the definition could be in more compact form.

+ /* y */ 0,
+ /* z */ 0
+};
+#define COMMAND_BF_MAX (sizeof(command_text_bf) / sizeof(*command_text_bf))

It seems that at least the trailing 0-elements are not required.

+ * message_get_command_text:
+ * Get Protocol message text from byte1 identifier
+ */
+static char *
+message_get_command_text(unsigned char c, CommunicationDirection direction)
..
+message_nchar(PGconn *conn, const char *v, int length, CommunicationDirection direction)

Also the function names are not very descriptive.

+message_Int(PGconn *conn, int v, int length, CommunicationDirection direct)

We are not using names mixing CamelCase and undercored there.

+ if (c >= 0 && c < COMMAND_BF_MAX)
+ {
+ text = command_text_bf[c];
+ if (text)
+ return text;
+ }
+
+ if (direction == FROM_BACKEND && c >= 0 && c < COMMAND_B_MAX)
+ {
+ text = command_text_b[c];
+ if (text)
..
+ if (direction == FROM_FRONTEND && c >= 0 && c < COMMAND_F_MAX)

This code is assuming that elements of command_text_bf is
mutually exclusive with command_text_b or _bf. That is, the first
has an element for 'C', others don't have an element in the same
position. But _bf[C] = "CommandComplete" and _f[C] = "Close". Is
it working correctly?

+typedef enum CommunicationDirection

The type CommunicationDirection is two-member enum which is
equivalent to just a boolean. Is there a reason you define that?

+typedef enum State
+typedef enum Type

The name is too generic.
+typedef struct _LoggingMsg
...
+} LoggingMsg;

Why the tag name is prefixed with an underscore?

+typedef struct _Frontend_Entry

The name doesn't give an idea of its characteristics.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ibrar Ahmed 2019-04-09 10:05:31 Re: dropdb --force
Previous Message Kyotaro HORIGUCHI 2019-04-09 09:49:42 Re: Problem with default partition pruning