Don't flatten join alias Vars that are stored within a GROUP RTE.

The RTE's groupexprs list is used for deparsing views, and for that
usage it must contain the original alias Vars; else we can get
incorrect SQL output.  But since commit 247dea89f,
parseCheckAggregates put the GROUP BY expressions through
flatten_join_alias_vars before building the RTE_GROUP RTE.
Changing the order of operations there is enough to fix it.

This patch unfortunately can do nothing for already-created views:
if they use a coding pattern that is subject to the bug, they will
deparse incorrectly and hence present a dump/reload hazard in the
future.  The only fix is to recreate the view from the original SQL.
But the trouble cases seem to be quite narrow.  AFAICT the output
was only wrong for "SELECT ... t1 LEFT JOIN t2 USING (x) GROUP BY x"
where t1.x and t2.x were not of identical data types and t1.x was
the side that required an implicit coercion.  If there was no hidden
coercion, or if the join was plain, RIGHT, or FULL, the deparsed
output was uglier than intended but not functionally wrong.

Reported-by: Swirl Smog Dowry <swirl-smog-dowry@duck.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CA+-gibjCg_vjcq3hWTM0sLs3_TUZ6Q9rkv8+pe2yJrdh4o4uoQ@mail.gmail.com
Backpatch-through: 18

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 25ee0f87d93209091f7ee116b448a234a9b7a6d6..d0187ea84a0835e5fe79c97fc3b267b437412c74 100644 (file)
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -1213,8 +1213,8 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
        }
 
        /*
-        * Build a list of the acceptable GROUP BY expressions for use by
-        * substitute_grouped_columns().
+        * Build a list of the acceptable GROUP BY expressions to save in the
+        * RTE_GROUP RTE, and for use by substitute_grouped_columns().
         *
         * We get the TLE, not just the expr, because GROUPING wants to know the
         * sortgroupref.
@@ -1231,6 +1231,23 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
                groupClauses = lappend(groupClauses, expr);
        }
 
+       /*
+        * If there are any acceptable GROUP BY expressions, build an RTE and
+        * nsitem for the result of the grouping step.  (It's important to do this
+        * before flattening join alias vars in groupClauses, because the RTE
+        * should preserve any alias vars that were in the input.)
+        */
+       if (groupClauses)
+       {
+               pstate->p_grouping_nsitem =
+                       addRangeTableEntryForGroup(pstate, groupClauses);
+
+               /* Set qry->rtable again in case it was previously NIL */
+               qry->rtable = pstate->p_rtable;
+               /* Mark the Query as having RTE_GROUP RTE */
+               qry->hasGroupRTE = true;
+       }
+
        /*
         * If there are join alias vars involved, we have to flatten them to the
         * underlying vars, so that aliased and unaliased vars will be correctly
@@ -1266,21 +1283,6 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
                }
        }
 
-       /*
-        * If there are any acceptable GROUP BY expressions, build an RTE and
-        * nsitem for the result of the grouping step.
-        */
-       if (groupClauses)
-       {
-               pstate->p_grouping_nsitem =
-                       addRangeTableEntryForGroup(pstate, groupClauses);
-
-               /* Set qry->rtable again in case it was previously NIL */
-               qry->rtable = pstate->p_rtable;
-               /* Mark the Query as having RTE_GROUP RTE */
-               qry->hasGroupRTE = true;
-       }
-
        /*
         * Replace grouped variables in the targetlist and HAVING clause with Vars
         * that reference the RTE_GROUP RTE.  Emit an error message if we find any

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index cae8e7bca313fa7e47f640c98232bde7d48abd54..bbd4554fa4f11d7653b6f44673b9bcb71edc61fd 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1794,6 +1794,19 @@ group by f2;
 ----+-------
 (0 rows)
 
+-- check that we preserve join alias in GROUP BY expressions
+create temp view v1 as
+select f1::int from t1 left join t2 using (f1) group by f1;
+select pg_get_viewdef('v1'::regclass);
+        pg_get_viewdef         
+-------------------------------
+  SELECT (f1)::integer AS f1  +
+    FROM (t1                  +
+      LEFT JOIN t2 USING (f1))+
+   GROUP BY f1;
+(1 row)
+
+drop view v1;
 drop table t1, t2;
 --
 -- Test planner's selection of pathkeys for ORDER BY aggregates

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 850f5a5787f52a23af2a3f545d47d346b9fe9712..5992cb7ca9b8f5273969032bb1923a2f5109fc67 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -649,6 +649,12 @@ select f2, count(*) from
 t1 x(x0,x1) left join (t1 left join t2 using(f2)) on (x0 = 0)
 group by f2;
 
+-- check that we preserve join alias in GROUP BY expressions
+create temp view v1 as
+select f1::int from t1 left join t2 using (f1) group by f1;
+select pg_get_viewdef('v1'::regclass);
+
+drop view v1;
 drop table t1, t2;
 
 --