Horton
Menu

Thread

  1. Fix a race condition in ConditionVariableTimedSleep()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-05-05T07:52:46Z

    Hi hackers,

While working on wait event related stuff I observed a failed assertion:

"
TRAP: failed Assert("node->next == 0 && node->prev == 0"), File: "../../../../src/include/storage/proclist.h", Line: 91
"

during pg_regress/database.

To reproduce, add an ereport(LOG,..) or CHECK_FOR_INTERRUPTS() or whatever
would trigger ConditionVariableBroadcast() in pgstat_report_wait_end():

And launch:

postgres=# CREATE TABLESPACE regress_tblspace LOCATION '/home/postgres/bdttbs';
CREATE TABLESPACE
postgres=# create database regression_bdt9;
CREATE DATABASE
postgres=# ALTER DATABASE regression_bdt9 SET TABLESPACE regress_tblspace;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

The issue is that in ConditionVariableTimedSleep() during the WaitLatch() we’ll
also trigger ProcessInterrupts() that will call ConditionVariableCancelSleep()
that will:

- remove the process from the wait list
- and also set cv_sleep_target to NULL.

Then in ConditionVariableTimedSleep(), we’ll go through:

“
          /*
           * If this process has been taken out of the wait list, then we know
           * that it has been signaled by ConditionVariableSignal (or
           * ConditionVariableBroadcast), so we should return to the caller. But
           * that doesn't guarantee that the exit condition is met, only that we
           * ought to check it.  So we must put the process back into the wait
           * list, to ensure we don't miss any additional wakeup occurring while
           * the caller checks its exit condition.  We can take ourselves out of
           * the wait list only when the caller calls
           * ConditionVariableCancelSleep.
           *
           * If we're still in the wait list, then the latch must have been set
           * by something other than ConditionVariableSignal; though we don't
           * guarantee not to return spuriously, we'll avoid this obvious case.
           */
          SpinLockAcquire(lock: &cv->mutex);
          if (!proclist_contains(&cv->wakeup, MyProcNumber, cvWaitLink))
          {
              done = true;
              proclist_push_tail(&cv->wakeup, MyProcNumber, cvWaitLink);
          }
          SpinLockRelease(&cv->mutex);
“

Means we re-add the process in the wait list. The issue is that cv_sleep_target
is still NULL so that we’ll exit ConditionVariableTimedSleep() due to:

"
          if (cv != cv_sleep_target)
              done = true;

          /* We were signaled, so return */
          if (done)
              return false;
"

Later we’ll want to add our process to a wait list, calling ConditionVariablePrepareToSleep()
that will not call ConditionVariableCancelSleep() due to:

“
      if (cv_sleep_target != NULL)
          ConditionVariableCancelSleep();
“

As cv_sleep_target is still NULL.
Finally calling proclist_push_tail() that will trigger the failed assertion.

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,

Regards,

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

  2. Re: Fix a race condition in ConditionVariableTimedSleep()

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2025-05-05T09:15:39Z

    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.

However AFAIU, the code expects that ConditionVariableCancelSleep()
should not be called while waiting for the latch, right? If that is
the case, does not the reproducer code violate this?

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  3. Re: Fix a race condition in ConditionVariableTimedSleep()

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-05-05T11:15:15Z

    05.05.2025 10:52, Bertrand Drouvot пишет:
> Hi hackers,
> 
> While working on wait event related stuff I observed a failed assertion:
> 
> "
> TRAP: failed Assert("node->next == 0 && node->prev == 0"), File: "../../../../src/include/storage/proclist.h", Line: 91
> "
> 
> during pg_regress/database.
> 
> To reproduce, add an ereport(LOG,..) or CHECK_FOR_INTERRUPTS() or whatever
> would trigger ConditionVariableBroadcast() in pgstat_report_wait_end():


Interestingly, our colleague stepped into same problem recently [1] . It
happened because he attempted to make overcomplex timeout (SIGALARM) handler.

But his solution was a bit different [2].

[1] https://postgr.es/m/076eb7bd-52e6-4a51-ba00-c744d027b15c@postgrespro.ru
[2]
https://postgr.es/m/attachment/175030/0001-CV-correctly-handle-cv_sleep_target-change.patch

And I believe, his solution is more elegant. Doesn't it?

But in first step, I doubt there should be any thing that cancels condition
variable during WaitLatch. Most probably you did wrong thing.

We convinced the colleague to rework the code to not trigger the issue in
first place.

-- 
regards
Yura Sokolov aka funny-falcon

  4. Re: Fix a race condition in ConditionVariableTimedSleep()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-05-05T13:38:33Z

    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

  5. Re: Fix a race condition in ConditionVariableTimedSleep()

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-05-05T14:05:17Z

    Hi,

On Mon, May 05, 2025 at 02:15:15PM +0300, Yura Sokolov wrote:
> Interestingly, our colleague stepped into same problem recently [1] . It
> happened because he attempted to make overcomplex timeout (SIGALARM) handler.
> 
> But his solution was a bit different [2].
> 
> [1] https://postgr.es/m/076eb7bd-52e6-4a51-ba00-c744d027b15c@postgrespro.ru
> [2]
> https://postgr.es/m/attachment/175030/0001-CV-correctly-handle-cv_sleep_target-change.patch
> 
> And I believe, his solution is more elegant. Doesn't it?

I'm not sure as it'd not maintain the initial intent to re-add to the
wait list.

> But in first step, I doubt there should be any thing that cancels condition
> variable during WaitLatch.

I'm not 100% sure.

>  Most probably you did wrong thing.

I "just" added an ereport(LOG,..) that has been enough to cancel the condition
variable and trigger the failed assertion. I agree that you could still consider
this as wrong thing if you think that we should not do anything that cancels
condition variable during WaitLatch though.

Regards,

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

  6. Re: Fix a race condition in ConditionVariableTimedSleep()

    Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-05-05T16:21:29Z

    05.05.2025 17:05, Bertrand Drouvot пишет:
> Hi,
> 
> On Mon, May 05, 2025 at 02:15:15PM +0300, Yura Sokolov wrote:
>> Interestingly, our colleague stepped into same problem recently [1] . It
>> happened because he attempted to make overcomplex timeout (SIGALARM) handler.
>>
>> But his solution was a bit different [2].
>>
>> [1] https://postgr.es/m/076eb7bd-52e6-4a51-ba00-c744d027b15c@postgrespro.ru
>> [2]
>> https://postgr.es/m/attachment/175030/0001-CV-correctly-handle-cv_sleep_target-change.patch
>>
>> And I believe, his solution is more elegant. Doesn't it?
> 
> I'm not sure as it'd not maintain the initial intent to re-add to the
> wait list.

It will be re-added in next iteration of outer sleeping loop.

>> But in first step, I doubt there should be any thing that cancels condition
>> variable during WaitLatch.
> 
> I'm not 100% sure.

There're no such things at the moment. WaitLatch doesn't check for
interrupts and doesn't do any other thing that could break
ConditionVariable. And I believe, if it start to do, we're going to be in
trouble.

> I "just" added an ereport(LOG,..) that has been enough to cancel the
condition
> variable and trigger the failed assertion.

It is because errfinish, called from ereport, invokes CHECK_FOR_INTERRUPTS.

Just wrap you ereport with HOLD_INTERRUPTS/RESUME_INTERRUPTS, and
everything will be "just" fine.

-- 
regards
Yura Sokolov aka funny-falcon