Feedback-ID: i0c21471d:Fastmail
MIME-Version: 1.0
Date: Tue, 05 Aug 2025 21:18:34 -0300
From: "Euler Taveira" <euler@eulerto.com>
To: "Nathan Bossart" <nathandbossart@gmail.com>,
 "Jacob Champion" <jacob.champion@enterprisedb.com>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
 "Dave Cramer" <davecramer@gmail.com>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Message-Id: <797cad77-647b-42c4-89b8-fd5bf8a8fd10@app.fastmail.com>
In-Reply-To: <aJJidMu-E-qD9hrG@nathan>
References: 
 <CADK3HHLs-8fTiYikJBU45bkMYABVifkYPQU9s3vwO4EUcuOo=w@mail.gmail.com>
 <202508041231.myxuzx2udlze@alvherre.pgsql> <aJEQUTYLkS836qu5@nathan>
 <CAOYmi+nQ75D+OcyDa0cH3q5Hh_vioErtDgXHMaNGK4xfeG2EPA@mail.gmail.com>
 <aJEk695hVaEc4ve3@nathan> <aJJidMu-E-qD9hrG@nathan>
Subject: Re: More protocol.h replacements this time into walsender.c
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/797cad77-647b-42c4-89b8-fd5bf8a8fd10%40app.fastmail.com>
Precedence: bulk

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/