Re: libpq debug log - Mailing list pgsql-hackers

Hello.

I came up with some random comments.

At Mon, 4 Mar 2019 08:13:00 +0000, "Iwata, Aya" <iwata.aya@jp.fujitsu.com> wrote in
<71E660EB361DF14299875B198D4CE5423DEF1844@g01jpexmbkw25>
> Hi,
> 
> Thank you for your comments and advice.
> 
> I'd like to consider your suggestions.
> I am planning to change libpq logging like this;
> 
> 1. Expand PQtrace() facility and improve libpq logging.
> 
> 2. Prepare "output level". There are 3 type of levels;
>     - TRADITIONAL    :  if set, outputs protocol messages
>     - LEVEL1        :  if set, outputs phase and time
>     - LEVEL2        :  if set, outputs both info TRADITIONAL and LEVEL1

You appear to want to segregate the "traditional" output from
what you want to emit. At least Tom is explicitly suggesting to
throw away the hypothtical use cases for it. You may sort out
what kind of information you/we want to emit as log messages from
scratch:p

You may organize log kinds into hierachical levels like server
log messages or into orthogonal types that are individually
turned on. But it is not necessarily be a parameter of a logging
processor. (mentioned below)

> 3. Add "output phase" information; This is the processing flow. (ex. When PQexec() start and end )

What is the advantage of it against just two independent messages
like PQexec_start and PQexec_end? (I don't see any advantage.)

> 4. Change output method to callback style; Default is stdout, and prepare other callback functions that will be used
morefrequently.
 

Are you going to make something less-used the default behavior? I
think no one is opposing rich functionality as far as it is
replaceable.

> 5. Initialization method;
> In current one: PQtrace(PGconn *conn, FILE *stream);
> Proposed change: PQtraceEx(PGconn *conn, FILE *stream, PQloggingProcessor callback_func , void *callback_arg,
PGLogminlevellevel);
 

I'm not sure we should add a new *EX() function. Couldn't we
just change the signature of PQtrace()?

callback_funs seems to be a single function. I think it's better
to have individual function for each message type.  Not
callback_func(PQLOG_EXEC_START, param_1, param_2,...)  ,but
PQloggingProcessor.PQexec_start(param_1, param_2, ...).

It is because the caller can simply pass values in its own type
to the function without casting or other transformations and
their types are checked statically.

I also think it's better that logger-specific paramters are not
passed in this level.  Maybe stream and level are logger-specific
paratmer, which can be combined into callback_arg, or can be
given via an API function.

> PQtrace() can be use as it is to consider compatibility with previous applications, 
> so I leave PQtrace() and created a new function PQtraceEx(). 
> 
> After discussing the abovementioned, then maybe we can discuss more about enabling trace output and changing the
outputstyle.
 

I'm not sure what you mean by "output style" but you can change
everything by replacing the whole callback processor, which may
be a dynamic loaded file which is loaded by the instruction in
both ~/.libpqrc and some API, like PQloadLoggingProcessor()?


> What do you think? I would appreciate your comments and suggestions.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center