Re: Synchronizing slots from primary to standby

On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, Mar 3, 2024 at 5:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
...
> > 9. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to wait up to flushed position and then let it send the changes
> > + * to logical subscribers one by one which are already covered in flushed
> > + * position without needing to wait on every change for standby
> > + * confirmation. Note that after receiving the shutdown signal, an ERROR
> > + * is reported if any slots are dropped, invalidated, or inactive. This
> > + * measure is taken to prevent the walsender from waiting indefinitely.
> > + */
> > + if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))
> > + return true;
> >
> > I felt it was confusing things for this function to also call to the
> > other one -- it seems an overlapping/muddling of the purpose of these.
> > I think it will be easier to understand if the calling code just calls
> > to one or both of these functions as required.
> >
>
> I felt otherwise because the caller has to call these functions at
> more than one place which makes the caller's code difficult to follow.
> It is better to encapsulate the computation of wait_event.
>

You may have misinterpreted my review comment because I didn't say
anything about changing the encapsulation of the computation of the
wait_event.

I only wrote it is better IMO for the functions to stick to just one
job each according to their function name. E.g.:
- NeedToWaitForStandby should *only* check if we need to wait for standbys
- NeedToWaitForWal should *only* check if we need to wait for WAL
flush; i.e. this shouldn't be also calling NeedToWaitForStandby().

Also, AFAICT the caller changes should not be difficult. Indeed, these
changes will make the code aligned properly with what the comments are
saying:

BEFORE
/*
 * Fast path to avoid acquiring the spinlock in case we already know we
 * have enough WAL available and all the standby servers have confirmed
 * receipt of WAL up to RecentFlushPtr. This is particularly interesting
 * if we're far behind.
 */
if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event))
  return RecentFlushPtr;

SUGGESTED
...
if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event) &&
  !NeedToWaitForStandby(loc, RecentFlushPtr, &wait_event)
  return RecentFlushPtr;

~~~

BEFORE
/*
 * Exit the loop if already caught up and doesn't need to wait for
 * standby slots.
 */
if (!wait_for_standby_at_stop &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event))
  break;

SUGGESTED
...
if (!wait_for_standby_at_stop &&
  !NeedToWaitForWal(loc, RecentFlushPtr, &wait_event) &&
  !NeedToWaitForStandby(loc, RecentFlushPtr, &wait_event))
  break;

----------
Kind Regards,
Peter Smith.
Fujitsu Australia