On Tue, Sep 5, 2023 at 3:13 AM Amit Langote <amitlangot...@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. 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. I suggest changing the subject line of the commit to something like "Remove obsolete executor cleanup code." > 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. The changes to ExecEndForeignScan look like they include stuff that belongs in 0001. Personally, I prefer explicit NULL-tests i.e. if (x != NULL) to implicit ones like if (x), but opinions vary. -- Robert Haas EDB: http://www.enterprisedb.com
