From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
Date: Mon, 09 Jan 2023 12:44:22 +0400
Message-ID: <865a4ff239b6824fd5bbdd4a972390d1e95144b4.camel@gmail.com> (raw)
In-Reply-To: <20230108164849.GA21731@telsasoft.com>
References: <039564d234fc3d014c555a7ee98be69a9e724836.camel@gmail.com>
<20221217143002.GR1153@telsasoft.com>
<20230108164849.GA21731@telsasoft.com>
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.camel@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.
Attachments:v2-0001-parts_done-via-IndexStmt.patch (text/x-patch, 6.5K) download
From 490d8afa7cb952e5b3947d81d85bf9690499e8a3 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@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.sgmlindex 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.yindex 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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilya.v.gladyshev@gmail.com, pryzby@telsasoft.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
In-Reply-To: <865a4ff239b6824fd5bbdd4a972390d1e95144b4.camel@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox