From 320bddf8fbefe31c150406187fcf8757e8a3ebba Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 9 Feb 2020 15:08:14 -0600
Subject: [PATCH v6 5/7] Refactor for consistency/symmetry

This moves hash instrumentation out of execGrouping.c / TupleHashTable and into
higher level nodes, for consistency with bitmapHeapScan.

This might be unimportant and maybe clearer left in execGrouping.c.
---
 src/backend/commands/explain.c            | 18 +++++++--------
 src/backend/executor/execGrouping.c       | 27 -----------------------
 src/backend/executor/nodeAgg.c            | 14 ++++++++----
 src/backend/executor/nodeRecursiveunion.c |  3 ++-
 src/backend/executor/nodeSetOp.c          |  6 ++++-
 src/backend/executor/nodeSubplan.c        | 10 +++++++--
 src/include/executor/executor.h           |  1 -
 src/include/executor/nodeAgg.h            |  1 +
 src/include/nodes/execnodes.h             | 24 ++++++++++++++++++--
 9 files changed, 57 insertions(+), 47 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 637480de79..eb093029d9 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1347,13 +1347,13 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			{
 				ExplainIndentText(es);
 				appendStringInfoString(es->str, "Hashtable: ");
-				show_tuplehash_info(&subplanstate->hashtable->instrument, es);
+				show_tuplehash_info(&subplanstate->instrument, es);
 			}
 			if (subplanstate->hashnulls)
 			{
 				ExplainIndentText(es);
 				appendStringInfoString(es->str, "Null Hashtable: ");
-				show_tuplehash_info(&subplanstate->hashnulls->instrument, es);
+				show_tuplehash_info(&subplanstate->instrument_nulls, es);
 			}
 		}
 		if (es->indent)
@@ -1387,14 +1387,14 @@ ExplainNode(PlanState *planstate, List *ancestors,
 		if (subplanstate && subplanstate->hashtable)
 		{
 			ExplainOpenGroup("Hashtable", "Hashtable", true, es);
-			show_tuplehash_info(&subplanstate->hashtable->instrument, es);
+			show_tuplehash_info(&subplanstate->instrument, es);
 			ExplainCloseGroup("Hashtable", "Hashtable", true, es);
 		}
 
 		if (subplanstate && subplanstate->hashnulls)
 		{
 			ExplainOpenGroup("Null Hashtable", "Null Hashtable", true, es);
-			show_tuplehash_info(&subplanstate->hashnulls->instrument, es);
+			show_tuplehash_info(&subplanstate->instrument_nulls, es);
 			ExplainCloseGroup("Null Hashtable", "Null Hashtable", true, es);
 		}
 	}
@@ -1925,14 +1925,14 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			{
 				SetOpState *sos = castNode(SetOpState, planstate);
 				if (sos->hashtable)
-					show_tuplehash_info(&sos->hashtable->instrument, es);
+					show_tuplehash_info(&sos->instrument, es);
 			}
 			break;
 		case T_RecursiveUnion:
 			{
 				RecursiveUnionState *rus = (RecursiveUnionState *)planstate;
 				if (rus->hashtable)
-					show_tuplehash_info(&rus->hashtable->instrument, es);
+					show_tuplehash_info(&rus->instrument, es);
 				break;
 			}
 		case T_Group:
@@ -2320,7 +2320,7 @@ show_agg_keys(AggState *astate, List *ancestors,
 								 ancestors, es);
 			Assert(astate->num_hashes <= 1);
 			if (astate->num_hashes)
-				show_tuplehash_info(&astate->perhash[0].hashtable->instrument, es);
+				show_tuplehash_info(&astate->perhash[0].instrument, es);
 		}
 
 		ancestors = list_delete_first(ancestors);
