Don't CHECK_FOR_INTERRUPTS between WaitLatch and ResetLatch.

This coding pattern creates a race condition, because if an interesting
interrupt happens after we've checked InterruptPending but before we reset
our latch, the latch-setting done by the signal handler would get lost,
and then we might block at WaitLatch in the next iteration without ever
noticing the interrupt condition.  You can put the CHECK_FOR_INTERRUPTS
before WaitLatch or after ResetLatch, but not between them.

Aside from fixing the bugs, add some explanatory comments to latch.h
to perhaps forestall the next person from making the same mistake.

In HEAD, also replace gather_readnext's direct call of
HandleParallelMessages with CHECK_FOR_INTERRUPTS.  It does not seem clean
or useful for this one caller to bypass ProcessInterrupts and go straight
to HandleParallelMessages; not least because that fails to consider the
InterruptPending flag, resulting in useless work both here
(if InterruptPending isn't set) and in the next CHECK_FOR_INTERRUPTS call
(if it is).

This thinko seems to have been introduced in the initial coding of
storage/ipc/shm_mq.c (commit ec9037df2), and then blindly copied into all
the subsequent parallel-query support logic.  Back-patch relevant hunks
to 9.4 to extirpate the error everywhere.

Discussion: <1661.1469996911@sss.pgh.pa.us>

diff --git a/src/backend/libpq/pqmq.c b/src/backend/libpq/pqmq.c
index 0a3c2b70cbe46f68f3ad076260007a2e9db48f62..549e76dfa0674cbad995a11fa43f61f80fd0dec0 100644 (file)
--- a/src/backend/libpq/pqmq.c
+++ b/src/backend/libpq/pqmq.c
@@ -172,8 +172,8 @@ mq_putmessage(char msgtype, const char *s, size_t len)
            break;
 
        WaitLatch(&MyProc->procLatch, WL_LATCH_SET, 0);
-       CHECK_FOR_INTERRUPTS();
        ResetLatch(&MyProc->procLatch);
+       CHECK_FOR_INTERRUPTS();
    }
 
    pq_mq_busy = false;

diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index f88e624a4131e158f37b3c80d6fdb8dd0ace4d27..65348dca97b18f2f8c9bf9dca50a40c4886c0553 100644 (file)
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -868,11 +868,11 @@ shm_mq_send_bytes(shm_mq_handle *mqh, Size nbytes, const void *data,
             */
            WaitLatch(MyLatch, WL_LATCH_SET, 0);
 
-           /* An interrupt may have occurred while we were waiting. */
-           CHECK_FOR_INTERRUPTS();
-
            /* Reset the latch so we don't spin. */
            ResetLatch(MyLatch);
+
+           /* An interrupt may have occurred while we were waiting. */
+           CHECK_FOR_INTERRUPTS();
        }
        else
        {
@@ -965,11 +965,11 @@ shm_mq_receive_bytes(shm_mq *mq, Size bytes_needed, bool nowait,
         */
        WaitLatch(MyLatch, WL_LATCH_SET, 0);
 
-       /* An interrupt may have occurred while we were waiting. */
-       CHECK_FOR_INTERRUPTS();
-
        /* Reset the latch so we don't spin. */
        ResetLatch(MyLatch);
+
+       /* An interrupt may have occurred while we were waiting. */
+       CHECK_FOR_INTERRUPTS();
    }
 }
 
@@ -1071,11 +1071,11 @@ shm_mq_wait_internal(volatile shm_mq *mq, PGPROC *volatile * ptr,
            /* Wait to be signalled. */
            WaitLatch(MyLatch, WL_LATCH_SET, 0);
 
-           /* An interrupt may have occurred while we were waiting. */
-           CHECK_FOR_INTERRUPTS();
-
            /* Reset the latch so we don't spin. */
            ResetLatch(MyLatch);
+
+           /* An interrupt may have occurred while we were waiting. */
+           CHECK_FOR_INTERRUPTS();
        }
    }
    PG_CATCH();

diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 28fc684d240e0ea0e3206b02c09b6496065adfc0..273836d5530c343faa3603844b36096120771f3b 100644 (file)
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -52,6 +52,22 @@
  * do. Otherwise, if someone sets the latch between the check and the
  * ResetLatch call, you will miss it and Wait will incorrectly block.
  *
+ * Another valid coding pattern looks like:
+ *
+ * for (;;)
+ * {
+ *    if (work to do)
+ *        Do Stuff(); // in particular, exit loop if some condition satisfied
+ *    WaitLatch();
+ *    ResetLatch();
+ * }
+ *
+ * This is useful to reduce latch traffic if it's expected that the loop's
+ * termination condition will often be satisfied in the first iteration;
+ * the cost is an extra loop iteration before blocking when it is not.
+ * What must be avoided is placing any checks for asynchronous events after
+ * WaitLatch and before ResetLatch, as that creates a race condition.
+ *
  * To wake up the waiter, you must first set a global flag or something
  * else that the wait loop tests in the "if (work to do)" part, and call
  * SetLatch *after* that. SetLatch is designed to return quickly if the

diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 7f2f5fd3ff72efb784c5c399fb43b17ed889465b..061af8b4676a2a15220560b2b51055e1cae09c26 100644 (file)
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -287,11 +287,11 @@ wait_for_workers_to_become_ready(worker_state *wstate,
            /* Wait to be signalled. */
            WaitLatch(MyLatch, WL_LATCH_SET, 0);
 
-           /* An interrupt may have occurred while we were waiting. */
-           CHECK_FOR_INTERRUPTS();
-
            /* Reset the latch so we don't spin. */
            ResetLatch(MyLatch);
+
+           /* An interrupt may have occurred while we were waiting. */
+           CHECK_FOR_INTERRUPTS();
        }
    }
    PG_CATCH();

diff --git a/src/test/modules/test_shm_mq/test.c b/src/test/modules/test_shm_mq/test.c
index 732376daff26f30543ad0a17721321f78fc7a24a..b36b4bf129347860049012ca78f36f5b386f275c 100644 (file)
--- a/src/test/modules/test_shm_mq/test.c
+++ b/src/test/modules/test_shm_mq/test.c
@@ -231,8 +231,8 @@ test_shm_mq_pipelined(PG_FUNCTION_ARGS)
             * for us to do.
             */
            WaitLatch(MyLatch, WL_LATCH_SET, 0);
-           CHECK_FOR_INTERRUPTS();
            ResetLatch(MyLatch);
+           CHECK_FOR_INTERRUPTS();
        }
    }