MIME-Version: 1.0
References: <F12DAA03-F4FA-4F12-8F17-B7C4BE361ED0@gmail.com>
 <63B9B6C7-BB04-4F6E-B3AC-6886DDC1C3FD@gmail.com>
 <CC114539-0EEF-415C-974E-AC864D3EFD22@gmail.com>
 <00D3DE2F-10B8-4439-91B4-D1C24A98627A@gmail.com>
 <DCFECDB5-1A12-49B4-814D-A6E94180D258@gmail.com>
 <ff590ff5-34a7-46c6-9f82-d0d0e43ddaec@postgrespro.ru>
In-Reply-To: <ff590ff5-34a7-46c6-9f82-d0d0e43ddaec@postgrespro.ru>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Wed, 22 Apr 2026 10:58:01 +0800
Message-ID: 
 <CABPTF7Vk2bZVoXpzfyGdaXQj+iyj5aenGU+9ryYR=imDynQkWA@mail.gmail.com>
Subject: Re: PoC: Add condition variable support to WaitEventSetWait()
To: Yura Sokolov <y.sokolov@postgrespro.ru>
Cc: Chao Li <li.evan.chao@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABPTF7Vk2bZVoXpzfyGdaXQj%2Biyj5aenGU%2B9ryYR%3DimDynQkWA%40mail.gmail.com>
Precedence: bulk

Hi Yura,

On Wed, Apr 22, 2026 at 1:06=E2=80=AFAM Yura Sokolov <y.sokolov@postgrespro=
.ru> wrote:
>
> 08.04.2026 11:22, Chao Li =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> >> 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=E2=80=99d 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 signa=
led. In this PoC, I kept that same behavior. However, I tried to make the W=
aitEventSet support for CVs generic, so that if we want to add actual signa=
l checking in the future, that would be possible.
> >>>>>
> >>>>> 2. To keep the design generic, this patch introduces a new wait eve=
nt type, WL_CONDITION_VARIABLE. A WL_CONDITION_VARIABLE event occupies a po=
sition in the event array, similar to latch and socket events. When a CV is=
 signaled, the corresponding WL_CONDITION_VARIABLE event is returned in occ=
urred_events.
> >>>>>
> >>>>> 3. The WaitEventSet APIs AddWaitEventToSet() and ModifyWaitEvent() =
are extended to support CVs by adding one more parameter =E2=80=9Ccv" to bo=
th 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, su=
ch 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=E2=80=99d 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 pa=
rticipate in the same waiting framework as sockets and latches. I think tha=
t is useful because it allows mixed waits to be handled through one interfa=
ce.
> >>>>>
> >>>>> 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 =3D 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 int=
o
> 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=3D1 (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 =E2=80=94 runs first */
if (set->latch && set->latch->is_set)
{
    ...
    returned_events++;
    if (returned_events =3D=3D nevents)
        break;       /* CV check below is never reached */
}

/* new CV check =E2=80=94 unreachable when buffer is full */
if (set->cv)
    cv_signaled =3D 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