Re: SQLFunctionCache and generic plans

Alexander Pyhalov писал(а) 2025-03-28 15:22:
> 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.

After writing some comments, looking at it once again, I've found that
one assumption is wrong - function can be discarded from cache during
its execution.

For example,

create or replace function recurse(anyelement) returns record as
$$
begin
if ($1 > 0) then
if (mod($1, 2) = 0) then
execute format($query$
create or replace function sql_recurse(anyelement) returns
record as
$q$ select recurse($1); select ($1,2); $q$ language sql;
$query$);
end if;
return sql_recurse($1 - 1);
else
return row($1, 1::int);
end if;
end;
$$ language plpgsql;

create or replace function sql_recurse(anyelement) returns record as
$$ select recurse($1); select ($1,2); $$ language sql;

create table t1 (i int);
insert into t1 values(2),(3),(4);

select sql_recurse(i) from t1;

leads to dropping cached plans while they are needed. Will look how
better to handle it.

Also one interesting note is as we don't use raw_parse_tree, it seems we
don't need plansource->parserSetup and plansource->parserSetupArg. It
seems we can avoid caching complete parse info.

--
Best regards,
Alexander Pyhalov,
Postgres Professional