Subject: Re: CREATE INDEX CONCURRENTLY on partitioned index

From: Alexander Pyhalov <[email-masked]>
Date: Thu, 10 Feb 2022 18:07:08 +0300

Justin Pryzby писал 2021-02-26 21:20:
> On Mon, Feb 15, 2021 at 10:07:05PM +0300, Anastasia Lubennikova wrote:
>> 5) Speaking of documentation, I think we need to add a paragraph about 
>> CIC
>> on partitioned indexes which will explain that invalid indexes may 
>> appear
>> and what user should do to fix them.
> 
> I'm not sure about that - it's already documented in general, for
> nonpartitioned indexes.

Hi.

I've rebased patches and tried to fix issues I've seen. I've fixed 
reference after table_close() in the first patch (can be seen while 
building with CPPFLAGS='-DRELCACHE_FORCE_RELEASE'). It seems childidxs 
shouldn't live in ind_context, so I moved it out of it. Updated 
documentation to state that CIC can leave invalid or valid indexes on 
partitions if it's not succeeded. Also merged old 
0002-f-progress-reporting.patch and 
0003-WIP-Add-SKIPVALID-flag-for-more-integration.patch. It seems the 
first one didn't really fixed issue with progress report (as 
ReindexRelationConcurrently() uses pgstat_progress_start_command(), 
which seems to mess up the effect of this command in DefineIndex()). 
Note, that third patch completely removes attempts to report create 
index progress correctly (reindex reports about individual commands, not 
the whole CREATE INDEX).

So I've added 0003-Try-to-fix-create-index-progress-report.patch, which 
tries to fix the mess with create index progress report. It introduces 
new flag REINDEXOPT_REPORT_CREATE_PART to ReindexParams->options. Given 
this flag, ReindexRelationConcurrently() will not report about 
individual operations start/stop, but ReindexMultipleInternal() will 
report about reindexed partitions. To make the issue worse, some 
partitions can be handled in ReindexPartitions() and 
ReindexMultipleInternal() should know how many to correctly update 
PROGRESS_CREATEIDX_PARTITIONS_DONE counter. Also it needs IndexOid to 
correctly generate pg_stat_progress_create_index record, so we pass 
these parameters to it.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

From eaad7c3ed2fda93fdb91aea60294f60489444bf7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:28:42 +0300
Subject: [PATCH 1/4] Allow CREATE INDEX CONCURRENTLY on partitioned table

0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-table.patch from
https://www.postgresql.org/message-id/[email-masked]

Fixes:
  - rel was used after table_close();
  - it seems childidxs shouldn't live in ind_context;
  - updated doc.
---
 doc/src/sgml/ref/create_index.sgml     |  14 +--
 src/backend/commands/indexcmds.c       | 151 ++++++++++++++++++-------
 src/test/regress/expected/indexing.out |  60 +++++++++-
 src/test/regress/sql/indexing.sql      |  18 ++-
 4 files changed, 186 insertions(+), 57 deletions(-)

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 91eaaabc90f..ffa98692430 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -641,7 +641,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while creating index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -688,15 +691,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
-    Concurrent builds for indexes on partitioned tables are currently not
-    supported.  However, you may concurrently build the index on each
-    partition individually and then finally create the partitioned index
-    non-concurrently in order to reduce the time where writes to the
-    partitioned table will be locked out.  In this case, building the
-    partitioned index is a metadata only operation.
-   </para>
-
   </refsect2>
  </refsect1>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 560dcc87a2c..666ced8e1d7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@

 /* non-export function prototypes */
+static void reindex_invalid_child_indexes(Oid indexRelationId);
 static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
 static void CheckPredicate(Expr *predicate);
 static void ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -670,17 +671,6 @@ DefineIndex(Oid relationId,
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
 	if (partitioned)
 	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
 		if (stmt->excludeOpNames)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1119,6 +1109,11 @@ DefineIndex(Oid relationId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1174,18 +1169,30 @@ DefineIndex(Oid relationId,
 		partdesc = RelationGetPartitionDesc(rel, true);
 		if ((!stmt->relation || stmt->relation->inh) && partdesc->nparts > 0)
 		{
+			/*
+			 * Need to close the relation before recursing into children, so
+			 * copy needed data into a longlived context.
+			 */
+
+			MemoryContext	ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+					ALLOCSET_DEFAULT_SIZES);
+			MemoryContext	oldcontext = MemoryContextSwitchTo(ind_context);
 			int			nparts = partdesc->nparts;
 			Oid		   *part_oids = palloc(sizeof(Oid) * nparts);
 			bool		invalidate_parent = false;
 			TupleDesc	parentDesc;
 			Oid		   *opfamOids;
+			char		*relname;
 
 			pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
 										 nparts);
 
 			memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+			parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
+			relname = pstrdup(RelationGetRelationName(rel));
+			table_close(rel, NoLock);
+			MemoryContextSwitchTo(oldcontext);
 
-			parentDesc = RelationGetDescr(rel);
 			opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
 			for (i = 0; i < numberOfKeyAttributes; i++)
 				opfamOids[i] = get_opclass_family(classObjectId[i]);
@@ -1220,18 +1227,21 @@ DefineIndex(Oid relationId,
 						ereport(ERROR,
 								(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 								 errmsg("cannot create unique index on partitioned table \"%s\"",
-										RelationGetRelationName(rel)),
+										relname),
 								 errdetail("Table \"%s\" contains partitions that are foreign tables.",
-										   RelationGetRelationName(rel))));
+										   relname)));
 
 					table_close(childrel, lockmode);
 					continue;
 				}
 
 				childidxs = RelationGetIndexList(childrel);
+
+				oldcontext = MemoryContextSwitchTo(ind_context);
 				attmap =
 					build_attrmap_by_name(RelationGetDescr(childrel),
 										  parentDesc);
+				MemoryContextSwitchTo(oldcontext);
 
 				foreach(cell, childidxs)
 				{
@@ -1302,10 +1312,14 @@ DefineIndex(Oid relationId,
 				 */
 				if (!found)
 				{
-					IndexStmt  *childStmt = copyObject(stmt);
+					IndexStmt  *childStmt;
 					bool		found_whole_row;
 					ListCell   *lc;
 
+					oldcontext = MemoryContextSwitchTo(ind_context);
+					childStmt = copyObject(stmt);
+					MemoryContextSwitchTo(oldcontext);
+
 					/*
 					 * We can't use the same index name for the child index,
 					 * so clear idxname to let the recursive invocation choose
@@ -1357,12 +1371,21 @@ DefineIndex(Oid relationId,
 								createdConstraintId,
 								is_alter_table, check_rights, check_not_in_use,
 								skip_build, quiet);
+					if (concurrent)
+					{
+						PopActiveSnapshot();
+						PushActiveSnapshot(GetTransactionSnapshot());
+						invalidate_parent = true;
+					}
 				}
 
-				pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
-											 i + 1);
+				/* For concurrent build, this is a catalog-only stage */
+				if (!concurrent)
+					pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+												 i + 1);
 				free_attrmap(attmap);
 			}
+			pfree(relname);
 
 			/*
 			 * The pg_index row we inserted for this index was marked
@@ -1370,41 +1393,33 @@ DefineIndex(Oid relationId,
 			 * invalid, this is incorrect, so update our row to invalid too.
 			 */
 			if (invalidate_parent)
-			{
-				Relation	pg_index = table_open(IndexRelationId, RowExclusiveLock);
-				HeapTuple	tup,
-							newtup;
-
-				tup = SearchSysCache1(INDEXRELID,
-									  ObjectIdGetDatum(indexRelationId));
-				if (!HeapTupleIsValid(tup))
-					elog(ERROR, "cache lookup failed for index %u",
-						 indexRelationId);
-				newtup = heap_copytuple(tup);
-				((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
-				CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
-				ReleaseSysCache(tup);
-				table_close(pg_index, RowExclusiveLock);
-				heap_freetuple(newtup);
-			}
-		}
+				index_set_state_flags(indexRelationId, INDEX_DROP_CLEAR_VALID);
+		} else
+			table_close(rel, NoLock);
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
 		 * done here.
 		 */
-		table_close(rel, NoLock);
 		if (!OidIsValid(parentIndexId))
+		{
+			if (concurrent)
+				reindex_invalid_child_indexes(indexRelationId);
+
 			pgstat_progress_end_command();
+		}
+
 		return address;
 	}
 
