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-06T21:00:24Z
Lists: pgsql-hackers

Attachments

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> No opinion without seeing what you propose to change.

> OK, will put out a proposal.

I began with the intention of making no non-cosmetic changes, but then
I started to wonder why ConditionVariablePrepareToSleep bothers with a
!proclist_contains test, when the calling process surely ought not be
in the list -- or any other list -- since it wasn't prepared to sleep.
And that led me down a different rabbit hole ending in the conclusion
that proclist.h could stand some improvements too.  I do not like the
fact that it's impossible to tell whether a proclist_node is in any
proclist or not.  Initially, a proclist_node contains zeroes which is
a distinguishable state, but proclist_delete_offset resets it to
next = prev = INVALID_PGPROCNO which looks the same as a node that's in a
singleton list.  We should have it reset to the initial state of zeroes
instead, and then we can add assertions to proclist_push_xxx that the
supplied node is not already in a list.  Hence, I propose the first
attached patch which tightens things up in proclist.h and then removes
the !proclist_contains test in ConditionVariablePrepareToSleep; the
assertion in proclist_push_tail supersedes that.

The second attached patch is the cosmetic changes I want to make in
condition_variable.c/.h.

I still think that we ought to change the Asserts on cv_sleep_target in
ConditionVariablePrepareToSleep and ConditionVariableSleep to be full
test-and-elog tests.  Those are checking a global correctness property
("global" meaning "interactions between completely unrelated modules can
break this"), and they'd be extremely cheap compared to the rest of what
those functions are doing, so I think insisting that they be Asserts is
penny wise and pound foolish.  Anybody besides Robert want to vote on
that?

Another loose end that I'm seeing here is that while a process waiting on
a condition variable will respond to a cancel or die interrupt, it will
not notice postmaster death.  This seems unwise to me.  I think we should
adjust the WaitLatch call to include WL_POSTMASTER_DEATH as a wake
condition and just do a summary proc_exit(1) if it sees that.  I'd even
argue that that is a back-patchable bug fix.

			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.