pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  

help / color / mirror / Atom feed

Re: 回复：how to create index concurrently on partitioned table
8+ messages / 3 participants
[nested] [flat]

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-01 05:51  Michael Paquier <michael@paquier.xyz>
  1 sibling, 1 reply; 8+ messages in thread

From: Michael Paquier @ 2020-09-01 05:51 UTC (permalink / raw)
  To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On Sun, Aug 09, 2020 at 06:44:23PM -0500, Justin Pryzby wrote:
> Thanks for looking.

The REINDEX patch is progressing its way, so I have looked a bit at
the part for CIC.

Visibly, the case of multiple partition layers is not handled
correctly.  Here is a sequence that gets broken:
CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id);
CREATE TABLE child_0_10 PARTITION OF parent_tab
     FOR VALUES FROM (0) TO (10);
CREATE TABLE child_10_20 PARTITION OF parent_tab
     FOR VALUES FROM (10) TO (20);
CREATE TABLE child_20_30 PARTITION OF parent_tab
     FOR VALUES FROM (20) TO (30);
INSERT INTO parent_tab VALUES (generate_series(0,29));
CREATE TABLE child_30_40 PARTITION OF parent_tab
     FOR VALUES FROM (30) TO (40)
     PARTITION BY RANGE(id);
CREATE TABLE child_30_35 PARTITION OF child_30_40
     FOR VALUES FROM (30) TO (35);
CREATE TABLE child_35_40 PARTITION OF child_30_40
     FOR VALUES FROM (35) TO (40);
INSERT INTO parent_tab VALUES (generate_series(30,39));
CREATE INDEX CONCURRENTLY parent_index ON parent_tab (id);

This fails as follows:
ERROR: XX000: unrecognized node type: 2139062143
LOCATION:  copyObjectImpl, copyfuncs.c:5718

And the problem visibly comes from some handling with the second level
of partitions, child_30_40 in my example above.  Even with that, this
outlines a rather severe problem in the patch: index_set_state_flags()
flips indisvalid on/off using a non-transactional update (see the use
of heap_inplace_update), meaning that if we fail in the middle of the
operation we may finish with a partition index tree where some of the
indexes are valid and some of them are invalid.  In order to make this
logic consistent, I am afraid that we will need to do two things:
- Change index_set_state_flags() so as it uses a transactional
update.  That's something I would like to change for other reasons,
like making sure that the REPLICA IDENTITY of a parent relation is
correctly reset when dropping a replica index.
- Make all the indexes of the partition tree valid in the *same*
sub-transaction.
You can note that this case is different than a concurrent REINDEX,
because in this case we just do an in-place change between the old and
new index, meaning that even if there is a failure happening while
processing, we may have some invalid indexes, but there are still
valid indexes attached to the partition tree, at any time.

+     MemoryContext   oldcontext = MemoryContextSwitchTo(ind_context);
      PartitionDesc partdesc = RelationGetPartitionDesc(rel);
      int         nparts = partdesc->nparts;
+     char        *relname = pstrdup(RelationGetRelationName(rel));
Er, no.  We should not play with the relation cache calls in a private
memory context.  I think that this needs much more thoughts.
--
Michael

Attachments:

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-02 01:39  Michael Paquier <michael@paquier.xyz>
  1 sibling, 1 reply; 8+ messages in thread

From: Michael Paquier @ 2020-09-02 01:39 UTC (permalink / raw)
  To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On Fri, Aug 14, 2020 at 09:29:45AM +0900, Michael Paquier wrote:
> Once this gets done, we should then be able to get rid of the extra
> session locking taken when building the list of partitions, limiting
> session locks to only be taken during the concurrent reindex of a
> single partition (the table itself for a partition table, and the
> parent table for a partition index), making the whole operation less
> invasive.

The problem with dropped relations in REINDEX has been addressed by
1d65416, so I have gone through this patch again and simplified the
use of session locks, these being taken only when doing a REINDEX
CONCURRENTLY for a given partition.  This part is in a rather
committable shape IMO, so I would like to get it done first, before
looking more at the other cases with CIC and CLUSTER.  I am still
planning to go through it once again.
--
Michael

Attachments:

reindex-part-v8.patch (text/x-diff, 42.4K) download
From 618760af9ce3229f775e1fdfb635837865aadb66 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Wed, 2 Sep 2020 10:37:46 +0900
Subject: [PATCH] Add support for partitioned tables and indexes with REINDEX

Until now, REINDEX was not able to work with partitioned tables and
indexes, forcing users to reindex partitions one by one.  This extends
REINDEX INDEX and REINDEX TABLE so as they can accept a partitioned
index and table in input, respectively, to reindex all the partitions
assigned to them.

This shares some logic with schema and database REINDEX as each
partition gets processed in its own transaction (this choice minimizes
the number of invalid indexes to one partition with REINDEX CONCURRENTLY
in case of a cancellation or failure in-flight, and the duration of each
transaction).  Isolation tests are added to emulate cases I bumped
into, particularly with the concurrent drop of a leaf partition
reindexed.

Per their multi-transaction nature, these new flavors cannot run in a
transaction block.

Author: Justin Pryzby, Michael Paquier
Discussion: https://postgr.es/m/db12e897-73ff-467e-94cb-4af03705435f.adger.lj@alibaba-inc.com
---
 src/include/commands/defrem.h                 |   6 +-
 src/backend/catalog/index.c                   |  25 +-
 src/backend/commands/indexcmds.c              | 242 ++++++++++++++----
 src/backend/tcop/utility.c                    |   6 +-
 .../isolation/expected/reindex-partitions.out | 107 ++++++++
 src/test/isolation/isolation_schedule         |   1 +
 .../isolation/specs/reindex-partitions.spec   |  61 +++++
 src/test/regress/expected/create_index.out    | 158 +++++++++++-
 src/test/regress/sql/create_index.sql         |  88 ++++++-
 doc/src/sgml/ref/reindex.sgml                 |  13 +-
 .../postgres_fdw/expected/postgres_fdw.out    |  21 ++
 contrib/postgres_fdw/sql/postgres_fdw.sql     |  20 ++
 src/tools/pgindent/typedefs.list              |   1 +
 13 files changed, 657 insertions(+), 92 deletions(-)
 create mode 100644 src/test/isolation/expected/reindex-partitions.out
 create mode 100644 src/test/isolation/specs/reindex-partitions.spec

diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index c26a102b17..22db3f660d 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,8 +34,10 @@ extern ObjectAddress DefineIndex(Oid relationId,
 								 bool check_not_in_use,
 								 bool skip_build,
 								 bool quiet);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent);
-extern Oid	ReindexTable(RangeVar *relation, int options, bool concurrent);
+extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent,
+						 bool isTopLevel);
+extern Oid	ReindexTable(RangeVar *relation, int options, bool concurrent,
+						 bool isTopLevel);
 extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 								  int options, bool concurrent);
 extern char *makeObjectName(const char *name1, const char *name2,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1d662e9af4..1764739ff5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -77,6 +77,7 @@
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/pg_rusage.h"
+#include "utils/rel.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tuplesort.h"
@@ -3473,11 +3474,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 									 iRel->rd_rel->relam);
 
 	/*
-	 * The case of reindexing partitioned tables and indexes is handled
-	 * differently by upper layers, so this case shouldn't arise.
+	 * Partitioned indexes should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (iRel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-		elog(ERROR, "unsupported relation kind for index \"%s\"",
+		elog(ERROR, "cannot reindex partitioned index \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(iRel)),
 			 RelationGetRelationName(iRel));
 
 	/*
@@ -3694,20 +3696,13 @@ reindex_relation(Oid relid, int flags, int options)
 		return false;
 
 	/*
-	 * This may be useful when implemented someday; but that day is not today.
-	 * For now, avoid erroring out when called in a multi-table context
-	 * (REINDEX SCHEMA) and happen to come across a partitioned table.  The
-	 * partitions may be reindexed on their own anyway.
+	 * Partitioned tables should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-	{
-		ereport(WARNING,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-						RelationGetRelationName(rel))));
-		table_close(rel, ShareLock);
-		return false;
-	}
+		elog(ERROR, "cannot reindex partitioned table \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(rel)),
+			 RelationGetRelationName(rel));
 
 	toast_relid = rel->rd_rel->reltoastrelid;
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b3a92381f9..75008eebde 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -88,7 +88,12 @@ static List *ChooseIndexColumnNames(List *indexElems);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
 											Oid relId, Oid oldRelId, void *arg);
 static bool ReindexRelationConcurrently(Oid relationOid, int options);
-static void ReindexPartitionedIndex(Relation parentIdx);
+
+static void reindex_partitions(Oid relid, int options, bool concurrent,
+							   bool isTopLevel);
+static void reindex_multiple_internal(List *relids, int options,
+									  bool concurrent);
+static void reindex_error_callback(void *args);
 static void update_relispartition(Oid relationId, bool newval);
 static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
 
@@ -101,6 +106,16 @@ struct ReindexIndexCallbackState
 	Oid			locked_table_oid;	/* tracks previously locked table */
 };
 
+/*
+ * callback arguments for reindex_error_callback()
+ */
+typedef struct ReindexErrorInfo
+{
+	char	   *relname;
+	char	   *relnamespace;
+	char		relkind;
+} ReindexErrorInfo;
+
 /*
  * CheckIndexCompatible
  *		Determine whether an existing index definition is compatible with a
@@ -2420,11 +2435,10 @@ ChooseIndexColumnNames(List *indexElems)
  *		Recreate a specific index.
  */
 void
-ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
+ReindexIndex(RangeVar *indexRelation, int options, bool concurrent, bool isTopLevel)
 {
 	struct ReindexIndexCallbackState state;
 	Oid			indOid;
-	Relation	irel;
 	char		persistence;
 
 	/*
@@ -2445,22 +2459,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 									  RangeVarCallbackForReindexIndex,
 									  &state);
 
-	/*
-	 * Obtain the current persistence of the existing index.  We already hold
-	 * lock on the index.
-	 */
-	irel = index_open(indOid, NoLock);
-
-	if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-	{
-		ReindexPartitionedIndex(irel);
-		return;
-	}
-
-	persistence = irel->rd_rel->relpersistence;
-	index_close(irel, NoLock);
-
-	if (concurrent && persistence != RELPERSISTENCE_TEMP)
+	persistence = get_rel_persistence(indOid);
+	if (get_rel_relkind(indOid) == RELKIND_PARTITIONED_INDEX)
+		reindex_partitions(indOid, options, concurrent, isTopLevel);
+	else if (concurrent && persistence != RELPERSISTENCE_TEMP)
 		ReindexRelationConcurrently(indOid, options);
 	else
 		reindex_index(indOid, false, persistence,
@@ -2542,7 +2544,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
  *		Recreate all indexes of a table (and of its toast table, if any)
  */
 Oid
-ReindexTable(RangeVar *relation, int options, bool concurrent)
+ReindexTable(RangeVar *relation, int options, bool concurrent, bool isTopLevel)
 {
 	Oid			heapOid;
 	bool		result;
@@ -2560,7 +2562,9 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
 									   0,
 									   RangeVarCallbackOwnsTable, NULL);
 
-	if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+	if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
+		reindex_partitions(heapOid, options, concurrent, isTopLevel);
+	else if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
 	{
 		result = ReindexRelationConcurrently(heapOid, options);
 
@@ -2604,7 +2608,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	MemoryContext private_context;
 	MemoryContext old;
 	List	   *relids = NIL;
-	ListCell   *l;
 	int			num_keys;
 	bool		concurrent_warning = false;
 
@@ -2688,11 +2691,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 		 * Only regular tables and matviews can have indexes, so ignore any
 		 * other kind of relation.
 		 *
-		 * It is tempting to also consider partitioned tables here, but that
-		 * has the problem that if the children are in the same schema, they
-		 * would be processed twice.  Maybe we could have a separate list of
-		 * partitioned tables, and expand that afterwards into relids,
-		 * ignoring any duplicates.
+		 * Partitioned tables/indexes are skipped but matching leaf partitions
+		 * are processed.
 		 */
 		if (classtuple->relkind != RELKIND_RELATION &&
 			classtuple->relkind != RELKIND_MATVIEW)
@@ -2755,14 +2755,156 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	table_endscan(scan);
 	table_close(relationRelation, AccessShareLock);
 
-	/* Now reindex each rel in a separate transaction */
+	/*
+	 * Process each relation listed in a separate transaction.  Note that this
+	 * commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(relids, options, concurrent);
+
+	MemoryContextDelete(private_context);
+}
+
+/*
+ * Error callback specific to reindex_partitions().
+ */
+static void
+reindex_error_callback(void *arg)
+{
+	ReindexErrorInfo *errinfo = (ReindexErrorInfo *) arg;
+
+	Assert(errinfo->relkind == RELKIND_PARTITIONED_INDEX ||
+		   errinfo->relkind == RELKIND_PARTITIONED_TABLE);
+
+	if (errinfo->relkind == RELKIND_PARTITIONED_TABLE)
+		errcontext("while reindexing partitioned table \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+	else if (errinfo->relkind == RELKIND_PARTITIONED_INDEX)
+		errcontext("while reindexing partitioned index \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+}
+
+/*
+ * reindex_partitions
+ *
+ * Reindex a set of partitions, per the partitioned index or table given
+ * by the caller.
+ */
+static void
+reindex_partitions(Oid relid, int options, bool concurrent,
+				   bool isTopLevel)
+{
+	List	   *partitions = NIL;
+	char		relkind = get_rel_relkind(relid);
+	char	   *relname = get_rel_name(relid);
+	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
+	MemoryContext reindex_context;
+	List	   *inhoids;
+	ListCell   *lc;
+	ErrorContextCallback errcallback;
+	ReindexErrorInfo errinfo;
+
+	Assert(relkind == RELKIND_PARTITIONED_INDEX ||
+		   relkind == RELKIND_PARTITIONED_TABLE);
+
+	/*
+	 * Check if this runs in a transaction block, with an error callback to
+	 * provide the context under which a problem happens.
+	 */
+	errinfo.relname = pstrdup(relname);
+	errinfo.relnamespace = pstrdup(relnamespace);
+	errinfo.relkind = relkind;
+	errcallback.callback = reindex_error_callback;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
+	PreventInTransactionBlock(isTopLevel,
+							  relkind == RELKIND_PARTITIONED_TABLE ?
+							  "REINDEX TABLE" : "REINDEX INDEX");
+
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
+	/*
+	 * Create special memory context for cross-transaction storage.
+	 *
+	 * Since it is a child of PortalContext, it will go away eventually even
+	 * if we suffer an error so there is no need for special abort cleanup
+	 * logic.
+	 */
+	reindex_context = AllocSetContextCreate(PortalContext, "Reindex",
+											ALLOCSET_DEFAULT_SIZES);
+
+	/* ShareLock is enough to prevent schema modifications */
+	inhoids = find_all_inheritors(relid, ShareLock, NULL);
+
+	/*
+	 * The list of relations to reindex are the physical partitions of the
+	 * tree so discard any partitioned table or index.
+	 */
+	foreach(lc, inhoids)
+	{
+		Oid			partoid = lfirst_oid(lc);
+		Oid			parentoid;
+		char		partkind = get_rel_relkind(partoid);
+		MemoryContext old_context;
+
+		/*
+		 * This discards partitioned tables, partitioned indexes and foreign
+		 * tables.
+		 */
+		if (!RELKIND_HAS_STORAGE(partkind))
+			continue;
+
+		Assert(partkind == RELKIND_INDEX ||
+			   partkind == RELKIND_RELATION);
+
+		parentoid = (partkind == RELKIND_INDEX) ?
+			IndexGetRelation(partoid, false) : partoid;
+
+		/* Save partition OID */
+		old_context = MemoryContextSwitchTo(reindex_context);
+		partitions = lappend_oid(partitions, partoid);
+		MemoryContextSwitchTo(old_context);
+	}
+
+	/*
+	 * Process each partition listed in a separate transaction.  Note that
+	 * this commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(partitions, options, concurrent);
+
+	/*
+	 * Clean up working storage --- note we must do this after
+	 * StartTransactionCommand, else we might be trying to delete the active
+	 * context!
+	 */
+	MemoryContextDelete(reindex_context);
+}
+
+/*
+ * reindex_multiple_internal
+ *
+ * Reindex a list of relations, each one being processed in its own
+ * transaction.  This commits the existing transaction immediately,
+ * and starts a new transaction when finished.
+ */
+static void
+reindex_multiple_internal(List *relids, int options, bool concurrent)
+{
+	ListCell   *l;
+
 	PopActiveSnapshot();
 	CommitTransactionCommand();
+
 	foreach(l, relids)
 	{
 		Oid			relid = lfirst_oid(l);
+		char		relkind;
+		char		relpersistence;
 
 		StartTransactionCommand();
+
 		/* functions in indexes may want a snapshot set */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -2774,6 +2916,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 			continue;
 		}
 
+		relkind = get_rel_relkind(relid);
+		relpersistence = get_rel_persistence(relid);
+
+		/*
+		 * Partitioned tables and indexes can never be processed directly, and
+		 * a list of their leaves should be built first.
+		 */
+		Assert(relkind != RELKIND_PARTITIONED_INDEX &&
+			   relkind != RELKIND_PARTITIONED_TABLE);
+
 		if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
 		{
 			(void) ReindexRelationConcurrently(relid,
@@ -2781,6 +2933,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 											   REINDEXOPT_MISSING_OK);
 			/* ReindexRelationConcurrently() does the verbose output */
 		}
+		else if (relkind == RELKIND_INDEX)
+		{
+			reindex_index(relid, false, relpersistence,
+						  options | REINDEXOPT_REPORT_PROGRESS);
+			PopActiveSnapshot();
+		}
 		else
 		{
 			bool		result;
@@ -2803,9 +2961,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 
 		CommitTransactionCommand();
 	}
-	StartTransactionCommand();
 
-	MemoryContextDelete(private_context);
+	StartTransactionCommand();
 }
 
 
@@ -2817,8 +2974,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
  * view.  For tables and materialized views, all its indexes will be rebuilt,
  * excluding invalid indexes and any indexes used in exclusion constraints,
  * but including its associated toast table indexes.  For indexes, the index
- * itself will be rebuilt.  If 'relationOid' belongs to a partitioned table
- * then we issue a warning to mention these are not yet supported.
+ * itself will be rebuilt.
  *
  * The locks taken on parent tables and involved indexes are kept until the
  * transaction is committed, at which point a session lock is taken on each
@@ -3057,13 +3213,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 				MemoryContextSwitchTo(oldcontext);
 				break;
 			}
+
 		case RELKIND_PARTITIONED_TABLE:
-			/* see reindex_relation() */
-			ereport(WARNING,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-							get_rel_name(relationOid))));
-			return false;
+		case RELKIND_PARTITIONED_INDEX:
 		default:
 			/* Return error if type of relation is not supported */
 			ereport(ERROR,
@@ -3530,20 +3682,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	return true;
 }
 
-/*
- *	ReindexPartitionedIndex
- *		Reindex each child of the given partitioned index.
- *
- * Not yet implemented.
- */
-static void
-ReindexPartitionedIndex(Relation parentIdx)
-{
-	ereport(ERROR,
-			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-			 errmsg("REINDEX is not yet implemented for partitioned indexes")));
-}
-
 /*
  * Insert or delete an appropriate pg_inherits tuple to make the given index
  * be a partition of the indicated parent index.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6154d2c8c6..35553e04ce 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -926,10 +926,12 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 				switch (stmt->kind)
 				{
 					case REINDEX_OBJECT_INDEX:
-						ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
+						ReindexIndex(stmt->relation, stmt->options,
+									 stmt->concurrent, isTopLevel);
 						break;
 					case REINDEX_OBJECT_TABLE:
-						ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
+						ReindexTable(stmt->relation, stmt->options,
+									 stmt->concurrent, isTopLevel);
 						break;
 					case REINDEX_OBJECT_SCHEMA:
 					case REINDEX_OBJECT_SYSTEM:
diff --git a/src/test/isolation/expected/reindex-partitions.out b/src/test/isolation/expected/reindex-partitions.out
new file mode 100644
index 0000000000..5373abde25
--- /dev/null
+++ b/src/test/isolation/expected/reindex-partitions.out
@@ -0,0 +1,107 @@
+Parsed test spec with 3 sessions
+
+starting permutation: begin1 lockexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 6acbb695ec..65d1443ac6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -50,6 +50,7 @@ test: lock-committed-keyupdate
 test: update-locked-tuple
 test: reindex-concurrently
 test: reindex-schema
+test: reindex-partitions
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-partitions.spec b/src/test/isolation/specs/reindex-partitions.spec
new file mode 100644
index 0000000000..738473648d
--- /dev/null
+++ b/src/test/isolation/specs/reindex-partitions.spec
@@ -0,0 +1,61 @@
+# REINDEX with partitioned tables
+#
+# Ensure that concurrent and non-concurrent operations work correctly when
+# a REINDEX is performed on a partitioned table or index.
+
+# Note that this does not test for partition creation in parallel of a
+# REINDEX.  The initial transaction of REINDEX building the partition list
+# would make the CREATE TABLE command to wait, and once the first
+# transaction commits it could be possible for the CREATE TABLE command
+# to finish before the concurrent REINDEX is done, making the output
+# unpredictable.
+
+setup
+{
+	CREATE TABLE reind_conc_parent (id int) PARTITION BY RANGE (id);
+	CREATE TABLE reind_conc_0_10 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (0) TO (10);
+	CREATE TABLE reind_conc_10_20 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (10) TO (20);
+	INSERT INTO reind_conc_parent VALUES (generate_series(0, 19));
+}
+
+teardown
+{
+	DROP TABLE reind_conc_parent;
+}
+
+session "s1"
+step "begin1"          { BEGIN; }
+step "lockexcl1"       { LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE; }
+step "lockshare1"      { LOCK reind_conc_parent IN SHARE MODE; }
+step "lockupdate1"     { LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE; }
+step "lockpartexcl1"   { LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE; }
+step "lockpartshare1"  { LOCK reind_conc_10_20 IN SHARE MODE; }
+step "lockpartupdate1" { LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE; }
+step "end1"            { COMMIT; }
+
+session "s2"
+step "reindex2"        { REINDEX TABLE reind_conc_parent; }
+step "reindex_conc2"   { REINDEX TABLE CONCURRENTLY reind_conc_parent; }
+
+session "s3"
+step "drop3"           { DROP TABLE reind_conc_10_20; }
+
+# An existing partition leaf is dropped after reindex is done when the
+# parent is locked.
+permutation "begin1" "lockexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex_conc2" "drop3" "end1"
+
+# An existing partition leaf is dropped after reindex is done when this
+# leaf is locked.
+permutation "begin1" "lockpartexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex_conc2" "drop3" "end1"
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index e3e6634d7e..3ccf3719d4 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2196,18 +2196,6 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes to reindex
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes that can be reindexed concurrently
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
              relid             |         parentrelid         | level 
@@ -2320,6 +2308,152 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index;
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10;
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10;
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ERROR:  REINDEX INDEX cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned index "public.concur_reindex_part_index"
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part;
+ERROR:  "concur_reindex_part" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part;
+ERROR:  "concur_reindex_part" is not an index
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10;
+ERROR:  "concur_reindex_part_10" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10;
+ERROR:  "concur_reindex_part_10" is not an index
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ERROR:  REINDEX TABLE cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned table "public.concur_reindex_part"
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 -- Check errors
 -- Cannot run inside a transaction block
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f3667bacdc..6d98b73365 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -903,12 +903,6 @@ CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2 (c1);
 ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_part_index_0_2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
 -- REINDEX should preserve dependencies of partition tree.
@@ -948,6 +942,88 @@ WHERE classid = 'pg_class'::regclass AND
   ORDER BY 1, 2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
+
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10;
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part;
+REINDEX INDEX CONCURRENTLY concur_reindex_part;
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10;
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 
 -- Check errors
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..33af4ae02a 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -88,7 +88,9 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
     <term><literal>INDEX</literal></term>
     <listitem>
      <para>
-      Recreate the specified index.
+      Recreate the specified index. This form of <command>REINDEX</command>
+      cannot be executed inside a transaction block when used with a
+      partitioned index.
      </para>
     </listitem>
    </varlistentry>
@@ -99,6 +101,8 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
      <para>
       Recreate all indexes of the specified table.  If the table has a
       secondary <quote>TOAST</quote> table, that is reindexed as well.
+      This form of <command>REINDEX</command> cannot be executed inside a
+      transaction block when used with a partitioned table.
      </para>
     </listitem>
    </varlistentry>
@@ -259,8 +263,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
   </para>
 
   <para>
-   Reindexing partitioned tables or partitioned indexes is not supported.
-   Each individual partition can be reindexed separately instead.
+   Reindexing partitioned indexes or partitioned tables is supported
+   with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
+   respectively. Each partition of the specified partitioned relation is
+   reindexed in a separate transaction. Those commands cannot be used inside
+   a transaction block when working on a partitioned table or index.
   </para>
 
   <refsect2 id="sql-reindex-concurrently" xreflabel="Rebuilding Indexes Concurrently">
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 90db550b92..84bc0ee381 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4044,6 +4044,27 @@ SELECT f_test(100);
 
 DROP FUNCTION f_test(int);
 -- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+-- ===================================================================
 -- conversion error
 -- ===================================================================
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 83971665e3..d452d06343 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1081,6 +1081,26 @@ $$ LANGUAGE plpgsql;
 SELECT f_test(100);
 DROP FUNCTION f_test(int);
 
+-- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+
 -- ===================================================================
 -- conversion error
 -- ===================================================================
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3d990463ce..4f7b03630a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2036,6 +2036,7 @@ RegProcedure
 Regis
 RegisNode
 RegisteredBgWorker
+ReindexErrorInfo
 ReindexObjectType
 ReindexStmt
 ReindexType
-- 
2.28.0

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-03 19:02  Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
  parent: Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 8+ messages in thread

From: Anastasia Lubennikova @ 2020-09-03 19:02 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; Justin Pryzby <pryzby@telsasoft.com>; +Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On 02.09.2020 04:39, Michael Paquier wrote:
>
> The problem with dropped relations in REINDEX has been addressed by
> 1d65416, so I have gone through this patch again and simplified the
> use of session locks, these being taken only when doing a REINDEX
> CONCURRENTLY for a given partition.  This part is in a rather
> committable shape IMO, so I would like to get it done first, before
> looking more at the other cases with CIC and CLUSTER.  I am still
> planning to go through it once again.
> --
> Michael

Thank you for advancing this work.

I was reviewing the previous version, but the review became outdated 
before I sent it. Overall design is fine, but I see a bunch of things 
that need to be fixed before commit.

First of all, this patch fails at cfbot:

indexcmds.c:2848:7: error: variable ‘parentoid’ set but not used 
[-Werror=unused-but-set-variable]
Oid parentoid;^


It seems to be just a typo. With this minor fix the patch compiles and 
passes tests.

inline patch 1: src/backend/commands/indexcmds.c (+1/-1 lines) download inline-1.patch
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75008eebde..f5b3c53a83 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2864,7 +2864,7 @@ reindex_partitions(Oid relid, int options, bool concurrent,
  
                 /* Save partition OID */
                 old_context = MemoryContextSwitchTo(reindex_context);
-               partitions = lappend_oid(partitions, partoid);
+               partitions = lappend_oid(partitions, parentoid);
                 MemoryContextSwitchTo(old_context);
         }


If this guessed fix is correct, I see the problem in the patch logic. In 
reindex_partitions() we collect parent relations to pass them to 
reindex_multiple_internal(). It implicitly changes the logic from 
REINDEX INDEX to REINDEX RELATION, which is not the same, if table has 
more than one index. For example, if I add one more index to a 
partitioned table from a create_index.sql test:

create index idx on concur_reindex_part_0_2 using btree (c2);

and call

REINDEX INDEX CONCURRENTLY concur_reindex_part_index;

idx will be reindexed as well. I doubt that it is the desired behavior.


A few more random issues, that I noticed at first glance:

1) in create_index.sql

