Re: Broken order-of-operations in parallel query latch manipulation

On Mon, Aug 1, 2016 at 8:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>> On Mon, Aug 1, 2016 at 1:58 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>

Right and Thanks for the detailed explanation.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com