Re: SQLFunctionCache and generic plans

Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> writes:
> Now sql functions plans are actually saved. The most of it is a
> simplified version of plpgsql plan cache. Perhaps, I've missed
> something.

A couple of thoughts about v6:

* I don't think it's okay to just summarily do this:

/* It's stale; unlink and delete */
fcinfo->flinfo->fn_extra = NULL;
MemoryContextDelete(fcache->fcontext);
fcache = NULL;

when fmgr_sql sees that the cache is stale. If we're
doing a nested call of a recursive SQL function, this'd be
cutting the legs out from under the outer recursion level.
plpgsql goes to some lengths to do reference-counting of
function cache entries, and I think you need the same here.

* I don't like much of anything about 0004. It's messy and it
gives up all the benefit of plan caching in some pretty-common
cases (anywhere where the user was sloppy about what data type
is being returned). I wonder if we couldn't solve that with
more finesse by applying check_sql_fn_retval() to the query tree
after parse analysis, but before we hand it to plancache.c?
This is different from what happens now because we'd be applying
it before not after query rewrite, but I don't think that
query rewrite ever changes the targetlist results. Another
point is that the resultTargetList output might change subtly,
but I don't think we care there either: I believe we only examine
that output for its result data types and resjunk markers.
(This is nonobvious and inadequately documented, but for sure we
couldn't try to execute that tlist --- it's never passed through
the planner.)

* One diff that caught my eye was

- if (!ActiveSnapshotSet() &&
- plansource->raw_parse_tree &&
- analyze_requires_snapshot(plansource->raw_parse_tree))
+ if (!ActiveSnapshotSet() && StmtPlanRequiresRevalidation(plansource))

Because StmtPlanRequiresRevalidation uses
stmt_requires_parse_analysis, this is basically throwing away the
distinction between stmt_requires_parse_analysis and
analyze_requires_snapshot. I do not think that's okay, for the
reasons explained in analyze_requires_snapshot. We should expend the
additional notational overhead to keep those things separate.

* I'm also not thrilled by teaching StmtPlanRequiresRevalidation
how to do something equivalent to stmt_requires_parse_analysis for
Query trees. The entire point of the current division of labor is
for it *not* to know that, but keep the knowledge of the properties
of different statement types in parser/analyze.c. So it looks to me
like we need to add a function to parser/analyze.c that does this.
Not quite sure what to call it though.
querytree_requires_parse_analysis() would be a weird name, because
if it's a Query tree then it's already been through parse analysis.
Maybe querytree_requires_revalidation()?

regards, tom lane