-	if (!concurrent)
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
-		table_close(rel, NoLock);
+		/*
+		 * We're done if this is the top-level index,
+		 * or the catalog-only phase of a partition built concurrently
+		 */
 
-		/* If this is the top-level index, we're done. */
+		table_close(rel, NoLock);
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 
@@ -1617,6 +1632,62 @@ DefineIndex(Oid relationId,
 	return address;
 }
 
+/* Reindex invalid child indexes created earlier */
+static void
+reindex_invalid_child_indexes(Oid indexRelationId)
+{
+	ListCell *lc;
+	int		npart = 0;
+	ReindexParams params = {
+		.options = REINDEXOPT_CONCURRENTLY
+	};
+
+	MemoryContext	ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+			ALLOCSET_DEFAULT_SIZES);
+	MemoryContext	oldcontext;
+	List		*childs = find_inheritance_children(indexRelationId, ShareLock);
+	List		*partitions = NIL;
+
+	PreventInTransactionBlock(true, "REINDEX INDEX");
+
+	foreach (lc, childs)
+	{
+		Oid			partoid = lfirst_oid(lc);
+
+		/* XXX: need to retrofit progress reporting into it */
+		// pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+									 // npart++);
+
+		if (get_index_isvalid(partoid) ||
+				!RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
+			continue;
+
+		/* Save partition OID */
+		oldcontext = MemoryContextSwitchTo(ind_context);
+		partitions = lappend_oid(partitions, partoid);
+		MemoryContextSwitchTo(oldcontext);
+	}
+
+	/*
+	 * Process each partition listed in a separate transaction.  Note that
+	 * this commits and then starts a new transaction immediately.
+	 * XXX: since this is done in 2*N transactions, it could just as well
+	 * call ReindexRelationConcurrently directly
+	 */
+	ReindexMultipleInternal(partitions, &params);
+
+	/*
+	 * CIC needs to mark a partitioned index as VALID, which itself
+	 * requires setting READY, which is unset for CIC (even though
+	 * it's meaningless for an index without storage).
+	 * This must be done only while holding a lock which precludes adding
+	 * partitions.
+	 * See also: validatePartitionedIndex().
+	 */
+	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+	CommandCounterIncrement();
+	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
+}
 
 /*
  * CheckMutability
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 193f7801912..a4ccae50de3 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,63 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
-ERROR:  cannot create index on partitioned table "idxpart" concurrently
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+ERROR:  could not create unique index "idxpart2_a_idx2_ccnew"
+DETAIL:  Key (a)=(10) is duplicated.
+\d idxpart
+        Partitioned table "public.idxpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "idxpart_a_idx" btree (a)
+    "idxpart_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 2 (Use \d+ to list them.)
+
+\d idxpart1
+        Partitioned table "public.idxpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1_a_idx" btree (a) INVALID
+    "idxpart1_a_idx1" btree (a)
+    "idxpart1_a_idx2" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart2
+              Table "public.idxpart2"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (10) TO (20)
+Indexes:
+    "idxpart2_a_idx" btree (a)
+    "idxpart2_a_idx1" btree (a)
+    "idxpart2_a_idx2" UNIQUE, btree (a) INVALID
+    "idxpart2_a_idx2_ccnew" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email-masked]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 42f398b67c2..3d4b6e9bc95 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -29,10 +29,22 @@ select relname, relkind, relhassubclass, inhparent::regclass
 	where relname like 'idxpart%' order by relname;
 drop table idxpart;
 
--- Some unsupported features
+-- CIC on partitioned table
 create table idxpart (a int, b int, c text) partition by range (a);
-create table idxpart1 partition of idxpart for values from (0) to (10);
-create index concurrently on idxpart (a);
+create table idxpart1 partition of idxpart for values from (0) to (10) partition by range(a);
+create table idxpart11 partition of idxpart1 for values from (0) to (10) partition by range(a);
+create table idxpart111 partition of idxpart11 default partition by range(a);
+create table idxpart1111 partition of idxpart111 default partition by range(a);
+create table idxpart2 partition of idxpart for values from (10) to (20);
+insert into idxpart2 values(10),(10); -- not unique
+create index concurrently on idxpart (a); -- partitioned
+create index concurrently on idxpart1 (a); -- partitioned and partition
+create index concurrently on idxpart11 (a); -- partitioned and partition, with no leaves
+create index concurrently on idxpart2 (a); -- leaf
+create unique index concurrently on idxpart (a); -- partitioned, unique failure
+\d idxpart
+\d idxpart1
+\d idxpart2
 drop table idxpart;
 
 -- Verify bugfix with query on indexed partitioned table with no partitions
-- 
2.25.1

From 91bed69a737ca73fb5f79725e7bffc31e617b61b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:31:40 +0300
Subject: [PATCH 2/4] Add SKIPVALID flag for more integration

Combined
0002-f-progress-reporting.patch and
0003-WIP-Add-SKIPVALID-flag-for-more-integration.patch from
https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/commands/indexcmds.c | 57 ++++++++++----------------------
 src/include/catalog/index.h      |  1 +
 2 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 666ced8e1d7..56e4c0b7575 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1632,59 +1632,33 @@ DefineIndex(Oid relationId,
 	return address;
 }
 
-/* Reindex invalid child indexes created earlier */
+/*
+ * Reindex invalid child indexes created earlier thereby validating
+ * the parent index.
+ */
 static void
 reindex_invalid_child_indexes(Oid indexRelationId)
 {
-	ListCell *lc;
-	int		npart = 0;
 	ReindexParams params = {
-		.options = REINDEXOPT_CONCURRENTLY
+		.options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID
 	};
 
-	MemoryContext	ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
-			ALLOCSET_DEFAULT_SIZES);
-	MemoryContext	oldcontext;
-	List		*childs = find_inheritance_children(indexRelationId, ShareLock);
-	List		*partitions = NIL;
-
-	PreventInTransactionBlock(true, "REINDEX INDEX");
-
-	foreach (lc, childs)
-	{
-		Oid			partoid = lfirst_oid(lc);
-
-		/* XXX: need to retrofit progress reporting into it */
-		// pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
-									 // npart++);
-
-		if (get_index_isvalid(partoid) ||
-				!RELKIND_HAS_STORAGE(get_rel_relkind(partoid)))
-			continue;
-
-		/* Save partition OID */
-		oldcontext = MemoryContextSwitchTo(ind_context);
-		partitions = lappend_oid(partitions, partoid);
-		MemoryContextSwitchTo(oldcontext);
-	}
-
-	/*
-	 * Process each partition listed in a separate transaction.  Note that
-	 * this commits and then starts a new transaction immediately.
-	 * XXX: since this is done in 2*N transactions, it could just as well
-	 * call ReindexRelationConcurrently directly
-	 */
-	ReindexMultipleInternal(partitions, &params);
-
 	/*
 	 * CIC needs to mark a partitioned index as VALID, which itself
 	 * requires setting READY, which is unset for CIC (even though
 	 * it's meaningless for an index without storage).
 	 * This must be done only while holding a lock which precludes adding
 	 * partitions.
-	 * See also: validatePartitionedIndex().
 	 */
+	CommandCounterIncrement();
 	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+
+	/*
+	 * Process each partition listed in a separate transaction.  Note that
+	 * this commits and then starts a new transaction immediately.
+	 */
+	ReindexPartitions(indexRelationId, &params, true);
+
 	CommandCounterIncrement();
 	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
 }
@@ -3106,6 +3080,11 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		if (!RELKIND_HAS_STORAGE(partkind))
 			continue;
 
+		/* Skip valid indexes, if requested */
+		if ((params->options & REINDEXOPT_SKIPVALID) != 0 &&
+				get_index_isvalid(partoid))
+			continue;
+
 		Assert(partkind == RELKIND_INDEX ||
 			   partkind == RELKIND_RELATION);
 
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index a1d6e3b645f..c31b66ad0b9 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -42,6 +42,7 @@ typedef struct ReindexParams
 #define REINDEXOPT_REPORT_PROGRESS 0x02 /* report pgstat progress */
 #define REINDEXOPT_MISSING_OK 	0x04	/* skip missing relations */
 #define REINDEXOPT_CONCURRENTLY	0x08	/* concurrent mode */
+#define REINDEXOPT_SKIPVALID	0x10	/* skip valid indexes */
 
 /* state info for validate_index bulkdelete callback */
 typedef struct ValidateIndexState
-- 
2.25.1

From 1b9cb0f8f32c1863a52c5a3fd53103537a938ebd Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Tue, 8 Feb 2022 21:15:05 +0300
Subject: [PATCH 3/4] Try to fix create index progress report

---
 src/backend/commands/indexcmds.c | 67 ++++++++++++++++++++++++++------
 src/include/catalog/index.h      |  1 +
 2 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 56e4c0b7575..b237c30bc80 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -99,11 +99,14 @@ static void reindex_error_callback(void *args);
 static void ReindexPartitions(Oid relid, ReindexParams *params,
 							  bool isTopLevel);
 static void ReindexMultipleInternal(List *relids,
-									ReindexParams *params);
+									ReindexParams *params,
+									Oid parent,
+									int npart);
 static bool ReindexRelationConcurrently(Oid relationOid,
 										ReindexParams *params);
 static void update_relispartition(Oid relationId, bool newval);
 static inline void set_indexsafe_procflags(void);
+static void report_create_partition_index_done(Oid parent, int npart);
 
 /*
  * callback argument type for RangeVarCallbackForReindexIndex()
@@ -1184,6 +1187,7 @@ DefineIndex(Oid relationId,
 			Oid		   *opfamOids;
 			char		*relname;
 
+
 			pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
 										 nparts);
 
@@ -1640,7 +1644,7 @@ static void
 reindex_invalid_child_indexes(Oid indexRelationId)
 {
 	ReindexParams params = {
-		.options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID
+		.options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID | REINDEXOPT_REPORT_CREATE_PART
 	};
 
 	/*
@@ -1653,6 +1657,8 @@ reindex_invalid_child_indexes(Oid indexRelationId)
 	CommandCounterIncrement();
 	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
 
+	pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE, PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN);
+
 	/*
 	 * Process each partition listed in a separate transaction.  Note that
 	 * this commits and then starts a new transaction immediately.
@@ -2987,7 +2993,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	 * Process each relation listed in a separate transaction.  Note that this
 	 * commits and then starts a new transaction immediately.
 	 */
-	ReindexMultipleInternal(relids, params);
+	ReindexMultipleInternal(relids, params, InvalidOid, 0);
 
 	MemoryContextDelete(private_context);
 }
@@ -3023,6 +3029,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	char		relkind = get_rel_relkind(relid);
 	char	   *relname = get_rel_name(relid);
 	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
+	int			npart = 1;
 	MemoryContext reindex_context;
 	List	   *inhoids;
 	ListCell   *lc;
@@ -3083,7 +3090,11 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		/* Skip valid indexes, if requested */
 		if ((params->options & REINDEXOPT_SKIPVALID) != 0 &&
 				get_index_isvalid(partoid))
+		{
+			if (params->options & REINDEXOPT_REPORT_CREATE_PART)
+				report_create_partition_index_done(relid, npart++);
 			continue;
+		}
 
 		Assert(partkind == RELKIND_INDEX ||
 			   partkind == RELKIND_RELATION);
@@ -3098,7 +3109,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	 * Process each partition listed in a separate transaction.  Note that
 	 * this commits and then starts a new transaction immediately.
 	 */
-	ReindexMultipleInternal(partitions, params);
+	ReindexMultipleInternal(partitions, params, relid, npart);
 
 	/*
 	 * Clean up working storage --- note we must do this after
@@ -3116,7 +3127,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
  * and starts a new transaction when finished.
  */
 static void
-ReindexMultipleInternal(List *relids, ReindexParams *params)
+ReindexMultipleInternal(List *relids, ReindexParams *params, Oid parent, int npart)
 {
 	ListCell   *l;
 
@@ -3210,6 +3221,9 @@ ReindexMultipleInternal(List *relids, ReindexParams *params)
 		}
 
 		CommitTransactionCommand();
+
+		if (params->options & REINDEXOPT_REPORT_CREATE_PART)
+			report_create_partition_index_done(parent, npart++);
 	}
 
 	StartTransactionCommand();
@@ -3592,7 +3606,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
 			elog(ERROR, "cannot reindex a temporary table concurrently");
 
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+		/* Don't overwrite CREATE INDEX command */
+		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
 									  idx->tableId);
 
 		progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
@@ -3745,9 +3761,11 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 		/*
 		 * Update progress for the index to build, with the correct parent
-		 * table involved.
+		 * table involved.  Don't overwrite CREATE INDEX command.
 		 */
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX, newidx->tableId);
+		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+			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] = newidx->indexId;
@@ -3809,10 +3827,12 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 		/*
 		 * Update progress for the index to build, with the correct parent
-		 * table involved.
+		 * table involved. Don't overwrite CREATE INDEX command.
 		 */
-		pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
+		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+			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] = newidx->indexId;
@@ -4047,7 +4067,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 	MemoryContextDelete(private_context);
 
-	pgstat_progress_end_command();
+	/* Don't overwrite CREATE INDEX command. */
+	if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+		pgstat_progress_end_command();
 
 	return true;
 }
@@ -4183,6 +4205,29 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
 	}
 }
 
+/*
+ * Update pgstat progress report to indicate that create index on
+ * partition was finished.
+ */
+static void
+report_create_partition_index_done(Oid index, int npart)
+{
+	const int   progress_cols[] = {
+		PROGRESS_CREATEIDX_COMMAND,
+		PROGRESS_CREATEIDX_INDEX_OID,
+		PROGRESS_CREATEIDX_PHASE,
+		PROGRESS_CREATEIDX_PARTITIONS_DONE
+	};
+	const int64 progress_vals[] = {
+		PROGRESS_CREATEIDX_COMMAND_CREATE_CONCURRENTLY,
+		index,
+		PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN,
+		npart
+	};
+
+	pgstat_progress_update_multi_param(4, progress_cols, progress_vals);
+}
+
 /*
  * Subroutine of IndexSetParentIndex to update the relispartition flag of the
  * given index to the given value.
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c31b66ad0b9..b5b0a71e7d4 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -43,6 +43,7 @@ typedef struct ReindexParams
 #define REINDEXOPT_MISSING_OK 	0x04	/* skip missing relations */
 #define REINDEXOPT_CONCURRENTLY	0x08	/* concurrent mode */
 #define REINDEXOPT_SKIPVALID	0x10	/* skip valid indexes */
+#define REINDEXOPT_REPORT_CREATE_PART	0x20	/* report that index was created for partition */
 
 /* state info for validate_index bulkdelete callback */
 typedef struct ValidateIndexState
-- 
2.25.1

From 62f2816188d3d38fe0d76d3212d6f551a59c0ae7 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:39:59 +0300
Subject: [PATCH 4/4] ReindexPartitions() to set indisvalid

0004-ReindexPartitions-to-set-indisvalid.patch from
https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/commands/indexcmds.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b237c30bc80..355a7626549 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1651,8 +1651,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
 	 * CIC needs to mark a partitioned index as VALID, which itself
 	 * requires setting READY, which is unset for CIC (even though
 	 * it's meaningless for an index without storage).
-	 * This must be done only while holding a lock which precludes adding
-	 * partitions.
 	 */
 	CommandCounterIncrement();
 	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
@@ -1664,9 +1662,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
 	 * this commits and then starts a new transaction immediately.
 	 */
 	ReindexPartitions(indexRelationId, &params, true);
-
-	CommandCounterIncrement();
-	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
 }
 
 /*
@@ -3111,6 +3106,24 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	 */
 	ReindexMultipleInternal(partitions, params, relid, npart);
 
