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
