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

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.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)