Horton

Re: libpq debug log

Kyotaro Horiguchi <horikyota.ntt@gmail.com>

View thread View on postgr.es
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
To: tsunakawa.takay@fujitsu.com
Cc: iwata.aya@fujitsu.com, k.jamison@fujitsu.com, alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org
Date: 2021-02-09T08:29:31Z
Lists: pgsql-hackers 
At Tue, 9 Feb 2021 08:10:19 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> 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.)

Thanks and Yes, I also don't imagine that users change the timestmp
state repeatedly:p It's just a example.

> > > (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.

+1

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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.