public inbox for [email protected]
help / color / mirror / Atom feedFrom: vignesh C <[email protected]>
To: Amit Langote <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: David Rowley <[email protected]>
Cc: Jacob Champion <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Thom Brown <[email protected]>
Cc: Tom Lane <[email protected]>
Subject: Re: generic plans and "initial" pruning
Date: Fri, 5 Jan 2024 16:16:27 +0530
Message-ID: <CALDaNm35cU-e_SchWjKoe6Lruihwz8UM3jjPv4GVQsH7pjoTDQ@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqFpZ80UJKr4tZus4Omgg7YESzFXKSwSHRW2Ap2=XSVyUA@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CA+HiwqFhQ8tLMLQAWYRWiBQUrWSLM8qhVzJ4B5jWr=orUXfSyA@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<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>
<[email protected]>
<CA+HiwqFfC7ANtb+HAHYuR4wnwYbQdbK5B0ee0fjtNwTt+TOdwg@mail.gmail.com>
<CA+HiwqH3jY-W=bekWxFF=B+9tpS42_1sJGsre1Ks0ueQjhta2Q@mail.gmail.com>
<CA+HiwqEAHH=_PVG87rSHhQxmbHQ1dxSd58BVg=dHHfsgCeQFHw@mail.gmail.com>
<[email protected]>
<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>
On Mon, 20 Nov 2023 at 10:00, Amit Langote <[email protected]> wrote:
>
> On Thu, Sep 28, 2023 at 5:26 PM Amit Langote <[email protected]> wrote:
> > On Tue, Sep 26, 2023 at 10:06 PM Amit Langote <[email protected]> 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) = ExecInitNode(outerNode, estate, eflags);
> > > tupDesc = ExecGetResultType(outerPlanState(gatherstate));
> > > <some code that dereferences outDesc>
> > >
> > > If outerNode is a SeqScan and ExecInitSeqScan() returned early because
> > > 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 to
> > > NULL after freeing them more consistently across various ExecEnd*()
> > > routines and using the `if (pointer != NULL)` style over the `if
> > > (pointer)` more consistently.
> > >
> > > Updated 0001's commit message to remove the mention of its relation to
> > > 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 != NULL)
> > + {
> > + MemoryContextDelete(node->aggcontext);
> > + node->aggcontext = NULL;
> > + }
> > ...
> > + ExecEndNode(outerPlanState(node));
> > + outerPlanState(node) = 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_views.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/select_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 = 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/build/testrun/regress-running/re...
Regards,
Vingesh
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: generic plans and "initial" pruning
In-Reply-To: <CALDaNm35cU-e_SchWjKoe6Lruihwz8UM3jjPv4GVQsH7pjoTDQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox