Re: Fix a race condition in ConditionVariableTimedSleep()

Hi,

On Mon, May 05, 2025 at 12:15:39PM +0300, Nazir Bilal Yavuz wrote:
> Hi,
> 
> Thanks for the report and the patch!
> 
> On Mon, 5 May 2025 at 10:53, Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> > The proposed fix attached is done in ConditionVariableTimedSleep() as this is the
> > place that introduces the race condition. It re-assigns cv_sleep_target to cv
> > and then ensures that cv_sleep_target accurately describes which condition
> > variable we’re prepared to wait on.
> >
> > Looking forward to your feedback,
> 
> I am able to reproduce the race condition and confirm that the
> proposed patch fixes the problem.

Thanks for looking at it!

> However AFAIU, the code expects that ConditionVariableCancelSleep()
> should not be called while waiting for the latch

Are you referring to this comment "We can take ourselves out of the wait list only
when the caller calls ConditionVariableCancelSleep"?

> right?

If so, I'm not sure that this comment meant to say that "we should not call
ConditionVariableCancelSleep()" prior to this point..

If that's the case then we could add an Assert that cv_sleep_target should not
be NULL instead but still just "adding" an ereport(LOG,...) is enough to break
this rule (if any) so I'm tempted to say that's better to adress the case
instead.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com