Horton

RE: libpq debug log

tsunakawa.takay@fujitsu.com <tsunakawa.takay@fujitsu.com>

View thread View on postgr.es
From: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>
To: "iwata.aya@fujitsu.com" <iwata.aya@fujitsu.com>, 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>
Cc: "k.jamison@fujitsu.com" <k.jamison@fujitsu.com>, "alvherre@alvh.no-ip.org" <alvherre@alvh.no-ip.org>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2021-02-09T08:10:19Z
Lists: pgsql-hackers 
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> > (45)
> This looks like a fusion of PQtrace and PQtraceEX. By the way, the
> timestamp flag is needed at log emittion. So we can change the state
> anytime.
> 
> PQtrace(conn, of);
> PQtraceSetFlags(conn, PQTRACE_SUPPRESS_TIMESTAMPS);
> <logging without timestamps>
> PQtraceSetFlags(conn, 0);
> <logging with timestamps>

I find this better because the application does not have to call PQuntrace() and PQtrace() again to enable/disable timestamp output, which requires passing the FILE pointer again.  (I don't imagine applications would repeatedly turn logging on and off in practice, though.)


> > (46)
> >
> > If skipLogging is intended for use with backend -> frontend messages only,
> shouldn't it be placed in conn->b_msg?
> 
> The name skipLogging is somewhat obscure. The flag doesn't inhibit all
> logs from being emitted.  It seems like it represents how far bytes
> the logging mechanism consumed for the limited cases. Thus, I think it
> can be a cursor variable like inCursor.
> 
> If we have conn->be_msg->inLogged, for example, pqGetc and
> pqLogMessageByte1() are written as the follows.
> 
> pqGetc(char *result, PGconn *conn)
> {
> 	if (conn->inCursor >= conn->inEnd)
> 		return EOF;
> 
> 	*result = conn->inBuffer[conn->inCursor++];
> 
> 	if (conn->Pfdebug)
> 		pqLogMessageByte1(conn, *result);
> 
> 	return 0;
> }
> 
> pqLogMessageByte1(...)
> {
>    switch()
>    {
>      case LOG_FIRST_BYTE:
> 	   /* No point in logging already logged bytes */
>        if (conn->be_msg->inLogged >= conn->inCursor)
> 	     return;
>    ...
>    }
>    conn->be_msg->inLogged = conn->inCursor;
> }

This looks better design because stuff like skipLogging is an internal state of logging facility whose use should be restricted to fe-logging.c.

> (pqCheckInBufferSpace needs to adjust inLogged.)
> 
> I'm not sure this is easier to read than the skipLogging.

I'd like Iwata-san to evaluate this and decide whether to take this approach or the current one.


Regards
Takayuki Tsunakawa

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.