Thread: 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
Attachment
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