Skip to content

Commit ede0ab6

Browse files
RhodiumToadRhodiumToad
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 7ca3547 commit ede0ab6

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
@@ -1985,18 +1985,13 @@ find_expr_references_walker(Node *node,
19851985
context->addrs);
19861986
}
19871987

1988-
/* query_tree_walker ignores ORDER BY etc, but we need those opers */
1989-
find_expr_references_walker((Node *) query->sortClause, context);
1990-
find_expr_references_walker((Node *) query->groupClause, context);
1991-
find_expr_references_walker((Node *) query->windowClause, context);
1992-
find_expr_references_walker((Node *) query->distinctClause, context);
1993-
19941988
/* Examine substructure of query */
19951989
context->rtables = lcons(query->rtable, context->rtables);
19961990
result = query_tree_walker(query,
19971991
find_expr_references_walker,
19981992
(void *) context,
1999-
QTW_IGNORE_JOINALIASES);
1993+
QTW_IGNORE_JOINALIASES |
1994+
QTW_EXAMINE_SORTGROUP);
20001995
context->rtables = list_delete_first(context->rtables);
20011996
return result;
20021997
}

‎src/backend/nodes/nodeFuncs.c

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

2253+
/*
2254+
* We don't walk any utilityStmt here. However, we can't easily assert
2255+
* that it is absent, since there are at least two code paths by which
2256+
* action statements from CREATE RULE end up here, and NOTIFY is allowed
2257+
* in a rule action.
2258+
*/
2259+
22532260
if (walker((Node *) query->targetList, context))
22542261
return true;
22552262
if (walker((Node *) query->withCheckOptions, context))
@@ -2268,6 +2275,54 @@ query_tree_walker(Query *query,
22682275
return true;
22692276
if (walker(query->limitCount, context))
22702277
return true;
2278+
2279+
/*
2280+
* Most callers aren't interested in SortGroupClause nodes since those
2281+
* don't contain actual expressions. However they do contain OIDs which
2282+
* may be needed by dependency walkers etc.
2283+
*/
2284+
if ((flags & QTW_EXAMINE_SORTGROUP))
2285+
{
2286+
if (walker((Node *) query->groupClause, context))
2287+
return true;
2288+
if (walker((Node *) query->windowClause, context))
2289+
return true;
2290+
if (walker((Node *) query->sortClause, context))
2291+
return true;
2292+
if (walker((Node *) query->distinctClause, context))
2293+
return true;
2294+
}
2295+
else
2296+
{
2297+
/*
2298+
* But we need to walk the expressions under WindowClause nodes even
2299+
* if we're not interested in SortGroupClause nodes.
2300+
*/
2301+
ListCell *lc;
2302+
2303+
foreach(lc, query->windowClause)
2304+
{
2305+
WindowClause *wc = lfirst_node(WindowClause, lc);
2306+
2307+
if (walker(wc->startOffset, context))
2308+
return true;
2309+
if (walker(wc->endOffset, context))
2310+
return true;
2311+
}
2312+
}
2313+
2314+
/*
2315+
* groupingSets and rowMarks are not walked:
2316+
*
2317+
* groupingSets contain only ressortgrouprefs (integers) which are
2318+
* meaningless without the corresponding groupClause or tlist.
2319+
* Accordingly, any walker that needs to care about them needs to handle
2320+
* them itself in its Query processing.
2321+
*
2322+
* rowMarks is not walked because it contains only rangetable indexes (and
2323+
* flags etc.) and therefore should be handled at Query level similarly.
2324+
*/
2325+
22712326
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
22722327
{
22732328
if (walker((Node *) query->cteList, context))
@@ -3089,6 +3144,56 @@ query_tree_mutator(Query *query,
30893144
MUTATE(query->havingQual, query->havingQual, Node *);
30903145
MUTATE(query->limitOffset, query->limitOffset, Node *);
30913146
MUTATE(query->limitCount, query->limitCount, Node *);
3147+
3148+
/*
3149+
* Most callers aren't interested in SortGroupClause nodes since those
3150+
* don't contain actual expressions. However they do contain OIDs, which
3151+
* may be of interest to some mutators.
3152+
*/
3153+
3154+
if ((flags & QTW_EXAMINE_SORTGROUP))
3155+
{
3156+
MUTATE(query->groupClause, query->groupClause, List *);
3157+
MUTATE(query->windowClause, query->windowClause, List *);
3158+
MUTATE(query->sortClause, query->sortClause, List *);
3159+
MUTATE(query->distinctClause, query->distinctClause, List *);
3160+
}
3161+
else
3162+
{
3163+
/*
3164+
* But we need to mutate the expressions under WindowClause nodes even
3165+
* if we're not interested in SortGroupClause nodes.
3166+
*/
3167+
List *resultlist;
3168+
ListCell *temp;
3169+
3170+
resultlist = NIL;
3171+
foreach(temp, query->windowClause)
3172+
{
3173+
WindowClause *wc = lfirst_node(WindowClause, temp);
3174+
WindowClause *newnode;
3175+
3176+
FLATCOPY(newnode, wc, WindowClause);
3177+
MUTATE(newnode->startOffset, wc->startOffset, Node *);
3178+
MUTATE(newnode->endOffset, wc->endOffset, Node *);
3179+
3180+
resultlist = lappend(resultlist, (Node *) newnode);
3181+
}
3182+
query->windowClause = resultlist;
3183+
}
3184+
3185+
/*
3186+
* groupingSets and rowMarks are not mutated:
3187+
*
3188+
* groupingSets contain only ressortgroup refs (integers) which are
3189+
* meaningless without the groupClause or tlist. Accordingly, any mutator
3190+
* that needs to care about them needs to handle them itself in its Query
3191+
* processing.
3192+
*
3193+
* rowMarks contains only rangetable indexes (and flags etc.) and
3194+
* therefore should be handled at Query level similarly.
3195+
*/
3196+
30923197
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
30933198
MUTATE(query->cteList, query->cteList, List *);
30943199
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
/* callback function for check_functions_in_node */
2930
typedef bool (*check_function_callback) (Oid func_id, void *context);

‎src/test/regress/expected/window.out

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

1825+
-- Tests for problems with failure to walk or mutate expressions
1826+
-- within window frame clauses.
1827+
-- test walker (fails with collation error if expressions are not walked)
1828+
SELECT array_agg(i) OVER w
1829+
FROM generate_series(1,5) i
1830+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
1831+
array_agg
1832+
-----------
1833+
{1}
1834+
{1,2}
1835+
{2,3}
1836+
{3,4}
1837+
{4,5}
1838+
(5 rows)
1839+
1840+
-- test mutator (fails when inlined if expressions are not mutated)
1841+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
1842+
AS $$
1843+
SELECT array_agg(s) OVER w
1844+
FROM generate_series(1,5) s
1845+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
1846+
$$ LANGUAGE SQL STABLE;
1847+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
1848+
QUERY PLAN
1849+
------------------------------------------------------
1850+
Subquery Scan on f
1851+
-> WindowAgg
1852+
-> Sort
1853+
Sort Key: s.s
1854+
-> Function Scan on generate_series s
1855+
(5 rows)
1856+
1857+
SELECT * FROM pg_temp.f(2);
1858+
f
1859+
---------
1860+
{1,2,3}
1861+
{2,3,4}
1862+
{3,4,5}
1863+
{4,5}
1864+
{5}
1865+
(5 rows)
1866+

‎src/test/regress/sql/window.sql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -641,3 +641,22 @@ SELECT to_char(SUM(n::float8) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FO
641641
SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
642642
FROM (VALUES (1,true), (2,true), (3,false), (4,false), (5,true)) v(i,b)
643643
WINDOW w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING);
644+
645+
-- Tests for problems with failure to walk or mutate expressions
646+
-- within window frame clauses.
647+
648+
-- test walker (fails with collation error if expressions are not walked)
649+
SELECT array_agg(i) OVER w
650+
FROM generate_series(1,5) i
651+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
652+
653+
-- test mutator (fails when inlined if expressions are not mutated)
654+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
655+
AS $$
656+
SELECT array_agg(s) OVER w
657+
FROM generate_series(1,5) s
658+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
659+
$$ LANGUAGE SQL STABLE;
660+
661+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
662+
SELECT * FROM pg_temp.f(2);

0 commit comments

Comments
 (0)