Date: Fri, 3 Jan 2020 10:19:26 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Jeff Janes <jeff.janes@gmail.com>
Subject: explain HashAggregate to report bucket and memory stats
Message-ID: <20200103161925.GM12066@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="kORqDWCi7qDJ0mEj"
Content-Disposition: inline
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

On Sun, Feb 17, 2019 at 11:29:56AM -0500, Jeff Janes wrote:
https://www.postgresql.org/message-id/CAMkU%3D1zBJNVo2DGYBgLJqpu8fyjCE_ys%2Bmsr6pOEoiwA7y5jrA%40mail.gmail.com
> What would I find very useful is [...] if the HashAggregate node under
> "explain analyze" would report memory and bucket stats; and if the Aggregate
> node would report...anything.

Find attached my WIP attempt to implement this.

Jeff: can you suggest what details Aggregate should show ?

Justin

--kORqDWCi7qDJ0mEj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v1-0001-refactor-show_hinstrument-and-avoid-showing-memor.patch"

From 5d0afe5d92649f575d9b09ae19b31d2bfd5bfd12 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Wed, 1 Jan 2020 13:09:33 -0600
Subject: [PATCH v1 1/2] refactor: show_hinstrument and avoid showing memory
 use if not verbose..

This changes explain analyze at least for Hash(join), but doesn't break
affect regression tests, since they all run explain without analyze, so
nbatch=0, and no stats are shown.

But for future patch to show stats for HashAgg (for which nbatch=1, always), we
want to show buckets in explain analyze, but don't want to show memory, which
is machine-specific.
---
 src/backend/commands/explain.c | 73 ++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 497a3bd..d5eaf15 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -101,6 +101,7 @@ static void show_sortorder_options(StringInfo buf, Node *sortexpr,
 static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 							 List *ancestors, ExplainState *es);
 static void show_sort_info(SortState *sortstate, ExplainState *es);
+static void show_hinstrument(ExplainState *es, HashInstrumentation *h);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 								ExplainState *es);
@@ -2702,43 +2703,59 @@ show_hash_info(HashState *hashstate, ExplainState *es)
 		}
 	}
 
-	if (hinstrument.nbatch > 0)
-	{
-		long		spacePeakKb = (hinstrument.space_peak + 1023) / 1024;
+	show_hinstrument(es, &hinstrument);
+}
 
-		if (es->format != EXPLAIN_FORMAT_TEXT)
-		{
-			ExplainPropertyInteger("Hash Buckets", NULL,
-								   hinstrument.nbuckets, es);
-			ExplainPropertyInteger("Original Hash Buckets", NULL,
-								   hinstrument.nbuckets_original, es);
-			ExplainPropertyInteger("Hash Batches", NULL,
-								   hinstrument.nbatch, es);
-			ExplainPropertyInteger("Original Hash Batches", NULL,
-								   hinstrument.nbatch_original, es);
-			ExplainPropertyInteger("Peak Memory Usage", "kB",
-								   spacePeakKb, es);
-		}
-		else if (hinstrument.nbatch_original != hinstrument.nbatch ||
-				 hinstrument.nbuckets_original != hinstrument.nbuckets)
-		{
+/*
+ * Show hash bucket stats and (optionally) memory.
+ */
+static void
+show_hinstrument(ExplainState *es, HashInstrumentation *h)
+{
+	long		spacePeakKb = (h->space_peak + 1023) / 1024;
+
+	if (h->nbatch <= 0)
+		return;
+
+	if (es->format != EXPLAIN_FORMAT_TEXT)
+	{
+		ExplainPropertyInteger("Hash Buckets", NULL,
+							   h->nbuckets, es);
+		ExplainPropertyInteger("Original Hash Buckets", NULL,
+							   h->nbuckets_original, es);
+		ExplainPropertyInteger("Hash Batches", NULL,
+							   h->nbatch, es);
+		ExplainPropertyInteger("Original Hash Batches", NULL,
+							   h->nbatch_original, es);
+		ExplainPropertyInteger("Peak Memory Usage", "kB",
+							   spacePeakKb, es);
+	}
+	else
+	{
+		if (h->nbatch_original != h->nbatch ||
+			 h->nbuckets_original != h->nbuckets) {
 			appendStringInfoSpaces(es->str, es->indent * 2);
 			appendStringInfo(es->str,
-							 "Buckets: %d (originally %d)  Batches: %d (originally %d)  Memory Usage: %ldkB\n",
-							 hinstrument.nbuckets,
-							 hinstrument.nbuckets_original,
-							 hinstrument.nbatch,
-							 hinstrument.nbatch_original,
-							 spacePeakKb);
+						"Buckets: %d (originally %d)   Batches: %d (originally %d)",
+						h->nbuckets,
+						h->nbuckets_original,
+						h->nbatch,
+						h->nbatch_original);
 		}
 		else
 		{
 			appendStringInfoSpaces(es->str, es->indent * 2);
 			appendStringInfo(es->str,
-							 "Buckets: %d  Batches: %d  Memory Usage: %ldkB\n",
-							 hinstrument.nbuckets, hinstrument.nbatch,
-							 spacePeakKb);
+						"Buckets: %d  Batches: %d",
+						h->nbuckets,
+						h->nbatch);
 		}
+
+		if (es->verbose && es->analyze)
+			appendStringInfo(es->str,
+					"  Memory Usage: %ldkB",
+					spacePeakKb);
+		appendStringInfoChar(es->str, '\n');
 	}
 }
 
-- 
2.7.4


--kORqDWCi7qDJ0mEj
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v1-0002-explain-analyze-to-show-stats-from-hash-aggregate.patch"

From d691e492b619e5cc6a1fcd4134728c1c0852d589 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Tue, 31 Dec 2019 18:49:41 -0600
Subject: [PATCH v1 2/2] explain analyze to show stats from (hash) aggregate..

..as suggested by Jeff Janes
---
 src/backend/commands/explain.c      | 50 +++++++++++++++++++++++++++++++------
 src/backend/executor/execGrouping.c | 10 ++++++++
 src/include/executor/nodeAgg.h      |  1 +
 src/include/nodes/execnodes.h       | 27 ++++++++++----------
 4 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d5eaf15..22d6087 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -18,6 +18,7 @@
 #include "commands/createas.h"
 #include "commands/defrem.h"
 #include "commands/prepare.h"
+#include "executor/nodeAgg.h"
 #include "executor/nodeHash.h"
 #include "foreign/fdwapi.h"
 #include "jit/jit.h"
@@ -82,6 +83,7 @@ static void show_sort_keys(SortState *sortstate, List *ancestors,
 						   ExplainState *es);
 static void show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
 								   ExplainState *es);
+static void show_agg_info(AggState *astate, ExplainState *es);
 static void show_agg_keys(AggState *astate, List *ancestors,
 						  ExplainState *es);
 static void show_grouping_sets(PlanState *planstate, Agg *agg,
@@ -101,7 +103,7 @@ static void show_sortorder_options(StringInfo buf, Node *sortexpr,
 static void show_tablesample(TableSampleClause *tsc, PlanState *planstate,
 							 List *ancestors, ExplainState *es);
 static void show_sort_info(SortState *sortstate, ExplainState *es);
-static void show_hinstrument(ExplainState *es, HashInstrumentation *h);
+static void show_hinstrument(ExplainState *es, HashInstrumentation *h, bool showbatch);
 static void show_hash_info(HashState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 								ExplainState *es);
@@ -1848,6 +1850,8 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			if (plan->qual)
 				show_instrumentation_count("Rows Removed by Filter", 1,
 										   planstate, es);
+			show_agg_info(castNode(AggState, planstate), es);
+
 			break;
 		case T_Group:
 			show_group_keys(castNode(GroupState, planstate), ancestors, es);
@@ -2057,6 +2061,23 @@ ExplainNode(PlanState *planstate, List *ancestors,
 }
 
 /*
+ * Show instrumentation info for an Agg node.
+ */
+static void
+show_agg_info(AggState *astate, ExplainState *es)
+{
+	// perhash->aggnode->numGroups; memctx; AggState->
+
+	for (int i=0; i<astate->num_hashes; ++i) {
+		HashInstrumentation *hinstrument = &astate->perhash->hashtable->hinstrument;
+// fprintf(stderr, "memallocated %lu\n", astate->hashcontext->ecxt_per_query_memory->mem_allocated);
+		show_hinstrument(es, hinstrument, false);
+	}
+
+	// TODO
+}
+
+/*
  * Show the targetlist of a plan node
  */
 static void
@@ -2703,19 +2724,26 @@ show_hash_info(HashState *hashstate, ExplainState *es)
 		}
 	}
 
-	show_hinstrument(es, &hinstrument);
+	show_hinstrument(es, &hinstrument, true);
 }
 
 /*
  * Show hash bucket stats and (optionally) memory.
  */
 static void
