From: Justin Pryzby <pryzby@telsasoft.com>
To: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
Date: Mon, 12 Dec 2022 22:43:31 -0600
Message-ID: <20221213044331.GJ27893@telsasoft.com> (raw)
In-Reply-To: <D385C1FC-23C3-464B-B260-EA6398A53BAD@gmail.com>
References: <a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel@gmail.com>
<20221211063334.GB27893@telsasoft.com>
<D385C1FC-23C3-464B-B260-EA6398A53BAD@gmail.com>
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
Attachments:0001-report-top-parent-progress-for-CREATE-INDEX.patch (text/x-diff, 5.9K) download
From 2e93bd37ca3c8add1f8e3e44a9f3906c332b83f2 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@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.cindex 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.cindex 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.hindex 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
0001-report-top-parent-progress-for-CREATE-INDEX.txt (text/x-diff, 6.2K) download
From cc6237f4f8e32c63611d37839758b1a30a174c69 Mon Sep 17 00:00:00 2001
From: Ilya Gladyshev <ilya.v.gladyshev@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.sgmlindex 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.cindex 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.cindex 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
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: pryzby@telsasoft.com, ilya.v.gladyshev@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Progress report of CREATE INDEX for nested partitioned tables
In-Reply-To: <20221213044331.GJ27893@telsasoft.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