Re: Memory leak in CachememoryContext

Hi Tom,

Thanks a lot for your possible approach for a solution.
I have implemented the approach by splitting the hash table into two parts.
Please find the attached patch for the same.

Thanks & Best Regards,
Ajit

On Wed, Apr 19, 2023 at 10:13 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Ajit Awekar <ajit(dot)awekar(at)enterprisedb(dot)com> writes:
> > Please find below simple repro for CacheMemoryContext memory leak
>
> Hm, yeah, reproduced here.
>
> > During anonymous block execution in the function "plpgsql_estate_setup",
> a
> > local casting hash table gets created in SPI memory context. When hash
> > table look up is performed in "get_cast_hashenty" function if entry is no
> > present , memory is allocated in CacheMemoryContext in function
> > "GetCachedExpression".At the end of proc execution SPI memory context is
> > deleted and hence local hash table gets deleted, but still entries remain
> > in Cachemeorycontext.
>
> Yeah, it's from using just a short-lived cast hash table for DO blocks.
> I think that was okay when it was written, but when we wheeled the
> CachedExpression machinery undeneath it, we created a problem.
>
> > Please find attached(memoryleakfix.patch) to this email.
>
> I don't think this fix is acceptable at all. A minor problem is that
> we can't change the API of GetCachedExpression() in stable branches,
> because extensions may be using it. We could work around that by
> making it a wrapper function. But the big problem is that this patch
> destroys the reason for using a CachedExpression in the first place:
> because you aren't linking it into cached_expression_list, the plancache
> will not detect events that should obsolete the expression. The
> test cases added by 04fe805a1 only cover regular functions, but one
> that did domain constraint DDL within a DO block would expose the
> shortcoming.
>
> A possible answer is to split plpgsql's cast hash table into two parts.
> The lower-level part would contain the hash key, cast_expr and
> cast_cexpr fields, and would have session lifespan and be used by
> both DO blocks and regular functions. In this way we'd not leak
> CachedExpressions. The upper-level hash table would contain the
> hash key, a link to the relevant lower-level entry, and the
> cast_exprstate, cast_in_use, cast_lxid fields. There would be a
> session-lifespan one of these plus one for each DO block, so that
> management of the ExprStates still works as it does now for DO blocks.
>
> This could be factored in other ways, and maybe another way would be
> simpler. But the idea is that DO blocks should use persistent
> CachedExpressions even though their cast_exprstates are transient.
>
> I've not tried to code this, do you want to?
>
> regards, tom lane
>