Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset - Mailing list pgsql-bugs

I wrote:
> I thought about that too, but that would result in two short-lived
> contexts and two reset operations per tuple cycle where only one
> is needed.  I'm rather tempted to fix nodeSubplan.c by making it
> use innerecontext->ecxt_per_tuple_memory instead of a separate
> hash tmpcontext.  That context is getting reset already, at least in
> buildSubPlanHash().  That's probably too risky for the back branches
> but we could do it in HEAD.

Concretely, I'm thinking about the attached.  0001 is the same
logic as in the v02 patch, but I felt we could make the code
be shorter and prettier instead of longer and uglier.  That's
meant for back-patch, and then 0002 is for master only.

            regards, tom lane

From 498a709bbb1e4942e15c8b1c82b181d9d2d3a686 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 8 Sep 2025 16:22:29 -0400
Subject: [PATCH v04 1/2] Fix memory leakage in nodeSubplan.c.

If the hash functions used for hashing tuples leak any memory,
we failed to clean that up, resulting in query-lifespan memory
leakage in queries using hashed subplans.  One way that could
happen is if the values being hashed require de-toasting, since
most of our hash functions don't trouble to clean up de-toasted
inputs.

Prior to commit bf6c614a2, this leakage was largely masked
because TupleHashTableMatch would reset hashtable->tempcxt
(via execTuplesMatch).  But it doesn't do that anymore, and
that's not really the right place for this anyway: doing it
there could reset the tempcxt many times per hash lookup, or
not at all.  Instead put reset calls into ExecHashSubPlan
and buildSubPlanHash.  Along the way to that, rearrange
ExecHashSubPlan so that there's just one place to call
MemoryContextReset instead of several.

This amounts to accepting the de-facto API spec that the caller
of the TupleHashTable routines is responsible for resetting the
tempcxt adequately often.  Although the other callers seem to
get this right, it was not documented anywhere, so add a comment
about it.

Bug: #19040
Reported-by: Haiyang Li <mohen.lhy@alibaba-inc.com>
Author: Haiyang Li <mohen.lhy@alibaba-inc.com>
Reviewed-by: Fei Changhong <feichanghong@qq.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19040-c9b6073ef814f48c@postgresql.org
Backpatch-through: 13
---
 src/backend/executor/execGrouping.c |  6 +++
 src/backend/executor/nodeSubplan.c  | 70 +++++++++++------------------
 2 files changed, 33 insertions(+), 43 deletions(-)

diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index b5400749353..75087204f0c 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -156,6 +156,12 @@ execTuplesHashPrepare(int numCols,
  *
  * Note that the keyColIdx, hashfunctions, and collations arrays must be
  * allocated in storage that will live as long as the hashtable does.
+ *
+ * LookupTupleHashEntry, FindTupleHashEntry, and related functions may leak
+ * memory in the tempcxt.  It is caller's responsibility to reset that context
+ * reasonably often, typically once per tuple.  (We do it that way, rather
+ * than managing an extra context within the hashtable, because in many cases
+ * the caller can specify a tempcxt that it needs to reset per-tuple anyway.)
  */
 TupleHashTable
 BuildTupleHashTable(PlanState *parent,
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index f7f6fc2da0b..8e55dcc159b 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -102,6 +102,7 @@ ExecHashSubPlan(SubPlanState *node,
                 ExprContext *econtext,
                 bool *isNull)
 {
+    bool        result = false;
     SubPlan    *subplan = node->subplan;
     PlanState  *planstate = node->planstate;
     TupleTableSlot *slot;
@@ -132,14 +133,6 @@ ExecHashSubPlan(SubPlanState *node,
     node->projLeft->pi_exprContext = econtext;
     slot = ExecProject(node->projLeft);

-    /*
-     * Note: because we are typically called in a per-tuple context, we have
-     * to explicitly clear the projected tuple before returning. Otherwise,
-     * we'll have a double-free situation: the per-tuple context will probably
-     * be reset before we're called again, and then the tuple slot will think
-     * it still needs to free the tuple.
-     */
-
     /*
      * If the LHS is all non-null, probe for an exact match in the main hash
      * table.  If we find one, the result is TRUE. Otherwise, scan the
@@ -161,19 +154,10 @@ ExecHashSubPlan(SubPlanState *node,
                                slot,
                                node->cur_eq_comp,
                                node->lhs_hash_expr) != NULL)
-        {
-            ExecClearTuple(slot);
-            return BoolGetDatum(true);
-        }
-        if (node->havenullrows &&
-            findPartialMatch(node->hashnulls, slot, node->cur_eq_funcs))
-        {
-            ExecClearTuple(slot);
+            result = true;
+        else if (node->havenullrows &&
+                 findPartialMatch(node->hashnulls, slot, node->cur_eq_funcs))
             *isNull = true;
-            return BoolGetDatum(false);
-        }
-        ExecClearTuple(slot);
-        return BoolGetDatum(false);
     }

     /*
@@ -186,34 +170,31 @@ ExecHashSubPlan(SubPlanState *node,
      * aren't provably unequal to the LHS; if so, the result is UNKNOWN.
      * Otherwise, the result is FALSE.
      */
-    if (node->hashnulls == NULL)
-    {
-        ExecClearTuple(slot);
-        return BoolGetDatum(false);
-    }
-    if (slotAllNulls(slot))
-    {
-        ExecClearTuple(slot);
+    else if (node->hashnulls == NULL)
+         /* just return FALSE */ ;
+    else if (slotAllNulls(slot))
         *isNull = true;
-        return BoolGetDatum(false);
-    }
     /* Scan partly-null table first, since more likely to get a match */
-    if (node->havenullrows &&
-        findPartialMatch(node->hashnulls, slot, node->cur_eq_funcs))
-    {
-        ExecClearTuple(slot);
+    else if (node->havenullrows &&
+             findPartialMatch(node->hashnulls, slot, node->cur_eq_funcs))
         *isNull = true;
-        return BoolGetDatum(false);
-    }
-    if (node->havehashrows &&
-        findPartialMatch(node->hashtable, slot, node->cur_eq_funcs))
-    {
-        ExecClearTuple(slot);
+    else if (node->havehashrows &&
+             findPartialMatch(node->hashtable, slot, node->cur_eq_funcs))
         *isNull = true;
-        return BoolGetDatum(false);
-    }
+
+    /*
+     * Note: because we are typically called in a per-tuple context, we have
+     * to explicitly clear the projected tuple before returning. Otherwise,
+     * we'll have a double-free situation: the per-tuple context will probably
+     * be reset before we're called again, and then the tuple slot will think
+     * it still needs to free the tuple.
+     */
     ExecClearTuple(slot);
-    return BoolGetDatum(false);
+
+    /* Also must reset the hashtempcxt after each hashtable lookup. */
+    MemoryContextReset(node->hashtempcxt);
+
+    return BoolGetDatum(result);
 }

 /*
@@ -642,6 +623,9 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
          * during ExecProject.
          */
         ResetExprContext(innerecontext);
+
+        /* Also must reset the hashtempcxt after each hashtable lookup. */
+        MemoryContextReset(node->hashtempcxt);
     }

     /*
--
2.43.7

From 3df0846dfe41955353287b4b33caf43e1dd2e399 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 8 Sep 2025 16:41:03 -0400
Subject: [PATCH v04 2/2] Eliminate duplicative hashtempcxt in nodeSubplan.c.

Instead of building a separate memory context that's used just
for running hash functions, make the hash functions run in the
per-tuple context of the node's innerecontext.  This saves a
little space at runtime, and it avoids needing to reset two
contexts instead of one inside buildSubPlanHash's main loop.
Per discussion of bug #19040, although this is not directly
a fix for that.

Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19040-c9b6073ef814f48c@postgresql.org
---
 src/backend/executor/nodeSubplan.c | 19 +++++--------------
 src/include/nodes/execnodes.h      |  1 -
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 8e55dcc159b..53fb56f7388 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -191,8 +191,8 @@ ExecHashSubPlan(SubPlanState *node,
      */
     ExecClearTuple(slot);

-    /* Also must reset the hashtempcxt after each hashtable lookup. */
-    MemoryContextReset(node->hashtempcxt);
+    /* Also must reset the innerecontext after each hashtable lookup. */
+    ResetExprContext(node->innerecontext);

     return BoolGetDatum(result);
 }
@@ -529,7 +529,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
                                               0,
                                               node->planstate->state->es_query_cxt,
                                               node->hashtablecxt,
-                                              node->hashtempcxt,
+                                              innerecontext->ecxt_per_tuple_memory,
                                               false);

     if (!subplan->unknownEqFalse)
@@ -558,7 +558,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
                                                   0,
                                                   node->planstate->state->es_query_cxt,
                                                   node->hashtablecxt,
-                                                  node->hashtempcxt,
+                                                  innerecontext->ecxt_per_tuple_memory,
                                                   false);
     }
     else
@@ -620,12 +620,9 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)

         /*
          * Reset innerecontext after each inner tuple to free any memory used
-         * during ExecProject.
+         * during ExecProject and hashtable lookup.
          */
         ResetExprContext(innerecontext);
-
-        /* Also must reset the hashtempcxt after each hashtable lookup. */
-        MemoryContextReset(node->hashtempcxt);
     }

     /*
@@ -842,7 +839,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
     sstate->hashtable = NULL;
     sstate->hashnulls = NULL;
     sstate->hashtablecxt = NULL;
-    sstate->hashtempcxt = NULL;
     sstate->innerecontext = NULL;
     sstate->keyColIdx = NULL;
     sstate->tab_eq_funcoids = NULL;
@@ -898,11 +894,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
             AllocSetContextCreate(CurrentMemoryContext,
                                   "Subplan HashTable Context",
                                   ALLOCSET_DEFAULT_SIZES);
-        /* and a small one for the hash tables to use as temp storage */
-        sstate->hashtempcxt =
-            AllocSetContextCreate(CurrentMemoryContext,
-                                  "Subplan HashTable Temp Context",
-                                  ALLOCSET_SMALL_SIZES);
         /* and a short-lived exprcontext for function evaluation */
         sstate->innerecontext = CreateExprContext(estate);

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index de782014b2d..71857feae48 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1020,7 +1020,6 @@ typedef struct SubPlanState
     bool        havehashrows;    /* true if hashtable is not empty */
     bool        havenullrows;    /* true if hashnulls is not empty */
     MemoryContext hashtablecxt; /* memory context containing hash tables */
-    MemoryContext hashtempcxt;    /* temp memory context for hash tables */
     ExprContext *innerecontext; /* econtext for computing inner tuples */
     int            numCols;        /* number of columns being hashed */
     /* each of the remaining fields is an array of length numCols: */
--
2.43.7