Re: SQLFunctionCache and generic plans

Tom Lane писал(а) 2025-03-14 23:52:
> 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.
>

Hi.
Sorry that didn't reply immediately, was busy with another tasks.

> 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've added some comments to functions.c. Modified comments you've
spotted out.

>
> 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.

Also merged introducing plan cache to sql functions and session-level
plan cache support. Mostly they were separate for historic reasons.

>
> 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.

Plan is invalidated in both cases (before and after the patch).
What happens here is that earlier when revalidation happened, we
couldn't find renamed function.
Now function in Query is identified by its oid, and it didn't change.
So, we still can find function by oid and rebuild cached plan.

> 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?

I've added sample, which is fixed by this patch. It can happen that
plan is adjusted and saved. Later it's invalidated and when revalidation
happens,
we miss modifications, added by check_sql_fn_retval(). Another
interesting issue
is that cached plan is checked for being valid before function starts
executing
(like earlier planning happened before function started executing). So,
once we
discard cached plans, plan for second query in function is not
invalidated immediately,
just on the second execution. And after rebuilding plan, it becomes
wrong.

--
Best regards,
Alexander Pyhalov,
Postgres Professional