From 3d78449f1c3d2fef8e96f4874f13004325026305 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Wed, 1 Feb 2023 21:50:21 -0600 Subject: [PATCH 2/4] s!fix CREATE INDEX progress report with nested partitions The progress reporting was added in v12 (ab0dfc961) but the original patch didn't seem to consider the possibility of nested partitioning. When called recursively, DefineIndex() would clobber the number of completed partitions, and it was possible to end up with the TOTAL counter greater than the DONE counter. This clarifies/re-defines that the progress reporting counts both direct and indirect children, but doesn't count intermediate partitioned tables: - The TOTAL counter is set once at the start of the command. - For indexes which are newly-built, the recursively-called DefineIndex() increments the DONE counter. - For pre-existing indexes which are ATTACHed rather than built, DefineIndex() increments the DONE counter, and if the attached index is partitioned, the counter is incremented to account for each of its leaf partitions. Author: Ilya Gladyshev Reviewed-By: Justin Pryzby, Tomas Vondra, Dean Rasheed, Alvaro Herrera, Matthias van de Meent Discussion: https://www.postgresql.org/message-id/flat/a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel%40gmail.com --- doc/src/sgml/monitoring.sgml | 10 +++++-- src/backend/commands/indexcmds.c | 30 +++++++++++-------- src/backend/utils/activity/backend_progress.c | 13 ++++---- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index a911900271c..fa139dcece7 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -6601,7 +6601,10 @@ FROM pg_stat_get_backend_idset() AS backendid; When creating an index on a partitioned table, this column is set to - the total number of leaf partitions on which the index is to be created or attached. + the total number of partitions on which the index is to be created or attached. + In the case of intermediate partitioned tables, this includes both + direct and indirect partitions, but excludes the intermediate + partitioned tables themselves. This field is 0 during a REINDEX. @@ -6612,7 +6615,10 @@ FROM pg_stat_get_backend_idset() AS backendid; When creating an index on a partitioned table, this column is set to - the number of leaf partitions on which the index has been created or attached. + the number of partitions on which the index has been created or attached. + In the case of intermediate partitioned tables, this includes both + direct and indirect partitions, but excludes the intermediate + partitioned tables themselves. This field is 0 during a REINDEX. diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 936b4e3c1db..84c84c41acc 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -144,7 +144,7 @@ count_leaf_partitions(Oid relid) foreach(lc, childs) { - Oid partrelid = lfirst_oid(lc); + Oid partrelid = lfirst_oid(lc); if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid))) nleaves++; @@ -1243,9 +1243,13 @@ DefineIndex(Oid relationId, Relation parentIndex; TupleDesc parentDesc; + /* + * Set the total number of partitions at the start of the command, + * but don't change it when being called recursively. + */ if (!OidIsValid(parentIndexId)) { - int total_parts; + int total_parts; total_parts = count_leaf_partitions(relationId); pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, @@ -1465,19 +1469,21 @@ DefineIndex(Oid relationId, } else { - int attached_parts = 1; + int attached_parts; - if (RELKIND_HAS_PARTITIONS(child_relkind)) - attached_parts = count_leaf_partitions(childRelid); + /* + * Avoid the overhead of counting partitions when that + * can't apply. + */ + attached_parts = RELKIND_HAS_PARTITIONS(child_relkind) ? + count_leaf_partitions(childRelid) : 1; /* - * If the index was attached, we need to update progress - * here, in its parent. For a partitioned index, we need - * to mark all of its children that were included in - * PROGRESS_CREATEIDX_PARTITIONS_TOTAL as done. If the - * index was built by calling DefineIndex() recursively, - * the called function is responsible for updating the - * progress report for built indexes. + * If a pre-existing index was attached, the progress + * report is updated here. If the index was partitioned, + * all the children that were counted towards + * PROGRESS_CREATEIDX_PARTITIONS_TOTAL are counted as + * done. */ pgstat_progress_incr_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, attached_parts); } diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c index 45f7e7144b4..2a9994b98fd 100644 --- a/src/backend/utils/activity/backend_progress.c +++ b/src/backend/utils/activity/backend_progress.c @@ -61,12 +61,14 @@ pgstat_progress_update_param(int index, int64 val) /*----------- * pgstat_progress_incr_param() - * - * Increment index'th member in st_progress_param[] of own backend entry. + * Increment index'th member in st_progress_param[] of the current backend. *----------- */ -void pgstat_progress_incr_param(int index, int64 incr) { +void +pgstat_progress_incr_param(int index, int64 incr) +{ volatile PgBackendStatus *beentry = MyBEEntry; - int64 val; + int64 val; Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM); @@ -74,8 +76,9 @@ void pgstat_progress_incr_param(int index, int64 incr) { return; /* - * Because current backend is the only process that writes to its own status, - * we don't need to do the looping to read the value. + * Because no other process should write to this backend's own status, we + * can read its value from shared memory without needing to loop to ensure + * its consistency. */ val = beentry->st_progress_param[index]; val += incr; -- 2.25.1