Re: Converting SetOp to read its two inputs separately - Mailing list pgsql-hackers
| From | Tom Lane |
|---|---|
| Subject | Re: Converting SetOp to read its two inputs separately |
| Date | |
| Msg-id | 538343.1734646986@sss.pgh.pa.us Whole thread Raw |
| In response to | Re: Converting SetOp to read its two inputs separately (Tom Lane <tgl@sss.pgh.pa.us>) |
| Responses |
Re: Converting SetOp to read its two inputs separately
|
| List | pgsql-hackers |
Pushed ... and now I have one more beef about the way things are
in this area. I don't think we should leave the compatibility
function BuildTupleHashTable() in place in HEAD. Making it a
wrapper around a new function BuildTupleHashTableExt() was a fine
solution for preserving ABI in released branches, but that doesn't
mean we should clutter the code with unused ABI hacks forevermore.
Attached is a patch to take it out and then rename
BuildTupleHashTableExt() back to BuildTupleHashTable().
Since BuildTupleHashTableExt has already grown more arguments
in HEAD than it had in v17, renaming it doesn't increase the number of
places that will have to be touched in any extensions that were using
this infrastructure. Removal of the compatibility wrapper could force
some code updates, but really we want those places to update anyway.
I also made an effort at fixing the woefully out of date
header comment for it.
regards, tom lane
From 322de6ae7d91c28e97f31ba1aed878181bcdc5aa Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 19 Dec 2024 15:25:57 -0500
Subject: [PATCH v1] Get rid of old version of BuildTupleHashTable().
It was reasonable to preserve the old API of BuildTupleHashTable()
in the back branches, but in HEAD we should actively discourage use
of that version. There are no remaining callers in core, so just
get rid of it. Then rename BuildTupleHashTableExt() back to
BuildTupleHashTable().
While at it, fix up the miserably-poorly-maintained header comment
for BuildTupleHashTable[Ext]. It looks like more than one patch in
this area has had the opinion that updating comments is beneath them.
---
src/backend/executor/execGrouping.c | 93 +++++++++--------------
src/backend/executor/nodeAgg.c | 28 +++----
src/backend/executor/nodeRecursiveunion.c | 30 ++++----
src/backend/executor/nodeSetOp.c | 30 ++++----
src/backend/executor/nodeSubplan.c | 58 +++++++-------
src/include/executor/executor.h | 22 ++----
6 files changed, 115 insertions(+), 146 deletions(-)
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 7491e53c03..0aa9e92ad9 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -135,36 +135,43 @@ execTuplesHashPrepare(int numCols,
/*
* Construct an empty TupleHashTable
*
- * inputOps: slot ops for input hash values, or NULL if unknown or not fixed
- * numCols, keyColIdx: identify the tuple fields to use as lookup key
- * eqfunctions: equality comparison functions to use
- * hashfunctions: datatype-specific hashing functions to use
+ * parent: PlanState node that will own this hash table
+ * inputDesc: tuple descriptor for input tuples
+ * inputOps: slot ops for input tuples, or NULL if unknown or not fixed
+ * numCols: number of columns to be compared (length of next 4 arrays)
+ * keyColIdx: indexes of tuple columns to compare
+ * eqfuncoids: OIDs of equality comparison functions to use
+ * hashfunctions: FmgrInfos of datatype-specific hashing functions to use
+ * collations: collations to use in comparisons
* nbuckets: initial estimate of hashtable size
* additionalsize: size of data stored in ->additional
* metacxt: memory context for long-lived allocation, but not per-entry data
* tablecxt: memory context in which to store table entries
* tempcxt: short-lived context for evaluation hash and comparison functions
+ * use_variable_hash_iv: if true, adjust hash IV per-parallel-worker
*
- * The function arrays may be made with execTuplesHashPrepare(). Note they
+ * The hashfunctions array may be made with execTuplesHashPrepare(). Note they
* are not cross-type functions, but expect to see the table datatype(s)
* on both sides.
*
- * Note that keyColIdx, eqfunctions, and hashfunctions must be allocated in
- * storage that will live as long as the hashtable does.
+ * Note that the keyColIdx, hashfunctions, and collations arrays must be
+ * allocated in storage that will live as long as the hashtable does.
*/
TupleHashTable
-BuildTupleHashTableExt(PlanState *parent,
- TupleDesc inputDesc,
- const TupleTableSlotOps *inputOps,
- int numCols, AttrNumber *keyColIdx,
- const Oid *eqfuncoids,
- FmgrInfo *hashfunctions,
- Oid *collations,
- long nbuckets, Size additionalsize,
- MemoryContext metacxt,
- MemoryContext tablecxt,
- MemoryContext tempcxt,
- bool use_variable_hash_iv)
+BuildTupleHashTable(PlanState *parent,
+ TupleDesc inputDesc,
+ const TupleTableSlotOps *inputOps,
+ int numCols,
+ AttrNumber *keyColIdx,
+ const Oid *eqfuncoids,
+ FmgrInfo *hashfunctions,
+ Oid *collations,
+ long nbuckets,
+ Size additionalsize,
+ MemoryContext metacxt,
+ MemoryContext tablecxt,
+ MemoryContext tempcxt,
+ bool use_variable_hash_iv)
{
TupleHashTable hashtable;
Size entrysize = sizeof(TupleHashEntryData) + additionalsize;
@@ -216,14 +223,14 @@ BuildTupleHashTableExt(PlanState *parent,
&TTSOpsMinimalTuple);
/*
- * If the old reset interface is used (i.e. BuildTupleHashTable, rather
- * than BuildTupleHashTableExt), allowing JIT would lead to the generated
- * functions to a) live longer than the query b) be re-generated each time
- * the table is being reset. Therefore prevent JIT from being used in that
- * case, by not providing a parent node (which prevents accessing the
- * JitContext in the EState).
+ * If the caller fails to make the metacxt different from the tablecxt,
+ * allowing JIT would lead to the generated functions to a) live longer
+ * than the query or b) be re-generated each time the table is being
+ * reset. Therefore prevent JIT from being used in that case, by not
+ * providing a parent node (which prevents accessing the JitContext in the
+ * EState).
*/
- allow_jit = metacxt != tablecxt;
+ allow_jit = (metacxt != tablecxt);
/* build hash ExprState for all columns */
hashtable->tab_hash_expr = ExecBuildHash32FromAttrs(inputDesc,
@@ -256,41 +263,9 @@ BuildTupleHashTableExt(PlanState *parent,
return hashtable;
}
-/*
- * BuildTupleHashTable is a backwards-compatibility wrapper for
- * BuildTupleHashTableExt(), that allocates the hashtable's metadata in
- * tablecxt. Note that hashtables created this way cannot be reset leak-free
- * with ResetTupleHashTable().
- */
-TupleHashTable
-BuildTupleHashTable(PlanState *parent,
- TupleDesc inputDesc,
- int numCols, AttrNumber *keyColIdx,
- const Oid *eqfuncoids,
- FmgrInfo *hashfunctions,
- Oid *collations,
- long nbuckets, Size additionalsize,
- MemoryContext tablecxt,
- MemoryContext tempcxt,
- bool use_variable_hash_iv)
-{
- return BuildTupleHashTableExt(parent,
- inputDesc,
- NULL,
- numCols, keyColIdx,
- eqfuncoids,
- hashfunctions,
- collations,
- nbuckets, additionalsize,
- tablecxt,
- tablecxt,
- tempcxt,
- use_variable_hash_iv);
-}
-
/*
* Reset contents of the hashtable to be empty, preserving all the non-content
- * state. Note that the tablecxt passed to BuildTupleHashTableExt() should
+ * state. Note that the tablecxt passed to BuildTupleHashTable() should
* also be reset, otherwise there will be leaks.
*/
void
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 53931c8285..54e7806fc0 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1518,20 +1518,20 @@ build_hash_table(AggState *aggstate, int setno, long nbuckets)
*/
additionalsize = aggstate->numtrans * sizeof(AggStatePerGroupData);
- perhash->hashtable = BuildTupleHashTableExt(&aggstate->ss.ps,
- perhash->hashslot->tts_tupleDescriptor,
- perhash->hashslot->tts_ops,
- perhash->numCols,
- perhash->hashGrpColIdxHash,
- perhash->eqfuncoids,
- perhash->hashfunctions,
- perhash->aggnode->grpCollations,
- nbuckets,
- additionalsize,
- metacxt,
- hashcxt,
- tmpcxt,
- DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit));
+ perhash->hashtable = BuildTupleHashTable(&aggstate->ss.ps,
+ perhash->hashslot->tts_tupleDescriptor,
+ perhash->hashslot->tts_ops,
+ perhash->numCols,
+ perhash->hashGrpColIdxHash,
+ perhash->eqfuncoids,
+ perhash->hashfunctions,
+ perhash->aggnode->grpCollations,
+ nbuckets,
+ additionalsize,
+ metacxt,
+ hashcxt,
+ tmpcxt,
+ DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit));
}
/*
diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c
index b577b72b50..8bbec46017 100644
--- a/src/backend/executor/nodeRecursiveunion.c
+++ b/src/backend/executor/nodeRecursiveunion.c
@@ -39,23 +39,23 @@ build_hash_table(RecursiveUnionState *rustate)
/*
* If both child plans deliver the same fixed tuple slot type, we can tell
- * BuildTupleHashTableExt to expect that slot type as input. Otherwise,
+ * BuildTupleHashTable to expect that slot type as input. Otherwise,
* we'll pass NULL denoting that any slot type is possible.
*/
- rustate->hashtable = BuildTupleHashTableExt(&rustate->ps,
- desc,
- ExecGetCommonChildSlotOps(&rustate->ps),
- node->numCols,
- node->dupColIdx,
- rustate->eqfuncoids,
- rustate->hashfunctions,
- node->dupCollations,
- node->numGroups,
- 0,
- rustate->ps.state->es_query_cxt,
- rustate->tableContext,
- rustate->tempContext,
- false);
+ rustate->hashtable = BuildTupleHashTable(&rustate->ps,
+ desc,
+ ExecGetCommonChildSlotOps(&rustate->ps),
+ node->numCols,
+ node->dupColIdx,
+ rustate->eqfuncoids,
+ rustate->hashfunctions,
+ node->dupCollations,
+ node->numGroups,
+ 0,
+ rustate->ps.state->es_query_cxt,
+ rustate->tableContext,
+ rustate->tempContext,
+ false);
}
diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c
index 09089204e8..54311d31e0 100644
--- a/src/backend/executor/nodeSetOp.c
+++ b/src/backend/executor/nodeSetOp.c
@@ -92,23 +92,23 @@ build_hash_table(SetOpState *setopstate)
/*
* If both child plans deliver the same fixed tuple slot type, we can tell
- * BuildTupleHashTableExt to expect that slot type as input. Otherwise,
+ * BuildTupleHashTable to expect that slot type as input. Otherwise,
* we'll pass NULL denoting that any slot type is possible.
*/
- setopstate->hashtable = BuildTupleHashTableExt(&setopstate->ps,
- desc,
- ExecGetCommonChildSlotOps(&setopstate->ps),
- node->numCols,
- node->cmpColIdx,
- setopstate->eqfuncoids,
- setopstate->hashfunctions,
- node->cmpCollations,
- node->numGroups,
- 0,
- setopstate->ps.state->es_query_cxt,
- setopstate->tableContext,
- econtext->ecxt_per_tuple_memory,
- false);
+ setopstate->hashtable = BuildTupleHashTable(&setopstate->ps,
+ desc,
+ ExecGetCommonChildSlotOps(&setopstate->ps),
+ node->numCols,
+ node->cmpColIdx,
+ setopstate->eqfuncoids,
+ setopstate->hashfunctions,
+ node->cmpCollations,
+ node->numGroups,
+ 0,
+ setopstate->ps.state->es_query_cxt,
+ setopstate->tableContext,
+ econtext->ecxt_per_tuple_memory,
+ false);
}
/*
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index bb4a021919..316995ca9f 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -523,7 +523,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
* Because the input slot for each hash table is always the slot resulting
* from an ExecProject(), we can use TTSOpsVirtual for the input ops. This
* saves a needless fetch inner op step for the hashing ExprState created
- * in BuildTupleHashTableExt().
+ * in BuildTupleHashTable().
*/
MemoryContextReset(node->hashtablecxt);
node->havehashrows = false;
@@ -536,20 +536,20 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
if (node->hashtable)
ResetTupleHashTable(node->hashtable);
else
- node->hashtable = BuildTupleHashTableExt(node->parent,
- node->descRight,
- &TTSOpsVirtual,
- ncols,
- node->keyColIdx,
- node->tab_eq_funcoids,
- node->tab_hash_funcs,
- node->tab_collations,
- nbuckets,
- 0,
- node->planstate->state->es_query_cxt,
- node->hashtablecxt,
- node->hashtempcxt,
- false);
+ node->hashtable = BuildTupleHashTable(node->parent,
+ node->descRight,
+ &TTSOpsVirtual,
+ ncols,
+ node->keyColIdx,
+ node->tab_eq_funcoids,
+ node->tab_hash_funcs,
+ node->tab_collations,
+ nbuckets,
+ 0,
+ node->planstate->state->es_query_cxt,
+ node->hashtablecxt,
+ node->hashtempcxt,
+ false);
if (!subplan->unknownEqFalse)
{
@@ -565,20 +565,20 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
if (node->hashnulls)
ResetTupleHashTable(node->hashnulls);
else
- node->hashnulls = BuildTupleHashTableExt(node->parent,
- node->descRight,
- &TTSOpsVirtual,
- ncols,
- node->keyColIdx,
- node->tab_eq_funcoids,
- node->tab_hash_funcs,
- node->tab_collations,
- nbuckets,
- 0,
- node->planstate->state->es_query_cxt,
- node->hashtablecxt,
- node->hashtempcxt,
- false);
+ node->hashnulls = BuildTupleHashTable(node->parent,
+ node->descRight,
+ &TTSOpsVirtual,
+ ncols,
+ node->keyColIdx,
+ node->tab_eq_funcoids,
+ node->tab_hash_funcs,
+ node->tab_collations,
+ nbuckets,
+ 0,
+ node->planstate->state->es_query_cxt,
+ node->hashtablecxt,
+ node->hashtempcxt,
+ false);
}
else
node->hashnulls = NULL;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index dee866e96b..1c7fae0930 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -131,24 +131,18 @@ extern void execTuplesHashPrepare(int numCols,
FmgrInfo **hashFunctions);
extern TupleHashTable BuildTupleHashTable(PlanState *parent,
TupleDesc inputDesc,
- int numCols, AttrNumber *keyColIdx,
+ const TupleTableSlotOps *inputOps,
+ int numCols,
+ AttrNumber *keyColIdx,
const Oid *eqfuncoids,
FmgrInfo *hashfunctions,
Oid *collations,
- long nbuckets, Size additionalsize,
+ long nbuckets,
+ Size additionalsize,
+ MemoryContext metacxt,
MemoryContext tablecxt,
- MemoryContext tempcxt, bool use_variable_hash_iv);
-extern TupleHashTable BuildTupleHashTableExt(PlanState *parent,
- TupleDesc inputDesc,
- const TupleTableSlotOps *inputOps,
- int numCols, AttrNumber *keyColIdx,
- const Oid *eqfuncoids,
- FmgrInfo *hashfunctions,
- Oid *collations,
- long nbuckets, Size additionalsize,
- MemoryContext metacxt,
- MemoryContext tablecxt,
- MemoryContext tempcxt, bool use_variable_hash_iv);
+ MemoryContext tempcxt,
+ bool use_variable_hash_iv);
extern TupleHashEntry LookupTupleHashEntry(TupleHashTable hashtable,
TupleTableSlot *slot,
bool *isnew, uint32 *hash);
--
2.43.5
pgsql-hackers by date: