[PATCH] Refactor pqformat.{c,h} and protocol.h

Started by Aleksander Alekseevover 1 year ago11 messages

aleksander@timescale.com

over 1 year ago

3 attachment(s)

Hi,

While investigating a bug report [1]/messages/by-id/1df84daa-7d0d-e8cc-4762-85523e45e5e7@mailbox.org I wanted to find all the pieces
of code that form PqMsg_DataRow messages and couldn't easily do it.
This is because one authors prefer writing:

pq_beginmessage(buf, 'D');

.. while others:

pq_beginmessage(buf, PqMsg_DataRow);

The proposed patchset fixes this.

- Patch 1 replaces all the char's with PqMsg's
- Patch 2 makes PqMsg an enum. This ensures that the problem will not
appear again in the future and also gives us a bit more type-safety.
- Patch 3 rearranges the order of the functions in pqformat.{c,h} a
bit to make the code easier to read.

[1]: /messages/by-id/1df84daa-7d0d-e8cc-4762-85523e45e5e7@mailbox.org

--
Best regards,
Aleksander Alekseev

Attachments:

v1-0001-Always-pass-PgMsg_-to-pg_beginmessage-_reuse.patchapplication/octet-stream; name=v1-0001-Always-pass-PgMsg_-to-pg_beginmessage-_reuse.patchDownload

From b0e9f664280bb6c748abe32424cf8068448968ec Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:07:16 +0300
Subject: [PATCH v1 1/3] Always pass PgMsg_* to pg_beginmessage[_reuse]

Before this patch the code could be written in one of the following ways:

1. pq_beginmessage(&buf, 'D');
2. pq_beginmessage(&buf, PgMsg_DataRow);

This first option is error-prone and makes finding all the places that
form PgMsg_DataRow messages difficult. Refactor the code to always use
the second option.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/access/common/printtup.c          | 5 +++--
 src/backend/commands/explain.c                | 3 ++-
 src/backend/replication/walsender.c           | 2 +-
 src/backend/tcop/postgres.c                   | 3 +--
 src/backend/utils/activity/backend_progress.c | 4 ++--
 src/include/libpq/protocol.h                  | 1 +
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index f2d5ca14fe..c78cc39308 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -17,6 +17,7 @@
 
 #include "access/printtup.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "tcop/pquery.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
