From: Alvaro Herrera Date: Mon, 30 May 2022 20:22:08 +0200 Subject: [PATCH] Revert concurrent indexing changes Specifically: c98763bf51bf Avoid spurious waits in concurrent indexing f9900df5f949 Avoid spurious wait in concurrent reindex d9d076222f5b VACUUM: ignore indexing operations with CONCURRENTLY --- doc/src/sgml/ref/create_index.sgml | 2 - doc/src/sgml/ref/reindex.sgml | 2 - src/backend/access/nbtree/nbtsort.c | 6 +- src/backend/commands/indexcmds.c | 108 +--------------------------- src/backend/storage/ipc/procarray.c | 39 ++-------- src/include/storage/proc.h | 8 +-- 6 files changed, 14 insertions(+), 151 deletions(-) diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml index 907324b93e..9ffcdc629e 100644 --- a/doc/src/sgml/ref/create_index.sgml +++ b/doc/src/sgml/ref/create_index.sgml @@ -861,8 +861,6 @@ Indexes: Like any long-running transaction, CREATE INDEX on a table can affect which tuples can be removed by concurrent VACUUM on any other table. - Excepted from this are operations with the CONCURRENTLY - option for indexes that are not partial and do not index any expressions. diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 36cb4a455b..336ca24b31 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -475,8 +475,6 @@ Indexes: Like any long-running transaction, REINDEX on a table can affect which tuples can be removed by concurrent VACUUM on any other table. - Excepted from this are operations with the CONCURRENTLY - option for indexes that are not partial and do not index any expressions. diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c index 9f60fa9894..79ea39b97f 100644 --- a/src/backend/access/nbtree/nbtsort.c +++ b/src/backend/access/nbtree/nbtsort.c @@ -1817,11 +1817,9 @@ _bt_parallel_build_main(dsm_segment *seg, shm_toc *toc) #endif /* BTREE_BUILD_STATS */ /* - * The only possible status flag that can be set to the parallel worker is - * PROC_IN_SAFE_IC. + * No flags should be set in the parallel worker. */ - Assert((MyProc->statusFlags == 0) || - (MyProc->statusFlags == PROC_IN_SAFE_IC)); + Assert(MyProc->statusFlags == 0); /* Set debug_query_string for individual workers first */ sharedquery = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT, true); diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index eac13ac0b7..097861d0e5 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -102,7 +102,6 @@ static void ReindexMultipleInternal(List *relids, static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params); static void update_relispartition(Oid relationId, bool newval); -static inline void set_indexsafe_procflags(void); /* * callback argument type for RangeVarCallbackForReindexIndex() @@ -394,10 +393,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts) * lazy VACUUMs, because they won't be fazed by missing index entries * either. (Manual ANALYZEs, however, can't be excluded because they * might be within transactions that are going to do arbitrary operations - * later.) Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY - * on indexes that are neither expressional nor partial are also safe to - * ignore, since we know that those processes won't examine any data - * outside the table they're indexing. + * later.) * * Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not * check for that. @@ -418,8 +414,7 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) VirtualTransactionId *old_snapshots; old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM - | PROC_IN_SAFE_IC, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, &n_old_snapshots); if (progress) pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots); @@ -439,8 +434,7 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) newer_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM - | PROC_IN_SAFE_IC, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, &n_newer_snapshots); for (j = i; j < n_old_snapshots; j++) { @@ -533,7 +527,6 @@ DefineIndex(Oid relationId, bool amcanorder; amoptions_function amoptions; bool partitioned; - bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -1061,10 +1054,6 @@ DefineIndex(Oid relationId, } } - /* Is index safe for others to ignore? See set_indexsafe_procflags() */ - safe_index = indexInfo->ii_Expressions == NIL && - indexInfo->ii_Predicate == NIL; - /* * Report index creation if appropriate (delay this till after most of the * error checks) @@ -1495,10 +1484,6 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - /* * The index is now visible, so we can report the OID. While on it, * include the report for the beginning of phase 2. @@ -1567,10 +1552,6 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - /* * Phase 3 of concurrent index build * @@ -1627,10 +1608,6 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); - /* Tell concurrent index builds to ignore us, if index qualifies */ - if (safe_index) - set_indexsafe_procflags(); - /* We should now definitely not be advertising any xmin. */ Assert(MyProc->xmin == InvalidTransactionId); @@ -3249,7 +3226,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) Oid indexId; Oid tableId; Oid amId; - bool safe; /* for set_indexsafe_procflags */ } ReindexIndexInfo; List *heapRelationIds = NIL; List *indexIds = NIL; @@ -3597,9 +3573,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); - /* determine safety of this index for set_indexsafe_procflags */ - idx->safe = (indexRel->rd_indexprs == NIL && - indexRel->rd_indpred == NIL); idx->tableId = RelationGetRelid(heapRel); idx->amId = indexRel->rd_rel->relam; @@ -3649,7 +3622,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) newidx = palloc(sizeof(ReindexIndexInfo)); newidx->indexId = newIndexId; - newidx->safe = idx->safe; newidx->tableId = idx->tableId; newidx->amId = idx->amId; @@ -3724,11 +3696,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) CommitTransactionCommand(); StartTransactionCommand(); - /* - * Because we don't take a snapshot in this transaction, there's no need - * to set the PROC_IN_SAFE_IC flag here. - */ - /* * Phase 2 of REINDEX CONCURRENTLY * @@ -3758,10 +3725,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* Set ActiveSnapshot since functions in the indexes may need it */ PushActiveSnapshot(GetTransactionSnapshot()); @@ -3785,11 +3748,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) StartTransactionCommand(); - /* - * Because we don't take a snapshot or Xid in this transaction, there's no - * need to set the PROC_IN_SAFE_IC flag here. - */ - /* * Phase 3 of REINDEX CONCURRENTLY * @@ -3818,10 +3776,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) */ CHECK_FOR_INTERRUPTS(); - /* Tell concurrent indexing to ignore us, if index qualifies */ - if (newidx->safe) - set_indexsafe_procflags(); - /* * Take the "reference snapshot" that will be used by validate_index() * to filter candidate tuples. @@ -3865,9 +3819,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) * interesting tuples. But since it might not contain tuples deleted * just before the reference snap was taken, we have to wait out any * transactions that might have older snapshots. - * - * Because we don't take a snapshot or Xid in this transaction, - * there's no need to set the PROC_IN_SAFE_IC flag here. */ pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_WAIT_3); @@ -3889,13 +3840,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) StartTransactionCommand(); - /* - * Because this transaction only does catalog manipulations and doesn't do - * any index operations, we can set the PROC_IN_SAFE_IC flag here - * unconditionally. - */ - set_indexsafe_procflags(); - forboth(lc, indexIds, lc2, newIndexIds) { ReindexIndexInfo *oldidx = lfirst(lc); @@ -3943,12 +3887,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) CommitTransactionCommand(); StartTransactionCommand(); - /* - * While we could set PROC_IN_SAFE_IC if all indexes qualified, there's no - * real need for that, because we only acquire an Xid after the wait is - * done, and that lasts for a very short period. - */ - /* * Phase 5 of REINDEX CONCURRENTLY * @@ -3979,12 +3917,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) CommitTransactionCommand(); StartTransactionCommand(); - /* - * While we could set PROC_IN_SAFE_IC if all indexes qualified, there's no - * real need for that, because we only acquire an Xid after the wait is - * done, and that lasts for a very short period. - */ - /* * Phase 6 of REINDEX CONCURRENTLY * @@ -4225,37 +4157,3 @@ update_relispartition(Oid relationId, bool newval) heap_freetuple(tup); table_close(classRel, RowExclusiveLock); } - -/* - * Set the PROC_IN_SAFE_IC flag in MyProc->statusFlags. - * - * When doing concurrent index builds, we can set this flag - * to tell other processes concurrently running CREATE - * INDEX CONCURRENTLY or REINDEX CONCURRENTLY to ignore us when - * doing their waits for concurrent snapshots. On one hand it - * avoids pointlessly waiting for a process that's not interesting - * anyway; but more importantly it avoids deadlocks in some cases. - * - * This can be done safely only for indexes that don't execute any - * expressions that could access other tables, so index must not be - * expressional nor partial. Caller is responsible for only calling - * this routine when that assumption holds true. - * - * (The flag is reset automatically at transaction end, so it must be - * set for each transaction.) - */ -static inline void -set_indexsafe_procflags(void) -{ - /* - * This should only be called before installing xid or xmin in MyProc; - * otherwise, concurrent processes could see an Xmin that moves backwards. - */ - Assert(MyProc->xid == InvalidTransactionId && - MyProc->xmin == InvalidTransactionId); - - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); - MyProc->statusFlags |= PROC_IN_SAFE_IC; - ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags; - LWLockRelease(ProcArrayLock); -} diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index cd58c5faf0..4da53a5b3f 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1659,13 +1659,7 @@ TransactionIdIsActive(TransactionId xid) * relations that's not required, since only backends in my own database could * ever see the tuples in them. Also, we can ignore concurrently running lazy * VACUUMs because (a) they must be working on other tables, and (b) they - * don't need to do snapshot-based lookups. Similarly, for the non-catalog - * horizon, we can ignore CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY - * when they are working on non-partial, non-expressional indexes, for the - * same reasons and because they can't run in transaction blocks. (They are - * not possible to ignore for catalogs, because CIC and RC do some catalog - * operations.) Do note that this means that CIC and RC must use a lock level - * that conflicts with VACUUM. + * don't need to do snapshot-based lookups. * * This also computes a horizon used to truncate pg_subtrans. For that * backends in all databases have to be considered, and concurrently running @@ -1715,6 +1709,9 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) bool in_recovery = RecoveryInProgress(); TransactionId *other_xids = ProcGlobal->xids; + /* inferred after ProcArrayLock is released */ + h->catalog_oldest_nonremovable = InvalidTransactionId; + LWLockAcquire(ProcArrayLock, LW_SHARED); h->latest_completed = ShmemVariableCache->latestCompletedXid; @@ -1734,7 +1731,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) h->oldest_considered_running = initial; h->shared_oldest_nonremovable = initial; - h->catalog_oldest_nonremovable = initial; h->data_oldest_nonremovable = initial; /* @@ -1833,25 +1829,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) (statusFlags & PROC_AFFECTS_ALL_HORIZONS) || in_recovery) { - /* - * We can ignore this backend if it's running CREATE INDEX - * CONCURRENTLY or REINDEX CONCURRENTLY on a "safe" index -- but - * only on vacuums of user-defined tables. - */ - if (!(statusFlags & PROC_IN_SAFE_IC)) - h->data_oldest_nonremovable = - TransactionIdOlder(h->data_oldest_nonremovable, xmin); - - /* Catalog tables need to consider all backends in this db */ - h->catalog_oldest_nonremovable = - TransactionIdOlder(h->catalog_oldest_nonremovable, xmin); + h->data_oldest_nonremovable = + TransactionIdOlder(h->data_oldest_nonremovable, xmin); } } - /* catalog horizon should never be later than data */ - Assert(TransactionIdPrecedesOrEquals(h->catalog_oldest_nonremovable, - h->data_oldest_nonremovable)); - /* * If in recovery fetch oldest xid in KnownAssignedXids, will be applied * after lock is released. @@ -1873,8 +1855,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) TransactionIdOlder(h->shared_oldest_nonremovable, kaxmin); h->data_oldest_nonremovable = TransactionIdOlder(h->data_oldest_nonremovable, kaxmin); - h->catalog_oldest_nonremovable = - TransactionIdOlder(h->catalog_oldest_nonremovable, kaxmin); /* temp relations cannot be accessed in recovery */ } else @@ -1901,9 +1881,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) h->data_oldest_nonremovable = TransactionIdRetreatedBy(h->data_oldest_nonremovable, vacuum_defer_cleanup_age); - h->catalog_oldest_nonremovable = - TransactionIdRetreatedBy(h->catalog_oldest_nonremovable, - vacuum_defer_cleanup_age); /* defer doesn't apply to temp relations */ } @@ -1926,9 +1903,7 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) h->shared_oldest_nonremovable = TransactionIdOlder(h->shared_oldest_nonremovable, h->slot_catalog_xmin); - h->catalog_oldest_nonremovable = - TransactionIdOlder(h->catalog_oldest_nonremovable, - h->slot_xmin); + h->catalog_oldest_nonremovable = h->data_oldest_nonremovable; h->catalog_oldest_nonremovable = TransactionIdOlder(h->catalog_oldest_nonremovable, h->slot_catalog_xmin); diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 2579e619eb..ebc0851dc9 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -53,10 +53,6 @@ struct XidCache */ #define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */ #define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */ -#define PROC_IN_SAFE_IC 0x04 /* currently running CREATE INDEX - * CONCURRENTLY or REINDEX - * CONCURRENTLY on non-expressional, - * non-partial index */ #define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */ #define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical * decoding outside xact */ @@ -66,13 +62,13 @@ struct XidCache /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ - (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND) + (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND) /* * Xmin-related flags. Make sure any flags that affect how the process' Xmin * value is interpreted by VACUUM are included here. */ -#define PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC) +#define PROC_XMIN_FLAGS (PROC_IN_VACUUM) /* * We allow a small number of "weak" relation locks (AccessShareLock, -- 2.30.2 --47cl72nybveaxqc3--