On Mon, 2022-12-12 at 22:43 -0600, Justin Pryzby wrote:
> On Mon, Dec 12, 2022 at 11:39:23PM +0400, Ilya Gladyshev wrote:
> >
> > > Could you check what I've written as a counter-proposal ?
> >
> > I think that this might be a good solution to start with, it gives
> > us the opportunity to improve the granularity later without any
> > surprising changes for the end user. We could use this patch for
> > previous versions and make more granular output in the latest. What
> > do you think?
>
> Somehow, it hadn't occured to me that my patch "lost granularity" by
> incrementing the progress bar by more than one... Shoot.
>
> > I actually think that the progress view would be better off without
> > the total number of partitions,
>
> Just curious - why ?
We don't really know how many indexes we are going to create, unless we
have some kind of preliminary "planning" stage where we acumulate all
the relations that will need to have indexes created (rather than
attached). And if someone wants the total, it can be calculated
manually without this view, it's less user-friendly, but if we can't do
it well, I would leave it up to the user.
>
> > With this in mind, I think your proposal to exclude catalog-only
> > indexes sounds reasonable to me, but I feel like the docs are off
> > in this case, because the attached indexes are not created, but we
> > pretend like they are in this metric, so we should fix one or the
> > other.
>
> I agree that the docs should indicate whether we're counting "all
> partitions", "direct partitions", and whether or not that includes
> partitioned partitions, or just leaf partitions.
Agree. I think that docs should also be explicit about the attached
indexes, if we decide to count them in as "created".
> I have another proposal: since the original patch 3.5 years ago
> didn't
> consider or account for sub-partitions, let's not start counting them
> now. It was never defined whether they were included or not (and I
> guess that they're not common) so we can take this opportunity to
> clarify the definition.
I have had this thought initially, but then I thought that it's not
what I would want, if I was to track progress of multi-level
partitioned tables (but yeah, I guess it's pretty uncommon). In this
respect, I like your initial counter-proposal more, because it leaves
us room to improve this in the future. Otherwise, if we commit to
reporting only top-level partitions now, I'm not sure we will have the
opportunity to change this.
> Alternately, if it's okay to add nparts_done to the IndexStmt, then
> that's easy.
Yeah, or we could add another argument to DefineIndex. I don't know if
it's ok, or which option is better here in terms of compatibility and
interface-wise, so I have tried both of them, see the attached patches.
From 4f55526f8a534d6b2f27b6b17cadaee0370e4cbc Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladys...@gmail.com>
Date: Tue, 13 Dec 2022 19:42:53 +0400
Subject: [PATCH] parts_done via DefineIndex args
---
src/backend/commands/indexcmds.c | 52 +++++++++++++++++++++++++++-----
src/backend/commands/tablecmds.c | 7 +++--
src/backend/tcop/utility.c | 3 +-
src/include/commands/defrem.h | 3 +-
4 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b5b860c3ab..9fa9109b9e 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -129,6 +129,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
@@ -530,7 +553,8 @@ DefineIndex(Oid relationId,
bool check_rights,
bool check_not_in_use,
bool skip_build,
- bool quiet)
+ bool quiet,
+ int *parts_done)
{
bool concurrent;
char *indexRelationName;
@@ -568,6 +592,7 @@ DefineIndex(Oid relationId,
Oid root_save_userid;
int root_save_sec_context;
int root_save_nestlevel;
+ int root_parts_done;
root_save_nestlevel = NewGUCNestLevel();
@@ -1218,8 +1243,17 @@ 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 */
+ root_parts_done = 0;
+ parts_done = &root_parts_done;
+ 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);
@@ -1426,13 +1460,11 @@ DefineIndex(Oid relationId,
indexRelationId, /* this is our child */
createdConstraintId,
is_alter_table,
check_rights, check_not_in_use,
- skip_build,
quiet);
+ skip_build,
quiet, parts_done);
SetUserIdAndSecContext(child_save_userid,
child_save_sec_context);
}
-
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
-
i + 1);
free_attrmap(attmap);
}
@@ -1483,9 +1515,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 (!OidIsValid(parentIndexId))
pgstat_progress_end_command();
+ else if (parts_done != NULL)
+
pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+
++(*parts_done));
return address;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0b352a5fff..dde57faf44 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1216,7 +1216,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid
ownerId,
InvalidOid,
RelationGetRelid(idxRel),
constraintOid,
- false, false, false, false,
false);
+ false, false, false, false,
false, NULL);
index_close(idxRel, AccessShareLock);
}
@@ -8577,7 +8577,8 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
check_rights,
false, /*
check_not_in_use - we did it already */
skip_build,
- quiet);
+ quiet,
+ NULL);
/*
* If TryReuseIndex() stashed a relfilenumber for us, we used it for the
@@ -18076,7 +18077,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation
attachrel)
DefineIndex(RelationGetRelid(attachrel), stmt,
InvalidOid,
RelationGetRelid(idxRel),
conOid,
- true, false, false, false,
false);
+ true, false, false, false,
false, NULL);
}
index_close(idxRel, AccessShareLock);
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 247d0816ad..daefd726cc 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1549,7 +1549,8 @@ ProcessUtilitySlow(ParseState *pstate,
true,
/* check_rights */
true,
/* check_not_in_use */
false,
/* skip_build */
- false);
/* quiet */
+ false,
/* quiet */
+ NULL);
/*
* Add the CREATE INDEX node itself to
stash right away;
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 1d3ce246c9..b6eb8564c1 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -33,7 +33,8 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool
check_rights,
bool
check_not_in_use,
bool
skip_build,
- bool quiet);
+ bool quiet,
+ int
*parts_done);
extern void ExecReindex(ParseState *pstate, ReindexStmt *stmt, bool
isTopLevel);
extern char *makeObjectName(const char *name1, const char *name2,
const char *label);
--
2.30.2
From 8d0798ece406ac88ea0b1dfdfa91aa234d99c560 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] parts_done via IndexStmt
---
src/backend/bootstrap/bootparse.y | 2 ++
src/backend/commands/indexcmds.c | 48 ++++++++++++++++++++++++++----
src/backend/parser/parse_utilcmd.c | 2 ++
src/include/nodes/parsenodes.h | 1 +
4 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/backend/bootstrap/bootparse.y b/src/backend/bootstrap/bootparse.y
index e6d62d81c1..6be3cc2dcf 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 b5b860c3ab..1904a3ddd9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -129,6 +129,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
@@ -1218,8 +1241,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);
@@ -1429,10 +1460,11 @@ 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;
}
+ /* TODO: Should we add num_leaf_partitions() for attached idxs? */
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
free_attrmap(attmap);
}
@@ -1483,9 +1515,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 (!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 f743cd548c..27335b0aba 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 bebb9620b2..a8b69c387a 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3015,6 +3015,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;
} IndexStmt;
/* ----------------------
--
2.30.2
