On Mon, Aug 1, 2016 at 8:45 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Amit Kapila <amit.kapil...@gmail.com> writes: >> On Mon, Aug 1, 2016 at 1:58 AM, Tom Lane <t...@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. >
Right and Thanks for the detailed explanation. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
