From db0f6cad4884bd4c835156d3a720d9a79dbd63a9 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 9 Oct 2015 14:31:04 -0400 Subject: [PATCH] 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. --- src/backend/postmaster/bgworker.c | 82 +++++++++------------------- src/backend/storage/ipc/procsignal.c | 11 +--- src/backend/storage/ipc/shm_mq.c | 76 +++++++++++--------------- src/backend/tcop/postgres.c | 4 +- src/include/storage/procsignal.h | 1 - src/test/modules/test_shm_mq/setup.c | 65 +++++++++------------- 6 files changed, 86 insertions(+), 153 deletions(-) diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index 68c9505809..c38d486a20 100644 --- 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 0abde43565..acfb4e9be8 100644 --- 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 c78f1650e6..80956ce430 100644 --- 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 aee13aec75..d30fe35c14 100644 --- 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 af1a0cd71f..50ffb13e1f 100644 --- 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 7f2f5fd3ff..dedd72f883 100644 --- 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, -- 2.40.0