Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Aug 1, 2016 at 1:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I believe this is wrong and the CHECK_FOR_INTERRUPTS needs to be before
>> or after the two latch operations. As-is, if the reason somebody set
>> our latch was to get us to notice that a CHECK_FOR_INTERRUPTS condition
>> happened, there's a race condition where we'd fail to realize that.
> I could see that in nodeGather.c, it might fail to notice the SetLatch
> done by worker process or spuriously woken up due to SetLatch for some
> unrelated reason. However, I don't see what problem it can cause
> apart from one extra loop cycle where it will try to process the tuple
> when actually there is no tuple in the queue.
Consider the following sequence of events:
1. gather_readnext reaches the WaitLatch, and is allowed to pass through
it for some unrelated reason, perhaps some long-since-handled SIGUSR1
from a worker process.
2. gather_readnext does CHECK_FOR_INTERRUPTS(), and sees nothing pending.
3. A SIGINT is received. StatementCancelHandler sets QueryCancelPending
and does SetLatch(MyLatch).
4. gather_readnext does ResetLatch(MyLatch).
5. gather_readnext runs through its loop again, finds nothing to do, and
reaches the WaitLatch.
6. The process is now sleeping on its latch, and might sit there a long
time before noticing the pending query cancel.
Obviously the window for this race condition is pretty tight --- there's
not many instructions between steps 2 and 4. But it can happen. If
memory serves, we've had actual field reports for race condition bugs
where the window that was being hit wasn't much more than a single
instruction.
Also, it's entirely possible that the bug could be masked, if there was
another CHECK_FOR_INTERRUPTS lurking anywhere in the code called within
the loop. That doesn't excuse this coding practice, though.
BTW, now that I look at it, CHECK_FOR_INTERRUPTS subsumes
HandleParallelMessages(), which means the direct call to the latter
at the top of gather_readnext's loop is pretty bogus. I now think
the right fix in gather_readnext is to move the CHECK_FOR_INTERRUPTS
macro to the top of the loop, replacing that call. The places in
shm_mq.c that have this issue should probably look like
ProcWaitForSignal, though.
regards, tom lane
