On Fri, Aug 11, 2023 at 9:50 AM Amit Langote <amitlangot...@gmail.com> wrote: > After removing the unnecessary cleanup code from most node types’ ExecEnd* > functions, one thing I’m tempted to do is remove the functions that do > nothing else but recurse to close the outerPlan, innerPlan child nodes. We > could instead have ExecEndNode() itself recurse to close outerPlan, innerPlan > child nodes at the top, which preserves the close-child-before-self behavior > for Gather* nodes, and close node type specific cleanup functions for nodes > that do have any local cleanup to do. Perhaps, we could 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’t 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." -- Robert Haas EDB: http://www.enterprisedb.com
