On 2020-Nov-26, Alvaro Herrera wrote:
> So let's discuss the next step in this series: what to do about REINDEX
> CONCURRENTLY.
> [...]
... as in the attached.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..9c1c0aad3e 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.
@@ -1564,9 +1564,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 takes a snapshot to do some catalog manipulations, after the
+ * wait is over.
+ */
/* We should now definitely not be advertising any xmin. */
Assert(MyProc->xmin == InvalidTransactionId);
@@ -3020,6 +3022,13 @@ ReindexMultipleInternal(List *relids, int options)
* concerns, so there's no need for the more complicated concurrent build,
* anyway, and a non-concurrent reindex is more efficient.
*/
+
+typedef struct reindex_idx
+{
+ Oid indexId;
+ bool safe;
+} reindex_idx;
+
static bool
ReindexRelationConcurrently(Oid relationOid, int options)
{
@@ -3132,10 +3141,15 @@ ReindexRelationConcurrently(Oid relationOid, int options)
get_rel_name(cellOid))));
else
{
+ reindex_idx *idx;
+
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
- indexIds = lappend_oid(indexIds, cellOid);
+ idx = palloc(sizeof(reindex_idx));
+ idx->indexId = cellOid;
+
+ indexIds = lappend(indexIds, idx);
MemoryContextSwitchTo(oldcontext);
}
@@ -3172,13 +3186,17 @@ ReindexRelationConcurrently(Oid relationOid, int options)
get_rel_name(cellOid))));
else
{
+ reindex_idx *idx;
+
/*
* Save the list of relation OIDs in private
* context
*/
oldcontext = MemoryContextSwitchTo(private_context);
- indexIds = lappend_oid(indexIds, cellOid);
+ idx = palloc(sizeof(reindex_idx));
+ idx->indexId = cellOid;
+ indexIds = lappend(indexIds, idx);
MemoryContextSwitchTo(oldcontext);
}
@@ -3197,6 +3215,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
Oid heapId = IndexGetRelation(relationOid,
(options & REINDEXOPT_MISSING_OK) != 0);
Relation heapRelation;
+ reindex_idx *idx;
/* if relation is missing, leave */
if (!OidIsValid(heapId))
@@ -3247,7 +3266,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
* Save the list of relation OIDs in private context. Note
* that invalid indexes are allowed here.
*/
- indexIds = lappend_oid(indexIds, relationOid);
+ idx = palloc(sizeof(reindex_idx));
+ idx->indexId = relationOid;
+ indexIds = lappend(indexIds, idx);
MemoryContextSwitchTo(oldcontext);
break;
@@ -3306,8 +3327,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
char *concurrentName;
- Oid indexId = lfirst_oid(lc);
+ reindex_idx *index = lfirst(lc);
+ Oid indexId = index->indexId;
Oid newIndexId;
+ reindex_idx *newidx;
Relation indexRel;
Relation heapRel;
Relation newIndexRel;
@@ -3347,12 +3370,20 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
+ /* determine safety of this index for set_indexsafe_procflags */
+ index->safe = (newIndexRel->rd_indexprs == NIL &&
+ newIndexRel->rd_indpred == NIL);
+
/*
* Save the list of OIDs and locks in private context
*/
oldcontext = MemoryContextSwitchTo(private_context);
- newIndexIds = lappend_oid(newIndexIds, newIndexId);
+ newidx = palloc(sizeof(reindex_idx));
+ newidx->indexId = newIndexId;
+ newidx->safe = index->safe;
+
+ newIndexIds = lappend(newIndexIds, newidx);
/*
* Save lockrelid to protect each relation from drop then close
@@ -3416,6 +3447,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
*
@@ -3434,7 +3470,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, newIndexIds)
{
Relation newIndexRel;
- Oid newIndexId = lfirst_oid(lc);
+ reindex_idx *idx = lfirst(lc);
+ Oid newIndexId = idx->indexId;
Oid heapId;
Oid indexam;
@@ -3448,6 +3485,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
CHECK_FOR_INTERRUPTS();
+ /* Tell concurrent indexing to ignore us, if index qualifies */
+ if (idx->safe)
+ set_indexsafe_procflags();
+
/* Set ActiveSnapshot since functions in the indexes may need it */
PushActiveSnapshot(GetTransactionSnapshot());
@@ -3477,8 +3518,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
*
@@ -3494,7 +3541,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, newIndexIds)
{
- Oid newIndexId = lfirst_oid(lc);
+ reindex_idx *idx = lfirst(lc);
+ Oid newIndexId = idx->indexId;
Oid heapId;
TransactionId limitXmin;
Snapshot snapshot;
@@ -3510,6 +3558,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
CHECK_FOR_INTERRUPTS();
+ /* Tell concurrent indexing to ignore us, if index qualifies */
+ if (idx->safe)
+ set_indexsafe_procflags();
+
/*
* Take the "reference snapshot" that will be used by validate_index()
* to filter candidate tuples.
@@ -3552,6 +3604,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
* To ensure no deadlocks, we must commit and start yet another
* transaction, and do our wait before any snapshot has been taken in
* it.
+ *
+ * Because we don't take a snapshot in this transaction, there's no
+ * need to set the PROC_IN_SAFE_IC flag here.
*/
CommitTransactionCommand();
StartTransactionCommand();
@@ -3582,11 +3637,20 @@ 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)
{
char *oldName;
- Oid oldIndexId = lfirst_oid(lc);
- Oid newIndexId = lfirst_oid(lc2);
+ reindex_idx *oldidx = lfirst(lc);
+ reindex_idx *newidx = lfirst(lc2);
+ Oid oldIndexId = oldidx->indexId;
+ Oid newIndexId = newidx->indexId;
Oid heapId;
/*
@@ -3632,6 +3696,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /*
+ * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+ * it only takes a snapshot to do some catalog manipulations, after the
+ * wait is over.
+ */
+
/*
* Phase 5 of REINDEX CONCURRENTLY
*
@@ -3646,7 +3716,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
- Oid oldIndexId = lfirst_oid(lc);
+ reindex_idx *idx = lfirst(lc);
+ Oid oldIndexId = idx->indexId;
Oid heapId;
/*
@@ -3664,6 +3735,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
CommitTransactionCommand();
StartTransactionCommand();
+ /*
+ * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+ * it only takes a snapshot to do some catalog manipulations, after all
+ * the waiting has been completed.
+ */
+
/*
* Phase 6 of REINDEX CONCURRENTLY
*
@@ -3681,7 +3758,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
- Oid oldIndexId = lfirst_oid(lc);
+ reindex_idx *idx = lfirst(lc);
+ Oid oldIndexId = idx->indexId;
ObjectAddress object;
object.classId = RelationRelationId;
@@ -3728,7 +3806,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
{
foreach(lc, newIndexIds)
{
- Oid indOid = lfirst_oid(lc);
+ reindex_idx *idx = lfirst(lc);
+ Oid indOid = idx->indexId;
ereport(INFO,
(errmsg("index \"%s.%s\" was reindexed",
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e77f76ae8a..e1a6bc5170 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 */
