Fix typo about WalSndPrepareWrite

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

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.

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.

On Thu, Jan 14, 2021 at 10:10 AM japin <japinli@hotmail.com> wrote:

Hi,

While reading the code about logical replication, I found that
WalSndPrepareWrite function says it use XLogSendPhysical to fill out the
sendtime, however, it actually done by WalSndWriteData. It looks like a
typo, attaching a very small patch to correct it.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

--
Best Wishes,
Ashutosh Bapat

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]src/backend/replication/walsender.c:247:static void WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid, 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, TransactionId xid, bool last_write) src/backend/replication/walsender.c:1255: * Actually write out data previously prepared by WalSndPrepareWrite out to.
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.

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?

[1]: src/backend/replication/walsender.c:247:static void WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid, 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, TransactionId xid, bool last_write) src/backend/replication/walsender.c:1255: * Actually write out data previously prepared by WalSndPrepareWrite out to
src/backend/replication/walsender.c:247:static void WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xid, 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, TransactionId xid, bool last_write)
src/backend/replication/walsender.c:1255: * Actually write out data previously prepared by WalSndPrepareWrite out to

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

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, TransactionId xid, 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, TransactionId xid, 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

On Thu, 14 Jan 2021 at 15:32, Kyotaro Horiguchi wrote:

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.

Is that mean the sendtime of WalSndPrepareWrite always fill out by WalSndWriteData?
If it is, I think we should modify the comment in WalSndPrepareWrite.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi,

thanks for the patch, however I don't think it is a typo. The comment indicates a technique that is used throughout many functions in walsender.c, which is to fill the send-timestamp as late as possible so it is more accurate. This is done by first reserving a spot in the write stream, write the actual data, and then finally write the current timestamp to the reserved spot. This technique is used in WalSndWriteData() and also XLogSendPhysical()... so really it doesn't matter which function name to put in the comment.

thank you!

-----------
Cary Huang
HighGo Software (Canada)

On Thu, 18 Feb 2021 at 02:13, Cary Huang <cary.huang@highgo.ca> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi,

thanks for the patch, however I don't think it is a typo. The comment indicates a technique that is used throughout many functions in walsender.c, which is to fill the send-timestamp as late as possible so it is more accurate. This is done by first reserving a spot in the write stream, write the actual data, and then finally write the current timestamp to the reserved spot. This technique is used in WalSndWriteData() and also XLogSendPhysical()... so really it doesn't matter which function name to put in the comment.

After review the code. It says "just as it's done in XLogSendPhysical", not fill
out the sendtime with XLogSendPhysical.

My bad. Sorry for the noise. I will close this cf entry.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.