Re: Possible bug: SQL function parameter in window frame definition

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