-show_hinstrument(ExplainState *es, HashInstrumentation *h)
+show_hinstrument(ExplainState *es, HashInstrumentation *h, bool showbatch)
 {
 	long		spacePeakKb = (h->space_peak + 1023) / 1024;
 
+	// Currently, this isn't shown for explain of hash(join) since nbatch=0 without analyze
+	// But, it's shown for hashAgg since nbatch=1, always.
+	// Need to 1) avoid showing memory use if !analyze; and, 2) avoid memory use if not verbose
+
 	if (h->nbatch <= 0)
 		return;
+	/* This avoids showing anything if it's explain without analyze; should we just check that, instead ? */
+	if (!es->analyze)
+		return;
 
 	if (es->format != EXPLAIN_FORMAT_TEXT)
 	{
@@ -2736,9 +2764,12 @@ show_hinstrument(ExplainState *es, HashInstrumentation *h)
 			 h->nbuckets_original != h->nbuckets) {
 			appendStringInfoSpaces(es->str, es->indent * 2);
 			appendStringInfo(es->str,
-						"Buckets: %d (originally %d)   Batches: %d (originally %d)",
+						"Buckets: %d (originally %d)",
 						h->nbuckets,
-						h->nbuckets_original,
+						h->nbuckets_original);
+			if (showbatch)
+				appendStringInfo(es->str,
+						"  Batches: %d (originally %d)",
 						h->nbatch,
 						h->nbatch_original);
 		}
@@ -2746,9 +2777,12 @@ show_hinstrument(ExplainState *es, HashInstrumentation *h)
 		{
 			appendStringInfoSpaces(es->str, es->indent * 2);
 			appendStringInfo(es->str,
-						"Buckets: %d  Batches: %d",
-						h->nbuckets,
-						h->nbatch);
+						"Buckets: %d",
+						h->nbuckets);
+				if (showbatch)
+					appendStringInfo(es->str,
+							"  Batches: %d",
+							h->nbatch);
 		}
 
 		if (es->verbose && es->analyze)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 3603c58..cf0fe3c 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -203,6 +203,11 @@ BuildTupleHashTableExt(PlanState *parent,
 		hashtable->hash_iv = 0;
 
 	hashtable->hashtab = tuplehash_create(metacxt, nbuckets, hashtable);
+	hashtable->hinstrument.nbuckets_original = nbuckets;
+	hashtable->hinstrument.nbuckets = nbuckets;
+	hashtable->hinstrument.space_peak = entrysize * hashtable->hashtab->size;
+	hashtable->hinstrument.nbatch_original = 1; /* Unused */
+	hashtable->hinstrument.nbatch = 1; /* Unused */
 
 	/*
 	 * We copy the input tuple descriptor just for safety --- we assume all
@@ -328,6 +333,11 @@ LookupTupleHashEntry(TupleHashTable hashtable, TupleTableSlot *slot,
 		{
 			/* created new entry */
 			*isnew = true;
+			/* maybe grew ? XXX: need to call max() here ? */
+			hashtable->hinstrument.nbuckets = hashtable->hashtab->size;
+			hashtable->hinstrument.space_peak = sizeof(TupleHashEntryData) * hashtable->hashtab->size +
+				sizeof(MinimalTuple) * hashtable->hashtab->size; // members ?
+
 			/* zero caller data */
 			entry->additional = NULL;
 			MemoryContextSwitchTo(hashtable->tablecxt);
diff --git a/src/include/executor/nodeAgg.h b/src/include/executor/nodeAgg.h
index 2fe82da..b2ab74c 100644
--- a/src/include/executor/nodeAgg.h
+++ b/src/include/executor/nodeAgg.h
@@ -302,6 +302,7 @@ typedef struct AggStatePerHashData
 	AttrNumber *hashGrpColIdxInput; /* hash col indices in input slot */
 	AttrNumber *hashGrpColIdxHash;	/* indices in hash table tuples */
 	Agg		   *aggnode;		/* original Agg node, for numGroups etc. */
+	// XXX struct HashInstrumentation hinstrument;
 }			AggStatePerHashData;
 
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index eaea1f3..ef83406 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -671,6 +671,19 @@ typedef struct ExecAuxRowMark
 typedef struct TupleHashEntryData *TupleHashEntry;
 typedef struct TupleHashTableData *TupleHashTable;
 
+/* ----------------
+ *	 Values displayed by EXPLAIN ANALYZE
+ * ----------------
+ */
+typedef struct HashInstrumentation
+{
+	int			nbuckets;		/* number of buckets at end of execution */
+	int			nbuckets_original;	/* planned number of buckets */
+	int			nbatch;			/* number of batches at end of execution */
+	int			nbatch_original;	/* planned number of batches */
+	size_t		space_peak;		/* peak memory usage in bytes */
+} HashInstrumentation;
+
 typedef struct TupleHashEntryData
 {
 	MinimalTuple firstTuple;	/* copy of first tuple in this group */
@@ -705,6 +718,7 @@ typedef struct TupleHashTableData
 	ExprState  *cur_eq_func;	/* comparator for input vs. table */
 	uint32		hash_iv;		/* hash-function IV */
 	ExprContext *exprcontext;	/* expression context */
+	struct HashInstrumentation hinstrument;	/* XXX: NOT A POINTER this worker's entry */
 }			TupleHashTableData;
 
 typedef tuplehash_iterator TupleHashIterator;
@@ -2235,19 +2249,6 @@ typedef struct GatherMergeState
 } GatherMergeState;
 
 /* ----------------
- *	 Values displayed by EXPLAIN ANALYZE
- * ----------------
- */
-typedef struct HashInstrumentation
-{
-	int			nbuckets;		/* number of buckets at end of execution */
-	int			nbuckets_original;	/* planned number of buckets */
-	int			nbatch;			/* number of batches at end of execution */
-	int			nbatch_original;	/* planned number of batches */
-	size_t		space_peak;		/* peak memory usage in bytes */
-} HashInstrumentation;
-
-/* ----------------
  *	 Shared memory container for per-worker hash information
  * ----------------
  */
-- 
2.7.4


--kORqDWCi7qDJ0mEj--