]> granicus.if.org Git - postgresql/commitdiff
Selectively include window frames in expression walks/mutates.
authorAndrew Gierth <rhodiumtoad@postgresql.org>
Thu, 3 Oct 2019 09:54:52 +0000 (10:54 +0100)
committerAndrew Gierth <rhodiumtoad@postgresql.org>
Thu, 3 Oct 2019 10:14:30 +0000 (11:14 +0100)
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
src/backend/nodes/nodeFuncs.c
src/include/nodes/nodeFuncs.h
src/test/regress/expected/window.out
src/test/regress/sql/window.sql

index a03347648308e785edf81269e3db5ec0a91e3723..6a03e9c4d3dd4b723b17d23edfe02dea06fa6099 100644 (file)
@@ -2013,18 +2013,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;
        }
index 54b3dcff3ec3c91a112710efd3117314752039d5..a41f7fc40607f64784d7c80a61cf8b537c740471 100644 (file)
@@ -2260,6 +2260,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))
@@ -2278,6 +2285,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))
@@ -3114,6 +3169,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 */
index 849f34d2a8e3202b89ae5f947b2e85fcb3440a9b..0c9a2bc46956c13a4180c53de4fca9afa742fd55 100644 (file)
@@ -24,6 +24,7 @@
 #define QTW_IGNORE_RANGE_TABLE         0x08    /* skip rangetable entirely */
 #define QTW_EXAMINE_RTES                       0x10    /* examine RTEs */
 #define QTW_DONT_COPY_QUERY                    0x20    /* 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);
index 562006a2b82839a298ed60884db2d7ad53676681..954f4a0c1c53d5fabf707316a80453f993149955 100644 (file)
@@ -3787,3 +3787,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)
+
index e2943a38f1e96c1befe504aae91d14a72bbd83b1..e28aec806cce856f297aa5ad5601a1b1675c01a0 100644 (file)
@@ -1241,3 +1241,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);