From d43837d03067487560af481474ae985df894f786 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 16 Mar 2013 23:22:17 -0400 Subject: [PATCH] Add lock_timeout configuration parameter. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This GUC allows limiting the time spent waiting to acquire any one heavyweight lock. In support of this, improve the recently-added timeout infrastructure to permit efficiently enabling or disabling multiple timeouts at once. That reduces the performance hit from turning on lock_timeout, though it's still not zero. Zoltán Böszörményi, reviewed by Tom Lane, Stephen Frost, and Hari Babu --- doc/src/sgml/config.sgml | 40 ++- src/backend/postmaster/autovacuum.c | 10 +- src/backend/storage/ipc/standby.c | 11 +- src/backend/storage/lmgr/proc.c | 50 ++- src/backend/tcop/postgres.c | 17 +- src/backend/utils/init/postinit.c | 18 ++ src/backend/utils/misc/guc.c | 11 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/backend/utils/misc/timeout.c | 303 ++++++++++++------ src/bin/pg_dump/pg_backup_archiver.c | 5 +- src/bin/pg_dump/pg_dump.c | 2 + src/include/storage/proc.h | 5 +- src/include/utils/timeout.h | 35 +- src/test/isolation/expected/timeouts.out | 73 +++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/timeouts.spec | 45 +++ 16 files changed, 511 insertions(+), 116 deletions(-) create mode 100644 src/test/isolation/expected/timeouts.out create mode 100644 src/test/isolation/specs/timeouts.spec diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 575b40b58d..8c520e1267 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -5077,7 +5077,7 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; - Abort any statement that takes over the specified number of + Abort any statement that takes more than the specified number of milliseconds, starting from the time the command arrives at the server from the client. If log_min_error_statement is set to ERROR or lower, the statement that timed out will also be @@ -5086,8 +5086,42 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; Setting statement_timeout in - postgresql.conf is not recommended because it - affects all sessions. + postgresql.conf is not recommended because it would + affect all sessions. + + + + + + lock_timeout (integer) + + lock_timeout configuration parameter + + + + Abort any statement that waits longer than the specified number of + milliseconds while attempting to acquire a lock on a table, index, + row, or other database object. The time limit applies separately to + each lock acquisition attempt. The limit applies both to explicit + locking requests (such as LOCK TABLE, or SELECT + FOR UPDATE without NOWAIT) and to implicitly-acquired + locks. If log_min_error_statement is set to + ERROR or lower, the statement that timed out will be + logged. A value of zero (the default) turns this off. + + + + Unlike statement_timeout, this timeout can only occur + while waiting for locks. Note that if statement_timeout + is nonzero, it is rather pointless to set lock_timeout to + the same or larger value, since the statement timeout would always + trigger first. + + + + Setting lock_timeout in + postgresql.conf is not recommended because it would + affect all sessions. diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 00cb3f760d..b4af6972c4 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -547,10 +547,11 @@ AutoVacLauncherMain(int argc, char *argv[]) SetConfigOption("zero_damaged_pages", "false", PGC_SUSET, PGC_S_OVERRIDE); /* - * Force statement_timeout to zero to avoid a timeout setting from - * preventing regular maintenance from being executed. + * Force statement_timeout and lock_timeout to zero to avoid letting these + * settings prevent regular maintenance from being executed. */ SetConfigOption("statement_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE); + SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE); /* * Force default_transaction_isolation to READ COMMITTED. We don't want @@ -1573,10 +1574,11 @@ AutoVacWorkerMain(int argc, char *argv[]) SetConfigOption("zero_damaged_pages", "false", PGC_SUSET, PGC_S_OVERRIDE); /* - * Force statement_timeout to zero to avoid a timeout setting from - * preventing regular maintenance from being executed. + * Force statement_timeout and lock_timeout to zero to avoid letting these + * settings prevent regular maintenance from being executed. */ SetConfigOption("statement_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE); + SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE); /* * Force default_transaction_isolation to READ COMMITTED. We don't want diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index a903f12766..fcf08f42b3 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -428,8 +428,15 @@ ResolveRecoveryConflictWithBufferPin(void) * Wake up at ltime, and check for deadlocks as well if we will be * waiting longer than deadlock_timeout */ - enable_timeout_after(STANDBY_DEADLOCK_TIMEOUT, DeadlockTimeout); - enable_timeout_at(STANDBY_TIMEOUT, ltime); + EnableTimeoutParams timeouts[2]; + + timeouts[0].id = STANDBY_TIMEOUT; + timeouts[0].type = TMPARAM_AT; + timeouts[0].fin_time = ltime; + timeouts[1].id = STANDBY_DEADLOCK_TIMEOUT; + timeouts[1].type = TMPARAM_AFTER; + timeouts[1].delay_ms = DeadlockTimeout; + enable_timeouts(timeouts, 2); } /* Wait to be signaled by UnpinBuffer() */ diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 2e012fad11..5809a79798 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -55,6 +55,7 @@ /* GUC variables */ int DeadlockTimeout = 1000; int StatementTimeout = 0; +int LockTimeout = 0; bool log_lock_waits = false; /* Pointer to this process's PGPROC and PGXACT structs, if any */ @@ -665,6 +666,7 @@ void LockErrorCleanup(void) { LWLockId partitionLock; + DisableTimeoutParams timeouts[2]; AbortStrongLockAcquire(); @@ -672,8 +674,19 @@ LockErrorCleanup(void) if (lockAwaited == NULL) return; - /* Turn off the deadlock timer, if it's still running (see ProcSleep) */ - disable_timeout(DEADLOCK_TIMEOUT, false); + /* + * Turn off the deadlock and lock timeout timers, if they are still + * running (see ProcSleep). Note we must preserve the LOCK_TIMEOUT + * indicator flag, since this function is executed before + * ProcessInterrupts when responding to SIGINT; else we'd lose the + * knowledge that the SIGINT came from a lock timeout and not an external + * source. + */ + timeouts[0].id = DEADLOCK_TIMEOUT; + timeouts[0].keep_indicator = false; + timeouts[1].id = LOCK_TIMEOUT; + timeouts[1].keep_indicator = true; + disable_timeouts(timeouts, 2); /* Unlink myself from the wait queue, if on it (might not be anymore!) */ partitionLock = LockHashPartitionLock(lockAwaited->hashcode); @@ -1072,8 +1085,24 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) * * By delaying the check until we've waited for a bit, we can avoid * running the rather expensive deadlock-check code in most cases. + * + * If LockTimeout is set, also enable the timeout for that. We can save a + * few cycles by enabling both timeout sources in one call. */ - enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout); + if (LockTimeout > 0) + { + EnableTimeoutParams timeouts[2]; + + timeouts[0].id = DEADLOCK_TIMEOUT; + timeouts[0].type = TMPARAM_AFTER; + timeouts[0].delay_ms = DeadlockTimeout; + timeouts[1].id = LOCK_TIMEOUT; + timeouts[1].type = TMPARAM_AFTER; + timeouts[1].delay_ms = LockTimeout; + enable_timeouts(timeouts, 2); + } + else + enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout); /* * If someone wakes us between LWLockRelease and PGSemaphoreLock, @@ -1240,9 +1269,20 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) } while (myWaitStatus == STATUS_WAITING); /* - * Disable the timer, if it's still running + * Disable the timers, if they are still running */ - disable_timeout(DEADLOCK_TIMEOUT, false); + if (LockTimeout > 0) + { + DisableTimeoutParams timeouts[2]; + + timeouts[0].id = DEADLOCK_TIMEOUT; + timeouts[0].keep_indicator = false; + timeouts[1].id = LOCK_TIMEOUT; + timeouts[1].keep_indicator = false; + disable_timeouts(timeouts, 2); + } + else + disable_timeout(DEADLOCK_TIMEOUT, false); /* * Re-acquire the lock table's partition lock. We have to do this to hold diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 407c548cf8..587d065f1c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -2883,7 +2883,22 @@ ProcessInterrupts(void) (errcode(ERRCODE_QUERY_CANCELED), errmsg("canceling authentication due to timeout"))); } - if (get_timeout_indicator(STATEMENT_TIMEOUT)) + + /* + * If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we + * prefer to report the former; but be sure to clear both. + */ + if (get_timeout_indicator(LOCK_TIMEOUT, true)) + { + ImmediateInterruptOK = false; /* not idle anymore */ + (void) get_timeout_indicator(STATEMENT_TIMEOUT, true); + DisableNotifyInterrupt(); + DisableCatchupInterrupt(); + ereport(ERROR, + (errcode(ERRCODE_QUERY_CANCELED), + errmsg("canceling statement due to lock timeout"))); + } + if (get_timeout_indicator(STATEMENT_TIMEOUT, true)) { ImmediateInterruptOK = false; /* not idle anymore */ DisableNotifyInterrupt(); diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 84270061d8..5f8f98b5e0 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -67,6 +67,7 @@ static void CheckMyDatabase(const char *name, bool am_superuser); static void InitCommunication(void); static void ShutdownPostgres(int code, Datum arg); static void StatementTimeoutHandler(void); +static void LockTimeoutHandler(void); static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); @@ -535,6 +536,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, { RegisterTimeout(DEADLOCK_TIMEOUT, CheckDeadLock); RegisterTimeout(STATEMENT_TIMEOUT, StatementTimeoutHandler); + RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler); } /* @@ -1052,6 +1054,22 @@ StatementTimeoutHandler(void) kill(MyProcPid, SIGINT); } +/* + * LOCK_TIMEOUT handler: trigger a query-cancel interrupt. + * + * This is identical to StatementTimeoutHandler, but since it's so short, + * we might as well keep the two functions separate for clarity. + */ +static void +LockTimeoutHandler(void) +{ +#ifdef HAVE_SETSID + /* try to signal whole process group */ + kill(-MyProcPid, SIGINT); +#endif + kill(MyProcPid, SIGINT); +} + /* * Returns true if at least one role is defined in this database cluster. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 98149fc10f..5246fc5b20 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1861,6 +1861,17 @@ static struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"lock_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Sets the maximum allowed duration of any wait for a lock."), + gettext_noop("A value of 0 turns off the timeout."), + GUC_UNIT_MS + }, + &LockTimeout, + 0, 0, INT_MAX, + NULL, NULL, NULL + }, + { {"vacuum_freeze_min_age", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Minimum age at which VACUUM should freeze a table row."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 62aea2f583..307b456f03 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -489,6 +489,7 @@ #default_transaction_deferrable = off #session_replication_role = 'origin' #statement_timeout = 0 # in milliseconds, 0 is disabled +#lock_timeout = 0 # in milliseconds, 0 is disabled #vacuum_freeze_min_age = 50000000 #vacuum_freeze_table_age = 150000000 #bytea_output = 'hex' # hex, escape diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c index 944c9a7bcb..2ee6e00ed2 100644 --- a/src/backend/utils/misc/timeout.c +++ b/src/backend/utils/misc/timeout.c @@ -31,7 +31,7 @@ typedef struct timeout_params volatile bool indicator; /* true if timeout has occurred */ /* callback function for timeout, or NULL if timeout not registered */ - timeout_handler timeout_handler; + timeout_handler_proc timeout_handler; TimestampTz start_time; /* time that timeout was last activated */ TimestampTz fin_time; /* if active, time it is due to fire */ @@ -55,9 +55,48 @@ static timeout_params *volatile active_timeouts[MAX_TIMEOUTS]; * Internal helper functions * * For all of these, it is caller's responsibility to protect them from - * interruption by the signal handler. + * interruption by the signal handler. Generally, call disable_alarm() + * first to prevent interruption, then update state, and last call + * schedule_alarm(), which will re-enable the interrupt if needed. *****************************************************************************/ +/* + * Disable alarm interrupts + * + * multi_insert must be true if the caller intends to activate multiple new + * timeouts. Otherwise it should be false. + */ +static void +disable_alarm(bool multi_insert) +{ + struct itimerval timeval; + + /* + * If num_active_timeouts is zero, and multi_insert is false, we don't + * have to call setitimer. There should not be any pending interrupt, and + * even if there is, the worst possible case is that the signal handler + * fires during schedule_alarm. (If it fires at any point before + * insert_timeout has incremented num_active_timeouts, it will do nothing, + * since it sees no active timeouts.) In that case we could end up + * scheduling a useless interrupt ... but when the extra interrupt does + * happen, the signal handler will do nothing, so it's all good. + * + * However, if the caller intends to do anything more after first calling + * insert_timeout, the above argument breaks down, since the signal + * handler could interrupt the subsequent operations leading to corrupt + * state. Out of an abundance of caution, we forcibly disable the timer + * even though it should be off already, just to be sure. Even though + * this setitimer call is probably useless, we're still ahead of the game + * compared to scheduling two or more timeouts independently. + */ + if (multi_insert || num_active_timeouts > 0) + { + MemSet(&timeval, 0, sizeof(struct itimerval)); + if (setitimer(ITIMER_REAL, &timeval, NULL) != 0) + elog(FATAL, "could not disable SIGALRM timer: %m"); + } +} + /* * Find the index of a given timeout reason in the active array. * If it's not there, return -1. @@ -94,7 +133,7 @@ insert_timeout(TimeoutId id, int index) active_timeouts[index] = &all_timeouts[id]; - /* NB: this must be the last step, see comments in enable_timeout */ + /* NB: this must be the last step, see comments in disable_alarm */ num_active_timeouts++; } @@ -116,6 +155,49 @@ remove_timeout_index(int index) num_active_timeouts--; } +/* + * Enable the specified timeout reason + */ +static void +enable_timeout(TimeoutId id, TimestampTz now, TimestampTz fin_time) +{ + int i; + + /* Assert request is sane */ + Assert(all_timeouts_initialized); + Assert(all_timeouts[id].timeout_handler != NULL); + + /* + * If this timeout was already active, momentarily disable it. We + * interpret the call as a directive to reschedule the timeout. + */ + i = find_active_timeout(id); + if (i >= 0) + remove_timeout_index(i); + + /* + * Find out the index where to insert the new timeout. We sort by + * fin_time, and for equal fin_time by priority. + */ + for (i = 0; i < num_active_timeouts; i++) + { + timeout_params *old_timeout = active_timeouts[i]; + + if (fin_time < old_timeout->fin_time) + break; + if (fin_time == old_timeout->fin_time && id < old_timeout->index) + break; + } + + /* + * Mark the timeout active, and insert it into the active list. + */ + all_timeouts[id].indicator = false; + all_timeouts[id].start_time = now; + all_timeouts[id].fin_time = fin_time; + insert_timeout(id, i); +} + /* * Schedule alarm for the next active timeout, if any * @@ -260,7 +342,7 @@ InitializeTimeouts(void) * return a timeout ID. */ TimeoutId -RegisterTimeout(TimeoutId id, timeout_handler handler) +RegisterTimeout(TimeoutId id, timeout_handler_proc handler) { Assert(all_timeouts_initialized); @@ -283,75 +365,6 @@ RegisterTimeout(TimeoutId id, timeout_handler handler) return id; } -/* - * Enable the specified timeout reason - */ -static void -enable_timeout(TimeoutId id, TimestampTz now, TimestampTz fin_time) -{ - struct itimerval timeval; - int i; - - /* Assert request is sane */ - Assert(all_timeouts_initialized); - Assert(all_timeouts[id].timeout_handler != NULL); - - /* - * Disable the timer if it is active; this avoids getting interrupted by - * the signal handler and thereby possibly getting confused. We will - * re-enable the interrupt below. - * - * If num_active_timeouts is zero, we don't have to call setitimer. There - * should not be any pending interrupt, and even if there is, the worst - * possible case is that the signal handler fires during schedule_alarm. - * (If it fires at any point before insert_timeout has incremented - * num_active_timeouts, it will do nothing.) In that case we could end up - * scheduling a useless interrupt ... but when the interrupt does happen, - * the signal handler will do nothing, so it's all good. - */ - if (num_active_timeouts > 0) - { - MemSet(&timeval, 0, sizeof(struct itimerval)); - if (setitimer(ITIMER_REAL, &timeval, NULL) != 0) - elog(FATAL, "could not disable SIGALRM timer: %m"); - } - - /* - * If this timeout was already active, momentarily disable it. We - * interpret the call as a directive to reschedule the timeout. - */ - i = find_active_timeout(id); - if (i >= 0) - remove_timeout_index(i); - - /* - * Find out the index where to insert the new timeout. We sort by - * fin_time, and for equal fin_time by priority. - */ - for (i = 0; i < num_active_timeouts; i++) - { - timeout_params *old_timeout = active_timeouts[i]; - - if (fin_time < old_timeout->fin_time) - break; - if (fin_time == old_timeout->fin_time && id < old_timeout->index) - break; - } - - /* - * Activate the timeout. - */ - all_timeouts[id].indicator = false; - all_timeouts[id].start_time = now; - all_timeouts[id].fin_time = fin_time; - insert_timeout(id, i); - - /* - * Set the timer. - */ - schedule_alarm(now); -} - /* * Enable the specified timeout to fire after the specified delay. * @@ -363,10 +376,16 @@ enable_timeout_after(TimeoutId id, int delay_ms) TimestampTz now; TimestampTz fin_time; + /* Disable timeout interrupts for safety. */ + disable_alarm(false); + + /* Queue the timeout at the appropriate time. */ now = GetCurrentTimestamp(); fin_time = TimestampTzPlusMilliseconds(now, delay_ms); - enable_timeout(id, now, fin_time); + + /* Set the timer interrupt. */ + schedule_alarm(now); } /* @@ -379,7 +398,64 @@ enable_timeout_after(TimeoutId id, int delay_ms) void enable_timeout_at(TimeoutId id, TimestampTz fin_time) { - enable_timeout(id, GetCurrentTimestamp(), fin_time); + TimestampTz now; + + /* Disable timeout interrupts for safety. */ + disable_alarm(false); + + /* Queue the timeout at the appropriate time. */ + now = GetCurrentTimestamp(); + enable_timeout(id, now, fin_time); + + /* Set the timer interrupt. */ + schedule_alarm(now); +} + +/* + * Enable multiple timeouts at once. + * + * This works like calling enable_timeout_after() and/or enable_timeout_at() + * multiple times. Use this to reduce the number of GetCurrentTimestamp() + * and setitimer() calls needed to establish multiple timeouts. + */ +void +enable_timeouts(const EnableTimeoutParams *timeouts, int count) +{ + TimestampTz now; + int i; + + /* Disable timeout interrupts for safety. */ + disable_alarm(count > 1); + + /* Queue the timeout(s) at the appropriate times. */ + now = GetCurrentTimestamp(); + + for (i = 0; i < count; i++) + { + TimeoutId id = timeouts[i].id; + TimestampTz fin_time; + + switch (timeouts[i].type) + { + case TMPARAM_AFTER: + fin_time = TimestampTzPlusMilliseconds(now, + timeouts[i].delay_ms); + enable_timeout(id, now, fin_time); + break; + + case TMPARAM_AT: + enable_timeout(id, now, timeouts[i].fin_time); + break; + + default: + elog(ERROR, "unrecognized timeout type %d", + (int) timeouts[i].type); + break; + } + } + + /* Set the timer interrupt. */ + schedule_alarm(now); } /* @@ -394,29 +470,14 @@ enable_timeout_at(TimeoutId id, TimestampTz fin_time) void disable_timeout(TimeoutId id, bool keep_indicator) { - struct itimerval timeval; int i; /* Assert request is sane */ Assert(all_timeouts_initialized); Assert(all_timeouts[id].timeout_handler != NULL); - /* - * Disable the timer if it is active; this avoids getting interrupted by - * the signal handler and thereby possibly getting confused. We will - * re-enable the interrupt if necessary below. - * - * If num_active_timeouts is zero, we don't have to call setitimer. There - * should not be any pending interrupt, and even if there is, the signal - * handler will not do anything. In this situation the only thing we - * really have to do is reset the timeout's indicator. - */ - if (num_active_timeouts > 0) - { - MemSet(&timeval, 0, sizeof(struct itimerval)); - if (setitimer(ITIMER_REAL, &timeval, NULL) != 0) - elog(FATAL, "could not disable SIGALRM timer: %m"); - } + /* Disable timeout interrupts for safety. */ + disable_alarm(false); /* Find the timeout and remove it from the active list. */ i = find_active_timeout(id); @@ -427,7 +488,48 @@ disable_timeout(TimeoutId id, bool keep_indicator) if (!keep_indicator) all_timeouts[id].indicator = false; - /* Now re-enable the timer, if necessary. */ + /* Reschedule the interrupt, if any timeouts remain active. */ + if (num_active_timeouts > 0) + schedule_alarm(GetCurrentTimestamp()); +} + +/* + * Cancel multiple timeouts at once. + * + * The timeouts' I've-been-fired indicators are reset, + * unless timeouts[i].keep_indicator is true. + * + * This works like calling disable_timeout() multiple times. + * Use this to reduce the number of GetCurrentTimestamp() + * and setitimer() calls needed to cancel multiple timeouts. + */ +void +disable_timeouts(const DisableTimeoutParams *timeouts, int count) +{ + int i; + + Assert(all_timeouts_initialized); + + /* Disable timeout interrupts for safety. */ + disable_alarm(false); + + /* Cancel the timeout(s). */ + for (i = 0; i < count; i++) + { + TimeoutId id = timeouts[i].id; + int idx; + + Assert(all_timeouts[id].timeout_handler != NULL); + + idx = find_active_timeout(id); + if (idx >= 0) + remove_timeout_index(idx); + + if (!timeouts[i].keep_indicator) + all_timeouts[id].indicator = false; + } + + /* Reschedule the interrupt, if any timeouts remain active. */ if (num_active_timeouts > 0) schedule_alarm(GetCurrentTimestamp()); } @@ -442,6 +544,7 @@ disable_all_timeouts(bool keep_indicators) struct itimerval timeval; int i; + /* Forcibly reset the timer, whether we think it's active or not */ MemSet(&timeval, 0, sizeof(struct itimerval)); if (setitimer(ITIMER_REAL, &timeval, NULL) != 0) elog(FATAL, "could not disable SIGALRM timer: %m"); @@ -457,11 +560,21 @@ disable_all_timeouts(bool keep_indicators) /* * Return the timeout's I've-been-fired indicator + * + * If reset_indicator is true, reset the indicator when returning true. + * To avoid missing timeouts due to race conditions, we are careful not to + * reset the indicator when returning false. */ bool -get_timeout_indicator(TimeoutId id) +get_timeout_indicator(TimeoutId id, bool reset_indicator) { - return all_timeouts[id].indicator; + if (all_timeouts[id].indicator) + { + if (reset_indicator) + all_timeouts[id].indicator = false; + return true; + } + return false; } /* diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index d500bfd234..19d12788d9 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -2592,9 +2592,12 @@ _tocEntryIsACL(TocEntry *te) static void _doSetFixedOutputState(ArchiveHandle *AH) { - /* Disable statement_timeout in archive for pg_restore/psql */ + /* Disable statement_timeout since restore is probably slow */ ahprintf(AH, "SET statement_timeout = 0;\n"); + /* Likewise for lock_timeout */ + ahprintf(AH, "SET lock_timeout = 0;\n"); + /* Select the correct character set encoding */ ahprintf(AH, "SET client_encoding = '%s';\n", pg_encoding_to_char(AH->public.encoding)); diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 94584292dc..093be9e16d 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -957,6 +957,8 @@ setup_connection(Archive *AH, const char *dumpencoding, char *use_role) */ if (AH->remoteVersion >= 70300) ExecuteSqlStatement(AH, "SET statement_timeout = 0"); + if (AH->remoteVersion >= 90300) + ExecuteSqlStatement(AH, "SET lock_timeout = 0"); /* * Quote all identifiers, if requested. diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index d571c35ab6..3b04d3c1fb 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -168,8 +168,8 @@ typedef struct PGXACT uint8 vacuumFlags; /* vacuum-related flags, see above */ bool overflowed; - bool delayChkpt; /* true if this proc delays checkpoint start */ - /* previously called InCommit */ + bool delayChkpt; /* true if this proc delays checkpoint start; + * previously called InCommit */ uint8 nxids; } PGXACT; @@ -222,6 +222,7 @@ extern PGPROC *PreparedXactProcs; /* configurable options */ extern int DeadlockTimeout; extern int StatementTimeout; +extern int LockTimeout; extern bool log_lock_waits; diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h index dd58c0e103..06bcc5a0f3 100644 --- a/src/include/utils/timeout.h +++ b/src/include/utils/timeout.h @@ -25,6 +25,7 @@ typedef enum TimeoutId /* Predefined timeout reasons */ STARTUP_PACKET_TIMEOUT, DEADLOCK_TIMEOUT, + LOCK_TIMEOUT, STATEMENT_TIMEOUT, STANDBY_DEADLOCK_TIMEOUT, STANDBY_TIMEOUT, @@ -35,20 +36,48 @@ typedef enum TimeoutId } TimeoutId; /* callback function signature */ -typedef void (*timeout_handler) (void); +typedef void (*timeout_handler_proc) (void); + +/* + * Parameter structure for setting multiple timeouts at once + */ +typedef enum TimeoutType +{ + TMPARAM_AFTER, + TMPARAM_AT +} TimeoutType; + +typedef struct +{ + TimeoutId id; /* timeout to set */ + TimeoutType type; /* TMPARAM_AFTER or TMPARAM_AT */ + int delay_ms; /* only used for TMPARAM_AFTER */ + TimestampTz fin_time; /* only used for TMPARAM_AT */ +} EnableTimeoutParams; + +/* + * Parameter structure for clearing multiple timeouts at once + */ +typedef struct +{ + TimeoutId id; /* timeout to clear */ + bool keep_indicator; /* keep the indicator flag? */ +} DisableTimeoutParams; /* timeout setup */ extern void InitializeTimeouts(void); -extern TimeoutId RegisterTimeout(TimeoutId id, timeout_handler handler); +extern TimeoutId RegisterTimeout(TimeoutId id, timeout_handler_proc handler); /* timeout operation */ extern void enable_timeout_after(TimeoutId id, int delay_ms); extern void enable_timeout_at(TimeoutId id, TimestampTz fin_time); +extern void enable_timeouts(const EnableTimeoutParams *timeouts, int count); extern void disable_timeout(TimeoutId id, bool keep_indicator); +extern void disable_timeouts(const DisableTimeoutParams *timeouts, int count); extern void disable_all_timeouts(bool keep_indicators); /* accessors */ -extern bool get_timeout_indicator(TimeoutId id); +extern bool get_timeout_indicator(TimeoutId id, bool reset_indicator); extern TimestampTz get_timeout_start_time(TimeoutId id); #endif /* TIMEOUT_H */ diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out new file mode 100644 index 0000000000..0ad792ae09 --- /dev/null +++ b/src/test/isolation/expected/timeouts.out @@ -0,0 +1,73 @@ +Parsed test spec with 2 sessions + +starting permutation: rdtbl sto locktbl +step rdtbl: SELECT * FROM accounts; +accountid balance + +checking 600 +savings 600 +step sto: SET statement_timeout = 1000; +step locktbl: LOCK TABLE accounts; +step locktbl: <... completed> +ERROR: canceling statement due to statement timeout + +starting permutation: rdtbl lto locktbl +step rdtbl: SELECT * FROM accounts; +accountid balance + +checking 600 +savings 600 +step lto: SET lock_timeout = 1000; +step locktbl: LOCK TABLE accounts; +step locktbl: <... completed> +ERROR: canceling statement due to lock timeout + +starting permutation: rdtbl lsto locktbl +step rdtbl: SELECT * FROM accounts; +accountid balance + +checking 600 +savings 600 +step lsto: SET lock_timeout = 1000; SET statement_timeout = 2000; +step locktbl: LOCK TABLE accounts; +step locktbl: <... completed> +ERROR: canceling statement due to lock timeout + +starting permutation: rdtbl slto locktbl +step rdtbl: SELECT * FROM accounts; +accountid balance + +checking 600 +savings 600 +step slto: SET lock_timeout = 2000; SET statement_timeout = 1000; +step locktbl: LOCK TABLE accounts; +step locktbl: <... completed> +ERROR: canceling statement due to statement timeout + +starting permutation: wrtbl sto update +step wrtbl: UPDATE accounts SET balance = balance + 100; +step sto: SET statement_timeout = 1000; +step update: DELETE FROM accounts WHERE accountid = 'checking'; +step update: <... completed> +ERROR: canceling statement due to statement timeout + +starting permutation: wrtbl lto update +step wrtbl: UPDATE accounts SET balance = balance + 100; +step lto: SET lock_timeout = 1000; +step update: DELETE FROM accounts WHERE accountid = 'checking'; +step update: <... completed> +ERROR: canceling statement due to lock timeout + +starting permutation: wrtbl lsto update +step wrtbl: UPDATE accounts SET balance = balance + 100; +step lsto: SET lock_timeout = 1000; SET statement_timeout = 2000; +step update: DELETE FROM accounts WHERE accountid = 'checking'; +step update: <... completed> +ERROR: canceling statement due to lock timeout + +starting permutation: wrtbl slto update +step wrtbl: UPDATE accounts SET balance = balance + 100; +step slto: SET lock_timeout = 2000; SET statement_timeout = 1000; +step update: DELETE FROM accounts WHERE accountid = 'checking'; +step update: <... completed> +ERROR: canceling statement due to statement timeout diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index c4d6719de6..081e11f2a1 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -20,3 +20,4 @@ test: delete-abort-savept test: delete-abort-savept-2 test: aborted-keyrevoke test: drop-index-concurrently-1 +test: timeouts diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec new file mode 100644 index 0000000000..000b50c9c9 --- /dev/null +++ b/src/test/isolation/specs/timeouts.spec @@ -0,0 +1,45 @@ +# Simple tests for statement_timeout and lock_timeout features + +setup +{ + CREATE TABLE accounts (accountid text PRIMARY KEY, balance numeric not null); + INSERT INTO accounts VALUES ('checking', 600), ('savings', 600); +} + +teardown +{ + DROP TABLE accounts; +} + +session "s1" +setup { BEGIN ISOLATION LEVEL READ COMMITTED; } +step "rdtbl" { SELECT * FROM accounts; } +step "wrtbl" { UPDATE accounts SET balance = balance + 100; } +teardown { ABORT; } + +session "s2" +setup { BEGIN ISOLATION LEVEL READ COMMITTED; } +step "sto" { SET statement_timeout = 1000; } +step "lto" { SET lock_timeout = 1000; } +step "lsto" { SET lock_timeout = 1000; SET statement_timeout = 2000; } +step "slto" { SET lock_timeout = 2000; SET statement_timeout = 1000; } +step "locktbl" { LOCK TABLE accounts; } +step "update" { DELETE FROM accounts WHERE accountid = 'checking'; } +teardown { ABORT; } + +# statement timeout, table-level lock +permutation "rdtbl" "sto" "locktbl" +# lock timeout, table-level lock +permutation "rdtbl" "lto" "locktbl" +# lock timeout expires first, table-level lock +permutation "rdtbl" "lsto" "locktbl" +# statement timeout expires first, table-level lock +permutation "rdtbl" "slto" "locktbl" +# statement timeout, row-level lock +permutation "wrtbl" "sto" "update" +# lock timeout, row-level lock +permutation "wrtbl" "lto" "update" +# lock timeout expires first, row-level lock +permutation "wrtbl" "lsto" "update" +# statement timeout expires first, row-level lock +permutation "wrtbl" "slto" "update"