RE: libpq debug log

From: "Iwata, Aya" <iwata(dot)aya(at)jp(dot)fujitsu(dot)com>
To: 'Kyotaro HORIGUCHI' <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "robertmhaas(at)gmail(dot)com" <robertmhaas(at)gmail(dot)com>, "pchampion(at)pivotal(dot)io" <pchampion(at)pivotal(dot)io>, "jdoty(at)pivotal(dot)io" <jdoty(at)pivotal(dot)io>, "raam(dot)soft(at)gmail(dot)com" <raam(dot)soft(at)gmail(dot)com>, "Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>, "Nagaura, Ryohei" <nagaura(dot)ryohei(at)jp(dot)fujitsu(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "nagata(at)sraoss(dot)co(dot)jp" <nagata(at)sraoss(dot)co(dot)jp>, "kommi(dot)haribabu(at)gmail(dot)com" <kommi(dot)haribabu(at)gmail(dot)com>, "peter(dot)eisentraut(at)2ndquadrant(dot)com" <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Subject: RE: libpq debug log
Date: 2019-04-16 07:23:21
Message-ID: 71E660EB361DF14299875B198D4CE5423DF18A38@g01jpexmbkw25
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi Horiguchi-san,

Thank you for your reviewing.
I updated patch. Please see my attached patch.

> +/* 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.
Thank you. I changed the description more clear.

>
> + /* 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.
Sure. I removed.

> + * 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.
Thank you. I fixed function names and added descriptions.

>
> +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?
Elements sent from both the backend and the frontend are 'c' and 'd'.
There is no same elements in protocol_message_type_b and _bf.
The same applies to protocol_message_type_f and _bf too. So I think it is 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.
Thank you. I fixed.

Regards,
Aya Iwata

Attachment Content-Type Size
v3-libpq-PQtrace-output-one-line.patch application/octet-stream 19.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peifeng Qiu 2019-04-16 09:17:50 Compile with 64-bit kerberos on Windows
Previous Message Magnus Hagander 2019-04-16 07:14:48 Re: Commit message / hash in commitfest page.