Subject: [PATCH v12 5/5] More refactoring

From: Justin Pryzby <[email-masked]>
Date: Sun, 1 Nov 2020 13:46:18 -0600

---
 src/backend/commands/indexcmds.c           | 183 +++++++++------------
 src/test/regress/expected/create_index.out |   4 +-
 2 files changed, 83 insertions(+), 104 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 08d44a1999..4586942960 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -102,8 +102,8 @@ static void ReindexMultipleInternal(List *relids,
 									ReindexParams *params);
 static bool ReindexRelationConcurrently(Oid relationOid,
 										ReindexParams *params);
-static List *ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds,
-										int options, MemoryContext private_context);
+static List *ReindexIndexesConcurrently(List *indexIds, int options,
+										MemoryContext private_context);
 static void update_relispartition(Oid relationId, bool newval);
 static inline void set_indexsafe_procflags(void);
 
@@ -2625,8 +2625,8 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel)
 			.indexId = indOid,
 			/* other fields set later */
 		};
+
 		ReindexIndexesConcurrently(list_make1(&idxinfo),
-				list_make1_oid(IndexGetRelation(indOid, false)),
 				params->options, CurrentMemoryContext);
 	}
 	else
@@ -2961,12 +2961,11 @@ reindex_error_callback(void *arg)

 /*
- * Given a list of index oids, return a list of leaf partitions by removing
- * any intermediate parents.  heaprels is populated with the corresponding
- * tables.
+ * Given a list of index oids, return a new list of leaf partitions by
+ * excluding any intermediate parents.
  */
 static List *
-leaf_partitions(List *inhoids, int options, List **heaprels)
+leaf_partitions(List *inhoids, int options)
 {
 	List		*partitions = NIL;
 	ListCell	*lc;
@@ -2998,7 +2997,6 @@ leaf_partitions(List *inhoids, int options, List **heaprels)
 
 		/* Save partition OID in current MemoryContext */
 		partitions = lappend_oid(partitions, partoid);
-		*heaprels = lappend_oid(*heaprels, tableoid);
 	}
 
 	return partitions;
@@ -3010,13 +3008,11 @@ leaf_partitions(List *inhoids, int options, List **heaprels)
  *
  * Reindex a set of partitions, per the partitioned index or table given
  * by the caller.