+	/*
+	 * If indexes exist on all of the partitioned table's children, and we
+	 * just reindexed them, then we know they're valid, and so can mark the
+	 * parent index as valid.
+	 * This handles the case of CREATE INDEX CONCURRENTLY.
+	 * See also: validatePartitionedIndex().
+	 */
+	if (get_rel_relkind(relid) == RELKIND_PARTITIONED_INDEX
+			&& !get_index_isvalid(relid))
+	{
+		Oid	tableoid = IndexGetRelation(relid, false);
+		List	*child_tables = find_all_inheritors(tableoid, ShareLock, NULL);
+
+		/* Both lists include their parent relation as well as any intermediate partitioned rels */
+		if (list_length(inhoids) == list_length(child_tables))
+			index_set_state_flags(relid, INDEX_CREATE_SET_VALID);
+	}
+
 	/*
 	 * Clean up working storage --- note we must do this after
 	 * StartTransactionCommand, else we might be trying to delete the active
-- 
2.25.1

---

From: Alexander Pyhalov <[email-masked]>
Date: Wed, 02 Mar 2022 10:24:59 +0300

Hi.

I've added 0005-Mark-intermediate-partitioned-indexes-as-valid.patch 
which fixed the following issues - when partitioned index is created, 
indexes on intermediate partitioned tables were preserved in invalid 
state. Also added some more tests.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

From 18fa3c27a3311294a7abfdc0674ef6143c65423b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:28:42 +0300
Subject: [PATCH 1/5] Allow CREATE INDEX CONCURRENTLY on partitioned table

Fixes:

-   <para>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index cd30f15eba6..a34a1b133a0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@

 	if (stmt->deferrable)

 

 
 				foreach(cell, childidxs)
 				{

 
 			/*
 
 		/*
 
-	if (!concurrent)

 
 
 /*
  * CheckMutability
 
 drop table idxpart;

From 1cba217c9293a4dac1b08451000442733c8c78cd Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:31:40 +0300
Subject: [PATCH 2/5] Add SKIPVALID flag for more integration

Combined

index a34a1b133a0..090e792ff47 100644

 

From 1a4e873d247dffc1b1af16a546f0d28714d3fb9b Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Tue, 8 Feb 2022 21:15:05 +0300
Subject: [PATCH 3/5] Try to fix create index progress report

---

index 090e792ff47..57df92985fe 100644
 
 /*
 
+

 	/*

 
 
 	/*

 

 
 		/*
 
 		/*

 
 	return true;
 }
 
+/*

From 44055a8f644d153a6df919eba02db41f8085b9e8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:39:59 +0300
Subject: [PATCH 4/5] ReindexPartitions() to set indisvalid

index 57df92985fe..21f1ceaea63 100644
 
 /*
 
+	/*

From 75db7db9fae509c64609315b86aa85706e710e2b Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Mon, 28 Feb 2022 10:50:58 +0300
Subject: [PATCH 5/5] Mark intermediate partitioned indexes as valid

---
 src/backend/commands/indexcmds.c       | 33 ++++++++++-
 src/test/regress/expected/indexing.out | 80 +++++++++++++++++++++++++-
 src/test/regress/sql/indexing.sql      |  8 +++
 3 files changed, 118 insertions(+), 3 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 21f1ceaea63..fabb5b14898 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3021,6 +3021,7 @@ static void
 ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 {
 	List	   *partitions = NIL;
+	List	   *inhpartindexes = NIL;
 	char		relkind = get_rel_relkind(relid);
 	char	   *relname = get_rel_name(relid);
 	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
@@ -3075,6 +3076,17 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		char		partkind = get_rel_relkind(partoid);
 		MemoryContext old_context;
 
+		/* Create a list of invalid inherited partitioned indexes */
+		if (partkind == RELKIND_PARTITIONED_INDEX)
+		{
+			if (partoid == relid || get_index_isvalid(partoid))
+				continue;
+
+			old_context = MemoryContextSwitchTo(reindex_context);
+			inhpartindexes = lappend_oid(inhpartindexes, partoid);
+			MemoryContextSwitchTo(old_context);
+		}
+
 		/*
 		 * This discards partitioned tables, partitioned indexes and foreign
 		 * tables.
@@ -3119,9 +3131,28 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		Oid	tableoid = IndexGetRelation(relid, false);
 		List	*child_tables = find_all_inheritors(tableoid, ShareLock, NULL);
 
-		/* Both lists include their parent relation as well as any intermediate partitioned rels */
+		/*
+		 * Both lists include their parent relation as well as any
+		 * intermediate partitioned rels
+		 */
 		if (list_length(inhoids) == list_length(child_tables))
