Horton

RE: libpq debug log

Iwata, Aya <iwata.aya@jp.fujitsu.com>

View thread View on postgr.es
From: "Iwata, Aya" <iwata.aya@jp.fujitsu.com>
To: 'Kyotaro HORIGUCHI' <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: "tgl@sss.pgh.pa.us" <tgl@sss.pgh.pa.us>, "robertmhaas@gmail.com" <robertmhaas@gmail.com>, "pchampion@pivotal.io" <pchampion@pivotal.io>, "jdoty@pivotal.io" <jdoty@pivotal.io>, "raam.soft@gmail.com" <raam.soft@gmail.com>, "Jamison, Kirk" <k.jamison@jp.fujitsu.com>, "Nagaura, Ryohei" <nagaura.ryohei@jp.fujitsu.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, "nagata@sraoss.co.jp" <nagata@sraoss.co.jp>, "kommi.haribabu@gmail.com" <kommi.haribabu@gmail.com>, "peter.eisentraut@2ndquadrant.com" <peter.eisentraut@2ndquadrant.com>
Date: 2019-04-16T07:23:21Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Rename PQtraceSetFlags() to PQsetTraceFlags().

  2. Suppress length of Notice/Error msgs in PQtrace regress mode

  3. Strip file names reported in error messages on Windows, too.

  4. Fix setvbuf()-induced crash in libpq_pipeline

  5. libpq_pipeline: Must strdup(optarg) to avoid crash

  6. Remove setvbuf() call from PQtrace()

  7. Initialize conn->Pfdebug to NULL when creating a connection

  8. Disable force_parallel_mode in libpq_pipeline

  9. libpq_pipeline: add PQtrace() support and tests

  10. Improve PQtrace() output format

  11. Re-simplify management of inStart in pqParseInput3's subroutines.