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 ?
> 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.
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.
Alternately, if it's okay to add nparts_done to the IndexStmt, then
that's easy.
--
Justin
>From 2e93bd37ca3c8add1f8e3e44a9f3906c332b83f2 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladys...@gmail.com>
Date: Fri, 9 Dec 2022 23:17:29 +0400
Subject: [PATCH] report top parent progress for CREATE INDEX
! add asserts, avoid global var
! do not count intermediate/nested/sub-partitions
---
src/backend/commands/indexcmds.c | 30 ++++++-
src/backend/utils/activity/backend_progress.c | 79 +++++++++++++++++++
src/include/nodes/parsenodes.h | 2 +
3 files changed, 107 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b5b860c3abf..6e6bba9d3a9 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1218,8 +1218,28 @@ DefineIndex(Oid relationId,
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ if (!OidIsValid(parentIndexId))
+ {
+ int nleaves = 0;
+ List *childs;
+ ListCell *lc;
+
+ /*
+ * Count the number of physical children, excluding foreign
+ * tables, intermediate partitioned tables, as well as the
+ * partitioned index itself.
+ */
+ childs = find_all_inheritors(relationId, NoLock, NULL);
+ foreach(lc, childs)
+ {
+ Oid partrelid = lfirst_oid(lc);
+ if (RELKIND_HAS_STORAGE(get_rel_relkind(partrelid)))
+ nleaves++;
+ }
+
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ nleaves);
+ }
/* Make a local copy of partdesc->oids[], just for safety */
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1431,8 +1451,10 @@ DefineIndex(Oid relationId,
child_save_sec_context);
}
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
+ if (RELKIND_HAS_STORAGE(get_rel_relkind(childRelid)))
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ ++stmt->nparts_done);
+
free_attrmap(attmap);
}
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index f29199725b7..c661ad94782 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -10,6 +10,7 @@
*/
#include "postgres.h"
+#include "commands/progress.h"
#include "port/atomics.h" /* for memory barriers */
#include "utils/backend_progress.h"
#include "utils/backend_status.h"
@@ -37,6 +38,82 @@ pgstat_progress_start_command(ProgressCommandType cmdtype, Oid relid)
PGSTAT_END_WRITE_ACTIVITY(beentry);
}
+static void
+pgstat_progress_asserts(void)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+ volatile int64 *a = beentry->st_progress_param;
+
+ /*
+ * If the command fails due to interrupt or error, the values may be
+ * less than rather than equal to expected, final value.
+ */
+
+ switch (beentry->st_progress_command)
+ {
+ case PROGRESS_COMMAND_VACUUM:
+ Assert(a[PROGRESS_VACUUM_HEAP_BLKS_SCANNED] <=
+ a[PROGRESS_VACUUM_TOTAL_HEAP_BLKS]);
+ Assert(a[PROGRESS_VACUUM_HEAP_BLKS_VACUUMED] <=
+ a[PROGRESS_VACUUM_TOTAL_HEAP_BLKS]);
+ Assert(a[PROGRESS_VACUUM_NUM_DEAD_TUPLES] <=
+ a[PROGRESS_VACUUM_MAX_DEAD_TUPLES]);
+ break;
+
+ case PROGRESS_COMMAND_ANALYZE:
+ Assert(a[PROGRESS_ANALYZE_BLOCKS_DONE] <=
+ a[PROGRESS_ANALYZE_BLOCKS_TOTAL]);
+ /* Extended stats can be skipped */
+ Assert(a[PROGRESS_ANALYZE_EXT_STATS_COMPUTED] <=
+ a[PROGRESS_ANALYZE_EXT_STATS_TOTAL]);
+ Assert(a[PROGRESS_ANALYZE_CHILD_TABLES_DONE] <=
+ a[PROGRESS_ANALYZE_CHILD_TABLES_TOTAL]);
+ break;
+
+ case PROGRESS_COMMAND_CLUSTER:
+ Assert(a[PROGRESS_CLUSTER_HEAP_BLKS_SCANNED] <=
+ a[PROGRESS_CLUSTER_TOTAL_HEAP_BLKS]);
+ break;
+
+ case PROGRESS_COMMAND_CREATE_INDEX:
+ Assert(a[PROGRESS_CREATEIDX_TUPLES_DONE] <=
+ a[PROGRESS_CREATEIDX_TUPLES_TOTAL]);
+ /* The command can fail before finishing indexes */
+ Assert(a[PROGRESS_CREATEIDX_PARTITIONS_DONE] <=
+ a[PROGRESS_CREATEIDX_PARTITIONS_TOTAL]);
+ break;
+
+ case PROGRESS_COMMAND_BASEBACKUP:
+ /* progress reporting is optional for these */
+ if (a[PROGRESS_BASEBACKUP_BACKUP_TOTAL] >= 0)
+ {
+ Assert(a[PROGRESS_BASEBACKUP_BACKUP_STREAMED] <=
+ a[PROGRESS_BASEBACKUP_BACKUP_TOTAL]);
+ Assert(a[PROGRESS_BASEBACKUP_TBLSPC_STREAMED] <=
+ a[PROGRESS_BASEBACKUP_TBLSPC_TOTAL]);
+ }
+ break;
+
+ case PROGRESS_COMMAND_COPY:
+#if 0
+// This currently fails file_fdw tests, since pgstat_prorgress evidently fails
+// to support simultaneous copy commands, as happens during JOIN.
+ /* bytes progress is not available in all cases */
+ if (a[PROGRESS_COPY_BYTES_TOTAL] > 0)
+ // Assert(a[PROGRESS_COPY_BYTES_PROCESSED] <= a[PROGRESS_COPY_BYTES_TOTAL]);
+ if (a[PROGRESS_COPY_BYTES_PROCESSED] > a[PROGRESS_COPY_BYTES_TOTAL])
+ elog(WARNING, "PROGRESS_COPY_BYTES_PROCESSED %ld %ld",
+ a[PROGRESS_COPY_BYTES_PROCESSED],
+ a[PROGRESS_COPY_BYTES_TOTAL]);
+
+ break;
+#endif
+
+ case PROGRESS_COMMAND_INVALID:
+ break; /* Do nothing */
+ }
+}
+
/*-----------
* pgstat_progress_update_param() -
*
@@ -105,6 +182,8 @@ pgstat_progress_end_command(void)
if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
return;
+ pgstat_progress_asserts();
+
PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
beentry->st_progress_command_target = InvalidOid;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index bebb9620b27..e07527d80f2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3015,6 +3015,8 @@ 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 nparts_done; /* number of partitions processed during
+ * index creation */
} IndexStmt;
/* ----------------------
--
2.25.1
>From cc6237f4f8e32c63611d37839758b1a30a174c69 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladys...@gmail.com>
Date: Fri, 9 Dec 2022 23:17:29 +0400
Subject: [PATCH] report top parent progress for CREATE INDEX
---
doc/src/sgml/monitoring.sgml | 2 +
src/backend/commands/indexcmds.c | 17 +++-
src/backend/utils/activity/backend_progress.c | 79 +++++++++++++++++++
3 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 11a8ebe5ec7..c2389477356 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -6536,6 +6536,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<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.
+ This includes direct children, but not indirect children.
This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
@@ -6547,6 +6548,7 @@ FROM pg_stat_get_backend_idset() AS backendid;
<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.
+ This includes direct children, but not indirect children.
This field is <literal>0</literal> during a <literal>REINDEX</literal>.
</para></entry>
</row>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b5b860c3abf..e97ba963c37 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1218,8 +1218,12 @@ DefineIndex(Oid relationId,
Relation parentIndex;
TupleDesc parentDesc;
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
- nparts);
+ if (!OidIsValid(parentIndexId))
+ {
+ /* Set the number of partitions to the number of *direct* children */
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_TOTAL,
+ nparts);
+ }
/* Make a local copy of partdesc->oids[], just for safety */
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
@@ -1284,6 +1288,9 @@ DefineIndex(Oid relationId,
SetUserIdAndSecContext(child_save_userid,
child_save_sec_context);
table_close(childrel, lockmode);
+ if (!OidIsValid(parentIndexId))
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ i + 1);
continue;
}
@@ -1431,8 +1438,10 @@ DefineIndex(Oid relationId,
child_save_sec_context);
}
- pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
- i + 1);
+ if (!OidIsValid(parentIndexId))
+ pgstat_progress_update_param(PROGRESS_CREATEIDX_PARTITIONS_DONE,
+ i + 1);
+
free_attrmap(attmap);
}
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index f29199725b7..c661ad94782 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -10,6 +10,7 @@
*/
#include "postgres.h"
+#include "commands/progress.h"
#include "port/atomics.h" /* for memory barriers */
#include "utils/backend_progress.h"
#include "utils/backend_status.h"
@@ -37,6 +38,82 @@ pgstat_progress_start_command(ProgressCommandType cmdtype, Oid relid)
PGSTAT_END_WRITE_ACTIVITY(beentry);
}
+static void
+pgstat_progress_asserts(void)
+{
+ volatile PgBackendStatus *beentry = MyBEEntry;
+ volatile int64 *a = beentry->st_progress_param;
+
+ /*
+ * If the command fails due to interrupt or error, the values may be
+ * less than rather than equal to expected, final value.
+ */
+
+ switch (beentry->st_progress_command)
+ {
+ case PROGRESS_COMMAND_VACUUM:
+ Assert(a[PROGRESS_VACUUM_HEAP_BLKS_SCANNED] <=
+ a[PROGRESS_VACUUM_TOTAL_HEAP_BLKS]);
+ Assert(a[PROGRESS_VACUUM_HEAP_BLKS_VACUUMED] <=
+ a[PROGRESS_VACUUM_TOTAL_HEAP_BLKS]);
+ Assert(a[PROGRESS_VACUUM_NUM_DEAD_TUPLES] <=
+ a[PROGRESS_VACUUM_MAX_DEAD_TUPLES]);
+ break;
+
+ case PROGRESS_COMMAND_ANALYZE:
+ Assert(a[PROGRESS_ANALYZE_BLOCKS_DONE] <=
+ a[PROGRESS_ANALYZE_BLOCKS_TOTAL]);
+ /* Extended stats can be skipped */
+ Assert(a[PROGRESS_ANALYZE_EXT_STATS_COMPUTED] <=
+ a[PROGRESS_ANALYZE_EXT_STATS_TOTAL]);
+ Assert(a[PROGRESS_ANALYZE_CHILD_TABLES_DONE] <=
+ a[PROGRESS_ANALYZE_CHILD_TABLES_TOTAL]);
+ break;
+
+ case PROGRESS_COMMAND_CLUSTER:
+ Assert(a[PROGRESS_CLUSTER_HEAP_BLKS_SCANNED] <=
+ a[PROGRESS_CLUSTER_TOTAL_HEAP_BLKS]);
+ break;
+
+ case PROGRESS_COMMAND_CREATE_INDEX:
+ Assert(a[PROGRESS_CREATEIDX_TUPLES_DONE] <=
+ a[PROGRESS_CREATEIDX_TUPLES_TOTAL]);
+ /* The command can fail before finishing indexes */
+ Assert(a[PROGRESS_CREATEIDX_PARTITIONS_DONE] <=
+ a[PROGRESS_CREATEIDX_PARTITIONS_TOTAL]);
+ break;
+
+ case PROGRESS_COMMAND_BASEBACKUP:
+ /* progress reporting is optional for these */
+ if (a[PROGRESS_BASEBACKUP_BACKUP_TOTAL] >= 0)
+ {
+ Assert(a[PROGRESS_BASEBACKUP_BACKUP_STREAMED] <=
+ a[PROGRESS_BASEBACKUP_BACKUP_TOTAL]);
+ Assert(a[PROGRESS_BASEBACKUP_TBLSPC_STREAMED] <=
+ a[PROGRESS_BASEBACKUP_TBLSPC_TOTAL]);
+ }
+ break;
+
+ case PROGRESS_COMMAND_COPY:
+#if 0
+// This currently fails file_fdw tests, since pgstat_prorgress evidently fails
+// to support simultaneous copy commands, as happens during JOIN.
+ /* bytes progress is not available in all cases */
+ if (a[PROGRESS_COPY_BYTES_TOTAL] > 0)
+ // Assert(a[PROGRESS_COPY_BYTES_PROCESSED] <= a[PROGRESS_COPY_BYTES_TOTAL]);
+ if (a[PROGRESS_COPY_BYTES_PROCESSED] > a[PROGRESS_COPY_BYTES_TOTAL])
+ elog(WARNING, "PROGRESS_COPY_BYTES_PROCESSED %ld %ld",
+ a[PROGRESS_COPY_BYTES_PROCESSED],
+ a[PROGRESS_COPY_BYTES_TOTAL]);
+
+ break;
+#endif
+
+ case PROGRESS_COMMAND_INVALID:
+ break; /* Do nothing */
+ }
+}
+
/*-----------
* pgstat_progress_update_param() -
*
@@ -105,6 +182,8 @@ pgstat_progress_end_command(void)
if (beentry->st_progress_command == PROGRESS_COMMAND_INVALID)
return;
+ pgstat_progress_asserts();
+
PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
beentry->st_progress_command_target = InvalidOid;
--
2.25.1
