Date: Sun, 26 Jan 2020 11:17:37 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: pgsql-hackers@lists.postgresql.org
Cc: Jeff Janes <jeff.janes@gmail.com>
Subject: Re: explain HashAggregate to report bucket and memory stats
Message-ID: <20200126171737.GR13621@telsasoft.com>
References: <20200103161925.GM12066@telsasoft.com>
 <20200126141425.GQ13621@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="JfVplkuTfB13Rsg5"
Content-Disposition: inline
In-Reply-To: <20200126141425.GQ13621@telsasoft.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


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

On Sun, Jan 26, 2020 at 08:14:25AM -0600, Justin Pryzby wrote:
> On Fri, Jan 03, 2020 at 10:19:25AM -0600, Justin Pryzby wrote:
> > 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 ?
> 
> Rebased on top of 10013684970453a0ddc86050bba813c611114321
> And added https://commitfest.postgresql.org/27/2428/

Attached for real.

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

From 42634353f44ab44a06ffe4ae36a0dcbb848408ca 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 affect
regression tests, since all the HashJoin tests seem to 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, since
it's machine-specific.
---
 src/backend/commands/explain.c | 79 +++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f523adb..11b5857 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -103,6 +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_hash_info(HashState *hashstate, ExplainState *es);
 static void show_tidbitmap_info(BitmapHeapScanState *planstate,
 								ExplainState *es);
@@ -2713,43 +2714,67 @@ show_hash_info(HashState *hashstate, ExplainState *es)
 		}
 	}
 
-	if (hinstrument.nbatch > 0)
+	show_hinstrument(es, &hinstrument);
+}
+
+/*
+ * Show hash bucket stats and (optionally) memory.
+ */
+static void
+show_hinstrument(ExplainState *es, HashInstrumentation *h)
+{
+	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)
+	{
+		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
 	{
-		long		spacePeakKb = (hinstrument.space_peak + 1023) / 1024;
 
-		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)
-		{
+		if (h->nbatch_original != h->nbatch ||
+			 h->nbuckets_original != h->nbuckets) {
 			ExplainIndentText(es);
 			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
 		{
 			ExplainIndentText(es);
 			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


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

From 4b61e5ae514f5e5bc430bcf1132597ba4ecd202b 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      | 41 +++++++++++++++++++++++++++++--------
 src/backend/executor/execGrouping.c | 10 +++++++++
 src/include/nodes/execnodes.h       | 27 ++++++++++++------------
 3 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 11b5857..9493e7d 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"
@@ -84,6 +85,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,
@@ -103,7 +105,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);
@@ -1889,6 +1891,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);
@@ -2072,6 +2076,21 @@ 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);
+	}
+}
+
+/*
  * Show the targetlist of a plan node
  */
 static void
@@ -2714,14 +2733,14 @@ 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;
 
@@ -2755,9 +2774,12 @@ show_hinstrument(ExplainState *es, HashInstrumentation *h)
 			 h->nbuckets_original != h->nbuckets) {
 			ExplainIndentText(es);
 			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);
 		}
@@ -2765,9 +2787,12 @@ show_hinstrument(ExplainState *es, HashInstrumentation *h)
 		{
 			ExplainIndentText(es);
 			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/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 1f6f5bb..913416f 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 */
+	HashInstrumentation hinstrument;	/* instrumentation for EXPLAIN */
 }			TupleHashTableData;
 
 typedef tuplehash_iterator TupleHashIterator;
@@ -2243,19 +2257,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


--JfVplkuTfB13Rsg5--