> On Tue, Nov 03, 2020 at 07:14:47PM +0100, Dmitry Dolgov wrote: > > On Thu, Aug 20, 2020 at 03:11:19PM +0900, Michael Paquier wrote: > > On Wed, Aug 19, 2020 at 02:16:46PM -0400, Alvaro Herrera wrote: > > > I did not set the flag in REINDEX CONCURRENTLY, but as I understand it > > > can be done too, since in essence it's the same thing as a CIC from a > > > snapshot management point of view. > > > > Yes, I see no problems for REINDEX CONCURRENTLY as well as long as > > there are no predicates and expressions involved. The transactions > > that should be patched are all started in ReindexRelationConcurrently. > > The transaction of index_concurrently_swap() cannot set up that > > though. Only thing to be careful is to make sure that safe_flag is > > correct depending on the list of indexes worked on. > > Hi, > > After looking through the thread and reading the patch it seems good, > and there are only few minor questions: > > * Doing the same for REINDEX CONCURRENTLY, which does make sense. In > fact it's already mentioned in the commentaries as done, which a bit > confusing.
Just to give it a shot, would the attached change be enough?
>From d30d8acf91679985970334069ab7f1f8f7fc3ec5 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 4 Aug 2020 22:04:57 -0400 Subject: [PATCH v3] Avoid spurious CREATE INDEX CONCURRENTLY waits --- src/backend/commands/indexcmds.c | 122 ++++++++++++++++++++++++++++++- src/include/storage/proc.h | 6 +- 2 files changed, 124 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 75552c64ed..5019397d50 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -385,7 +385,10 @@ 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.) + * 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. * * Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not * check for that. @@ -406,7 +409,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) VirtualTransactionId *old_snapshots; old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM + | PROC_IN_SAFE_IC, &n_old_snapshots); if (progress) pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots); @@ -426,7 +430,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress) newer_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false, - PROC_IS_AUTOVACUUM | PROC_IN_VACUUM, + PROC_IS_AUTOVACUUM | PROC_IN_VACUUM + | PROC_IN_SAFE_IC, &n_newer_snapshots); for (j = i; j < n_old_snapshots; j++) { @@ -519,6 +524,7 @@ DefineIndex(Oid relationId, bool amcanorder; amoptions_function amoptions; bool partitioned; + bool safe_index; Datum reloptions; int16 *coloptions; IndexInfo *indexInfo; @@ -1045,6 +1051,17 @@ DefineIndex(Oid relationId, } } + /* + * When doing concurrent index builds, we can set a PGPROC flag to tell + * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY + * to ignore us when waiting for concurrent snapshots. That can only be + * done for indexes that don't execute any expressions. Determine that. + * (The flag is reset automatically at transaction end, so it must be + * set for each transaction.) + */ + safe_index = indexInfo->ii_Expressions == NIL && + indexInfo->ii_Predicate == NIL; + /* * Report index creation if appropriate (delay this till after most of the * error checks) @@ -1431,6 +1448,15 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * The index is now visible, so we can report the OID. */ @@ -1490,6 +1516,15 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Phase 3 of concurrent index build * @@ -1546,6 +1581,15 @@ DefineIndex(Oid relationId, CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* We should now definitely not be advertising any xmin. */ Assert(MyProc->xmin == InvalidTransactionId); @@ -3021,6 +3065,7 @@ ReindexRelationConcurrently(Oid relationOid, int options) PROGRESS_CREATEIDX_ACCESS_METHOD_OID }; int64 progress_vals[4]; + bool safe_index = true; /* * Create a memory context that will survive forced transaction commits we @@ -3324,6 +3369,23 @@ ReindexRelationConcurrently(Oid relationOid, int options) */ newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock); + /* + * When doing concurrent reindex, we can set a PGPROC flag to tell + * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX + * CONCURRENTLY to ignore us when waiting for concurrent snapshots. + * That can only be done for indexes that don't execute any + * expressions. Determine that for all involved indexes together. (The + * flag is reset automatically at transaction end, so it must be set + * for each transaction.) + */ + if (safe_index) + { + IndexInfo *newIndexInfo = BuildIndexInfo(newIndexRel); + + safe_index = newIndexInfo->ii_Expressions == NIL && + newIndexInfo->ii_Predicate == NIL; + } + /* * Save the list of OIDs and locks in private context */ @@ -3393,6 +3455,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Phase 2 of REINDEX CONCURRENTLY * @@ -3456,6 +3527,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) } StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Phase 3 of REINDEX CONCURRENTLY * @@ -3559,6 +3639,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + forboth(lc, indexIds, lc2, newIndexIds) { char *oldName; @@ -3609,6 +3698,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Phase 5 of REINDEX CONCURRENTLY * @@ -3641,6 +3739,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) CommitTransactionCommand(); StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* * Phase 6 of REINDEX CONCURRENTLY * @@ -3692,6 +3799,15 @@ ReindexRelationConcurrently(Oid relationOid, int options) /* Start a new transaction to finish process properly */ StartTransactionCommand(); + /* Tell concurrent index builds to ignore us, if index qualifies */ + if (safe_index) + { + LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + MyProc->vacuumFlags |= PROC_IN_SAFE_IC; + ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags; + LWLockRelease(ProcArrayLock); + } + /* Log what we did */ if (options & REINDEXOPT_VERBOSE) { diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 9c9a50ae45..50ce5c8cf2 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -53,13 +53,17 @@ 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 */ /* flags reset at EOXact */ #define PROC_VACUUM_STATE_MASK \ - (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND) + (PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND) /* * We allow a small number of "weak" relation locks (AccessShareLock, -- 2.21.0