Re: pg_get_viewdef() produces non-round-trippable SQL for views with USING join on mismatched integer types - Mailing list pgsql-bugs

I wrote:
> The first groupexpr is the same as the joinaliasvars entry for that
> column in the JOIN RTE.  This surprises me: I'd expect to see a
> reference to the join output column there, ie Var 3/1, because I'm
> pretty sure that's what parsing of "GROUP BY year" would have produced
> initially.  If it were like that, I think ruleutils would produce the
> desired output.  So I'd tentatively classify this as "join alias Vars
> are being flattened too soon".  Richard, any thoughts?

The problem is obvious after looking at parseCheckAggregates: the
RTE_GROUP RTE is manufactured using the groupClauses list after we
have flattened that, which we are only doing for comparison purposes;
it shouldn't affect what goes into the parse tree.  I experimented
with just changing the order of operations, and that seems to fix it.
The lack of any effect on check-world shows we need more test cases
here ...

            regards, tom lane

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 25ee0f87d93..d0187ea84a0 100644
--- 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