Thread: Fix typo about WalSndPrepareWrite
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.
Attachment
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> 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.
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.
[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
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
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,
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
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 manyfunctions in walsender.c, which is to fill the send-timestamp as late as possible so it is more accurate. This is doneby first reserving a spot in the write stream, write the actual data, and then finally write the current timestamp tothe reserved spot. This technique is used in WalSndWriteData() and also XLogSendPhysical()... so really it doesn't matterwhich 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 manyfunctions in walsender.c, which is to fill the send-timestamp as late as possible so it is more accurate. This is doneby first reserving a spot in the write stream, write the actual data, and then finally write the current timestamp tothe reserved spot. This technique is used in WalSndWriteData() and also XLogSendPhysical()... so really it doesn't matterwhich 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.