Skip to content

Commit 3473f81

Browse files
committed
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 ec4be2e 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
1 parent 668918f commit 3473f81

File tree

5 files changed

+169
-7
lines changed

5 files changed

+169
-7
lines changed

src/backend/catalog/dependency.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1873,18 +1873,13 @@ find_expr_references_walker(Node *node,
18731873
context->addrs);
18741874
}
18751875

1876-
/* query_tree_walker ignores ORDER BY etc, but we need those opers */
1877-
find_expr_references_walker((Node *) query->sortClause, context);
1878-
find_expr_references_walker((Node *) query->groupClause, context);
1879-
find_expr_references_walker((Node *) query->windowClause, context);
1880-
find_expr_references_walker((Node *) query->distinctClause, context);
1881-
18821876
/* Examine substructure of query */
18831877
context->rtables = lcons(query->rtable, context->rtables);
18841878
result = query_tree_walker(query,
18851879
find_expr_references_walker,
18861880
(void *) context,
1887-
QTW_IGNORE_JOINALIASES);
1881+
QTW_IGNORE_JOINALIASES |
1882+
QTW_EXAMINE_SORTGROUP);
18881883
context->rtables = list_delete_first(context->rtables);
18891884
return result;
18901885
}

src/backend/nodes/nodeFuncs.c

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1944,6 +1944,13 @@ query_tree_walker(Query *query,
19441944
{
19451945
Assert(query != NULL && IsA(query, Query));
19461946

1947+
/*
1948+
* We don't walk any utilityStmt here. However, we can't easily assert
1949+
* that it is absent, since there are at least two code paths by which
1950+
* action statements from CREATE RULE end up here, and NOTIFY is allowed
1951+
* in a rule action.
1952+
*/
1953+
19471954
if (walker((Node *) query->targetList, context))
19481955
return true;
19491956
if (walker((Node *) query->withCheckOptions, context))
@@ -1960,6 +1967,54 @@ query_tree_walker(Query *query,
19601967
return true;
19611968
if (walker(query->limitCount, context))
19621969
return true;
1970+
1971+
/*
1972+
* Most callers aren't interested in SortGroupClause nodes since those
1973+
* don't contain actual expressions. However they do contain OIDs which
1974+
* may be needed by dependency walkers etc.
1975+
*/
1976+
if ((flags & QTW_EXAMINE_SORTGROUP))
1977+
{
1978+
if (walker((Node *) query->groupClause, context))
1979+
return true;
1980+
if (walker((Node *) query->windowClause, context))
1981+
return true;
1982+
if (walker((Node *) query->sortClause, context))
1983+
return true;
1984+
if (walker((Node *) query->distinctClause, context))
1985+
return true;
1986+
}
1987+
else
1988+
{
1989+
/*
1990+
* But we need to walk the expressions under WindowClause nodes even
1991+
* if we're not interested in SortGroupClause nodes.
1992+
*/
1993+
ListCell *lc;
1994+
1995+
foreach(lc, query->windowClause)
1996+
{
1997+
WindowClause *wc = lfirst_node(WindowClause, lc);
1998+
1999+
if (walker(wc->startOffset, context))
2000+
return true;
2001+
if (walker(wc->endOffset, context))
2002+
return true;
2003+
}
2004+
}
2005+
2006+
/*
2007+
* groupingSets and rowMarks are not walked:
2008+
*
2009+
* groupingSets contain only ressortgrouprefs (integers) which are
2010+
* meaningless without the corresponding groupClause or tlist.
2011+
* Accordingly, any walker that needs to care about them needs to handle
2012+
* them itself in its Query processing.
2013+
*
2014+
* rowMarks is not walked because it contains only rangetable indexes (and
2015+
* flags etc.) and therefore should be handled at Query level similarly.
2016+
*/
2017+
19632018
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
19642019
{
19652020
if (walker((Node *) query->cteList, context))
@@ -2695,6 +2750,56 @@ query_tree_mutator(Query *query,
26952750
MUTATE(query->havingQual, query->havingQual, Node *);
26962751
MUTATE(query->limitOffset, query->limitOffset, Node *);
26972752
MUTATE(query->limitCount, query->limitCount, Node *);
2753+
2754+
/*
2755+
* Most callers aren't interested in SortGroupClause nodes since those
2756+
* don't contain actual expressions. However they do contain OIDs, which
2757+
* may be of interest to some mutators.
2758+
*/
2759+
2760+
if ((flags & QTW_EXAMINE_SORTGROUP))
2761+
{
2762+
MUTATE(query->groupClause, query->groupClause, List *);
2763+
MUTATE(query->windowClause, query->windowClause, List *);
2764+
MUTATE(query->sortClause, query->sortClause, List *);
2765+
MUTATE(query->distinctClause, query->distinctClause, List *);
2766+
}
2767+
else
2768+
{
2769+
/*
2770+
* But we need to mutate the expressions under WindowClause nodes even
2771+
* if we're not interested in SortGroupClause nodes.
2772+
*/
2773+
List *resultlist;
2774+
ListCell *temp;
2775+
2776+
resultlist = NIL;
2777+
foreach(temp, query->windowClause)
2778+
{
2779+
WindowClause *wc = lfirst_node(WindowClause, temp);
2780+
WindowClause *newnode;
2781+
2782+
FLATCOPY(newnode, wc, WindowClause);
2783+
MUTATE(newnode->startOffset, wc->startOffset, Node *);
2784+
MUTATE(newnode->endOffset, wc->endOffset, Node *);
2785+
2786+
resultlist = lappend(resultlist, (Node *) newnode);
2787+
}
2788+
query->windowClause = resultlist;
2789+
}
2790+
2791+
/*
2792+
* groupingSets and rowMarks are not mutated:
2793+
*
2794+
* groupingSets contain only ressortgroup refs (integers) which are
2795+
* meaningless without the groupClause or tlist. Accordingly, any mutator
2796+
* that needs to care about them needs to handle them itself in its Query
2797+
* processing.
2798+
*
2799+
* rowMarks contains only rangetable indexes (and flags etc.) and
2800+
* therefore should be handled at Query level similarly.
2801+
*/
2802+
26982803
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
26992804
MUTATE(query->cteList, query->cteList, List *);
27002805
else /* else copy CTE list as-is */

src/include/nodes/nodeFuncs.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#define QTW_IGNORE_RANGE_TABLE 0x08 /* skip rangetable entirely */
2525
#define QTW_EXAMINE_RTES 0x10 /* examine RTEs */
2626
#define QTW_DONT_COPY_QUERY 0x20 /* do not copy top Query */
27+
#define QTW_EXAMINE_SORTGROUP 0x80 /* include SortGroupNode lists */
2728

2829

2930
extern Oid exprType(const Node *expr);

src/test/regress/expected/window.out

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1781,3 +1781,45 @@ SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
17811781
5 | t | t | t
17821782
(5 rows)
17831783

1784+
-- Tests for problems with failure to walk or mutate expressions
1785+
-- within window frame clauses.
1786+
-- test walker (fails with collation error if expressions are not walked)
1787+
SELECT array_agg(i) OVER w
1788+
FROM generate_series(1,5) i
1789+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
1790+
array_agg
1791+
-----------
1792+
{1}
1793+
{1,2}
1794+
{2,3}
1795+
{3,4}
1796+
{4,5}
1797+
(5 rows)
1798+
1799+
-- test mutator (fails when inlined if expressions are not mutated)
1800+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
1801+
AS $$
1802+
SELECT array_agg(s) OVER w
1803+
FROM generate_series(1,5) s
1804+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
1805+
$$ LANGUAGE SQL STABLE;
1806+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
1807+
QUERY PLAN
1808+
------------------------------------------------------
1809+
Subquery Scan on f
1810+
-> WindowAgg
1811+
-> Sort
1812+
Sort Key: s.s
1813+
-> Function Scan on generate_series s
1814+
(5 rows)
1815+
1816+
SELECT * FROM pg_temp.f(2);
1817+
f
1818+
---------
1819+
{1,2,3}
1820+
{2,3,4}
1821+
{3,4,5}
1822+
{4,5}
1823+
{5}
1824+
(5 rows)
1825+

src/test/regress/sql/window.sql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,3 +621,22 @@ SELECT to_char(SUM(n::float8) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FO
621621
SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
622622
FROM (VALUES (1,true), (2,true), (3,false), (4,false), (5,true)) v(i,b)
623623
WINDOW w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING);
624+
625+
-- Tests for problems with failure to walk or mutate expressions
626+
-- within window frame clauses.
627+
628+
-- test walker (fails with collation error if expressions are not walked)
629+
SELECT array_agg(i) OVER w
630+
FROM generate_series(1,5) i
631+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
632+
633+
-- test mutator (fails when inlined if expressions are not mutated)
634+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
635+
AS $$
636+
SELECT array_agg(s) OVER w
637+
FROM generate_series(1,5) s
638+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
639+
$$ LANGUAGE SQL STABLE;
640+
641+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
642+
SELECT * FROM pg_temp.f(2);

0 commit comments

Comments
 (0)