From b7a1c5539ad34d7357e04cc58f9c02a101482737 Mon Sep 17 00:00:00 2001 From: Andrew Gierth Date: Thu, 3 Oct 2019 10:54:52 +0100 Subject: [PATCH] Selectively include window frames in expression walks/mutates. query_tree_walker and query_tree_mutator were skipping the windowClause of the query, without regard for the fact that the startOffset and endOffset in a WindowClause node are expression trees that need to be processed. This was an oversight in commit ec4be2ee6 from 2010 which added the expression fields; the main symptom is that function parameters in window frame clauses don't work in inlined functions. Fix (as conservatively as possible since this needs to not break existing out-of-tree callers) and add tests. Backpatch all the way, since this has been broken since 9.0. Per report from Alastair McKinley; fix by me with kibitzing and review from Tom Lane. Discussion: https://postgr.es/m/DB6PR0202MB2904E7FDDA9D81504D1E8C68E3800@DB6PR0202MB2904.eurprd02.prod.outlook.com --- src/backend/catalog/dependency.c | 9 +-- src/backend/nodes/nodeFuncs.c | 105 +++++++++++++++++++++++++++ src/include/nodes/nodeFuncs.h | 1 + src/test/regress/expected/window.out | 42 +++++++++++ src/test/regress/sql/window.sql | 19 +++++ 5 files changed, 169 insertions(+), 7 deletions(-) diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index dd0a7d8dac..03582781f6 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -2214,18 +2214,13 @@ find_expr_references_walker(Node *node, context->addrs); } - /* 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 */ context->rtables = lcons(query->rtable, context->rtables); result = query_tree_walker(query, find_expr_references_walker, (void *) context, - QTW_IGNORE_JOINALIASES); + QTW_IGNORE_JOINALIASES | + QTW_EXAMINE_SORTGROUP); context->rtables = list_delete_first(context->rtables); return result; } diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c index 18bd5ac903..b655931e48 100644 --- a/src/backend/nodes/nodeFuncs.c +++ b/src/backend/nodes/nodeFuncs.c @@ -2278,6 +2278,13 @@ query_tree_walker(Query *query, { Assert(query != NULL && IsA(query, Query)); + /* + * We don't walk any utilityStmt here. However, we can't easily assert + * that it is absent, since there are at least two code paths by which + * action statements from CREATE RULE end up here, and NOTIFY is allowed + * in a rule action. + */ + if (walker((Node *) query->targetList, context)) return true; if (walker((Node *) query->withCheckOptions, context)) @@ -2296,6 +2303,54 @@ query_tree_walker(Query *query, return true; if (walker(query->limitCount, context)) return true; + + /* + * Most callers aren't interested in SortGroupClause nodes since those + * don't contain actual expressions. However they do contain OIDs which + * may be needed by dependency walkers etc. + */ + if ((flags & QTW_EXAMINE_SORTGROUP)) + { + if (walker((Node *) query->groupClause, context)) + return true; + if (walker((Node *) query->windowClause, context)) + return true; + if (walker((Node *) query->sortClause, context)) + return true; + if (walker((Node *) query->distinctClause, context)) + return true; + } + else + { + /* + * But we need to walk the expressions under WindowClause nodes even + * if we're not interested in SortGroupClause nodes. + */ + ListCell *lc; + + foreach(lc, query->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, lc); + + if (walker(wc->startOffset, context)) + return true; + if (walker(wc->endOffset, context)) + return true; + } + } + + /* + * groupingSets and rowMarks are not walked: + * + * groupingSets contain only ressortgrouprefs (integers) which are + * meaningless without the corresponding groupClause or tlist. + * Accordingly, any walker that needs to care about them needs to handle + * them itself in its Query processing. + * + * rowMarks is not walked because it contains only rangetable indexes (and + * flags etc.) and therefore should be handled at Query level similarly. + */ + if (!(flags & QTW_IGNORE_CTE_SUBQUERIES)) { if (walker((Node *) query->cteList, context)) @@ -3153,6 +3208,56 @@ query_tree_mutator(Query *query, MUTATE(query->havingQual, query->havingQual, Node *); MUTATE(query->limitOffset, query->limitOffset, Node *); MUTATE(query->limitCount, query->limitCount, Node *); + + /* + * Most callers aren't interested in SortGroupClause nodes since those + * don't contain actual expressions. However they do contain OIDs, which + * may be of interest to some mutators. + */ + + if ((flags & QTW_EXAMINE_SORTGROUP)) + { + MUTATE(query->groupClause, query->groupClause, List *); + MUTATE(query->windowClause, query->windowClause, List *); + MUTATE(query->sortClause, query->sortClause, List *); + MUTATE(query->distinctClause, query->distinctClause, List *); + } + else + { + /* + * But we need to mutate the expressions under WindowClause nodes even + * if we're not interested in SortGroupClause nodes. + */ + List *resultlist; + ListCell *temp; + + resultlist = NIL; + foreach(temp, query->windowClause) + { + WindowClause *wc = lfirst_node(WindowClause, temp); + WindowClause *newnode; + + FLATCOPY(newnode, wc, WindowClause); + MUTATE(newnode->startOffset, wc->startOffset, Node *); + MUTATE(newnode->endOffset, wc->endOffset, Node *); + + resultlist = lappend(resultlist, (Node *) newnode); + } + query->windowClause = resultlist; + } + + /* + * groupingSets and rowMarks are not mutated: + * + * groupingSets contain only ressortgroup refs (integers) which are + * meaningless without the groupClause or tlist. Accordingly, any mutator + * that needs to care about them needs to handle them itself in its Query + * processing. + * + * rowMarks contains only rangetable indexes (and flags etc.) and + * therefore should be handled at Query level similarly. + */ + if (!(flags & QTW_IGNORE_CTE_SUBQUERIES)) MUTATE(query->cteList, query->cteList, List *); else /* else copy CTE list as-is */ diff --git a/src/include/nodes/nodeFuncs.h b/src/include/nodes/nodeFuncs.h index 0cb931c82c..4b5408fa9b 100644 --- a/src/include/nodes/nodeFuncs.h +++ b/src/include/nodes/nodeFuncs.h @@ -27,6 +27,7 @@ #define QTW_EXAMINE_RTES_AFTER 0x20 /* examine RTE nodes after their * contents */ #define QTW_DONT_COPY_QUERY 0x40 /* do not copy top Query */ +#define QTW_EXAMINE_SORTGROUP 0x80 /* include SortGroupNode lists */ /* callback function for check_functions_in_node */ typedef bool (*check_function_callback) (Oid func_id, void *context); diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out index edc93d5729..d5fd4045f9 100644 --- a/src/test/regress/expected/window.out +++ b/src/test/regress/expected/window.out @@ -3821,3 +3821,45 @@ SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w 5 | t | t | t (5 rows) +-- Tests for problems with failure to walk or mutate expressions +-- within window frame clauses. +-- test walker (fails with collation error if expressions are not walked) +SELECT array_agg(i) OVER w + FROM generate_series(1,5) i +WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW); + array_agg +----------- + {1} + {1,2} + {2,3} + {3,4} + {4,5} +(5 rows) + +-- test mutator (fails when inlined if expressions are not mutated) +CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[] +AS $$ + SELECT array_agg(s) OVER w + FROM generate_series(1,5) s + WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING) +$$ LANGUAGE SQL STABLE; +EXPLAIN (costs off) SELECT * FROM pg_temp.f(2); + QUERY PLAN +------------------------------------------------------ + Subquery Scan on f + -> WindowAgg + -> Sort + Sort Key: s.s + -> Function Scan on generate_series s +(5 rows) + +SELECT * FROM pg_temp.f(2); + f +--------- + {1,2,3} + {2,3,4} + {3,4,5} + {4,5} + {5} +(5 rows) + diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql index fc6d4cc903..fe273aa31e 100644 --- a/src/test/regress/sql/window.sql +++ b/src/test/regress/sql/window.sql @@ -1257,3 +1257,22 @@ SELECT to_char(SUM(n::float8) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FO SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w FROM (VALUES (1,true), (2,true), (3,false), (4,false), (5,true)) v(i,b) WINDOW w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING); + +-- Tests for problems with failure to walk or mutate expressions +-- within window frame clauses. + +-- test walker (fails with collation error if expressions are not walked) +SELECT array_agg(i) OVER w + FROM generate_series(1,5) i +WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW); + +-- test mutator (fails when inlined if expressions are not mutated) +CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[] +AS $$ + SELECT array_agg(s) OVER w + FROM generate_series(1,5) s + WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING) +$$ LANGUAGE SQL STABLE; + +EXPLAIN (costs off) SELECT * FROM pg_temp.f(2); +SELECT * FROM pg_temp.f(2); -- 2.40.0