Hello. Thank you for the new patch.
At Tue, 9 Apr 2019 06:19:32 +0000, "Iwata, Aya" <iwata....@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
