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