Skip to content

Commit b7a1c55

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 b1c1aa5 commit b7a1c55

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
@@ -2214,18 +2214,13 @@ find_expr_references_walker(Node *node,
22142214
context->addrs);
22152215
}
22162216

2217-
/* query_tree_walker ignores ORDER BY etc, but we need those opers */
2218-
find_expr_references_walker((Node *) query->sortClause, context);
2219-
find_expr_references_walker((Node *) query->groupClause, context);
2220-
find_expr_references_walker((Node *) query->windowClause, context);
2221-
find_expr_references_walker((Node *) query->distinctClause, context);
2222-
22232217
/* Examine substructure of query */
22242218
context->rtables = lcons(query->rtable, context->rtables);
22252219
result = query_tree_walker(query,
22262220
find_expr_references_walker,
22272221
(void *) context,
2228-
QTW_IGNORE_JOINALIASES);
2222+
QTW_IGNORE_JOINALIASES |
2223+
QTW_EXAMINE_SORTGROUP);
22292224
context->rtables = list_delete_first(context->rtables);
22302225
return result;
22312226
}

‎src/backend/nodes/nodeFuncs.c

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

2281+
/*
2282+
* We don't walk any utilityStmt here. However, we can't easily assert
2283+
* that it is absent, since there are at least two code paths by which
2284+
* action statements from CREATE RULE end up here, and NOTIFY is allowed
2285+
* in a rule action.
2286+
*/
2287+
22812288
if (walker((Node *) query->targetList, context))
22822289
return true;
22832290
if (walker((Node *) query->withCheckOptions, context))
@@ -2296,6 +2303,54 @@ query_tree_walker(Query *query,
22962303
return true;
22972304
if (walker(query->limitCount, context))
22982305
return true;
2306+
2307+
/*
2308+
* Most callers aren't interested in SortGroupClause nodes since those
2309+
* don't contain actual expressions. However they do contain OIDs which
2310+
* may be needed by dependency walkers etc.
2311+
*/
2312+
if ((flags & QTW_EXAMINE_SORTGROUP))
2313+
{
2314+
if (walker((Node *) query->groupClause, context))
2315+
return true;
2316+
if (walker((Node *) query->windowClause, context))
2317+
return true;
2318+
if (walker((Node *) query->sortClause, context))
2319+
return true;
2320+
if (walker((Node *) query->distinctClause, context))
2321+
return true;
2322+
}
2323+
else
2324+
{
2325+
/*
2326+
* But we need to walk the expressions under WindowClause nodes even
2327+
* if we're not interested in SortGroupClause nodes.
2328+
*/
2329+
ListCell *lc;
2330+
2331+
foreach(lc, query->windowClause)
2332+
{
2333+
WindowClause *wc = lfirst_node(WindowClause, lc);
2334+
2335+
if (walker(wc->startOffset, context))
2336+
return true;
2337+
if (walker(wc->endOffset, context))
2338+
return true;
2339+
}
2340+
}
2341+
2342+
/*
2343+
* groupingSets and rowMarks are not walked:
2344+
*
2345+
* groupingSets contain only ressortgrouprefs (integers) which are
2346+
* meaningless without the corresponding groupClause or tlist.
2347+
* Accordingly, any walker that needs to care about them needs to handle
2348+
* them itself in its Query processing.
2349+
*
2350+
* rowMarks is not walked because it contains only rangetable indexes (and
2351+
* flags etc.) and therefore should be handled at Query level similarly.
2352+
*/
2353+
22992354
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
23002355
{
23012356
if (walker((Node *) query->cteList, context))
@@ -3153,6 +3208,56 @@ query_tree_mutator(Query *query,
31533208
MUTATE(query->havingQual, query->havingQual, Node *);
31543209
MUTATE(query->limitOffset, query->limitOffset, Node *);
31553210
MUTATE(query->limitCount, query->limitCount, Node *);
3211+
3212+
/*
3213+
* Most callers aren't interested in SortGroupClause nodes since those
3214+
* don't contain actual expressions. However they do contain OIDs, which
3215+
* may be of interest to some mutators.
3216+
*/
3217+
3218+
if ((flags & QTW_EXAMINE_SORTGROUP))
3219+
{
3220+
MUTATE(query->groupClause, query->groupClause, List *);
3221+
MUTATE(query->windowClause, query->windowClause, List *);
3222+
MUTATE(query->sortClause, query->sortClause, List *);
3223+
MUTATE(query->distinctClause, query->distinctClause, List *);
3224+
}
3225+
else
3226+
{
3227+
/*
3228+
* But we need to mutate the expressions under WindowClause nodes even
3229+
* if we're not interested in SortGroupClause nodes.
3230+
*/
3231+
List *resultlist;
3232+
ListCell *temp;
3233+
3234+
resultlist = NIL;
3235+
foreach(temp, query->windowClause)
3236+
{
3237+
WindowClause *wc = lfirst_node(WindowClause, temp);
3238+
WindowClause *newnode;
3239+
3240+
FLATCOPY(newnode, wc, WindowClause);
3241+
MUTATE(newnode->startOffset, wc->startOffset, Node *);
3242+
MUTATE(newnode->endOffset, wc->endOffset, Node *);
3243+
3244+
resultlist = lappend(resultlist, (Node *) newnode);
3245+
}
3246+
query->windowClause = resultlist;
3247+
}
3248+
3249+
/*
3250+
* groupingSets and rowMarks are not mutated:
3251+
*
3252+
* groupingSets contain only ressortgroup refs (integers) which are
3253+
* meaningless without the groupClause or tlist. Accordingly, any mutator
3254+
* that needs to care about them needs to handle them itself in its Query
3255+
* processing.
3256+
*
3257+
* rowMarks contains only rangetable indexes (and flags etc.) and
3258+
* therefore should be handled at Query level similarly.
3259+
*/
3260+
31563261
if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
31573262
MUTATE(query->cteList, query->cteList, List *);
31583263
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
@@ -27,6 +27,7 @@
2727
#define QTW_EXAMINE_RTES_AFTER 0x20 /* examine RTE nodes after their
2828
* contents */
2929
#define QTW_DONT_COPY_QUERY 0x40 /* do not copy top Query */
30+
#define QTW_EXAMINE_SORTGROUP 0x80 /* include SortGroupNode lists */
3031

3132
/* callback function for check_functions_in_node */
3233
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
@@ -3821,3 +3821,45 @@ SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
38213821
5 | t | t | t
38223822
(5 rows)
38233823

3824+
-- Tests for problems with failure to walk or mutate expressions
3825+
-- within window frame clauses.
3826+
-- test walker (fails with collation error if expressions are not walked)
3827+
SELECT array_agg(i) OVER w
3828+
FROM generate_series(1,5) i
3829+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
3830+
array_agg
3831+
-----------
3832+
{1}
3833+
{1,2}
3834+
{2,3}
3835+
{3,4}
3836+
{4,5}
3837+
(5 rows)
3838+
3839+
-- test mutator (fails when inlined if expressions are not mutated)
3840+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
3841+
AS $$
3842+
SELECT array_agg(s) OVER w
3843+
FROM generate_series(1,5) s
3844+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
3845+
$$ LANGUAGE SQL STABLE;
3846+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
3847+
QUERY PLAN
3848+
------------------------------------------------------
3849+
Subquery Scan on f
3850+
-> WindowAgg
3851+
-> Sort
3852+
Sort Key: s.s
3853+
-> Function Scan on generate_series s
3854+
(5 rows)
3855+
3856+
SELECT * FROM pg_temp.f(2);
3857+
f
3858+
---------
3859+
{1,2,3}
3860+
{2,3,4}
3861+
{3,4,5}
3862+
{4,5}
3863+
{5}
3864+
(5 rows)
3865+

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1257,3 +1257,22 @@ SELECT to_char(SUM(n::float8) OVER (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FO
12571257
SELECT i, b, bool_and(b) OVER w, bool_or(b) OVER w
12581258
FROM (VALUES (1,true), (2,true), (3,false), (4,false), (5,true)) v(i,b)
12591259
WINDOW w AS (ORDER BY i ROWS BETWEEN CURRENT ROW AND 1 FOLLOWING);
1260+
1261+
-- Tests for problems with failure to walk or mutate expressions
1262+
-- within window frame clauses.
1263+
1264+
-- test walker (fails with collation error if expressions are not walked)
1265+
SELECT array_agg(i) OVER w
1266+
FROM generate_series(1,5) i
1267+
WINDOW w AS (ORDER BY i ROWS BETWEEN (('foo' < 'foobar')::integer) PRECEDING AND CURRENT ROW);
1268+
1269+
-- test mutator (fails when inlined if expressions are not mutated)
1270+
CREATE FUNCTION pg_temp.f(group_size BIGINT) RETURNS SETOF integer[]
1271+
AS $$
1272+
SELECT array_agg(s) OVER w
1273+
FROM generate_series(1,5) s
1274+
WINDOW w AS (ORDER BY s ROWS BETWEEN CURRENT ROW AND GROUP_SIZE FOLLOWING)
1275+
$$ LANGUAGE SQL STABLE;
1276+
1277+
EXPLAIN (costs off) SELECT * FROM pg_temp.f(2);
1278+
SELECT * FROM pg_temp.f(2);

0 commit comments

Comments
 (0)