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

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

Tom> It looks to me that the reason is that query_tree_mutator
Tom> (likewise query_tree_walker) fails to visit query->windowClause,

I noticed this too. I spent some time looking at what might break if
that was changed (found two places so far, see attached draft patch).

Tom> which is a bug of the first magnitude if we allow those to contain
Tom> expressions. Not sure how we've missed that up to now.

I suspect because the partition/order by expressions are actually in the
targetlist instead (with only SortGroupClause nodes in the
windowClause), so only window framing expressions are being missed.

Tom> Looking at struct Query, it seems like that's not the only
Tom> questionable omission. We're also not descending into

Tom> Node *utilityStmt; /* non-null if commandType == CMD_UTILITY */

I assume that utility statements are doing any necessary expression
processing themselves...

Tom> List *groupClause; /* a list of SortGroupClause's */

There's at least one place that walks this (and the distinct and sort
clauses) explicitly (find_expr_references_walker) but most places just
aren't interested in SortGroupClause nodes given that the actual
expressions are elsewhere.

Tom> List *groupingSets; /* a list of GroupingSet's if present */

Likewise, GroupingSet nodes are not any form of expression, they only
reference the groupClause entries.

Tom> List *distinctClause; /* a list of SortGroupClause's */
Tom> List *sortClause; /* a list of SortGroupClause's */

Same goes as for groupClause.

Tom> List *rowMarks; /* a list of RowMarkClause's */

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.

--
Andrew (irc:RhodiumToad)