Date: Mon, 12 Dec 2022 22:43:31 -0600
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
Message-ID: <20221213044331.GJ27893@telsasoft.com>
References: <a15f904a70924ffa4ca25c3c744cff31e0e6e143.camel@gmail.com>
 <20221211063334.GB27893@telsasoft.com>
 <D385C1FC-23C3-464B-B260-EA6398A53BAD@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="YD3LsXFS42OYHhNZ"
Content-Disposition: inline
In-Reply-To: <D385C1FC-23C3-464B-B260-EA6398A53BAD@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221213044331.GJ27893%40telsasoft.com>
Precedence: bulk


--YD3LsXFS42OYHhNZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--YD3LsXFS42OYHhNZ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-report-top-parent-progress-for-CREATE-INDEX.patch"

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.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


--YD3LsXFS42OYHhNZ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0001-report-top-parent-progress-for-CREATE-INDEX.txt"

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.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


--YD3LsXFS42OYHhNZ--