Tom, Thanks a lot for your patch. I applied the changes and confirmed there is no memory leak with the V2 patch. We are not using MemoryContext variables "cast_hash_context" and "shared_cast_context".
Thanks & Best Regards, Ajit On Sat, Apr 22, 2023 at 4:49 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Ajit Awekar <ajit.awe...@enterprisedb.com> writes: > > I have implemented the approach by splitting the hash table into two > parts. > > Please find the attached patch for the same. > > I found a few things not to like about this: > > * You didn't update the comment describing these hash tables. > > * I wasn't really thrilled about renaming the plpgsql_CastHashEntry > typedef, as that seemed to just create uninteresting diff noise. > Also, "SessionCastHashEntry" versus "PrivateCastHashEntry" seems a > very misleading choice of names, since one of the "PrivateCastHashEntry" > hash tables is in fact session-lifespan. After some thought I left > the "upper" hash table entry type as plpgsql_CastHashEntry so that > code outside the immediate area needn't be affected, and named the > "lower" table cast_expr_hash, with entry type plpgsql_CastExprHashEntry. > I'm not wedded to those names though, if you have a better idea. > > (BTW, it's completely reasonable to rename the type as an intermediate > step in making a patch like this, since it ensures you'll examine > every existing usage to choose the right thing to change it to. But > I generally rename things back afterwards.) > > * I didn't like having to do two hashtable lookups during every > call even after we've fully cached the info. That's easy to avoid > by keeping a link to the associated "lower" hashtable entry in the > "upper" ones. > > * You removed the reset of cast_exprstate etc from the code path where > we've just reconstructed the cast_expr. That's a mistake since it > might allow us to skip rebuilding the derived expression state after > a DDL change. > > > Also, while looking at this I noticed that we are no longer making > any use of estate->cast_hash_context. That's not the fault of > your patch; it's another oversight in the one that added the > CachedExpression mechanism. The compiled expressions used to be > stored in that context, but now the plancache is responsible for > them and we are never putting anything in the cast_hash_context. > So we might as well get rid of that and save 8K of wasted memory. > This allows some simplification in the hashtable setup code too. > > In short, I think we need something more like the attached. > > (Note to self: we can't remove the cast_hash_context field in > back branches for fear of causing an ABI break for pldebugger. > But we can leave it unused, I think.) > > regards, tom lane > >
