Re: libpq debug log

Hello. Thank you for the new patch.

At Tue, 9 Apr 2019 06:19:32 +0000, "Iwata, Aya" <iwata.aya@jp.fujitsu.com> wrote in
<71E660EB361DF14299875B198D4CE5423DF161BA@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