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 <t...@sss.pgh.pa.us> wrote: > Ajit Awekar <ajit.awe...@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 >
memoryleakfix_V1.patch
Description: Binary data
