Horton

Re: Condition variable live lock

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Thomas Munro <thomas.munro@enterprisedb.com>, Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2018-01-08T20:45:02Z
Lists: pgsql-hackers 
Robert Haas <robertmhaas@gmail.com> writes:
> I guess my aversion to converting the existing Assert-test into an
> elog test was really a concern that we'd be countenancing the use of
> CVs in any coding pattern more complicated than a very simple
> test-and-wait loop.  Suppose someone were to propose adding runtime
> checks that when we release a spinlock, it is held by the process that
> tried to release it.  Someone might reply that such a check ought to
> be unnecessary because the code protected by a spinlock ought to be a
> short, straight-line critical section and therefore we should be able
> to spot any such coding error by inspection.

Right, *because* we have the rule that spinlock-protected code must be
short straight-line segments.  There is certainly no such rule for
LWLocks, and I'm not sure why you think that CVs have more restricted
use-cases than LWLocks.

There is a potential issue, if you call random code inside the wait loop,
that that code might reset the process latch and thereby lose a signal for
the CV.  As far as I can see at the moment, that'd only be a hazard for
code executed before the loop's ConditionVariableSleep, not after, so even
that can be avoided if you code properly.  In any case, the proposed
generalization of CVs poses no such hazard: yes, we might reset the latch,
but if so the outer CV loop will see that it's lost its prepared sleep
state and will be forced to recheck its exit condition.

> It's often the case that mechanisms like this end up getting
> used profitably in a lot of places not imagined by their original
> creator, and that might be the case here.

Yeah, that's exactly my expectation.  If I thought we were only going to
have five uses of condition variables forevermore, I wouldn't be putting
much time into them.

> I think an extremely *likely* programming error when programming with
> CVs is to have a code path where ConditionVariableCancelSleep() does
> not get called.  The proposed change could make such mistakes much
> less noticeable.

Actually, the proposed change would turn it into barely an error at all.
The only advantage of cancelling a sleep immediately is that you avoid
possibly getting a no-longer-useful latch event later.

			regards, tom lane

Commits

  1. Add WL_EXIT_ON_PM_DEATH pseudo-event.

  2. While waiting for a condition variable, detect postmaster death.

  3. Fix race condition during replication origin drop.

  4. Allow ConditionVariable[PrepareTo]Sleep to auto-switch between CVs.

  5. Cosmetic improvements in condition_variable.[hc].

  6. Improve error detection capability in proclists.

  7. Remove return values of ConditionVariableSignal/Broadcast.

  8. Reorder steps in ConditionVariablePrepareToSleep for more safety.

  9. Rewrite ConditionVariableBroadcast() to avoid live-lock.

  10. Add parallel-aware hash joins.