Are these two lines intentional checks that must fail? If so, I propose 
to add a comment.

REINDEX TABLE concur_reindex_part_index;
REINDEX TABLE CONCURRENTLY concur_reindex_part_index;

A few lines around also look like they were copy-pasted and need a 
second look.

2)  This part of ReindexRelationConcurrently() is misleading.

         case RELKIND_PARTITIONED_TABLE:
         case RELKIND_PARTITIONED_INDEX:
         default:
             /* Return error if type of relation is not supported */
             ereport(ERROR,
                     (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                      errmsg("cannot reindex this type of relation concurrently")));


Maybe assertion is enough. It seems, that we should never reach this 
code because both ReindexTable and ReindexIndex handle those relkinds 
separately.  Which leads me to the next question.

3) Is there a reason, why reindex_partitions() is not inside 
ReindexRelationConcurrently() ? I think that logically it belongs there.

4) I haven't tested multi-level partitions yet. In any case, it would be 
good to document this behavior explicitly.


I need a bit more time to review this patch more thoroughly. Please, 
wait for it, before committing.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-04 00:51  Michael Paquier <michael@paquier.xyz>
  parent: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
  0 siblings, 1 reply; 8+ messages in thread

From: Michael Paquier @ 2020-09-04 00:51 UTC (permalink / raw)
  To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On Thu, Sep 03, 2020 at 10:02:58PM +0300, Anastasia Lubennikova wrote:
> First of all, this patch fails at cfbot:
> 
> indexcmds.c:2848:7: error: variable ‘parentoid’ set but not used
> [-Werror=unused-but-set-variable]
> Oid parentoid;^

Missed this warning, thanks for pointing it out.  This is an incorrect
rebase: this variable was used as a workaround to take a session lock
on the parent table to be reindexed, session lock logic existing to
prevent cache lookup errors when dropping some portions of the tree
concurrently (1d65416 as you already know).  But we don't need that
anymore.

> If this guessed fix is correct, I see the problem in the patch logic. In
> reindex_partitions() we collect parent relations to pass them to
> reindex_multiple_internal(). It implicitly changes the logic from REINDEX
> INDEX to REINDEX RELATION, which is not the same, if table has more than one
> index.

Incorrect guess here.  parentoid refers to the parent table of an
index, so by saving its OID in the list of things to reindex, you
would finish by reindexing all the indexes of a partition.  We need to
use partoid, as returned by find_all_inheritors() for all the members
of the partition tree (relid can be a partitioned index or partitioned
table in reindex_partitions).

> 1) in create_index.sql
> 
> Are these two lines intentional checks that must fail? If so, I propose to
> add a comment.
> 
> REINDEX TABLE concur_reindex_part_index;
> REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
> 
> A few lines around also look like they were copy-pasted and need a second
> look.

You can note some slight differences though.  These are test cases for
REINDEX INDEX with tables, and REINDEX TABLE with indexes.  What you
are quoting here is the part for indexes with REINDEX TABLE.  And
there are already comments, see:
"-- REINDEX INDEX fails for partitioned tables"
"-- REINDEX TABLE fails for partitioned indexes"

Perhaps that's not enough, so I have added some more comments to
outline that these commands fail (8 commands in total).

> 2)  This part of ReindexRelationConcurrently() is misleading.
> 
> Maybe assertion is enough. It seems, that we should never reach this code
> because both ReindexTable and ReindexIndex handle those relkinds
> separately.  Which leads me to the next question.

Yes, we could use an assert, but I did not feel any strong need to
change that either for this patch.

> 3) Is there a reason, why reindex_partitions() is not inside
> ReindexRelationConcurrently() ? I think that logically it belongs there.

Yes, it simplifies the build of the index list indexIds, as there is
no need to loop back into a different routine if working on a table or
a matview.

> 4) I haven't tested multi-level partitions yet. In any case, it would be
> good to document this behavior explicitly.

Not sure what addition we could do here.  The patch states that each
partition of the partitioned relation defined gets reindexed, which
implies that this handles multiple layers automatically.

> I need a bit more time to review this patch more thoroughly. Please, wait
> for it, before committing.

Glad to hear that, please take the time you need.
--
Michael

Attachments:

reindex-part-v9.patch (text/x-diff, 40.3K) download
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index c26a102b17..22db3f660d 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,8 +34,10 @@ extern ObjectAddress DefineIndex(Oid relationId,
 								 bool check_not_in_use,
 								 bool skip_build,
 								 bool quiet);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent);
-extern Oid	ReindexTable(RangeVar *relation, int options, bool concurrent);
+extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent,
+						 bool isTopLevel);
+extern Oid	ReindexTable(RangeVar *relation, int options, bool concurrent,
+						 bool isTopLevel);
 extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 								  int options, bool concurrent);
 extern char *makeObjectName(const char *name1, const char *name2,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1d662e9af4..1764739ff5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -77,6 +77,7 @@
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/pg_rusage.h"
+#include "utils/rel.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tuplesort.h"
@@ -3473,11 +3474,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 									 iRel->rd_rel->relam);
 
 	/*
-	 * The case of reindexing partitioned tables and indexes is handled
-	 * differently by upper layers, so this case shouldn't arise.
+	 * Partitioned indexes should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (iRel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-		elog(ERROR, "unsupported relation kind for index \"%s\"",
+		elog(ERROR, "cannot reindex partitioned index \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(iRel)),
 			 RelationGetRelationName(iRel));
 
 	/*
@@ -3694,20 +3696,13 @@ reindex_relation(Oid relid, int flags, int options)
 		return false;
 
 	/*
-	 * This may be useful when implemented someday; but that day is not today.
-	 * For now, avoid erroring out when called in a multi-table context
-	 * (REINDEX SCHEMA) and happen to come across a partitioned table.  The
-	 * partitions may be reindexed on their own anyway.
+	 * Partitioned tables should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-	{
-		ereport(WARNING,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-						RelationGetRelationName(rel))));
-		table_close(rel, ShareLock);
-		return false;
-	}
+		elog(ERROR, "cannot reindex partitioned table \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(rel)),
+			 RelationGetRelationName(rel));
 
 	toast_relid = rel->rd_rel->reltoastrelid;
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b3a92381f9..a756c309e0 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -88,7 +88,12 @@ static List *ChooseIndexColumnNames(List *indexElems);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
 											Oid relId, Oid oldRelId, void *arg);
 static bool ReindexRelationConcurrently(Oid relationOid, int options);
-static void ReindexPartitionedIndex(Relation parentIdx);
+
+static void reindex_partitions(Oid relid, int options, bool concurrent,
+							   bool isTopLevel);
+static void reindex_multiple_internal(List *relids, int options,
+									  bool concurrent);
+static void reindex_error_callback(void *args);
 static void update_relispartition(Oid relationId, bool newval);
 static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
 
@@ -101,6 +106,16 @@ struct ReindexIndexCallbackState
 	Oid			locked_table_oid;	/* tracks previously locked table */
 };
 
+/*
+ * callback arguments for reindex_error_callback()
+ */
+typedef struct ReindexErrorInfo
+{
+	char	   *relname;
+	char	   *relnamespace;
+	char		relkind;
+} ReindexErrorInfo;
+
 /*
  * CheckIndexCompatible
  *		Determine whether an existing index definition is compatible with a
@@ -2420,11 +2435,10 @@ ChooseIndexColumnNames(List *indexElems)
  *		Recreate a specific index.
  */
 void
-ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
+ReindexIndex(RangeVar *indexRelation, int options, bool concurrent, bool isTopLevel)
 {
 	struct ReindexIndexCallbackState state;
 	Oid			indOid;
-	Relation	irel;
 	char		persistence;
 
 	/*
@@ -2445,22 +2459,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
 									  RangeVarCallbackForReindexIndex,
 									  &state);
 
-	/*
-	 * Obtain the current persistence of the existing index.  We already hold
-	 * lock on the index.
-	 */
-	irel = index_open(indOid, NoLock);
-
-	if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-	{
-		ReindexPartitionedIndex(irel);
-		return;
-	}
-
-	persistence = irel->rd_rel->relpersistence;
-	index_close(irel, NoLock);
-
-	if (concurrent && persistence != RELPERSISTENCE_TEMP)
+	persistence = get_rel_persistence(indOid);
+	if (get_rel_relkind(indOid) == RELKIND_PARTITIONED_INDEX)
+		reindex_partitions(indOid, options, concurrent, isTopLevel);
+	else if (concurrent && persistence != RELPERSISTENCE_TEMP)
 		ReindexRelationConcurrently(indOid, options);
 	else
 		reindex_index(indOid, false, persistence,
@@ -2542,7 +2544,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
  *		Recreate all indexes of a table (and of its toast table, if any)
  */
 Oid
-ReindexTable(RangeVar *relation, int options, bool concurrent)
+ReindexTable(RangeVar *relation, int options, bool concurrent, bool isTopLevel)
 {
 	Oid			heapOid;
 	bool		result;
@@ -2560,7 +2562,9 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
 									   0,
 									   RangeVarCallbackOwnsTable, NULL);
 
-	if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+	if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
+		reindex_partitions(heapOid, options, concurrent, isTopLevel);
+	else if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
 	{
 		result = ReindexRelationConcurrently(heapOid, options);
 
@@ -2604,7 +2608,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	MemoryContext private_context;
 	MemoryContext old;
 	List	   *relids = NIL;
-	ListCell   *l;
 	int			num_keys;
 	bool		concurrent_warning = false;
 
@@ -2688,11 +2691,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 		 * Only regular tables and matviews can have indexes, so ignore any
 		 * other kind of relation.
 		 *
-		 * It is tempting to also consider partitioned tables here, but that
-		 * has the problem that if the children are in the same schema, they
-		 * would be processed twice.  Maybe we could have a separate list of
-		 * partitioned tables, and expand that afterwards into relids,
-		 * ignoring any duplicates.
+		 * Partitioned tables/indexes are skipped but matching leaf partitions
+		 * are processed.
 		 */
 		if (classtuple->relkind != RELKIND_RELATION &&
 			classtuple->relkind != RELKIND_MATVIEW)
@@ -2755,14 +2755,152 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	table_endscan(scan);
 	table_close(relationRelation, AccessShareLock);
 
-	/* Now reindex each rel in a separate transaction */
+	/*
+	 * Process each relation listed in a separate transaction.  Note that this
+	 * commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(relids, options, concurrent);
+
+	MemoryContextDelete(private_context);
+}
+
+/*
+ * Error callback specific to reindex_partitions().
+ */
+static void
+reindex_error_callback(void *arg)
+{
+	ReindexErrorInfo *errinfo = (ReindexErrorInfo *) arg;
+
+	Assert(errinfo->relkind == RELKIND_PARTITIONED_INDEX ||
+		   errinfo->relkind == RELKIND_PARTITIONED_TABLE);
+
+	if (errinfo->relkind == RELKIND_PARTITIONED_TABLE)
+		errcontext("while reindexing partitioned table \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+	else if (errinfo->relkind == RELKIND_PARTITIONED_INDEX)
+		errcontext("while reindexing partitioned index \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+}
+
+/*
+ * reindex_partitions
+ *
+ * Reindex a set of partitions, per the partitioned index or table given
+ * by the caller.
+ */
+static void
+reindex_partitions(Oid relid, int options, bool concurrent,
+				   bool isTopLevel)
+{
+	List	   *partitions = NIL;
+	char		relkind = get_rel_relkind(relid);
+	char	   *relname = get_rel_name(relid);
+	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
+	MemoryContext reindex_context;
+	List	   *inhoids;
+	ListCell   *lc;
+	ErrorContextCallback errcallback;
+	ReindexErrorInfo errinfo;
+
+	Assert(relkind == RELKIND_PARTITIONED_INDEX ||
+		   relkind == RELKIND_PARTITIONED_TABLE);
+
+	/*
+	 * Check if this runs in a transaction block, with an error callback to
+	 * provide the context under which a problem happens.
+	 */
+	errinfo.relname = pstrdup(relname);
+	errinfo.relnamespace = pstrdup(relnamespace);
+	errinfo.relkind = relkind;
+	errcallback.callback = reindex_error_callback;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
+	PreventInTransactionBlock(isTopLevel,
+							  relkind == RELKIND_PARTITIONED_TABLE ?
+							  "REINDEX TABLE" : "REINDEX INDEX");
+
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
+	/*
+	 * Create special memory context for cross-transaction storage.
+	 *
+	 * Since it is a child of PortalContext, it will go away eventually even
+	 * if we suffer an error so there is no need for special abort cleanup
+	 * logic.
+	 */
+	reindex_context = AllocSetContextCreate(PortalContext, "Reindex",
+											ALLOCSET_DEFAULT_SIZES);
+
+	/* ShareLock is enough to prevent schema modifications */
+	inhoids = find_all_inheritors(relid, ShareLock, NULL);
+
+	/*
+	 * The list of relations to reindex are the physical partitions of the
+	 * tree so discard any partitioned table or index.
+	 */
+	foreach(lc, inhoids)
+	{
+		Oid			partoid = lfirst_oid(lc);
+		char		partkind = get_rel_relkind(partoid);
+		MemoryContext old_context;
+
+		/*
+		 * This discards partitioned tables, partitioned indexes and foreign
+		 * tables.
+		 */
+		if (!RELKIND_HAS_STORAGE(partkind))
+			continue;
+
+		Assert(partkind == RELKIND_INDEX ||
+			   partkind == RELKIND_RELATION);
+
+		/* Save partition OID */
+		old_context = MemoryContextSwitchTo(reindex_context);
+		partitions = lappend_oid(partitions, partoid);
+		MemoryContextSwitchTo(old_context);
+	}
+
+	/*
+	 * Process each partition listed in a separate transaction.  Note that
+	 * this commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(partitions, options, concurrent);
+
+	/*
+	 * Clean up working storage --- note we must do this after
+	 * StartTransactionCommand, else we might be trying to delete the active
+	 * context!
+	 */
+	MemoryContextDelete(reindex_context);
+}
+
+/*
+ * reindex_multiple_internal
+ *
+ * Reindex a list of relations, each one being processed in its own
+ * transaction.  This commits the existing transaction immediately,
+ * and starts a new transaction when finished.
+ */
+static void
+reindex_multiple_internal(List *relids, int options, bool concurrent)
+{
+	ListCell   *l;
+
 	PopActiveSnapshot();
 	CommitTransactionCommand();
+
 	foreach(l, relids)
 	{
 		Oid			relid = lfirst_oid(l);
+		char		relkind;
+		char		relpersistence;
 
 		StartTransactionCommand();
+
 		/* functions in indexes may want a snapshot set */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -2774,6 +2912,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 			continue;
 		}
 
+		relkind = get_rel_relkind(relid);
+		relpersistence = get_rel_persistence(relid);
+
+		/*
+		 * Partitioned tables and indexes can never be processed directly, and
+		 * a list of their leaves should be built first.
+		 */
+		Assert(relkind != RELKIND_PARTITIONED_INDEX &&
+			   relkind != RELKIND_PARTITIONED_TABLE);
+
 		if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
 		{
 			(void) ReindexRelationConcurrently(relid,
@@ -2781,6 +2929,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 											   REINDEXOPT_MISSING_OK);
 			/* ReindexRelationConcurrently() does the verbose output */
 		}
+		else if (relkind == RELKIND_INDEX)
+		{
+			reindex_index(relid, false, relpersistence,
+						  options | REINDEXOPT_REPORT_PROGRESS);
+			PopActiveSnapshot();
+		}
 		else
 		{
 			bool		result;
@@ -2803,9 +2957,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 
 		CommitTransactionCommand();
 	}
-	StartTransactionCommand();
 
-	MemoryContextDelete(private_context);
+	StartTransactionCommand();
 }
 
 
@@ -2817,8 +2970,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
  * view.  For tables and materialized views, all its indexes will be rebuilt,
  * excluding invalid indexes and any indexes used in exclusion constraints,
  * but including its associated toast table indexes.  For indexes, the index
- * itself will be rebuilt.  If 'relationOid' belongs to a partitioned table
- * then we issue a warning to mention these are not yet supported.
+ * itself will be rebuilt.
  *
  * The locks taken on parent tables and involved indexes are kept until the
  * transaction is committed, at which point a session lock is taken on each
@@ -3057,13 +3209,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 				MemoryContextSwitchTo(oldcontext);
 				break;
 			}
+
 		case RELKIND_PARTITIONED_TABLE:
-			/* see reindex_relation() */
-			ereport(WARNING,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-							get_rel_name(relationOid))));
-			return false;
+		case RELKIND_PARTITIONED_INDEX:
 		default:
 			/* Return error if type of relation is not supported */
 			ereport(ERROR,
@@ -3530,20 +3678,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	return true;
 }
 
-/*
- *	ReindexPartitionedIndex
- *		Reindex each child of the given partitioned index.
- *
- * Not yet implemented.
- */
-static void
-ReindexPartitionedIndex(Relation parentIdx)
-{
-	ereport(ERROR,
-			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-			 errmsg("REINDEX is not yet implemented for partitioned indexes")));
-}
-
 /*
  * Insert or delete an appropriate pg_inherits tuple to make the given index
  * be a partition of the indicated parent index.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6154d2c8c6..35553e04ce 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -926,10 +926,12 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 				switch (stmt->kind)
 				{
 					case REINDEX_OBJECT_INDEX:
-						ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
+						ReindexIndex(stmt->relation, stmt->options,
+									 stmt->concurrent, isTopLevel);
 						break;
 					case REINDEX_OBJECT_TABLE:
-						ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
+						ReindexTable(stmt->relation, stmt->options,
+									 stmt->concurrent, isTopLevel);
 						break;
 					case REINDEX_OBJECT_SCHEMA:
 					case REINDEX_OBJECT_SYSTEM:
diff --git a/src/test/isolation/expected/reindex-partitions.out b/src/test/isolation/expected/reindex-partitions.out
new file mode 100644
index 0000000000..5373abde25
--- /dev/null
+++ b/src/test/isolation/expected/reindex-partitions.out
@@ -0,0 +1,107 @@
+Parsed test spec with 3 sessions
+
+starting permutation: begin1 lockexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 6acbb695ec..65d1443ac6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -50,6 +50,7 @@ test: lock-committed-keyupdate
 test: update-locked-tuple
 test: reindex-concurrently
 test: reindex-schema
+test: reindex-partitions
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-partitions.spec b/src/test/isolation/specs/reindex-partitions.spec
new file mode 100644
index 0000000000..738473648d
--- /dev/null
+++ b/src/test/isolation/specs/reindex-partitions.spec
@@ -0,0 +1,61 @@
+# REINDEX with partitioned tables
+#
+# Ensure that concurrent and non-concurrent operations work correctly when
+# a REINDEX is performed on a partitioned table or index.
+
+# Note that this does not test for partition creation in parallel of a
+# REINDEX.  The initial transaction of REINDEX building the partition list
+# would make the CREATE TABLE command to wait, and once the first
+# transaction commits it could be possible for the CREATE TABLE command
+# to finish before the concurrent REINDEX is done, making the output
+# unpredictable.
+
+setup
+{
+	CREATE TABLE reind_conc_parent (id int) PARTITION BY RANGE (id);
+	CREATE TABLE reind_conc_0_10 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (0) TO (10);
+	CREATE TABLE reind_conc_10_20 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (10) TO (20);
+	INSERT INTO reind_conc_parent VALUES (generate_series(0, 19));
+}
+
+teardown
+{
+	DROP TABLE reind_conc_parent;
+}
+
+session "s1"
+step "begin1"          { BEGIN; }
+step "lockexcl1"       { LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE; }
+step "lockshare1"      { LOCK reind_conc_parent IN SHARE MODE; }
+step "lockupdate1"     { LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE; }
+step "lockpartexcl1"   { LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE; }
+step "lockpartshare1"  { LOCK reind_conc_10_20 IN SHARE MODE; }
+step "lockpartupdate1" { LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE; }
+step "end1"            { COMMIT; }
+
+session "s2"
+step "reindex2"        { REINDEX TABLE reind_conc_parent; }
+step "reindex_conc2"   { REINDEX TABLE CONCURRENTLY reind_conc_parent; }
+
+session "s3"
+step "drop3"           { DROP TABLE reind_conc_10_20; }
+
+# An existing partition leaf is dropped after reindex is done when the
+# parent is locked.
+permutation "begin1" "lockexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex_conc2" "drop3" "end1"
+
+# An existing partition leaf is dropped after reindex is done when this
+# leaf is locked.
+permutation "begin1" "lockpartexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex_conc2" "drop3" "end1"
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index e3e6634d7e..12d2a87aa6 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2196,18 +2196,6 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes to reindex
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes that can be reindexed concurrently
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
              relid             |         parentrelid         | level 
@@ -2320,6 +2308,152 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index; -- error
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index; -- error
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10; -- error
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10; -- error
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ERROR:  REINDEX INDEX cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned index "public.concur_reindex_part_index"
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part; -- error
+ERROR:  "concur_reindex_part" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part; -- error
+ERROR:  "concur_reindex_part" is not an index
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10; -- error
+ERROR:  "concur_reindex_part_10" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10; -- error
+ERROR:  "concur_reindex_part_10" is not an index
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ERROR:  REINDEX TABLE cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned table "public.concur_reindex_part"
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 -- Check errors
 -- Cannot run inside a transaction block
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f3667bacdc..37f7259da9 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -903,12 +903,6 @@ CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2 (c1);
 ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_part_index_0_2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
 -- REINDEX should preserve dependencies of partition tree.
@@ -948,6 +942,88 @@ WHERE classid = 'pg_class'::regclass AND
   ORDER BY 1, 2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
+
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index; -- error
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index; -- error
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10; -- error
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10; -- error
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part; -- error
+REINDEX INDEX CONCURRENTLY concur_reindex_part; -- error
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10; -- error
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10; -- error
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 
 -- Check errors
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..33af4ae02a 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -88,7 +88,9 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
     <term><literal>INDEX</literal></term>
     <listitem>
      <para>
-      Recreate the specified index.
+      Recreate the specified index. This form of <command>REINDEX</command>
+      cannot be executed inside a transaction block when used with a
+      partitioned index.
      </para>
     </listitem>
    </varlistentry>
@@ -99,6 +101,8 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
      <para>
       Recreate all indexes of the specified table.  If the table has a
       secondary <quote>TOAST</quote> table, that is reindexed as well.
+      This form of <command>REINDEX</command> cannot be executed inside a
+      transaction block when used with a partitioned table.
      </para>
     </listitem>
    </varlistentry>
@@ -259,8 +263,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
   </para>
 
   <para>
-   Reindexing partitioned tables or partitioned indexes is not supported.
-   Each individual partition can be reindexed separately instead.
+   Reindexing partitioned indexes or partitioned tables is supported
+   with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
+   respectively. Each partition of the specified partitioned relation is
+   reindexed in a separate transaction. Those commands cannot be used inside
+   a transaction block when working on a partitioned table or index.
   </para>
 
   <refsect2 id="sql-reindex-concurrently" xreflabel="Rebuilding Indexes Concurrently">
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 90db550b92..84bc0ee381 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4044,6 +4044,27 @@ SELECT f_test(100);
 
 DROP FUNCTION f_test(int);
 -- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+-- ===================================================================
 -- conversion error
 -- ===================================================================
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 83971665e3..d452d06343 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1081,6 +1081,26 @@ $$ LANGUAGE plpgsql;
 SELECT f_test(100);
 DROP FUNCTION f_test(int);
 
+-- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+
 -- ===================================================================
 -- conversion error
 -- ===================================================================
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 500623e230..c83821c0b5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2037,6 +2037,7 @@ RegProcedure
 Regis
 RegisNode
 RegisteredBgWorker
+ReindexErrorInfo
 ReindexObjectType
 ReindexStmt
 ReindexType

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-07 01:58  Michael Paquier <michael@paquier.xyz>
  parent: Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 8+ messages in thread

From: Michael Paquier @ 2020-09-07 01:58 UTC (permalink / raw)
  To: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On Fri, Sep 04, 2020 at 09:51:13AM +0900, Michael Paquier wrote:
> Glad to hear that, please take the time you need.

Attached is a rebased version to address the various conflicts after
844c05a.
--
Michael

Attachments:

reindex-part-v10.patch (text/x-diff, 40.3K) download
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 3129b684f6..7a079ef07f 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,8 +34,8 @@ extern ObjectAddress DefineIndex(Oid relationId,
 								 bool check_not_in_use,
 								 bool skip_build,
 								 bool quiet);
-extern void ReindexIndex(RangeVar *indexRelation, int options);
-extern Oid	ReindexTable(RangeVar *relation, int options);
+extern void ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel);
+extern Oid	ReindexTable(RangeVar *relation, int options, bool isTopLevel);
 extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 								  int options);
 extern char *makeObjectName(const char *name1, const char *name2,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b2c8cb320e..117e3fdef7 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -77,6 +77,7 @@
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/pg_rusage.h"
+#include "utils/rel.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/tuplesort.h"
@@ -3486,11 +3487,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
 									 iRel->rd_rel->relam);
 
 	/*
-	 * The case of reindexing partitioned tables and indexes is handled
-	 * differently by upper layers, so this case shouldn't arise.
+	 * Partitioned indexes should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (iRel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-		elog(ERROR, "unsupported relation kind for index \"%s\"",
+		elog(ERROR, "cannot reindex partitioned index \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(iRel)),
 			 RelationGetRelationName(iRel));
 
 	/*
@@ -3707,20 +3709,13 @@ reindex_relation(Oid relid, int flags, int options)
 		return false;
 
 	/*
-	 * This may be useful when implemented someday; but that day is not today.
-	 * For now, avoid erroring out when called in a multi-table context
-	 * (REINDEX SCHEMA) and happen to come across a partitioned table.  The
-	 * partitions may be reindexed on their own anyway.
+	 * Partitioned tables should never get processed here, as they have no
+	 * physical storage.
 	 */
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-	{
-		ereport(WARNING,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-						RelationGetRelationName(rel))));
-		table_close(rel, ShareLock);
-		return false;
-	}
+		elog(ERROR, "cannot reindex partitioned table \"%s.%s\"",
+			 get_namespace_name(RelationGetNamespace(rel)),
+			 RelationGetRelationName(rel));
 
 	toast_relid = rel->rd_rel->reltoastrelid;
 
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 430e88b4c9..80570318b4 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -88,7 +88,10 @@ static List *ChooseIndexColumnNames(List *indexElems);
 static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
 											Oid relId, Oid oldRelId, void *arg);
 static bool ReindexRelationConcurrently(Oid relationOid, int options);
