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 <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/4cc13ba1-4248-4884-b6ba-4805349e7f39@iki.fi

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 6be565155ab6e825f7f6abb8c33e7949300cabc3..748c06b51cba96dbed304c2a81f8e5832af5a800 100644 (file)
--- 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 afffab77106f1d53b93bfe3556de4e2e0d654bac..6db803476c4f70b418227ae778cf12032a6e647c 100644 (file)
--- 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 696bbb7b9114004850f07ab4d8a899c47ad19463..fdeed0f3953480bd6da84f9c4d7341589e5e597d 100644 (file)
--- 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 b4a8d2f3a1c0284fa8adf4919bce80e0d1786081..d01d7a0898c18ad06d0957e375ec1b63902246d6 100644 (file)
--- 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 039bc8353be2b7dbd37833a983ca4976c3714f14..81f1960a635a28bc40aa3e0004675d1e0a5b5949 100644 (file)
--- 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 da7b5e78d30cfb638e8452edf60d28790862abc7..3a8593f87ba4ce5efaef851b2edad05783cca6bf 100644 (file)
--- 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);