agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Justin Pryzby <pryzbyj@telsasoft.com>
Subject: [PATCH v7 5/9] Refactor for consistency/symmetry
Date: Sun, 9 Feb 2020 15:08:14 -0600
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 | 28 -----------------------
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(+), 48 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e0dc5a092a..cef19c684b 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 844dd3ba86..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,35 +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
- {
- /* hashtable->entrysize includes additionalsize */
- hashtable->instrument.space_peak_hash = Max(
- hashtable->instrument.space_peak_hash,
- hashtable->hashtab->size * sizeof(TupleHashEntryData));
-
- hashtable->instrument.space_peak_tuples = Max(
- hashtable->instrument.space_peak_tuples,
- hashtable->hashtab->members * hashtable->entrysize);
- }
-}
-
/*
* 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 e5aa0629a7..867da6eebf 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 f4f2ede207..94890512dc 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
--UfEAyuTBtIjiZzX6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0006-TupleHashTable.entrysize-was-unused-except-for-in.patch"
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzbyj@telsasoft.com
Subject: Re: [PATCH v7 5/9] Refactor for consistency/symmetry
In-Reply-To: <no-message-id-709841@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox