From c106ef08071ad611fdf4febb3a8d2da441272a6d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 30 Oct 2025 11:21:22 -0400 Subject: Use BumpContext contexts in TupleHashTables, and do some code cleanup. For all extant uses of TupleHashTables, execGrouping.c itself does nothing with the "tablecxt" except to allocate new hash entries in it, and the callers do nothing with it except to reset the whole context. So this is an ideal use-case for a BumpContext, and the hash tables are frequently big enough for the savings to be significant. (Commit cc721c459 already taught nodeAgg.c this idea, but neglected the other callers of BuildTupleHashTable.) While at it, let's clean up some ill-advised leftovers from rebasing TupleHashTables on simplehash.h: * Many comments and variable names were based on the idea that the tablecxt holds the whole TupleHashTable, whereas now it only holds the hashed tuples (plus any caller-defined "additional storage"). Rename to names like tuplescxt and tuplesContext, and adjust the comments. Also adjust the memory context names to be like " hashed tuples". * Make ResetTupleHashTable() reset the tuplescxt rather than relying on the caller to do so; that was fairly bizarre and seems like a recipe for leaks. This is less efficient in the case where nodeAgg.c uses the same tuplescxt for several different hashtables, but only microscopically so because mcxt.c will short-circuit the extra resets via its isReset flag. I judge the extra safety and intellectual cleanliness well worth those few cycles. * Remove the long-obsolete "allow_jit" check added by ac88807f9; instead, just Assert that metacxt and tuplescxt are different. We need that anyway for this definition of ResetTupleHashTable() to be safe. There is a side issue of the extent to which this change invalidates the planner's estimates of hashtable memory consumption. However, those estimates are already pretty bad, so improving them seems like it can be a separate project. This change is useful to do first to establish consistent executor behavior that the planner can expect. A loose end not addressed here is that the "entrysize" calculation in BuildTupleHashTable seems wrong: "sizeof(TupleHashEntryData) + additionalsize" corresponds neither to the size of the simplehash entries nor to the total space needed per tuple. It's questionable why BuildTupleHashTable is second-guessing its caller's nbuckets choice at all, since the original source of the number should have had more information. But that all seems wrapped up with the planner's estimation logic, so let's leave it for the planned followup patch. Reported-by: Jeff Janes Reported-by: David Rowley Author: Tom Lane Reviewed-by: David Rowley Discussion: https://postgr.es/m/CAMkU=1zia0JfW_QR8L5xA2vpa0oqVuiapm78h=WpNsHH13_9uw@mail.gmail.com Discussion: https://postgr.es/m/2268409.1761512111@sss.pgh.pa.us --- src/backend/executor/nodeRecursiveunion.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) (limited to 'src/backend/executor/nodeRecursiveunion.c') diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c index 40f66fd0680..ebb7919b49b 100644 --- a/src/backend/executor/nodeRecursiveunion.c +++ b/src/backend/executor/nodeRecursiveunion.c @@ -53,7 +53,7 @@ build_hash_table(RecursiveUnionState *rustate) node->numGroups, 0, rustate->ps.state->es_query_cxt, - rustate->tableContext, + rustate->tuplesContext, rustate->tempContext, false); } @@ -197,7 +197,7 @@ ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags) rustate->hashfunctions = NULL; rustate->hashtable = NULL; rustate->tempContext = NULL; - rustate->tableContext = NULL; + rustate->tuplesContext = NULL; /* initialize processing state */ rustate->recursing = false; @@ -209,7 +209,8 @@ ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags) * If hashing, we need a per-tuple memory context for comparisons, and a * longer-lived context to store the hash table. The table can't just be * kept in the per-query context because we want to be able to throw it - * away when rescanning. + * away when rescanning. We can use a BumpContext to save storage, + * because we will have no need to delete individual table entries. */ if (node->numCols > 0) { @@ -217,10 +218,10 @@ ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags) AllocSetContextCreate(CurrentMemoryContext, "RecursiveUnion", ALLOCSET_DEFAULT_SIZES); - rustate->tableContext = - AllocSetContextCreate(CurrentMemoryContext, - "RecursiveUnion hash table", - ALLOCSET_DEFAULT_SIZES); + rustate->tuplesContext = + BumpContextCreate(CurrentMemoryContext, + "RecursiveUnion hashed tuples", + ALLOCSET_DEFAULT_SIZES); } /* @@ -288,11 +289,11 @@ ExecEndRecursiveUnion(RecursiveUnionState *node) tuplestore_end(node->working_table); tuplestore_end(node->intermediate_table); - /* free subsidiary stuff including hashtable */ + /* free subsidiary stuff including hashtable data */ if (node->tempContext) MemoryContextDelete(node->tempContext); - if (node->tableContext) - MemoryContextDelete(node->tableContext); + if (node->tuplesContext) + MemoryContextDelete(node->tuplesContext); /* * close down subplans @@ -328,10 +329,6 @@ ExecReScanRecursiveUnion(RecursiveUnionState *node) if (outerPlan->chgParam == NULL) ExecReScan(outerPlan); - /* Release any hashtable storage */ - if (node->tableContext) - MemoryContextReset(node->tableContext); - /* Empty hashtable if needed */ if (plan->numCols > 0) ResetTupleHashTable(node->hashtable); -- cgit v1.2.3