More protocol.h replacements this time into walsender.c

On Tue, Jul 22, 2025 at 05:54:48PM -0400, Dave Cramer wrote:

Patch attached

Thanks. I plan to look into committing this tomorrow.

--
nathan

Committed. I noticed that there are several characters with no match in
protocol.h. It might be worth adding those.

In walsender.c:

1537: pq_sendbyte(ctx->out, 'w');
2353: case 'r':
2357: case 'h':
2361: case 'p':
2755: pq_sendbyte(&output_message, 's');
3367: pq_sendbyte(&output_message, 'w');
4138: pq_sendbyte(&output_message, 'k');

In walreceiver.c:

829: case 'w': /* WAL records */
853: case 'k': /* Keepalive */
1133: pq_sendbyte(&reply_message, 'r');
1237: pq_sendbyte(&reply_message, 'h');

In logical/worker.c:

3854: if (c == 'w')
3876: else if (c == 'k')
3895: else if (c == 's') /* Primary status update */
4127: pq_sendbyte(reply_message, 'r');
4298: pq_sendbyte(request_message, 'p');

--
nathan

On Wed, 23 Jul 2025 at 11:40, Nathan Bossart <nathandbossart@gmail.com>
wrote:

Committed. I noticed that there are several characters with no match in
protocol.h. It might be worth adding those.

In walsender.c:

1537: pq_sendbyte(ctx->out, 'w');
2353: case 'r':
2357: case 'h':
2361: case 'p':
2755: pq_sendbyte(&output_message, 's');
3367: pq_sendbyte(&output_message, 'w');
4138: pq_sendbyte(&output_message, 'k');

In walreceiver.c:

829: case 'w': /* WAL
records */
853: case 'k': /*
Keepalive */
1133: pq_sendbyte(&reply_message, 'r');
1237: pq_sendbyte(&reply_message, 'h');

In logical/worker.c:

3854: if (c == 'w')
3876: else if (c == 'k')
3895: else if (c == 's') /* Primary
status update */
4127: pq_sendbyte(reply_message, 'r');
4298: pq_sendbyte(request_message, 'p');

Interesting, yes I will add those

Thanks!
Dave

On Thu, 24 Jul 2025 at 05:34, Dave Cramer <davecramer@gmail.com> wrote:

On Wed, 23 Jul 2025 at 11:40, Nathan Bossart <nathandbossart@gmail.com>
wrote:

Committed. I noticed that there are several characters with no match in
protocol.h. It might be worth adding those.

In walsender.c:

1537: pq_sendbyte(ctx->out, 'w');
2353: case 'r':
2357: case 'h':
2361: case 'p':
2755: pq_sendbyte(&output_message, 's');
3367: pq_sendbyte(&output_message, 'w');
4138: pq_sendbyte(&output_message, 'k');

In walreceiver.c:

829: case 'w': /* WAL
records */
853: case 'k': /*
Keepalive */
1133: pq_sendbyte(&reply_message, 'r');
1237: pq_sendbyte(&reply_message, 'h');

In logical/worker.c:

3854: if (c == 'w')
3876: else if (c == 'k')
3895: else if (c == 's') /*
Primary status update */
4127: pq_sendbyte(reply_message, 'r');
4298: pq_sendbyte(request_message, 'p');

Interesting, yes I will add those

Patch attached

Dave Cramer

Hello,

Since this is a whole new symbol, I'd rather you use the term WAL rather than Xlog ...

--
Álvaro Herrera

On Thu, Jul 24, 2025 at 12:04 PM Dave Cramer <davecramer@gmail.com> wrote:

Patch attached

+/* Replication Protocol sent by the primary */
+
+#define PqMsg_XlogData              'w'
+#define PqMsg_PrimaryKeepAlive      'k'
+#define PqMsg_PrimaryStatusUpdate   's'
+
+
+/* Replication Protocol sent by the standby */
+
+#define PqMsg_StandbyStatus         'r'
+#define PqMsg_HotStandbyFeedback    'h'
+#define PqMsg_RequestPrimaryStatus  'p'

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

