Rename libpq trace internal functions

Started by Peter Eisentrautover 1 year ago3 messages
#1Peter Eisentraut
peter@eisentraut.org
1 attachment(s)

libpq's pqTraceOutputMessage() used to look like this:

case 'Z': /* Ready For Query */
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

Commit f4b54e1ed98 introduced macros for protocol characters, so now
it looks like this:

case PqMsg_ReadyForQuery:
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

But this introduced a disconnect between the symbol in the switch case
and the function name to be called, so this made the manageability of
this file a bit worse.

This patch changes the function names to match, so now it looks like
this:

case PqMsg_ReadyForQuery:
pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
break;

(This also improves the readability of the file in general, since some
function names like "pqTraceOutputt" were a little hard to read
accurately.)

Some protocol characters have different meanings to and from the
server. The old code structure had a common function for both, for
example, pqTraceOutputD(). The new structure splits this up into
separate ones to match the protocol message name, like
pqTraceOutput_Describe() and pqTraceOutput_DataRow().

Attachments:

0001-Rename-libpq-trace-internal-functions.patchtext/plain; charset=UTF-8; name=0001-Rename-libpq-trace-internal-functions.patchDownload
From 00a9abce09ec22ff9eb617123bd4bda5701118a2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 24 Apr 2024 09:27:46 +0200
Subject: [PATCH] Rename libpq trace internal functions

libpq's pqTraceOutputMessage() used to look like this:

    case 'Z':               /* Ready For Query */
        pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
        break;

Commit f4b54e1ed98 introduced macros for protocol characters, so now
it looks like this:

    case PqMsg_ReadyForQuery:
        pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
        break;

But this introduced a disconnect between the symbol in the switch case
and the function name to be called, so this made the manageability of
this file a bit worse.

This patch changes the function names to match, so now it looks like
this:

    case PqMsg_ReadyForQuery:
        pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
        break;

(This also improves the readability of the file in general, since some
function names like "pqTraceOutputt" were a little hard to read
accurately.)

Some protocol characters have different meanings to and from the
server.  The old code structure had a common function for both, for
example, pqTraceOutputD().  The new structure splits this up into
separate ones to match the protocol message name, like
pqTraceOutput_Describe() and pqTraceOutput_DataRow().
---
 src/interfaces/libpq/fe-trace.c | 195 +++++++++++++++-----------------
 1 file changed, 91 insertions(+), 104 deletions(-)

diff --git a/src/interfaces/libpq/fe-trace.c b/src/interfaces/libpq/fe-trace.c
index c9932fc8a6b..2f5d490c5ee 100644
--- a/src/interfaces/libpq/fe-trace.c
+++ b/src/interfaces/libpq/fe-trace.c
@@ -215,9 +215,8 @@ pqTraceOutputNchar(FILE *pfdebug, int len, const char *data, int *cursor)
  * Output functions by protocol message type
  */
 
-/* NotificationResponse */
 static void
