Re: BUG #19040: Memory leak in hashed subplan node due to missing hashtempcxt reset

> On Sep 9, 2025, at 04:46, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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

The v04-0001 looks good for me. I am also considering whether there is a way to
detect a memory leak. One preliminary idea is that for short-lived context such
as the "Subplan HashTable Temp Context", we can assume that MemoryContextReset
will be called frequently. Therefore, at the time of deletion, the memory usage
should not be excessively large. Based on this assumption, we could implement
the following check:
```
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 15fa4d0a55e..56218cb6863 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -166,6 +166,7 @@ static void MemoryContextStatsInternal(MemoryContext context, int level,
static void MemoryContextStatsPrint(MemoryContext context, void *passthru,
const char *stats_string,
bool print_to_stderr);
+static bool CheckMemoryContextLeak(MemoryContext context);

/*
* You should not do memory allocations within a critical section, because
@@ -502,6 +503,7 @@ MemoryContextDeleteOnly(MemoryContext context)
Assert(context != CurrentMemoryContext);
/* All the children should've been deleted already */
Assert(context->firstchild == NULL);
+ Assert(CheckMemoryContextLeak(context));

/*
* It's not entirely clear whether 'tis better to do this before or after
@@ -530,6 +532,24 @@ MemoryContextDeleteOnly(MemoryContext context)
VALGRIND_DESTROY_MEMPOOL(context);
}

+static bool
+CheckMemoryContextLeak(MemoryContext context)
+{
+#ifdef MEMORY_CONTEXT_CHECKING
+ if (!context->name)
+ return true;
+
+ if (!strcmp(context->name, "Subplan HashTable Temp Context") == 0)
+ return true;
+
+ /* The size of short-lived contexts should be kept under 1 MB. */
+ if ((MemoryContextMemAllocated(context, false) > 1024 * 1024))
+ return false;
+#endif
+ return true;
+}
+
+
/*
* MemoryContextDeleteChildren
* Delete all the descendants of the named context and release all
```

In debug mode, a memory leak can be easily detected with the following SQL.
After applying v04-0001, it runs normally:
```sql
with s as (
select
i::numeric as a
from
generate_series(1, 50000) i
)
select * from s where a not in (select * from s)
```

For v04-0002, I checked the commit history, and before commit 133924e1,
buildSubPlanHash was using ecxt_per_tuple_memory. It seems that the
"Subplan HashTable Temp Context" was introduced in order to fix a certain bug.

Best Regards,
Fei Changhong