On Tue, Oct 4, 2016 at 3:12 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Mon, Aug 15, 2016 at 5:58 PM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> Please find attached a -v2 of your patch which includes suggestions
>> 1-3 above.
>
> Here's a rebased patch. ConditionVariableSleep now takes
> wait_event_info. Anyone using this in patches for core probably needs
> to add enumerators to the WaitEventXXX enums in pgstat.h to describe
> their wait points.
So, in my original patch, it was intended that cvSleeping was the
definitive source of truth as to whether a particular backend is
committed to sleeping or not. That had some bugs, I guess, but in
your version, there are now two sources of truth. On the one hand,
there's still cvSleeping. On the other hand, there's now also whether
proclist_contains(&cv->wakeup, MyProc->pgprocno, cvWaitLink) returns
true.
I'm not sure whether that causes any serious problem. It seems
possible, for example, that ConditionVariableSignal() could clear the
cvSleeping flag for a backend that's now waiting for some OTHER
condition variable, because once it pops the victim off the list and
releases the spinlock, the other backend could meanwhile
ConditionVariableCancelSleep() and then do anything it likes,
including sleep on some other condition variable. Now I think that
the worst thing that will happen is that the other backend will
receive a spurious wakeup, but I'm not quite sure. The whole point of
having cvSleeping in the first place is so that we don't get spurious
wakeups just because somebody happens to set your process latch, so it
seems a bit unfortunate that it doesn't actually prevent that from
happening.
I wonder if we shouldn't try to create the invariant that when the CV
mutex is not help, the state of cvSleeping has to be true if we're in
the proclist and false if we're not. So ConditionVariableSignal()
would clear the flag before releasing the spinlock, for example. Then
I think we wouldn't need proclist_contains().
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
