Fix CREATE INDEX progress reporting for multi-level partitioning.

The "partitions_total" and "partitions_done" fields were updated
as though the current level of partitioning was the only one.
In multi-level cases, not only could partitions_total change
over the course of the command, but partitions_done could go
backwards or exceed the currently-reported partitions_total.

Fix by setting partitions_total to the total number of direct
and indirect children once at command start, and then just
incrementing partitions_done at appropriate points.  Invent
a new progress monitoring function "pgstat_progress_incr_param"
to simplify doing the latter.  We can avoid adding cost for the
former when doing CREATE INDEX, because ProcessUtility already
enumerates the children and it's pretty easy to pass the count
down to DefineIndex.  In principle the same could be done in
ALTER TABLE, but that's structurally difficult; for now, just
eat the cost of an extra find_all_inheritors scan in that case.

Ilya Gladyshev and Justin Pryzby

Discussion: https://postgr.es/m/a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel@gmail.com

Author: tglsfdc

doc/src/sgml/monitoring.sgml

@@ -6817,7 +6817,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
        <structfield>pid</structfield> <type>integer</type>
       </para>
       <para>
-       Process ID of backend.
+       Process ID of the backend creating indexes.
       </para></entry>
      </row>
 
@@ -6863,7 +6863,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
        <structfield>command</structfield> <type>text</type>
       </para>
       <para>
-       The command that is running: <literal>CREATE INDEX</literal>,
+       Specific command type: <literal>CREATE INDEX</literal>,
        <literal>CREATE INDEX CONCURRENTLY</literal>,
        <literal>REINDEX</literal>, or <literal>REINDEX CONCURRENTLY</literal>.
       </para></entry>
@@ -6946,9 +6946,10 @@ FROM pg_stat_get_backend_idset() AS backendid;
        <structfield>partitions_total</structfield> <type>bigint</type>
       </para>
       <para>
-       When creating an index on a partitioned table, this column is set to
-       the total number of partitions on which the index is to be created.
-       This field is <literal>0</literal> during a <literal>REINDEX</literal>.
+       Total number of partitions on which the index is to be created
+       or attached, including both direct and indirect partitions.
+       <literal>0</literal> during a <literal>REINDEX</literal>, or when
+       the index is not partitioned.
       </para></entry>
      </row>
 
@@ -6957,9 +6958,10 @@ FROM pg_stat_get_backend_idset() AS backendid;
        <structfield>partitions_done</structfield> <type>bigint</type>
       </para>
       <para>
-       When creating an index on a partitioned table, this column is set to
-       the number of partitions on which the index has been created.
-       This field is <literal>0</literal> during a <literal>REINDEX</literal>.
+       Number of partitions on which the index has already been created
+       or attached, including both direct and indirect partitions.
+       <literal>0</literal> during a <literal>REINDEX</literal>, or when
+       the index is not partitioned.
       </para></entry>
      </row>
     </tbody>

src/backend/bootstrap/bootparse.y

@@ -306,6 +306,7 @@ Boot_DeclareIndexStmt:
 								$4,
 								InvalidOid,
 								InvalidOid,
+								-1,
 								false,
 								false,
 								false,
@@ -358,6 +359,7 @@ Boot_DeclareUniqueIndexStmt:
 								$5,
 								InvalidOid,
 								InvalidOid,
+								-1,
 								false,
 								false,
 								false,

src/backend/commands/indexcmds.c

@@ -513,6 +513,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
  *		of a partitioned index.
  * 'parentConstraintId': the OID of the parent constraint; InvalidOid if not
  *		the child of a constraint (only used when recursing)
+ * 'total_parts': total number of direct and indirect partitions of relation;
+ *		pass -1 if not known or rel is not partitioned.
  * 'is_alter_table': this is due to an ALTER rather than a CREATE operation.
  * 'check_rights': check for CREATE rights in namespace and tablespace.  (This
  *		should be true except when ALTER is deleting/recreating an index.)
@@ -530,6 +532,7 @@ DefineIndex(Oid relationId,
 			Oid indexRelationId,
 			Oid parentIndexId,
 			Oid parentConstraintId,
+			int total_parts,
 			bool is_alter_table,
 			bool check_rights,
 			bool check_not_in_use,
@@ -1225,8 +1228,37 @@ DefineIndex(Oid relationId,
 			Relation	parentIndex;
 			TupleDesc	parentDesc;
 
-			pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
-										 nparts);
+			/*
+			 * Report the total number of partitions at the start of the
+			 * command; don't update it when being called recursively.
+			 */
+			if (!OidIsValid(parentIndexId))
+			{
+				/*
+				 * When called by ProcessUtilitySlow, the number of partitions
+				 * is passed in as an optimization; but other callers pass -1
+				 * since they don't have the value handy.  This should count
+				 * partitions the same way, ie one less than the number of
+				 * relations find_all_inheritors reports.
+				 *
+				 * We assume we needn't ask find_all_inheritors to take locks,
+				 * because that should have happened already for all callers.
+				 * Even if it did not, this is safe as long as we don't try to
+				 * touch the partitions here; the worst consequence would be a
+				 * bogus progress-reporting total.
+				 */
+				if (total_parts < 0)
+				{
+					List	   *children = find_all_inheritors(relationId,
+															   NoLock, NULL);
+
+					total_parts = list_length(children) - 1;
+					list_free(children);
+				}
+
+				pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+											 total_parts);
+			}
 
 			/* Make a local copy of partdesc->oids[], just for safety */
 			memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1353,6 +1385,18 @@ DefineIndex(Oid relationId,
 							invalidate_parent = true;
 
 						found = true;
+
+						/*
+						 * Report this partition as processed.  Note that if
+						 * the partition has children itself, we'd ideally
+						 * count the children and update the progress report
+						 * for all of them; but that seems unduly expensive.
+						 * Instead, the progress report will act like all such
+						 * indirect children were processed in zero time at
+						 * the end of the command.
+						 */
+						pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+
 						/* keep lock till commit */
 						index_close(cldidx, NoLock);
 						break;
@@ -1432,14 +1476,13 @@ DefineIndex(Oid relationId,
 								InvalidOid, /* no predefined OID */
 								indexRelationId,	/* this is our child */
 								createdConstraintId,
+								-1,
 								is_alter_table, check_rights, check_not_in_use,
 								skip_build, quiet);
 					SetUserIdAndSecContext(child_save_userid,
 										   child_save_sec_context);
 				}
 
-				pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
-											 i + 1);
 				free_attrmap(attmap);
 			}
 
@@ -1479,6 +1522,12 @@ DefineIndex(Oid relationId,
 		table_close(rel, NoLock);
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
+		else
+		{
+			/* Update progress for an intermediate partitioned index itself */
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
+		}
+
 		return address;
 	}
 
@@ -1490,9 +1539,14 @@ DefineIndex(Oid relationId,
 		/* Close the heap and we're done, in the non-concurrent case */
 		table_close(rel, NoLock);
 
-		/* If this is the top-level index, we're done. */
+		/*
+		 * If this is the top-level index, the command is done overall;
+		 * otherwise, increment progress to report one child index is done.
+		 */
 		if (!OidIsValid(parentIndexId))
 			pgstat_progress_end_command();
+		else
+			pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, 1);
 
 		return address;
 	}

src/backend/commands/tablecmds.c

@@ -1216,6 +1216,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 						InvalidOid,
 						RelationGetRelid(idxRel),
 						constraintOid,
+						-1,
 						false, false, false, false, false);
 
 			index_close(idxRel, AccessShareLock);
@@ -8640,6 +8641,7 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
 						  InvalidOid,	/* no predefined OID */
 						  InvalidOid,	/* no parent index */
 						  InvalidOid,	/* no parent constraint */
+						  -1,	/* total_parts unknown */
 						  true, /* is_alter_table */
 						  check_rights,
 						  false,	/* check_not_in_use - we did it already */
