>>>>> "Tom" == Tom Lane <tgl@xxxxxxxxxxxxx> 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);
