MIME-Version: 1.0
References: <8216639.NyiUUSuA9g@aivenlaptop> <3084885.1675783797@sss.pgh.pa.us>
 <b69515864a7021505340f85476dc111e@postgrespro.ru> <CAPpHfdue9RVJKo33jKwrCeEXec0BTTH7CTxVcyXGMbfMExaGLA@mail.gmail.com>
 <758251661a3e8760ff52dd5c672a7505@postgrespro.ru>
In-Reply-To: <758251661a3e8760ff52dd5c672a7505@postgrespro.ru>
From: Pavel Stehule <pavel.stehule@gmail.com>
Date: Tue, 31 Dec 2024 16:39:45 +0100
Message-ID: <CAFj8pRCoR9B+Ap70TQqWUa0PeeEk94D6VW20YbWN2Pm4b6-JMQ@mail.gmail.com>
Subject: Re: SQLFunctionCache and generic plans
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, pgsql-hackers@lists.postgresql.org, 
	Ronan Dunklau <ronan.dunklau@aiven.io>, Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: multipart/alternative; boundary="00000000000036f9e9062a92bf1e"
Archived-At: <https://www.postgresql.org/message-id/CAFj8pRCoR9B%2BAp70TQqWUa0PeeEk94D6VW20YbWN2Pm4b6-JMQ%40mail.gmail.com>
Precedence: bulk

--00000000000036f9e9062a92bf1e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi

=C3=BAt 31. 12. 2024 v 16:36 odes=C3=ADlatel Alexander Pyhalov <
a.pyhalov@postgrespro.ru> napsal:

> Hi.
>
> >> What should we do with "pre-parsed" SQL functions (when prosrc is
> >> empty)? How should we create cached plans when we don't have raw
> >> parsetrees?
> >> Currently we can create cached plans without raw parsetrees, but this
> >> means that plan revalidation doesn't work, choose_custom_plan()
> >> always returns false and we get generic plan. Perhaps, we need some
> >> form
> >> of GetCachedPlan(), which ignores raw_parse_tree?
> >
> > I don't think you need a new form of GetCachedPlan().  Instead, it
> > seems that StmtPlanRequiresRevalidation() should be revised.  As I got
> > from comments and the d8b2fcc9d4 commit message, the primary goal was
> > to skip revalidation of utility statements.  Skipping revalidation was
> > a positive side effect, as long as we didn't support custom plans for
> > them anyway.  But as you're going to change this,
> > StmtPlanRequiresRevalidation() needs to be revised.
> >
>
> Thanks for feedback.
>
> I've modifed StmtPlanRequiresRevalidation() so that it looks on queries
> command type.
> Not sure if it's enough or I have to recreate something more similar to
> stmt_requires_parse_analysis()
> logic. I was wondering if this can lead to triggering plan revalidation
> in RevalidateCachedQuery().
> I suppose not - as we plan in executor (so shouldn't catch userid change
> or see some changes in
> related objects. Revalidation would kill our plan, destroying
> resultDesc.
>
> Also while looking at this, fixed processing of instead of rules (which
> would lead to NULL execution_state).
> --
>

there are lot of fails found by tester

Please, can you check it?

regards

Pavel

> Best regards,
> Alexander Pyhalov,
> Postgres Professional

--00000000000036f9e9062a92bf1e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>Hi<br></div><br><div class=3D"gmail_quote gmail_quote=
_container"><div dir=3D"ltr" class=3D"gmail_attr">=C3=BAt 31. 12. 2024 v=C2=
=A016:36 odes=C3=ADlatel Alexander Pyhalov &lt;<a href=3D"mailto:a.pyhalov@=
postgrespro.ru">a.pyhalov@postgrespro.ru</a>&gt; napsal:<br></div><blockquo=
te class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px =
solid rgb(204,204,204);padding-left:1ex">Hi.<br>
<br>
&gt;&gt; What should we do with &quot;pre-parsed&quot; SQL functions (when =
prosrc is<br>
&gt;&gt; empty)? How should we create cached plans when we don&#39;t have r=
aw<br>
&gt;&gt; parsetrees?<br>
&gt;&gt; Currently we can create cached plans without raw parsetrees, but t=
his<br>
&gt;&gt; means that plan revalidation doesn&#39;t work, choose_custom_plan(=
)<br>
&gt;&gt; always returns false and we get generic plan. Perhaps, we need som=
e <br>
&gt;&gt; form<br>
&gt;&gt; of GetCachedPlan(), which ignores raw_parse_tree?<br>
&gt; <br>
&gt; I don&#39;t think you need a new form of GetCachedPlan().=C2=A0 Instea=
d, it<br>
&gt; seems that StmtPlanRequiresRevalidation() should be revised.=C2=A0 As =
I got<br>
&gt; from comments and the d8b2fcc9d4 commit message, the primary goal was<=
br>
&gt; to skip revalidation of utility statements.=C2=A0 Skipping revalidatio=
n was<br>
&gt; a positive side effect, as long as we didn&#39;t support custom plans =
for<br>
&gt; them anyway.=C2=A0 But as you&#39;re going to change this,<br>
&gt; StmtPlanRequiresRevalidation() needs to be revised.<br>
&gt; <br>
<br>
Thanks for feedback.<br>
<br>
I&#39;ve modifed StmtPlanRequiresRevalidation() so that it looks on queries=
 <br>
command type.<br>
Not sure if it&#39;s enough or I have to recreate something more similar to=
 <br>
stmt_requires_parse_analysis()<br>
logic. I was wondering if this can lead to triggering plan revalidation <br=
>
in RevalidateCachedQuery().<br>
I suppose not - as we plan in executor (so shouldn&#39;t catch userid chang=
e <br>
or see some changes in<br>
related objects. Revalidation would kill our plan, destroying <br>
resultDesc.<br>
<br>
Also while looking at this, fixed processing of instead of rules (which <br=
>
would lead to NULL execution_state).<br>
-- <br></blockquote><div><br></div><div>there are lot of fails found by tes=
ter</div><div><br></div><div>Please, can you check it?</div><div><br></div>=
<div>regards</div><div><br></div><div>Pavel <br></div><blockquote class=3D"=
gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(20=
4,204,204);padding-left:1ex">
Best regards,<br>
Alexander Pyhalov,<br>
Postgres Professional</blockquote></div></div>

--00000000000036f9e9062a92bf1e--