>From 6dfca6b69a5d6f8bb36b3a7df1b7e4005d166b3d Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 15 Jan 2015 00:57:12 +0100 Subject: [PATCH 08/10] Remove remnants of ImmediateInterruptOK handling. Now that nothing sets ImmediateInterruptOK to true anymore, we can remove all the supporting code. --- src/backend/storage/ipc/ipc.c | 2 -- src/backend/tcop/postgres.c | 52 ----------------------------------- src/backend/utils/error/elog.c | 29 ++----------------- src/backend/utils/init/globals.c | 1 - src/backend/utils/misc/timeout.c | 38 ++----------------------- src/include/miscadmin.h | 1 - src/test/modules/test_shm_mq/worker.c | 6 +--- 7 files changed, 7 insertions(+), 122 deletions(-) diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c index f0f7939..6bc0b06 100644 --- a/src/backend/storage/ipc/ipc.c +++ b/src/backend/storage/ipc/ipc.c @@ -165,8 +165,6 @@ proc_exit_prepare(int code) InterruptPending = false; ProcDiePending = false; QueryCancelPending = false; - /* And let's just make *sure* we're not interrupted ... */ - ImmediateInterruptOK = false; InterruptHoldoffCount = 1; CritSectionCount = 0; diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 0723c81..9620b4d 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -2622,21 +2622,6 @@ die(SIGNAL_ARGS) { InterruptPending = true; ProcDiePending = true; - - /* - * If it's safe to interrupt, and we're waiting for a lock, service - * the interrupt immediately - */ - if (ImmediateInterruptOK && InterruptHoldoffCount == 0 && - CritSectionCount == 0) - { - /* bump holdoff count to make ProcessInterrupts() a no-op */ - /* until we are done getting ready for it */ - InterruptHoldoffCount++; - LockErrorCleanup(); /* prevent CheckDeadLock from running */ - InterruptHoldoffCount--; - ProcessInterrupts(); - } } /* If we're still here, waken anything waiting on the process latch */ @@ -2661,21 +2646,6 @@ StatementCancelHandler(SIGNAL_ARGS) { InterruptPending = true; QueryCancelPending = true; - - /* - * If it's safe to interrupt, and we're waiting for a lock, service - * the interrupt immediately - */ - if (ImmediateInterruptOK && InterruptHoldoffCount == 0 && - CritSectionCount == 0) - { - /* bump holdoff count to make ProcessInterrupts() a no-op */ - /* until we are done getting ready for it */ - InterruptHoldoffCount++; - LockErrorCleanup(); /* prevent CheckDeadLock from running */ - InterruptHoldoffCount--; - ProcessInterrupts(); - } } /* If we're still here, waken anything waiting on the process latch */ @@ -2816,21 +2786,6 @@ RecoveryConflictInterrupt(ProcSignalReason reason) */ if (reason == PROCSIG_RECOVERY_CONFLICT_DATABASE) RecoveryConflictRetryable = false; - - /* - * If it's safe to interrupt, and we're waiting for a lock, service - * the interrupt immediately - */ - if (ImmediateInterruptOK && InterruptHoldoffCount == 0 && - CritSectionCount == 0) - { - /* bump holdoff count to make ProcessInterrupts() a no-op */ - /* until we are done getting ready for it */ - InterruptHoldoffCount++; - LockErrorCleanup(); /* prevent CheckDeadLock from running */ - InterruptHoldoffCount--; - ProcessInterrupts(); - } } /* @@ -2863,7 +2818,6 @@ ProcessInterrupts(void) { ProcDiePending = false; QueryCancelPending = false; /* ProcDie trumps QueryCancel */ - ImmediateInterruptOK = false; /* not idle anymore */ /* As in quickdie, don't risk sending to client during auth */ if (ClientAuthInProgress && whereToSendOutput == DestRemote) whereToSendOutput = DestNone; @@ -2904,7 +2858,6 @@ ProcessInterrupts(void) if (ClientConnectionLost) { QueryCancelPending = false; /* lost connection trumps QueryCancel */ - ImmediateInterruptOK = false; /* not idle anymore */ /* don't send to client, we already know the connection to be dead. */ whereToSendOutput = DestNone; ereport(FATAL, @@ -2921,7 +2874,6 @@ ProcessInterrupts(void) */ if (get_timeout_indicator(LOCK_TIMEOUT, true)) { - ImmediateInterruptOK = false; /* not idle anymore */ (void) get_timeout_indicator(STATEMENT_TIMEOUT, true); ereport(ERROR, (errcode(ERRCODE_LOCK_NOT_AVAILABLE), @@ -2929,21 +2881,18 @@ ProcessInterrupts(void) } if (get_timeout_indicator(STATEMENT_TIMEOUT, true)) { - ImmediateInterruptOK = false; /* not idle anymore */ ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg("canceling statement due to statement timeout"))); } if (IsAutoVacuumWorkerProcess()) { - ImmediateInterruptOK = false; /* not idle anymore */ ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg("canceling autovacuum task"))); } if (RecoveryConflictPending) { - ImmediateInterruptOK = false; /* not idle anymore */ RecoveryConflictPending = false; pgstat_report_recovery_conflict(RecoveryConflictReason); if (DoingCommandRead) @@ -2967,7 +2916,6 @@ ProcessInterrupts(void) */ if (!DoingCommandRead) { - ImmediateInterruptOK = false; /* not idle anymore */ ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg("canceling statement due to user request"))); diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index bee2c92..136f731 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -412,7 +412,6 @@ errfinish(int dummy,...) { ErrorData *edata = &errordata[errordata_stack_depth]; int elevel; - bool save_ImmediateInterruptOK; MemoryContext oldcontext; ErrorContextCallback *econtext; @@ -421,18 +420,6 @@ errfinish(int dummy,...) elevel = edata->elevel; /* - * Ensure we can't get interrupted while performing error reporting. This - * is needed to prevent recursive entry to functions like syslog(), which - * may not be re-entrant. - * - * Note: other places that save-and-clear ImmediateInterruptOK also do - * HOLD_INTERRUPTS(), but that should not be necessary here since we don't - * call anything that could turn on ImmediateInterruptOK. - */ - save_ImmediateInterruptOK = ImmediateInterruptOK; - ImmediateInterruptOK = false; - - /* * Do processing in ErrorContext, which we hope has enough reserved space * to report an error. */ @@ -463,10 +450,6 @@ errfinish(int dummy,...) * itself be inside a holdoff section. If necessary, such a handler * could save and restore InterruptHoldoffCount for itself, but this * should make life easier for most.) - * - * Note that we intentionally don't restore ImmediateInterruptOK here, - * even if it was set at entry. We definitely don't want that on - * while doing error cleanup. */ InterruptHoldoffCount = 0; @@ -572,15 +555,9 @@ errfinish(int dummy,...) } /* - * We reach here if elevel <= WARNING. OK to return to caller, so restore - * caller's setting of ImmediateInterruptOK. - */ - ImmediateInterruptOK = save_ImmediateInterruptOK; - - /* - * But check for cancel/die interrupt first --- this is so that the user - * can stop a query emitting tons of notice or warning messages, even if - * it's in a loop that otherwise fails to check for interrupts. + * Check for cancel/die interrupt first --- this is so that the user can + * stop a query emitting tons of notice or warning messages, even if it's + * in a loop that otherwise fails to check for interrupts. */ CHECK_FOR_INTERRUPTS(); } diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index c35867b..8d3d51b 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -30,7 +30,6 @@ volatile bool InterruptPending = false; volatile bool QueryCancelPending = false; volatile bool ProcDiePending = false; volatile bool ClientConnectionLost = false; -volatile bool ImmediateInterruptOK = false; volatile uint32 InterruptHoldoffCount = 0; volatile uint32 CritSectionCount = 0; diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c index ce4bc13..a7ec665 100644 --- a/src/backend/utils/misc/timeout.c +++ b/src/backend/utils/misc/timeout.c @@ -259,27 +259,12 @@ static void handle_sig_alarm(SIGNAL_ARGS) { int save_errno = errno; - bool save_ImmediateInterruptOK = ImmediateInterruptOK; /* - * We may be executing while ImmediateInterruptOK is true (e.g., when - * mainline is waiting for a lock). If SIGINT or similar arrives while - * this code is running, we'd lose control and perhaps leave our data - * structures in an inconsistent state. Disable immediate interrupts, and - * just to be real sure, bump the holdoff counter as well. (The reason - * for this belt-and-suspenders-too approach is to make sure that nothing - * bad happens if a timeout handler calls code that manipulates - * ImmediateInterruptOK.) - * - * Note: it's possible for a SIGINT to interrupt handle_sig_alarm before - * we manage to do this; the net effect would be as if the SIGALRM event - * had been silently lost. Therefore error recovery must include some - * action that will allow any lost interrupt to be rescheduled. Disabling - * some or all timeouts is sufficient, or if that's not appropriate, - * reschedule_timeouts() can be called. Also, the signal blocking hazard - * described below applies here too. + * Bump the holdoff counter, to make sure nothing we call will process + * interrupts directly. No timeout handler should do that, but these + * failures are hard to debug, so better be sure. */ - ImmediateInterruptOK = false; HOLD_INTERRUPTS(); /* @@ -332,24 +317,7 @@ handle_sig_alarm(SIGNAL_ARGS) } } - /* - * Re-allow query cancel, and then try to service any cancel request that - * arrived meanwhile (this might in particular include a cancel request - * fired by one of the timeout handlers). Since we are in a signal - * handler, we mustn't call ProcessInterrupts unless ImmediateInterruptOK - * is set; if it isn't, the cancel will happen at the next mainline - * CHECK_FOR_INTERRUPTS. - * - * Note: a longjmp from here is safe so far as our own data structures are - * concerned; but on platforms that block a signal before calling the - * handler and then un-block it on return, longjmping out of the signal - * handler leaves SIGALRM still blocked. Error cleanup is responsible for - * unblocking any blocked signals. - */ RESUME_INTERRUPTS(); - ImmediateInterruptOK = save_ImmediateInterruptOK; - if (save_ImmediateInterruptOK) - CHECK_FOR_INTERRUPTS(); errno = save_errno; } diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 6e33a17..9bc6b71 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -80,7 +80,6 @@ extern PGDLLIMPORT volatile bool ProcDiePending; extern volatile bool ClientConnectionLost; /* these are marked volatile because they are examined by signal handlers: */ -extern PGDLLIMPORT volatile bool ImmediateInterruptOK; extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 CritSectionCount; diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c index a9d9e0e..691a568 100644 --- a/src/test/modules/test_shm_mq/worker.c +++ b/src/test/modules/test_shm_mq/worker.c @@ -60,13 +60,9 @@ test_shm_mq_main(Datum main_arg) * * We want CHECK_FOR_INTERRUPTS() to kill off this worker process just as * it would a normal user backend. To make that happen, we establish a - * signal handler that is a stripped-down version of die(). We don't have - * any equivalent of the backend's command-read loop, where interrupts can - * be processed immediately, so make sure ImmediateInterruptOK is turned - * off. + * signal handler that is a stripped-down version of die(). */ pqsignal(SIGTERM, handle_sigterm); - ImmediateInterruptOK = false; BackgroundWorkerUnblockSignals(); /* -- 2.0.0.rc2.4.g1dc51c6.dirty