On Mon, Jan 2, 2023 at 8:38 AM Thomas Munro <thomas.mu...@gmail.com> wrote:
> On Sat, Dec 31, 2022 at 6:36 PM Noah Misch <n...@leadboat.com> wrote:
> > On Sat, Dec 31, 2022 at 10:06:53AM +1300, Thomas Munro wrote:
> > > Idea #8 is a realisation that twisting oneself into a pretzel to avoid
> > > having to change the regexp code or its REG_CANCEL control flow may be
> > > a bit silly. If the only thing it really needs to do is free some
> > > memory, maybe the regexp module should provide a function that frees
> > > everything that is safe to call from our rcancelrequested callback, so
> > > we can do so before we longjmp back to Kansas. Then the REG_CANCEL
> > > code paths would be effectively unreachable in PostgreSQL. I don't
> > > know if it's better or worse than your idea #6, "use palloc instead,
> > > it already has garbage collection, duh", but it's a different take on
> > > the same realisation that this is just about free().
> >
> > PG_TRY() isn't free, so it's nice that (6) doesn't add one. If (6) fails in
> > some not-yet-revealed way, (8) could get more relevant.
> >
> > > I guess idea #6 must be pretty easy to try: just point that MALLOC()
> > > macro to palloc(), and do a plain old CFI() in rcancelrequested().
>
> It's not quite so easy: in RE_compile_and_cache we construct objects
> with arbitrary cache-managed lifetime, which suggests we need a cache
> memory context, but we could also fail mid construction, which
> suggests we'd need a dedicated per-regex object memory context that is
> made permanent with the MemoryContextSetParent() trick (as we see
> elsewhere for cached things that are constructed by code that might
> throw), ...
Here's an experiment-grade attempt at idea #6 done that way, for
discussion. You can see how much memory is wasted by each regex_t,
which I guess is probably on the order of a couple of hundred KB if
you use all 32 regex cache slots using ALLOCSET_SMALL_SIZES as I did
here:
postgres=# select 'x' ~ 'hello world .*';
-[ RECORD 1 ]
?column? | f
postgres=# select * from pg_backend_memory_contexts where name =
'RegexpMemoryContext';
-[ RECORD 1 ]-+-------------------------
name | RegexpMemoryContext
ident | hello world .*
parent | RegexpCacheMemoryContext
level | 2
total_bytes | 13376
total_nblocks | 5
free_bytes | 5144
free_chunks | 8
used_bytes | 8232
There's some more memory allocated in regc_pg_locale.c with raw
malloc() that could probably benefit from a pallocisation just to be
able to measure it, but I didn't touch that here.
The recovery conflict patch itself is unchanged, except that I removed
the claim in the commit message that this would be back-patched. It's
pretty clear that this would need to spend a decent amount of time on
master only.
From ee8eafb249368b889308fa23704f2a34a575b254 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Wed, 4 Jan 2023 14:15:40 +1300
Subject: [PATCH v4 1/2] Use MemoryContext API for regexp memory management.
Previously, regex_t objects' memory was managed using malloc() and
free() directly. Since regexp compilation can take same time, regcomp()
would periodically call a callback function that would check for query
cancelation. That design assumed that asynchronous query cancelation
could be detected by reading flags set by signal handlers, and that the
flags were a reliable indication that a later CHECK_FOR_INTERRUPTS()
would exit or throw an error. This allowed the code to free memory
before aborting.
A later commit will refactor the recovery conflict interrupt system, to
move its logic out of signal handlers, because it is not
async-signal-safe (among other problems). That would break the above
assumption, so we need another approach to memory cleanup.
Switch to using palloc(), the standard mechanism for garbage collection
in PostgreSQL. Since regex_t objects have to survive across transaction
boundaries in a cache, introduce RegexpCacheMemoryContext. Since
partial regex_t objects need to be cleaned up on failure to compile due
to interruption, also give each regex_t its own context. It is
re-parented to the longer living RegexpCacheMemoryContext only if
compilation is successful, following a pattern seen elsewhere in the
tree.
XXX Experimental, may contain nuts
---
src/backend/regex/regcomp.c | 14 +++----
src/backend/utils/adt/regexp.c | 70 ++++++++++++++++++++--------------
src/include/regex/regcustom.h | 6 +--
3 files changed, 52 insertions(+), 38 deletions(-)
diff --git a/src/backend/regex/regcomp.c b/src/backend/regex/regcomp.c
index bb8c240598..c0f8e77b49 100644
--- a/src/backend/regex/regcomp.c
+++ b/src/backend/regex/regcomp.c
@@ -2471,17 +2471,17 @@ rfree(regex_t *re)
/*
* rcancelrequested - check for external request to cancel regex operation
*
- * Return nonzero to fail the operation with error code REG_CANCEL,
- * zero to keep going
- *
- * The current implementation is Postgres-specific. If we ever get around
- * to splitting the regex code out as a standalone library, there will need
- * to be some API to let applications define a callback function for this.
+ * The current implementation always returns 0, if CHECK_FOR_INTERRUPTS()
+ * doesn't exit non-locally via ereport(). Memory allocated while compiling is
+ * expected to be cleaned up by virtue of being allocated using palloc in a
+ * suitable memory context.
*/
static int
rcancelrequested(void)
{
- return InterruptPending && (QueryCancelPending || ProcDiePending);
+ CHECK_FOR_INTERRUPTS();
+
+ return 0;
}
/*
diff --git a/src/backend/utils/adt/regexp.c b/src/backend/utils/adt/regexp.c
index 810dcb85b6..0336bf70f3 100644
--- a/src/backend/utils/adt/regexp.c
+++ b/src/backend/utils/adt/regexp.c
@@ -96,9 +96,13 @@ typedef struct regexp_matches_ctx
#define MAX_CACHED_RES 32
#endif
+/* A parent memory context for regular expressions. */
+static MemoryContext RegexpCacheMemoryContext;
+
/* this structure describes one cached regular expression */
typedef struct cached_re_str
{
+ MemoryContext cre_context; /* memory context for this regexp */
char *cre_pat; /* original RE (not null terminated!) */
int cre_pat_len; /* length of original RE, in bytes */
int cre_flags; /* compile flags: extended,icase etc */
@@ -145,6 +149,7 @@ RE_compile_and_cache(text *text_re, int cflags, Oid collation)
int regcomp_result;
cached_re_str re_temp;
char errMsg[100];
+ MemoryContext oldcontext;
/*
* Look for a match among previously compiled REs. Since the data
@@ -172,6 +177,13 @@ RE_compile_and_cache(text *text_re, int cflags, Oid collation)
}
}
+ /* Set up the cache memory on first go through. */
+ if (unlikely(RegexpCacheMemoryContext == NULL))
+ RegexpCacheMemoryContext =
+ AllocSetContextCreate(TopMemoryContext,
+ "RegexpCacheMemoryContext",
+ ALLOCSET_SMALL_SIZES);
+
/*
* Couldn't find it, so try to compile the new RE. To avoid leaking
* resources on failure, we build into the re_temp local.
@@ -183,6 +195,18 @@ RE_compile_and_cache(text *text_re, int cflags, Oid collation)
pattern,
text_re_len);
+ /*
+ * Make a memory context for this compiled regexp. This is initially a
+ * child of the current memory context, so it will be cleaned up
+ * automatically if compilation is interrupted and throws an ERROR.
+ * We'll re-parent it under the longer lived cache context if we make it
+ * to the bottom of this function.
+ */
+ re_temp.cre_context = AllocSetContextCreate(CurrentMemoryContext,
+ "RegexpMemoryContext",
+ ALLOCSET_SMALL_SIZES);
+ oldcontext = MemoryContextSwitchTo(re_temp.cre_context);
+
regcomp_result = pg_regcomp(&re_temp.cre_re,
pattern,
pattern_len,
@@ -193,37 +217,24 @@ RE_compile_and_cache(text *text_re, int cflags, Oid collation)
if (regcomp_result != REG_OKAY)
{
- /* re didn't compile (no need for pg_regfree, if so) */
-
- /*
- * Here and in other places in this file, do CHECK_FOR_INTERRUPTS
- * before reporting a regex error. This is so that if the regex
- * library aborts and returns REG_CANCEL, we don't print an error
- * message that implies the regex was invalid.
- */
- CHECK_FOR_INTERRUPTS();
-
+ /* re didn't compile */
pg_regerror(regcomp_result, &re_temp.cre_re, errMsg, sizeof(errMsg));
ereport(ERROR,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
errmsg("invalid regular expression: %s", errMsg)));
}
+ /* Copy the pattern into the per-regexp memory context. */
+ re_temp.cre_pat = palloc(text_re_len + 1);
+ memcpy(re_temp.cre_pat, text_re_val, text_re_len);
+
/*
- * We use malloc/free for the cre_pat field because the storage has to
- * persist across transactions, and because we want to get control back on
- * out-of-memory. The Max() is because some malloc implementations return
- * NULL for malloc(0).
+ * NUL-terminate it only for the benefit of the identifier used for the
+ * memory context, visible int he pg_backend_memory_contexts view.
*/
- re_temp.cre_pat = malloc(Max(text_re_len, 1));
- if (re_temp.cre_pat == NULL)
- {
- pg_regfree(&re_temp.cre_re);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory")));
- }
- memcpy(re_temp.cre_pat, text_re_val, text_re_len);
+ re_temp.cre_pat[text_re_len] = 0;
+ MemoryContextSetIdentifier(re_temp.cre_context, re_temp.cre_pat);
+
re_temp.cre_pat_len = text_re_len;
re_temp.cre_flags = cflags;
re_temp.cre_collation = collation;
@@ -236,16 +247,21 @@ RE_compile_and_cache(text *text_re, int cflags, Oid collation)
{
--num_res;
Assert(num_res < MAX_CACHED_RES);
- pg_regfree(&re_array[num_res].cre_re);
- free(re_array[num_res].cre_pat);
+ /* Delete the memory context holding the regexp and pattern. */
+ MemoryContextDelete(re_array[num_res].cre_context);
}
+ /* Re-parent the memory context to our long-lived cache context. */
+ MemoryContextSetParent(re_temp.cre_context, RegexpCacheMemoryContext);
+
if (num_res > 0)
memmove(&re_array[1], &re_array[0], num_res * sizeof(cached_re_str));
re_array[0] = re_temp;
num_res++;
+ MemoryContextSwitchTo(oldcontext);
+
return &re_array[0].cre_re;
}
@@ -283,7 +299,6 @@ RE_wchar_execute(regex_t *re, pg_wchar *data, int data_len,
if (regexec_result != REG_OKAY && regexec_result != REG_NOMATCH)
{
/* re failed??? */
- CHECK_FOR_INTERRUPTS();
pg_regerror(regexec_result, re, errMsg, sizeof(errMsg));
ereport(ERROR,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
@@ -1976,7 +1991,6 @@ regexp_fixed_prefix(text *text_re, bool case_insensitive, Oid collation,
default:
/* re failed??? */
- CHECK_FOR_INTERRUPTS();
pg_regerror(re_result, re, errMsg, sizeof(errMsg));
ereport(ERROR,
(errcode(ERRCODE_INVALID_REGULAR_EXPRESSION),
@@ -1990,7 +2004,7 @@ regexp_fixed_prefix(text *text_re, bool case_insensitive, Oid collation,
slen = pg_wchar2mb_with_len(str, result, slen);
Assert(slen < maxlen);
- free(str);
+ pfree(str);
return result;
}
diff --git a/src/include/regex/regcustom.h b/src/include/regex/regcustom.h
index fc158e1bb7..8f4025128e 100644
--- a/src/include/regex/regcustom.h
+++ b/src/include/regex/regcustom.h
@@ -49,9 +49,9 @@
/* overrides for regguts.h definitions, if any */
#define FUNCPTR(name, args) (*name) args
-#define MALLOC(n) malloc(n)
-#define FREE(p) free(VS(p))
-#define REALLOC(p,n) realloc(VS(p),n)
+#define MALLOC(n) palloc_extended((n), MCXT_ALLOC_NO_OOM)
+#define FREE(p) pfree(VS(p))
+#define REALLOC(p,n) repalloc_extended(VS(p),(n), MCXT_ALLOC_NO_OOM)
#define assert(x) Assert(x)
/* internal character type and related */
--
2.35.1
From dcae4fed08e34b43d450ebbd39aa1590acfc6693 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 v4 2/2] 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 all recovery conflict
checking logic into the next CFI following the standard pattern.
Reviewed-by: Andres Freund <and...@anarazel.de>
Reviewed-by: Michael Paquier <mich...@paquier.xyz>
Reviewed-by: Robert Haas <robertmh...@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGK3PGKwcKqzoosamn36YW-fsuTdOPPF1i_rtEO%3DnEYKSg%40mail.gmail.com
---
src/backend/storage/buffer/bufmgr.c | 4 +-
src/backend/storage/ipc/procsignal.c | 12 +-
src/backend/tcop/postgres.c | 312 ++++++++++++++-------------
src/include/storage/procsignal.h | 4 +-
src/include/tcop/tcopprot.h | 3 +-
5 files changed, 172 insertions(+), 163 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 3fb38a25cf..378f88ce11 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4373,8 +4373,8 @@ LockBufferForCleanup(Buffer buffer)
}
/*
- * Check called from RecoveryConflictInterrupt handler when Startup
- * process requests cancellation of all pin holders that are blocking it.
+ * Check called from ProcessRecoveryConflictInterrupts() when Startup process
+ * requests cancellation of all pin holders that are blocking it.
*/
bool
HoldingBufferPinThatDelaysRecovery(void)
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 43fbbdbc86..17ed71f790 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -658,22 +658,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 31479e8212..24930f93ba 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -158,9 +158,8 @@ 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;
+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;
@@ -179,7 +178,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);
@@ -2466,9 +2464,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.");
@@ -2993,137 +2991,190 @@ 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 */
+}
- /*
- * Don't joggle the elbow of proc_exit
- */
- if (!proc_exit_inprogress)
+/*
+ * Check one individual conflict reason.
+ */
+static void
+ProcessRecoveryConflictInterrupt(ProcSignalReason reason)
+{
+ switch (reason)
{
- RecoveryConflictReason = reason;
- switch (reason)
- {
- case PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK:
+ case PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK:
- /*
- * If we aren't waiting for a lock we can never deadlock.
- */
- if (!IsWaitingForLock())
- return;
+ /*
+ * If we aren't waiting for a lock we can never deadlock.
+ */
+ if (!IsWaitingForLock())
+ return;
- /* Intentional fall through to check wait for pin */
- /* FALLTHROUGH */
+ /* Intentional fall through to check wait for pin */
+ /* FALLTHROUGH */
- case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
+ case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
- /*
- * If PROCSIG_RECOVERY_CONFLICT_BUFFERPIN is requested but we
- * aren't blocking the Startup process there is nothing more
- * to do.
- *
- * When PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK is
- * requested, if we're waiting for locks and the startup
- * process is not waiting for buffer pin (i.e., also waiting
- * for locks), we set the flag so that ProcSleep() will check
- * for deadlocks.
- */
- if (!HoldingBufferPinThatDelaysRecovery())
- {
- if (reason == PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK &&
- GetStartupBufferPinWaitBufId() < 0)
- CheckDeadLockAlert();
- return;
- }
+ /*
+ * If PROCSIG_RECOVERY_CONFLICT_BUFFERPIN is requested but we
+ * aren't blocking the Startup process there is nothing more to
+ * do.
+ *
+ * When PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK is requested,
+ * if we're waiting for locks and the startup process is not
+ * waiting for buffer pin (i.e., also waiting for locks), we set
+ * the flag so that ProcSleep() will check for deadlocks.
+ */
+ if (!HoldingBufferPinThatDelaysRecovery())
+ {
+ if (reason == PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK &&
+ GetStartupBufferPinWaitBufId() < 0)
+ CheckDeadLockAlert();
+ return;
+ }
- MyProc->recoveryConflictPending = true;
+ MyProc->recoveryConflictPending = true;
- /* Intentional fall through to error handling */
- /* FALLTHROUGH */
+ /* Intentional fall through to error handling */
+ /* FALLTHROUGH */
+
+ case PROCSIG_RECOVERY_CONFLICT_LOCK:
+ case PROCSIG_RECOVERY_CONFLICT_TABLESPACE:
+ case PROCSIG_RECOVERY_CONFLICT_SNAPSHOT:
- case PROCSIG_RECOVERY_CONFLICT_LOCK:
- case PROCSIG_RECOVERY_CONFLICT_TABLESPACE:
- case PROCSIG_RECOVERY_CONFLICT_SNAPSHOT:
+ /*
+ * If we aren't in a transaction any longer then ignore.
+ */
+ if (!IsTransactionOrTransactionBlock())
+ return;
+ /*
+ * If we're not in a subtransaction then we are OK to throw an
+ * ERROR to resolve the conflict. Otherwise drop through to the
+ * FATAL case.
+ *
+ * XXX other times that we can throw just an ERROR *may* be
+ * PROCSIG_RECOVERY_CONFLICT_LOCK if no locks are held in parent
+ * transactions
+ *
+ * PROCSIG_RECOVERY_CONFLICT_SNAPSHOT if no snapshots are held by
+ * parent transactions and the transaction is not
+ * transaction-snapshot mode
+ *
+ * PROCSIG_RECOVERY_CONFLICT_TABLESPACE if no temp files or
+ * cursors open in parent transactions
+ */
+ if (!IsSubTransaction())
+ {
/*
- * If we aren't in a transaction any longer then ignore.
+ * If we already aborted then we no longer need to cancel. We
+ * do this here since we do not wish to ignore aborted
+ * subtransactions, which must cause FATAL, currently.
*/
- if (!IsTransactionOrTransactionBlock())
+ if (IsAbortedTransactionBlockState())
return;
/*
- * If we can abort just the current subtransaction then we are
- * OK to throw an ERROR to resolve the conflict. Otherwise
- * drop through to the FATAL case.
- *
- * XXX other times that we can throw just an ERROR *may* be
- * PROCSIG_RECOVERY_CONFLICT_LOCK if no locks are held in
- * parent transactions
- *
- * PROCSIG_RECOVERY_CONFLICT_SNAPSHOT if no snapshots are held
- * by parent transactions and the transaction is not
- * transaction-snapshot mode
- *
- * PROCSIG_RECOVERY_CONFLICT_TABLESPACE if no temp files or
- * cursors open in parent transactions
+ * 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. We'll drop through to the FATAL case
+ * below to dislodge it, in that case.
*/
- if (!IsSubTransaction())
+ if (!DoingCommandRead)
{
- /*
- * If we already aborted then we no longer need to cancel.
- * We do this here since we do not wish to ignore aborted
- * subtransactions, which must cause FATAL, currently.
- */
- if (IsAbortedTransactionBlockState())
+ /* 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;
return;
+ }
- RecoveryConflictPending = true;
- QueryCancelPending = true;
- InterruptPending = true;
+ /*
+ * We are cleared to throw an ERROR. We have a top-level
+ * transaction that we can abort and a conflict that isn't
+ * inherently non-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;
}
+ }
- /* Intentional fall through to session cancel */
- /* FALLTHROUGH */
-
- case PROCSIG_RECOVERY_CONFLICT_DATABASE:
- RecoveryConflictPending = true;
- ProcDiePending = true;
- InterruptPending = true;
- break;
+ /* Intentional fall through to session cancel */
+ /* FALLTHROUGH */
- default:
- elog(FATAL, "unrecognized conflict mode: %d",
- (int) reason);
- }
+ case PROCSIG_RECOVERY_CONFLICT_DATABASE:
- Assert(RecoveryConflictPending && (QueryCancelPending || ProcDiePending));
+ /*
+ * Retrying is not possible because the database is dropped, or we
+ * decided above that we couldn't resolve the conflict with an
+ * ERROR and fell through. Terminate the session.
+ */
+ pgstat_report_recovery_conflict(reason);
+ ereport(FATAL,
+ (errcode(reason == PROCSIG_RECOVERY_CONFLICT_DATABASE ?
+ ERRCODE_DATABASE_DROPPED :
+ 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.")));
+ break;
- /*
- * 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;
+ default:
+ elog(FATAL, "unrecognized conflict mode: %d", (int) reason);
}
+}
+
+/*
+ * Check each possible recovery conflict reason.
+ */
+static void
+ProcessRecoveryConflictInterrupts(void)
+{
+ 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.
+ * 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.
*/
- SetLatch(MyLatch);
+ Assert(!proc_exit_inprogress);
+ Assert(InterruptHoldoffCount == 0);
+ Assert(RecoveryConflictPending);
- errno = save_errno;
+ RecoveryConflictPending = false;
+
+ for (reason = PROCSIG_RECOVERY_CONFLICT_FIRST;
+ reason <= PROCSIG_RECOVERY_CONFLICT_LAST;
+ reason++)
+ {
+ if (RecoveryConflictPendingReasons[reason])
+ {
+ RecoveryConflictPendingReasons[reason] = false;
+ ProcessRecoveryConflictInterrupt(reason);
+ }
+ }
}
/*
@@ -3178,24 +3229,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),
@@ -3238,31 +3271,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)
{
@@ -3321,16 +3336,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
@@ -3346,6 +3351,9 @@ ProcessInterrupts(void)
}
}
+ if (RecoveryConflictPending)
+ ProcessRecoveryConflictInterrupts();
+
if (IdleInTransactionSessionTimeoutPending)
{
/*
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 45e2f1fe9d..362301ccf2 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 abd7b4fff3..ab43b638ee 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -70,8 +70,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.35.1
