Обсуждение: 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

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 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)

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, 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 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].  If we're going the prefix route,
would we add another prefix for those, or use the replication one?

[0] https://postgr.es/m/aIOkE7fgvFOu0FI_%40nathan

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

On Mon, Aug 04, 2025 at 02:11:26PM -0700, Jacob Champion wrote:
> 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.

Ah, I missed that finer detail.  IIUC the codes at hands are _only_ used in
these tunneled messages, in which case they belong to a distinct category.

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

These also seem to use the same tunneling mechanism.  I retract my
objection.

-- 
nathan

Here is an updated patch that includes 1) added uses of PqMsg_* macros, 2)
new PqReplMsg_* macros, and 3) new PqBackupMsg_* macros.  Thoughts?

-- 
nathan

On 2025-Aug-05, Nathan Bossart wrote:

> Here is an updated patch that includes 1) added uses of PqMsg_* macros, 2)
> new PqReplMsg_* macros, and 3) new PqBackupMsg_* macros.  Thoughts?

Hmm, I think if you're going to add the backup ones, then it'd be good
to update ReceiveArchiveStreamChunk() to use these macros and this
header file.

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

On Tue, Aug 5, 2025, at 4:58 PM, Nathan Bossart wrote:
> Here is an updated patch that includes 1) added uses of PqMsg_* macros, 2)
> new PqReplMsg_* macros, and 3) new PqBackupMsg_* macros.  Thoughts?
>

-                * 'd' means a standby reply wrapped in a CopyData packet.
+                * CopyData means a standby reply wrapped in a CopyData
+                * packet.
                 */

Shouldn't it be PqMsg_CopyData instead of CopyData (first reference)?

The function LogicalParallelApplyLoop() has

            /*
             * The first byte of messages sent from leader apply worker to
             * parallel apply workers can only be 'w'.
             */
            c = pq_getmsgbyte(&s);
            if (c != 'w') 
                elog(ERROR, "unexpected message \"%c\"", c);

it needs to be adjusted.

The function XLogWalRcvProcessMsg() has

    switch (type)
    {
        case 'w':               /* WAL records */
            {
                StringInfoData incoming_message;

...

        case 'k':               /* Keepalive */
            {
                StringInfoData incoming_message;


it also needs to be adjusted.

There is also some references into receivelog.c. The functions sendFeedback()
and HandleCopyStream() contain some references to be replaced. There are other
functions too that refers to these replication codes on comments. It seems a
good idea to replace these references too.

There are also some references in pg_recvlogical.c. The functions
sendFeedback() and StreamLogicalLog() contain some references to be replaced.

Alvaro already pointed out the other cases in the pg_basebackup.c file.

May I suggest that you put these code before authentication codes? It seems
natural to have these new codes near the existing ones.

For the reference, I used grep -r -E "'[a-z]'" in some directories to catch
these cases.

It is a bit off-topic for this thread but looking at the replication protocol
messages, a different pattern is used for naming the messages such as

WALData
Primary keepalive message
Standby status update
Hot standby feedback message
new archive
manifest
archive or manifest data
progress report

I would expect to see the same pattern (Pascal case) as the wire protocol.

AuthenticationSASLFinal
BackendKeyData
Bind


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/

Okay, I think I've addressed all the latest feedback in v5.

-- 
nathan

On 2025-Aug-05, Nathan Bossart wrote:

> Okay, I think I've addressed all the latest feedback in v5.

LGTM.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor.  Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

On Wed, Aug 6, 2025, at 12:26 AM, Nathan Bossart wrote:
> Okay, I think I've addressed all the latest feedback in v5.
>

LGTM.

I tried to apply your patch and it says

$ git am -3 v5-0001-Expand-usage-of-protocol-characters.patch
Applying: Expand usage of protocol characters.
Warning: commit message did not conform to UTF-8.
You may want to amend it after fixing the message, or set the config
variable i18n.commitEncoding to the encoding your project uses.

It seems your file is ISO-8859-1 but your commit message claims to be UTF-8.


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/

On Wed, Aug 06, 2025 at 02:54:13PM -0300, Euler Taveira wrote:
> I tried to apply your patch and it says
> 
> $ git am -3 v5-0001-Expand-usage-of-protocol-characters.patch
> Applying: Expand usage of protocol characters.
> Warning: commit message did not conform to UTF-8.
> You may want to amend it after fixing the message, or set the config
> variable i18n.commitEncoding to the encoding your project uses.
> 
> It seems your file is ISO-8859-1 but your commit message claims to be UTF-8.

I think my .muttrc needed some adjustments.  Let's see if this works...

-- 
nathan

Committed.

-- 
nathan