@@ -2347,7 +2347,7 @@ show_grouping_sets(AggState *aggstate, Agg *agg,
 
 	show_grouping_set_info(aggstate, agg, NULL, context, useprefix, ancestors,
 			aggstate->num_hashes ?
-			&aggstate->perhash[setno++].hashtable->instrument : NULL,
+			&aggstate->perhash[setno++].instrument : NULL,
 			es);
 
 	foreach(lc, agg->chain)
@@ -2360,7 +2360,7 @@ show_grouping_sets(AggState *aggstate, Agg *agg,
 				aggnode->aggstrategy == AGG_MIXED)
 		{
 			Assert(setno < aggstate->num_hashes);
-			inst = &aggstate->perhash[setno++].hashtable->instrument;
+			inst = &aggstate->perhash[setno++].instrument;
 		}
 
 		show_grouping_set_info(aggstate, aggnode, sortnode,
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 2f5258e1d2..009d27b9a8 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -188,7 +188,6 @@ BuildTupleHashTableExt(PlanState *parent,
 	hashtable->inputslot = NULL;
 	hashtable->in_hash_funcs = NULL;
 	hashtable->cur_eq_func = NULL;
-	memset(&hashtable->instrument, 0, sizeof(hashtable->instrument));
 
 	/*
 	 * If parallelism is in use, even if the master backend is performing the
@@ -204,7 +203,6 @@ BuildTupleHashTableExt(PlanState *parent,
 		hashtable->hash_iv = 0;
 
 	hashtable->hashtab = tuplehash_create(metacxt, nbuckets, hashtable);
-	UpdateTupleHashTableStats(hashtable, true);
 
 	/*
 	 * We copy the input tuple descriptor just for safety --- we assume all
@@ -283,34 +281,9 @@ BuildTupleHashTable(PlanState *parent,
 void
 ResetTupleHashTable(TupleHashTable hashtable)
 {
-	UpdateTupleHashTableStats(hashtable, false);
 	tuplehash_reset(hashtable->hashtab);
 }
 
-/* Update instrumentation stats */
-void
-UpdateTupleHashTableStats(TupleHashTable hashtable, bool initial)
-{
-	hashtable->instrument.nbuckets = hashtable->hashtab->size;
-	if (initial)
-	{
-		hashtable->instrument.nbuckets_original = hashtable->hashtab->size;
-		hashtable->instrument.space_peak_hash = hashtable->hashtab->size *
-			sizeof(TupleHashEntryData);
-		hashtable->instrument.space_peak_tuples = 0;
-	}
-	else
-	{
-#define maxself(a,b) a=Max(a,b)
-		/* hashtable->entrysize includes additionalsize */
-		maxself(hashtable->instrument.space_peak_hash,
-				hashtable->hashtab->size * sizeof(TupleHashEntryData));
-		maxself(hashtable->instrument.space_peak_tuples,
-				hashtable->hashtab->members * hashtable->entrysize);
-#undef maxself
-	}
-}
-
 /*
  * Find or create a hashtable entry for the tuple group containing the
  * given tuple.  The tuple must be the same type as the hashtable entries.
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 4b8525edb6..3d60f7a314 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1334,6 +1334,9 @@ build_hash_table(AggState *aggstate, int setno, long nbuckets)
 		hashcxt,
 		tmpcxt,
 		DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit));
+
+	InitTupleHashTableStats(perhash->instrument,
+			perhash->hashtable->hashtab, additionalsize);
 }
 
 /*
@@ -1710,9 +1713,10 @@ agg_retrieve_direct(AggState *aggstate)
 				 */
 				initialize_phase(aggstate, 0);
 				aggstate->table_filled = true;
-				UpdateTupleHashTableStats(aggstate->perhash[0].hashtable, false);
+				UpdateTupleHashTableStats(aggstate->perhash[0].instrument,
+						aggstate->perhash[0].hashtable->hashtab);
 				ResetTupleHashIterator(aggstate->perhash[0].hashtable,
-									   &aggstate->perhash[0].hashiter);
+						&aggstate->perhash[0].hashiter);
 				select_current_set(aggstate, 0, true);
 				return agg_retrieve_hash_table(aggstate);
 			}
@@ -1913,7 +1917,8 @@ agg_retrieve_direct(AggState *aggstate)
 						aggstate->current_phase == 1)
 				{
 					for (int i = 0; i < aggstate->num_hashes; i++)
-						UpdateTupleHashTableStats(aggstate->perhash[i].hashtable, false);
+						UpdateTupleHashTableStats(aggstate->perhash[i].instrument,
+								aggstate->perhash[i].hashtable->hashtab);
 				}
 			}
 