-pqTraceOutputA(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_NotificationResponse(FILE *f, const char *message, int *cursor, bool regress)
 {
 	fprintf(f, "NotificationResponse\t");
 	pqTraceOutputInt32(f, message, cursor, regress);
@@ -225,9 +224,8 @@ pqTraceOutputA(FILE *f, const char *message, int *cursor, bool regress)
 	pqTraceOutputString(f, message, cursor, false);
 }
 
-/* Bind */
 static void
-pqTraceOutputB(FILE *f, const char *message, int *cursor)
+pqTraceOutput_Bind(FILE *f, const char *message, int *cursor)
 {
 	int			nparams;
 
@@ -256,49 +254,45 @@ pqTraceOutputB(FILE *f, const char *message, int *cursor)
 		pqTraceOutputInt16(f, message, cursor);
 }
 
-/* Close(F) or CommandComplete(B) */
 static void
-pqTraceOutputC(FILE *f, bool toServer, const char *message, int *cursor)
+pqTraceOutput_Close(FILE *f, const char *message, int *cursor)
 {
-	if (toServer)
-	{
-		fprintf(f, "Close\t");
-		pqTraceOutputByte1(f, message, cursor);
-		pqTraceOutputString(f, message, cursor, false);
-	}
-	else
-	{
-		fprintf(f, "CommandComplete\t");
-		pqTraceOutputString(f, message, cursor, false);
-	}
+	fprintf(f, "Close\t");
+	pqTraceOutputByte1(f, message, cursor);
+	pqTraceOutputString(f, message, cursor, false);
+}
+
+static void
+pqTraceOutput_CommandComplete(FILE *f, const char *message, int *cursor)
+{
+	fprintf(f, "CommandComplete\t");
+	pqTraceOutputString(f, message, cursor, false);
 }
 
-/* Describe(F) or DataRow(B) */
 static void
-pqTraceOutputD(FILE *f, bool toServer, const char *message, int *cursor)
+pqTraceOutput_DataRow(FILE *f, const char *message, int *cursor)
 {
-	if (toServer)
+	int			nfields;
+	int			len;
+	int			i;
+
+	fprintf(f, "DataRow\t");
+	nfields = pqTraceOutputInt16(f, message, cursor);
+	for (i = 0; i < nfields; i++)
 	{
-		fprintf(f, "Describe\t");
-		pqTraceOutputByte1(f, message, cursor);
-		pqTraceOutputString(f, message, cursor, false);
+		len = pqTraceOutputInt32(f, message, cursor, false);
+		if (len == -1)
+			continue;
+		pqTraceOutputNchar(f, len, message, cursor);
 	}
-	else
-	{
-		int			nfields;
-		int			len;
-		int			i;
+}
 
-		fprintf(f, "DataRow\t");
-		nfields = pqTraceOutputInt16(f, message, cursor);
-		for (i = 0; i < nfields; i++)
-		{
-			len = pqTraceOutputInt32(f, message, cursor, false);
-			if (len == -1)
-				continue;
-			pqTraceOutputNchar(f, len, message, cursor);
-		}
-	}
+static void
+pqTraceOutput_Describe(FILE *f, const char *message, int *cursor)
+{
+	fprintf(f, "Describe\t");
+	pqTraceOutputByte1(f, message, cursor);
+	pqTraceOutputString(f, message, cursor, false);
 }
 
 /* NoticeResponse / ErrorResponse */
@@ -322,31 +316,29 @@ pqTraceOutputNR(FILE *f, const char *type, const char *message, int *cursor,
 	}
 }
 
-/* Execute(F) or ErrorResponse(B) */
 static void
-pqTraceOutputE(FILE *f, bool toServer, const char *message, int *cursor, bool regress)
+pqTraceOutput_ErrorResponse(FILE *f, const char *message, int *cursor, bool regress)
 {
-	if (toServer)
-	{
-		fprintf(f, "Execute\t");
-		pqTraceOutputString(f, message, cursor, false);
-		pqTraceOutputInt32(f, message, cursor, false);
-	}
-	else
-		pqTraceOutputNR(f, "ErrorResponse", message, cursor, regress);
+	pqTraceOutputNR(f, "ErrorResponse", message, cursor, regress);
 }
 
-/* CopyFail */
 static void
-pqTraceOutputf(FILE *f, const char *message, int *cursor)
+pqTraceOutput_Execute(FILE *f, const char *message, int *cursor, bool regress)
+{
+	fprintf(f, "Execute\t");
+	pqTraceOutputString(f, message, cursor, false);
+	pqTraceOutputInt32(f, message, cursor, false);
+}
+
+static void
+pqTraceOutput_CopyFail(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "CopyFail\t");
 	pqTraceOutputString(f, message, cursor, false);
 }
 
-/* FunctionCall */
 static void