@@ -18106,6 +18108,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation attachrel)
 			DefineIndex(RelationGetRelid(attachrel), stmt, InvalidOid,
 						RelationGetRelid(idxRel),
 						conOid,
+						-1,
 						true, false, false, false, false);
 		}
 

src/backend/tcop/utility.c

@@ -1464,6 +1464,7 @@ ProcessUtilitySlow(ParseState *pstate,
 					IndexStmt  *stmt = (IndexStmt *) parsetree;
 					Oid			relid;
 					LOCKMODE	lockmode;
+					int			nparts = -1;
 					bool		is_alter_table;
 
 					if (stmt->concurrent)
@@ -1494,7 +1495,9 @@ ProcessUtilitySlow(ParseState *pstate,
 					 *
 					 * We also take the opportunity to verify that all
 					 * partitions are something we can put an index on, to
-					 * avoid building some indexes only to fail later.
+					 * avoid building some indexes only to fail later.  While
+					 * at it, also count the partitions, so that DefineIndex
+					 * needn't do a duplicative find_all_inheritors search.
 					 */
 					if (stmt->relation->inh &&
 						get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE)
@@ -1505,7 +1508,8 @@ ProcessUtilitySlow(ParseState *pstate,
 						inheritors = find_all_inheritors(relid, lockmode, NULL);
 						foreach(lc, inheritors)
 						{
-							char		relkind = get_rel_relkind(lfirst_oid(lc));
+							Oid			partrelid = lfirst_oid(lc);
+							char		relkind = get_rel_relkind(partrelid);
 
 							if (relkind != RELKIND_RELATION &&
 								relkind != RELKIND_MATVIEW &&
@@ -1523,6 +1527,8 @@ ProcessUtilitySlow(ParseState *pstate,
 										 errdetail("Table \"%s\" contains partitions that are foreign tables.",
 												   stmt->relation->relname)));
 						}
+						/* count direct and indirect children, but not rel */
+						nparts = list_length(inheritors) - 1;
 						list_free(inheritors);
 					}
 
@@ -1548,6 +1554,7 @@ ProcessUtilitySlow(ParseState *pstate,
 									InvalidOid, /* no predefined OID */
 									InvalidOid, /* no parent index */
 									InvalidOid, /* no parent constraint */
+									nparts, /* # of partitions, or -1 */
 									is_alter_table,
 									true,	/* check_rights */
 									true,	/* check_not_in_use */

src/backend/utils/activity/backend_progress.c

@@ -58,6 +58,27 @@ pgstat_progress_update_param(int index, int64 val)
 	PGSTAT_END_WRITE_ACTIVITY(beentry);
 }
 
+/*-----------
+ * pgstat_progress_incr_param() -
+ *
+ * Increment index'th member in st_progress_param[] of own backend entry.
+ *-----------
+ */
+void
+pgstat_progress_incr_param(int index, int64 incr)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM);
+
+	if (!beentry || !pgstat_track_activities)
+		return;
+
+	PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
+	beentry->st_progress_param[index] += incr;
+	PGSTAT_END_WRITE_ACTIVITY(beentry);
+}
+
 /*-----------
  * pgstat_progress_update_multi_param() -
  *

src/include/commands/defrem.h

@@ -29,6 +29,7 @@ extern ObjectAddress DefineIndex(Oid relationId,
 								 Oid indexRelationId,
 								 Oid parentIndexId,
 								 Oid parentConstraintId,
+								 int total_parts,
 								 bool is_alter_table,
 								 bool check_rights,
 								 bool check_not_in_use,

src/include/utils/backend_progress.h

@@ -36,6 +36,7 @@ typedef enum ProgressCommandType
 extern void pgstat_progress_start_command(ProgressCommandType cmdtype,
 										  Oid relid);
 extern void pgstat_progress_update_param(int index, int64 val);
+extern void pgstat_progress_incr_param(int index, int64 incr);
 extern void pgstat_progress_update_multi_param(int nparam, const int *index,
 											   const int64 *val);
 extern void pgstat_progress_end_command(void);