Andrew Gierth <andrew@xxxxxxxxxxxxxxxxxxxx> writes: > "Tom" == Tom Lane <tgl@xxxxxxxxxxxxx> 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
