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?
This commit is contained in:
Tom Lane
parent
6fb791199d
commit
82170c747b
1 changed files with 60 additions and 37 deletions
|
|
@ -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;
|
||||
|
||||
/*
|
||||
|
|
|
|||
Loading…
Reference in a new issue