Re: Possible bug: SQL function parameter in window frame definition - Mailing list pgsql-general

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> We could minimize the chance of breakage in a back-patched fix
 Andrew> by having query_tree_walker/mutator iterate the windowClause
 Andrew> list itself

Here is a draft patch along those lines; the intent of this one is that
no existing walker or mutator should need to change (the change to the
dependency code is basically cosmetic I believe, just avoids walking
some things twice).

Also added some tests.

-- 
Andrew (irc:RhodiumToad)

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..d063bee271 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -2296,6 +2296,33 @@ query_tree_walker(Query *query,
         return true;
     if (walker(query->limitCount, context))
         return true;
+    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
+    {
+        /*
+         * We need to walk the expressions in 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;
+        }
+    }
     if (!(flags & QTW_IGNORE_CTE_SUBQUERIES))
     {
         if (walker((Node *) query->cteList, context))
@@ -3153,6 +3180,38 @@ query_tree_mutator(Query *query,
     MUTATE(query->havingQual, query->havingQual, Node *);
     MUTATE(query->limitOffset, query->limitOffset, Node *);
     MUTATE(query->limitCount, query->limitCount, Node *);
+
+    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
+    {
+        /*
+         * We need to mutate the expressions in 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;
+    }
+
     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);