MIME-Version: 1.0
Date: Mon, 31 Mar 2025 13:31:45 +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: <3605427.1743351017@sss.pgh.pa.us>
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>
 <db42573039cc66815e80a48589eebea8@postgrespro.ru>
 <2691107.1743257329@sss.pgh.pa.us> <3605427.1743351017@sss.pgh.pa.us>
Message-ID: <760f8088e1f0c5c08e9dab8a60f9368a@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/760f8088e1f0c5c08e9dab8a60f9368a%40postgrespro.ru>
Precedence: bulk

Hi.

Tom Lane писал(а) 2025-03-30 19:10:
> I spent some time reading and reworking this code, and have
> arrived at a patch set that I'm pretty happy with.  I'm not
> sure it's quite committable but it's close:
> 

> 0005: This extracts the RLS test case you had and commits it
> with the old non-failing behavior, just so that we can see that
> the new code does it differently.  (I didn't adopt your test
> from rules.sql, because AFAICS it works the same with or without
> this patch set.  What was the point of that one again?)

The test was introduced after my error to handle case when 
execution_state
is NULL in fcache->func_state list due to statement being completely 
removed
by instead rule. After founding this issue, I've added a test to cover 
it.
Not sure if it should be preserved.

> 
> 0006: The guts of the patch.  I couldn't break this down any
> further.
> 
> One big difference from what you had is that there is only one path
> of control: we always use the plan cache.  The hack you had to not
> use it for triggers was only needed because you didn't include the
> right cache key items to distinguish different trigger usages, but
> the code coming from plpgsql has that right.
> 

Yes, now it looks much more consistent. Still going through it. Will
do some additional testing here. So far have checked all known corner
cases and found no issues.

> Also, the memory management is done a bit differently.  The
> "fcontext" memory context holding the SQLFunctionCache struct is
> now discarded at the end of each execution of the SQL function,
> which considerably alleviates worries about leaking memory there.
> I invented a small "SQLFunctionLink" struct that is what fn_extra
> points at, and it survives as long as the FmgrInfo does, so that's
> what saves us from redoing hash key computations in most cases.

I've looked through it and made some tests, including ones which caused 
me
to create separate context for planing. Was a bit worried that it has 
gone,
but now, as fcache->fcontext is deleted in the end of function 
execution,
I don't see leaks, which were the initial reason for introducing it.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional