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>
Cc: 'Alvaro Herrera' <alvherre@alvh.no-ip.org>, "'pgsql-hackers@lists.postgresql.org'" <pgsql-hackers@lists.postgresql.org>
Date: 2021-01-25T09:20:29Z
Lists: pgsql-hackers 
Iwata-san,


(25)
+		conn->fe_msg = malloc(sizeof(offsetof(pqFrontendMessage, fields)) +
+							  DEF_FE_MSGFIELDS * sizeof(pqFrontendMessageField));

It's incorrect that offsetof() is nested in sizeof().  See my original review comment.


(26)
+bool
+pqTraceInit(PGconn *conn, bits32 flags)
+{
...
+		conn->be_msg->state = LOG_FIRST_BYTE;
+		conn->be_msg->length = 0;
+	}
...
+	conn->be_msg->state = LOG_FIRST_BYTE;
+	conn->be_msg->length = 0;

The former is redundant?


(27)
+/*
+ * Deallocate frontend-message tracking memory.  We only do this because
+ * it can grow arbitrarily large, and skip it in the initial state, because
+ * it's likely pointless.
+ */
+void
+pqTraceUninit(PGconn *conn)
+{
+	if (conn->fe_msg &&
+		conn->fe_msg->num_fields != DEF_FE_MSGFIELDS)
+	{
+		pfree(conn->fe_msg);
+		conn->fe_msg = NULL;
+	}
+}

I thought this function plays the reverse role of pqTraceInit(), but it only frees memory for frontend messages.  Plus, use free() instead of pfree(), because malloc() is used to allocate memory.


(28)
+void PQtrace(PGconn *conn, FILE *stream, bits32 flags);

bits32 can't be used because it's a data type defined in c.h, which should not be exposed to applications.  I think you can just int or something.


Considering these and the compilation error Kirk-san found, I'd like you to do more self-review before I resume this review.


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.