@@ -1990,7 +1995,8 @@ agg_fill_hash_table(AggState *aggstate)
 
 	aggstate->table_filled = true;
 	for (int i = 0; i < aggstate->num_hashes; i++)
-		UpdateTupleHashTableStats(aggstate->perhash[i].hashtable, false);
+		UpdateTupleHashTableStats(aggstate->perhash[i].instrument,
+				aggstate->perhash[i].hashtable->hashtab);
 
 	/* Initialize to walk the first hash table */
 	select_current_set(aggstate, 0, true);
diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c
index 93272c28b1..594abdbdf0 100644
--- a/src/backend/executor/nodeRecursiveunion.c
+++ b/src/backend/executor/nodeRecursiveunion.c
@@ -50,6 +50,7 @@ build_hash_table(RecursiveUnionState *rustate)
 												rustate->tableContext,
 												rustate->tempContext,
 												false);
+	InitTupleHashTableStats(rustate->instrument, rustate->hashtable->hashtab, 0);
 }
 
 
@@ -157,7 +158,7 @@ ExecRecursiveUnion(PlanState *pstate)
 	}
 
 	if (node->hashtable)
-		UpdateTupleHashTableStats(node->hashtable, false);
+		UpdateTupleHashTableStats(node->instrument, node->hashtable->hashtab);
 
 	return NULL;
 }
diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c
index 9c0e0ab96e..a3860758eb 100644
--- a/src/backend/executor/nodeSetOp.c
+++ b/src/backend/executor/nodeSetOp.c
@@ -139,6 +139,9 @@ build_hash_table(SetOpState *setopstate)
 												   setopstate->tableContext,
 												   econtext->ecxt_per_tuple_memory,
 												   false);
+
+	InitTupleHashTableStats(setopstate->instrument,
+			setopstate->hashtable->hashtab, 0);
 }
 
 /*
@@ -415,7 +418,8 @@ setop_fill_hash_table(SetOpState *setopstate)
 
 	setopstate->table_filled = true;
 	/* Initialize to walk the hash table */
-	UpdateTupleHashTableStats(setopstate->hashtable, false);
+	UpdateTupleHashTableStats(setopstate->instrument,
+			setopstate->hashtable->hashtab);
 	ResetTupleHashIterator(setopstate->hashtable, &setopstate->hashiter);
 }
 
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 22c32612ba..03533538f6 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -505,6 +505,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 	if (node->hashtable)
 		ResetTupleHashTable(node->hashtable);
 	else
+	{
 		node->hashtable = BuildTupleHashTableExt(node->parent,
 												 node->descRight,
 												 ncols,
@@ -518,6 +519,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 												 node->hashtablecxt,
 												 node->hashtempcxt,
 												 false);
+		InitTupleHashTableStats(node->instrument, node->hashtable->hashtab, 0);
+	}
 
 	if (!subplan->unknownEqFalse)
 	{
@@ -533,6 +536,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 		if (node->hashnulls)
 			ResetTupleHashTable(node->hashnulls);
 		else
+		{
 			node->hashnulls = BuildTupleHashTableExt(node->parent,
 													 node->descRight,
 													 ncols,
@@ -546,6 +550,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 													 node->hashtablecxt,
 													 node->hashtempcxt,
 													 false);
+			InitTupleHashTableStats(node->instrument_nulls, node->hashnulls->hashtab, 0);
+		}
 	}
 	else
 		node->hashnulls = NULL;
@@ -621,9 +627,9 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 	ExecClearTuple(node->projRight->pi_state.resultslot);
 
 	MemoryContextSwitchTo(oldcontext);
-	UpdateTupleHashTableStats(node->hashtable, false);
+	UpdateTupleHashTableStats(node->instrument, node->hashtable->hashtab);
 	if (node->hashnulls)
-		UpdateTupleHashTableStats(node->hashnulls, false);
+		UpdateTupleHashTableStats(node->instrument_nulls, node->hashnulls->hashtab);
 }
 
 /*
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 34199b57e6..81fdfa4add 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -150,7 +150,6 @@ extern TupleHashEntry FindTupleHashEntry(TupleHashTable hashtable,
 										 ExprState *eqcomp,
 										 FmgrInfo *hashfunctions);
 extern void ResetTupleHashTable(TupleHashTable hashtable);
-extern void UpdateTupleHashTableStats(TupleHashTable hashtable, bool initial);
 
 /*
  * prototypes from functions in execJunk.c
diff --git a/src/include/executor/nodeAgg.h b/src/include/executor/nodeAgg.h
index 264916f9a9..2072c18b54 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. */
+	HashTableInstrumentation    instrument;
 }			AggStatePerHashData;
 
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b1e2d1f1ae..11fe866c1c 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -691,8 +691,25 @@ typedef struct TupleHashEntryData
 #define SH_DECLARE
 #include "lib/simplehash.h"
 
+#define InitTupleHashTableStats(instr, htable, addsize) \
+	do{\
+	instr.entrysize = sizeof(MinimalTuple) + addsize; \
+	instr.nbuckets = htable->size; \
+	instr.nbuckets_original = htable->size; \
+	instr.space_peak_hash = htable->size * sizeof(TupleHashEntryData); \
+	instr.space_peak_tuples = 0; \
+	}while(0)
+
+#define UpdateTupleHashTableStats(instr, htable) \
+	do{\
+	instr.nbuckets = htable->size; \
+	instr.space_peak_hash = Max(instr.space_peak_hash, htable->size*sizeof(TupleHashEntryData)); \
+	instr.space_peak_tuples = Max(instr.space_peak_tuples, htable->members*instr.entrysize );\
+	}while(0)
+
 typedef struct HashTableInstrumentation
 {
+	size_t	entrysize;				/* Includes additionalsize */
 	size_t	nbuckets;				/* number of buckets at end of execution */
 	size_t	nbuckets_original;		/* planned number of buckets */
 	size_t	space_peak_hash;	/* peak memory usage in bytes */
@@ -717,7 +734,6 @@ typedef struct TupleHashTableData
 	ExprState  *cur_eq_func;	/* comparator for input vs. table */
 	uint32		hash_iv;		/* hash-function IV */
 	ExprContext *exprcontext;	/* expression context */
-	HashTableInstrumentation instrument;
 }			TupleHashTableData;
 
 typedef tuplehash_iterator TupleHashIterator;
@@ -883,6 +899,8 @@ typedef struct SubPlanState
 	FmgrInfo   *lhs_hash_funcs; /* hash functions for lefthand datatype(s) */
 	FmgrInfo   *cur_eq_funcs;	/* equality functions for LHS vs. table */
 	ExprState  *cur_eq_comp;	/* equality comparator for LHS vs. table */
+	HashTableInstrumentation instrument;
+	HashTableInstrumentation instrument_nulls; /* instrumentation for nulls hashtable */
 } SubPlanState;
 
 /* ----------------
@@ -1291,6 +1309,7 @@ typedef struct RecursiveUnionState
 	MemoryContext tempContext;	/* short-term context for comparisons */
 	TupleHashTable hashtable;	/* hash table for tuples already seen */
 	MemoryContext tableContext; /* memory context containing hash table */
+	HashTableInstrumentation instrument;
 } RecursiveUnionState;
 
 /* ----------------
@@ -1609,7 +1628,7 @@ typedef struct BitmapHeapScanState
 	TBMSharedIterator *shared_tbmiterator;
 	TBMSharedIterator *shared_prefetch_iterator;
 	ParallelBitmapHeapState *pstate;
-	HashTableInstrumentation	instrument;
+	HashTableInstrumentation instrument;
 } BitmapHeapScanState;
 
 /* ----------------
@@ -2324,6 +2343,7 @@ typedef struct SetOpState
 	MemoryContext tableContext; /* memory context containing hash table */
 	bool		table_filled;	/* hash table filled yet? */
 	TupleHashIterator hashiter; /* for iterating through hash table */
+	HashTableInstrumentation instrument;
 } SetOpState;
 
 /* ----------------
-- 
2.17.0