-static void ReindexPartitionedIndex(Relation parentIdx);
+
+static void reindex_partitions(Oid relid, int options, bool isTopLevel);
+static void reindex_multiple_internal(List *relids, int options);
+static void reindex_error_callback(void *args);
 static void update_relispartition(Oid relationId, bool newval);
 static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
 
@@ -101,6 +104,16 @@ struct ReindexIndexCallbackState
 	Oid			locked_table_oid;	/* tracks previously locked table */
 };
 
+/*
+ * callback arguments for reindex_error_callback()
+ */
+typedef struct ReindexErrorInfo
+{
+	char	   *relname;
+	char	   *relnamespace;
+	char		relkind;
+} ReindexErrorInfo;
+
 /*
  * CheckIndexCompatible
  *		Determine whether an existing index definition is compatible with a
@@ -2420,12 +2433,12 @@ ChooseIndexColumnNames(List *indexElems)
  *		Recreate a specific index.
  */
 void
-ReindexIndex(RangeVar *indexRelation, int options)
+ReindexIndex(RangeVar *indexRelation, int options, bool isTopLevel)
 {
 	struct ReindexIndexCallbackState state;
 	Oid			indOid;
-	Relation	irel;
 	char		persistence;
+	char		relkind;
 
 	/*
 	 * Find and lock index, and check permissions on table; use callback to
@@ -2445,24 +2458,17 @@ ReindexIndex(RangeVar *indexRelation, int options)
 									  0,
 									  RangeVarCallbackForReindexIndex,
 									  &state);
-
 	/*
-	 * Obtain the current persistence of the existing index.  We already hold
-	 * lock on the index.
+	 * Obtain the current persistence and kind of the existing index.  We
+	 * already hold a lock on the index.
 	 */
-	irel = index_open(indOid, NoLock);
+	persistence = get_rel_persistence(indOid);
+	relkind = get_rel_relkind(indOid);
 
-	if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
-	{
-		ReindexPartitionedIndex(irel);
-		return;
-	}
-
-	persistence = irel->rd_rel->relpersistence;
-	index_close(irel, NoLock);
-
-	if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
-		persistence != RELPERSISTENCE_TEMP)
+	if (relkind == RELKIND_PARTITIONED_INDEX)
+		reindex_partitions(indOid, options, isTopLevel);
+	else if ((options & REINDEXOPT_CONCURRENTLY) != 0  &&
+			 persistence != RELPERSISTENCE_TEMP)
 		ReindexRelationConcurrently(indOid, options);
 	else
 		reindex_index(indOid, false, persistence,
@@ -2545,7 +2551,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
  *		Recreate all indexes of a table (and of its toast table, if any)
  */
 Oid
-ReindexTable(RangeVar *relation, int options)
+ReindexTable(RangeVar *relation, int options, bool isTopLevel)
 {
 	Oid			heapOid;
 	bool		result;
@@ -2564,8 +2570,10 @@ ReindexTable(RangeVar *relation, int options)
 									   0,
 									   RangeVarCallbackOwnsTable, NULL);
 
-	if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
-		get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+	if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
+		reindex_partitions(heapOid, options, isTopLevel);
+	else if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
+			 get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
 	{
 		result = ReindexRelationConcurrently(heapOid, options);
 
@@ -2609,7 +2617,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	MemoryContext private_context;
 	MemoryContext old;
 	List	   *relids = NIL;
-	ListCell   *l;
 	int			num_keys;
 	bool		concurrent_warning = false;
 
@@ -2694,11 +2701,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 		 * Only regular tables and matviews can have indexes, so ignore any
 		 * other kind of relation.
 		 *
-		 * It is tempting to also consider partitioned tables here, but that
-		 * has the problem that if the children are in the same schema, they
-		 * would be processed twice.  Maybe we could have a separate list of
-		 * partitioned tables, and expand that afterwards into relids,
-		 * ignoring any duplicates.
+		 * Partitioned tables/indexes are skipped but matching leaf partitions
+		 * are processed.
 		 */
 		if (classtuple->relkind != RELKIND_RELATION &&
 			classtuple->relkind != RELKIND_MATVIEW)
@@ -2761,14 +2765,151 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 	table_endscan(scan);
 	table_close(relationRelation, AccessShareLock);
 
-	/* Now reindex each rel in a separate transaction */
+	/*
+	 * Process each relation listed in a separate transaction.  Note that this
+	 * commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(relids, options);
+
+	MemoryContextDelete(private_context);
+}
+
+/*
+ * Error callback specific to reindex_partitions().
+ */
+static void
+reindex_error_callback(void *arg)
+{
+	ReindexErrorInfo *errinfo = (ReindexErrorInfo *) arg;
+
+	Assert(errinfo->relkind == RELKIND_PARTITIONED_INDEX ||
+		   errinfo->relkind == RELKIND_PARTITIONED_TABLE);
+
+	if (errinfo->relkind == RELKIND_PARTITIONED_TABLE)
+		errcontext("while reindexing partitioned table \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+	else if (errinfo->relkind == RELKIND_PARTITIONED_INDEX)
+		errcontext("while reindexing partitioned index \"%s.%s\"",
+				   errinfo->relnamespace, errinfo->relname);
+}
+
+/*
+ * reindex_partitions
+ *
+ * Reindex a set of partitions, per the partitioned index or table given
+ * by the caller.
+ */
+static void
+reindex_partitions(Oid relid, int options, bool isTopLevel)
+{
+	List	   *partitions = NIL;
+	char		relkind = get_rel_relkind(relid);
+	char	   *relname = get_rel_name(relid);
+	char	   *relnamespace = get_namespace_name(get_rel_namespace(relid));
+	MemoryContext reindex_context;
+	List	   *inhoids;
+	ListCell   *lc;
+	ErrorContextCallback errcallback;
+	ReindexErrorInfo errinfo;
+
+	Assert(relkind == RELKIND_PARTITIONED_INDEX ||
+		   relkind == RELKIND_PARTITIONED_TABLE);
+
+	/*
+	 * Check if this runs in a transaction block, with an error callback to
+	 * provide the context under which a problem happens.
+	 */
+	errinfo.relname = pstrdup(relname);
+	errinfo.relnamespace = pstrdup(relnamespace);
+	errinfo.relkind = relkind;
+	errcallback.callback = reindex_error_callback;
+	errcallback.arg = (void *) &errinfo;
+	errcallback.previous = error_context_stack;
+	error_context_stack = &errcallback;
+
+	PreventInTransactionBlock(isTopLevel,
+							  relkind == RELKIND_PARTITIONED_TABLE ?
+							  "REINDEX TABLE" : "REINDEX INDEX");
+
+	/* Pop the error context stack */
+	error_context_stack = errcallback.previous;
+
+	/*
+	 * Create special memory context for cross-transaction storage.
+	 *
+	 * Since it is a child of PortalContext, it will go away eventually even
+	 * if we suffer an error so there is no need for special abort cleanup
+	 * logic.
+	 */
+	reindex_context = AllocSetContextCreate(PortalContext, "Reindex",
+											ALLOCSET_DEFAULT_SIZES);
+
+	/* ShareLock is enough to prevent schema modifications */
+	inhoids = find_all_inheritors(relid, ShareLock, NULL);
+
+	/*
+	 * The list of relations to reindex are the physical partitions of the
+	 * tree so discard any partitioned table or index.
+	 */
+	foreach(lc, inhoids)
+	{
+		Oid			partoid = lfirst_oid(lc);
+		char		partkind = get_rel_relkind(partoid);
+		MemoryContext old_context;
+
+		/*
+		 * This discards partitioned tables, partitioned indexes and foreign
+		 * tables.
+		 */
+		if (!RELKIND_HAS_STORAGE(partkind))
+			continue;
+
+		Assert(partkind == RELKIND_INDEX ||
+			   partkind == RELKIND_RELATION);
+
+		/* Save partition OID */
+		old_context = MemoryContextSwitchTo(reindex_context);
+		partitions = lappend_oid(partitions, partoid);
+		MemoryContextSwitchTo(old_context);
+	}
+
+	/*
+	 * Process each partition listed in a separate transaction.  Note that
+	 * this commits and then starts a new transaction immediately.
+	 */
+	reindex_multiple_internal(partitions, options);
+
+	/*
+	 * Clean up working storage --- note we must do this after
+	 * StartTransactionCommand, else we might be trying to delete the active
+	 * context!
+	 */
+	MemoryContextDelete(reindex_context);
+}
+
+/*
+ * reindex_multiple_internal
+ *
+ * Reindex a list of relations, each one being processed in its own
+ * transaction.  This commits the existing transaction immediately,
+ * and starts a new transaction when finished.
+ */
+static void
+reindex_multiple_internal(List *relids, int options)
+{
+	ListCell   *l;
+
 	PopActiveSnapshot();
 	CommitTransactionCommand();
+
 	foreach(l, relids)
 	{
 		Oid			relid = lfirst_oid(l);
+		char		relkind;
+		char		relpersistence;
 
 		StartTransactionCommand();
+
 		/* functions in indexes may want a snapshot set */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -2780,6 +2921,16 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 			continue;
 		}
 
+		relkind = get_rel_relkind(relid);
+		relpersistence = get_rel_persistence(relid);
+
+		/*
+		 * Partitioned tables and indexes can never be processed directly, and
+		 * a list of their leaves should be built first.
+		 */
+		Assert(relkind != RELKIND_PARTITIONED_INDEX &&
+			   relkind != RELKIND_PARTITIONED_TABLE);
+
 		if ((options & REINDEXOPT_CONCURRENTLY) != 0 &&
 			get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
 		{
@@ -2788,6 +2939,12 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 											   REINDEXOPT_MISSING_OK);
 			/* ReindexRelationConcurrently() does the verbose output */
 		}
+		else if (relkind == RELKIND_INDEX)
+		{
+			reindex_index(relid, false, relpersistence,
+						  options | REINDEXOPT_REPORT_PROGRESS);
+			PopActiveSnapshot();
+		}
 		else
 		{
 			bool		result;
@@ -2810,9 +2967,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
 
 		CommitTransactionCommand();
 	}
-	StartTransactionCommand();
 
-	MemoryContextDelete(private_context);
+	StartTransactionCommand();
 }
 
 
@@ -2824,8 +2980,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
  * view.  For tables and materialized views, all its indexes will be rebuilt,
  * excluding invalid indexes and any indexes used in exclusion constraints,
  * but including its associated toast table indexes.  For indexes, the index
- * itself will be rebuilt.  If 'relationOid' belongs to a partitioned table
- * then we issue a warning to mention these are not yet supported.
+ * itself will be rebuilt.
  *
  * The locks taken on parent tables and involved indexes are kept until the
  * transaction is committed, at which point a session lock is taken on each
@@ -3064,13 +3219,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 				MemoryContextSwitchTo(oldcontext);
 				break;
 			}
+
 		case RELKIND_PARTITIONED_TABLE:
-			/* see reindex_relation() */
-			ereport(WARNING,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
-							get_rel_name(relationOid))));
-			return false;
+		case RELKIND_PARTITIONED_INDEX:
 		default:
 			/* Return error if type of relation is not supported */
 			ereport(ERROR,
@@ -3537,20 +3688,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	return true;
 }
 
-/*
- *	ReindexPartitionedIndex
- *		Reindex each child of the given partitioned index.
- *
- * Not yet implemented.
- */
-static void
-ReindexPartitionedIndex(Relation parentIdx)
-{
-	ereport(ERROR,
-			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-			 errmsg("REINDEX is not yet implemented for partitioned indexes")));
-}
-
 /*
  * Insert or delete an appropriate pg_inherits tuple to make the given index
  * be a partition of the indicated parent index.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index b4cde5565e..9713a7ac41 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -926,10 +926,12 @@ standard_ProcessUtility(PlannedStmt *pstmt,
 				switch (stmt->kind)
 				{
 					case REINDEX_OBJECT_INDEX:
-						ReindexIndex(stmt->relation, stmt->options);
+						ReindexIndex(stmt->relation, stmt->options,
+									 isTopLevel);
 						break;
 					case REINDEX_OBJECT_TABLE:
-						ReindexTable(stmt->relation, stmt->options);
+						ReindexTable(stmt->relation, stmt->options,
+									 isTopLevel);
 						break;
 					case REINDEX_OBJECT_SCHEMA:
 					case REINDEX_OBJECT_SYSTEM:
diff --git a/src/test/isolation/expected/reindex-partitions.out b/src/test/isolation/expected/reindex-partitions.out
new file mode 100644
index 0000000000..5373abde25
--- /dev/null
+++ b/src/test/isolation/expected/reindex-partitions.out
@@ -0,0 +1,107 @@
+Parsed test spec with 3 sessions
+
+starting permutation: begin1 lockexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockexcl1: LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockshare1: LOCK reind_conc_parent IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockupdate1: LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartexcl1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartexcl1: LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent;
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartshare1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartshare1: LOCK reind_conc_10_20 IN SHARE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex2: REINDEX TABLE reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex2: <... completed>
+step drop3: <... completed>
+
+starting permutation: begin1 lockpartupdate1 reindex_conc2 drop3 end1
+step begin1: BEGIN;
+step lockpartupdate1: LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE;
+step reindex_conc2: REINDEX TABLE CONCURRENTLY reind_conc_parent; <waiting ...>
+step drop3: DROP TABLE reind_conc_10_20; <waiting ...>
+step end1: COMMIT;
+step reindex_conc2: <... completed>
+step drop3: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 6acbb695ec..65d1443ac6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -50,6 +50,7 @@ test: lock-committed-keyupdate
 test: update-locked-tuple
 test: reindex-concurrently
 test: reindex-schema
+test: reindex-partitions
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-partitions.spec b/src/test/isolation/specs/reindex-partitions.spec
new file mode 100644
index 0000000000..738473648d
--- /dev/null
+++ b/src/test/isolation/specs/reindex-partitions.spec
@@ -0,0 +1,61 @@
+# REINDEX with partitioned tables
+#
+# Ensure that concurrent and non-concurrent operations work correctly when
+# a REINDEX is performed on a partitioned table or index.
+
+# Note that this does not test for partition creation in parallel of a
+# REINDEX.  The initial transaction of REINDEX building the partition list
+# would make the CREATE TABLE command to wait, and once the first
+# transaction commits it could be possible for the CREATE TABLE command
+# to finish before the concurrent REINDEX is done, making the output
+# unpredictable.
+
+setup
+{
+	CREATE TABLE reind_conc_parent (id int) PARTITION BY RANGE (id);
+	CREATE TABLE reind_conc_0_10 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (0) TO (10);
+	CREATE TABLE reind_conc_10_20 PARTITION OF reind_conc_parent
+	  FOR VALUES FROM (10) TO (20);
+	INSERT INTO reind_conc_parent VALUES (generate_series(0, 19));
+}
+
+teardown
+{
+	DROP TABLE reind_conc_parent;
+}
+
+session "s1"
+step "begin1"          { BEGIN; }
+step "lockexcl1"       { LOCK reind_conc_parent IN ACCESS EXCLUSIVE MODE; }
+step "lockshare1"      { LOCK reind_conc_parent IN SHARE MODE; }
+step "lockupdate1"     { LOCK reind_conc_parent IN SHARE UPDATE EXCLUSIVE MODE; }
+step "lockpartexcl1"   { LOCK reind_conc_10_20 IN ACCESS EXCLUSIVE MODE; }
+step "lockpartshare1"  { LOCK reind_conc_10_20 IN SHARE MODE; }
+step "lockpartupdate1" { LOCK reind_conc_10_20 IN SHARE UPDATE EXCLUSIVE MODE; }
+step "end1"            { COMMIT; }
+
+session "s2"
+step "reindex2"        { REINDEX TABLE reind_conc_parent; }
+step "reindex_conc2"   { REINDEX TABLE CONCURRENTLY reind_conc_parent; }
+
+session "s3"
+step "drop3"           { DROP TABLE reind_conc_10_20; }
+
+# An existing partition leaf is dropped after reindex is done when the
+# parent is locked.
+permutation "begin1" "lockexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockupdate1" "reindex_conc2" "drop3" "end1"
+
+# An existing partition leaf is dropped after reindex is done when this
+# leaf is locked.
+permutation "begin1" "lockpartexcl1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartexcl1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartshare1" "reindex_conc2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex2" "drop3" "end1"
+permutation "begin1" "lockpartupdate1" "reindex_conc2" "drop3" "end1"
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 814416d936..64c0c66859 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2194,18 +2194,6 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
-ERROR:  REINDEX is not yet implemented for partitioned indexes
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes to reindex
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE:  table "concur_reindex_part_10" has no indexes that can be reindexed concurrently
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
              relid             |         parentrelid         | level 
@@ -2318,6 +2306,152 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
  concur_reindex_part_index_0_2 | concur_reindex_part_index_0 |     2
 (5 rows)
 
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index; -- error
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index; -- error
+ERROR:  "concur_reindex_part_index" is not a table or materialized view
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10; -- error
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10; -- error
+ERROR:  "concur_reindex_part_index_10" is not a table or materialized view
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ERROR:  REINDEX INDEX cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned index "public.concur_reindex_part_index"
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part; -- error
+ERROR:  "concur_reindex_part" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part; -- error
+ERROR:  "concur_reindex_part" is not an index
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10; -- error
+ERROR:  "concur_reindex_part_10" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10; -- error
+ERROR:  "concur_reindex_part_10" is not an index
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ERROR:  REINDEX TABLE cannot run inside a transaction block
+CONTEXT:  while reindexing partitioned table "public.concur_reindex_part"
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part 
+-------------------------
+ 
+(1 row)
+
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+            relname            | relkind |          state           
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index     | I       | relfilenode is unchanged
+ concur_reindex_part_index_0   | I       | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i       | relfilenode has changed
+ concur_reindex_part_index_0_2 | i       | relfilenode has changed
+ concur_reindex_part_index_10  | I       | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 -- Check errors
 -- Cannot run inside a transaction block
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f3667bacdc..37f7259da9 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -903,12 +903,6 @@ CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2 (c1);
 ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_part_index_0_2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
 -- REINDEX should preserve dependencies of partition tree.
@@ -948,6 +942,88 @@ WHERE classid = 'pg_class'::regclass AND
   ORDER BY 1, 2;
 SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
   ORDER BY relid, level;
+
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index; -- error
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index; -- error
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10; -- error
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10; -- error
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+  RETURNS VOID AS
+  $func$
+  BEGIN
+  EXECUTE format('
+    CREATE TABLE %I AS
+      SELECT oid, relname, relfilenode, relkind, reltoastrelid
+      FROM pg_class
+      WHERE oid IN
+         (SELECT relid FROM pg_partition_tree(''%I''));',
+	 relname, indname);
+  END
+  $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+  RETURNS TABLE (relname name, relkind "char", state text) AS
+  $func$
+  BEGIN
+    RETURN QUERY EXECUTE
+      format(
+        'SELECT  b.relname,
+                 b.relkind,
+                 CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+                 ELSE ''relfilenode has changed'' END
+           -- Do not join with OID here as CONCURRENTLY changes it.
+           FROM %I b JOIN pg_class a ON b.relname = a.relname
+           ORDER BY 1;', tabname);
+  END
+  $func$ LANGUAGE plpgsql;
+--  Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part; -- error
+REINDEX INDEX CONCURRENTLY concur_reindex_part; -- error
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10; -- error
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10; -- error
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+
+-- Cleanup of partition tree used for REINDEX test.
 DROP TABLE concur_reindex_part;
 
 -- Check errors
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index c16f223e4e..33af4ae02a 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -88,7 +88,9 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
     <term><literal>INDEX</literal></term>
     <listitem>
      <para>
-      Recreate the specified index.
+      Recreate the specified index. This form of <command>REINDEX</command>
+      cannot be executed inside a transaction block when used with a
+      partitioned index.
      </para>
     </listitem>
    </varlistentry>
@@ -99,6 +101,8 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
      <para>
       Recreate all indexes of the specified table.  If the table has a
       secondary <quote>TOAST</quote> table, that is reindexed as well.
+      This form of <command>REINDEX</command> cannot be executed inside a
+      transaction block when used with a partitioned table.
      </para>
     </listitem>
    </varlistentry>
@@ -259,8 +263,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
   </para>
 
   <para>
-   Reindexing partitioned tables or partitioned indexes is not supported.
-   Each individual partition can be reindexed separately instead.
+   Reindexing partitioned indexes or partitioned tables is supported
+   with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
+   respectively. Each partition of the specified partitioned relation is
+   reindexed in a separate transaction. Those commands cannot be used inside
+   a transaction block when working on a partitioned table or index.
   </para>
 
   <refsect2 id="sql-reindex-concurrently" xreflabel="Rebuilding Indexes Concurrently">
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 90db550b92..84bc0ee381 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -4044,6 +4044,27 @@ SELECT f_test(100);
 
 DROP FUNCTION f_test(int);
 -- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+ERROR:  "reindex_foreign" is not a table or materialized view
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+-- ===================================================================
 -- conversion error
 -- ===================================================================
 ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 83971665e3..d452d06343 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1081,6 +1081,26 @@ $$ LANGUAGE plpgsql;
 SELECT f_test(100);
 DROP FUNCTION f_test(int);
 
+-- ===================================================================
+-- REINDEX
+-- ===================================================================
+-- remote table is not created here
+CREATE FOREIGN TABLE reindex_foreign (c1 int, c2 int)
+  SERVER loopback2 OPTIONS (table_name 'reindex_local');
+REINDEX TABLE reindex_foreign; -- error
+REINDEX TABLE CONCURRENTLY reindex_foreign; -- error
+DROP FOREIGN TABLE reindex_foreign;
+-- partitions and foreign tables
+CREATE TABLE reind_fdw_parent (c1 int) PARTITION BY RANGE (c1);
+CREATE TABLE reind_fdw_0_10 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (0) TO (10);
+CREATE FOREIGN TABLE reind_fdw_10_20 PARTITION OF reind_fdw_parent
+  FOR VALUES FROM (10) TO (20)
+  SERVER loopback OPTIONS (table_name 'reind_local_10_20');
+REINDEX TABLE reind_fdw_parent; -- ok
+REINDEX TABLE CONCURRENTLY reind_fdw_parent; -- ok
+DROP TABLE reind_fdw_parent;
+
 -- ===================================================================
 -- conversion error
 -- ===================================================================
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 500623e230..c83821c0b5 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2037,6 +2037,7 @@ RegProcedure
 Regis
 RegisNode
 RegisteredBgWorker
+ReindexErrorInfo
 ReindexObjectType
 ReindexStmt
 ReindexType

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-07 14:23  Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
  parent: Michael Paquier <michael@paquier.xyz>
  0 siblings, 0 replies; 8+ messages in thread

From: Anastasia Lubennikova @ 2020-09-07 14:23 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On 07.09.2020 04:58, Michael Paquier wrote:
> On Fri, Sep 04, 2020 at 09:51:13AM +0900, Michael Paquier wrote:
>> Glad to hear that, please take the time you need.
> Attached is a rebased version to address the various conflicts after
> 844c05a.
> --
> Michael
>
Thank you.

With the fix for the cycle in reindex_partitions() and new comments 
added, I think this patch is ready for commit.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-08 02:39  Justin Pryzby <pryzby@telsasoft.com>
  parent: Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 8+ messages in thread

From: Justin Pryzby @ 2020-09-08 02:39 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; +Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

Thanks for completing and pushing the REINDEX patch and others.

Here's a rebasified + fixed version of the others.

On Tue, Sep 01, 2020 at 02:51:58PM +0900, Michael Paquier wrote:
> The REINDEX patch is progressing its way, so I have looked a bit at
> the part for CIC.
> 
> Visibly, the case of multiple partition layers is not handled
> correctly.  Here is a sequence that gets broken:
..
> This fails as follows:
> ERROR: XX000: unrecognized node type: 2139062143
> LOCATION:  copyObjectImpl, copyfuncs.c:5718

Because copyObject needed to be called within a longlived context.

Also, my previous revision failed to implement your suggestion to first build
catalog entries with INVALID indexes and to then reindex them.  Fixed.

-- 
Justin

Attachments: download all (2 files, .tar.gz)

v6-0001-Allow-CREATE-INDEX-CONCURRENTLY-on-partitioned-ta.patch (text/x-diff, 13.0K) download
From 17a14e5ad128024b05ae288a5096148b3f114c98 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 6 Jun 2020 17:42:23 -0500
Subject: [PATCH v6 1/2] Allow CREATE INDEX CONCURRENTLY on partitioned table

Note, this effectively reverts 050098b14, so take care to not reintroduce the
bug it fixed.
---
 doc/src/sgml/ref/create_index.sgml     |   9 --
 src/backend/commands/indexcmds.c       | 118 ++++++++++++++++++++-----
 src/test/regress/expected/indexing.out |  60 ++++++++++++-
 src/test/regress/sql/indexing.sql      |  18 +++-
 4 files changed, 165 insertions(+), 40 deletions(-)

diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 33aa64e81d..c780dc9547 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -657,15 +657,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 f1b5f87e6a..61d0c4914c 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -665,17 +665,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),
@@ -1110,6 +1099,11 @@ DefineIndex(Oid relationId,
 		if (pd->nparts != 0)
 			flags |= INDEX_CREATE_INVALID;
 	}
+	else if (concurrent && OidIsValid(parentIndexId))
+	{
+		/* If concurrent, initial build of index partitions as "invalid" */
+		flags |= INDEX_CREATE_INVALID;
+	}
 
 	if (stmt->deferrable)
 		constr_flags |= INDEX_CONSTR_CREATE_DEFERRABLE;
