From: Tom Lane Date: Sat, 25 Mar 2023 19:33:56 +0000 (-0400) Subject: Fix CREATE INDEX progress reporting for multi-level partitioning. X-Git-Tag: REL_16_BETA1~428 X-Git-Url: http://git.postgresql.org/gitweb/?a=commitdiff_plain;h=27f5c712b2c57d1c676fbf110705ac881057b57e;p=postgresql.git 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 --- diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 21e6ce2841e..c809ff1ba4a 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -6817,7 +6817,7 @@ FROM pg_stat_get_backend_idset() AS backendid; pid integer - Process ID of backend. + Process ID of the backend creating indexes. @@ -6863,7 +6863,7 @@ FROM pg_stat_get_backend_idset() AS backendid; command text - The command that is running: CREATE INDEX, + Specific command type: CREATE INDEX, CREATE INDEX CONCURRENTLY, REINDEX, or REINDEX CONCURRENTLY. @@ -6946,9 +6946,10 @@ FROM pg_stat_get_backend_idset() AS backendid; partitions_total bigint - 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 0 during a REINDEX. + Total number of partitions on which the index is to be created + or attached, including both direct and indirect partitions. + 0 during a REINDEX, or when + the index is not partitioned. @@ -6957,9 +6958,10 @@ FROM pg_stat_get_backend_idset() AS backendid; partitions_done bigint - 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 0 during a REINDEX. + Number of partitions on which the index has already been created + or attached, including both direct and indirect partitions. + 0 during a REINDEX, or when + the index is not partitioned. diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index 86804bb598e..81a1b7bfec3 100644 --- a/src/backend/bootstrap/bootparse.y +++ b/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, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index ff48f44c66f..3ec8b5cca6c 100644 --- a/src/backend/commands/indexcmds.c +++ b/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; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 3e2c5f797cd..9a877f90d36 100644 --- a/src/backend/commands/tablecmds.c +++ b/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); } diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index eada7353639..30b51bf4d30 100644 --- a/src/backend/tcop/utility.c +++ b/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 */ diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c index d96af812b19..fb48eafef9a 100644 --- a/src/backend/utils/activity/backend_progress.c +++ b/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() - * diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h index 4f7f87fc62c..478203ed4c4 100644 --- a/src/include/commands/defrem.h +++ b/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, diff --git a/src/include/utils/backend_progress.h b/src/include/utils/backend_progress.h index 005e5d75ab6..a84752ade99 100644 --- a/src/include/utils/backend_progress.h +++ b/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);