Actually, I noticed two things. The first of them, addressed in this
new version of the patch, is that REINDEX CONCURRENTLY is doing a lot of
repetitive work by reopening each index and table in the build/validate
loops, so that they can report progress. This is easy to remedy by
adding a couple more members to the new struct (which I also renamed to
ReindexIndexInfo), for tableId and amId. The code seems a bit simpler
this way.
The other thing is that ReindexRelationConcurrenty seems to always be
called with the relations already locked by RangeVarGetRelidExtended.
So claiming to acquire locks on the relations over and over is
pointless. (I only noticed this because there was an obvious deadlock
hazard in one of the loops, that locked index before table.) I think we
should reduce all those to NoLock. My patch does not do that.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ca24620fd0..a52cb16b5e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -114,6 +114,17 @@ typedef struct ReindexErrorInfo
char relkind;
} ReindexErrorInfo;
+/*
+ * Index to process in ReindexRelationConcurrently
+ */
+typedef struct ReindexIndexInfo
+{
+ Oid indexId;
+ Oid tableId;
+ Oid amId;
+ bool safe; /* for set_indexsafe_procflags */
+} ReindexIndexInfo;
+
/*
* CheckIndexCompatible
* Determine whether an existing index definition is compatible with a
@@ -385,7 +396,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 +1575,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);
@@ -3132,10 +3145,16 @@ ReindexRelationConcurrently(Oid relationOid, int options)
get_rel_name(cellOid))));
else
{
+ ReindexIndexInfo *idx;
+
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
- indexIds = lappend_oid(indexIds, cellOid);
+ idx = palloc(sizeof(ReindexIndexInfo));
+ idx->indexId = cellOid;
+ /* other fields set later */
+
+ indexIds = lappend(indexIds, idx);
MemoryContextSwitchTo(oldcontext);
}
@@ -3172,13 +3191,18 @@ ReindexRelationConcurrently(Oid relationOid, int options)
get_rel_name(cellOid))));
else
{
+ ReindexIndexInfo *idx;
+
/*
* Save the list of relation OIDs in private
* context
*/
oldcontext = MemoryContextSwitchTo(private_context);
- indexIds = lappend_oid(indexIds, cellOid);
+ idx = palloc(sizeof(ReindexIndexInfo));
+ idx->indexId = cellOid;
+ indexIds = lappend(indexIds, idx);
+ /* other fields set later */
MemoryContextSwitchTo(oldcontext);
}
@@ -3197,6 +3221,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
Oid heapId = IndexGetRelation(relationOid,
(options & REINDEXOPT_MISSING_OK) != 0);
Relation heapRelation;
+ ReindexIndexInfo *idx;
/* if relation is missing, leave */
if (!OidIsValid(heapId))
@@ -3247,7 +3272,10 @@ 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(ReindexIndexInfo));
+ idx->indexId = relationOid;
+ indexIds = lappend(indexIds, idx);
+ /* other fields set later */
MemoryContextSwitchTo(oldcontext);
break;
@@ -3306,31 +3334,40 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
char *concurrentName;
- Oid indexId = lfirst_oid(lc);
+ ReindexIndexInfo *idx = lfirst(lc);
+ ReindexIndexInfo *newidx;
Oid newIndexId;
Relation indexRel;
Relation heapRel;
Relation newIndexRel;
LockRelId *lockrelid;
- indexRel = index_open(indexId, ShareUpdateExclusiveLock);
+ /* XXX pointless lock acquisition */
+ indexRel = index_open(idx->indexId, ShareUpdateExclusiveLock);
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;
+
/* This function shouldn't be called for temporary relations. */
if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
elog(ERROR, "cannot reindex a temporary table concurrently");
pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
- RelationGetRelid(heapRel));
+ idx->tableId);
+
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = 0; /* initializing */
- progress_vals[2] = indexId;
- progress_vals[3] = indexRel->rd_rel->relam;
+ progress_vals[2] = idx->indexId;
+ progress_vals[3] = idx->amId;
pgstat_progress_update_multi_param(4, progress_index, progress_vals);
/* Choose a temporary relation name for the new index */
- concurrentName = ChooseRelationName(get_rel_name(indexId),
+ concurrentName = ChooseRelationName(get_rel_name(idx->indexId),
NULL,
"ccnew",
get_rel_namespace(indexRel->rd_index->indrelid),
@@ -3338,7 +3375,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
/* Create new index definition based on given index */
newIndexId = index_concurrently_create_copy(heapRel,
- indexId,
+ idx->indexId,
concurrentName);
/*
@@ -3352,7 +3389,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
oldcontext = MemoryContextSwitchTo(private_context);
- newIndexIds = lappend_oid(newIndexIds, newIndexId);
+ newidx = palloc(sizeof(ReindexIndexInfo));
+ newidx->indexId = newIndexId;
+ newidx->safe = idx->safe;
+ newidx->tableId = idx->tableId;
+ newidx->amId = idx->amId;
+
+ newIndexIds = lappend(newIndexIds, newidx);
/*
* Save lockrelid to protect each relation from drop then close
@@ -3380,6 +3423,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
foreach(lc, heapRelationIds)
{
+ /* XXX pointless lock acquisition */
Relation heapRelation = table_open(lfirst_oid(lc), ShareUpdateExclusiveLock);
LockRelId *lockrelid;
LOCKTAG *heaplocktag;
@@ -3416,6 +3460,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
*
@@ -3433,10 +3482,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, newIndexIds)
{
- Relation newIndexRel;
- Oid newIndexId = lfirst_oid(lc);
- Oid heapId;
- Oid indexam;
+ ReindexIndexInfo *newidx = lfirst(lc);
/* Start new transaction for this index's concurrent build */
StartTransactionCommand();
@@ -3448,37 +3494,38 @@ 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());
- /*
- * Index relation has been closed by previous commit, so reopen it to
- * get its information.
- */
- newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
- heapId = newIndexRel->rd_index->indrelid;
- indexam = newIndexRel->rd_rel->relam;
- index_close(newIndexRel, NoLock);
-
/*
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = PROGRESS_CREATEIDX_PHASE_BUILD;
- progress_vals[2] = newIndexId;
- progress_vals[3] = indexam;
+ progress_vals[2] = newidx->indexId;
+ progress_vals[3] = newidx->amId;
pgstat_progress_update_multi_param(4, progress_index, progress_vals);
/* Perform concurrent build of new index */
- index_concurrently_build(heapId, newIndexId);
+ index_concurrently_build(newidx->tableId, newidx->indexId);
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,12 +3541,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, newIndexIds)
{
- Oid newIndexId = lfirst_oid(lc);
- Oid heapId;
+ ReindexIndexInfo *newidx = lfirst(lc);
TransactionId limitXmin;
Snapshot snapshot;
- Relation newIndexRel;
- Oid indexam;
StartTransactionCommand();
@@ -3510,6 +3554,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.
@@ -3517,27 +3565,19 @@ ReindexRelationConcurrently(Oid relationOid, int options)
snapshot = RegisterSnapshot(GetTransactionSnapshot());
PushActiveSnapshot(snapshot);
- /*
- * Index relation has been closed by previous commit, so reopen it to
- * get its information.
- */
- newIndexRel = index_open(newIndexId, ShareUpdateExclusiveLock);
- heapId = newIndexRel->rd_index->indrelid;
- indexam = newIndexRel->rd_rel->relam;
- index_close(newIndexRel, NoLock);
-
/*
* Update progress for the index to build, with the correct parent
* table involved.
*/
- pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, heapId);
+ pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+ newidx->tableId);
progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
progress_vals[1] = PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN;
- progress_vals[2] = newIndexId;
- progress_vals[3] = indexam;
+ progress_vals[2] = newidx->indexId;
+ progress_vals[3] = newidx->amId;
pgstat_progress_update_multi_param(4, progress_index, progress_vals);
- validate_index(heapId, newIndexId, snapshot);
+ validate_index(newidx->tableId, newidx->indexId, snapshot);
/*
* We can now do away with our active snapshot, we still need to save
@@ -3552,6 +3592,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,12 +3625,18 @@ 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);
+ ReindexIndexInfo *newidx = lfirst(lc2);
char *oldName;
- Oid oldIndexId = lfirst_oid(lc);
- Oid newIndexId = lfirst_oid(lc2);
- Oid heapId;
/*
* Check for user-requested abort. This is inside a transaction so as
@@ -3596,27 +3645,25 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
CHECK_FOR_INTERRUPTS();
- heapId = IndexGetRelation(oldIndexId, false);
-
/* Choose a relation name for old index */
- oldName = ChooseRelationName(get_rel_name(oldIndexId),
+ oldName = ChooseRelationName(get_rel_name(oldidx->indexId),
NULL,
"ccold",
- get_rel_namespace(heapId),
+ get_rel_namespace(oldidx->tableId),
false);
/*
* Swap old index with the new one. This also marks the new one as
* valid and the old one as not valid.
*/
- index_concurrently_swap(newIndexId, oldIndexId, oldName);
+ index_concurrently_swap(newidx->indexId, oldidx->indexId, oldName);
/*
* Invalidate the relcache for the table, so that after this commit
* all sessions will refresh any cached plans that might reference the
* index.
*/
- CacheInvalidateRelcacheByRelid(heapId);
+ CacheInvalidateRelcacheByRelid(oldidx->tableId);
/*
* CCI here so that subsequent iterations see the oldName in the
@@ -3632,6 +3679,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,8 +3699,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
- Oid oldIndexId = lfirst_oid(lc);
- Oid heapId;
+ ReindexIndexInfo *oldidx = lfirst(lc);
/*
* Check for user-requested abort. This is inside a transaction so as
@@ -3656,14 +3708,19 @@ ReindexRelationConcurrently(Oid relationOid, int options)
*/
CHECK_FOR_INTERRUPTS();
- heapId = IndexGetRelation(oldIndexId, false);
- index_concurrently_set_dead(heapId, oldIndexId);
+ index_concurrently_set_dead(oldidx->tableId, oldidx->indexId);
}
/* Commit this transaction to make the updates visible. */
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,11 +3738,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
foreach(lc, indexIds)
{
- Oid oldIndexId = lfirst_oid(lc);
+ ReindexIndexInfo *idx = lfirst(lc);
ObjectAddress object;
object.classId = RelationRelationId;
- object.objectId = oldIndexId;
+ object.objectId = idx->indexId;
object.objectSubId = 0;
add_exact_object_address(&object, objects);
@@ -3728,7 +3785,8 @@ ReindexRelationConcurrently(Oid relationOid, int options)
{
foreach(lc, newIndexIds)
{
- Oid indOid = lfirst_oid(lc);
+ ReindexIndexInfo *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 */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b8ca8cffd9..f2df682a9d 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2046,6 +2046,7 @@ Regis
RegisNode
RegisteredBgWorker
ReindexErrorInfo
+ReindexIndexInfo
ReindexObjectType
ReindexStmt
ReindexType