+/* These are the codes sent by the frontend and backend. */
+
+#define PqMsg_PasswordMessage 'p'
+
+/* These are the codes sent by the frontend and backend. */

Is this change intended?

--Jacob

On Thu, 24 Jul 2025 at 17:05, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

On Thu, Jul 24, 2025 at 12:04 PM Dave Cramer <davecramer@gmail.com> wrote:

Patch attached

+/* Replication Protocol sent by the primary */
+
+#define PqMsg_XlogData              'w'
+#define PqMsg_PrimaryKeepAlive      'k'
+#define PqMsg_PrimaryStatusUpdate   's'
+
+
+/* Replication Protocol sent by the standby */
+
+#define PqMsg_StandbyStatus         'r'
+#define PqMsg_HotStandbyFeedback    'h'
+#define PqMsg_RequestPrimaryStatus  'p'

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

I'm going to wait to see if there are any other opinions. Last time I did
this there were quite a few opinions before finally settling on the naming

+/* These are the codes sent by the frontend and backend. */
+
+#define PqMsg_PasswordMessage 'p'
+
+/* These are the codes sent by the frontend and backend. */

Is this change intended?

It was as it lines up with the others at least in my editor.
I'm not married to it.

Dave

On Thu, 24 Jul 2025 at 16:49, Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

Since this is a whole new symbol, I'd rather you use the term WAL rather
than Xlog ...

Fair enough

Dave

On 2025-Jul-24, Dave Cramer wrote:

On Thu, 24 Jul 2025 at 17:05, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

On Thu, Jul 24, 2025 at 12:04 PM Dave Cramer <davecramer@gmail.com> wrote:

+/* Replication Protocol sent by the primary */
+
+#define PqMsg_XlogData              'w'
+#define PqMsg_PrimaryKeepAlive      'k'
+#define PqMsg_PrimaryStatusUpdate   's'
+
+
+/* Replication Protocol sent by the standby */
+
+#define PqMsg_StandbyStatus         'r'
+#define PqMsg_HotStandbyFeedback    'h'
+#define PqMsg_RequestPrimaryStatus  'p'

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

I'm going to wait to see if there are any other opinions. Last time I did
this there were quite a few opinions before finally settling on the naming

Count me in.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"The Postgresql hackers have what I call a "NASA space shot" mentality.
Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)

Dave Cramer

On Fri, 25 Jul 2025 at 04:11, Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Jul-24, Dave Cramer wrote:

On Thu, 24 Jul 2025 at 17:05, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

On Thu, Jul 24, 2025 at 12:04 PM Dave Cramer <davecramer@gmail.com>

wrote:

+/* Replication Protocol sent by the primary */
+
+#define PqMsg_XlogData              'w'
+#define PqMsg_PrimaryKeepAlive      'k'
+#define PqMsg_PrimaryStatusUpdate   's'
+
+
+/* Replication Protocol sent by the standby */
+
+#define PqMsg_StandbyStatus         'r'
+#define PqMsg_HotStandbyFeedback    'h'
+#define PqMsg_RequestPrimaryStatus  'p'

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

I'm going to wait to see if there are any other opinions. Last time I did
this there were quite a few opinions before finally settling on the

naming

Count me in.

FYI, the reason I used XLogData is because the term is used multiple times
here https://www.postgresql.org/docs/devel/protocol-replication.html

Dave

On 2025-Jul-25, Dave Cramer wrote:

FYI, the reason I used XLogData is because the term is used multiple times
here https://www.postgresql.org/docs/devel/protocol-replication.html

Yeah, we could rename it, as in the attached. It doesn't harm anything.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On Fri, 25 Jul 2025 at 06:23, Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2025-Jul-25, Dave Cramer wrote:

FYI, the reason I used XLogData is because the term is used multiple

times

here https://www.postgresql.org/docs/devel/protocol-replication.html

Yeah, we could rename it, as in the attached. It doesn't harm anything.

Consistency is good. If your patch were applied, then it would be
consistent to use WALData

Dave

On Fri, Jul 25, 2025 at 06:28:28AM -0400, Dave Cramer wrote:

