Re: Possible bug: SQL function parameter in window frame definition - Mailing list pgsql-general

>>>>> "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)

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index dd0a7d8dac..2862c47314 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2217,7 +2217,6 @@ find_expr_references_walker(Node *node,
         /* query_tree_walker ignores ORDER BY etc, but we need those opers */
         find_expr_references_walker((Node *) query->sortClause, context);
         find_expr_references_walker((Node *) query->groupClause, context);
-        find_expr_references_walker((Node *) query->windowClause, context);
         find_expr_references_walker((Node *) query->distinctClause, context);
 
         /* Examine substructure of query */
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 18bd5ac903..7f485ae29a 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2292,6 +2292,8 @@ query_tree_walker(Query *query,
         return true;
     if (walker(query->havingQual, context))
         return true;
+    if (walker(query->windowClause, context))
+        return true;
     if (walker(query->limitOffset, context))
         return true;
     if (walker(query->limitCount, context))
@@ -3151,6 +3153,7 @@ query_tree_mutator(Query *query,
     MUTATE(query->jointree, query->jointree, FromExpr *);
     MUTATE(query->setOperations, query->setOperations, Node *);
     MUTATE(query->havingQual, query->havingQual, Node *);
+    MUTATE(query->windowClause, query->windowClause, List *);
     MUTATE(query->limitOffset, query->limitOffset, Node *);
     MUTATE(query->limitCount, query->limitCount, Node *);
     if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
diff --git a/src/backend/parser/parse_collate.c b/src/backend/parser/parse_collate.c
index 31a5f702a9..dabd904999 100644
--- a/src/backend/parser/parse_collate.c
+++ b/src/backend/parser/parse_collate.c
@@ -485,6 +485,7 @@ assign_collations_walker(Node *node, assign_collations_context *context)
         case T_FromExpr:
         case T_OnConflictExpr:
         case T_SortGroupClause:
+        case T_WindowClause:
             (void) expression_tree_walker(node,
                                           assign_collations_walker,
                                           (void *) &loccontext);