Hi Yura,
On Wed, Apr 22, 2026 at 1:06 AM Yura Sokolov <[email protected]> wrote:
>
> 08.04.2026 11:22, Chao Li пишет:
> >> On Apr 8, 2026, at 11:50, Chao Li <[email protected]> wrote:
> >>> On Apr 2, 2026, at 15:38, Chao Li <[email protected]> wrote:
> >>>> On Mar 31, 2026, at 16:59, Chao Li <[email protected]> wrote:
> >>>>> On Mar 31, 2026, at 15:28, Chao Li <[email protected]> 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 check whether the CV has been
> >>>>> signaled. In this PoC, I kept that same behavior. However, I tried to
> >>>>> make the WaitEventSet support 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_VARIABLE event 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 more parameter “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(), since CVs
> >>>>> do not rely on the kernel and it might therefore make sense to decouple
> >>>>> them from socket and latch handling. But for 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 the same waiting framework as sockets
> >>>>> and latches. I think that is useful because it allows mixed waits to be
> >>>>> handled through one 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 updated v1.
> >>>
> >>> 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