-pqTraceOutputF(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_FunctionCall(FILE *f, const char *message, int *cursor, bool regress)
 {
 	int			nfields;
 	int			nbytes;
@@ -371,9 +363,8 @@ pqTraceOutputF(FILE *f, const char *message, int *cursor, bool regress)
 	pqTraceOutputInt16(f, message, cursor);
 }
 
-/* CopyInResponse */
 static void
-pqTraceOutputG(FILE *f, const char *message, int *cursor)
+pqTraceOutput_CopyInResponse(FILE *f, const char *message, int *cursor)
 {
 	int			nfields;
 
@@ -385,9 +376,8 @@ pqTraceOutputG(FILE *f, const char *message, int *cursor)
 		pqTraceOutputInt16(f, message, cursor);
 }
 
-/* CopyOutResponse */
 static void
-pqTraceOutputH(FILE *f, const char *message, int *cursor)
+pqTraceOutput_CopyOutResponse(FILE *f, const char *message, int *cursor)
 {
 	int			nfields;
 
@@ -399,18 +389,16 @@ pqTraceOutputH(FILE *f, const char *message, int *cursor)
 		pqTraceOutputInt16(f, message, cursor);
 }
 
-/* BackendKeyData */
 static void
-pqTraceOutputK(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_BackendKeyData(FILE *f, const char *message, int *cursor, bool regress)
 {
 	fprintf(f, "BackendKeyData\t");
 	pqTraceOutputInt32(f, message, cursor, regress);
 	pqTraceOutputInt32(f, message, cursor, regress);
 }
 
-/* Parse */
 static void
-pqTraceOutputP(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_Parse(FILE *f, const char *message, int *cursor, bool regress)
 {
 	int			nparams;
 
@@ -423,34 +411,30 @@ pqTraceOutputP(FILE *f, const char *message, int *cursor, bool regress)
 		pqTraceOutputInt32(f, message, cursor, regress);
 }
 
-/* Query */
 static void
-pqTraceOutputQ(FILE *f, const char *message, int *cursor)
+pqTraceOutput_Query(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "Query\t");
 	pqTraceOutputString(f, message, cursor, false);
 }
 
-/* Authentication */
 static void
-pqTraceOutputR(FILE *f, const char *message, int *cursor)
+pqTraceOutput_Authentication(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "Authentication\t");
 	pqTraceOutputInt32(f, message, cursor, false);
 }
 
-/* ParameterStatus */
 static void
-pqTraceOutputS(FILE *f, const char *message, int *cursor)
+pqTraceOutput_ParameterStatus(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "ParameterStatus\t");
 	pqTraceOutputString(f, message, cursor, false);
 	pqTraceOutputString(f, message, cursor, false);
 }
 
-/* ParameterDescription */
 static void
-pqTraceOutputt(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_ParameterDescription(FILE *f, const char *message, int *cursor, bool regress)
 {
 	int			nfields;
 
@@ -461,9 +445,8 @@ pqTraceOutputt(FILE *f, const char *message, int *cursor, bool regress)
 		pqTraceOutputInt32(f, message, cursor, regress);
 }
 
-/* RowDescription */
 static void
-pqTraceOutputT(FILE *f, const char *message, int *cursor, bool regress)
+pqTraceOutput_RowDescription(FILE *f, const char *message, int *cursor, bool regress)
 {
 	int			nfields;
 
@@ -482,18 +465,16 @@ pqTraceOutputT(FILE *f, const char *message, int *cursor, bool regress)
 	}
 }
 
-/* NegotiateProtocolVersion */
 static void
-pqTraceOutputv(FILE *f, const char *message, int *cursor)
+pqTraceOutput_NegotiateProtocolVersion(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "NegotiateProtocolVersion\t");
 	pqTraceOutputInt32(f, message, cursor, false);
 	pqTraceOutputInt32(f, message, cursor, false);
 }
 
-/* FunctionCallResponse */
 static void
-pqTraceOutputV(FILE *f, const char *message, int *cursor)
+pqTraceOutput_FunctionCallResponse(FILE *f, const char *message, int *cursor)
 {
 	int			len;
 
@@ -503,9 +484,8 @@ pqTraceOutputV(FILE *f, const char *message, int *cursor)
 		pqTraceOutputNchar(f, len, message, cursor);
 }
 
-/* CopyBothResponse */
 static void
-pqTraceOutputW(FILE *f, const char *message, int *cursor, int length)
+pqTraceOutput_CopyBothResponse(FILE *f, const char *message, int *cursor, int length)
 {
 	fprintf(f, "CopyBothResponse\t");
 	pqTraceOutputByte1(f, message, cursor);
@@ -514,9 +494,8 @@ pqTraceOutputW(FILE *f, const char *message, int *cursor, int length)
 		pqTraceOutputInt16(f, message, cursor);
 }
 