@@ -1162,6 +1156,14 @@ DefineIndex(Oid relationId,
 		 */
 		if (!stmt->relation || stmt->relation->inh)
 		{
+			/*
+			 * 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);
 			PartitionDesc partdesc = RelationGetPartitionDesc(rel);
 			int			nparts = partdesc->nparts;
 			Oid		   *part_oids = palloc(sizeof(Oid) * nparts);
@@ -1173,8 +1175,10 @@ DefineIndex(Oid relationId,
 										 nparts);
 
 			memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
+			parentDesc = CreateTupleDescCopy(RelationGetDescr(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]);
@@ -1217,10 +1221,12 @@ DefineIndex(Oid relationId,
 					continue;
 				}
 
+				oldcontext = MemoryContextSwitchTo(ind_context);
 				childidxs = RelationGetIndexList(childrel);
 				attmap =
 					build_attrmap_by_name(RelationGetDescr(childrel),
 										  parentDesc);
+				MemoryContextSwitchTo(oldcontext);
 
 				foreach(cell, childidxs)
 				{
@@ -1291,10 +1297,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
@@ -1306,8 +1316,8 @@ DefineIndex(Oid relationId,
 					childStmt->relation = NULL;
 					childStmt->indexOid = InvalidOid;
 					childStmt->oldNode = InvalidOid;
-					childStmt->oldCreateSubid = InvalidSubTransactionId;
-					childStmt->oldFirstRelfilenodeSubid = InvalidSubTransactionId;
+					// childStmt->oldCreateSubid = InvalidSubTransactionId;
+					// childStmt->oldFirstRelfilenodeSubid = InvalidSubTransactionId;
 
 					/*
 					 * Adjust any Vars (both in expressions and in the index's
@@ -1346,10 +1356,18 @@ 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);
 			}
 
@@ -1376,34 +1394,86 @@ DefineIndex(Oid relationId,
 				table_close(pg_index, RowExclusiveLock);
 				heap_freetuple(newtup);
 			}
+		} else
+			table_close(rel, NoLock);
+
+		if (concurrent)
+		{
+			List	*childs;
+			ListCell *lc;
+			int		npart = 0;
+
+			/* Reindex invalid child indexes created earlier */
+			MemoryContext	ind_context = AllocSetContextCreate(PortalContext, "CREATE INDEX",
+					ALLOCSET_DEFAULT_SIZES);
+			MemoryContext	oldcontext = MemoryContextSwitchTo(ind_context);
+			childs = find_inheritance_children(indexRelationId, NoLock);
+			MemoryContextSwitchTo(oldcontext);
+
+			/* Make the catalog changes visible to get_partition_parent */
+			PopActiveSnapshot();
+			CommandCounterIncrement();
+
+			foreach (lc, childs)
+			{
+				Oid			indexrelid = lfirst_oid(lc);
+
+				if (!OidIsValid(parentIndexId))
+					pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+												 npart++);
+
+				CommandCounterIncrement();
+				PushActiveSnapshot(GetTransactionSnapshot());
+
+				if (get_index_isvalid(indexrelid))
+				{
+					PopActiveSnapshot();
+					continue;
+				}
+
+				/* This may be a partitioned index, which is fine too */
+				ReindexRelationConcurrently(indexrelid, 0);
+			}
+
+			/*
+			 * 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).
+			 */
+			CommitTransactionCommand();
+			StartTransactionCommand();
+			PushActiveSnapshot(GetTransactionSnapshot());
+			index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+			CommandCounterIncrement();
+			index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
 		}
 
 		/*
 		 * Indexes on partitioned tables are not themselves built, so we're
 		 * done here.
 		 */
-		table_close(rel, NoLock);
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 		return address;
 	}
 
-	if (!concurrent)
+	table_close(rel, NoLock);
+	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. */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
 
 		return address;
 	}
 
-	/* save lockrelid and locktag for below, then close rel */
+	/* save lockrelid and locktag for below */
 	heaprelid = rel->rd_lockInfo.lockRelId;
 	SET_LOCKTAG_RELATION(heaplocktag, heaprelid.dbId, heaprelid.relId);
