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

This moves hash instrumentation out of execGrouping.c / TupleHashTable and into
higher level nodes, for consistency with bitmapHeapScan.
---
 src/backend/commands/explain.c            | 18 +++++++++---------
 src/backend/executor/execGrouping.c       | 25 -------------------------
 src/backend/executor/nodeAgg.c            |  9 ++++++---
 src/backend/executor/nodeRecursiveunion.c |  3 ++-
 src/backend/executor/nodeSetOp.c          |  3 ++-
 src/backend/executor/nodeSubplan.c        | 11 ++++++++---
 src/include/executor/executor.h           |  1 -
 src/include/executor/nodeAgg.h            |  1 +
 src/include/nodes/execnodes.h             | 22 +++++++++++++++++++++-
 9 files changed, 49 insertions(+), 44 deletions(-)

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d71f5f1..1415bce 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1341,11 +1341,11 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			Assert(subplanstate != NULL);
 			/* Show hash stats for hashed subplan */
 			if (subplanstate->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)
@@ -1376,10 +1376,10 @@ ExplainNode(PlanState *planstate, List *ancestors,
 			ExplainPropertyText("Custom Plan Provider", custom_name, es);
 		ExplainPropertyBool("Parallel Aware", plan->parallel_aware, es);
 		if (subplanstate && subplanstate->hashtable)
-			show_tuplehash_info(&subplanstate->hashtable->instrument, es);
+			show_tuplehash_info(&subplanstate->instrument, 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);
 		}
 	}
@@ -1911,14 +1911,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:
@@ -2305,7 +2305,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);
@@ -2332,7 +2332,7 @@ show_grouping_sets(AggState *aggstate, Agg *agg,
 	// this will show things twice??
 	show_grouping_set_keys(aggstate, agg, NULL,
 						   context, useprefix, ancestors, es,
-						   aggstate->num_hashes ? &aggstate->perhash[0].hashtable->instrument : NULL);
+						   aggstate->num_hashes ? &aggstate->perhash[0].instrument : NULL);
 
 	foreach(lc, agg->chain)
 	{
@@ -2344,7 +2344,7 @@ show_grouping_sets(AggState *aggstate, Agg *agg,
 				aggnode->aggstrategy == AGG_MIXED) {
 			int	nth = list_cell_number(agg->chain, lc);
 			Assert(nth < aggstate->num_hashes);
-			inst = &aggstate->perhash[nth].hashtable->instrument;
+			inst = &aggstate->perhash[nth].instrument;
 		}
 		else
 			inst = NULL;
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index dcc365c..de0205f 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -191,7 +191,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
@@ -207,7 +206,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
@@ -286,32 +284,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 81dda1b..396446b 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1268,6 +1268,7 @@ build_hash_table(AggState *aggstate)
 		if (perhash->hashtable)
 			ResetTupleHashTable(perhash->hashtable);
 		else
+		{
 			perhash->hashtable = BuildTupleHashTableExt(&aggstate->ss.ps,
 														perhash->hashslot->tts_tupleDescriptor,
 														perhash->numCols,
@@ -1281,6 +1282,8 @@ build_hash_table(AggState *aggstate)
 														aggstate->hashcontext->ecxt_per_tuple_memory,
 														tmpmem,
 														DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit));
+			InitTupleHashTableStats(perhash->instrument, perhash->hashtable->hashtab, additionalsize);
+		}
 	}
 }
 
@@ -1666,7 +1669,7 @@ 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);
 				select_current_set(aggstate, 0, true);
@@ -1869,7 +1872,7 @@ 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);
 				}
 			}
 
@@ -1947,7 +1950,7 @@ 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 93272c2..594abdb 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 9c0e0ab..4a56290 100644
--- a/src/backend/executor/nodeSetOp.c
+++ b/src/backend/executor/nodeSetOp.c
@@ -139,6 +139,7 @@ build_hash_table(SetOpState *setopstate)
 												   setopstate->tableContext,
 												   econtext->ecxt_per_tuple_memory,
 												   false);
+	InitTupleHashTableStats(setopstate->instrument, setopstate->hashtable->hashtab, 0);
 }
 
 /*
@@ -415,7 +416,7 @@ 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 eec849c..a5b71fa 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -507,6 +507,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 	if (node->hashtable)
 		ResetTupleHashTable(node->hashtable);
 	else
+	{
 		node->hashtable = BuildTupleHashTableExt(node->parent,
 												 node->descRight,
 												 ncols,
@@ -520,6 +521,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 												 node->hashtablecxt,
 												 node->hashtempcxt,
 												 false);
+		InitTupleHashTableStats(node->instrument, node->hashtable->hashtab, 0);
+	}
 
 	if (!subplan->unknownEqFalse)
 	{
@@ -534,7 +537,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 
 		if (node->hashnulls)
 			ResetTupleHashTable(node->hashtable);
-		else
+		else {
 			node->hashnulls = BuildTupleHashTableExt(node->parent,
 													 node->descRight,
 													 ncols,
@@ -548,6 +551,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
 													 node->hashtablecxt,
 													 node->hashtempcxt,
 													 false);
+			InitTupleHashTableStats(node->instrument_nulls, node->hashnulls->hashtab, 0);
+		}
 	}
 
 	/*
@@ -621,9 +626,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 34199b5..81fdfa4 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 264916f..008fda3 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. */
+	hash_instrumentation    instrument;
 }			AggStatePerHashData;
 
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index f5740c7..df56330 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -688,9 +688,26 @@ 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)
+
 /* XXX: not to be confused with struct HashInstrumentation... */
 typedef struct hash_instrumentation
 {
+	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 */
@@ -715,7 +732,6 @@ typedef struct TupleHashTableData
 	ExprState  *cur_eq_func;	/* comparator for input vs. table */
 	uint32		hash_iv;		/* hash-function IV */
 	ExprContext *exprcontext;	/* expression context */
-	hash_instrumentation instrument;
 }			TupleHashTableData;
 
 typedef tuplehash_iterator TupleHashIterator;
@@ -881,6 +897,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 */
+	hash_instrumentation instrument;
+	hash_instrumentation instrument_nulls; /* instrumentation for nulls hashtable */
 } SubPlanState;
 
 /* ----------------
@@ -1289,6 +1307,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 */
+	hash_instrumentation	instrument;
 } RecursiveUnionState;
 
 /* ----------------
@@ -2322,6 +2341,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 */
+	hash_instrumentation instrument;
 } SetOpState;
 
 /* ----------------
-- 
2.7.4