+		{
 			index_set_state_flags(relid, INDEX_CREATE_SET_VALID);
+
+			/* Mark any intermediate partitioned index as valid */
+			foreach(lc, inhpartindexes)
+			{
+				Oid         partoid = lfirst_oid(lc);
+
+				Assert(get_rel_relkind(partoid) == RELKIND_PARTITIONED_INDEX);
+				Assert(!get_index_isvalid(partoid));
+
+				/* Can't mark an index valid without marking it ready */
+				index_set_state_flags(partoid, INDEX_CREATE_SET_READY);
+				CommandCounterIncrement();
+				index_set_state_flags(partoid, INDEX_CREATE_SET_VALID);
+			}
+		}
 	}
 
 	/*
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index a4ccae50de3..b4f1aea6fca 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -57,6 +57,8 @@ create table idxpart11 partition of idxpart1 for values from (0) to (10) partiti
 create table idxpart111 partition of idxpart11 default partition by range(a);
 create table idxpart1111 partition of idxpart111 default partition by range(a);
 create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
 insert into idxpart2 values(10),(10); -- not unique
 create index concurrently on idxpart (a); -- partitioned
 create index concurrently on idxpart1 (a); -- partitioned and partition
@@ -76,7 +78,7 @@ Partition key: RANGE (a)
 Indexes:
     "idxpart_a_idx" btree (a)
     "idxpart_a_idx1" UNIQUE, btree (a) INVALID
-Number of partitions: 2 (Use \d+ to list them.)
+Number of partitions: 3 (Use \d+ to list them.)
 
 \d idxpart1
         Partitioned table "public.idxpart1"
@@ -88,11 +90,59 @@ Number of partitions: 2 (Use \d+ to list them.)
 Partition of: idxpart FOR VALUES FROM (0) TO (10)
 Partition key: RANGE (a)
 Indexes:
-    "idxpart1_a_idx" btree (a) INVALID
+    "idxpart1_a_idx" btree (a)
     "idxpart1_a_idx1" btree (a)
     "idxpart1_a_idx2" UNIQUE, btree (a) INVALID
 Number of partitions: 1 (Use \d+ to list them.)
 
+\d idxpart11
+       Partitioned table "public.idxpart11"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart1 FOR VALUES FROM (0) TO (10)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart11_a_idx" btree (a)
+    "idxpart11_a_idx1" btree (a)
+    "idxpart11_a_idx2" btree (a)
+    "idxpart11_a_idx3" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart111
+       Partitioned table "public.idxpart111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart11 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart111_a_idx" btree (a)
+    "idxpart111_a_idx1" btree (a)
+    "idxpart111_a_idx2" btree (a)
+    "idxpart111_a_idx3" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart1111
+      Partitioned table "public.idxpart1111"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart111 DEFAULT
+Partition key: RANGE (a)
+Indexes:
+    "idxpart1111_a_idx" btree (a)
+    "idxpart1111_a_idx1" btree (a)
+    "idxpart1111_a_idx2" btree (a)
+    "idxpart1111_a_idx3" UNIQUE, btree (a) INVALID
+Number of partitions: 0
+
 \d idxpart2
               Table "public.idxpart2"
  Column |  Type   | Collation | Nullable | Default 
@@ -107,6 +157,32 @@ Indexes:
     "idxpart2_a_idx2" UNIQUE, btree (a) INVALID
     "idxpart2_a_idx2_ccnew" UNIQUE, btree (a) INVALID
 
+\d idxpart3
+        Partitioned table "public.idxpart3"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart FOR VALUES FROM (30) TO (40)
+Partition key: RANGE (a)
+Indexes:
+    "idxpart3_a_idx" btree (a)
+    "idxpart3_a_idx1" UNIQUE, btree (a) INVALID
+Number of partitions: 1 (Use \d+ to list them.)
+
+\d idxpart31
+             Table "public.idxpart31"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+ c      | text    |           |          | 
+Partition of: idxpart3 DEFAULT
+Indexes:
+    "idxpart31_a_idx" btree (a)
+    "idxpart31_a_idx1" UNIQUE, btree (a) INVALID
+
 drop table idxpart;
 -- Verify bugfix with query on indexed partitioned table with no partitions
 -- https://postgr.es/m/[email-masked]
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 3d4b6e9bc95..06673c15199 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -36,6 +36,9 @@ create table idxpart11 partition of idxpart1 for values from (0) to (10) partiti
 create table idxpart111 partition of idxpart11 default partition by range(a);
 create table idxpart1111 partition of idxpart111 default partition by range(a);
 create table idxpart2 partition of idxpart for values from (10) to (20);
+create table idxpart3 partition of idxpart for values from (30) to (40) partition by range(a);
+create table idxpart31 partition of idxpart3 default;
+
 insert into idxpart2 values(10),(10); -- not unique
 create index concurrently on idxpart (a); -- partitioned
 create index concurrently on idxpart1 (a); -- partitioned and partition
@@ -44,7 +47,12 @@ create index concurrently on idxpart2 (a); -- leaf
 create unique index concurrently on idxpart (a); -- partitioned, unique failure
 \d idxpart
 \d idxpart1
+\d idxpart11
+\d idxpart111
+\d idxpart1111
 \d idxpart2
+\d idxpart3
+\d idxpart31
 drop table idxpart;

---

From: Greg Stark <[email-masked]>
Date: Fri, 25 Mar 2022 01:05:49 -0400

This patch is marked "waiting on author" in the CF. However the most
recent emails have patches and it's not clear to me what's left from
previous reviews that might not be addressed yet. Should this patch be
marked "Needs Review"?

Anastasia and Alexander are marked as reviewers. Are you still able to
review it or are there still pending issues that need to be resolved
from previous reviews?

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 25 Mar 2022 08:13:55 -0500

> review it or are there still pending issues that need to be resolved

I still haven't responded to Alexander's feedback, so I need to do that.
(Sorry).

However, since the patch attracted no attention for 50 some weeks last year, so
now is a weird time to shift attention to it.  As such, I will move it to the
next CF.

https://www.postgresql.org/message-id/flat/[email-masked]#da169a0a518bf8121604437d9ab053b3

-- 
Justin

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 28 Jun 2022 13:33:10 -0500

Hi,

On Thu, Feb 10, 2022 at 06:07:08PM +0300, Alexander Pyhalov wrote:
> I've rebased patches and tried to fix issues I've seen. I've fixed reference
> after table_close() in the first patch (can be seen while building with
> CPPFLAGS='-DRELCACHE_FORCE_RELEASE').

Thanks for finding that.

The patches other than 0001 are more experimental, and need someone to check if
it's even a good approach to use, so I kept them separate from the essential
patch.

Your latest 0005 patch (mark intermediate partitioned indexes as valid) is
probably fixing a bug in my SKIPVALID patch, right ?  I'm not sure whether the
SKIPVALID patch should be merged into 0001, and I've been awaiting feedback on
the main patch before handling progress reporting.

Sorry for not responding sooner.  The patch saw no activity for ~11 months so I
wasn't prepared to pick it up in March, at least not without guidance from a
committer.

Would you want to take over this patch ?  I wrote it following someone's
question, but don't expect that I'd use the feature myself.  I can help review
it or try to clarify the organization of my existing patches (but still haven't
managed to work my way through your amendments to my patches).

Thanks for caring about partitioned DDL ;)

-- 
Justin

---

From: Alexander Pyhalov <[email-masked]>
Date: Wed, 29 Jun 2022 09:45:49 +0300

Justin Pryzby писал 2022-06-28 21:33:
>> reference

Hi. I think it's more about fixing ReindexPartitions-to-set-indisvalid 
patch, as
we also should mark intermediate indexes as valid when reindex succeeds.

> managed to work my way through your amendments to my patches).

Yes, I'm glad to work on the patches, as this for us this is a very 
important feature.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Alexander Pyhalov <[email-masked]>
Date: Wed, 29 Jun 2022 11:01:37 +0300

>> CPPFLAGS='-DRELCACHE_FORCE_RELEASE').

Rebased patches on the current master.
They still require proper review.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

From 5c11849ceb2a1feb0e44dbdf30cc27de0282a659 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Mon, 28 Feb 2022 10:50:58 +0300
Subject: [PATCH 5/5] Mark intermediate partitioned indexes as valid

---

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index d09f0390413..d3ced6265b6 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3139,6 +3139,7 @@ static void
 ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 {
 	List	   *partitions = NIL;
+	List	   *inhpartindexes = NIL;
 	char		relkind = get_rel_relkind(relid);
 	char	   *relname = get_rel_name(relid);
 	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
@@ -3193,6 +3194,17 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 		char		partkind = get_rel_relkind(partoid);
 		MemoryContext old_context;
 
@@ -3237,9 +3249,28 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)

 	/*
 
 \d idxpart1
 
+\d idxpart11
 
+\d idxpart3
 
2.34.1

From 71838e0146e5150013c48818710d899e69786dc0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:39:59 +0300
Subject: [PATCH 4/5] ReindexPartitions() to set indisvalid

index b231a05b8c9..d09f0390413 100644
@@ -1716,8 +1716,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
@@ -1729,9 +1727,6 @@ reindex_invalid_child_indexes(Oid indexRelationId)
 
 /*
@@ -3229,6 +3224,24 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 	 */
 	ReindexMultipleInternal(partitions, params, relid, npart);
 
