Ajit Awekar <ajit.awekar@enterprisedb.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
