On 2021-Jan-12, Álvaro Herrera wrote:
> > For the 0001 patch, since ReindexIndexInfo is used only within
> > ReindexRelationConcurrently() I think it’s a function-local structure
> > type. So we can declare it within the function. What do you think?
>
> That's a good idea. Pushed with that change, thanks.
Here's the other patch, with comments fixed per reviews, and rebased to
account for the scope change.
--
Álvaro Herrera Valdivia, Chile
Tulio: oh, para qué servirá este boton, Juan Carlos?
Policarpo: No, aléjense, no toquen la consola!
Juan Carlos: Lo apretaré una y otra vez.
>From f2476636a358b77aca4b7adb36ca8e383e78af9b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Mon, 30 Nov 2020 16:01:46 -0300
Subject: [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
---
src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
src/include/storage/proc.h | 1 +
2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,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
+ * 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.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
- /* Tell concurrent index builds to ignore us, if index qualifies */
- if (safe_index)
- set_indexsafe_procflags();
+ /*
+ * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+ * it only acquires an Xid to do some catalog manipulations, after the
+ * wait is over.
+ */
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
Oid indexId;
Oid tableId;
Oid amId;
+ bool safe; /* for set_indexsafe_procflags */
} ReindexIndexInfo;
List *heapRelationIds = NIL;
List *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
heapRel = table_open(indexRel->rd_index->indrelid,
ShareUpdateExclusiveLock);
+ /* 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;
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
newidx = palloc(sizeof(ReindexIndexInfo));
newidx->indexId = newIndexId;
+ newidx->safe = idx->safe;
newidx->tableId = idx->tableId;
newidx->amId = idx->amId;
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
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
*
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
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());
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
PopActiveSnapshot();
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 3 of REINDEX CONCURRENTLY
*
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
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.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
* 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 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);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
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);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /*
+ * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+ * it only acquires an Xid to do some catalog manipulations, after the
+ * wait is over.
+ */
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /*
+ * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+ * it only acquires an Xid to do some catalog manipulations, after all the
+ * waiting has been completed.
+ */
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ 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 */
--
2.20.1