@@ -170,7 +171,7 @@ SendRowDescriptionMessage(StringInfo buf, TupleDesc typeinfo,
 	ListCell   *tlist_item = list_head(targetlist);
 
 	/* tuple descriptor message type */
-	pq_beginmessage_reuse(buf, 'T');
+	pq_beginmessage_reuse(buf, PqMsg_RowDescription);
 	/* # of attrs in tuples */
 	pq_sendint16(buf, natts);
 
@@ -322,7 +323,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 	/*
 	 * Prepare a DataRow message (note buffer is in per-query context)
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 118db12903..5771aabf40 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,6 +21,7 @@
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -5497,7 +5498,7 @@ serializeAnalyzeReceive(TupleTableSlot *slot, DestReceiver *self)
 	 * Note that we fill a StringInfo buffer the same as printtup() does, so
 	 * as to capture the costs of manipulating the strings accurately.
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 2d1a9ec900..ca205594bd 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -695,7 +695,7 @@ UploadManifest(void)
 	ib = CreateIncrementalBackupInfo(mcxt);
 
 	/* Send a CopyInResponse message */
-	pq_beginmessage(&buf, 'G');
+	pq_beginmessage(&buf, PqMsg_CopyInResponse);
 	pq_sendbyte(&buf, 0);
 	pq_sendint16(&buf, 0);
 	pq_endmessage_reuse(&buf);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e39c6804a7..ea517f4b9b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2651,8 +2651,7 @@ exec_describe_statement_message(const char *stmt_name)
 	/*
 	 * First describe the parameters...
 	 */
-	pq_beginmessage_reuse(&row_description_buf, 't');	/* parameter description
-														 * message type */
+	pq_beginmessage_reuse(&row_description_buf, PqMsg_ParameterDescription);
 	pq_sendint16(&row_description_buf, psrc->num_params);
 
 	for (int i = 0; i < psrc->num_params; i++)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index bfb9b7704b..c78c5eb507 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -92,7 +92,7 @@ void
 pgstat_progress_parallel_incr_param(int index, int64 incr)
 {
 	/*
-	 * Parallel workers notify a leader through a 'P' protocol message to
+	 * Parallel workers notify a leader through a PqMsg_Progress message to
 	 * update progress, passing the progress index and incremented value.
 	 * Leaders can just call pgstat_progress_incr_param directly.
 	 */
@@ -102,7 +102,7 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
 
 		initStringInfo(&progress_message);
 
-		pq_beginmessage(&progress_message, 'P');
+		pq_beginmessage(&progress_message, PqMsg_Progress);
 		pq_sendint32(&progress_message, index);
 		pq_sendint64(&progress_message, incr);
 		pq_endmessage(&progress_message);
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 4b8d440365..8c0f095edf 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -47,6 +47,7 @@
 #define PqMsg_EmptyQueryResponse	'I'
 #define PqMsg_BackendKeyData		'K'
 #define PqMsg_NoticeResponse		'N'
+#define PqMsg_Progress              'P'
 #define PqMsg_AuthenticationRequest 'R'
 #define PqMsg_ParameterStatus		'S'
 #define PqMsg_RowDescription		'T'
-- 
2.45.2

v1-0002-Intoruce-PgMsg-enum.patchapplication/octet-stream; name=v1-0002-Intoruce-PgMsg-enum.patchDownload

From c43292f7b69b9dbaf2c5b4a5e329040a0aa98e1f Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:33:18 +0300
Subject: [PATCH v1 2/3] Intoruce PgMsg enum.

Previously we passed PgMsg_* as regular char's. There are two problems with
this approach:

1. Lack of type-safety. One can pass a wrong char value and the code would
   compile.
2. Some authors passed PgMsg_DataRow while others passed 'D' which made
   the code incosistent and difficult to search.

This patch metigates both problems by using enum instead of char's.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/access/common/printtup.c |  1 -
 src/backend/commands/explain.c       |  1 -
 src/backend/libpq/pqformat.c         |  8 +--
 src/include/libpq/pqformat.h         |  9 +--
 src/include/libpq/protocol.h         | 91 ++++++++++++++--------------
 5 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index c78cc39308..1e67aea3b1 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -17,7 +17,6 @@
 
 #include "access/printtup.h"
 #include "libpq/pqformat.h"
-#include "libpq/protocol.h"
 #include "tcop/pquery.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5771aabf40..4cd01bbbc3 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,7 +21,6 @@
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
 #include "libpq/pqformat.h"
-#include "libpq/protocol.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index aa9433bb3b..fca295b8f7 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -85,7 +85,7 @@
  * --------------------------------
  */
 void
-pq_beginmessage(StringInfo buf, char msgtype)
+pq_beginmessage(StringInfo buf, PgMsg msgtype)
 {
 	initStringInfo(buf);
 
@@ -106,7 +106,7 @@ pq_beginmessage(StringInfo buf, char msgtype)
  * --------------------------------
  */
 void
-pq_beginmessage_reuse(StringInfo buf, char msgtype)
+pq_beginmessage_reuse(StringInfo buf, PgMsg msgtype)
 {
 	resetStringInfo(buf);
 
@@ -364,7 +364,7 @@ pq_endtypsend(StringInfo buf)
  * --------------------------------
  */
 void
-pq_puttextmessage(char msgtype, const char *str)
+pq_puttextmessage(PgMsg msgtype, const char *str)
 {
 	int			slen = strlen(str);
 	char	   *p;
@@ -385,7 +385,7 @@ pq_puttextmessage(char msgtype, const char *str)
  * --------------------------------
  */
 void
-pq_putemptymessage(char msgtype)
+pq_putemptymessage(PgMsg msgtype)
 {
 	(void) pq_putmessage(msgtype, NULL, 0);
 }
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 7f48cbded9..dda004943c 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -13,12 +13,13 @@
 #ifndef PQFORMAT_H
 #define PQFORMAT_H
 
+#include "protocol.h"
 #include "lib/stringinfo.h"
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
 
-extern void pq_beginmessage(StringInfo buf, char msgtype);
-extern void pq_beginmessage_reuse(StringInfo buf, char msgtype);
+extern void pq_beginmessage(StringInfo buf, PgMsg msgtype);
+extern void pq_beginmessage_reuse(StringInfo buf, PgMsg msgtype);
 extern void pq_endmessage(StringInfo buf);
 extern void pq_endmessage_reuse(StringInfo buf);
 
@@ -191,8 +192,8 @@ pq_sendint(StringInfo buf, uint32 i, int b)
 extern void pq_begintypsend(StringInfo buf);
 extern bytea *pq_endtypsend(StringInfo buf);
 
-extern void pq_puttextmessage(char msgtype, const char *str);
-extern void pq_putemptymessage(char msgtype);
+extern void pq_puttextmessage(PgMsg msgtype, const char *str);
+extern void pq_putemptymessage(PgMsg msgtype);
 
 extern int	pq_getmsgbyte(StringInfo msg);
 extern unsigned int pq_getmsgint(StringInfo msg, int b);
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 8c0f095edf..f70fa768f7 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -14,57 +14,58 @@
 #ifndef PROTOCOL_H
 #define PROTOCOL_H
 
-/* These are the request codes sent by the frontend. */
+typedef enum PgMsg
+{
+	/* These are the request codes sent by the frontend. */
 
-#define PqMsg_Bind					'B'
-#define PqMsg_Close					'C'
-#define PqMsg_Describe				'D'
-#define PqMsg_Execute				'E'
-#define PqMsg_FunctionCall			'F'
-#define PqMsg_Flush					'H'
-#define PqMsg_Parse					'P'
-#define PqMsg_Query					'Q'
-#define PqMsg_Sync					'S'
-#define PqMsg_Terminate				'X'
-#define PqMsg_CopyFail				'f'
-#define PqMsg_GSSResponse			'p'
-#define PqMsg_PasswordMessage		'p'
-#define PqMsg_SASLInitialResponse	'p'
-#define PqMsg_SASLResponse			'p'
+	PqMsg_Bind = 'B',
+	PqMsg_Close = 'C',
+	PqMsg_Describe = 'D',
+	PqMsg_Execute = 'E',
+	PqMsg_FunctionCall = 'F',
+	PqMsg_Flush = 'H',
+	PqMsg_Parse = 'P',
+	PqMsg_Query = 'Q',
+	PqMsg_Sync = 'S',
+	PqMsg_Terminate = 'X',
+	PqMsg_CopyFail = 'f',
+	PqMsg_GSSResponse = 'p',
+	PqMsg_PasswordMessage = 'p',
+	PqMsg_SASLInitialResponse = 'p',
+	PqMsg_SASLResponse = 'p',
 
+	/* These are the response codes sent by the backend. */
 
-/* These are the response codes sent by the backend. */
+	PqMsg_ParseComplete = '1',
+	PqMsg_BindComplete = '2',
+	PqMsg_CloseComplete = '3',
+	PqMsg_NotificationResponse = 'A',
+	PqMsg_CommandComplete = 'C',
+	PqMsg_DataRow = 'D',
+	PqMsg_ErrorResponse = 'E',
+	PqMsg_CopyInResponse = 'G',
+	PqMsg_CopyOutResponse = 'H',
+	PqMsg_EmptyQueryResponse = 'I',
+	PqMsg_BackendKeyData = 'K',
+	PqMsg_NoticeResponse = 'N',
+	PqMsg_Progress = 'P',
+	PqMsg_AuthenticationRequest = 'R',
+	PqMsg_ParameterStatus = 'S',
+	PqMsg_RowDescription = 'T',
+	PqMsg_FunctionCallResponse = 'V',
+	PqMsg_CopyBothResponse = 'W',
+	PqMsg_ReadyForQuery = 'Z',
+	PqMsg_NoData = 'n',
+	PqMsg_PortalSuspended = 's',
+	PqMsg_ParameterDescription = 't',
+	PqMsg_NegotiateProtocolVersion = 'v',
 
-#define PqMsg_ParseComplete			'1'
-#define PqMsg_BindComplete			'2'
-#define PqMsg_CloseComplete			'3'
-#define PqMsg_NotificationResponse	'A'
-#define PqMsg_CommandComplete		'C'
-#define PqMsg_DataRow				'D'
-#define PqMsg_ErrorResponse			'E'
-#define PqMsg_CopyInResponse		'G'
-#define PqMsg_CopyOutResponse		'H'
-#define PqMsg_EmptyQueryResponse	'I'
-#define PqMsg_BackendKeyData		'K'
-#define PqMsg_NoticeResponse		'N'
-#define PqMsg_Progress              'P'
-#define PqMsg_AuthenticationRequest 'R'
-#define PqMsg_ParameterStatus		'S'
-#define PqMsg_RowDescription		'T'
-#define PqMsg_FunctionCallResponse	'V'
-#define PqMsg_CopyBothResponse		'W'
-#define PqMsg_ReadyForQuery			'Z'
-#define PqMsg_NoData				'n'
-#define PqMsg_PortalSuspended		's'
-#define PqMsg_ParameterDescription	't'
-#define PqMsg_NegotiateProtocolVersion 'v'
+	/* These are the codes sent by both the frontend and backend. */
 
+	PqMsg_CopyDone = 'c',
+	PqMsg_CopyData = 'd',
 
-/* These are the codes sent by both the frontend and backend. */
-
-#define PqMsg_CopyDone				'c'
-#define PqMsg_CopyData				'd'
-
+}			PgMsg;
 
 /* These are the authentication request codes sent by the backend. */
 
-- 
2.45.2

v1-0003-Rearrange-the-order-of-functions-in-pgformat.-c-h.patchapplication/octet-stream; name=v1-0003-Rearrange-the-order-of-functions-in-pgformat.-c-h.patchDownload

From bcf1842349fb476852c6d3e7cea41b7244dea6c0 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:55:54 +0300
Subject: [PATCH v1 3/3] Rearrange the order of functions in pgformat.{c,h}

Place functions that send protocol messages of given PgMsg type closer
to each other. This makes no difference for the compiler but makes the code
easier to read for humans.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/libpq/pqformat.c | 70 ++++++++++++++++++------------------
 src/include/libpq/pqformat.h |  6 ++--
 2 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index fca295b8f7..5f4562fc45 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -79,6 +79,40 @@
 #include "port/pg_bswap.h"
 #include "varatt.h"
 
+/* --------------------------------
+ *		pq_puttextmessage - generate a character set-converted message in one step
+ *
+ *		This is the same as the pqcomm.c routine pq_putmessage, except that
+ *		the message body is a null-terminated string to which encoding
+ *		conversion applies.
+ * --------------------------------
+ */
+void
+pq_puttextmessage(PgMsg msgtype, const char *str)
+{
+	int			slen = strlen(str);
+	char	   *p;
+
+	p = pg_server_to_client(str, slen);
+	if (p != str)				/* actual conversion has been done? */
+	{
+		(void) pq_putmessage(msgtype, p, strlen(p) + 1);
+		pfree(p);
+		return;
+	}
+	(void) pq_putmessage(msgtype, str, slen + 1);
+}
+
+
+/* --------------------------------
+ *		pq_putemptymessage - convenience routine for message with empty body
+ * --------------------------------
+ */
+void
+pq_putemptymessage(PgMsg msgtype)
+{
+	(void) pq_putmessage(msgtype, NULL, 0);
+}
 
 /* --------------------------------
  *		pq_beginmessage		- initialize for sending a message
@@ -355,42 +389,6 @@ pq_endtypsend(StringInfo buf)
 }
 
 
-/* --------------------------------
- *		pq_puttextmessage - generate a character set-converted message in one step
- *
- *		This is the same as the pqcomm.c routine pq_putmessage, except that
- *		the message body is a null-terminated string to which encoding
- *		conversion applies.
- * --------------------------------
- */
-void
-pq_puttextmessage(PgMsg msgtype, const char *str)
-{
-	int			slen = strlen(str);
-	char	   *p;
-
-	p = pg_server_to_client(str, slen);
-	if (p != str)				/* actual conversion has been done? */
-	{
-		(void) pq_putmessage(msgtype, p, strlen(p) + 1);
-		pfree(p);
-		return;
-	}
-	(void) pq_putmessage(msgtype, str, slen + 1);
-}
-
-
-/* --------------------------------
- *		pq_putemptymessage - convenience routine for message with empty body
- * --------------------------------
- */
-void
-pq_putemptymessage(PgMsg msgtype)
-{
-	(void) pq_putmessage(msgtype, NULL, 0);
-}
-
-
 /* --------------------------------
  *		pq_getmsgbyte	- get a raw byte from a message buffer
  * --------------------------------
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index dda004943c..b8f29cee2f 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -18,6 +18,9 @@
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
 
+extern void pq_puttextmessage(PgMsg msgtype, const char *str);
+extern void pq_putemptymessage(PgMsg msgtype);
+
 extern void pq_beginmessage(StringInfo buf, PgMsg msgtype);
 extern void pq_beginmessage_reuse(StringInfo buf, PgMsg msgtype);
 extern void pq_endmessage(StringInfo buf);
@@ -192,9 +195,6 @@ pq_sendint(StringInfo buf, uint32 i, int b)
 extern void pq_begintypsend(StringInfo buf);
 extern bytea *pq_endtypsend(StringInfo buf);
 
-extern void pq_puttextmessage(PgMsg msgtype, const char *str);
-extern void pq_putemptymessage(PgMsg msgtype);
-
 extern int	pq_getmsgbyte(StringInfo msg);
 extern unsigned int pq_getmsgint(StringInfo msg, int b);
 extern int64 pq_getmsgint64(StringInfo msg);
-- 
2.45.2

Aleksander Alekseev

aleksander@timescale.com

over 1 year ago

In reply to: Aleksander Alekseev (#1)

3 attachment(s)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

Hi,

The proposed patchset fixes this.

In v1 I mistakenly named the enum PgMsg while it should have been
PqMsg. Here is the corrected patchset.

--
Best regards,
Aleksander Alekseev

Attachments:

v2-0002-Introduce-PqMsg-enum.patchapplication/octet-stream; name=v2-0002-Introduce-PqMsg-enum.patchDownload

From 405c93009d7e9309b4ad63e1c40a555c9bc41f1c Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:33:18 +0300
Subject: [PATCH v2 2/3] Introduce PqMsg enum.

Previously we passed PqMsg_* as regular char's. There are two problems with
this approach:

1. Lack of type-safety. One can pass a wrong char value and the code would
   compile.
2. Some authors passed PqMsg_DataRow while others passed 'D' which made
   the code inconsistent and difficult to search.

This patch mitigates both problems by using enum instead of char's.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/access/common/printtup.c |  1 -
 src/backend/commands/explain.c       |  1 -
 src/backend/libpq/pqformat.c         |  8 +--
 src/include/libpq/pqformat.h         |  9 +--
 src/include/libpq/protocol.h         | 91 ++++++++++++++--------------
 5 files changed, 55 insertions(+), 55 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index c78cc39308..1e67aea3b1 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -17,7 +17,6 @@
 
 #include "access/printtup.h"
 #include "libpq/pqformat.h"
-#include "libpq/protocol.h"
 #include "tcop/pquery.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5771aabf40..4cd01bbbc3 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,7 +21,6 @@
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
 #include "libpq/pqformat.h"
-#include "libpq/protocol.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index aa9433bb3b..e908144f44 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -85,7 +85,7 @@
  * --------------------------------
  */
 void
-pq_beginmessage(StringInfo buf, char msgtype)
+pq_beginmessage(StringInfo buf, PqMsg msgtype)
 {
 	initStringInfo(buf);
 
@@ -106,7 +106,7 @@ pq_beginmessage(StringInfo buf, char msgtype)
  * --------------------------------
  */
 void
-pq_beginmessage_reuse(StringInfo buf, char msgtype)
+pq_beginmessage_reuse(StringInfo buf, PqMsg msgtype)
 {
 	resetStringInfo(buf);
 
@@ -364,7 +364,7 @@ pq_endtypsend(StringInfo buf)
  * --------------------------------
  */
 void
-pq_puttextmessage(char msgtype, const char *str)
+pq_puttextmessage(PqMsg msgtype, const char *str)
 {
 	int			slen = strlen(str);
 	char	   *p;
@@ -385,7 +385,7 @@ pq_puttextmessage(char msgtype, const char *str)
  * --------------------------------
  */
 void
-pq_putemptymessage(char msgtype)
+pq_putemptymessage(PqMsg msgtype)
 {
 	(void) pq_putmessage(msgtype, NULL, 0);
 }
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 7f48cbded9..f9e785e6a5 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -13,12 +13,13 @@
 #ifndef PQFORMAT_H
 #define PQFORMAT_H
 
+#include "protocol.h"
 #include "lib/stringinfo.h"
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
 
-extern void pq_beginmessage(StringInfo buf, char msgtype);
-extern void pq_beginmessage_reuse(StringInfo buf, char msgtype);
+extern void pq_beginmessage(StringInfo buf, PqMsg msgtype);
+extern void pq_beginmessage_reuse(StringInfo buf, PqMsg msgtype);
 extern void pq_endmessage(StringInfo buf);
 extern void pq_endmessage_reuse(StringInfo buf);
 
@@ -191,8 +192,8 @@ pq_sendint(StringInfo buf, uint32 i, int b)
 extern void pq_begintypsend(StringInfo buf);
 extern bytea *pq_endtypsend(StringInfo buf);
 
-extern void pq_puttextmessage(char msgtype, const char *str);
-extern void pq_putemptymessage(char msgtype);
+extern void pq_puttextmessage(PqMsg msgtype, const char *str);
+extern void pq_putemptymessage(PqMsg msgtype);
 
 extern int	pq_getmsgbyte(StringInfo msg);
 extern unsigned int pq_getmsgint(StringInfo msg, int b);
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 8c0f095edf..840a049d04 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -14,57 +14,58 @@
 #ifndef PROTOCOL_H
 #define PROTOCOL_H
 
-/* These are the request codes sent by the frontend. */
+typedef enum PqMsg
+{
+	/* These are the request codes sent by the frontend. */
 
-#define PqMsg_Bind					'B'
-#define PqMsg_Close					'C'
-#define PqMsg_Describe				'D'
-#define PqMsg_Execute				'E'
-#define PqMsg_FunctionCall			'F'
-#define PqMsg_Flush					'H'
-#define PqMsg_Parse					'P'
-#define PqMsg_Query					'Q'
-#define PqMsg_Sync					'S'
-#define PqMsg_Terminate				'X'
-#define PqMsg_CopyFail				'f'
-#define PqMsg_GSSResponse			'p'
-#define PqMsg_PasswordMessage		'p'
-#define PqMsg_SASLInitialResponse	'p'
-#define PqMsg_SASLResponse			'p'
+	PqMsg_Bind = 'B',
+	PqMsg_Close = 'C',
+	PqMsg_Describe = 'D',
+	PqMsg_Execute = 'E',
+	PqMsg_FunctionCall = 'F',
+	PqMsg_Flush = 'H',
+	PqMsg_Parse = 'P',
+	PqMsg_Query = 'Q',
+	PqMsg_Sync = 'S',
+	PqMsg_Terminate = 'X',
+	PqMsg_CopyFail = 'f',
+	PqMsg_GSSResponse = 'p',
+	PqMsg_PasswordMessage = 'p',
+	PqMsg_SASLInitialResponse = 'p',
+	PqMsg_SASLResponse = 'p',
 
+	/* These are the response codes sent by the backend. */
 
-/* These are the response codes sent by the backend. */
+	PqMsg_ParseComplete = '1',
+	PqMsg_BindComplete = '2',
+	PqMsg_CloseComplete = '3',
+	PqMsg_NotificationResponse = 'A',
+	PqMsg_CommandComplete = 'C',
+	PqMsg_DataRow = 'D',
+	PqMsg_ErrorResponse = 'E',
+	PqMsg_CopyInResponse = 'G',
+	PqMsg_CopyOutResponse = 'H',
+	PqMsg_EmptyQueryResponse = 'I',
+	PqMsg_BackendKeyData = 'K',
+	PqMsg_NoticeResponse = 'N',
+	PqMsg_Progress = 'P',
+	PqMsg_AuthenticationRequest = 'R',
+	PqMsg_ParameterStatus = 'S',
+	PqMsg_RowDescription = 'T',
+	PqMsg_FunctionCallResponse = 'V',
+	PqMsg_CopyBothResponse = 'W',
+	PqMsg_ReadyForQuery = 'Z',
+	PqMsg_NoData = 'n',
+	PqMsg_PortalSuspended = 's',
+	PqMsg_ParameterDescription = 't',
+	PqMsg_NegotiateProtocolVersion = 'v',
 
-#define PqMsg_ParseComplete			'1'
-#define PqMsg_BindComplete			'2'
-#define PqMsg_CloseComplete			'3'
-#define PqMsg_NotificationResponse	'A'
-#define PqMsg_CommandComplete		'C'
-#define PqMsg_DataRow				'D'
-#define PqMsg_ErrorResponse			'E'
-#define PqMsg_CopyInResponse		'G'
-#define PqMsg_CopyOutResponse		'H'
-#define PqMsg_EmptyQueryResponse	'I'
-#define PqMsg_BackendKeyData		'K'
-#define PqMsg_NoticeResponse		'N'
-#define PqMsg_Progress              'P'
-#define PqMsg_AuthenticationRequest 'R'
-#define PqMsg_ParameterStatus		'S'
-#define PqMsg_RowDescription		'T'
-#define PqMsg_FunctionCallResponse	'V'
-#define PqMsg_CopyBothResponse		'W'
-#define PqMsg_ReadyForQuery			'Z'
-#define PqMsg_NoData				'n'
-#define PqMsg_PortalSuspended		's'
-#define PqMsg_ParameterDescription	't'
-#define PqMsg_NegotiateProtocolVersion 'v'
+	/* These are the codes sent by both the frontend and backend. */
 
+	PqMsg_CopyDone = 'c',
+	PqMsg_CopyData = 'd',
 
-/* These are the codes sent by both the frontend and backend. */
-
-#define PqMsg_CopyDone				'c'
-#define PqMsg_CopyData				'd'
-
+}			PqMsg;
 
 /* These are the authentication request codes sent by the backend. */
 
-- 
2.45.2

v2-0001-Always-pass-PqMsg_-to-pq_beginmessage-_reuse.patchapplication/octet-stream; name=v2-0001-Always-pass-PqMsg_-to-pq_beginmessage-_reuse.patchDownload

From 5bf14eaf0cff9035137b87bb1d8b6d1dde082000 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:07:16 +0300
Subject: [PATCH v2 1/3] Always pass PqMsg_* to pq_beginmessage[_reuse]

Before this patch the code could be written in one of the following ways:

1. pq_beginmessage(&buf, 'D');
2. pq_beginmessage(&buf, PqMsg_DataRow);

This first option is error-prone and makes finding all the places that
form PqMsg_DataRow messages difficult. Refactor the code to always use
the second option.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/access/common/printtup.c          | 5 +++--
 src/backend/commands/explain.c                | 3 ++-
 src/backend/replication/walsender.c           | 2 +-
 src/backend/tcop/postgres.c                   | 3 +--
 src/backend/utils/activity/backend_progress.c | 4 ++--
 src/include/libpq/protocol.h                  | 1 +
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index f2d5ca14fe..c78cc39308 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -17,6 +17,7 @@
 
 #include "access/printtup.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "tcop/pquery.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
@@ -170,7 +171,7 @@ SendRowDescriptionMessage(StringInfo buf, TupleDesc typeinfo,
 	ListCell   *tlist_item = list_head(targetlist);
 
 	/* tuple descriptor message type */
-	pq_beginmessage_reuse(buf, 'T');
+	pq_beginmessage_reuse(buf, PqMsg_RowDescription);
 	/* # of attrs in tuples */
 	pq_sendint16(buf, natts);
 
@@ -322,7 +323,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 	/*
 	 * Prepare a DataRow message (note buffer is in per-query context)
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 118db12903..5771aabf40 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,6 +21,7 @@
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -5497,7 +5498,7 @@ serializeAnalyzeReceive(TupleTableSlot *slot, DestReceiver *self)
 	 * Note that we fill a StringInfo buffer the same as printtup() does, so
 	 * as to capture the costs of manipulating the strings accurately.
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 2d1a9ec900..ca205594bd 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -695,7 +695,7 @@ UploadManifest(void)
 	ib = CreateIncrementalBackupInfo(mcxt);
 
 	/* Send a CopyInResponse message */
-	pq_beginmessage(&buf, 'G');
+	pq_beginmessage(&buf, PqMsg_CopyInResponse);
 	pq_sendbyte(&buf, 0);
 	pq_sendint16(&buf, 0);
 	pq_endmessage_reuse(&buf);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e39c6804a7..ea517f4b9b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2651,8 +2651,7 @@ exec_describe_statement_message(const char *stmt_name)
 	/*
 	 * First describe the parameters...
 	 */
-	pq_beginmessage_reuse(&row_description_buf, 't');	/* parameter description
-														 * message type */
+	pq_beginmessage_reuse(&row_description_buf, PqMsg_ParameterDescription);
 	pq_sendint16(&row_description_buf, psrc->num_params);
 
 	for (int i = 0; i < psrc->num_params; i++)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index bfb9b7704b..c78c5eb507 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -92,7 +92,7 @@ void
 pgstat_progress_parallel_incr_param(int index, int64 incr)
 {
 	/*
-	 * Parallel workers notify a leader through a 'P' protocol message to
+	 * Parallel workers notify a leader through a PqMsg_Progress message to
 	 * update progress, passing the progress index and incremented value.
 	 * Leaders can just call pgstat_progress_incr_param directly.
 	 */
@@ -102,7 +102,7 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
 
 		initStringInfo(&progress_message);
 
-		pq_beginmessage(&progress_message, 'P');
+		pq_beginmessage(&progress_message, PqMsg_Progress);
 		pq_sendint32(&progress_message, index);
 		pq_sendint64(&progress_message, incr);
 		pq_endmessage(&progress_message);
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 4b8d440365..8c0f095edf 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -47,6 +47,7 @@
 #define PqMsg_EmptyQueryResponse	'I'
 #define PqMsg_BackendKeyData		'K'
 #define PqMsg_NoticeResponse		'N'
+#define PqMsg_Progress              'P'
 #define PqMsg_AuthenticationRequest 'R'
 #define PqMsg_ParameterStatus		'S'
 #define PqMsg_RowDescription		'T'
-- 
2.45.2

v2-0003-Rearrange-the-order-of-functions-in-pqformat.-c-h.patchapplication/octet-stream; name=v2-0003-Rearrange-the-order-of-functions-in-pqformat.-c-h.patchDownload

From fc2c4ca33bdeb78f4fd6d1d7710d0ccd671e69b8 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 16:52:56 +0300
Subject: [PATCH v2 3/3] Rearrange the order of functions in pqformat.{c,h}

Place functions that send protocol messages of given PqMsg type closer
to each other. This makes no difference for the compiler but makes the code
easier to read for humans.

Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
 src/backend/libpq/pqformat.c | 76 +++++++++++++++++-------------------
 src/include/libpq/pqformat.h |  6 +--
 2 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index e908144f44..9432749587 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -31,6 +31,8 @@
 /*
  * INTERFACE ROUTINES
  * Message assembly and output:
+ *		pq_puttextmessage - generate a character set-converted message in one step
+ *		pq_putemptymessage - convenience routine for message with empty body
  *		pq_beginmessage - initialize StringInfo buffer
  *		pq_sendbyte		- append a raw byte to a StringInfo buffer
  *		pq_sendint		- append a binary integer to a StringInfo buffer
@@ -51,10 +53,6 @@
  *		pq_begintypsend - initialize StringInfo buffer
  *		pq_endtypsend	- return the completed string as a "bytea*"
  *
- * Special-case message output:
- *		pq_puttextmessage - generate a character set-converted message in one step
- *		pq_putemptymessage - convenience routine for message with empty body
- *
  * Message parsing after input:
  *		pq_getmsgbyte	- get a raw byte from a message buffer
  *		pq_getmsgint	- get a binary integer from a message buffer
@@ -79,6 +77,40 @@
 #include "port/pg_bswap.h"
 #include "varatt.h"
 
+/* --------------------------------
+ *		pq_puttextmessage - generate a character set-converted message in one step
+ *
+ *		This is the same as the pqcomm.c routine pq_putmessage, except that
+ *		the message body is a null-terminated string to which encoding
+ *		conversion applies.
+ * --------------------------------
+ */
+void
+pq_puttextmessage(PqMsg msgtype, const char *str)
+{
+	int			slen = strlen(str);
+	char	   *p;
+
+	p = pg_server_to_client(str, slen);
+	if (p != str)				/* actual conversion has been done? */
+	{
+		(void) pq_putmessage(msgtype, p, strlen(p) + 1);
+		pfree(p);
+		return;
+	}
+	(void) pq_putmessage(msgtype, str, slen + 1);
+}
+
+
+/* --------------------------------
+ *		pq_putemptymessage - convenience routine for message with empty body
+ * --------------------------------
+ */
+void
+pq_putemptymessage(PqMsg msgtype)
+{
+	(void) pq_putmessage(msgtype, NULL, 0);
+}
 
 /* --------------------------------
  *		pq_beginmessage		- initialize for sending a message
@@ -355,42 +387,6 @@ pq_endtypsend(StringInfo buf)
 }
 
 
-/* --------------------------------
- *		pq_puttextmessage - generate a character set-converted message in one step
- *
- *		This is the same as the pqcomm.c routine pq_putmessage, except that
- *		the message body is a null-terminated string to which encoding
- *		conversion applies.
- * --------------------------------
- */
-void
-pq_puttextmessage(PqMsg msgtype, const char *str)
-{
-	int			slen = strlen(str);
-	char	   *p;
-
-	p = pg_server_to_client(str, slen);
-	if (p != str)				/* actual conversion has been done? */
-	{
-		(void) pq_putmessage(msgtype, p, strlen(p) + 1);
-		pfree(p);
-		return;
-	}
-	(void) pq_putmessage(msgtype, str, slen + 1);
-}
-
-
-/* --------------------------------
- *		pq_putemptymessage - convenience routine for message with empty body
- * --------------------------------
- */
-void
-pq_putemptymessage(PqMsg msgtype)
-{
-	(void) pq_putmessage(msgtype, NULL, 0);
-}
-
-
 /* --------------------------------
  *		pq_getmsgbyte	- get a raw byte from a message buffer
  * --------------------------------
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index f9e785e6a5..00ef87acb2 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -18,6 +18,8 @@
 #include "mb/pg_wchar.h"
 #include "port/pg_bswap.h"
 
+extern void pq_puttextmessage(PqMsg msgtype, const char *str);
+extern void pq_putemptymessage(PqMsg msgtype);
 extern void pq_beginmessage(StringInfo buf, PqMsg msgtype);
 extern void pq_beginmessage_reuse(StringInfo buf, PqMsg msgtype);
 extern void pq_endmessage(StringInfo buf);
@@ -191,10 +193,6 @@ pq_sendint(StringInfo buf, uint32 i, int b)
 
 extern void pq_begintypsend(StringInfo buf);
 extern bytea *pq_endtypsend(StringInfo buf);
-
-extern void pq_puttextmessage(PqMsg msgtype, const char *str);
-extern void pq_putemptymessage(PqMsg msgtype);
-
 extern int	pq_getmsgbyte(StringInfo msg);
 extern unsigned int pq_getmsgint(StringInfo msg, int b);
 extern int64 pq_getmsgint64(StringInfo msg);
-- 
2.45.2

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Aleksander Alekseev (#1)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

On Tue, Jul 16, 2024 at 04:09:44PM +0300, Aleksander Alekseev wrote:

- Patch 1 replaces all the char's with PqMsg's

Thanks. I'll look into committing this one in the near future.

- Patch 2 makes PqMsg an enum. This ensures that the problem will not
appear again in the future and also gives us a bit more type-safety.

This was briefly brought up in the discussion that ultimately led to
protocol.h [0]/messages/by-id/20230807091044.jjgsl2rgheazaung@alvherre.pgsql. I don't have a particularly strong opinion on the matter,
but I will note that protocol.h was intended to be easily usable in
third-party code, and changing it from characters to an enum from v17 to
v18 might cause some extra code churn.

[0]: /messages/by-id/20230807091044.jjgsl2rgheazaung@alvherre.pgsql

--
nathan

Tom Lane

tgl@sss.pgh.pa.us

over 1 year ago

In reply to: Nathan Bossart (#3)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

Nathan Bossart <nathandbossart@gmail.com> writes:

This was briefly brought up in the discussion that ultimately led to
protocol.h [0]. I don't have a particularly strong opinion on the matter,
but I will note that protocol.h was intended to be easily usable in
third-party code, and changing it from characters to an enum from v17 to
v18 might cause some extra code churn.

We could avoid that issue by back-patching into 17; I don't think
it's quite too late for that, and the header is new as of 17.

However, I'm generally -1 on the proposal independently of that,
because I think it is the wrong thing from an ABI standpoint.
The message type codes in the protocol are chars, full stop.
If we declare the data type as an enum, we have basically zero
control over how wide the compiler will choose to make that ---
although it's pretty likely that it *won't* choose char width.
So you could not represent the message layout accurately with
an enum. Perhaps nobody is doing that, but it seems to me like
a foot-gun of roughly the same caliber as not using an enum.
Also, going in this direction would require adding casts between
char and enum in assorted places, which might be error-prone or
warning-inducing.

So on the whole, "leave it alone" seems like the right answer.

(This applies only to the s/char/enum/ proposal; I've not read
the patchset further than that.)

regards, tom lane

Aleksander Alekseev

aleksander@timescale.com

over 1 year ago

In reply to: Tom Lane (#4)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

Hi,

This was briefly brought up in the discussion that ultimately led to
protocol.h [0]. I don't have a particularly strong opinion on the matter,
but I will note that protocol.h was intended to be easily usable in
third-party code, and changing it from characters to an enum from v17 to
v18 might cause some extra code churn.

We could avoid that issue by back-patching into 17; I don't think
it's quite too late for that, and the header is new as of 17.

However, I'm generally -1 on the proposal independently of that,

[...]

(This applies only to the s/char/enum/ proposal; I've not read
the patchset further than that.)

That's fair enough. Also it's not clear how much type safety enums
give us exactly. E.g. after applying 0002 pq_putmessage() a.k.a.
PQcommMethods->putmessage() silently casts its first argument from
`enum PqMsg` to `char` (shrug).

--
Best regards,
Aleksander Alekseev

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Aleksander Alekseev (#5)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

I took a closer look at 0001.

diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 4b8d440365..8c0f095edf 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -47,6 +47,7 @@
 #define PqMsg_EmptyQueryResponse	'I'
 #define PqMsg_BackendKeyData		'K'
 #define PqMsg_NoticeResponse		'N'
+#define PqMsg_Progress              'P'
 #define PqMsg_AuthenticationRequest 'R'
 #define PqMsg_ParameterStatus		'S'
 #define PqMsg_RowDescription		'T'

As discussed elsewhere [0]/messages/by-id/20230829161555.GB2136737@nathanxps13, we can add the leader/worker protocol
characters to protocol.h, but they should probably go in a separate
section. I'd recommend breaking that part out to a separate patch, too.

[0]: /messages/by-id/20230829161555.GB2136737@nathanxps13

--
nathan

Aleksander Alekseev

aleksander@timescale.com

over 1 year ago

In reply to: Nathan Bossart (#6)

2 attachment(s)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

Hi,

As discussed elsewhere [0], we can add the leader/worker protocol
characters to protocol.h, but they should probably go in a separate
section. I'd recommend breaking that part out to a separate patch, too.

OK, here is the updated patchset. This time I chose not to include this patch:

- Patch 3 rearranges the order of the functions in pqformat.{c,h} a
bit to make the code easier to read.

... since arguably there is not much value in it. Please let me know
if you think it's actually needed.

--
Best regards,
Aleksander Alekseev

Attachments:

v3-0001-Add-PqMsg_Progress-definition.patchapplication/octet-stream; name=v3-0001-Add-PqMsg_Progress-definition.patchDownload

From c0c3f8b86966079659d0085cfe58a4b6b6bdc124 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 20:50:36 +0300
Subject: [PATCH v3 1/2] Add PqMsg_Progress definition

Introduce a macro definition for 'P' message tag used in leader/worker
protocol.

Aleksander Alekseev, reviewed by Nathan Bossart.
Discussion: https://postgr.es/m/CAJ7c6TNTd09AZq8tGaHS3LDyH_CCnpv0oOz2wN1dGe8zekxrdQ@mail.gmail.com
---
 src/backend/utils/activity/backend_progress.c | 4 ++--
 src/include/libpq/protocol.h                  | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index bfb9b7704b..c78c5eb507 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -92,7 +92,7 @@ void
 pgstat_progress_parallel_incr_param(int index, int64 incr)
 {
 	/*
-	 * Parallel workers notify a leader through a 'P' protocol message to
+	 * Parallel workers notify a leader through a PqMsg_Progress message to
 	 * update progress, passing the progress index and incremented value.
 	 * Leaders can just call pgstat_progress_incr_param directly.
 	 */
@@ -102,7 +102,7 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
 
 		initStringInfo(&progress_message);
 
-		pq_beginmessage(&progress_message, 'P');
+		pq_beginmessage(&progress_message, PqMsg_Progress);
 		pq_sendint32(&progress_message, index);
 		pq_sendint64(&progress_message, incr);
 		pq_endmessage(&progress_message);
diff --git a/src/include/libpq/protocol.h b/src/include/libpq/protocol.h
index 4b8d440365..554e6921d4 100644
--- a/src/include/libpq/protocol.h
+++ b/src/include/libpq/protocol.h
@@ -64,6 +64,8 @@
 #define PqMsg_CopyDone				'c'
 #define PqMsg_CopyData				'd'
 
+/* These are the codes used by parallel leader/worker protocol. */
+#define PqMsg_Progress              'P'
 
 /* These are the authentication request codes sent by the backend. */
 
-- 
2.45.2

v3-0002-Always-pass-PqMsg_-to-pq_beginmessage-_reuse.patchapplication/octet-stream; name=v3-0002-Always-pass-PqMsg_-to-pq_beginmessage-_reuse.patchDownload

From 80fd0d249025ae786b7d5ce1e5f445a1f3f39bdf Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Jul 2024 15:07:16 +0300
Subject: [PATCH v3 2/2] Always pass PqMsg_* to pq_beginmessage[_reuse]

Before this patch the code could be written in one of the following ways:

1. pq_beginmessage(&buf, 'D');
2. pq_beginmessage(&buf, PqMsg_DataRow);

This first option is error-prone and makes finding all the places that
form PqMsg_DataRow messages difficult. Refactor the code to always use
the second option.

Aleksander Alekseev, reviewed by Nathan Bossart
Discussion: https://postgr.es/m/CAJ7c6TNTd09AZq8tGaHS3LDyH_CCnpv0oOz2wN1dGe8zekxrdQ@mail.gmail.com
---
 src/backend/access/common/printtup.c | 5 +++--
 src/backend/commands/explain.c       | 3 ++-
 src/backend/replication/walsender.c  | 2 +-
 src/backend/tcop/postgres.c          | 3 +--
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index f2d5ca14fe..c78cc39308 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -17,6 +17,7 @@
 
 #include "access/printtup.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "tcop/pquery.h"
 #include "utils/lsyscache.h"
 #include "utils/memdebug.h"
@@ -170,7 +171,7 @@ SendRowDescriptionMessage(StringInfo buf, TupleDesc typeinfo,
 	ListCell   *tlist_item = list_head(targetlist);
 
 	/* tuple descriptor message type */
-	pq_beginmessage_reuse(buf, 'T');
+	pq_beginmessage_reuse(buf, PqMsg_RowDescription);
 	/* # of attrs in tuples */
 	pq_sendint16(buf, natts);
 
@@ -322,7 +323,7 @@ printtup(TupleTableSlot *slot, DestReceiver *self)
 	/*
 	 * Prepare a DataRow message (note buffer is in per-query context)
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 118db12903..5771aabf40 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -21,6 +21,7 @@
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
 #include "libpq/pqformat.h"
+#include "libpq/protocol.h"
 #include "nodes/extensible.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
@@ -5497,7 +5498,7 @@ serializeAnalyzeReceive(TupleTableSlot *slot, DestReceiver *self)
 	 * Note that we fill a StringInfo buffer the same as printtup() does, so
 	 * as to capture the costs of manipulating the strings accurately.
 	 */
-	pq_beginmessage_reuse(buf, 'D');
+	pq_beginmessage_reuse(buf, PqMsg_DataRow);
 
 	pq_sendint16(buf, natts);
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 2d1a9ec900..ca205594bd 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -695,7 +695,7 @@ UploadManifest(void)
 	ib = CreateIncrementalBackupInfo(mcxt);
 
 	/* Send a CopyInResponse message */
-	pq_beginmessage(&buf, 'G');
+	pq_beginmessage(&buf, PqMsg_CopyInResponse);
 	pq_sendbyte(&buf, 0);
 	pq_sendint16(&buf, 0);
 	pq_endmessage_reuse(&buf);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e39c6804a7..ea517f4b9b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2651,8 +2651,7 @@ exec_describe_statement_message(const char *stmt_name)
 	/*
 	 * First describe the parameters...
 	 */
-	pq_beginmessage_reuse(&row_description_buf, 't');	/* parameter description
-														 * message type */
+	pq_beginmessage_reuse(&row_description_buf, PqMsg_ParameterDescription);
 	pq_sendint16(&row_description_buf, psrc->num_params);
 
 	for (int i = 0; i < psrc->num_params; i++)
-- 
2.45.2

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Aleksander Alekseev (#7)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

On Tue, Jul 16, 2024 at 09:14:35PM +0300, Aleksander Alekseev wrote:

As discussed elsewhere [0], we can add the leader/worker protocol
characters to protocol.h, but they should probably go in a separate
section. I'd recommend breaking that part out to a separate patch, too.

OK, here is the updated patchset. This time I chose not to include this patch:

Thanks. The only thing that stands out to me is the name of the parallel
leader/worker protocol message. In the original thread for protocol
characters, some early versions of the patch called it a "parallel
progress" message, but this new one just calls it PqMsg_Progress. I guess
PqMsg_ParallelProgress might be a tad more descriptive and less likely to
cause naming collisions with new frontend/backend messages, but I'm not
tremendously worried about either of those things. Thoughts?

--
nathan

Aleksander Alekseev

aleksander@timescale.com

over 1 year ago

In reply to: Nathan Bossart (#8)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

Hi,

Thanks. The only thing that stands out to me is the name of the parallel
leader/worker protocol message. In the original thread for protocol
characters, some early versions of the patch called it a "parallel
progress" message, but this new one just calls it PqMsg_Progress. I guess
PqMsg_ParallelProgress might be a tad more descriptive and less likely to
cause naming collisions with new frontend/backend messages, but I'm not
tremendously worried about either of those things. Thoughts?

Personally I'm fine with either option.

--
Best regards,
Aleksander Alekseev

#10

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Aleksander Alekseev (#9)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

On Tue, Jul 16, 2024 at 10:58:37PM +0300, Aleksander Alekseev wrote:

Thanks. The only thing that stands out to me is the name of the parallel
leader/worker protocol message. In the original thread for protocol
characters, some early versions of the patch called it a "parallel
progress" message, but this new one just calls it PqMsg_Progress. I guess
PqMsg_ParallelProgress might be a tad more descriptive and less likely to
cause naming collisions with new frontend/backend messages, but I'm not
tremendously worried about either of those things. Thoughts?

Personally I'm fine with either option.

Alright. Well, I guess I'll flip a coin tomorrow unless someone else
chimes in with an opinion.

--
nathan

#11

Nathan Bossart

nathandbossart@gmail.com

over 1 year ago

In reply to: Nathan Bossart (#10)

Re: [PATCH] Refactor pqformat.{c,h} and protocol.h

On Tue, Jul 16, 2024 at 04:38:06PM -0500, Nathan Bossart wrote:

Alright. Well, I guess I'll flip a coin tomorrow unless someone else
chimes in with an opinion.

Committed and back-patched to v17. I left it as PqMsg_Progress.

--
nathan