MIME-Version: 1.0
References: <20221221101846.7zsi7lscnm5ediik@alvherre.pgsql>
 <1350682.1671635927@sss.pgh.pa.us> <4191508.1674157166@sss.pgh.pa.us>
 <CA+HiwqFhQ8tLMLQAWYRWiBQUrWSLM8qhVzJ4B5jWr=orUXfSyA@mail.gmail.com>
 <349124.1674185509@sss.pgh.pa.us> <CA+HiwqGFm_5aUqPnt=WCarkJ2ZU6F8kD8pFeGurHP+NZWS8KQw@mail.gmail.com>
 <CA+HiwqEDr9m3NGrmiOgatCnRPwD95=MHgWQdwvnoMyQd3k9-Yw@mail.gmail.com>
 <CA+HiwqGTrQ=ywAmB2zP81jcENZh1vLuyJaC2-xhWvBsnXWgZYQ@mail.gmail.com>
 <CA+HiwqGe0W2C+SrKcxrk-r4JjO0sCfL581p1M2bzr_LSrzGn+g@mail.gmail.com>
 <20230207180855.xy5m4puwh5gzd7xy@awork3.anarazel.de> <CA+HiwqGBumxzWctnUy33dHy2uGCtfmcqKyw4FONJNyitJvujWw@mail.gmail.com>
 <CA+HiwqHmdH2bcUtBGncwB7iJ9N0VTkUo4YPYFNtJL_f3kkau=g@mail.gmail.com>
 <CA+HiwqHKTxfaYc=e4mVOv0iDm3vVK56WOBCddzYdXKaWQqniww@mail.gmail.com>
 <CA+HiwqHQ1PM+HXoEdvutj0huhu2cfmuPa8wtctor0NNADzZVvA@mail.gmail.com>
 <CA+HiwqH=cbBocfSmyjd_N7ZceZ3RtXuQ=rNkAfdn+RwqMGY9fQ@mail.gmail.com>
 <CA+HiwqHoZSM4A0HKoTERmp=_stQjpjmomgg=rCf_4x4qCpxbZA@mail.gmail.com>
 <1274885.1680558101@sss.pgh.pa.us> <CA+HiwqFfC7ANtb+HAHYuR4wnwYbQdbK5B0ee0fjtNwTt+TOdwg@mail.gmail.com>
 <CA+HiwqH3jY-W=bekWxFF=B+9tpS42_1sJGsre1Ks0ueQjhta2Q@mail.gmail.com>
 <CA+HiwqEAHH=_PVG87rSHhQxmbHQ1dxSd58BVg=dHHfsgCeQFHw@mail.gmail.com>
 <9D23D8BB-83EE-451E-95C1-CF42ADB76869@yesql.se> <CA+HiwqHrkhNe=EUixymT0Nynp78Dnaqnf5qQnCowJd3ZSzXvFg@mail.gmail.com>
 <CA+HiwqEDbf=+s73hAF0PigWORRx+YWwbCQtuuWtHzc3ko_DGpw@mail.gmail.com>
 <CAA-aLv5EDpYBaZrPjE_kkaoERQmAPHO=fm-FwDsw3xJG5gb8Lg@mail.gmail.com>
 <CA+HiwqHBbptyxjQx7964DjitA8FVNs1MN=uwrzRy=oOD0Hy3ag@mail.gmail.com>
 <CA+HiwqEP3j25702EeergM7o8GqC79Dx-3gHKnvfa8oRJiXBDgA@mail.gmail.com>
 <CA+HiwqE=qxN5C-oN5vguBZOZGyDRAMV2EW1pO_hObcpf6X5QwQ@mail.gmail.com>
 <CA+Tgmoan++3tDKjBysnp3QdJc_f+zKYFezXQw9jUPvRY=kZ9Cg@mail.gmail.com>
 <CA+HiwqGFYpPRxQVUfeds1QX1U6o1QkKMYjHrjn0+0XEcgUV7+A@mail.gmail.com>
 <CA+TgmoacPSTXkPFivji-kA=DSp3jMi0TLonmtckDeq3p3=UP9w@mail.gmail.com>
 <CA+HiwqFRJ9NsF5s_Yno3kQ4rLtkWxb86fikeUdjseub8j8rHeA@mail.gmail.com>
 <CA+HiwqG=5uLuwR+8xNR-HN_5mj6c9t4kqVuwrcfhOuKONgDiGw@mail.gmail.com>
 <CA+TgmoYhqXKbpcJH+Oa=J=c-=zxwoTHQenbOtsLsft0a0_J_og@mail.gmail.com>
 <CA+HiwqGjaDzk8Q1Gapx8bnrFHTry92u52C8dEHKvZsVkq2VpJg@mail.gmail.com>
 <CA+TgmobC6_4xasJoccq7t7A6ZRVo7PQN+NJJHDmipnNY-eONog@mail.gmail.com>
 <CA+HiwqGSmTE37MmERJREqnO=w5NQHLPutDoq4sAvvXQXWwUPCw@mail.gmail.com>
 <CA+TgmoYP-Aimev3Wa6O10TC3u_eRsODAZo0Lff3Ey-2mHdZAzw@mail.gmail.com>
 <CA+HiwqFOLQt054o08yQZdXXEVM=4Sr8smLAspJsbYjLNxRG5Pw@mail.gmail.com>
 <CA+HiwqFM6qE-qc5SQyQYXf-OTsPOaCVq=qDqBnyra_mthySLRw@mail.gmail.com>
 <CA+HiwqG9YF-xxnXRW604LkJbTvFABwfSBgrNcQFMCL5-QG=P0g@mail.gmail.com> <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
