On Thu, 2010-01-07 at 12:14 -0500, Tom Lane wrote:
> Simon Riggs <si...@2ndquadrant.com> writes:
> > On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
> >> @Simon: Is there a reason why you have not yet removed recoveryConflictMode
> >> from PGPROC?
>
> > Unfortunately we still need a mechanism to mark which backends have been
> > cancelled already. Transaction state for virtual transactions isn't
> > visible on the procarray, so we need something there to indicate that a
> > backend has been sent a conflict. Otherwise we'd end up waiting for it
> > endlessly. The name will be changing though.
>
> While we're discussing this: the current coding with
> AbortOutOfAnyTransaction within ProcessInterrupts is *utterly* unsafe.
> I realize that's just a toy placeholder, but getting rid of it has to be
> on the list of stop-ship items. Right at the moment I'd prefer to see
> CONFLICT_MODE_ERROR always turned into CONFLICT_MODE_FATAL than to
> imagine this is going to work.
Hmmm. Can you check my current attempt? This may suffer this problem.
If, so can you explain a little more for me? Thanks.
--
Simon Riggs www.2ndQuadrant.com
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index ea40420..e05792e 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -313,8 +313,7 @@ IsTransactionState(void)
/*
* IsAbortedTransactionBlockState
*
- * This returns true if we are currently running a query
- * within an aborted transaction block.
+ * This returns true if we are within an aborted transaction block.
*/
bool
IsAbortedTransactionBlockState(void)
@@ -2692,6 +2691,48 @@ AbortCurrentTransaction(void)
}
/*
+ * AbortTransactionAndAnySubtransactions
+ *
+ * Similar to AbortCurrentTransaction but if any subtransactions
+ * in progress we abort them *and* all of their parents. So this is
+ * used when the caller wishes to make the abort untrappable by the user.
+ * After this has run IsAbortedTransactionBlockState() will be true.
+ */
+void
+AbortTransactionAndAnySubtransactions(void)
+{
+ TransactionState s = CurrentTransactionState;
+
+ switch (s->blockState)
+ {
+ case TBLOCK_DEFAULT:
+ case TBLOCK_STARTED:
+ case TBLOCK_BEGIN:
+ case TBLOCK_INPROGRESS:
+ case TBLOCK_END:
+ case TBLOCK_ABORT:
+ case TBLOCK_SUBABORT:
+ case TBLOCK_ABORT_END:
+ case TBLOCK_ABORT_PENDING:
+ case TBLOCK_PREPARE:
+ case TBLOCK_SUBABORT_END:
+ case TBLOCK_SUBABORT_RESTART:
+ AbortCurrentTransaction();
+ break;
+
+ case TBLOCK_SUBINPROGRESS:
+ case TBLOCK_SUBBEGIN:
+ case TBLOCK_SUBEND:
+ case TBLOCK_SUBABORT_PENDING:
+ case TBLOCK_SUBRESTART:
+ AbortSubTransaction();
+ CleanupSubTransaction();
+ AbortTransactionAndAnySubtransactions();
+ break;
+ }
+}
+
+/*
* PreventTransactionChain
*
* This routine is to be called by statements that must not run inside
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 85f14f6..e2e64dd 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -324,6 +324,7 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
/* must be cleared with xid/xmin: */
proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
proc->inCommit = false; /* be sure this is cleared in abort */
+ proc->recoveryConflictPending = false;
/* Clear the subtransaction-XID cache too while holding the lock */
proc->subxids.nxids = 0;
@@ -350,6 +351,7 @@ ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
/* must be cleared with xid/xmin: */
proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
proc->inCommit = false; /* be sure this is cleared in abort */
+ proc->recoveryConflictPending = false;
Assert(proc->subxids.nxids == 0);
Assert(proc->subxids.overflowed == false);
@@ -377,7 +379,7 @@ ProcArrayClearTransaction(PGPROC *proc)
proc->xid = InvalidTransactionId;
proc->lxid = InvalidLocalTransactionId;
proc->xmin = InvalidTransactionId;
- proc->recoveryConflictMode = 0;
+ proc->recoveryConflictPending = false;
/* redundant, but just in case */
proc->vacuumFlags &= ~PROC_VACUUM_STATE_MASK;
@@ -1665,7 +1667,7 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
if (proc->pid == 0)
continue;
- if (skipExistingConflicts && proc->recoveryConflictMode > 0)
+ if (skipExistingConflicts && proc->recoveryConflictPending)
continue;
if (!OidIsValid(dbOid) ||
@@ -1704,7 +1706,7 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
* Returns pid of the process signaled, or 0 if not found.
*/
pid_t
-CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
+CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
{
ProcArrayStruct *arrayP = procArray;
int index;
@@ -1722,28 +1724,22 @@ CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
if (procvxid.backendId == vxid.backendId &&
procvxid.localTransactionId == vxid.localTransactionId)
{
- /*
- * Issue orders for the proc to read next time it receives SIGINT
- */
- if (proc->recoveryConflictMode < cancel_mode)
- proc->recoveryConflictMode = cancel_mode;
-
+ proc->recoveryConflictPending = true;
pid = proc->pid;
+ if (pid != 0)
+ {
+ /*
+ * Kill the pid if it's still here. If not, that's what we wanted
+ * so ignore any errors.
+ */
+ (void) SendProcSignal(pid, sigmode, vxid.backendId);
+ }
break;
}
}
LWLockRelease(ProcArrayLock);
- if (pid != 0)
- {
- /*
- * Kill the pid if it's still here. If not, that's what we wanted
- * so ignore any errors.
- */
- kill(pid, SIGINT);
- }
-
return pid;
}
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 480af00..c4007fe 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -24,6 +24,8 @@
#include "storage/procsignal.h"
#include "storage/shmem.h"
#include "storage/sinval.h"
+#include "storage/standby.h"
+#include "tcop/tcopprot.h"
/*
@@ -258,5 +260,11 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
if (CheckProcSignal(PROCSIG_NOTIFY_INTERRUPT))
HandleNotifyInterrupt();
+ if (CheckProcSignal(PROCSIG_CONFLICT_ERROR_INTERRUPT))
+ RecoveryConflictInterrupt(CONFLICT_MODE_ERROR);
+
+ if (CheckProcSignal(PROCSIG_CONFLICT_FATAL_INTERRUPT))
+ RecoveryConflictInterrupt(CONFLICT_MODE_FATAL);
+
errno = save_errno;
}
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 0b4e022..0ea7a32 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -159,8 +159,6 @@ WaitExceedsMaxStandbyDelay(void)
* recovery processing. Judgement has already been passed on it within
* a specific rmgr. Here we just issue the orders to the procs. The procs
* then throw the required error as instructed.
- *
- * We may ask for a specific cancel_mode, typically ERROR or FATAL.
*/
void
ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
@@ -218,12 +216,16 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
if (WaitExceedsMaxStandbyDelay())
{
pid_t pid;
+ ProcSignalReason sigmode = PROCSIG_CONFLICT_ERROR_INTERRUPT;
+
+ if (cancel_mode == CONFLICT_MODE_FATAL)
+ sigmode = PROCSIG_CONFLICT_FATAL_INTERRUPT;
/*
* Now find out who to throw out of the balloon.
*/
Assert(VirtualTransactionIdIsValid(*waitlist));
- pid = CancelVirtualTransaction(*waitlist, cancel_mode);
+ pid = CancelVirtualTransaction(*waitlist, sigmode);
if (pid != 0)
{
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 352ff08..a708369 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -318,7 +318,7 @@ InitProcess(void)
MyProc->waitProcLock = NULL;
for (i = 0; i < NUM_LOCK_PARTITIONS; i++)
SHMQueueInit(&(MyProc->myProcLocks[i]));
- MyProc->recoveryConflictMode = 0;
+ MyProc->recoveryConflictPending = false;
/*
* We might be reusing a semaphore that belonged to a failed process. So
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3bddf49..3ebde79 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -172,6 +172,9 @@ static int UseNewLine = 1; /* Use newlines query delimiters (the default) */
static int UseNewLine = 0; /* Use EOF as query delimiters */
#endif /* TCOP_DONTUSENEWLINE */
+/* whether we were cancelled during recovery by conflict processing or not */
+static bool RecoveryConflictPending = false;
+static bool TransactionCancelPending = false;
/* ----------------------------------------------------------------
* decls for routines only used in this file
@@ -185,6 +188,7 @@ static List *pg_rewrite_query(Query *query);
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 start_xact_command(void);
static void finish_xact_command(void);
static bool IsTransactionExitStmt(Node *parsetree);
@@ -945,7 +949,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
/* Make sure we are in a transaction command */
start_xact_command();
@@ -1254,7 +1259,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
/*
* Set up a snapshot if parse analysis/planning will need one.
@@ -1534,7 +1540,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
/*
* Create the portal. Allow silent replacement of an existing portal only
@@ -1975,7 +1982,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
/* Check for cancel signal before we start execution */
CHECK_FOR_INTERRUPTS();
@@ -2236,6 +2244,20 @@ errdetail_params(ParamListInfo params)
}
/*
+ * 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;
+}
+
+/*
* exec_describe_statement_message
*
* Process a "Describe" message for a prepared statement
@@ -2292,7 +2314,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
if (whereToSendOutput != DestRemote)
return; /* can't actually do anything... */
@@ -2372,7 +2395,8 @@ 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")));
+ "commands ignored until end of transaction block"),
+ errdetail_abort()));
if (whereToSendOutput != DestRemote)
return; /* can't actually do anything... */
@@ -2643,9 +2667,59 @@ StatementCancelHandler(SIGNAL_ARGS)
* If it's safe to interrupt, and we're waiting for a lock, service
* the interrupt immediately. No point in interrupting if we're
* waiting for input, however.
+ */
+ if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
+ CritSectionCount == 0 && !DoingCommandRead)
+ {
+ /* bump holdoff count to make ProcessInterrupts() a no-op */
+ /* until we are done getting ready for it */
+ InterruptHoldoffCount++;
+ LockWaitCancel(); /* prevent CheckDeadLock from running */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+ InterruptHoldoffCount--;
+ ProcessInterrupts();
+ }
+ }
+
+ errno = save_errno;
+}
+
+void
+RecoveryConflictInterrupt(int conflict_mode)
+{
+ int save_errno = errno;
+
+ /*
+ * Don't joggle the elbow of proc_exit
+ */
+ if (!proc_exit_inprogress)
+ {
+ switch (conflict_mode)
+ {
+ case CONFLICT_MODE_FATAL:
+ ProcDiePending = true;
+ break;
+
+ case CONFLICT_MODE_ERROR:
+ if (IsAbortedTransactionBlockState())
+ return;
+ TransactionCancelPending = true;
+ break;
+
+ default:
+ elog(ERROR, "Unknown conflict mode");
+ }
+
+ RecoveryConflictPending = true;
+ InterruptPending = true;
+
+ /*
+ * If it's safe to interrupt, and we're waiting for input or a lock,
+ * service the interrupt immediately. Same as in die()
*/
- if (InterruptHoldoffCount == 0 && CritSectionCount == 0 &&
- (DoingCommandRead || ImmediateInterruptOK))
+ if (ImmediateInterruptOK && InterruptHoldoffCount == 0 &&
+ CritSectionCount == 0)
{
/* bump holdoff count to make ProcessInterrupts() a no-op */
/* until we are done getting ready for it */
@@ -2709,6 +2783,10 @@ ProcessInterrupts(void)
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating autovacuum process due to administrator command")));
+ else if (RecoveryConflictPending)
+ ereport(NOTICE,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to conflict with recovery")));
else
ereport(FATAL,
(errcode(ERRCODE_ADMIN_SHUTDOWN),
@@ -2735,59 +2813,42 @@ ProcessInterrupts(void)
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling autovacuum task")));
- else
- {
- int cancelMode = MyProc->recoveryConflictMode;
-
- /*
- * XXXHS: We don't yet have a clean way to cancel an
- * idle-in-transaction session, so make it FATAL instead.
- * This isn't as bad as it looks because we don't issue a
- * CONFLICT_MODE_ERROR for a session with proc->xmin == 0
- * on cleanup conflicts. There's a possibility that we
- * marked somebody as a conflict and then they go idle.
- */
- if (DoingCommandRead && IsTransactionBlock() &&
- cancelMode == CONFLICT_MODE_ERROR)
- {
- cancelMode = CONFLICT_MODE_FATAL;
- }
-
- switch (cancelMode)
- {
- case CONFLICT_MODE_FATAL:
- Assert(RecoveryInProgress());
- ereport(FATAL,
- (errcode(ERRCODE_QUERY_CANCELED),
- errmsg("canceling session due to conflict with recovery")));
-
- case CONFLICT_MODE_ERROR:
- /*
- * We are aborting because we need to release
- * locks. So we need to abort out of all
- * subtransactions to make sure we release
- * all locks at whatever their level.
- *
- * XXX Should we try to examine the
- * transaction tree and cancel just enough
- * subxacts to remove locks? Doubt it.
- */
- Assert(RecoveryInProgress());
- AbortOutOfAnyTransaction();
- ereport(ERROR,
- (errcode(ERRCODE_QUERY_CANCELED),
- errmsg("canceling statement due to conflict with recovery")));
- return;
-
- default:
- /* No conflict pending, so fall through */
- break;
- }
-
+ else
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to user request")));
- }
+ }
+ if (TransactionCancelPending)
+ {
+ TransactionCancelPending = false;
+ ImmediateInterruptOK = false; /* not idle anymore */
+
+ /*
+ * Cancel the transaction whether or not it was idle, so don't mention
+ * the word idle in the message.
+ */
+ if (RecoveryConflictPending)
+ ereport(NOTICE,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("canceling transaction due to conflict with recovery")));
+ else
+ ereport(NOTICE,
+ (errcode(ERRCODE_QUERY_CANCELED),
+ errmsg("canceling transaction due to administrator request")));
+
+ /*
+ * Indicate transaction aborted by recovery so we can use the appropriate
+ * error message when we enter the aborted state.
+ */
+ AbortTransactionAndAnySubtransactions();
+ RecoveryConflictPending = false;
+
+ /*
+ * Set the display correctly now, rather than wait for client wait loop
+ * to cycle round and reset display at some later time.
+ */
+ set_ps_display("idle in transaction (aborted)", false);
+ pgstat_report_activity("<IDLE> in transaction (aborted)");
}
/* If we get here, do nothing (probably, QueryCancelPending was reset) */
}
@@ -3603,7 +3664,12 @@ PostgresMain(int argc, char *argv[], const char *username)
*/
if (send_ready_for_query)
{
- if (IsTransactionOrTransactionBlock())
+ if (IsAbortedTransactionBlockState())
+ {
+ set_ps_display("idle in transaction (aborted)", false);
+ pgstat_report_activity("<IDLE> in transaction (aborted)");
+ }
+ else if (IsTransactionOrTransactionBlock())
{
set_ps_display("idle in transaction", false);
pgstat_report_activity("<IDLE> in transaction");
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 6eee8ca..89481a3 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -204,6 +204,7 @@ extern bool IsTransactionBlock(void);
extern bool IsTransactionOrTransactionBlock(void);
extern char TransactionBlockStatusCode(void);
extern void AbortOutOfAnyTransaction(void);
+extern void AbortTransactionAndAnySubtransactions(void);
extern void PreventTransactionChain(bool isTopLevel, const char *stmtType);
extern void RequireTransactionChain(bool isTopLevel, const char *stmtType);
extern bool IsInTransactionChain(bool isTopLevel);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 2298bf2..8105d39 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -96,11 +96,11 @@ struct PGPROC
uint8 vacuumFlags; /* vacuum-related flags, see above */
/*
- * While in hot standby mode, setting recoveryConflictMode instructs
- * the backend to commit suicide. Possible values are the same as those
- * passed to ResolveRecoveryConflictWithVirtualXIDs().
+ * 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.
*/
- int recoveryConflictMode;
+ bool recoveryConflictPending;
/* Info about LWLock the process is currently waiting for, if any. */
bool lwWaiting; /* true if waiting for an LW lock */
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 1cee639..ec358b1 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -15,6 +15,7 @@
#define PROCARRAY_H
#include "storage/lock.h"
+#include "storage/procsignal.h"
#include "storage/standby.h"
#include "utils/snapshot.h"
@@ -58,8 +59,7 @@ extern VirtualTransactionId *GetCurrentVirtualXIDs(TransactionId limitXmin,
int *nvxids);
extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin,
Oid dbOid, bool skipExistingConflicts);
-extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid,
- int cancel_mode);
+extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode);
extern int CountActiveBackends(void);
extern int CountDBBackends(Oid databaseid);
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 7f3b65e..59a2f97 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -31,6 +31,8 @@ typedef enum
{
PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
+ PROCSIG_CONFLICT_ERROR_INTERRUPT, /* recovery conflict error */
+ PROCSIG_CONFLICT_FATAL_INTERRUPT, /* recovery conflict fatal */
NUM_PROCSIGNALS /* Must be last! */
} ProcSignalReason;
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 38df3d7..d6a7f0c 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -26,6 +26,7 @@ extern int vacuum_defer_cleanup_age;
extern void ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist,
char *reason, int cancel_mode);
+extern void HandleConflictInterrupt(int conflict_mode);
extern void InitRecoveryTransactionEnvironment(void);
extern void ShutdownRecoveryTransactionEnvironment(void);
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index d7ede2b..fc5ab9e 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -63,6 +63,7 @@ extern bool assign_max_stack_depth(int newval, bool doit, GucSource source);
extern void die(SIGNAL_ARGS);
extern void quickdie(SIGNAL_ARGS);
extern void StatementCancelHandler(SIGNAL_ARGS);
+extern void RecoveryConflictInterrupt(int conflict_mode);
extern void FloatExceptionHandler(SIGNAL_ARGS);
extern void prepare_for_client_read(void);
extern void client_read_ended(void);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
