From bd3f6fdc187f92e8e98018e9c9cfd9cac8753dc7 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 30 Sep 2020 20:24:38 -0700 Subject: [PATCH v2] Add INTERRUPTIBLE option to VACUUM & ANALYZE, test interruption. Partially because that's practically useful, partially because it's useful for testing (see 5871f09c985). To implement add the new PROC_IS_INTERRUPTIBLE flag, controlling whether a process should be interrupted if it holds a required lock. As PROC_VACUUM_FOR_WRAPAROUND was only used to signal that a PROC_IS_AUTOVACUUM proc should not be interrupted, I have removed that. Now that it's easier to trigger interruptible vacuums, add a test doing so. FIXME: change commit message based on whether we keep the behaviour of autovacuum cancelling analyze or not. Discussion: https://www.postgresql.org/message-id/20200909041147.amcitkxmlexgjfty%40alap3.anarazel.de --- src/include/commands/vacuum.h | 1 + src/include/storage/lock.h | 6 +- src/include/storage/proc.h | 5 +- src/backend/commands/analyze.c | 22 +++++++ src/backend/commands/vacuum.c | 14 +++-- src/backend/postmaster/autovacuum.c | 2 + src/backend/storage/lmgr/deadlock.c | 63 ++++++++++--------- src/backend/storage/lmgr/proc.c | 24 +++---- doc/src/sgml/ref/analyze.sgml | 16 +++++ doc/src/sgml/ref/vacuum.sgml | 17 +++++ src/test/isolation/expected/vacuum-cancel.out | 41 ++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/vacuum-cancel.spec | 55 ++++++++++++++++ 13 files changed, 218 insertions(+), 49 deletions(-) create mode 100644 src/test/isolation/expected/vacuum-cancel.out create mode 100644 src/test/isolation/specs/vacuum-cancel.spec diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index d9475c99890..7e064ef45e9 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -215,6 +215,7 @@ typedef struct VacuumParams int multixact_freeze_table_age; /* multixact age at which to scan * whole table */ bool is_wraparound; /* force a for-wraparound vacuum */ + bool is_interruptible; /* allow cancelling on lock conflict */ int log_min_duration; /* minimum execution threshold in ms at * which verbose logs are activated, -1 * to use default */ diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h index 1c3e9c1999f..f21f9f3f974 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -499,8 +499,8 @@ typedef enum DS_NO_DEADLOCK, /* no deadlock detected */ DS_SOFT_DEADLOCK, /* deadlock avoided by queue rearrangement */ DS_HARD_DEADLOCK, /* deadlock, no way out but ERROR */ - DS_BLOCKED_BY_AUTOVACUUM /* no deadlock; queue blocked by autovacuum - * worker */ + DS_BLOCKED_BY_INTERRUPTIBLE /* no deadlock; queue blocked by interruptible + * task (e.g. autovacuum worker) */ } DeadLockState; /* @@ -588,7 +588,7 @@ extern void lock_twophase_standby_recover(TransactionId xid, uint16 info, void *recdata, uint32 len); extern DeadLockState DeadLockCheck(PGPROC *proc); -extern PGPROC *GetBlockingAutoVacuumPgproc(void); +extern PGPROC *GetBlockingInterruptiblePgproc(void); extern void DeadLockReport(void) pg_attribute_noreturn(); extern void RememberSimpleDeadLock(PGPROC *proc1, LOCKMODE lockmode, diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 9c9a50ae457..e660972a010 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -53,13 +53,14 @@ struct XidCache */ #define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */ #define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */ -#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */ +#define PROC_IS_INTERRUPTIBLE 0x08 /* vacuum / analyze may be interrupted + * when locks conflict */ #define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical * decoding outside xact */ /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ - (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND) + (PROC_IN_VACUUM | PROC_IS_INTERRUPTIBLE) /* * We allow a small number of "weak" relation locks (AccessShareLock, diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 8af12b5c6b2..9bafd07e758 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -252,6 +252,15 @@ analyze_rel(Oid relid, RangeVar *relation, pgstat_progress_start_command(PROGRESS_COMMAND_ANALYZE, RelationGetRelid(onerel)); + /* mark us as interruptible if appropriate */ + if (params->is_interruptible) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IS_INTERRUPTIBLE; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Do the normal non-recursive ANALYZE. We can skip this for partitioned * tables, which don't contain any rows. @@ -276,6 +285,19 @@ analyze_rel(Oid relid, RangeVar *relation, relation_close(onerel, NoLock); pgstat_progress_end_command(); + + /* + * Reset flags if we set them. Need to that (in contrast to VACUUM) + * because analyze can be run within a transaction, so we can't rely on + * the end-of-xact code doing this for us. + */ + if (params->is_interruptible) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags &= ~PROC_IS_INTERRUPTIBLE; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } } /* diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index ddeec870d81..df717d2951a 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -123,6 +123,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel) verbose = defGetBoolean(opt); else if (strcmp(opt->defname, "skip_locked") == 0) skip_locked = defGetBoolean(opt); + else if (strcmp(opt->defname, "interruptible") == 0) + params.is_interruptible = defGetBoolean(opt); else if (!vacstmt->is_vacuumcmd) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), @@ -1754,19 +1756,21 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params) * contents of other tables is arguably broken, but we won't break it * here by violating transaction semantics.) * - * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by - * autovacuum; it's used to avoid canceling a vacuum that was invoked - * in an emergency. + * We also set the INTERRUPTIBLE flag when user indicated or when + * started by autovacuum (except for anti-wraparound autovacuum, which + * we do not want to cancel), that governs whether the process may be + * cancelled upon a lock conflict. * * Note: these flags remain set until CommitTransaction or * AbortTransaction. We don't want to clear them until we reset * MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId() * might appear to go backwards, which is probably Not Good. */ + Assert(!params->is_wraparound || !params->is_interruptible); LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); MyProc->vacuumFlags |= PROC_IN_VACUUM; - if (params->is_wraparound) - MyProc->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND; + if (params->is_interruptible) + MyProc->vacuumFlags |= PROC_IS_INTERRUPTIBLE; ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; LWLockRelease(ProcArrayLock); } diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 2cef56f115f..630e068649e 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2915,6 +2915,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map, tab->at_params.multixact_freeze_min_age = multixact_freeze_min_age; tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age; tab->at_params.is_wraparound = wraparound; + /* Allow interrupting autovacuum unless it's an emergency one */ + tab->at_params.is_interruptible = !wraparound; tab->at_params.log_min_duration = log_min_duration; tab->at_vacuum_cost_limit = vac_cost_limit; tab->at_vacuum_cost_delay = vac_cost_delay; diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c index e1246b8a4da..425a81194f7 100644 --- a/src/backend/storage/lmgr/deadlock.c +++ b/src/backend/storage/lmgr/deadlock.c @@ -124,8 +124,8 @@ static int maxPossibleConstraints; static DEADLOCK_INFO *deadlockDetails; static int nDeadlockDetails; -/* PGPROC pointer of any blocking autovacuum worker found */ -static PGPROC *blocking_autovacuum_proc = NULL; +/* PGPROC pointer of any blocking but interruptible process found */ +static PGPROC *blocking_interruptible_proc = NULL; /* @@ -224,8 +224,8 @@ DeadLockCheck(PGPROC *proc) nPossibleConstraints = 0; nWaitOrders = 0; - /* Initialize to not blocked by an autovacuum worker */ - blocking_autovacuum_proc = NULL; + /* Initialize to not blocked by an interruptible process */ + blocking_interruptible_proc = NULL; /* Search for deadlocks and possible fixes */ if (DeadLockCheckRecurse(proc)) @@ -278,24 +278,24 @@ DeadLockCheck(PGPROC *proc) /* Return code tells caller if we had to escape a deadlock or not */ if (nWaitOrders > 0) return DS_SOFT_DEADLOCK; - else if (blocking_autovacuum_proc != NULL) - return DS_BLOCKED_BY_AUTOVACUUM; + else if (blocking_interruptible_proc != NULL) + return DS_BLOCKED_BY_INTERRUPTIBLE; else return DS_NO_DEADLOCK; } /* - * Return the PGPROC of the autovacuum that's blocking a process. + * Return the PGPROC of an interruptible process that's blocking a process. * * We reset the saved pointer as soon as we pass it back. */ PGPROC * -GetBlockingAutoVacuumPgproc(void) +GetBlockingInterruptiblePgproc(void) { PGPROC *ptr; - ptr = blocking_autovacuum_proc; - blocking_autovacuum_proc = NULL; + ptr = blocking_interruptible_proc; + blocking_interruptible_proc = NULL; return ptr; } @@ -606,30 +606,37 @@ FindLockCycleRecurseMember(PGPROC *checkProc, } /* - * No deadlock here, but see if this proc is an autovacuum - * that is directly hard-blocking our own proc. If so, - * report it so that the caller can send a cancel signal - * to it, if appropriate. If there's more than one such - * proc, it's indeterminate which one will be reported. + * No deadlock here, but see if this proc is an + * interruptible process that is directly hard-blocking + * our own proc. If so, report it so that the caller can + * send a cancel signal to it, if appropriate. If there's + * more than one such proc, it's indeterminate which one + * will be reported. * - * We don't touch autovacuums that are indirectly blocking + * We don't touch processes that are indirectly blocking * us; it's up to the direct blockee to take action. This * rule simplifies understanding the behavior and ensures - * that an autovacuum won't be canceled with less than - * deadlock_timeout grace period. + * such a process won't be canceled with a grace period of + * less than deadlock_timeout. * - * Note we read vacuumFlags without any locking. This is - * OK only for checking the PROC_IS_AUTOVACUUM flag, - * because that flag is set at process start and never - * reset. There is logic elsewhere to avoid canceling an - * autovacuum that is working to prevent XID wraparound - * problems (which needs to read a different vacuumFlag - * bit), but we don't do that here to avoid grabbing - * ProcArrayLock. + * Note we read vacuumFlags without any locking. That is + * ok because 32bit reads are atomic, because surrounding + * operations provide strong enough memory barriers, and + * because we re-check whether IS_INTERRUPTIBLE is still + * set before actually cancelling the backend. */ if (checkProc == MyProc && - proc->vacuumFlags & PROC_IS_AUTOVACUUM) - blocking_autovacuum_proc = proc; + proc->vacuumFlags & PROC_IS_INTERRUPTIBLE) + { + /* + * XXX: At some point there could be more than one + * proc blocking us here. Currently that's not + * possible because of the lock levels used by VACUUM + * / ANALYZE, which are the only ones to set this flag + * currently, but ... + */ + blocking_interruptible_proc = proc; + } /* We're done looking at this proclock */ break; diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 88566bd9fab..f95eb549e3c 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -1064,7 +1064,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) PROC_QUEUE *waitQueue = &(lock->waitProcs); LOCKMASK myHeldLocks = MyProc->heldLocks; bool early_deadlock = false; - bool allow_autovacuum_cancel = true; + bool allow_interruptible_cancel = true; ProcWaitStatus myWaitStatus; PGPROC *proc; PGPROC *leader = MyProc->lockGroupLeader; @@ -1304,23 +1304,21 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) myWaitStatus = *((volatile ProcWaitStatus *) &MyProc->waitStatus); /* - * If we are not deadlocked, but are waiting on an autovacuum-induced - * task, send a signal to interrupt it. + * If we are not deadlocked, but are waiting on an interruptible + * (e.g. autovacuum-induced) task, send a signal to interrupt it. */ - if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM && allow_autovacuum_cancel) + if (deadlock_state == DS_BLOCKED_BY_INTERRUPTIBLE && allow_interruptible_cancel) { - PGPROC *autovac = GetBlockingAutoVacuumPgproc(); + PGPROC *autovac = GetBlockingInterruptiblePgproc(); uint8 vacuumFlags; LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); /* - * Only do it if the worker is not working to protect against Xid - * wraparound. + * Only do it if the process is still interruptible. */ vacuumFlags = ProcGlobal->vacuumFlags[autovac->pgxactoff]; - if ((vacuumFlags & PROC_IS_AUTOVACUUM) && - !(vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND)) + if (vacuumFlags & PROC_IS_INTERRUPTIBLE) { int pid = autovac->pid; StringInfoData locktagbuf; @@ -1340,8 +1338,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) LWLockRelease(ProcArrayLock); /* send the autovacuum worker Back to Old Kent Road */ + /* + * FIXME: do we want to continue to identify autovacuum here? + * Could do so based on PROC_IS_AUTOVACUUM. + */ ereport(DEBUG1, - (errmsg("sending cancel to blocking autovacuum PID %d", + (errmsg("sending cancel to blocking autovacuum|XXX PID %d", pid), errdetail_log("%s", logbuf.data))); @@ -1370,7 +1372,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) LWLockRelease(ProcArrayLock); /* prevent signal from being sent again more than once */ - allow_autovacuum_cancel = false; + allow_interruptible_cancel = false; } /* diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml index 5ac3ba83219..c7f3f58c6da 100644 --- a/doc/src/sgml/ref/analyze.sgml +++ b/doc/src/sgml/ref/analyze.sgml @@ -28,6 +28,7 @@ ANALYZE [ VERBOSE ] [ table_and_columnsboolean ] SKIP_LOCKED [ boolean ] + INTERRUPTIBLE [ boolean ] and table_and_columns is: @@ -95,6 +96,21 @@ ANALYZE [ VERBOSE ] [ table_and_columns + + INTERRUPTIBLE + + + Specifies whether ANALYZE may be cancelled when + another connection needs a lock conflicting with this. + ANALYZE. That can be useful to reduce the impact of + running ANALYZE in a busy system, by e.g. allowing + DDL commands to run before ANALYZE has finished. If + the option is not specified ANALYZE is not + interruptible. + + + + boolean diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml index 26ede69bb31..b3e31423f17 100644 --- a/doc/src/sgml/ref/vacuum.sgml +++ b/doc/src/sgml/ref/vacuum.sgml @@ -35,6 +35,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean ] TRUNCATE [ boolean ] PARALLEL integer + INTERRUPTIBLE [ boolean ] and table_and_columns is: @@ -255,6 +256,22 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ boolean diff --git a/src/test/isolation/expected/vacuum-cancel.out b/src/test/isolation/expected/vacuum-cancel.out new file mode 100644 index 00000000000..7914df3f8a2 --- /dev/null +++ b/src/test/isolation/expected/vacuum-cancel.out @@ -0,0 +1,41 @@ +Parsed test spec with 3 sessions + +starting permutation: s1_begin s1_block_vacuum s2_delete s2_vacuum_interruptible s1_commit +step s1_begin: BEGIN; +step s1_block_vacuum: LOCK pg_class IN SHARE MODE; +step s2_delete: DELETE FROM test_vacuum_cancel; +step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; +step s1_commit: COMMIT; +step s2_vacuum_interruptible: <... completed> + +starting permutation: s1_begin s1_block_vacuum s2_delete s2_analyze_interruptible s1_commit +step s1_begin: BEGIN; +step s1_block_vacuum: LOCK pg_class IN SHARE MODE; +step s2_delete: DELETE FROM test_vacuum_cancel; +step s2_analyze_interruptible: ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; +step s1_commit: COMMIT; +step s2_analyze_interruptible: <... completed> + +starting permutation: s1_begin s1_block_vacuum s2_delete s2_vacuum_interruptible s3_lock s3_commit s1_commit +step s1_begin: BEGIN; +step s1_block_vacuum: LOCK pg_class IN SHARE MODE; +step s2_delete: DELETE FROM test_vacuum_cancel; +step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; +step s3_lock: BEGIN; LOCK test_vacuum_cancel; +step s3_lock: <... completed> +step s2_vacuum_interruptible: <... completed> +error in steps s3_lock s2_vacuum_interruptible: ERROR: canceling statement due to user request +step s3_commit: COMMIT; +step s1_commit: COMMIT; + +starting permutation: s1_begin s1_block_vacuum s2_delete s2_analyze_interruptible s3_lock s3_commit s1_commit +step s1_begin: BEGIN; +step s1_block_vacuum: LOCK pg_class IN SHARE MODE; +step s2_delete: DELETE FROM test_vacuum_cancel; +step s2_analyze_interruptible: ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; +step s3_lock: BEGIN; LOCK test_vacuum_cancel; +step s3_lock: <... completed> +step s2_analyze_interruptible: <... completed> +error in steps s3_lock s2_analyze_interruptible: ERROR: canceling statement due to user request +step s3_commit: COMMIT; +step s1_commit: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index aa386ab1a25..ff437fcbdd3 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -78,6 +78,7 @@ test: timeouts test: vacuum-concurrent-drop test: vacuum-conflict test: vacuum-skip-locked +test: vacuum-cancel test: predicate-hash test: predicate-gist test: predicate-gin diff --git a/src/test/isolation/specs/vacuum-cancel.spec b/src/test/isolation/specs/vacuum-cancel.spec new file mode 100644 index 00000000000..f8e502bb99f --- /dev/null +++ b/src/test/isolation/specs/vacuum-cancel.spec @@ -0,0 +1,55 @@ +setup +{ + CREATE TABLE test_vacuum_cancel(data text); + /* don't want autovacuum clean up to-be-cleaned data concurrently */ + ALTER TABLE test_vacuum_cancel SET (AUTOVACUUM_ENABLED = false); + /* insert some data */ + INSERT INTO test_vacuum_cancel VALUES('somedata'),('otherdata'); +} + +teardown +{ + DROP TABLE test_vacuum_cancel; +} + +session "s1" +step "s1_begin" { BEGIN; } +step "s1_block_vacuum" { LOCK pg_class IN SHARE MODE; } +step "s1_commit" { COMMIT; } + +session "s2" +step "s2_delete" { DELETE FROM test_vacuum_cancel; } +step "s2_vacuum_interruptible" { VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; } +step "s2_analyze_interruptible" { ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; } + +session "s3" +step "s3_lock" { BEGIN; LOCK test_vacuum_cancel; } +step "s3_commit" { COMMIT; } + +# First test if releasing the lock on pg_class allows to continue +permutation + # block vacuum + "s1_begin" "s1_block_vacuum" "s2_delete" + # vacuum, which will be blocked by the above + "s2_vacuum_interruptible" + # and allow to continue + "s1_commit" +permutation "s1_begin" "s1_block_vacuum" "s2_delete" "s2_analyze_interruptible" "s1_commit" + +# Then track that concurrent LOCK interrupts VACUUM / ANALYZE +permutation + # block vacuum + "s1_begin" "s1_block_vacuum" "s2_delete" + # vacuum, which will be blocked by the above + "s2_vacuum_interruptible" + # issue lock request conflicting with vacuum + "s3_lock" + # and release the blocking lock request again. Done separately, to + # ensure isolationtester schedules predictably + "s3_commit" "s1_commit" + +permutation + "s1_begin" "s1_block_vacuum" "s2_delete" + "s2_analyze_interruptible" + "s3_lock" + "s3_commit" "s1_commit" -- 2.28.0.651.g306ee63a70