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