On Fri, Jul 5, 2019 at 1:40 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> I think this is looking pretty good and I'm planning to commit it
> soon. The convention for CHECK_FOR_INTERRUPTS() in latch wait loops
> seems to be to put it after the ResetLatch(), so I've moved it in the
> attached version (though I don't think it was wrong where it was).
> Also pgindented and with my proposed commit message. I've also
> attached a throw-away test module that gives you CALL poke() and
> SELECT wait_for_poke(timeout) using a CV.
I thought of one small problem with the current coding. Suppose there
are two processes A and B waiting on a CV, and another process C calls
ConditionVariableSignal() to signal one process. Around the same
time, A times out and exits via this code path:
+ /* Timed out */
+ if (rc == 0)
+ return true;
Suppose ConditionVariableSignal() set A's latch immediately after
WaitEventSetWait() returned 0 in A. Now A won't report the CV signal
to the caller, and B is still waiting, so effectively nobody has
received the message and yet C thinks it has signalled a waiter if
there is one. My first thought is that we could simply remove the
above-quoted hunk and fall through to the second timeout-detecting
code. That'd mean that if we've been signalled AND timed out as of
that point in the code, we'll prefer to report the signal, and it also
reduces the complexity of the function to have only one "return true"
path.
That still leaves the danger that the CV can be signalled some time
after ConditionVariableTimedSleep() returns. So now I'm wondering if
ConditionVariableCancelSleep() should signal the CV if it discovers
that this process is not in the proclist, on the basis that that must
indicate that we've been signalled even though we're not interested in
the message anymore, and yet some other process else might be
interested, and that might have been the only signal that is ever
going to be delivered by ConditionVariableSignal().
--
Thomas Munro
https://enterprisedb.com
