Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: PoC: Add condition variable support to WaitEventSetWait()
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <caccf9f0-6ed5-4ae5-88f6-d71e68ee0e9b@postgrespro.ru>
Date: Thu, 23 Apr 2026 16:15:40 +0800
Cc: Xuneng Zhou <xunengzhou@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <289B3400-74C9-4C1C-9882-2C6F3C515FE2@gmail.com>
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>
 <CABPTF7Vk2bZVoXpzfyGdaXQj+iyj5aenGU+9ryYR=imDynQkWA@mail.gmail.com>
 <caccf9f0-6ed5-4ae5-88f6-d71e68ee0e9b@postgrespro.ru>
To: Yura Sokolov <y.sokolov@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/289B3400-74C9-4C1C-9882-2C6F3C515FE2%40gmail.com>
Precedence: bulk


> On Apr 22, 2026, at 21:13, Yura Sokolov <y.sokolov@postgrespro.ru> =
wrote:
>=20
> 22.04.2026 05:58, Xuneng Zhou =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
>> Hi Yura,
>>=20
>> On Wed, Apr 22, 2026 at 1:06=E2=80=AFAM Yura Sokolov =
<y.sokolov@postgrespro.ru> wrote:
>>>=20
>>> 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:
>>>>>>>>=20
>>>>>>>> Hi,
>>>>>>>>=20
>>>>>>>> There is an XXX comment in WalSndWait():
>>>>>>>> ```
>>>>>>>> * XXX: A desirable future improvement would be to add support =
for CVs
>>>>>>>> * into WaitEventSetWait().
>>>>>>>> ```
>>>>>>>>=20
>>>>>>>> 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.
>>>>>>>>=20
>>>>>>>> I=E2=80=99d like to highlight a few key points about the =
design:
>>>>>>>>=20
>>>>>>>> 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.
>>>>>>>>=20
>>>>>>>> 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.
>>>>>>>>=20
>>>>>>>> 3. The WaitEventSet APIs AddWaitEventToSet() and =
ModifyWaitEvent() are extended to support CVs by adding one more =
parameter =E2=80=9Ccv" 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=E2=80=99d be interested in hearing comments on this part of =
the design.
>>>>>>>>=20
>>>>>>>> 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.
>>>>>>>>=20
>>>>>>>> Here is the v1 patch.
>>>>>>>>=20
>>>>>>>=20
>>>>>>> I just noticed that I missed checking in my last edit when =
switching to the other branch, so attaching an updated v1.
>>>>>>=20
>>>>>> PFA v2 - fixed a CI failure from contrib/postgres_fdw.
>>>>>=20
>>>>> Fixed a CI test failure and rebased.
>>>>=20
>>>> PFA v4 - try to fix a test failure on windows.
>>>=20
>>> Good day, Chao Li.
>>>=20
>>> ConditionalVariable works through the MyLatch.
>>> And almost always there is single latch for one backend:
>>> MyLatch =3D PGPROC->procLatch;
>>>=20
>>> (Single exception I found is XLogRecoveryCtl->recoveryWakeupLatch.)
>>>=20
>>> And that is where ConditionVariablePrepareToSleep were used in =
WalSndWait
>>> but without check: FeBeWaitSet always waits for MyLatch.
>>>=20
>>> 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?
>>>=20
>>> 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.
>>>=20
>>> And check after waiting for conditional variable set had to be =
placed into
>>> WaitEventSetWaitBlock where all other such checks are.
>>>=20
>>> All written above is just my humble opinion.
>>> Excuse me if i'm too strict.
>>>=20
>>=20
>> 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.
>=20
> Nope.
> Patch may say if ConditionVariable was signalled.
> But it cann't say if MyLatch were set as well without touching
> ConditionVariable.
> It is not better than current state in term of code clearness and =
flawlessness.

Hi Yura,

Thank you for the review. This patch is marked as =E2=80=9CPoC=E2=80=9D, =
so nothing has been finalized yet, and I am open to any comments and =
suggestions.

In theory, ConditionVariable relies on MyLatch, and in most cases a =
process has only one MyLatch, so your point that WL_LATCH_SET on MyLatch =
and WL_CONDITION_VARIABLE should ideally be mutually exclusive makes =
sense.

However, the current code does not really follow that principle. Looking =
at the current code in WalSndWait():
```
	if (wait_event =3D=3D WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION)
		=
ConditionVariablePrepareToSleep(&WalSndCtl->wal_confirm_rcv_cv);
	else if (MyWalSnd->kind =3D=3D REPLICATION_KIND_PHYSICAL)
		=
ConditionVariablePrepareToSleep(&WalSndCtl->wal_flush_cv);
	else if (MyWalSnd->kind =3D=3D REPLICATION_KIND_LOGICAL)
		=
ConditionVariablePrepareToSleep(&WalSndCtl->wal_replay_cv);

	if (WaitEventSetWait(FeBeWaitSet, timeout, &event, 1, =
wait_event) =3D=3D 1 &&
		(event.events & WL_POSTMASTER_DEATH))
	{
		ConditionVariableCancelSleep();
		proc_exit(1);
	}

	ConditionVariableCancelSleep();
```

It calls ConditionVariablePrepareToSleep() on a specific CV, so ideally =
only ConditionVariableSignal() on that specific CV should wake the wait.

However, the current code actually relies on =
WaitEventSetWait(FeBeWaitSet) waiting on MyLatch, where FeBeWaitSet =
includes WL_LATCH_SET. So if someone calls SetLatch(the_proc_latch), the =
wait will still wake up. The code does not check at all which CV was =
actually signaled.

With this patch, we would be able to verify whether the CV was signaled. =
The current PoC does not do that yet, because I simply followed the =
existing behavior, but with the new WaitEventSetWait() support, it would =
be easy to add such a check. =46rom that perspective, I think the PoC is =
already better than the current code.

As for your question, =E2=80=9CHow will you distinguish which one was =
fired?=E2=80=9D: when FeBeWaitSet includes both WL_LATCH_SET and =
WL_CONDITION_VARIABLE, and MyLatch is used for WL_LATCH_SET, then if the =
CV is signaled, WL_LATCH_SET may also fire as a side effect. I think =
that would be okay. Do you have a use case where that would lead to a =
problem?

Again, this PoC version is still far from the final version. Any =
discussion is very welcome.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/