+	/*
2.34.1

From f5b8afe1aafb78c53a527d379b978ce9f1fe06d6 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Tue, 8 Feb 2022 21:15:05 +0300
Subject: [PATCH 3/5] Try to fix create index progress report

---

index 149235780b9..b231a05b8c9 100644
@@ -102,11 +102,14 @@ static void reindex_error_callback(void *args);
 
 /*
  * callback argument type for RangeVarCallbackForReindexIndex()
@@ -1220,6 +1223,7 @@ DefineIndex(Oid relationId,
 			Oid		   *opfamOids;
 			char		*relname;
 
+
 
@@ -1705,7 +1709,7 @@ static void
 reindex_invalid_child_indexes(Oid indexRelationId)
 {
 	ReindexParams params = {
-		.options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID
+		.options = REINDEXOPT_CONCURRENTLY | REINDEXOPT_SKIPVALID | REINDEXOPT_REPORT_CREATE_PART
 	};
 
 	/*
@@ -1718,6 +1722,8 @@ reindex_invalid_child_indexes(Oid indexRelationId)
 	CommandCounterIncrement();
 	index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
 
@@ -3105,7 +3111,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 
@@ -3141,6 +3147,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
@@ -3201,7 +3208,11 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 
@@ -3216,7 +3227,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 
 	/*
@@ -3234,7 +3245,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)
 
@@ -3328,6 +3339,9 @@ ReindexMultipleInternal(List *relids, ReindexParams *params)
 		}

 	StartTransactionCommand();
@@ -3723,7 +3737,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		if (indexRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
 			elog(ERROR, "cannot reindex a temporary table concurrently");

 		progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
@@ -3883,9 +3899,11 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 		/*
@@ -3947,10 +3965,12 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 		/*
@@ -4185,7 +4205,9 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)

 
 	return true;
 }
@@ -4321,6 +4343,29 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
 	}
 }
 
+/*
 
2.34.1

From 4f16e729a835dc8123a8ce593e6ae47542b82c83 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:31:40 +0300
Subject: [PATCH 2/5] Add SKIPVALID flag for more integration

Combined

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 784f6d9eb87..149235780b9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1697,59 +1697,33 @@ DefineIndex(Oid relationId,
 	return address;
 }

@@ -3224,6 +3198,11 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel)

 
2.34.1

From f4933625541b030fabcb15d1426f496598949898 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 7 Feb 2022 10:28:42 +0300
Subject: [PATCH 1/5] Allow CREATE INDEX CONCURRENTLY on partitioned table

Fixes:

index 9ffcdc629e6..2040b27b685 100644
@@ -642,7 +642,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
@@ -689,15 +692,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 99f5ab83c32..784f6d9eb87 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -68,6 +68,7 @@

@@ -695,17 +696,6 @@ DefineIndex(Oid relationId,
@@ -1145,6 +1135,11 @@ DefineIndex(Oid relationId,
 
 	if (stmt->deferrable)
@@ -1210,18 +1205,30 @@ DefineIndex(Oid relationId,

 
@@ -1265,9 +1272,9 @@ DefineIndex(Oid relationId,
 
 					AtEOXact_GUC(false, child_save_nestlevel);
 					SetUserIdAndSecContext(child_save_userid,
@@ -1277,9 +1284,12 @@ DefineIndex(Oid relationId,
 				}

 				foreach(cell, childidxs)
 				{
@@ -1353,10 +1363,14 @@ DefineIndex(Oid relationId,
 				 */
 				if (!found)
 				{
-					IndexStmt  *childStmt = copyObject(stmt);
+					IndexStmt  *childStmt;
 					bool		found_whole_row;
 					ListCell   *lc;
 
+					oldcontext = MemoryContextSwitchTo(ind_context);
+					childStmt = copyObject(stmt);
+					MemoryContextSwitchTo(oldcontext);
+
 					/*
 					 * We can't use the same index name for the child index,
 					 * so clear idxname to let the recursive invocation choose
@@ -1417,12 +1431,21 @@ DefineIndex(Oid relationId,
 								skip_build, quiet);
 					SetUserIdAndSecContext(child_save_userid,
 										   child_save_sec_context);
+					if (concurrent)
+					{
+						PopActiveSnapshot();
+						PushActiveSnapshot(GetTransactionSnapshot());
+						invalidate_parent = true;
+					}
 				}

 			/*
@@ -1430,24 +1453,9 @@ DefineIndex(Oid relationId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
@@ -1455,21 +1463,28 @@ DefineIndex(Oid relationId,
 		 */
 		AtEOXact_GUC(false, root_save_nestlevel);
 		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
 		if (!OidIsValid(parentIndexId))
+		{
+			if (concurrent)
+				reindex_invalid_child_indexes(indexRelationId);
+
 			pgstat_progress_end_command();
+		}
+
 		return address;
 	}
 
 	AtEOXact_GUC(false, root_save_nestlevel);
 	SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
 
-	if (!concurrent)

@@ -1682,6 +1697,62 @@ DefineIndex(Oid relationId,
 	return address;
 }

 /*
  * CheckMutability
 
 drop table idxpart;

2.34.1

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 20 Nov 2022 21:00:11 -0600

I finally found time to digest and integrate your changes into my local
branch.  This fixes the three issues you reported: FORCE_RELEASE, issue
with INVALID partitions issue (for which I adapted your patch into an
earlier patch in my series), and progress reporting.  And rebased.

-- 
Justin

---

From: Alexander Pyhalov <[email-masked]>
Date: Mon, 21 Nov 2022 17:44:22 +0300

Hi.

Thank you for the effort.
I've looked through and tested new patch a bit. Overall it looks good to 
me.
The question I have is whether we should update 
pg_stat_progress_create_index in reindex_invalid_child_indexes(), when 
we skip valid indexes?
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Ilya Gladyshev <[email-masked]>
Date: Sat, 03 Dec 2022 19:13:30 +0400

Hi,

Thank you Justin and Alexander for working on this, I have reviewed and
tested the latest patch, it works well, the problems mentioned
previously are all fixed. I like the idea of sharing code of reindex
and index, but I have noticed some peculiarities as a user. 

The reporting is somewhat confusing as it switches to reporting for
reindex concurrently while building child indexes, this should be fixed
with the simple patch I have attached. Another thing that I have
noticed is that REINDEX, which is used under the hood, creates new
indexes with suffix _ccnew, and if the index building fails, the
indexes that could not be build will have the name with _ccnew suffix.
This can actually be seen in your test:

ERROR:  could not create unique index "idxpart2_a_idx2_ccnew"

I find it quite confusing and I don't think that this the expected
behavior (if it is, I think it should be documented, like it is for
REINDEX). As an example of problems that it might entail, DROP INDEX
will not drop all the invalid indexes in the inheritance tree, because
it will leave _ccnew indexes in place, which is ok for reindex
concurrently, but that's not how C-I-C works now. I think that fixing
this problem requires some heavy code rewrite and I'm not quite sure
how to go about it, if you have any ideas, I will be happy to try them
out.

Thanks,
Ilya
From 8eb9fd7ce7d34c5c323c47b60a7f883f360ef090 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <[email-masked]>
Date: Sat, 3 Dec 2022 18:20:03 +0400
Subject: [PATCH] turn off reindex reporting for create

---
 src/backend/commands/indexcmds.c | 36 +++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index a2775931e2..b3c713037f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3804,14 +3804,16 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 
 		/* Don't overwrite CREATE INDEX command */
 		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+		{
 			pgstat_progress_start_command(PROGRESS_COMMAND_CREATE_INDEX,
 									  idx->tableId);
 
-		progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
-		progress_vals[1] = 0;	/* initializing */
-		progress_vals[2] = idx->indexId;
-		progress_vals[3] = idx->amId;
-		pgstat_progress_update_multi_param(4, progress_index, progress_vals);
+			progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
+			progress_vals[1] = 0;	/* initializing */
+			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(idx->indexId),
@@ -3967,13 +3969,15 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		 * table involved.  Don't overwrite CREATE INDEX command.
 		 */
 		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+		{
 			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] = newidx->indexId;
-		progress_vals[3] = newidx->amId;
-		pgstat_progress_update_multi_param(4, progress_index, progress_vals);
+			progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
+			progress_vals[1] = PROGRESS_CREATEIDX_PHASE_BUILD;
+			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(newidx->tableId, newidx->indexId);
@@ -4033,14 +4037,16 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
 		 * table involved. Don't overwrite CREATE INDEX command.
 		 */
 		if (!(params->options & REINDEXOPT_REPORT_CREATE_PART))
+		{
 			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] = newidx->indexId;
-		progress_vals[3] = newidx->amId;
-		pgstat_progress_update_multi_param(4, progress_index, progress_vals);
+			progress_vals[0] = PROGRESS_CREATEIDX_COMMAND_REINDEX_CONCURRENTLY;
+			progress_vals[1] = PROGRESS_CREATEIDX_PHASE_VALIDATE_IDXSCAN;
+			progress_vals[2] = newidx->indexId;
+			progress_vals[3] = newidx->amId;
+			pgstat_progress_update_multi_param(4, progress_index, progress_vals);
+		}
 
 		validate_index(newidx->tableId, newidx->indexId, snapshot);
 
-- 
2.30.2

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 4 Dec 2022 13:09:35 -0600

> this problem requires some heavy code rewrite and I'm not quite sure

This beavior is fixed.  I re-factored and re-implented to use
DefineIndex() for building indexes concurrently rather than reindexing.
That makes the patch smaller, actually, and has the added benefit of
splitting off the "Concurrently" part of DefineIndex() into a separate
function.

