Re: SQLFunctionCache and generic plans

I spent some time today going through the actual code in this patch.
I realized that there's no longer any point in 0001: the later patches
don't move or repeatedly-call that bit of code, so it can be left as-is.

What I think we could stand to split out, though, is the changes in
the plancache support. The new 0001 attached is just the plancache
and analyze.c changes. That could be committed separately, although
of course there's little point in pushing it till we're happy with
the rest.

In general, this patch series is paying far too little attention to
updating existing comments that it obsoletes or adding new ones
explaining what's going on. For example, the introductory comment
for struct SQLFunctionCache still says

* Note that currently this has only the lifespan of the calling query.
* Someday we should rewrite this code to use plancache.c to save parse/plan
* results for longer than that.

and I wonder how much of the para after that is still accurate either.
The new structs aren't adequately documented either IMO. We now have
about three different structs that have something to do with caches
by their names, but the reader is left to guess how they fit together.
Another example is that the header comment for init_execution_state
still describes an argument list it hasn't got anymore.

I tried to clean up the comment situation in the plancache in 0001,
but I've not done much of anything to functions.c.

I'm fairly confused why 0002 and 0003 are separate patches, and the
commit messages for them do nothing to clarify that. It seems like
you're expecting reviewers to review a very transitory state of
affairs in 0002, and it's not clear why. Maybe the code is fine
and you just need to explain the change sequence a bit more
in the commit messages. 0002 could stand to explain the point
of the new test cases, too, especially since one of them seems to
be demonstrating the fixing of a pre-existing bug.

Something is very wrong in 0004: it should not be breaking that
test case in test_extensions. It seems to me we should already
have the necessary infrastructure for that, in that the plan
ought to have a PlanInvalItem referencing public.dep_req2(),
and the ALTER SET SCHEMA that gets done on that function should
result in an invalidation. So it looks to me like that patch
has somehow rearranged things so we miss an invalidation.
I've not tried to figure out why. I'm also sad that 0004
doesn't appear to include any test cases showing it doing
something right: without that, why do it at all?

regards, tom lane