-	table_close(rel, NoLock);
 
 	/*
 	 * For a concurrent build, it's important to make the catalog entries
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 7e78a07af8..f6eba3dd40 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)
+    "idxpart1_a_idx1" btree (a)
+    "idxpart1_a_idx2" UNIQUE, btree (a)
+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/20180124162006.pmapfiznhgngwtjf@alvherre.pgsql
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index 42f398b67c..3d4b6e9bc9 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.17.0

v6-0002-Implement-CLUSTER-of-partitioned-table.patch (text/x-diff, 10.3K) download
From c9c4de2549dff93bf16aaa70ed8342aa338d3dfe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 7 Jun 2020 16:58:42 -0500
Subject: [PATCH v6 2/2] Implement CLUSTER of partitioned table..

This requires either specification of a partitioned index on which to cluster,
or that an partitioned index was previously set clustered.

XXX: should mark_index_clustered() SET WITHOUT CLUSTER for any parent indexes
of an index partition which were previously-clustered ?
---
 src/backend/commands/cluster.c        | 141 +++++++++++++++++++-------
 src/bin/psql/tab-complete.c           |   1 +
 src/test/regress/expected/cluster.out |  23 ++++-
 src/test/regress/sql/cluster.sql      |  12 ++-
 4 files changed, 134 insertions(+), 43 deletions(-)

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0d647e912c..6ead8ffc79 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -33,6 +33,7 @@
 #include "catalog/namespace.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_am.h"
+#include "catalog/pg_inherits.h"
 #include "catalog/toasting.h"
 #include "commands/cluster.h"
 #include "commands/progress.h"
@@ -75,6 +76,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
 							TransactionId *pFreezeXid,
 							MultiXactId *pCutoffMulti);
 static List *get_tables_to_cluster(MemoryContext cluster_context);
+static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
+		Oid indexOid);
+static void cluster_multiple_rels(List *rvs, bool isTopLevel, int options);
 
 
 /*---------------------------------------------------------------------------
@@ -127,14 +131,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
 					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 					 errmsg("cannot cluster temporary tables of other sessions")));
 
-		/*
-		 * Reject clustering a partitioned table.
-		 */
-		if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot cluster a partitioned table")));
-
 		if (stmt->indexname == NULL)
 		{
 			ListCell   *index;
@@ -172,8 +168,37 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
 		/* close relation, keep lock till commit */
 		table_close(rel, NoLock);
 
-		/* Do the job. */
-		cluster_rel(tableOid, indexOid, stmt->options, isTopLevel);
+		if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+		{
+			/* Do the job. */
+			cluster_rel(tableOid, indexOid, stmt->options, isTopLevel);
+		} else {
+			List	   *rvs;
+			MemoryContext cluster_context;
+
+			/* Check index directly since cluster_rel isn't called for partitioned table */
+			check_index_is_clusterable(rel, indexOid, true, AccessExclusiveLock);
+
+			/* Refuse to hold strong locks in a user transaction */
+			PreventInTransactionBlock(isTopLevel, "CLUSTER");
+
+			cluster_context = AllocSetContextCreate(PortalContext,
+												"Cluster",
+												ALLOCSET_DEFAULT_SIZES);
+
+			rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
+			cluster_multiple_rels(rvs, isTopLevel, stmt->options);
+
+			/* Start a new transaction for the cleanup work. */
+			StartTransactionCommand();
+
+			rel = table_open(tableOid, ShareUpdateExclusiveLock);
+			mark_index_clustered(rel, indexOid, true);
+			table_close(rel, NoLock);
+
+			/* Clean up working storage */
+			MemoryContextDelete(cluster_context);
+		}
 	}
 	else
 	{
@@ -183,7 +208,6 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
 		 */
 		MemoryContext cluster_context;
 		List	   *rvs;
-		ListCell   *rv;
 
 		/*
 		 * We cannot run this form of CLUSTER inside a user transaction block;
@@ -207,26 +231,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
 		 */
 		rvs = get_tables_to_cluster(cluster_context);
 
-		/* Commit to get out of starting transaction */
-		PopActiveSnapshot();
-		CommitTransactionCommand();
-
-		/* Ok, now that we've got them all, cluster them one by one */
-		foreach(rv, rvs)
-		{
-			RelToCluster *rvtc = (RelToCluster *) lfirst(rv);
-
-			/* Start a new transaction for each relation. */
-			StartTransactionCommand();
-			/* functions in indexes may want a snapshot set */
-			PushActiveSnapshot(GetTransactionSnapshot());
-			/* Do the job. */
-			cluster_rel(rvtc->tableOid, rvtc->indexOid,
-						stmt->options | CLUOPT_RECHECK,
-						isTopLevel);
-			PopActiveSnapshot();
-			CommitTransactionCommand();
-		}
+		cluster_multiple_rels(rvs, isTopLevel, stmt->options);
 
 		/* Start a new transaction for the cleanup work. */
 		StartTransactionCommand();
@@ -487,12 +492,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
 	Relation	pg_index;
 	ListCell   *index;
 
-	/* Disallow applying to a partitioned table */
-	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("cannot mark index clustered in partitioned table")));
-
 	/*
 	 * If the index is already marked clustered, no need to do anything.
 	 */
@@ -1563,3 +1562,71 @@ get_tables_to_cluster(MemoryContext cluster_context)
 
 	return rvs;
 }
+
+/*
+ * Return a List of tables and associated index, where each index is a
+ * partition of the given index
+ */
+static List *
+get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
+{
+	List		*inhoids;
+	ListCell	*lc;
+	List		*rvs = NIL;
+
+	MemoryContext old_context = MemoryContextSwitchTo(cluster_context);
+
+	inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+	foreach(lc, inhoids)
+	{
+		Oid		indexrelid = lfirst_oid(lc);
+		Oid		relid = IndexGetRelation(indexrelid, false);
+		RelToCluster	*rvtc;
+
+		/*
+		 * We have a full list of direct and indirect children, so skip
+		 * partitioned tables and just handle their children.
+		 */
+		if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
+			continue;
+
+		rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
+		rvtc->tableOid = relid;
+		rvtc->indexOid = indexrelid;
+		rvs = lappend(rvs, rvtc);
+	}
+
+	MemoryContextSwitchTo(old_context);
+	return rvs;
+}
+
+/* Cluster each relation in a separate transaction */
+static void
+cluster_multiple_rels(List *rvs, bool isTopLevel, int options)
+{
+		ListCell *lc;
+
+		/* Commit to get out of starting transaction */
+		PopActiveSnapshot();
+		CommitTransactionCommand();
+
+		/* Ok, now that we've got them all, cluster them one by one */
+		foreach(lc, rvs)
+		{
+			RelToCluster *rvtc = (RelToCluster *) lfirst(lc);
+
+			/* Start a new transaction for each relation. */
+			StartTransactionCommand();
+
+			/* functions in indexes may want a snapshot set */
+			PushActiveSnapshot(GetTransactionSnapshot());
+
+			/* Do the job. */
+			cluster_rel(rvtc->tableOid, rvtc->indexOid,
+						options | CLUOPT_RECHECK,
+						isTopLevel);
+
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index f41785f11c..49a2648991 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -587,6 +587,7 @@ static const SchemaQuery Query_for_list_of_clusterables = {
 	.catname = "pg_catalog.pg_class c",
 	.selcondition =
 	"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+	CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
 	CppAsString2(RELKIND_MATVIEW) ")",
 	.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
 	.namespace = "c.relnamespace",
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..21b28fde6e 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -439,13 +439,28 @@ select * from clstr_temp;
 
 drop table clstr_temp;
 RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
 CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
 CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
-ERROR:  cannot mark index clustered in partitioned table
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
 CLUSTER clstrpart USING clstrpart_idx;
-ERROR:  cannot cluster a partitioned table
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
+       Partitioned table "public.clstrpart"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+Partition key: RANGE (a)
+Indexes:
+    "clstrpart_idx" btree (a) CLUSTER
+Number of partitions: 2 (Use \d+ to list them.)
+
 DROP TABLE clstrpart;
 -- Test CLUSTER with external tuplesorting
 create table clstr_4 as select * from tenk1;
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..4a76848213 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -196,11 +196,19 @@ drop table clstr_temp;
 
 RESET SESSION AUTHORIZATION;
 
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
 CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
 CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
 CLUSTER clstrpart USING clstrpart_idx;
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
 DROP TABLE clstrpart;
 
 -- Test CLUSTER with external tuplesorting
-- 
2.17.0

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: 回复：how to create index concurrently on partitioned table
@ 2020-09-08 04:31  Michael Paquier <michael@paquier.xyz>
  parent: Justin Pryzby <pryzby@telsasoft.com>
  0 siblings, 0 replies; 8+ messages in thread

From: Michael Paquier @ 2020-09-08 04:31 UTC (permalink / raw)
  To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: 李杰(慎追) <adger.lj@alibaba-inc.com>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>; 曾文旌(义从) <wenjing.zwj@alibaba-inc.com>; Alvaro Herrera <alvherre@2ndquadrant.com>

On Mon, Sep 07, 2020 at 09:39:16PM -0500, Justin Pryzby wrote:
> Also, my previous revision failed to implement your suggestion to first build
> catalog entries with INVALID indexes and to then reindex them.  Fixed.

-   childStmt->oldCreateSubid = InvalidSubTransactionId;
-   childStmt->oldFirstRelfilenodeSubid = childStmt->InvalidSubTransactionId;
+   // childStmt->oldCreateSubid = childStmt->InvalidSubTransactionId;
+   // childStmt->oldFirstRelfilenodeSubid = InvalidSubTransactionId;
In the CIC patch, what is that about?  It is hard to follow what you
are trying to achieve here.

+           index_set_state_flags(indexRelationId, INDEX_CREATE_SET_READY);
+           CommandCounterIncrement();
+           index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
Anyway, this part of the logic is still not good here.  If we fail in
the middle here, we may run into problems with a single partition
index that has only a portion of its flags set.  As this gets called
for each partitioned index, it also means that we could still finish
in a state where we have only a partially-valid partition tree.  For
example, imagine a partition tree with 2 levels (as reported by
pg_partition_tree), then the following happens if an index is created
concurrently from the partitioned table of level 0:
1) indexes are created in level 2 first
2) partitioned table of level 1 is switched to be ready and valid
3) indexes of level 1 are created.
4) partitioned table of level 0 is switched to be ready and valid
If the command has a failure, say between 2 and 3, we would have as
result a command that has partially succeeded in creating a partition
tree, while the user was looking for an index for the full tree.  This
comes back to my previous points, where we should make
index_set_state_flags() first, and I have begun a separate thread
about that:
https://commitfest.postgresql.org/30/2725/

Second, it also means that the patch should really switch all the
indexes to be valid in one single transaction, and that we also need
more careful refactoring of DefineIndex().

I also had a quick look at the patch for CLUSTER, and it does a much
better job, still it has issues.

+   MemoryContext old_context = MemoryContextSwitchTo(cluster_context);
+
+   inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+   foreach(lc, inhoids)
The area where a private memory context is used should be minimized.
In this case, you just need to hold the context while saving the
relation and clustered index information in the list of RelToCluster
items.  As a whole, this case is more simple than CIC, so I'd like to
think that it would be good to work on that as next target.

Coming to my last point..  This thread has dealt since the beginning
with three different problems:
- REINDEX on partitioned relations.
- CLUSTER on partitioned relations.
- CIC on partitioned relations.  (Should we also care about DROP INDEX
CONCURRENTLY as well?)

The first problem has been solved, not the two others yet.  Do you
think that it could be possible to begin two new separate threads for
the remaining issues, with dedicated CF entries?  We could also mark
the existing one as committed, retitled for REINDEX as a matter of
clarity.  Also, please note that I am not sure if I will be able to
look more at this thread in this CF.
--
Michael

Attachments:

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 8+ messages in thread

end of thread, other threads:[~2020-09-08 04:31 UTC | newest]

Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 05:51 ` Michael Paquier <michael@paquier.xyz>
2020-09-08 02:39   ` Justin Pryzby <pryzby@telsasoft.com>
2020-09-08 04:31     ` Michael Paquier <michael@paquier.xyz>
2020-09-02 01:39 ` Michael Paquier <michael@paquier.xyz>
2020-09-03 19:02   ` Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
2020-09-04 00:51     ` Michael Paquier <michael@paquier.xyz>
2020-09-07 01:58       ` Michael Paquier <michael@paquier.xyz>
2020-09-07 14:23         ` Anastasia Lubennikova <a.lubennikova@postgrespro.ru>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox