On Tue, Sep 5, 2023 at 11:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Sep 5, 2023 at 3:13 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > Attached 0001 removes unnecessary cleanup calls from ExecEnd*() routines.
>
> It also adds a few random Assert()s to verify that unrelated pointers
> are not NULL. I suggest that it shouldn't do that.
OK, removed.
> The commit message doesn't mention the removal of the calls to
> ExecDropSingleTupleTableSlot. It's not clear to me why that's OK and I
> think it would be nice to mention it in the commit message, assuming
> that it is in fact OK.
That is not OK, so I dropped their removal. I think I confused them
with slots in other functions initialized with
ExecInitExtraTupleSlot() that *are* put into the estate.
> I suggest changing the subject line of the commit to something like
> "Remove obsolete executor cleanup code."
Sure.
> > 0002 adds NULLness checks in ExecEnd*() routines on some pointers that
> > may not be initialized by the corresponding ExecInit*() routines in
> > the case where it returns early.
>
> I think you should only add these where it's needed. For example, I
> think list_free_deep(NIL) is fine.
OK, done.
> The changes to ExecEndForeignScan look like they include stuff that
> belongs in 0001.
Oops, yes. Moved to 0001.
> Personally, I prefer explicit NULL-tests i.e. if (x != NULL) to
> implicit ones like if (x), but opinions vary.
I agree, so changed all the new tests to use (x != NULL) form.
Typically, I try to stick with whatever style is used in the nearby
code, though I can see both styles being used in the ExecEnd*()
routines. I opted to use the style that we both happen to prefer.
Attached updated patches. Thanks for the review.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
