On Tue, Mar 5, 2024 at 9:15 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Mar 5, 2024 at 6:10 AM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > ======
> > src/backend/replication/walsender.c
> >
> > 5. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to wait up to the flushed position and then let the WalSender
> > + * send the changes to logical subscribers one by one which are already
> > + * covered by the flushed position without needing to wait on every change
> > + * for standby confirmation.
> > + */
> > + if (NeedToWaitForStandbys(flushed_lsn, wait_event))
> > + return true;
> > +
> > + *wait_event = 0;
> > + return false;
> > +}
> > +
> >
> > 5a.
> > The comment (or part of it?) seems misplaced because it is talking
> > WalSender sending changes, but that is not happening in this function.
> >
>
> I don't think so. This is invoked only by walsender and a static
> function. I don't see any other better place to mention this.
>
> > Also, isn't what this is saying already described by the other comment
> > in the caller? e.g.:
> >
>
> Oh no, here we are explaining the wait order.
I think there is a scope of improvement here. The comment inside
NeedToWaitForWal() which states that we need to wait here for standbys
on flush-position(and not on each change) should be outside of this
function. It is too embedded. And the comment which states the order
of wait (first flush and then standbys confirmation) should be outside
the for-loop in WalSndWaitForWal(), but yes we do need both the
comments. Attached a patch (.txt) for comments improvement, please
merge if appropriate.
thanks
Shveta
From e03332839dd804f3bc38937e677ba87ac10f981b Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.ma...@gmail.com>
Date: Tue, 5 Mar 2024 11:29:52 +0530
Subject: [PATCH v2] Comments improvement.
---
src/backend/replication/walsender.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/replication/walsender.c
b/src/backend/replication/walsender.c
index 96f44ba85d..6a082b42eb 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1799,13 +1799,6 @@ NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr
flushed_lsn,
return true;
}
- /*
- * Check if the standby slots have caught up to the flushed position. It
- * is good to wait up to the flushed position and then let the WalSender
- * send the changes to logical subscribers one by one which are already
- * covered by the flushed position without needing to wait on every
change
- * for standby confirmation.
- */
if (NeedToWaitForStandbys(flushed_lsn, wait_event))
return true;
@@ -1818,7 +1811,10 @@ NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr
flushed_lsn,
*
* If the walsender holds a logical slot that has enabled failover, we also
* wait for all the specified streaming replication standby servers to
- * confirm receipt of WAL up to RecentFlushPtr.
+ * confirm receipt of WAL up to RecentFlushPtr. It's beneficial to wait
+ * here for the confirmation from standbys up to RecentFlushPtr rather
+ * than waiting in ReorderBufferCommit() before transmitting each
+ * change to logical subscribers, which is already covered in RecentFlushPtr.
*
* Returns end LSN of flushed WAL. Normally this will be >= loc, but if we
* detect a shutdown request (either from postmaster or client) we will return
@@ -1847,6 +1843,12 @@ WalSndWaitForWal(XLogRecPtr loc)
else
RecentFlushPtr = GetXLogReplayRecPtr(NULL);
+ /*
+ * In the loop, we wait for the necessary WALs to be flushed to disk
+ * initially, and then we wait for standbys to catch up. Upon receiving
+ * the shutdown signal, we immediately transition to waiting for
standbys
+ * to catch up.
+ */
for (;;)
{
bool wait_for_standby_at_stop = false;
@@ -1877,12 +1879,10 @@ WalSndWaitForWal(XLogRecPtr loc)
XLogBackgroundFlush();
/*
- * Within the loop, we wait for the necessary WALs to be
flushed to
- * disk first, followed by waiting for standbys to catch up if
there
- * are enough WALs or upon receiving the shutdown signal. To
avoid the
- * scenario where standbys need to catch up to a newer WAL
location in
- * each iteration, we update our idea of the currently flushed
- * position only if we are not waiting for standbys to catch up.
+ * Update our idea of the currently flushed position, but do it
only
+ * if we haven't started waiting for standbys. This is done to
prevent
+ * standbys from needing to catch up to a more recent WAL
location
+ * with each iteration.
*/
if (wait_event != WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION)
{
--
2.34.1
