Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> Now probably this is never called on utility statements, and maybe
> Tom> there is never a reason for anyone to examine or mutate
> Tom> SortGroupClauses, GroupingSets, or RowMarkClauses, but I'm not
> Tom> sure it's any business of this module to assume that.
> I think the logic that query_tree_walker is specifically there to walk
> places that might contain _expressions_ is reasonably valid. That said,
> the fact that we do have one caller that finds it necessary to
> explicitly walk some of the places that query_tree_walker omits suggests
> that this decision may have been a mistake.
I'm okay with assuming that these functions aren't used on utility
statements (but maybe we should add Assert(query->utilityStmt == NULL)?).
I'm a bit uncomfortable with skipping the other lists. Admittedly,
there's probably not huge value in examining SortGroupClauses in a
vacuum (that is, without knowing which list they appear in). The only
application I can think of offhand is extracting dependencies, which
is already covered by that one caller you mention.
However, we need to fix this in all active branches, and I definitely
agree with minimizing the amount of change to back branches.
The fact that the minimal change breaks (or exposes an oversight in)
assign_collations_walker makes it very plausible that it will also
break somebody's third-party code. If we push the API change further
we increase the risk of breaking stuff. That seems OK in HEAD but
not in back branches.
regards, tom lane
