Re: Fix typo about WalSndPrepareWrite - Mailing list pgsql-hackers

At Thu, 14 Jan 2021 06:46:35 +0000, Li Japin <japinli@hotmail.com> wrote in 
> 
> On Jan 14, 2021, at 12:56 PM, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com<mailto:ashutosh.bapat.oss@gmail.com>>
wrote:
> 
> Hi Japin,
> Thanks for the report.
> 
> I think that comment is correct. It refers to the following code
> blocks of XLogSendPhysical()
> 
> 2744     /*
> 2745      * OK to read and send the slice.
> 2746      */
> 2747     resetStringInfo(&output_message);
> 2748     pq_sendbyte(&output_message, 'w');
> 2749
> 2750     pq_sendint64(&output_message, startptr);    /* dataStart */
> 2751     pq_sendint64(&output_message, SendRqstPtr); /* walEnd */
> 2752     pq_sendint64(&output_message, 0);   /* sendtime, filled in last */
> 
> 2803      * Fill the send timestamp last, so that it is taken as late
> as possible.
> 2804      */
> 2805     resetStringInfo(&tmpbuf);
> 2806     pq_sendint64(&tmpbuf, GetCurrentTimestamp());
> 2807     memcpy(&output_message.data[1 + sizeof(int64) + sizeof(int64)],
> 2808            tmpbuf.data, sizeof(int64));
> 2809
> 2810     pq_putmessage_noblock('d', output_message.data, output_message.len);
> 
> 
> After a quick search, I found that WalSndPrepareWrite and WalSndWriteData are always pairs [1].
> IIUC the space of sendtime leave by WalSndPrepareWrite, it always fill out by WalSndWriteData.
> 
> 
> WalSndWriteData() also fills the timestamp there but it may not always
> be used with WalSndPrepareWrite, at least theoretically. So it's the
> XLogSendPhysical() that it's referring to.

The two functions are the body of two logical-decoding API
functions. They are assumed to be called in that order. See
OutputPluginWrite() for the restriction. The sequence of the two
logica-decoding funcitons and the code block in XLogSendPhysical are
parallels in (theoretically) different protocols.

> Maybe a better way is to separate those two codeblocks into functions
> and use it in WalSndPrepareWrite, WalSndWriteData and XLogSendPhysical
> appropriately. That way we don't have to add a comment like that and
> the sanity of 'w' message is preserved.
> 
> Sure, we can write a separate function to fill out the sendtime.  Any thoughts?

So I put -1 since they (physical and logical, not prepare and write)
are for distinct protocols.

> [1]
> src/backend/replication/walsender.c:247:static void WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn,
TransactionIdxid, bool last_write);
 
> src/backend/replication/walsender.c:1015:
 WalSndPrepareWrite, WalSndWriteData,
 
> src/backend/replication/walsender.c:1176:                                                         WalSndPrepareWrite,
WalSndWriteData,
> src/backend/replication/walsender.c:1233:WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn,
TransactionIdxid, bool last_write)
 
> src/backend/replication/walsender.c:1255: * Actually write out data previously prepared by WalSndPrepareWrite out to

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center