Re: PoC: Add condition variable support to WaitEventSetWait() - Mailing list pgsql-hackers

Hi Yura,

On Wed, Apr 22, 2026 at 1:06 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>
> 08.04.2026 11:22, Chao Li пишет:
> >> On Apr 8, 2026, at 11:50, Chao Li <li.evan.chao@gmail.com> wrote:
> >>> On Apr 2, 2026, at 15:38, Chao Li <li.evan.chao@gmail.com> wrote:
> >>>> On Mar 31, 2026, at 16:59, Chao Li <li.evan.chao@gmail.com> wrote:
> >>>>> On Mar 31, 2026, at 15:28, Chao Li <li.evan.chao@gmail.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> There is an XXX comment in WalSndWait():
> >>>>> ```
> >>>>>  * XXX: A desirable future improvement would be to add support for CVs
> >>>>>  * into WaitEventSetWait().
> >>>>> ```
> >>>>>
> >>>>> I have been exploring a possible approach for that. This patch is a PoC that adds ConditionVariable support to
WaitEventSet.This v1 is mainly intended to gather feedback on the design, so I have only done some basic testing so
far,such as a normal logical replication workflow. 
> >>>>>
> >>>>> I’d like to highlight a few key points about the design:
> >>>>>
> >>>>> 1. In the current WalSndWait(), although it prepares to sleep on a ConditionVariable, it does not actually
checkwhether the CV has been signaled. In this PoC, I kept that same behavior. However, I tried to make the
WaitEventSetsupport for CVs generic, so that if we want to add actual signal checking in the future, that would be
possible.
> >>>>>
> >>>>> 2. To keep the design generic, this patch introduces a new wait event type, WL_CONDITION_VARIABLE. A
WL_CONDITION_VARIABLEevent occupies a position in the event array, similar to latch and socket events. When a CV is
signaled,the corresponding WL_CONDITION_VARIABLE event is returned in occurred_events. 
> >>>>>
> >>>>> 3. The WaitEventSet APIs AddWaitEventToSet() and ModifyWaitEvent() are extended to support CVs by adding one
moreparameter “cv" to both APIs. The downside of this approach is that all call sites of these two APIs need to be
updated.I also considered adding separate APIs for CVs, such as AddWaitEventToSetForCV() and ModifyWaitEventForCV(),
sinceCVs do not rely on the kernel and it might therefore make sense to decouple them from socket and latch handling.
Butfor v1, I chose the more generic approach. I’d be interested in hearing comments on this part of the design. 
> >>>>>
> >>>>> 4. One important point is that this patch extends the WaitEventSet abstraction, not the underlying kernel wait
primitives.A ConditionVariable is still a userspace/shared-memory concept, but with this design it can participate in
thesame waiting framework as sockets and latches. I think that is useful because it allows mixed waits to be handled
throughone interface. 
> >>>>>
> >>>>> Here is the v1 patch.
> >>>>>
> >>>>
> >>>> I just noticed that I missed checking in my last edit when switching to the other branch, so attaching an
updatedv1. 
> >>>
> >>> PFA v2 - fixed a CI failure from contrib/postgres_fdw.
> >>
> >> Fixed a CI test failure and rebased.
> >
> > PFA v4 - try to fix a test failure on windows.
>
> Good day, Chao Li.
>
> ConditionalVariable works through the MyLatch.
> And almost always there is single latch for one backend:
>  MyLatch = PGPROC->procLatch;
>
> (Single exception I found is XLogRecoveryCtl->recoveryWakeupLatch.)
>
> And that is where ConditionVariablePrepareToSleep were used in WalSndWait
> but without check: FeBeWaitSet always waits for MyLatch.
>
> Therefore, I don't clear get, how you suggest to simultaneously wait
> WL_LATCH_SET on MyLatch and WL_CONDITION_VARIABLE?
> How you will distinguish which one was fired?
>
> It looks to my, WL_LATCH_SET on MyLatch and WL_CONDITION_VARIABLE had to be
> mutual exclusive. At least unless ConditionVariable internal are changed.
>
> And check after waiting for conditional variable set had to be placed into
> WaitEventSetWaitBlock where all other such checks are.
>
> All written above is just my humble opinion.
> Excuse me if i'm too strict.
>

I think you're correct that ConditionVariableSignal() wakes the target
process by calling SetLatch(&proc->procLatch). So every CV signal also
sets MyLatch. The patch introduces ConditionVariableSignaled() to
distinguish the two by checking CV wait-list membership (not the latch
state itself), so in principle the two sources are distinguishable.

In practice, however, the current patch seems not reliably report
WL_CONDITION_VARIABLE.

Inside WaitEventSetWait(), the latch check runs before the new CV
check. When a CV signal fires, both conditions become true
simultaneously: MyLatch is set AND the proc has been removed from the
CV wait list. With nevents=1 (which is what WalSndWait passes), the
latch event fills the one-slot output buffer and the loop breaks
before reaching the CV check:
/* existing latch check — runs first */
if (set->latch && set->latch->is_set)
{
    ...
    returned_events++;
    if (returned_events == nevents)
        break;       /* CV check below is never reached */
}

/* new CV check — unreachable when buffer is full */
if (set->cv)
    cv_signaled = ConditionVariableSignaled(set->cv);

So for the converted caller, WL_CONDITION_VARIABLE is not reliably
observable. In the common case where the latch readiness is returned
first, the one-slot buffer is filled by WL_LATCH_SET and the CV check
is skipped.

--
Best,
Xuneng