On Sun, 2023-01-08 at 10:48 -0600, Justin Pryzby wrote: > On Sat, Dec 17, 2022 at 08:30:02AM -0600, Justin Pryzby wrote: > > We have the common problem of too many patches. > > > > https://www.postgresql.org/message-id/a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel%40gmail.com > > This changes the progress reporting to show indirect children as > > "total", and adds a global variable to track recursion into > > DefineIndex(), allowing it to be incremented without the value > > being > > lost to the caller. > > > > https://www.postgresql.org/message-id/20221211063334.GB27893%40telsasoft.com > > This also counts indirect children, but only increments the > > progress > > reporting in the parent. This has the disadvantage that when > > intermediate partitions are in use, the done_partitions counter > > will > > "jump" from (say) 20 to 30 without ever hitting 21-29. > > > > https://www.postgresql.org/message-id/20221213044331.GJ27893%40telsasoft.com > > This has two alternate patches: > > - One patch changes to only update progress reporting of *direct* > > children. This is minimal, but discourages any future plan to > > track > > progress involving intermediate partitions with finer > > granularity. > > - A alternative patch adds IndexStmt.nparts_done, and allows > > reporting > > fine-grained progress involving intermediate partitions. > > > > https://www.postgresql.org/message-id/flat/039564d234fc3d014c555a7ee98be69a9e724836.ca...@gmail.com > > This also reports progress of intermediate children. The first > > patch > > does it by adding an argument to DefineIndex() (which isn't okay to > > backpatch). And an alternate patch does it by adding to IndexStmt. > > > > @committers: Is it okay to add nparts_done to IndexStmt ? > > Any hint about this ? > > This should be resolved before the "CIC on partitioned tables" patch, > which I think is otherwise done.
I suggest that we move on with the IndexStmt patch and see what the committers have to say about it. I have brushed the patch up a bit, fixing TODOs and adding docs as per our discussion above.
From 490d8afa7cb952e5b3947d81d85bf9690499e8a3 Mon Sep 17 00:00:00 2001 From: Ilya Gladyshev <ilya.v.gladys...@gmail.com> Date: Tue, 13 Dec 2022 22:04:45 +0400 Subject: [PATCH v2] parts_done via IndexStmt --- doc/src/sgml/monitoring.sgml | 4 +-- src/backend/bootstrap/bootparse.y | 2 ++ src/backend/commands/indexcmds.c | 57 +++++++++++++++++++++++++++--- src/backend/parser/parse_utilcmd.c | 2 ++ src/include/nodes/parsenodes.h | 1 + 5 files changed, 59 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index cf220c3bcb..2c32a92364 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -6565,7 +6565,7 @@ FROM pg_stat_get_backend_idset() AS backendid; </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. + the total number of leaf partitions on which the index is to be created or attached. This field is <literal>0</literal> during a <literal>REINDEX</literal>. </para></entry> </row> @@ -6576,7 +6576,7 @@ FROM pg_stat_get_backend_idset() AS backendid; </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. + the number of leaf partitions on which the index has been created or attached. This field is <literal>0</literal> during a <literal>REINDEX</literal>. </para></entry> </row> diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y index 86804bb598..dad9c38d90 100644 --- a/src/backend/bootstrap/bootparse.y +++ b/src/backend/bootstrap/bootparse.y @@ -296,6 +296,7 @@ Boot_DeclareIndexStmt: stmt->concurrent = false; stmt->if_not_exists = false; stmt->reset_default_tblspc = false; + stmt->parts_done = -1; /* locks and races need not concern us in bootstrap mode */ relationId = RangeVarGetRelid(stmt->relation, NoLock, @@ -348,6 +349,7 @@ Boot_DeclareUniqueIndexStmt: stmt->concurrent = false; stmt->if_not_exists = false; stmt->reset_default_tblspc = false; + stmt->parts_done = -1; /* locks and races need not concern us in bootstrap mode */ relationId = RangeVarGetRelid(stmt->relation, NoLock, diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 8afc006f89..be4d1c884e 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -130,6 +130,29 @@ typedef struct ReindexErrorInfo char relkind; } ReindexErrorInfo; + +/* + * Count the number of direct and indirect leaf partitions, excluding foreign + * tables. + */ +static int +num_leaf_partitions(Oid relid) +{ + int nleaves = 0; + List *childs = find_all_inheritors(relid, NoLock, NULL); + ListCell *lc; + + foreach(lc, childs) + { + Oid partrelid = lfirst_oid(lc); + if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid))) + nleaves++; + } + + list_free(childs); + return nleaves; +} + /* * CheckIndexCompatible * Determine whether an existing index definition is compatible with a @@ -1219,8 +1242,16 @@ DefineIndex(Oid relationId, Relation parentIndex; TupleDesc parentDesc; - pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL, - nparts); + if (!OidIsValid(parentIndexId)) + { + int total_parts; + + /* Init counter of done partitions when processing root index */ + stmt->parts_done = 0; + total_parts = num_leaf_partitions(relationId); + 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); @@ -1430,10 +1461,20 @@ DefineIndex(Oid relationId, skip_build, quiet); SetUserIdAndSecContext(child_save_userid, child_save_sec_context); + /* childStmt has more recent data on done parts, update the parent */ + stmt->parts_done = childStmt->parts_done; + } + else + { + /* + * If the index has been attached, count all of its leaf indexes as attached, + * because PROGRESS_CREATEIDX_PARTITIONS_TOTAL includes them all initially. + */ + stmt->parts_done += num_leaf_partitions(childRelid); + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + stmt->parts_done); } - pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, - i + 1); free_attrmap(attmap); } @@ -1484,9 +1525,15 @@ 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, we're done, otherwise, increment + * done partition counter, if the counter is initialized. + */ if (!OidIsValid(parentIndexId)) pgstat_progress_end_command(); + else if (stmt->parts_done >= 0) + pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE, + ++stmt->parts_done); return address; } diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index bffa9f8dd0..1a286694da 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1599,6 +1599,7 @@ generateClonedIndexStmt(RangeVar *heapRel, Relation source_idx, index->concurrent = false; index->if_not_exists = false; index->reset_default_tblspc = false; + index->parts_done = -1; /* * We don't try to preserve the name of the source index; instead, just @@ -2217,6 +2218,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt) index->concurrent = false; index->if_not_exists = false; index->reset_default_tblspc = constraint->reset_default_tblspc; + index->parts_done = -1; /* * If it's ALTER TABLE ADD CONSTRAINT USING INDEX, look up the index and diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index cfeca96d53..2191d28e4a 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3004,6 +3004,7 @@ typedef struct IndexStmt bool if_not_exists; /* just do nothing if index already exists? */ bool reset_default_tblspc; /* reset default_tablespace prior to * executing */ + int parts_done; /* counter of created/attached indexes for a partitioned index */ } IndexStmt; /* ---------------------- -- 2.30.2