Re: Condition variable live lock

On Sun, Jan 7, 2018 at 10:00 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

+1

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

+1

> 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?

I liked your follow-up idea better (see below).

> 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.

Yeah. As far as I know so far, every place where we wait on a
WaitEventSet falls into one of 4 categories when it comes to
postmaster death:

1. We proc_exit(1) if WL_POSTMASTER_DEATH is returned.
2. We ereport(FATAL) if WL_POSTMASTER_DEATH is returned.
3. We asked for WL_POSTMASTER_DEATH in the WaitEventSet, but we don't
actually bother checking for it in the returned value. Instead we
call PostmasterIsAlive() every time through the loop (pgarch.c,
syncrep.c, walsender.c and walreceiver.c).
4. We didn't ask for WL_POSTMASTER_DEATH.

Do I have that right? I guess category 3 is suboptimal especially on
some clunky but loveable kernels[1] and all cases of 4 including this
one are probably bugs. That makes me wonder why we don't change the
WaitEventSet API so that it calls proc_exit(1) for you by default if
you didn't ask to receive WL_POSTMASTER_DEATH explicitly, to handle
category 1 for free. If you asked for WL_POSTMASTER_DEATH explicitly
then we could return it, to support category 2 callers that want to do
something different. Just a thought.

Another possibility for this particular case would be that the client
of ConditionVariable would like to be able to chose how to handle
postmaster death, and in turn the client of Barrier (a client) might
like to be able to choose too. But in every case I'm aware of today
proc_exit(1) is the right thing to do, so teaching
ConditionVariableSleep() to do that seems OK to me.

On Sun, Jan 7, 2018 at 10:43 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Actually ... perhaps a better design would be to have
> ConditionVariable[PrepareTo]Sleep auto-cancel any prepared sleep for
> a different condition variable, analogously to what we just did in
> ConditionVariableBroadcast, on the same theory that whenever control
> returns to the other CV wait loop it can re-establish the relevant
> state easily enough. I have to think that if the use of CVs grows
> much, the existing restriction is going to become untenable anyway,
> so why not just get rid of it?

+1

It's a more robust API this way.

On Mon, Jan 8, 2018 at 3:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Thanks. BTW, I realized that there is a second (and perhaps more
> important) reason why we can only prepare one CV sleep at a time:
> we only have one cvWaitLink in our PGPROC. So I'm now inclined
> to word the revised comment in ConditionVariablePrepareToSleep as
>
> /*
> * If some other sleep is already prepared, cancel it; this is necessary
> * because we have just one static variable tracking the prepared sleep,
> * and also only one cvWaitLink in our PGPROC. It's okay to do this
> * because whenever control does return to the other test-and-sleep loop,
> * its ConditionVariableSleep call will just re-establish that sleep as
> * the prepared one.
> */

+1

[1] https://www.postgresql.org/message-id/flat/CAEepm%3D0qQ6DO-u%3D25ny5EJAUbWeHbAQgJj1UJFAL1NWJNxC%2Bgg%40mail.gmail.com

--
Thomas Munro
http://www.enterprisedb.com