- * XXX: should be further refactored with logic from ReindexRelationConcurrently
  */
 static void
 ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 {
-	List	   *partitions = NIL,
-			*heaprels = NIL;
+	List	   *partitions = NIL;
 	char		relkind = get_rel_relkind(relid);
 	char	   *relname = get_rel_name(relid);
 	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
@@ -3070,7 +3066,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	if (relkind == RELKIND_PARTITIONED_INDEX)
 	{
 		old_context = MemoryContextSwitchTo(reindex_context);
-		partitions = leaf_partitions(inhoids, params->options, &heaprels);
+		partitions = leaf_partitions(inhoids, params->options);
 		MemoryContextSwitchTo(old_context);
 	} else {
 		/* Loop over parent tables */
@@ -3083,7 +3079,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 			parttable = table_open(partoid, ShareLock);
 			old_context = MemoryContextSwitchTo(reindex_context);
 			partindexes = RelationGetIndexList(parttable);
-			partindexes = leaf_partitions(partindexes, params->options, &heaprels);
+			partindexes = leaf_partitions(partindexes, params->options);
 			partitions = list_concat(partitions, partindexes);
 
 			MemoryContextSwitchTo(old_context);
@@ -3092,10 +3088,9 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	}
 
 	if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 &&
-		relkind == RELKIND_PARTITIONED_INDEX &&
 		get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
 	{
-		List			   *idxinfos = NIL;
+		List			    *idxinfos = NIL;
 		ReindexIndexInfo	*idxinfo;
 
 		old_context = MemoryContextSwitchTo(reindex_context);
@@ -3110,7 +3105,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		MemoryContextSwitchTo(old_context);
 
 		/* Process all indexes in a single loop */
-		ReindexIndexesConcurrently(idxinfos, heaprels, params->options, reindex_context);
+		ReindexIndexesConcurrently(idxinfos, params->options, reindex_context);
 	} else {
 		/*
 		 * Process each partition listed in a separate transaction.  Note that
@@ -3262,7 +3257,6 @@ ReindexMultipleInternal(List *relids, ReindexParams *params)
 static bool
 ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 {
-	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
 	List	   *newIndexIds = NIL;
 	ListCell   *lc,
@@ -3315,14 +3309,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 				 */
 				Relation	heapRelation;
 
-				/* Save the list of relation OIDs in private context */
-				oldcontext = MemoryContextSwitchTo(private_context);
-
-				/* Track this relation for session locks */
-				heapRelationIds = lappend_oid(heapRelationIds, relationOid);
-
-				MemoryContextSwitchTo(oldcontext);
-
 				if (IsCatalogRelationOid(relationOid))
 					ereport(ERROR,
 							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -3335,7 +3321,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 												  ShareUpdateExclusiveLock);
 					/* leave if relation does not exist */
 					if (!heapRelation)
-						break;
+						break; // XXX: lremove
 				}
 				else
 					heapRelation = table_open(relationOid,
@@ -3386,14 +3372,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 					Relation	toastRelation = table_open(toastOid,
 														   ShareUpdateExclusiveLock);
 
-					/* Save the list of relation OIDs in private context */
-					oldcontext = MemoryContextSwitchTo(private_context);
-
-					/* Track this relation for session locks */
-					heapRelationIds = lappend_oid(heapRelationIds, toastOid);
-
-					MemoryContextSwitchTo(oldcontext);
-
 					foreach(lc2, RelationGetIndexList(toastRelation))
 					{
 						Oid			cellOid = lfirst_oid(lc2);
@@ -3434,70 +3412,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 				break;
 			}
 		case RELKIND_INDEX:
-			{
-				Oid			heapId = IndexGetRelation(relationOid,
-													  (params->options & REINDEXOPT_MISSING_OK) != 0);
-				Relation	heapRelation;
-				ReindexIndexInfo *idx;
-
-				/* if relation is missing, leave */
-				if (!OidIsValid(heapId))
-					break;
-
-				if (IsCatalogRelationOid(heapId))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot reindex system catalogs concurrently")));
-
-				/*
-				 * Don't allow reindex for an invalid index on TOAST table, as
-				 * if rebuilt it would not be possible to drop it.  Match
-				 * error message in reindex_index().
-				 */
-				if (IsToastNamespace(get_rel_namespace(relationOid)) &&
-					!get_index_isvalid(relationOid))
-					ereport(ERROR,
-							(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-							 errmsg("cannot reindex invalid index on TOAST table")));
-
-				/*
-				 * Check if parent relation can be locked and if it exists,
-				 * this needs to be done at this stage as the list of indexes
-				 * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
-				 * should not be used once all the session locks are taken.
-				 */
-				if ((params->options & REINDEXOPT_MISSING_OK) != 0)
-				{
-					heapRelation = try_table_open(heapId,
-												  ShareUpdateExclusiveLock);
-					/* leave if relation does not exist */
-					if (!heapRelation)
-						break;
-				}
-				else
-					heapRelation = table_open(heapId,
-											  ShareUpdateExclusiveLock);
-				table_close(heapRelation, NoLock);
-
-				/* Save the list of relation OIDs in private context */
-				oldcontext = MemoryContextSwitchTo(private_context);
-
-				/* Track the heap relation of this index for session locks */
-				heapRelationIds = list_make1_oid(heapId);
-
-				/*
-				 * Save the list of relation OIDs in private context.  Note
-				 * that invalid indexes are allowed here.
-				 */
-				idx = palloc(sizeof(ReindexIndexInfo));
-				idx->indexId = relationOid;
-				indexIds = lappend(indexIds, idx);
-				/* other fields set later */
-
-				MemoryContextSwitchTo(oldcontext);
-				break;
-			}
-
 		case RELKIND_PARTITIONED_TABLE:
 		case RELKIND_PARTITIONED_INDEX:
 		default:
@@ -3521,10 +3435,8 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		return false;
 	}
 
-	Assert(heapRelationIds != NIL);
-
 	/* Do the work */
-	newIndexIds = ReindexIndexesConcurrently(indexIds, heapRelationIds, params->options, private_context);
+	newIndexIds = ReindexIndexesConcurrently(indexIds, params->options, private_context);
 
 	/* Log what we did */
 	if ((params->options & REINDEXOPT_VERBOSE) != 0)
@@ -3566,9 +3478,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
  * This is called by ReindexRelationConcurrently and
  */
 static List *
-ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, int options,
+ReindexIndexesConcurrently(List *indexIds, int options,
 		MemoryContext private_context)
 {
+	List		*heapRelationIds = NIL;
 	List	   *newIndexIds = NIL;
 	List	   *relationLocks = NIL;
 	List	   *lockTags = NIL;
@@ -3586,6 +3499,72 @@ ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, int options,
 	};
 	int64		progress_vals[4];
 
+	foreach(lc, indexIds)
+	{
+		ReindexIndexInfo	*idx = lfirst(lc);
+		Oid			indexrelid = idx->indexId;
+		Oid			heapId = IndexGetRelation(indexrelid,
+											  (options & REINDEXOPT_MISSING_OK) != 0);
+		Relation	heapRelation;
+
+		/* if relation is missing, leave */
+		if (!OidIsValid(heapId))
+			break; // XXX: ldelete?
+
+		if (IsCatalogRelationOid(heapId))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot reindex system catalogs concurrently")));
+
+		/*
+		 * Don't allow reindex for an invalid index on TOAST table, as
+		 * if rebuilt it would not be possible to drop it.  Match
+		 * error message in reindex_index().
+		 */
+		if (IsToastNamespace(get_rel_namespace(indexrelid)) &&
+			!get_index_isvalid(indexrelid))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot reindex invalid index on TOAST table")));
+
+		if (IsToastNamespace(get_rel_namespace(indexrelid)) &&
+			!get_index_isvalid(indexrelid))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot reindex invalid index on TOAST table")));
+
+		/*
+		 * Check if parent relation can be locked and if it exists,
+		 * this needs to be done at this stage as the list of indexes
+		 * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK
+		 * should not be used once all the session locks are taken.
+		 */
+		if ((options & REINDEXOPT_MISSING_OK) != 0)
+		{
+			heapRelation = try_table_open(heapId,
+										  ShareUpdateExclusiveLock);
+			/* leave if relation does not exist */
+			if (!heapRelation)
+				break; // ldelete
+		}
+		else
+			heapRelation = table_open(heapId,
+									  ShareUpdateExclusiveLock);
+		table_close(heapRelation, NoLock);
+
+		/* Save the list of relation OIDs in private context */
+		oldcontext = MemoryContextSwitchTo(private_context);
+
+		/* Track the heap relation of this index for session locks */
+		heapRelationIds = lappend_oid(heapRelationIds, heapId);
+		// heapRelationIds = list_make1_oid(heapId);
+
+		/* Note that invalid indexes are allowed here. */
+
+		MemoryContextSwitchTo(oldcontext);
+		// break;
+	}
+
 	/*-----
 	 * Now we have all the indexes we want to process in indexIds.
 	 *
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 4a03ab2abb..fc6afab58a 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2470,12 +2470,12 @@ COMMIT;
 REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index
-ERROR:  concurrent index creation on system catalog tables is not supported
+ERROR:  cannot reindex system catalogs concurrently
 -- These are the toast table and index of pg_authid.
 REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table
 ERROR:  cannot reindex system catalogs concurrently
 REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
-ERROR:  concurrent index creation on system catalog tables is not supported
+ERROR:  cannot reindex system catalogs concurrently
 REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
 ERROR:  cannot reindex system catalogs concurrently
 -- Warns about catalog relations
-- 
2.17.0

--MfFXiAuoTsnnDAfZ--