From cd375d5b6d5f7d89375541af444e16dd93d27a03 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 3 Feb 2026 15:08:13 +0200 Subject: [PATCH] Remove useless errdetail_abort() I don't understand how to reach errdetail_abort() with MyProc->recoveryConflictPending set. If a recovery conflict signal is received, ProcessRecoveryConflictInterrupt() raises an ERROR or FATAL error to cancel the query or connection, and abort processing clears the flag. The error message from ProcessRecoveryConflictInterrupt() is very clear that the query or connection was terminated because of recovery conflict. The only way to reach it AFAICS is with a race condition, if the startup process sends a recovery conflict signal when the transaction has just entered aborted state for some other reason. And in that case the detail would be misleading, as the transaction was already aborted for some other reason, not because of the recovery conflict. errdetail_abort() was the only user of the recoveryConflictPending flag in PGPROC, so we can remove that and all the related code too. Reviewed-by: Chao Li Discussion: https://www.postgresql.org/message-id/4cc13ba1-4248-4884-b6ba-4805349e7f39@iki.fi --- src/backend/storage/ipc/procarray.c | 20 +++-------------- src/backend/storage/ipc/standby.c | 15 ++++++------- src/backend/storage/lmgr/proc.c | 1 - src/backend/tcop/postgres.c | 35 +++++------------------------ src/include/storage/proc.h | 7 ------ src/include/storage/procarray.h | 6 ++--- 6 files changed, 18 insertions(+), 66 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 6be565155ab..748c06b51cb 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -708,8 +708,6 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid) /* be sure this is cleared in abort */ proc->delayChkptFlags = 0; - proc->recoveryConflictPending = false; - /* must be cleared with xid/xmin: */ /* avoid unnecessarily dirtying shared cachelines */ if (proc->statusFlags & PROC_VACUUM_STATE_MASK) @@ -750,8 +748,6 @@ ProcArrayEndTransactionInternal(PGPROC *proc, TransactionId latestXid) /* be sure this is cleared in abort */ proc->delayChkptFlags = 0; - proc->recoveryConflictPending = false; - /* must be cleared with xid/xmin: */ /* avoid unnecessarily dirtying shared cachelines */ if (proc->statusFlags & PROC_VACUUM_STATE_MASK) @@ -933,7 +929,6 @@ ProcArrayClearTransaction(PGPROC *proc) proc->vxid.lxid = InvalidLocalTransactionId; proc->xmin = InvalidTransactionId; - proc->recoveryConflictPending = false; Assert(!(proc->statusFlags & PROC_VACUUM_STATE_MASK)); Assert(!proc->delayChkptFlags); @@ -3445,19 +3440,12 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid) } /* - * CancelVirtualTransaction - used in recovery conflict processing + * SignalVirtualTransaction - used in recovery conflict processing * * Returns pid of the process signaled, or 0 if not found. */ pid_t -CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode) -{ - return SignalVirtualTransaction(vxid, sigmode, true); -} - -pid_t -SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode, - bool conflictPending) +SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode) { ProcArrayStruct *arrayP = procArray; int index; @@ -3476,7 +3464,6 @@ SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode, if (procvxid.procNumber == vxid.procNumber && procvxid.localTransactionId == vxid.localTransactionId) { - proc->recoveryConflictPending = conflictPending; pid = proc->pid; if (pid != 0) { @@ -3618,7 +3605,7 @@ CountDBConnections(Oid databaseid) * CancelDBBackends --- cancel backends that are using specified database */ void -CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending) +CancelDBBackends(Oid databaseid, ProcSignalReason sigmode) { ProcArrayStruct *arrayP = procArray; int index; @@ -3638,7 +3625,6 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending) GET_VXID_FROM_PGPROC(procvxid, *proc); - proc->recoveryConflictPending = conflictPending; pid = proc->pid; if (pid != 0) { diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index afffab77106..6db803476c4 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -390,7 +390,7 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist, * Now find out who to throw out of the balloon. */ Assert(VirtualTransactionIdIsValid(*waitlist)); - pid = CancelVirtualTransaction(*waitlist, reason); + pid = SignalVirtualTransaction(*waitlist, reason); /* * Wait a little bit for it to die so that we avoid flooding @@ -581,7 +581,7 @@ ResolveRecoveryConflictWithDatabase(Oid dbid) */ while (CountDBBackends(dbid) > 0) { - CancelDBBackends(dbid, PROCSIG_RECOVERY_CONFLICT_DATABASE, true); + CancelDBBackends(dbid, PROCSIG_RECOVERY_CONFLICT_DATABASE); /* * Wait awhile for them to die so that we avoid flooding an @@ -724,8 +724,7 @@ ResolveRecoveryConflictWithLock(LOCKTAG locktag, bool logging_conflict) while (VirtualTransactionIdIsValid(*backends)) { SignalVirtualTransaction(*backends, - PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK, - false); + PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK); backends++; } @@ -881,11 +880,11 @@ SendRecoveryConflictWithBufferPin(ProcSignalReason reason) /* * We send signal to all backends to ask them if they are holding the - * buffer pin which is delaying the Startup process. We must not set the - * conflict flag yet, since most backends will be innocent. Let the - * SIGUSR1 handling in each backend decide their own fate. + * buffer pin which is delaying the Startup process. Most of them will be + * innocent, but we let the SIGUSR1 handling in each backend decide their + * own fate. */ - CancelDBBackends(InvalidOid, reason, false); + CancelDBBackends(InvalidOid, reason); } /* diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 696bbb7b911..fdeed0f3953 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -506,7 +506,6 @@ InitProcess(void) Assert(dlist_is_empty(&(MyProc->myProcLocks[i]))); } #endif - MyProc->recoveryConflictPending = false; /* Initialize fields for sync rep */ MyProc->waitLSN = InvalidXLogRecPtr; diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index b4a8d2f3a1c..d01d7a0898c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -175,7 +175,6 @@ static void forbidden_in_wal_sender(char firstchar); 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 void bind_param_error_callback(void *arg); static void start_xact_command(void); static void finish_xact_command(void); @@ -1141,8 +1140,7 @@ exec_simple_query(const char *query_string) ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); /* Make sure we are in a transaction command */ start_xact_command(); @@ -1498,8 +1496,7 @@ exec_parse_message(const char *query_string, /* string to execute */ ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); /* * Create the CachedPlanSource before we do parse analysis, since it @@ -1750,8 +1747,7 @@ exec_bind_message(StringInfo input_message) ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); /* * Create the portal. Allow silent replacement of an existing portal only @@ -2255,8 +2251,7 @@ exec_execute_message(const char *portal_name, long max_rows) ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); /* Check for cancel signal before we start execution */ CHECK_FOR_INTERRUPTS(); @@ -2536,20 +2531,6 @@ errdetail_params(ParamListInfo params) return 0; } -/* - * errdetail_abort - * - * Add an errdetail() line showing abort reason, if any. - */ -static int -errdetail_abort(void) -{ - if (MyProc->recoveryConflictPending) - errdetail("Abort reason: recovery conflict"); - - return 0; -} - /* * errdetail_recovery_conflict * @@ -2692,8 +2673,7 @@ exec_describe_statement_message(const char *stmt_name) ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); if (whereToSendOutput != DestRemote) return; /* can't actually do anything... */ @@ -2769,8 +2749,7 @@ exec_describe_portal_message(const char *portal_name) ereport(ERROR, (errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION), errmsg("current transaction is aborted, " - "commands ignored until end of transaction block"), - errdetail_abort())); + "commands ignored until end of transaction block"))); if (whereToSendOutput != DestRemote) return; /* can't actually do anything... */ @@ -3139,8 +3118,6 @@ ProcessRecoveryConflictInterrupt(ProcSignalReason reason) return; } - MyProc->recoveryConflictPending = true; - /* Intentional fall through to error handling */ /* FALLTHROUGH */ diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 039bc8353be..81f1960a635 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -235,13 +235,6 @@ struct PGPROC bool isRegularBackend; /* true if it's a regular backend. */ - /* - * While in hot standby mode, shows that a conflict signal has been sent - * for the current transaction. Set/cleared while holding ProcArrayLock, - * though not required. Accessed without lock, if needed. - */ - bool recoveryConflictPending; - /* * Info about LWLock the process is currently waiting for, if any. * diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h index da7b5e78d30..3a8593f87ba 100644 --- a/src/include/storage/procarray.h +++ b/src/include/storage/procarray.h @@ -77,14 +77,12 @@ extern VirtualTransactionId *GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0, bool allDbs, int excludeVacuum, int *nvxids); extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid); -extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode); -extern pid_t SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode, - bool conflictPending); +extern pid_t SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode); extern bool MinimumActiveBackends(int min); extern int CountDBBackends(Oid databaseid); extern int CountDBConnections(Oid databaseid); -extern void CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending); +extern void CancelDBBackends(Oid databaseid, ProcSignalReason sigmode); extern int CountUserBackends(Oid roleid); extern bool CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared); -- 2.47.3