obsolete reference to a SubPlan field
I noticed $subject while looking at something involving SubLinks and
SubPlans. It seems eab6b8b27eb removed the "plan" field from the
SubPlan node struct definition, but the following line from
expression_tree_mutator():
/* but not the sub-Plan itself, which is referenced as-is */
and the following from expression_tree_walker():
/* recurse into the testexpr, but not into the Plan */
both of which I think refer to that no-longer-existent field, appear
to have survived multiple commits that moved the SubPlan expression
processing code around.
Attached patch removes those.
--
Amit Langote
EDB: http://www.enterprisedb.com
Attachments:
SubPlan-obsolete-comment.patchapplication/octet-stream; name=SubPlan-obsolete-comment.patchDownload
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 47d0564fa2..9dbed97cec 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2058,7 +2058,7 @@ expression_tree_walker(Node *node,
{
SubPlan *subplan = (SubPlan *) node;
- /* recurse into the testexpr, but not into the Plan */
+ /* recurse into the testexpr */
if (walker(subplan->testexpr, context))
return true;
/* also examine args list */
@@ -2852,7 +2852,6 @@ expression_tree_mutator(Node *node,
MUTATE(newnode->testexpr, subplan->testexpr, Node *);
/* transform args list (params to be passed to subplan) */
MUTATE(newnode->args, subplan->args, List *);
- /* but not the sub-Plan itself, which is referenced as-is */
return (Node *) newnode;
}
break;
On Wed, Feb 2, 2022 at 3:08 AM Amit Langote <amitlangote09@gmail.com> wrote:
I noticed $subject while looking at something involving SubLinks and
SubPlans. It seems eab6b8b27eb removed the "plan" field from the
SubPlan node struct definition, but the following line from
expression_tree_mutator():/* but not the sub-Plan itself, which is referenced as-is */
and the following from expression_tree_walker():
/* recurse into the testexpr, but not into the Plan */
both of which I think refer to that no-longer-existent field, appear
to have survived multiple commits that moved the SubPlan expression
processing code around.Attached patch removes those.
Looks right to me. Tom, any comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 2, 2022 at 3:08 AM Amit Langote <amitlangote09@gmail.com> wrote:
Attached patch removes those.
Looks right to me. Tom, any comments?
I'm pretty sure I left those comments alone on purpose back in 2007,
and I don't find simply removing them to be an improvement.
In principle, readers might expect that tree walkers/mutators
would descend to a SubPlan's query, as they do for a SubLink's
query. Calling out the fact that that doesn't happen seems
useful to me. If you don't like this particular wording of those
comments, we can discuss better wordings ... but I doubt that
nothing-at-all is better.
regards, tom lane
On Mon, Mar 14, 2022 at 1:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 2, 2022 at 3:08 AM Amit Langote <amitlangote09@gmail.com> wrote:
Attached patch removes those.
Looks right to me. Tom, any comments?
I'm pretty sure I left those comments alone on purpose back in 2007,
and I don't find simply removing them to be an improvement.In principle, readers might expect that tree walkers/mutators
would descend to a SubPlan's query, as they do for a SubLink's
query. Calling out the fact that that doesn't happen seems
useful to me. If you don't like this particular wording of those
comments, we can discuss better wordings ... but I doubt that
nothing-at-all is better.
OK, well if it was intentional, then I do think some rewording is
justified. It seems to me that those comments, and especially the one
in expression_tree_mutator(), seem like they are talking about an
actual pointer, rather than some other kind of logical link. Otherwise
why is it sort of referenced along the way as we go through other
things that are all actual pointers? It certainly leads one to expect
a Plan * that isn't there.
I think it's a fine idea to explain that, on a conceptual level, we're
just walking the Plan data structure and the pointers which it
contains, and therefore won't reach down into a sub-Plan that is not
something we actually point to. Or whatever words clarify the intent.
But a passing reference that sounds like a pointer when it isn't is
worse than nothing at all.
--
Robert Haas
EDB: http://www.enterprisedb.com