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>
In-Reply-To: <CA+HiwqG=5uLuwR+8xNR-HN_5mj6c9t4kqVuwrcfhOuKONgDiGw@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 28 Aug 2023 09:43:41 -0400
Message-ID: <CA+TgmoYhqXKbpcJH+Oa=J=c-=zxwoTHQenbOtsLsft0a0_J_og@mail.gmail.com>
Subject: Re: generic plans and "initial" pruning
To: Amit Langote <amitlangote09@gmail.com>
Cc: 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/CA%2BTgmoYhqXKbpcJH%2BOa%3DJ%3Dc-%3DzxwoTHQenbOtsLsft0a0_J_og%40mail.gmail.com>
Precedence: bulk

On Fri, Aug 11, 2023 at 9:50=E2=80=AFAM Amit Langote <amitlangote09@gmail.c=
om> wrote:
> After removing the unnecessary cleanup code from most node types=E2=80=99=
 ExecEnd* functions, one thing I=E2=80=99m tempted to do is remove the func=
tions that do nothing else but recurse to close the outerPlan, innerPlan ch=
ild nodes.  We could instead have ExecEndNode() itself recurse to close out=
erPlan, innerPlan child nodes at the top, which preserves the close-child-b=
efore-self behavior for Gather* nodes, and close node type specific cleanup=
 functions for nodes that do have any local cleanup to do.  Perhaps, we cou=
ld even use planstate_tree_walker() called at the top instead of the usual =
bottom so that nodes with a list of child subplans like Append also don=E2=
=80=99t need to have their own ExecEnd* functions.

I think 0001 needs to be split up. Like, this is code cleanup:

-       /*
-        * Free the exprcontext
-        */
-       ExecFreeExprContext(&node->ss.ps);

This is providing for NULL pointers where we don't currently:

-       list_free_deep(aggstate->hash_batches);
+       if (aggstate->hash_batches)
+               list_free_deep(aggstate->hash_batches);

And this is the early return mechanism per se:

+       if (!ExecPlanStillValid(estate))
+               return aggstate;

I think at least those 3 kinds of changes deserve to be in separate
patches with separate commit messages explaining the rationale behind
each e.g. "Remove unnecessary cleanup calls in ExecEnd* functions.
These calls are no longer required, because <reasons>. Removing them
saves a few CPU cycles and simplifies planned refactoring, so do
that."

--=20
Robert Haas
EDB: http://www.enterprisedb.com