Fix typo about WalSndPrepareWrite
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t43531psql -h localhost -U postgresBuilt from patchset v1 (message #1), October 23, 2026 at 12:54 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t43531_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t43531_1 && git checkout t43531_1Patchset v1 (message #1) is on t43531_1
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.
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, passedHi,
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.