Re: Memory leak in CachememoryContext

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