Remove set_latch_on_sigusr1 flag.

This flag has proven to be a recipe for bugs, and it doesn't seem like
it can really buy anything in terms of performance.  So let's just
*always* set the process latch when we receive SIGUSR1 instead of
trying to do it only when needed.

Per my recent proposal on pgsql-hackers.

diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 68c9505809eaea221efa767198a03f078bae80db..c38d486a20e253cbc1360cdb800da5c61da999d4 100644 (file)
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -954,45 +954,31 @@ WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
 {
        BgwHandleStatus status;
        int                     rc;
-       bool            save_set_latch_on_sigusr1;
 
-       save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
-       set_latch_on_sigusr1 = true;
-
-       PG_TRY();
+       for (;;)
        {
-               for (;;)
-               {
-                       pid_t           pid;
+               pid_t           pid;
 
-                       CHECK_FOR_INTERRUPTS();
+               CHECK_FOR_INTERRUPTS();
 
-                       status = GetBackgroundWorkerPid(handle, &pid);
-                       if (status == BGWH_STARTED)
-                               *pidp = pid;
-                       if (status != BGWH_NOT_YET_STARTED)
-                               break;
-
-                       rc = WaitLatch(MyLatch,
-                                                  WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
+               status = GetBackgroundWorkerPid(handle, &pid);
+               if (status == BGWH_STARTED)
+                       *pidp = pid;
+               if (status != BGWH_NOT_YET_STARTED)
+                       break;
 
-                       if (rc & WL_POSTMASTER_DEATH)
-                       {
-                               status = BGWH_POSTMASTER_DIED;
-                               break;
-                       }
+               rc = WaitLatch(MyLatch,
+                                          WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
 
-                       ResetLatch(MyLatch);
+               if (rc & WL_POSTMASTER_DEATH)
+               {
+                       status = BGWH_POSTMASTER_DIED;
+                       break;
                }
+
+               ResetLatch(MyLatch);
        }
-       PG_CATCH();
-       {
-               set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
 
-       set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
        return status;
 }
 
@@ -1009,40 +995,26 @@ WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)
 {
        BgwHandleStatus status;
        int                     rc;
-       bool            save_set_latch_on_sigusr1;
-
-       save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
-       set_latch_on_sigusr1 = true;
 
-       PG_TRY();
+       for (;;)
        {
-               for (;;)
-               {
-                       pid_t           pid;
+               pid_t           pid;
 
-                       CHECK_FOR_INTERRUPTS();
+               CHECK_FOR_INTERRUPTS();
 
-                       status = GetBackgroundWorkerPid(handle, &pid);
-                       if (status == BGWH_STOPPED)
-                               return status;
+               status = GetBackgroundWorkerPid(handle, &pid);
+               if (status == BGWH_STOPPED)
+                       return status;
 
-                       rc = WaitLatch(&MyProc->procLatch,
-                                                  WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
+               rc = WaitLatch(&MyProc->procLatch,
+                                          WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
 
-                       if (rc & WL_POSTMASTER_DEATH)
-                               return BGWH_POSTMASTER_DIED;
+               if (rc & WL_POSTMASTER_DEATH)
+                       return BGWH_POSTMASTER_DIED;
 
-                       ResetLatch(&MyProc->procLatch);
-               }
-       }
-       PG_CATCH();
-       {
-               set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
-               PG_RE_THROW();
+               ResetLatch(&MyProc->procLatch);
        }
-       PG_END_TRY();
 
-       set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
        return status;
 }
 

diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 0abde43565bcc631e12eaf2694671dbe7cbafcfb..acfb4e9be82bf5d70f4031e5e7746ac843736844 100644 (file)
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -59,14 +59,6 @@ typedef struct
  */
 #define NumProcSignalSlots     (MaxBackends + NUM_AUXPROCTYPES)
 
-/*
- * If this flag is set, the process latch will be set whenever SIGUSR1
- * is received.  This is useful when waiting for a signal from the postmaster.
- * Spurious wakeups must be expected.  Make sure that the flag is cleared
- * in the error path.
- */
-bool           set_latch_on_sigusr1;
-
 static ProcSignalSlot *ProcSignalSlots = NULL;
 static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
 
@@ -296,8 +288,7 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
        if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
                RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
 
-       if (set_latch_on_sigusr1)
-               SetLatch(MyLatch);
+       SetLatch(MyLatch);
 
        latch_sigusr1_handler();
 

diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index c78f1650e6af4eaf741be75448766ce7a09ec03c..80956ce4304ae7db2778876a958922f2aeb9b6af 100644 (file)
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -962,63 +962,49 @@ static bool
 shm_mq_wait_internal(volatile shm_mq *mq, PGPROC *volatile * ptr,
                                         BackgroundWorkerHandle *handle)
 {
-       bool            save_set_latch_on_sigusr1;
        bool            result = false;
 
-       save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
-       if (handle != NULL)
-               set_latch_on_sigusr1 = true;
-
-       PG_TRY();
+       for (;;)
        {
-               for (;;)
+               BgwHandleStatus status;
+               pid_t           pid;
+               bool            detached;
+
+               /* Acquire the lock just long enough to check the pointer. */
+               SpinLockAcquire(&mq->mq_mutex);
+               detached = mq->mq_detached;
+               result = (*ptr != NULL);
+               SpinLockRelease(&mq->mq_mutex);
+
+               /* Fail if detached; else succeed if initialized. */
+               if (detached)
+               {
+                       result = false;
+                       break;
+               }
+               if (result)
+                       break;
+
+               if (handle != NULL)
                {
-                       BgwHandleStatus status;
-                       pid_t           pid;
-                       bool            detached;
-
-                       /* Acquire the lock just long enough to check the pointer. */
-                       SpinLockAcquire(&mq->mq_mutex);
-                       detached = mq->mq_detached;
-                       result = (*ptr != NULL);
-                       SpinLockRelease(&mq->mq_mutex);
-
-                       /* Fail if detached; else succeed if initialized. */
-                       if (detached)
+                       /* Check for unexpected worker death. */
+                       status = GetBackgroundWorkerPid(handle, &pid);
+                       if (status != BGWH_STARTED && status != BGWH_NOT_YET_STARTED)
                        {
                                result = false;
                                break;
                        }
-                       if (result)
-                               break;
-
-                       if (handle != NULL)
-                       {
-                               /* Check for unexpected worker death. */
-                               status = GetBackgroundWorkerPid(handle, &pid);
-                               if (status != BGWH_STARTED && status != BGWH_NOT_YET_STARTED)
-                               {
-                                       result = false;
-                                       break;
-                               }
-                       }
+               }
 
-                       /* Wait to be signalled. */
-                       WaitLatch(MyLatch, WL_LATCH_SET, 0);
+               /* Wait to be signalled. */
+               WaitLatch(MyLatch, WL_LATCH_SET, 0);
 
-                       /* An interrupt may have occurred while we were waiting. */
-                       CHECK_FOR_INTERRUPTS();
+               /* An interrupt may have occurred while we were waiting. */
+               CHECK_FOR_INTERRUPTS();
 
-                       /* Reset the latch so we don't spin. */
-                       ResetLatch(MyLatch);
-               }
-       }
-       PG_CATCH();
-       {
-               set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
-               PG_RE_THROW();
+               /* Reset the latch so we don't spin. */
+               ResetLatch(MyLatch);
        }
-       PG_END_TRY();
 
        return result;
 }

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index aee13aec757b87aaba93c043ebb7528fb5a66d8a..d30fe35c14fcc00176a8c5594f98537807c7583a 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2825,9 +2825,7 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
        /*
         * Set the process latch. This function essentially emulates signal
         * handlers like die() and StatementCancelHandler() and it seems prudent
-        * to behave similarly as they do. Alternatively all plain backend code
-        * waiting on that latch, expecting to get interrupted by query cancels et
-        * al., would also need to set set_latch_on_sigusr1.
+        * to behave similarly as they do.
         */
        SetLatch(MyLatch);
 

diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index af1a0cd71f25b900ed9187b5007203142b858a1d..50ffb13e1f2b7bfcc70aec16a7cbf4b80b23bab8 100644 (file)
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -55,6 +55,5 @@ extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
                           BackendId backendId);
 
 extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
-extern PGDLLIMPORT bool set_latch_on_sigusr1;
 
 #endif   /* PROCSIGNAL_H */

diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 7f2f5fd3ff72efb784c5c399fb43b17ed889465b..dedd72f88385f4f08eb8b6bb82f424750ffa6258 100644 (file)
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -255,51 +255,38 @@ static void
 wait_for_workers_to_become_ready(worker_state *wstate,
                                                                 volatile test_shm_mq_header *hdr)
 {
-       bool            save_set_latch_on_sigusr1;
        bool            result = false;
 
-       save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
-       set_latch_on_sigusr1 = true;
-
-       PG_TRY();
+       for (;;)
        {
-               for (;;)
+               int                     workers_ready;
+
+               /* If all the workers are ready, we have succeeded. */
+               SpinLockAcquire(&hdr->mutex);
+               workers_ready = hdr->workers_ready;
+               SpinLockRelease(&hdr->mutex);
+               if (workers_ready >= wstate->nworkers)
                {
-                       int                     workers_ready;
-
-                       /* If all the workers are ready, we have succeeded. */
-                       SpinLockAcquire(&hdr->mutex);
-                       workers_ready = hdr->workers_ready;
-                       SpinLockRelease(&hdr->mutex);
-                       if (workers_ready >= wstate->nworkers)
-                       {
-                               result = true;
-                               break;
-                       }
-
-                       /* If any workers (or the postmaster) have died, we have failed. */
-                       if (!check_worker_status(wstate))
-                       {
-                               result = false;
-                               break;
-                       }
-
-                       /* 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);
+                       result = true;
+                       break;
                }
+
+               /* If any workers (or the postmaster) have died, we have failed. */
+               if (!check_worker_status(wstate))
+               {
+                       result = false;
+                       break;
+               }
+
+               /* 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);
        }
-       PG_CATCH();
-       {
-               set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
-               PG_RE_THROW();
-       }
-       PG_END_TRY();
 
        if (!result)
                ereport(ERROR,