Re: Fix typo about WalSndPrepareWrite
On Thu, 18 Feb 2021 at 02:13, Cary Huang 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.
Re: Fix typo about WalSndPrepareWrite
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)
Re: Fix typo about WalSndPrepareWrite
On Thu, 14 Jan 2021 at 15:32, Kyotaro Horiguchi wrote:
> At Thu, 14 Jan 2021 06:46:35 +, Li Japin wrote in
>>
>> On Jan 14, 2021, at 12:56 PM, Ashutosh Bapat
>> mailto:ashutosh.bapat@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(_message);
>> 2748 pq_sendbyte(_message, 'w');
>> 2749
>> 2750 pq_sendint64(_message, startptr);/* dataStart */
>> 2751 pq_sendint64(_message, SendRqstPtr); /* walEnd */
>> 2752 pq_sendint64(_message, 0); /* sendtime, filled in last */
>>
>> 2803 * Fill the send timestamp last, so that it is taken as late
>> as possible.
>> 2804 */
>> 2805 resetStringInfo();
>> 2806 pq_sendint64(, GetCurrentTimestamp());
>> 2807 memcpy(_message.data[1 + sizeof(int64) + sizeof(int64)],
>> 2808tmpbuf.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.
Re: Fix typo about WalSndPrepareWrite
At Thu, 14 Jan 2021 06:46:35 +, Li Japin wrote in
>
> On Jan 14, 2021, at 12:56 PM, Ashutosh Bapat
> mailto:ashutosh.bapat@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(_message);
> 2748 pq_sendbyte(_message, 'w');
> 2749
> 2750 pq_sendint64(_message, startptr);/* dataStart */
> 2751 pq_sendint64(_message, SendRqstPtr); /* walEnd */
> 2752 pq_sendint64(_message, 0); /* sendtime, filled in last */
>
> 2803 * Fill the send timestamp last, so that it is taken as late
> as possible.
> 2804 */
> 2805 resetStringInfo();
> 2806 pq_sendint64(, GetCurrentTimestamp());
> 2807 memcpy(_message.data[1 + sizeof(int64) + sizeof(int64)],
> 2808tmpbuf.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
Re: Fix typo about WalSndPrepareWrite
On Jan 14, 2021, at 12:56 PM, Ashutosh Bapat
mailto:ashutosh.bapat@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(_message);
2748 pq_sendbyte(_message, 'w');
2749
2750 pq_sendint64(_message, startptr);/* dataStart */
2751 pq_sendint64(_message, SendRqstPtr); /* walEnd */
2752 pq_sendint64(_message, 0); /* sendtime, filled in last */
2803 * Fill the send timestamp last, so that it is taken as late
as possible.
2804 */
2805 resetStringInfo();
2806 pq_sendint64(, GetCurrentTimestamp());
2807 memcpy(_message.data[1 + sizeof(int64) + sizeof(int64)],
2808tmpbuf.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.
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
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: 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(_message);
2748 pq_sendbyte(_message, 'w');
2749
2750 pq_sendint64(_message, startptr);/* dataStart */
2751 pq_sendint64(_message, SendRqstPtr); /* walEnd */
2752 pq_sendint64(_message, 0); /* sendtime, filled in last */
2803 * Fill the send timestamp last, so that it is taken as late
as possible.
2804 */
2805 resetStringInfo();
2806 pq_sendint64(, GetCurrentTimestamp());
2807 memcpy(_message.data[1 + sizeof(int64) + sizeof(int64)],
2808tmpbuf.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 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
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. >From a5b710e744fe25e10f5a9480d3976ed36fa241ea Mon Sep 17 00:00:00 2001 From: Japin Li Date: Thu, 14 Jan 2021 12:23:56 +0800 Subject: [PATCH v1 1/1] Fix typo about WalSndPrepareWrite WalSndPrepareWrite function comment uses XLogSendPhysical to fill the sendtime, but it actually done by WalSndWriteData. --- src/backend/replication/walsender.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index fe0d368a35..87bd647338 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1243,7 +1243,7 @@ WalSndPrepareWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xi pq_sendint64(ctx->out, lsn); /* walEnd */ /* - * Fill out the sendtime later, just as it's done in XLogSendPhysical, but + * Fill out the sendtime later, just as it's done in WalSndWriteData, but * reserve space here. */ pq_sendint64(ctx->out, 0); /* sendtime */ -- 2.30.0