-/* ReadyForQuery */
 static void
-pqTraceOutputZ(FILE *f, const char *message, int *cursor)
+pqTraceOutput_ReadyForQuery(FILE *f, const char *message, int *cursor)
 {
 	fprintf(f, "ReadyForQuery\t");
 	pqTraceOutputByte1(f, message, cursor);
@@ -575,10 +554,10 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer)
 			/* No message content */
 			break;
 		case PqMsg_NotificationResponse:
-			pqTraceOutputA(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_NotificationResponse(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_Bind:
-			pqTraceOutputB(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_Bind(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_CopyDone:
 			fprintf(conn->Pfdebug, "CopyDone");
@@ -587,7 +566,10 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer)
 		case PqMsg_CommandComplete:
 			/* Close(F) and CommandComplete(B) use the same identifier. */
 			Assert(PqMsg_Close == PqMsg_CommandComplete);
-			pqTraceOutputC(conn->Pfdebug, toServer, message, &logCursor);
+			if (toServer)
+				pqTraceOutput_Close(conn->Pfdebug, message, &logCursor);
+			else
+				pqTraceOutput_CommandComplete(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_CopyData:
 			/* Drop COPY data to reduce the overhead of logging. */
@@ -595,37 +577,42 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer)
 		case PqMsg_Describe:
 			/* Describe(F) and DataRow(B) use the same identifier. */
 			Assert(PqMsg_Describe == PqMsg_DataRow);
-			pqTraceOutputD(conn->Pfdebug, toServer, message, &logCursor);
+			if (toServer)
+				pqTraceOutput_Describe(conn->Pfdebug, message, &logCursor);
+			else
+				pqTraceOutput_DataRow(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_Execute:
 			/* Execute(F) and ErrorResponse(B) use the same identifier. */
 			Assert(PqMsg_Execute == PqMsg_ErrorResponse);
-			pqTraceOutputE(conn->Pfdebug, toServer, message, &logCursor,
-						   regress);
+			if (toServer)
+				pqTraceOutput_Execute(conn->Pfdebug, message, &logCursor, regress);
+			else
+				pqTraceOutput_ErrorResponse(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_CopyFail:
-			pqTraceOutputf(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_CopyFail(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_FunctionCall:
-			pqTraceOutputF(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_FunctionCall(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_CopyInResponse:
-			pqTraceOutputG(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_CopyInResponse(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_Flush:
 			/* Flush(F) and CopyOutResponse(B) use the same identifier */
 			Assert(PqMsg_CopyOutResponse == PqMsg_Flush);
-			if (!toServer)
-				pqTraceOutputH(conn->Pfdebug, message, &logCursor);
-			else
+			if (toServer)
 				fprintf(conn->Pfdebug, "Flush");	/* no message content */
+			else
+				pqTraceOutput_CopyOutResponse(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_EmptyQueryResponse:
 			fprintf(conn->Pfdebug, "EmptyQueryResponse");
 			/* No message content */
 			break;
 		case PqMsg_BackendKeyData:
-			pqTraceOutputK(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_BackendKeyData(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_NoData:
 			fprintf(conn->Pfdebug, "NoData");
@@ -636,47 +623,47 @@ pqTraceOutputMessage(PGconn *conn, const char *message, bool toServer)
 							&logCursor, regress);
 			break;
 		case PqMsg_Parse:
-			pqTraceOutputP(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_Parse(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_Query:
-			pqTraceOutputQ(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_Query(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_AuthenticationRequest:
-			pqTraceOutputR(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_Authentication(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_PortalSuspended:
 			fprintf(conn->Pfdebug, "PortalSuspended");
 			/* No message content */
 			break;
 		case PqMsg_Sync:
-			/* Parameter Status(B) and Sync(F) use the same identifier */
+			/* ParameterStatus(B) and Sync(F) use the same identifier */
 			Assert(PqMsg_ParameterStatus == PqMsg_Sync);
-			if (!toServer)
-				pqTraceOutputS(conn->Pfdebug, message, &logCursor);
-			else
+			if (toServer)
 				fprintf(conn->Pfdebug, "Sync"); /* no message content */
+			else
+				pqTraceOutput_ParameterStatus(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_ParameterDescription:
-			pqTraceOutputt(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_ParameterDescription(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_RowDescription:
-			pqTraceOutputT(conn->Pfdebug, message, &logCursor, regress);
+			pqTraceOutput_RowDescription(conn->Pfdebug, message, &logCursor, regress);
 			break;
 		case PqMsg_NegotiateProtocolVersion:
-			pqTraceOutputv(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_NegotiateProtocolVersion(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_FunctionCallResponse:
-			pqTraceOutputV(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_FunctionCallResponse(conn->Pfdebug, message, &logCursor);
 			break;
 		case PqMsg_CopyBothResponse:
-			pqTraceOutputW(conn->Pfdebug, message, &logCursor, length);
+			pqTraceOutput_CopyBothResponse(conn->Pfdebug, message, &logCursor, length);
 			break;
 		case PqMsg_Terminate:
 			fprintf(conn->Pfdebug, "Terminate");
 			/* No message content */
 			break;
 		case PqMsg_ReadyForQuery:
-			pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
+			pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
 			break;
 		default:
 			fprintf(conn->Pfdebug, "Unknown message: %02x", id);

base-commit: 286eea5d3ae33a8c22d46a051f8ac6069c8b779d
-- 
2.44.0

#2Yugo NAGATA
nagata@sraoss.co.jp
In reply to: Peter Eisentraut (#1)
Re: Rename libpq trace internal functions

On Wed, 24 Apr 2024 09:39:02 +0200
Peter Eisentraut <peter@eisentraut.org> wrote:

libpq's pqTraceOutputMessage() used to look like this:

case 'Z': /* Ready For Query */
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

Commit f4b54e1ed98 introduced macros for protocol characters, so now
it looks like this:

case PqMsg_ReadyForQuery:
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

But this introduced a disconnect between the symbol in the switch case
and the function name to be called, so this made the manageability of
this file a bit worse.

This patch changes the function names to match, so now it looks like
this:

case PqMsg_ReadyForQuery:
pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
break;

+1

I prefer the new function names since it seems more natural and easier to read.

I noticed pqTraceOutputNR() is left as is, is this intentional? Or, shoud this
be changed to pqTranceOutput_NoticeResponse()?

Regards,
Yugo Nagata

(This also improves the readability of the file in general, since some
function names like "pqTraceOutputt" were a little hard to read
accurately.)

Some protocol characters have different meanings to and from the
server. The old code structure had a common function for both, for
example, pqTraceOutputD(). The new structure splits this up into
separate ones to match the protocol message name, like
pqTraceOutput_Describe() and pqTraceOutput_DataRow().

--
Yugo NAGATA <nagata@sraoss.co.jp>

#3Peter Eisentraut
peter@eisentraut.org
In reply to: Yugo NAGATA (#2)
Re: Rename libpq trace internal functions

On 24.04.24 12:34, Yugo NAGATA wrote:

On Wed, 24 Apr 2024 09:39:02 +0200
Peter Eisentraut <peter@eisentraut.org> wrote:

libpq's pqTraceOutputMessage() used to look like this:

case 'Z': /* Ready For Query */
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

Commit f4b54e1ed98 introduced macros for protocol characters, so now
it looks like this:

case PqMsg_ReadyForQuery:
pqTraceOutputZ(conn->Pfdebug, message, &logCursor);
break;

But this introduced a disconnect between the symbol in the switch case
and the function name to be called, so this made the manageability of
this file a bit worse.

This patch changes the function names to match, so now it looks like
this:

case PqMsg_ReadyForQuery:
pqTraceOutput_ReadyForQuery(conn->Pfdebug, message, &logCursor);
break;

+1

I prefer the new function names since it seems more natural and easier to read.

I noticed pqTraceOutputNR() is left as is, is this intentional? Or, shoud this
be changed to pqTranceOutput_NoticeResponse()?

pqTraceOutputNR() is shared code used internally by _ErrorResponse() and
_NoticeResponse(). I have updated the comments a bit to make this clearer.

With that, I have committed it. Thanks.