On Tue, Apr 12, 2022 at 10:50 AM Andres Freund <and...@anarazel.de> wrote:
> On 2022-04-12 10:33:28 +1200, Thomas Munro wrote:
> > Instead of bothering to create N different XXXPending variables for
> > the different conflict "reasons", I used an array. Other than that,
> > it's much like existing examples.
>
> It kind of bothers me that each pending conflict has its own external function
> call. It doesn't actually cost anything, because it's quite unlikely that
> there's more than one pending conflict. Besides aesthetically displeasing,
> it also leads to an unnecessarily large amount of code needed, because the
> calls to RecoveryConflictInterrupt() can't be merged...
Ok, in this version there's two levels of flag:
RecoveryConflictPending, so we do nothing if that's not set, and then
the loop over RecoveryConflictPendingReasons is moved into
ProcessRecoveryConflictInterrupts(). Better?
> What might actually make more sense is to just have a bitmask or something?
Yeah, in fact I'm exploring something like that in later bigger
redesign work[1] that gets rid of signal handlers. Here I'm looking
for something simple and potentially back-patchable and I don't want
to have to think about async signal safety of bit-level manipulations.
> It's pretty weird that we have all this stuff that we then just check a short
> while later in ProcessInterrupts() whether they've been set.
>
> Seems like it'd make more sense to throw the error in
> ProcessRecoveryConflictInterrupt(), now that it's not in a a signal handler
> anymore?
Yeah. The thing that was putting me off doing that (and caused me to
get kinda stuck in the valley of indecision for a while here, sorry
about that) aside from trying to keep the diff small, was the need to
replicate this self-loathing code in a second place:
if (QueryCancelPending && QueryCancelHoldoffCount != 0)
{
/*
* Re-arm InterruptPending so that we process the cancel request as
* soon as we're done reading the message. (XXX this is seriously
* ugly: it complicates INTERRUPTS_CAN_BE_PROCESSED(), and it means we
* can't use that macro directly as the initial test in this function,
* meaning that this code also creates opportunities for other bugs to
* appear.)
*/
But I have now tried doing that anyway, and I hope the simplification
in other ways makes it worth it. Thoughts, objections?
> > /*
> > @@ -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);
> > + }
> > + }
>
> Hm. This seems like it shouldn't be in ProcessInterrupts(). How about checking
> calling a wrapper doing all this if RecoveryConflictPending?
I moved the loop into ProcessRecoveryConflictInterrupt() and added an
"s" to the latter's name. It already had the right indentation level
to contain a loop, once I realised that the test of
proc_exit_inprogress must be redundant.
Better?
[1]
https://www.postgresql.org/message-id/flat/CA%2BhUKG%2B3MkS21yK4jL4cgZywdnnGKiBg0jatoV6kzaniBmcqbQ%40mail.gmail.com
From fc9b7c1c68404eede7161615e6a7b5ac2155d0ba Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 10 May 2022 16:00:23 +1200
Subject: [PATCH v2] Fix recovery conflict SIGUSR1 handling.
We shouldn't be doing real work in a signal handler, to avoid reaching
code that is not safe in that context. Move the check into the next
CFI.
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 | 191 ++++++++++++++-------------
src/include/storage/procsignal.h | 4 +-
src/include/tcop/tcopprot.h | 3 +-
4 files changed, 107 insertions(+), 103 deletions(-)
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 00d66902d8..1268eeba7c 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -644,22 +644,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 304cce135a..324ca816e1 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -171,10 +171,9 @@ static const char *userDoption = NULL; /* -D switch */
static bool EchoQuery = false; /* -E switch */
static bool UseSemiNewlineNewline = false; /* -j switch */
-/* whether or not, and why, we were canceled by conflict with recovery */
-static bool RecoveryConflictPending = false;
-static bool RecoveryConflictRetryable = true;
-static ProcSignalReason RecoveryConflictReason;
+/* whether or not, and why, we were cancelled by conflict with recovery */
+static volatile sig_atomic_t RecoveryConflictPending = false;
+static volatile sig_atomic_t RecoveryConflictPendingReasons[NUM_PROCSIGNALS];
/* reused buffer to pass to SendRowDescriptionMessage() */
static MemoryContext row_description_context = NULL;
@@ -193,7 +192,6 @@ static bool check_log_statement(List *stmt_list);
static int errdetail_execute(List *raw_parsetree_list);
static int errdetail_params(ParamListInfo params);
static int errdetail_abort(void);
-static int errdetail_recovery_conflict(void);
static void bind_param_error_callback(void *arg);
static void start_xact_command(void);
static void finish_xact_command(void);
@@ -2465,9 +2463,9 @@ errdetail_abort(void)
* Add an errdetail() line showing conflict source.
*/
static int
-errdetail_recovery_conflict(void)
+errdetail_recovery_conflict(ProcSignalReason reason)
{
- switch (RecoveryConflictReason)
+ switch (reason)
{
case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
errdetail("User was holding shared buffer pin for too long.");
@@ -2992,22 +2990,46 @@ 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;
+ RecoveryConflictPendingReasons[reason] = true;
+ RecoveryConflictPending = true;
+ InterruptPending = true;
+ /* latch will be set by procsignal_sigusr1_handler */
+}
+
+/*
+ * Check individual recovery conflict reasons. Called when
+ * RecoveryConflictPending is set.
+ */
+static void
+ProcessRecoveryConflictInterrupts(void)
+{
+ ProcSignalReason reason;
/*
- * Don't joggle the elbow of proc_exit
+ * We don't need to worry about joggling the elbow of proc_exit, because
+ * proc_exit_prepare() holds interrupts, so ProcessInterrupts() won't call
+ * us.
*/
- if (!proc_exit_inprogress)
+ Assert(!proc_exit_inprogress);
+ Assert(InterruptHoldoffCount == 0);
+ Assert(RecoveryConflictPending);
+
+ RecoveryConflictPending = false;
+
+ for (reason = PROCSIG_RECOVERY_CONFLICT_FIRST;
+ reason <= PROCSIG_RECOVERY_CONFLICT_LAST;
+ reason++)
{
- RecoveryConflictReason = reason;
+ if (!RecoveryConflictPendingReasons[reason])
+ continue;
+ RecoveryConflictPendingReasons[reason] = false;
+
switch (reason)
{
case PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK:
@@ -3016,7 +3038,7 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
* If we aren't waiting for a lock we can never deadlock.
*/
if (!IsWaitingForLock())
- return;
+ continue;
/* Intentional fall through to check wait for pin */
/* FALLTHROUGH */
@@ -3039,7 +3061,7 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
if (reason == PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK &&
GetStartupBufferPinWaitBufId() < 0)
CheckDeadLockAlert();
- return;
+ continue;
}
MyProc->recoveryConflictPending = true;
@@ -3055,7 +3077,7 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
* If we aren't in a transaction any longer then ignore.
*/
if (!IsTransactionOrTransactionBlock())
- return;
+ continue;
/*
* If we can abort just the current subtransaction then we are
@@ -3081,11 +3103,47 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
* subtransactions, which must cause FATAL, currently.
*/
if (IsAbortedTransactionBlockState())
- return;
+ continue;
+
+ /*
+ * If a recovery conflict happens while we are waiting for
+ * input from the client, the client is presumably just
+ * sitting idle in a transaction, preventing recovery from
+ * making progress. Terminate the connection to dislodge
+ * it.
+ */
+ if (DoingCommandRead)
+ {
+ LockErrorCleanup();
+ pgstat_report_recovery_conflict(reason);
+ ereport(FATAL,
+ (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("terminating connection due to conflict with recovery"),
+ errdetail_recovery_conflict(reason),
+ errhint("In a moment you should be able to reconnect to the"
+ " database and repeat your command.")));
+ }
- RecoveryConflictPending = true;
- QueryCancelPending = true;
- InterruptPending = true;
+ /* Avoid losing sync in the FE/BE protocol. */
+ if (QueryCancelHoldoffCount != 0)
+ {
+ /*
+ * Re-arm and defer this interrupt until later. See
+ * similar code in ProcessInterrupts().
+ */
+ RecoveryConflictPendingReasons[reason] = true;
+ RecoveryConflictPending = true;
+ InterruptPending = true;
+ continue;
+ }
+
+ /* We can use ERROR, because this conflict is retryable. */
+ LockErrorCleanup();
+ pgstat_report_recovery_conflict(reason);
+ ereport(ERROR,
+ (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("canceling statement due to conflict with recovery"),
+ errdetail_recovery_conflict(reason)));
break;
}
@@ -3093,36 +3151,24 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
/* FALLTHROUGH */
case PROCSIG_RECOVERY_CONFLICT_DATABASE:
- RecoveryConflictPending = true;
- ProcDiePending = true;
- InterruptPending = true;
+ pgstat_report_recovery_conflict(reason);
+ if (reason == PROCSIG_RECOVERY_CONFLICT_DATABASE)
+ ereport(FATAL,
+ (errcode(ERRCODE_DATABASE_DROPPED),
+ errmsg("terminating connection due to conflict with recovery"),
+ errdetail_recovery_conflict(reason)));
+ else
+ ereport(FATAL,
+ (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("terminating connection due to conflict with recovery"),
+ errdetail_recovery_conflict(reason)));
break;
default:
elog(FATAL, "unrecognized conflict mode: %d",
(int) reason);
}
-
- Assert(RecoveryConflictPending && (QueryCancelPending || ProcDiePending));
-
- /*
- * All conflicts apart from database cause dynamic errors where the
- * command or transaction can be retried at a later point with some
- * potential for success. No need to reset this, since non-retryable
- * conflict errors are currently FATAL.
- */
- 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;
}
/*
@@ -3146,6 +3192,9 @@ ProcessInterrupts(void)
return;
InterruptPending = false;
+ if (RecoveryConflictPending)
+ ProcessRecoveryConflictInterrupts();
+
if (ProcDiePending)
{
ProcDiePending = false;
@@ -3177,24 +3226,6 @@ ProcessInterrupts(void)
*/
proc_exit(1);
}
- else if (RecoveryConflictPending && RecoveryConflictRetryable)
- {
- pgstat_report_recovery_conflict(RecoveryConflictReason);
- ereport(FATAL,
- (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
- errmsg("terminating connection due to conflict with recovery"),
- errdetail_recovery_conflict()));
- }
- else if (RecoveryConflictPending)
- {
- /* Currently there is only one non-retryable recovery conflict */
- Assert(RecoveryConflictReason == PROCSIG_RECOVERY_CONFLICT_DATABASE);
- pgstat_report_recovery_conflict(RecoveryConflictReason);
- ereport(FATAL,
- (errcode(ERRCODE_DATABASE_DROPPED),
- errmsg("terminating connection due to conflict with recovery"),
- errdetail_recovery_conflict()));
- }
else if (IsBackgroundWorker)
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -3237,31 +3268,13 @@ ProcessInterrupts(void)
errmsg("connection to client lost")));
}
- /*
- * If a recovery conflict happens while we are waiting for input from the
- * client, the client is presumably just sitting idle in a transaction,
- * preventing recovery from making progress. Terminate the connection to
- * dislodge it.
- */
- if (RecoveryConflictPending && DoingCommandRead)
- {
- QueryCancelPending = false; /* this trumps QueryCancel */
- RecoveryConflictPending = false;
- LockErrorCleanup();
- pgstat_report_recovery_conflict(RecoveryConflictReason);
- ereport(FATAL,
- (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
- errmsg("terminating connection due to conflict with recovery"),
- errdetail_recovery_conflict(),
- errhint("In a moment you should be able to reconnect to the"
- " database and repeat your command.")));
- }
-
/*
* Don't allow query cancel interrupts while reading input from the
* client, because we might lose sync in the FE/BE protocol. (Die
* interrupts are OK, because we won't read any further messages from the
* client in that case.)
+ *
+ * See similar logic in ProcessRecoveryConflictInterrupts().
*/
if (QueryCancelPending && QueryCancelHoldoffCount != 0)
{
@@ -3320,16 +3333,6 @@ ProcessInterrupts(void)
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling autovacuum task")));
}
- if (RecoveryConflictPending)
- {
- RecoveryConflictPending = false;
- LockErrorCleanup();
- pgstat_report_recovery_conflict(RecoveryConflictReason);
- ereport(ERROR,
- (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
- errmsg("canceling statement due to conflict with recovery"),
- errdetail_recovery_conflict()));
- }
/*
* If we are reading a command from the client, just ignore the cancel
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
