On Fri, Jul 5, 2019 at 1:40 PM Thomas Munro <thomas.mu...@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
