Re: Memory leak in CachememoryContext

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 <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Ajit Awekar <ajit(dot)awekar(at)enterprisedb(dot)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
>
>