From 1ba60808a23159b8d99cfec70111b6724a55e57b Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 12 Apr 2022 07:33:59 +1200 Subject: [PATCH] Fix recovery conflict SIGUSR1 handling. We shouldn't be doing real work in a signal handler. Move the check into the next CFI. (There's a related problem in the recovery conflict SIGALRM handling, for another patch.) Discussion: https://postgr.es/m/CA%2BhUKGK3PGKwcKqzoosamn36YW-fsuTdOPPF1i_rtEO%3DnEYKSg%40mail.gmail.com --- src/backend/storage/ipc/procsignal.c | 12 +++---- src/backend/tcop/postgres.c | 53 ++++++++++++++++++---------- src/include/storage/procsignal.h | 4 ++- src/include/tcop/tcopprot.h | 3 +- 4 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c index f41563a0a4..ce08580b5b 100644 --- a/src/backend/storage/ipc/procsignal.c +++ b/src/backend/storage/ipc/procsignal.c @@ -653,22 +653,22 @@ procsignal_sigusr1_handler(SIGNAL_ARGS) HandleLogMemoryContextInterrupt(); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_DATABASE)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_TABLESPACE)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_TABLESPACE); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_TABLESPACE); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_LOCK)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOCK); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_LOCK); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_SNAPSHOT); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK); if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN)) - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN); + HandleRecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN); SetLatch(MyLatch); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 95dc2e2c83..a89066badb 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -172,6 +172,7 @@ static bool EchoQuery = false; /* -E switch */ static bool UseSemiNewlineNewline = false; /* -j switch */ /* whether or not, and why, we were canceled by conflict with recovery */ +volatile sig_atomic_t RecoveryConflictInterruptPending[NUM_PROCSIGNALS]; static bool RecoveryConflictPending = false; static bool RecoveryConflictRetryable = true; static ProcSignalReason RecoveryConflictReason; @@ -2993,22 +2994,31 @@ FloatExceptionHandler(SIGNAL_ARGS) } /* - * RecoveryConflictInterrupt: out-of-line portion of recovery conflict - * handling following receipt of SIGUSR1. Designed to be similar to die() - * and StatementCancelHandler(). Called only by a normal user backend - * that begins a transaction during recovery. + * Tell the next CHECK_FOR_INTERRUPTS() to check for a particular type of + * recovery conflict. Runs in a SIGUSR1 handler. */ void -RecoveryConflictInterrupt(ProcSignalReason reason) +HandleRecoveryConflictInterrupt(ProcSignalReason reason) { - int save_errno = errno; + RecoveryConflictInterruptPending[reason] = true; + InterruptPending = true; + /* latch will be set by procsignal_sigusr1_handler */ +} +/* + * Check one recovery conflict reason. This is called when the corresponding + * RecoveryConflictInterruptPending flags is set. If we decide that a conflict + * exists, then RecoveryConflictReason and RecoveryConflictPending will be set, + * to be handled later in the same invocation of ProcessInterrupts(). + */ +static void +ProcessRecoveryConflictInterrupt(ProcSignalReason reason) +{ /* * Don't joggle the elbow of proc_exit */ if (!proc_exit_inprogress) { - RecoveryConflictReason = reason; switch (reason) { case PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK: @@ -3084,9 +3094,9 @@ RecoveryConflictInterrupt(ProcSignalReason reason) if (IsAbortedTransactionBlockState()) return; + RecoveryConflictReason = reason; RecoveryConflictPending = true; QueryCancelPending = true; - InterruptPending = true; break; } @@ -3094,9 +3104,9 @@ RecoveryConflictInterrupt(ProcSignalReason reason) /* FALLTHROUGH */ case PROCSIG_RECOVERY_CONFLICT_DATABASE: + RecoveryConflictReason = reason; RecoveryConflictPending = true; ProcDiePending = true; - InterruptPending = true; break; default: @@ -3115,15 +3125,6 @@ RecoveryConflictInterrupt(ProcSignalReason reason) if (reason == PROCSIG_RECOVERY_CONFLICT_DATABASE) RecoveryConflictRetryable = false; } - - /* - * Set the process latch. This function essentially emulates signal - * handlers like die() and StatementCancelHandler() and it seems prudent - * to behave similarly as they do. - */ - SetLatch(MyLatch); - - errno = save_errno; } /* @@ -3147,6 +3148,22 @@ ProcessInterrupts(void) return; InterruptPending = false; + /* + * Have we been asked to check for a recovery conflict? Processing these + * interrupts may result in RecoveryConflictPending and related variables + * being set, to be handled further down. + */ + for (int i = PROCSIG_RECOVERY_CONFLICT_FIRST; + i <= PROCSIG_RECOVERY_CONFLICT_LAST; + ++i) + { + if (RecoveryConflictInterruptPending[i]) + { + RecoveryConflictInterruptPending[i] = false; + ProcessRecoveryConflictInterrupt(i); + } + } + if (ProcDiePending) { ProcDiePending = false; diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h index ee636900f3..26d045950c 100644 --- a/src/include/storage/procsignal.h +++ b/src/include/storage/procsignal.h @@ -37,12 +37,14 @@ typedef enum PROCSIG_LOG_MEMORY_CONTEXT, /* ask backend to log the memory contexts */ /* Recovery conflict reasons */ - PROCSIG_RECOVERY_CONFLICT_DATABASE, + PROCSIG_RECOVERY_CONFLICT_FIRST, + PROCSIG_RECOVERY_CONFLICT_DATABASE = PROCSIG_RECOVERY_CONFLICT_FIRST, PROCSIG_RECOVERY_CONFLICT_TABLESPACE, PROCSIG_RECOVERY_CONFLICT_LOCK, PROCSIG_RECOVERY_CONFLICT_SNAPSHOT, PROCSIG_RECOVERY_CONFLICT_BUFFERPIN, PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK, + PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK, NUM_PROCSIGNALS /* Must be last! */ } ProcSignalReason; diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h index 87e408b719..6c3c91aeea 100644 --- a/src/include/tcop/tcopprot.h +++ b/src/include/tcop/tcopprot.h @@ -73,8 +73,7 @@ extern void die(SIGNAL_ARGS); extern void quickdie(SIGNAL_ARGS) pg_attribute_noreturn(); extern void StatementCancelHandler(SIGNAL_ARGS); extern void FloatExceptionHandler(SIGNAL_ARGS) pg_attribute_noreturn(); -extern void RecoveryConflictInterrupt(ProcSignalReason reason); /* called from SIGUSR1 - * handler */ +extern void HandleRecoveryConflictInterrupt(ProcSignalReason reason); extern void ProcessClientReadInterrupt(bool blocked); extern void ProcessClientWriteInterrupt(bool blocked); -- 2.30.2