In-Reply-To: <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Fri, 5 Jan 2024 16:16:27 +0530
Message-ID: <CALDaNm35cU-e_SchWjKoe6Lruihwz8UM3jjPv4GVQsH7pjoTDQ@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: Amit Langote <amitlangote09@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, 
	Andres Freund <andres@anarazel.de>, Daniel Gustafsson <daniel@yesql.se>, 
	David Rowley <dgrowleyml@gmail.com>, Jacob Champion <jchampion@timescale.com>, 
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>, Thom Brown <thom@linux.com>, 
	Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CALDaNm35cU-e_SchWjKoe6Lruihwz8UM3jjPv4GVQsH7pjoTDQ%40mail.gmail.com>
Precedence: bulk

On Mon, 20 Nov 2023 at 10:00, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Sep 28, 2023 at 5:26=E2=80=AFPM Amit Langote <amitlangote09@gmail=
.com> wrote:
> > On Tue, Sep 26, 2023 at 10:06=E2=80=AFPM Amit Langote <amitlangote09@gm=
ail.com> wrote:
> > > After sleeping on this, I think we do need the checks after all the
> > > ExecInitNode() calls too, because we have many instances of the code
> > > like the following one:
> > >
> > >     outerPlanState(gatherstate) =3D ExecInitNode(outerNode, estate, e=
flags);
> > >     tupDesc =3D ExecGetResultType(outerPlanState(gatherstate));
> > >     <some code that dereferences outDesc>
> > >
> > > If outerNode is a SeqScan and ExecInitSeqScan() returned early becaus=
e
> > > ExecOpenScanRelation() detected that plan was invalidated, then
> > > tupDesc would be NULL in this case, causing the code to crash.
> > >
> > > Now one might say that perhaps we should only add the
> > > is-CachedPlan-valid test in the instances where there is an actual
> > > risk of such misbehavior, but that could lead to confusion, now or
> > > later.  It seems better to add them after every ExecInitNode() call
> > > while we're inventing the notion, because doing so relieves the
> > > authors of future enhancements of the ExecInit*() routines from
> > > worrying about any of this.
> > >
> > > Attached 0003 should show how that turned out.
> > >
> > > Updated 0002 as mentioned in the previous reply -- setting pointers t=
o
> > > NULL after freeing them more consistently across various ExecEnd*()
> > > routines and using the `if (pointer !=3D NULL)` style over the `if
> > > (pointer)` more consistently.
> > >
> > > Updated 0001's commit message to remove the mention of its relation t=
o
> > > any future commits.  I intend to push it tomorrow.
> >
> > Pushed that one.  Here are the rebased patches.
> >
> > 0001 seems ready to me, but I'll wait a couple more days for others to
> > weigh in.  Just to highlight a kind of change that others may have
> > differing opinions on, consider this hunk from the patch:
> >
> > -   MemoryContextDelete(node->aggcontext);
> > +   if (node->aggcontext !=3D NULL)
> > +   {
> > +       MemoryContextDelete(node->aggcontext);
> > +       node->aggcontext =3D NULL;
> > +   }
> > ...
> > +   ExecEndNode(outerPlanState(node));
> > +   outerPlanState(node) =3D NULL;
> >
> > So the patch wants to enhance the consistency of setting the pointer
> > to NULL after freeing part.  Robert mentioned his preference for doing
> > it in the patch, which I agree with.
>
> Rebased.

There is a leak reported at [1], details for the same is available at [2]:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/select_views.out
/tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/select_v=
iews.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/select_views.out
2023-12-19 23:00:04.677385000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/sele=
ct_views.out
2023-12-19 23:06:26.870259000 +0000
@@ -1288,6 +1288,7 @@
       (102, '2011-10-12', 120),
       (102, '2011-10-28', 200),
       (103, '2011-10-15', 480);
+WARNING:  resource was not closed: relation "customer_pkey"
 CREATE VIEW my_property_normal AS
        SELECT * FROM customer WHERE name =3D current_user;
 CREATE VIEW my_property_secure WITH (security_barrier) A

[1] - https://cirrus-ci.com/task/6494009196019712
[2] - https://api.cirrus-ci.com/v1/artifact/task/6494009196019712/testrun/b=
uild/testrun/regress-running/regress/regression.diffs

Regards,
Vingesh