From 55e4ef138cc68cffd12df9fc9e5587590473a074 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 31 Oct 2002 21:34:17 +0000 Subject: [PATCH] Code review for statement_timeout patch. Fix some race conditions between signal handler and enable/disable code, avoid accumulation of timing error due to trying to maintain remaining-time instead of absolute-end-time, disable timeout before commit not after. --- src/backend/storage/lmgr/README | 6 +- src/backend/storage/lmgr/lock.c | 6 +- src/backend/storage/lmgr/proc.c | 300 ++++++++++++++++++-------------- src/backend/tcop/postgres.c | 28 ++- src/backend/utils/misc/guc.c | 3 +- src/include/storage/proc.h | 6 +- 6 files changed, 189 insertions(+), 160 deletions(-) diff --git a/src/backend/storage/lmgr/README b/src/backend/storage/lmgr/README index 3c783e7e5e..d04e7bb90f 100644 --- a/src/backend/storage/lmgr/README +++ b/src/backend/storage/lmgr/README @@ -1,4 +1,4 @@ -$Header: /cvsroot/pgsql/src/backend/storage/lmgr/README,v 1.11 2002/07/19 00:17:40 momjian Exp $ +$Header: /cvsroot/pgsql/src/backend/storage/lmgr/README,v 1.12 2002/10/31 21:34:16 tgl Exp $ LOCKING OVERVIEW @@ -392,7 +392,7 @@ Miscellaneous notes: asynchronous invocation of deadlock checking. A deadlock cycle in the WFG is formed when the last edge in the cycle is added; therefore the last process in the cycle to wait (the one from which that edge is outgoing) is -certain to detect and resolve the cycle when it later runs HandleDeadLock. +certain to detect and resolve the cycle when it later runs CheckDeadLock. This holds even if that edge addition created multiple cycles; the process may indeed abort without ever noticing those additional cycles, but we don't particularly care. The only other possible creation of deadlocks is @@ -402,7 +402,7 @@ it attempts to actually execute any rearrangement. 2. It is not certain that a deadlock will be resolved by aborting the last-to-wait process. If earlier waiters in the cycle have not yet run -HandleDeadLock, then the first one to do so will be the victim. +CheckDeadLock, then the first one to do so will be the victim. 3. No live (wakable) process can be missed by ProcLockWakeup, since it examines every member of the wait queue (this was not true in the 7.0 diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 3a2602777b..63531bbf3a 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v 1.116 2002/09/26 05:18:30 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/lock.c,v 1.117 2002/10/31 21:34:16 tgl Exp $ * * NOTES * Outside modules can create a lock table and acquire/release @@ -882,7 +882,7 @@ WaitOnLock(LOCKMETHOD lockmethod, LOCKMODE lockmode, /* * NOTE: Think not to put any shared-state cleanup after the call to * ProcSleep, in either the normal or failure path. The lock state - * must be fully set by the lock grantor, or by HandleDeadLock if we + * must be fully set by the lock grantor, or by CheckDeadLock if we * give up waiting for the lock. This is necessary because of the * possibility that a cancel/die interrupt will interrupt ProcSleep * after someone else grants us the lock, but before we've noticed it. @@ -899,7 +899,7 @@ WaitOnLock(LOCKMETHOD lockmethod, LOCKMODE lockmode, holder) != STATUS_OK) { /* - * We failed as a result of a deadlock, see HandleDeadLock(). Quit + * We failed as a result of a deadlock, see CheckDeadLock(). Quit * now. Removal of the holder and lock objects, if no longer * needed, will happen in xact cleanup (see above for motivation). */ diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 04f57707d1..4defa7ddbb 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v 1.126 2002/09/25 20:31:40 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/storage/lmgr/proc.c,v 1.127 2002/10/31 21:34:16 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -52,11 +52,11 @@ #include "storage/sinval.h" #include "storage/spin.h" +/* GUC variables */ int DeadlockTimeout = 1000; int StatementTimeout = 0; -int RemainingStatementTimeout = 0; -bool alarm_is_statement_timeout = false; +/* Pointer to this process's PGPROC struct, if any */ PGPROC *MyProc = NULL; /* @@ -75,8 +75,16 @@ static PGPROC *DummyProc = NULL; static bool waitingForLock = false; static bool waitingForSignal = false; +/* Mark these volatile because they can be changed by signal handler */ +static volatile bool statement_timeout_active = false; +static volatile bool deadlock_timeout_active = false; +/* statement_fin_time is valid only if statement_timeout_active is true */ +static struct timeval statement_fin_time; + + static void ProcKill(void); static void DummyProcKill(void); +static bool CheckStatementTimeout(void); /* @@ -796,14 +804,12 @@ ProcLockWakeup(LOCKMETHODTABLE *lockMethodTable, LOCK *lock) * and signal an error to ProcSleep. * -------------------- */ -void +static void CheckDeadLock(void) { - int save_errno = errno; - /* - * Acquire locktable lock. Note that the SIGALRM interrupt had better - * not be enabled anywhere that this process itself holds the + * Acquire locktable lock. Note that the deadlock check interrupt had + * better not be enabled anywhere that this process itself holds the * locktable lock, else this will wait forever. Also note that * LWLockAcquire creates a critical section, so that this routine * cannot be interrupted by cancel/die interrupts. @@ -821,13 +827,11 @@ CheckDeadLock(void) * This is quicker than checking our semaphore's state, since no * kernel call is needed, and it is safe because we hold the locktable * lock. - * */ if (MyProc->links.prev == INVALID_OFFSET || MyProc->links.next == INVALID_OFFSET) { LWLockRelease(LockMgrLock); - errno = save_errno; return; } @@ -840,7 +844,6 @@ CheckDeadLock(void) { /* No deadlock, so keep waiting */ LWLockRelease(LockMgrLock); - errno = save_errno; return; } @@ -853,7 +856,7 @@ CheckDeadLock(void) /* * Set MyProc->errType to STATUS_ERROR so that ProcSleep will report - * an error after we return from this signal handler. + * an error after we return from the signal handler. */ MyProc->errType = STATUS_ERROR; @@ -874,7 +877,6 @@ CheckDeadLock(void) * RemoveFromWaitQueue took care of waking up any such processes. */ LWLockRelease(LockMgrLock); - errno = save_errno; } @@ -933,188 +935,218 @@ ProcSendSignal(BackendId procId) * Delay is given in milliseconds. Caller should be sure a SIGALRM * signal handler is installed before this is called. * - * This code properly handles multiple alarms when the statement_timeout - * alarm is specified first. + * This code properly handles nesting of deadlock timeout alarms within + * statement timeout alarms. * * Returns TRUE if okay, FALSE on failure. */ bool enable_sig_alarm(int delayms, bool is_statement_timeout) { + struct timeval fin_time; #ifndef __BEOS__ - struct itimerval timeval, - remaining; - + struct itimerval timeval; #else - bigtime_t time_interval, - remaining; + bigtime_t time_interval; #endif - /* - * Don't set timer if the statement timeout scheduled before next - * alarm. - */ - if (alarm_is_statement_timeout && - !is_statement_timeout && - RemainingStatementTimeout <= delayms) - return true; + /* Compute target timeout time if we will need it */ + if (is_statement_timeout || statement_timeout_active) + { + gettimeofday(&fin_time, NULL); + fin_time.tv_sec += delayms / 1000; + fin_time.tv_usec += (delayms % 1000) * 1000; + if (fin_time.tv_usec >= 1000000) + { + fin_time.tv_sec++; + fin_time.tv_usec -= 1000000; + } + } + + if (is_statement_timeout) + { + /* Begin statement-level timeout */ + Assert(!deadlock_timeout_active); + statement_fin_time = fin_time; + statement_timeout_active = true; + } + else if (statement_timeout_active) + { + /* + * Begin deadlock timeout with statement-level timeout active + * + * Here, we want to interrupt at the closer of the two timeout + * times. If fin_time >= statement_fin_time then we need not + * touch the existing timer setting; else set up to interrupt + * at the deadlock timeout time. + * + * NOTE: in this case it is possible that this routine will be + * interrupted by the previously-set timer alarm. This is okay + * because the signal handler will do only what it should do according + * to the state variables. The deadlock checker may get run earlier + * than normal, but that does no harm. + */ + deadlock_timeout_active = true; + if (fin_time.tv_sec > statement_fin_time.tv_sec || + (fin_time.tv_sec == statement_fin_time.tv_sec && + fin_time.tv_usec >= statement_fin_time.tv_usec)) + return true; + } + else + { + /* Begin deadlock timeout with no statement-level timeout */ + deadlock_timeout_active = true; + } + /* If we reach here, okay to set the timer interrupt */ #ifndef __BEOS__ MemSet(&timeval, 0, sizeof(struct itimerval)); timeval.it_value.tv_sec = delayms / 1000; timeval.it_value.tv_usec = (delayms % 1000) * 1000; - if (setitimer(ITIMER_REAL, &timeval, &remaining)) + if (setitimer(ITIMER_REAL, &timeval, NULL)) return false; #else /* BeOS doesn't have setitimer, but has set_alarm */ time_interval = delayms * 1000; /* usecs */ - if ((remaining = set_alarm(time_interval, B_ONE_SHOT_RELATIVE_ALARM)) < 0) + if (set_alarm(time_interval, B_ONE_SHOT_RELATIVE_ALARM) < 0) return false; #endif - if (is_statement_timeout) - RemainingStatementTimeout = StatementTimeout; - else + return true; +} + +/* + * Cancel the SIGALRM timer, either for a deadlock timeout or a statement + * timeout. If a deadlock timeout is canceled, any active statement timeout + * remains in force. + * + * Returns TRUE if okay, FALSE on failure. + */ +bool +disable_sig_alarm(bool is_statement_timeout) +{ + /* + * Always disable the interrupt if it is active; this avoids being + * interrupted by the signal handler and thereby possibly getting + * confused. + * + * We will re-enable the interrupt if necessary in CheckStatementTimeout. + */ + if (statement_timeout_active || deadlock_timeout_active) { - /* Switching to non-statement-timeout alarm, get remaining time */ - if (alarm_is_statement_timeout) - { #ifndef __BEOS__ - /* We lose precision here because we convert to milliseconds */ - RemainingStatementTimeout = remaining.it_value.tv_sec * 1000 + - remaining.it_value.tv_usec / 1000; -#else - RemainingStatementTimeout = remaining / 1000; -#endif - /* Rounding could cause a zero */ - if (RemainingStatementTimeout == 0) - RemainingStatementTimeout = 1; - } + struct itimerval timeval; - if (RemainingStatementTimeout) + MemSet(&timeval, 0, sizeof(struct itimerval)); + if (setitimer(ITIMER_REAL, &timeval, NULL)) { - /* Remaining timeout alarm < delayms? */ - if (RemainingStatementTimeout <= delayms) - { - /* reinstall statement timeout alarm */ - alarm_is_statement_timeout = true; -#ifndef __BEOS__ - remaining.it_value.tv_sec = RemainingStatementTimeout / 1000; - remaining.it_value.tv_usec = (RemainingStatementTimeout % 1000) * 1000; - if (setitimer(ITIMER_REAL, &remaining, &timeval)) - return false; - else - return true; + statement_timeout_active = deadlock_timeout_active = false; + return false; + } #else - remaining = RemainingStatementTimeout * 1000; - if ((timeval = set_alarm(remaining, B_ONE_SHOT_RELATIVE_ALARM)) < 0) - return false; - else - return true; -#endif - } - else - RemainingStatementTimeout -= delayms; + /* BeOS doesn't have setitimer, but has set_alarm */ + if (set_alarm(B_INFINITE_TIMEOUT, B_PERIODIC_ALARM) < 0) + { + statement_timeout_active = deadlock_timeout_active = false; + return false; } +#endif } + /* Always cancel deadlock timeout, in case this is error cleanup */ + deadlock_timeout_active = false; + + /* Cancel or reschedule statement timeout */ if (is_statement_timeout) - alarm_is_statement_timeout = true; - else - alarm_is_statement_timeout = false; + statement_timeout_active = false; + else if (statement_timeout_active) + { + if (!CheckStatementTimeout()) + return false; + } return true; } + /* - * Cancel the SIGALRM timer. + * Check for statement timeout. If the timeout time has come, + * trigger a query-cancel interrupt; if not, reschedule the SIGALRM + * interrupt to occur at the right time. * - * This is also called if the timer has fired to reschedule - * the statement_timeout timer. - * - * Returns TRUE if okay, FALSE on failure. + * Returns true if okay, false if failed to set the interrupt. */ -bool -disable_sig_alarm(bool is_statement_timeout) +static bool +CheckStatementTimeout(void) { -#ifndef __BEOS__ - struct itimerval timeval, - remaining; + struct timeval now; - MemSet(&timeval, 0, sizeof(struct itimerval)); -#else - bigtime_t time_interval = 0; -#endif + if (!statement_timeout_active) + return true; /* do nothing if not active */ + + gettimeofday(&now, NULL); - if (!is_statement_timeout && RemainingStatementTimeout) + if (now.tv_sec > statement_fin_time.tv_sec || + (now.tv_sec == statement_fin_time.tv_sec && + now.tv_usec >= statement_fin_time.tv_usec)) { + /* Time to die */ + statement_timeout_active = false; + kill(MyProcPid, SIGINT); + } + else + { + /* Not time yet, so (re)schedule the interrupt */ #ifndef __BEOS__ - /* turn off timer and get remaining time, if any */ - if (setitimer(ITIMER_REAL, &timeval, &remaining)) + struct itimerval timeval; + + MemSet(&timeval, 0, sizeof(struct itimerval)); + timeval.it_value.tv_sec = statement_fin_time.tv_sec - now.tv_sec; + timeval.it_value.tv_usec = statement_fin_time.tv_usec - now.tv_usec; + if (timeval.it_value.tv_usec < 0) + { + timeval.it_value.tv_sec--; + timeval.it_value.tv_usec += 1000000; + } + if (setitimer(ITIMER_REAL, &timeval, NULL)) return false; - /* Add remaining time back because the timer didn't complete */ - RemainingStatementTimeout += remaining.it_value.tv_sec * 1000 + - remaining.it_value.tv_usec / 1000; - /* Prepare to set timer */ - timeval.it_value.tv_sec = RemainingStatementTimeout / 1000; - timeval.it_value.tv_usec = (RemainingStatementTimeout % 1000) * 1000; #else /* BeOS doesn't have setitimer, but has set_alarm */ - if ((time_interval = set_alarm(B_INFINITE_TIMEOUT, B_PERIODIC_ALARM)) < 0) - return false; - RemainingStatementTimeout += time_interval / 1000; - time_interval = RemainingStatementTimeout * 1000; -#endif - /* Restore remaining statement timeout value */ - alarm_is_statement_timeout = true; - } + bigtime_t time_interval; - /* - * Optimization: is_statement_timeout && RemainingStatementTimeout == - * 0 does nothing. This is for cases where no timeout was set. - */ - if (!is_statement_timeout || RemainingStatementTimeout) - { -#ifndef __BEOS__ - if (setitimer(ITIMER_REAL, &timeval, &remaining)) + time_interval = + (statement_fin_time.tv_sec - now.tv_sec) * 1000000 + + (statement_fin_time.tv_usec - now.tv_usec); + if (set_alarm(time_interval, B_ONE_SHOT_RELATIVE_ALARM) < 0) return false; -#else - if (time_interval) - { - if (set_alarm(time_interval, B_ONE_SHOT_RELATIVE_ALARM) < 0) - return false; - } - else - { - if (set_alarm(B_INFINITE_TIMEOUT, B_PERIODIC_ALARM) < 0) - return false; - } #endif } - if (is_statement_timeout) - RemainingStatementTimeout = 0; - return true; } /* - * Call alarm handler, either StatementCancel or Deadlock checker. + * Signal handler for SIGALRM + * + * Process deadlock check and/or statement timeout check, as needed. + * To avoid various edge cases, we must be careful to do nothing + * when there is nothing to be done. We also need to be able to + * reschedule the timer interrupt if called before end of statement. */ void handle_sig_alarm(SIGNAL_ARGS) { - if (alarm_is_statement_timeout) - { - RemainingStatementTimeout = 0; - alarm_is_statement_timeout = false; - kill(MyProcPid, SIGINT); - } - else + int save_errno = errno; + + if (deadlock_timeout_active) { + deadlock_timeout_active = false; CheckDeadLock(); - /* Reactivate any statement_timeout alarm. */ - disable_sig_alarm(false); } + + if (statement_timeout_active) + (void) CheckStatementTimeout(); + + errno = save_errno; } diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 393b6955ce..82eebee4fa 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/backend/tcop/postgres.c,v 1.306 2002/10/24 23:19:13 tgl Exp $ + * $Header: /cvsroot/pgsql/src/backend/tcop/postgres.c,v 1.307 2002/10/31 21:34:16 tgl Exp $ * * NOTES * this is the "main" module of the postgres backend and @@ -75,8 +75,6 @@ char *debug_query_string; /* for pgmonitor and /* Note: whereToSendOutput is initialized for the bootstrap/standalone case */ CommandDest whereToSendOutput = Debug; -extern int StatementTimeout; - static bool dontExecute = false; /* note: these declarations had better match tcopprot.h */ @@ -582,9 +580,6 @@ pg_exec_query_string(StringInfo query_string, /* string to execute */ start_xact_command(); xact_started = true; - if (StatementTimeout) - enable_sig_alarm(StatementTimeout, true); - /* * parse_context *must* be different from the execution memory * context, else the context reset at the bottom of the loop will @@ -931,8 +926,6 @@ pg_exec_query_string(StringInfo query_string, /* string to execute */ EndCommand(commandTag, dest); } /* end loop over parsetrees */ - disable_sig_alarm(true); - /* * Close down transaction statement, if one is open. (Note that this * will only happen if the querystring was empty.) @@ -964,6 +957,10 @@ start_xact_command(void) { elog(DEBUG1, "StartTransactionCommand"); StartTransactionCommand(false); + + /* Set statement timeout running, if any */ + if (StatementTimeout > 0) + enable_sig_alarm(StatementTimeout, true); } static void @@ -972,6 +969,9 @@ finish_xact_command(bool forceCommit) /* Invoke IMMEDIATE constraint triggers */ DeferredTriggerEndQuery(); + /* Cancel any active statement timeout before committing */ + disable_sig_alarm(true); + /* Now commit the command */ elog(DEBUG1, "CommitTransactionCommand"); @@ -1047,7 +1047,7 @@ die(SIGNAL_ARGS) /* until we are done getting ready for it */ InterruptHoldoffCount++; DisableNotifyInterrupt(); - /* Make sure HandleDeadLock won't run while shutting down... */ + /* Make sure CheckDeadLock won't run while shutting down... */ LockWaitCancel(); InterruptHoldoffCount--; ProcessInterrupts(); @@ -1648,8 +1648,7 @@ PostgresMain(int argc, char *argv[], const char *username) pqsignal(SIGINT, StatementCancelHandler); /* cancel current query */ pqsignal(SIGTERM, die); /* cancel current query and exit */ pqsignal(SIGQUIT, quickdie); /* hard crash time */ - pqsignal(SIGALRM, handle_sig_alarm); /* check for deadlock - * after timeout */ + pqsignal(SIGALRM, handle_sig_alarm); /* timeout conditions */ /* * Ignore failure to write to frontend. Note: if frontend closes @@ -1782,7 +1781,7 @@ PostgresMain(int argc, char *argv[], const char *username) if (!IsUnderPostmaster) { puts("\nPOSTGRES backend interactive interface "); - puts("$Revision: 1.306 $ $Date: 2002/10/24 23:19:13 $\n"); + puts("$Revision: 1.307 $ $Date: 2002/10/31 21:34:16 $\n"); } /* @@ -1829,6 +1828,8 @@ PostgresMain(int argc, char *argv[], const char *username) QueryCancelPending = false; InterruptHoldoffCount = 1; CritSectionCount = 0; /* should be unnecessary, but... */ + disable_sig_alarm(true); + QueryCancelPending = false; /* again in case timeout occurred */ DisableNotifyInterrupt(); debug_query_string = NULL; @@ -1915,9 +1916,6 @@ PostgresMain(int argc, char *argv[], const char *username) QueryCancelPending = false; /* forget any earlier CANCEL * signal */ - /* Stop any statement timer */ - disable_sig_alarm(true); - EnableNotifyInterrupt(); /* Allow "die" interrupt to be processed while waiting */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 2e386afab1..c4e3f53042 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5,7 +5,7 @@ * command, configuration file, and command line options. * See src/backend/utils/misc/README for more information. * - * $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.97 2002/10/02 16:27:57 momjian Exp $ + * $Header: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v 1.98 2002/10/31 21:34:17 tgl Exp $ * * Copyright 2000 by PostgreSQL Global Development Group * Written by Peter Eisentraut . @@ -56,7 +56,6 @@ extern bool Log_connections; extern int PreAuthDelay; extern int AuthenticationTimeout; -extern int StatementTimeout; extern int CheckPointTimeout; extern bool autocommit; extern int CommitDelay; diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index dd81713599..5cc4b7c968 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $Id: proc.h,v 1.61 2002/10/21 18:57:34 petere Exp $ + * $Id: proc.h,v 1.62 2002/10/31 21:34:17 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -86,8 +86,9 @@ typedef struct PROC_HDR } PROC_HDR; -/* configurable option */ +/* configurable options */ extern int DeadlockTimeout; +extern int StatementTimeout; /* @@ -105,7 +106,6 @@ extern int ProcSleep(LOCKMETHODTABLE *lockMethodTable, LOCKMODE lockmode, extern PGPROC *ProcWakeup(PGPROC *proc, int errType); extern void ProcLockWakeup(LOCKMETHODTABLE *lockMethodTable, LOCK *lock); extern bool LockWaitCancel(void); -extern void CheckDeadLock(void); extern void ProcWaitForSignal(void); extern void ProcCancelWaitForSignal(void); -- GitLab