MIME-Version: 1.0
Date: Sat, 29 Mar 2025 10:24:41 +0300
From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pavel Stehule <pavel.stehule@gmail.com>, Alexander Korotkov
 <aekorotkov@gmail.com>, pgsql-hackers@lists.postgresql.org, Ronan Dunklau
 <ronan.dunklau@aiven.io>
Subject: Re: SQLFunctionCache and generic plans
In-Reply-To: <fcc68b0ef570160cd6e88851eb244ccd@postgrespro.ru>
References: <8216639.NyiUUSuA9g@aivenlaptop>
 <3084885.1675783797@sss.pgh.pa.us>
 <b69515864a7021505340f85476dc111e@postgrespro.ru>
 <CAPpHfdue9RVJKo33jKwrCeEXec0BTTH7CTxVcyXGMbfMExaGLA@mail.gmail.com>
 <758251661a3e8760ff52dd5c672a7505@postgrespro.ru>
 <CAFj8pRCoR9B+Ap70TQqWUa0PeeEk94D6VW20YbWN2Pm4b6-JMQ@mail.gmail.com>
 <30f02ca545915627905fbe3f23d93ec1@postgrespro.ru>
 <2275997.1737138456@sss.pgh.pa.us>
 <ad7296dd981c5f8eb76ff129913a55fd@postgrespro.ru>
 <51ebc5f3b846ce279b75ac4f5e5ac33f@postgrespro.ru>
 <CAFj8pRD2Fa84HoH_R_S5aKngyA6a+Au7U6NyidfBMrwEjFAc9g@mail.gmail.com>
 <401652.1738598448@sss.pgh.pa.us>
 <CAFj8pRC_4gEfmDoB5KbacNDdt3e6Tnf1z1BFK=MVp3+2kcDVqw@mail.gmail.com>
 <CAFj8pRA7MFTfUPwDDWtghaUvAjbrMHFUPbHyLSX3h8+FDw4WFg@mail.gmail.com>
 <445556.1738612687@sss.pgh.pa.us> <464677.1738620525@sss.pgh.pa.us>
 <1fc18af56d5fb27bf3e153ffaa2ed70d@postgrespro.ru>
 <2615770.1740688857@sss.pgh.pa.us>
 <448c63cf9b4f917c01802309b9c29eb9@postgrespro.ru>
 <CAFj8pRDWDeF2cC+pCjLHJno7KnK5kdtjYN-f933R! ! HS7UneArFw@mail.gmail.com>
 <CAFj8pRD0diy5VDP=8B9au5sk_yRknCWqZx35VniBtP_OA4aJNw@mail.gmail.com>
 <2827970.1741890560@sss.pgh.pa.us>
 <337e011069b6831e063bfa346f49bb46@postgrespro.ru>
 <3344719.1741985546@sss.pgh.pa.us>
 <fcc68b0ef570160cd6e88851eb244ccd@postgrespro.ru>
Message-ID: <db42573039cc66815e80a48589eebea8@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/db42573039cc66815e80a48589eebea8%40postgrespro.ru>
Precedence: bulk

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