On Fri, 25 Jul 2025 at 06:23, �lvaro Herrera <alvherre@kurilemu.de> wrote:

Yeah, we could rename it, as in the attached. It doesn't harm anything.

Consistency is good. If your patch were applied, then it would be
consistent to use WALData

+1

--
nathan

On Thu, Jul 24, 2025 at 05:39:14PM -0400, Dave Cramer wrote:

On Thu, 24 Jul 2025 at 17:05, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

I'm going to wait to see if there are any other opinions. Last time I did
this there were quite a few opinions before finally settling on the naming

+1 to a new prefix. I don't have any strong opinions on the exact choice,
though. PqReplMsg, ReplMsg, PqMsg_Repl, etc. seem like some obvious
options.

--
nathan

On Fri, 25 Jul 2025 at 10:38, Nathan Bossart <nathandbossart@gmail.com>
wrote:

On Thu, Jul 24, 2025 at 05:39:14PM -0400, Dave Cramer wrote:

On Thu, 24 Jul 2025 at 17:05, Jacob Champion <
jacob.champion@enterprisedb.com> wrote:

Since these are part of the replication subprotocol (i.e. tunneled,
via CopyData) rather than the top-level wire protocol, do they deserve
their own prefix? PqReplMsg_* maybe?

I'm going to wait to see if there are any other opinions. Last time I did
this there were quite a few opinions before finally settling on the

naming

+1 to a new prefix. I don't have any strong opinions on the exact choice,
though. PqReplMsg, ReplMsg, PqMsg_Repl, etc. seem like some obvious
options.

I chose PqReplMsg patch attached

Dave

On 2025-Jul-25, Nathan Bossart wrote:

On Fri, Jul 25, 2025 at 06:28:28AM -0400, Dave Cramer wrote:

On Fri, 25 Jul 2025 at 06:23, Álvaro Herrera <alvherre@kurilemu.de> wrote:

Yeah, we could rename it, as in the attached. It doesn't harm anything.

Consistency is good. If your patch were applied, then it would be
consistent to use WALData

+1

Okay, done, thanks.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On 2025-Jul-28, Dave Cramer wrote:

I chose PqReplMsg patch attached

I think you sent a patch that applied on top of your previous patch
instead of a patch on top of master. Here it is as a standalone patch.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

On Mon, Aug 04, 2025 at 02:31:05PM +0200, �lvaro Herrera wrote:

+/* Replication Protocol, sent by the primary */
+
+#define PqReplMsg_WALData				'w'
+#define PqReplMsg_PrimaryKeepAlive		'k'
+#define PqReplMsg_PrimaryStatusUpdate	's'
+
+/* Replication Protocol, sent by the standby */
+
+#define PqReplMsg_StandbyStatus			'r'
+#define PqReplMsg_HotStandbyFeedback	'h'
+#define PqReplMsg_RequestPrimaryStatus	'p'

I know I previously +1'd a new prefix for these, but upon further review,
I'm not so sure about that. The replication protocol uses many of the
existing PqMsg macros already, so it would be a little strange if only a
subset of the replication protocol messages used the special prefix. And
IMO it would also be weird to duplicate all the macros used by both
protocols. There's also backups, which use the replication protocol but
have their own special characters [0]/messages/by-id/aIOkE7fgvFOu0FI_@nathan. If we're going the prefix route,
would we add another prefix for those, or use the replication one?

[0]: /messages/by-id/aIOkE7fgvFOu0FI_@nathan

--
nathan

On Mon, Aug 4, 2025 at 12:56 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

The replication protocol uses many of the
existing PqMsg macros already, so it would be a little strange if only a
subset of the replication protocol messages used the special prefix.

May I ask why? These messages are legitimately different; they're
tunneled through CopyData, so their reservations don't collide with
the top-level codes.

There's also backups, which use the replication protocol but
have their own special characters [0]. If we're going the prefix route,
would we add another prefix for those, or use the replication one?

My vote would be to add another. 'p' is a password message in the
top-level protocol (one of many, actually), a progress message in a
backup stream, and a status request in a replication stream, so I
think they deserve their own namespaces.

--Jacob