Thread

Re: SQLFunctionCache and generic plans

Alexander Pyhalov <a.pyhalov@postgrespro.ru> — 2025-03-29T07:24:41Z

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