From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Subject: [PATCH] Revert concurrent indexing changes
Date: Mon, 30 May 2022 20:22:08 +0200
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(-)
inline patch 1: doc/src/sgml/ref/create_index.sgml, doc/src/sgml/ref/reindex.sgml, src/backend/access/nbtree/nbtsort.c and 3 more (+14/-151 lines) download inline-1.patchdiff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgmlindex 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, <command>CREATE INDEX</command> on a
table can affect which tuples can be removed by concurrent
<command>VACUUM</command> on any other table.
- Excepted from this are operations with the <literal>CONCURRENTLY</literal>- option for indexes that are not partial and do not index any expressions.
</para>
<para>
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgmlindex 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, <command>REINDEX</command> on a table
can affect which tuples can be removed by concurrent
<command>VACUUM</command> on any other table.
- Excepted from this are operations with the <literal>CONCURRENTLY</literal>- option for indexes that are not partial and do not index any expressions.
</para>
<para>
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.cindex 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.cindex 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.cindex 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.hindex 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--
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@alvh.no-ip.org
Subject: Re: [PATCH] Revert concurrent indexing changes
In-Reply-To: <no-message-id-30238@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox