Re: Minimal logical decoding on standbys

On Fri, Mar 31, 2023 at 7:14 PM Drouvot, Bertrand
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> On 3/31/23 1:58 PM, Amit Kapila wrote:
> > On Fri, Mar 31, 2023 at 4:17 PM Drouvot, Bertrand
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >>
> >
> > + * This is needed for logical decoding on standby. Indeed the "problem" is that
> > + * WalSndWaitForWal() waits for the *replay* LSN to increase, but gets woken up
> > + * by walreceiver when new WAL has been flushed. Which means that typically
> > + * walsenders will get woken up at the same time that the startup process
> > + * will be - which means that by the time the logical walsender checks
> > + * GetXLogReplayRecPtr() it's unlikely that the startup process
> > already replayed
> > + * the record and updated XLogCtl->lastReplayedEndRecPtr.
> > + *
> > + * The ConditionVariable XLogRecoveryCtl->replayedCV solves this corner case.
> >
> > IIUC we are introducing condition variables as we can't rely on
> > current wait events because they will lead to spurious wakeups for
> > logical walsenders due to the below code in walreceiver:
> > XLogWalRcvFlush()
> > {
> > ...
> > /* Signal the startup process and walsender that new WAL has arrived */
> > WakeupRecovery();
> > if (AllowCascadeReplication())
> > WalSndWakeup();
> >
> > Is my understanding correct?
> >
>
> Both the walsender and the startup process are waked up at the
> same time. If the walsender does not find any new record that has been replayed
> (because the startup process did not replay yet), then it will sleep during i
> ts timeout time (then delaying the decoding).
>
> The CV helps to wake up the walsender has soon as a replay is done.
>
> > Can't we simply avoid waking up logical walsenders at this place and
> > rather wake them up at ApplyWalRecord() where the 0005 patch does
> > conditionvariable broadcast? Now, there doesn't seem to be anything
> > that distinguishes between logical and physical walsender but I guess
> > we can add a variable in WalSnd structure to identify it.
> >
>
> That sounds like a good idea. We could imagine creating a LogicalWalSndWakeup()
> doing the Walsender(s) triage based on a new variable (as you suggest).
>
> But, it looks to me that we:
>
> - would need to go through the list of all the walsenders to do the triage
> - could wake up some logical walsender(s) unnecessary
>

Why it could wake up unnecessarily?

> This extra work would occur during each replay.
>
> while with the CV, only the ones in the CV wait queue would be waked up.
>

Currently, we wake up walsenders only after writing some WAL records
at the time of flush, so won't it be better to wake up only after
applying some WAL records rather than after applying each record?

--
With Regards,
Amit Kapila.