From c35ba141de1fa04373671ba24c73eb0fe4862415 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 19 Sep 2022 12:16:02 -0400 Subject: [PATCH] Future-proof the recursion inside ExecShutdownNode(). The API contract for planstate_tree_walker() callbacks is that they take a PlanState pointer and a context pointer. Somebody figured they could save a couple lines of code by ignoring that, and passing ExecShutdownNode itself as the walker even though it has but one argument. Somewhat remarkably, we've gotten away with that so far. However, it seems clear that the upcoming C2x standard means to forbid such cases, and compilers that actively break such code likely won't be far behind. So spend the extra few lines of code to do it honestly with a separate walker function. In HEAD, we might as well go further and remove ExecShutdownNode's useless return value. I left that as-is in back branches though, to forestall complaints about ABI breakage. Back-patch, with the thought that this might become of practical importance before our stable branches are all out of service. It doesn't seem to be fixing any live bug on any currently known platform, however. Discussion: https://postgr.es/m/208054.1663534665@sss.pgh.pa.us --- src/backend/executor/execMain.c | 2 +- src/backend/executor/execProcnode.c | 11 +++++++++-- src/include/executor/executor.h | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index ef2fd46092e..d78862e660e 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1691,7 +1691,7 @@ ExecutePlan(EState *estate, * point. */ if (!(estate->es_top_eflags & EXEC_FLAG_BACKWARD)) - (void) ExecShutdownNode(planstate); + ExecShutdownNode(planstate); if (use_parallel_mode) ExitParallelMode(); diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c index b5667e53e5f..36406c3af57 100644 --- a/src/backend/executor/execProcnode.c +++ b/src/backend/executor/execProcnode.c @@ -121,6 +121,7 @@ static TupleTableSlot *ExecProcNodeFirst(PlanState *node); static TupleTableSlot *ExecProcNodeInstr(PlanState *node); +static bool ExecShutdownNode_walker(PlanState *node, void *context); /* ------------------------------------------------------------------------ @@ -768,8 +769,14 @@ ExecEndNode(PlanState *node) * Give execution nodes a chance to stop asynchronous resource consumption * and release any resources still held. */ -bool +void ExecShutdownNode(PlanState *node) +{ + (void) ExecShutdownNode_walker(node, NULL); +} + +static bool +ExecShutdownNode_walker(PlanState *node, void *context) { if (node == NULL) return false; @@ -789,7 +796,7 @@ ExecShutdownNode(PlanState *node) if (node->instrument && node->instrument->running) InstrStartNode(node->instrument); - planstate_tree_walker(node, ExecShutdownNode, NULL); + planstate_tree_walker(node, ExecShutdownNode_walker, context); switch (nodeTag(node)) { diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 82925b4b633..a91c472a0d9 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -239,7 +239,7 @@ extern PlanState *ExecInitNode(Plan *node, EState *estate, int eflags); extern void ExecSetExecProcNode(PlanState *node, ExecProcNodeMtd function); extern Node *MultiExecProcNode(PlanState *node); extern void ExecEndNode(PlanState *node); -extern bool ExecShutdownNode(PlanState *node); +extern void ExecShutdownNode(PlanState *node); extern void ExecSetTupleBound(int64 tuples_needed, PlanState *child_node);