Fix memory leakage in nodeSubplan.c.

If the hash functions used for hashing tuples leaked 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

diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index ba4f238ed9fb08c4ba949cd09d035c0810494f0d..dcb0c23a284d76b868e08edc1a1560f8c0b2e21a 100644 (file)
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -149,6 +149,12 @@ execTuplesHashPrepare(int numCols,
  *
  * Note that keyColIdx, eqfunctions, and hashfunctions 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
 BuildTupleHashTableExt(PlanState *parent,

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index c136f75ac2493acaeca826a3dab62e74ef5c602b..2a777c78007f6f54615387a11e4186c3fda0027d 100644 (file)
--- 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_funcs) != 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);
 }
 
 /*
@@ -643,6 +624,9 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
         * during ExecProject.
         */
        ResetExprContext(innerecontext);
+
+       /* Also must reset the hashtempcxt after each hashtable lookup. */
+       MemoryContextReset(node->hashtempcxt);
    }
 
    /*