This currently handles partitions with a loop around the whole CIC
implementation, which means that things like WaitForLockers() happen
once for each index, the same as REINDEX CONCURRENTLY on a partitioned
table.  Contrast that with ReindexRelationConcurrently(), which handles
all the indexes on a table in one pass by looping around indexes within
each phase.

BTW, it causes the patch to fail to apply in cfbot when you send an
additional (002) supplementary patch without including the original
(001) patch.  You can name it *.txt to avoid the issue.
https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F

Thanks for looking.

-- 
Justin

---

From: Ilya Gladyshev <[email-masked]>
Date: Fri, 09 Dec 2022 17:53:10 +0400

> splitting off the "Concurrently" part of DefineIndex() into a

Nice, I think it turned out pretty concise. I played around with the
patch quite a bit, didn't find any major problems, the only minor thing
that I can note is that we should skip the top parent index itself in
the loop not to increment the pg_stat counter, something like this:

diff --git a/src/backend/commands/indexcmds.c
b/src/backend/commands/indexcmds.c
index cfab45b999..9049540b5b 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1515,6 +1515,9 @@ DefineIndex(Oid relationId,
                        Oid                     indrelid =
lfirst_oid(lc);
                        Oid                     tabrelid =
IndexGetRelation(indrelid, false);
 
+                       if (indrelid == indexRelationId)
+                               continue;
+
                        if
(RELKIND_HAS_STORAGE(get_rel_relkind(indrelid)) &&
                                !get_index_isvalid(indrelid))
                        {
> https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F
My bad, didn't know about this, thanks for the link.

On a side note, I noticed that reindex behaviour is strange on
partitioned tables, it doesn't mark partitioned tables as valid after
reindexing children, as I could understand from the code and mailing
lists, this is the intended behaviour, but I can't quite understand the
rationale for it, do you know why it is done this way?

---

From: Justin Pryzby <[email-masked]>
Date: Sun, 26 Mar 2023 09:51:25 -0500

> all the indexes on a table in one pass by looping around indexes within

Rebased over the progress reporting fix (27f5c712b).

I added a list of (intermediate) partitioned tables, rather than looping
over the list of inheritors again, to save calling rel_get_relkind().

I think this patch is done.

-- 
Justin
>From 941f7f930fc18563e2da42143015b6573d5447b1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH] Allow CREATE INDEX CONCURRENTLY on partitioned table

https://www.postgresql.org/message-id/flat/[email-masked]
---
 doc/src/sgml/ddl.sgml                  |   4 +-
 doc/src/sgml/ref/create_index.sgml     |  14 +-
 src/backend/commands/indexcmds.c       | 200 ++++++++++++++++++-------
 src/test/regress/expected/indexing.out | 127 +++++++++++++++-
 src/test/regress/sql/indexing.sql      |  26 +++-
 5 files changed, 297 insertions(+), 74 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 91c036d1cbe..64efdf1e879 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4178,9 +4178,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
      so that they are applied automatically to the entire hierarchy.
      This is very
      convenient, as not only will the existing partitions become indexed, but
-     also any partitions that are created in the future will.  One limitation is
-     that it's not possible to use the <literal>CONCURRENTLY</literal>
-     qualifier when creating such a partitioned index.  To avoid long lock
+     also any partitions that are created in the future will.  To avoid long lock
      times, it is possible to use <command>CREATE INDEX ON ONLY</command>
      the partitioned table; such an index is marked invalid, and the partitions
      do not get the index applied automatically.  The indexes on partitions can
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 40986aa502f..b05102efdaf 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -645,7 +645,10 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
    <para>
     If a problem arises while scanning the table, such as a deadlock or a
     uniqueness violation in a unique index, the <command>CREATE INDEX</command>
-    command will fail but leave behind an <quote>invalid</quote> index. This index
+    command will fail but leave behind an <quote>invalid</quote> index.
+    If this happens while build an index concurrently on a partitioned
+    table, the command can also leave behind <quote>valid</quote> or
+    <quote>invalid</quote> indexes on table partitions.  The invalid index
     will be ignored for querying purposes because it might be incomplete;
     however it will still consume update overhead. The <application>psql</application>
     <command>\d</command> command will report such an index as <literal>INVALID</literal>:
@@ -692,15 +695,6 @@ Indexes:
     cannot.
    </para>
 
-   <para>
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3ec8b5cca6c..daba8b67dbe 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -93,6 +93,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
 							 bool primary, bool isconstraint);
 static char *ChooseIndexNameAddition(List *colnames);
 static List *ChooseIndexColumnNames(List *indexElems);
+static void DefineIndexConcurrentInternal(Oid relationId,
+										  Oid indexRelationId,
+										  IndexInfo *indexInfo,
+										  LOCKTAG heaplocktag,
+										  LockRelId heaprelid);
 static void ReindexIndex(RangeVar *indexRelation, ReindexParams *params,
 						 bool isTopLevel);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
@@ -559,7 +564,6 @@ DefineIndex(Oid relationId,
 	bool		amissummarizing;
 	amoptions_function amoptions;
 	bool		partitioned;
-	bool		safe_index;
 	Datum		reloptions;
 	int16	   *coloptions;
 	IndexInfo  *indexInfo;
@@ -567,12 +571,10 @@ DefineIndex(Oid relationId,
 	bits16		constr_flags;
 	int			numberOfAttributes;
 	int			numberOfKeyAttributes;
-	TransactionId limitXmin;
 	ObjectAddress address;
 	LockRelId	heaprelid;
 	LOCKTAG		heaplocktag;
 	LOCKMODE	lockmode;
-	Snapshot	snapshot;
 	Oid			root_save_userid;
 	int			root_save_sec_context;
 	int			root_save_nestlevel;
@@ -705,17 +707,6 @@ DefineIndex(Oid relationId,
 	partitioned = rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
 	if (partitioned)
 	{
-		/*
-		 * Note: we check 'stmt->concurrent' rather than 'concurrent', so that
-		 * the error is thrown also for temporary tables.  Seems better to be
-		 * consistent, even though we could do it on temporary table because
-		 * we're not actually doing it concurrently.
-		 */
-		if (stmt->concurrent)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot create index on partitioned table \"%s\" concurrently",
-							RelationGetRelationName(rel))));
 		if (stmt->excludeOpNames)
 			ereport(ERROR,
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1089,10 +1080,6 @@ DefineIndex(Oid relationId,
 		}
 	}
 
-	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
-	safe_index = indexInfo->ii_Expressions == NIL &&
-		indexInfo->ii_Predicate == NIL;
-
 	/*
 	 * Report index creation if appropriate (delay this till after most of the
 	 * error checks)
@@ -1157,6 +1144,11 @@ DefineIndex(Oid relationId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initially build index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
@@ -1494,58 +1486,54 @@ DefineIndex(Oid relationId,
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
-		 * done here.
+		 * done here in the non-concurrent case.
 		 */
-		AtEOXact_GUC(false, root_save_nestlevel);
-		SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
-		table_close(rel, NoLock);
-		if (!OidIsValid(parentIndexId))
-			pgstat_progress_end_command();
-		else
+		if (!concurrent)
 		{
-			/* Update progress for an intermediate partitioned index itself */
-			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
-		}
+			AtEOXact_GUC(false, root_save_nestlevel);
+			SetUserIdAndSecContext(root_save_userid, root_save_sec_context);
+			table_close(rel, NoLock);
 
-		return address;
+			if (!OidIsValid(parentIndexId))
+				pgstat_progress_end_command();
+			else
+			{
+				/*
+				 * Update progress for an intermediate partitioned index
+				 * itself
+				 */
+				pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+			}
+
+			return address;
+		}
 	}

-	if (!concurrent)
+	/*
+	 * All done in the non-concurrent case, and when building catalog entries
+	 * of partitions for CIC.
+	 */
+	if (!concurrent || OidIsValid(parentIndexId))
 	{
-		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
 		/*
 		 * If this is the top-level index, the command is done overall;
-		 * otherwise, increment progress to report one child index is done.
+		 * otherwise (when being called recursively), increment progress to
+		 * report that one child index is done.  Except in the concurrent
+		 * (catalog-only) case, which is handled later.
 		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
-		else
+		else if (!concurrent)
 			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
@@ -1556,6 +1544,114 @@ DefineIndex(Oid relationId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
+	if (!partitioned)
+	{
+		/* CREATE INDEX CONCURRENTLY on a nonpartitioned table */
+		DefineIndexConcurrentInternal(relationId, indexRelationId,
+									  indexInfo, heaplocktag, heaprelid);
+		pgstat_progress_end_command();
+		return address;
+	}
+	else
+	{
+		/*
+		 * For CIC on a partitioned table, finish by building indexes on
+		 * partitions
+		 */
+
+		ListCell   *lc;
+		List	   *childs;
+		List	   *partitioned = NIL;
+		MemoryContext cic_context,
+					old_context;
+
+		/* Create special memory context for cross-transaction storage */
+		cic_context = AllocSetContextCreate(PortalContext,
+											"Create index concurrently",
+											ALLOCSET_DEFAULT_SIZES);
+
+		old_context = MemoryContextSwitchTo(cic_context);
+		childs = find_all_inheritors(indexRelationId, ShareLock, NULL);
+		MemoryContextSwitchTo(old_context);
+
+		foreach(lc, childs)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+			Oid			tabrelid;
+			char		relkind;
+
+			/*
+			 * Pre-existing partitions which were ATTACHED were already
+			 * counted in the progress report.
+			 */
+			if (get_index_isvalid(indrelid))
+				continue;
+
+			/*
+			 * Partitioned indexes are counted in the progress report, but
+			 * don't need to be further processed.
+			 */
+			relkind = get_rel_relkind(indrelid);
+			if (!RELKIND_HAS_STORAGE(relkind))
+			{
+				/* The toplevel index doesn't count towards "partitions done" */
+				if (indrelid != indexRelationId)
+					pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
+				/*
+				 * Build up a list of all the intermediate partitioned tables
+				 * which will later need to be set valid.
+				 */
+				old_context = MemoryContextSwitchTo(cic_context);
+				partitioned = lappend_oid(partitioned, indrelid);
+				MemoryContextSwitchTo(old_context);
+				continue;
+			}
+
+			rel = table_open(relationId, ShareUpdateExclusiveLock);
+			heaprelid = rel->rd_lockInfo.lockRelId;
+			table_close(rel, ShareUpdateExclusiveLock);
+			SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
+
+			/* Process each partition in a separate transaction */
+			tabrelid = IndexGetRelation(indrelid, false);
+			DefineIndexConcurrentInternal(tabrelid, indrelid, indexInfo,
+										  heaplocktag, heaprelid);
+
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
+		/* Set as valid all partitioned indexes, including the parent */
+		foreach(lc, partitioned)
+		{
+			Oid			indrelid = lfirst_oid(lc);
+
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indrelid, INDEX_CREATE_SET_VALID);
+		}
+
+		MemoryContextDelete(cic_context);
+		pgstat_progress_end_command();
+		PopActiveSnapshot();
+		return address;
+	}
+}
+
+
+static void
+DefineIndexConcurrentInternal(Oid relationId,
+							  Oid indexRelationId, IndexInfo *indexInfo,
+							  LOCKTAG heaplocktag, LockRelId heaprelid)
+{
+	TransactionId limitXmin;
+	Snapshot	snapshot;
+
+	/* Is index safe for others to ignore?  See set_indexsafe_procflags() */
+	bool		safe_index = indexInfo->ii_Expressions == NIL &&
+		indexInfo->ii_Predicate == NIL;
+
 	/*
 	 * For a concurrent build, it's important to make the catalog entries
 	 * visible to other transactions before we start to build the index. That
@@ -1751,10 +1847,6 @@ DefineIndex(Oid relationId,
 	 * Last thing to do is release the session-level lock on the parent table.
 	 */
 	UnlockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock);
-
-	pgstat_progress_end_command();
-
-	return address;
 }

diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 1bdd430f063..f1beee6d240 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -50,11 +50,130 @@ select relname, relkind, relhassubclass, inhparent::regclass
 (8 rows)
 
 drop table idxpart;
+ERROR:  could not create unique index "idxpart2_a_idx1"
+    "idxpart1_a_idx1" UNIQUE, btree (a) INVALID
+    "idxpart11_a_idx1" UNIQUE, btree (a) INVALID
+    "idxpart111_a_idx1" UNIQUE, btree (a) INVALID
+    "idxpart1111_a_idx1" UNIQUE, btree (a) INVALID
+    "idxpart2_a_idx1" UNIQUE, btree (a) INVALID
index 429120e7104..fb0baedcc28 100644
@@ -29,10 +29,30 @@ select relname, relkind, relhassubclass, inhparent::regclass

---

From: Alexander Pyhalov <[email-masked]>
Date: Mon, 27 Mar 2023 13:28:24 +0300

Hi.

Overall looks good to me. However, I think that using 'partitioned' as 
list of partitioned index oids in DefineIndex() is a bit misleading - 
we've just used it as boolean, specifying if we are dealing with a 
partitioned relation.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Justin Pryzby <[email-masked]>
Date: Wed, 12 Jul 2023 21:27:56 -0500

On Mon, Mar 27, 2023 at 01:28:24PM +0300, Alexander Pyhalov wrote:
> Overall looks good to me. However, I think that using 'partitioned' as list
> of partitioned index oids in DefineIndex() is a bit misleading - we've just
> used it as boolean, specifying if we are dealing with a partitioned
> relation.

Right.  This is also rebased on 8c852ba9a4 (Allow some exclusion
constraints on partitions).

-- 
Justin
>From 3f60cbdd12b67115f86854ff60a4009028b8b99f Mon Sep 17 00:00:00 2001

https://www.postgresql.org/message-id/flat/[email-masked]
---
 doc/src/sgml/ddl.sgml                  |   4 +-
 doc/src/sgml/ref/create_index.sgml     |  14 +-
 src/backend/commands/indexcmds.c       | 201 ++++++++++++++++++-------
 src/test/regress/expected/indexing.out | 127 +++++++++++++++-
 src/test/regress/sql/indexing.sql      |  26 +++-
 5 files changed, 297 insertions(+), 75 deletions(-)

index 58aaa691c6a..afa982154a8 100644
@@ -4161,9 +4161,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02
 
-   <para>
 
index baf3e6e57a5..dfe64052b81 100644
@@ -92,6 +92,11 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId,
@@ -555,7 +560,6 @@ DefineIndex(Oid relationId,
@@ -563,12 +567,10 @@ DefineIndex(Oid relationId,
@@ -699,20 +701,6 @@ DefineIndex(Oid relationId,
 	 * partition.
-	if (partitioned)
 
 	/*
 	 * Don't try to CREATE INDEX on temp tables of other backends.
@@ -1102,10 +1090,6 @@ DefineIndex(Oid relationId,
 		}
 	}
 
@@ -1170,6 +1154,11 @@ DefineIndex(Oid relationId,
 
 	if (stmt->deferrable)
@@ -1519,21 +1508,7 @@ DefineIndex(Oid relationId,
 
 				/*
 				 * CCI here to make this update visible, in case this recurses
@@ -1545,37 +1520,49 @@ DefineIndex(Oid relationId,
 
 		/*
 
-		return address;

-	if (!concurrent)
+	/*
 
 		/*
 
 		return address;
@@ -1586,6 +1573,114 @@ DefineIndex(Oid relationId,
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
 	table_close(rel, NoLock);
 
+	if (!partitioned)
+	{
+		List	   *tosetvalid = NIL;
+				tosetvalid = lappend_oid(tosetvalid, indrelid);
+		foreach(lc, tosetvalid)
@@ -1781,10 +1876,6 @@ DefineIndex(Oid relationId,

index 2be8ffa7ec4..aefa203b14f 100644
 
 drop table idxpart;
index b69c41832ca..5ddeaf1c613 100644