From 82170c747bf74e31f7083849c07a53ec643356b4 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 7 Jan 2010 16:29:58 +0000
Subject: [PATCH] Fix (some of the) breakage introduced into query-cancel
processing by HS.
It is absolutely not okay to throw an ereport(ERROR) in any random place in
the code just because DoingCommandRead is set; interrupting, say, OpenSSL
in the midst of its activities is guaranteed to result in heartache.
Instead of that, undo the original optimizations that threw away
QueryCancelPending anytime we were starting or finishing a command read, and
instead discard the cancel request within ProcessInterrupts if we find that
there is no HS reason for forcing a cancel and we are DoingCommandRead.
In passing, may I once again condemn the practice of changing the code
and not fixing the adjacent comment that you just turned into a lie?
---
src/backend/tcop/postgres.c | 97 +++++++++++++++++++++++--------------
1 file changed, 60 insertions(+), 37 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a648bdf22ab..28560b94083 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -8,7 +8,7 @@
*
*
* IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.580 2010/01/02 16:57:52 momjian Exp $
+ * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.581 2010/01/07 16:29:58 tgl Exp $
*
* NOTES
* this is the "main" module of the postgres backend and
@@ -476,11 +476,10 @@ prepare_for_client_read(void)
EnableNotifyInterrupt();
EnableCatchupInterrupt();
- /* Allow "die" interrupt to be processed while waiting */
+ /* Allow cancel/die interrupts to be processed while waiting */
ImmediateInterruptOK = true;
/* And don't forget to detect one that already arrived */
- QueryCancelPending = false;
CHECK_FOR_INTERRUPTS();
}
}
@@ -494,7 +493,6 @@ client_read_ended(void)
if (DoingCommandRead)
{
ImmediateInterruptOK = false;
- QueryCancelPending = false; /* forget any CANCEL signal */
DisableNotifyInterrupt();
DisableCatchupInterrupt();
@@ -2640,12 +2638,11 @@ StatementCancelHandler(SIGNAL_ARGS)
QueryCancelPending = true;
/*
- * 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 it's safe to interrupt, and we're waiting for input or a lock,
+ * service the interrupt immediately
*/
- 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 */
@@ -2717,25 +2714,36 @@ ProcessInterrupts(void)
if (QueryCancelPending)
{
QueryCancelPending = false;
- ImmediateInterruptOK = false; /* not idle anymore */
- DisableNotifyInterrupt();
- DisableCatchupInterrupt();
- /* As in quickdie, don't risk sending to client during auth */
- if (ClientAuthInProgress && whereToSendOutput == DestRemote)
- whereToSendOutput = DestNone;
if (ClientAuthInProgress)
+ {
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+ /* As in quickdie, don't risk sending to client during auth */
+ if (whereToSendOutput == DestRemote)
+ whereToSendOutput = DestNone;
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling authentication due to timeout")));
- else if (cancel_from_timeout)
+ }
+ if (cancel_from_timeout)
+ {
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to statement timeout")));
- else if (IsAutoVacuumWorkerProcess())
+ }
+ if (IsAutoVacuumWorkerProcess())
+ {
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling autovacuum task")));
- else
+ }
{
int cancelMode = MyProc->recoveryConflictMode;
@@ -2756,34 +2764,50 @@ ProcessInterrupts(void)
switch (cancelMode)
{
case CONFLICT_MODE_FATAL:
- Assert(RecoveryInProgress());
- ereport(FATAL,
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+ 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,
+ /*
+ * 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.
+ */
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
+ 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;
+ /* No conflict pending, so fall through */
+ break;
}
+ }
+ /*
+ * If we are reading a command from the client, just ignore the
+ * cancel request --- sending an extra error message won't
+ * accomplish anything. Otherwise, go ahead and throw the error.
+ */
+ if (!DoingCommandRead)
+ {
+ ImmediateInterruptOK = false; /* not idle anymore */
+ DisableNotifyInterrupt();
+ DisableCatchupInterrupt();
ereport(ERROR,
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to user request")));
@@ -3626,7 +3650,6 @@ PostgresMain(int argc, char *argv[], const char *username)
* conditional since we don't want, say, reads on behalf of COPY FROM
* STDIN doing the same thing.)
*/
- QueryCancelPending = false; /* forget any earlier CANCEL signal */
DoingCommandRead = true;
/*
--
GitLab