Postpone aggregate checks until after collation is assigned.

Previously, parseCheckAggregates was run before
assign_query_collations, but this causes problems if any expression
has already had a collation assigned by some transform function (e.g.
transformCaseExpr) before parseCheckAggregates runs. The differing
collations would cause expressions not to be recognized as equal to
the ones in the GROUP BY clause, leading to spurious errors about
unaggregated column references.

The result was that CASE expr WHEN val ... would fail when "expr"
contained a GROUPING() expression or matched one of the group by
expressions, and where collatable types were involved; whereas the
supposedly identical CASE WHEN expr = val ... would succeed.

Backpatch all the way; this appears to have been wrong ever since
collations were introduced.

Per report from Guillaume Lelarge, analysis and patch by me.

Discussion: https://postgr.es/m/CAECtzeVSO_US8C2Khgfv54ZMUOBR4sWq+6_bLrETnWExHT=rFg@mail.gmail.com
Discussion: https://postgr.es/m/87muo0k0c7.fsf@news-spur.riddles.org.uk

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 34472a893a47efda8f54daec08323920080e2151..1155d7ad0a10e591935224e25b8056c5e0c7c2b1 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -395,11 +395,13 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
        qry->hasSubLinks = pstate->p_hasSubLinks;
        qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
        qry->hasAggs = pstate->p_hasAggs;
-       if (pstate->p_hasAggs)
-               parseCheckAggregates(pstate, qry);
 
        assign_query_collations(pstate, qry);
 
+       /* this must be done after collations, for reliable comparison of exprs */
+       if (pstate->p_hasAggs)
+               parseCheckAggregates(pstate, qry);
+
        return qry;
 }
 
@@ -1184,8 +1186,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
        qry->hasSubLinks = pstate->p_hasSubLinks;
        qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
        qry->hasAggs = pstate->p_hasAggs;
-       if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
-               parseCheckAggregates(pstate, qry);
 
        foreach(l, stmt->lockingClause)
        {
@@ -1195,6 +1195,10 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
 
        assign_query_collations(pstate, qry);
 
+       /* this must be done after collations, for reliable comparison of exprs */
+       if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
+               parseCheckAggregates(pstate, qry);
+
        return qry;
 }
 
@@ -1644,8 +1648,6 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
        qry->hasSubLinks = pstate->p_hasSubLinks;
        qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
        qry->hasAggs = pstate->p_hasAggs;
-       if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
-               parseCheckAggregates(pstate, qry);
 
        foreach(l, lockingClause)
        {
@@ -1655,6 +1657,10 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
 
        assign_query_collations(pstate, qry);
 
+       /* this must be done after collations, for reliable comparison of exprs */
+       if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
+               parseCheckAggregates(pstate, qry);
+
        return qry;
 }
 

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 99ca3271e20d97c3a4df3285e4f6fad27ef70c4f..0a675c9afb71a374aaeba4363d592b6269e24786 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1655,3 +1655,22 @@ select least_agg(variadic array[q1,q2]) from int8_tbl;
  -4567890123456789
 (1 row)
 
+-- check collation-sensitive matching between grouping expressions
+select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
+  from unnest(array['a','b']) u(v)
+ group by v||'a' order by 1;
+ ?column? | case | count 
+----------+------+-------
+ aa       |    1 |     1
+ ba       |    0 |     1
+(2 rows)
+
+select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
+  from unnest(array['a','b']) u(v)
+ group by v||'a' order by 1;
+ ?column? | case | count 
+----------+------+-------
+ aa       |    1 |     1
+ ba       |    0 |     1
+(2 rows)
+

diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 42020592f89ae4392a9ab1d03ace48da60cdf5c0..30e780f0b8bde3cca5acf3ee75a7d559646d7756 100644 (file)
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -899,4 +899,29 @@ DETAIL:  Some of the datatypes do not support sorting, which is required for gro
 select id, usort, grouping(id,usort), count(*) from (values (1,'1'::xid),(1,'2'::xid),(2,'1'::xid)) v(id,usort) group by grouping sets ((id), (usort));
 ERROR:  could not implement GROUP BY
 DETAIL:  Some of the datatypes do not support sorting, which is required for grouping sets.
+-- check collation-sensitive matching between grouping expressions
+-- (similar to a check for aggregates, but there are additional code
+-- paths for GROUPING, so check again here)
+select v||'a', case grouping(v||'a') when 1 then 1 else 0 end, count(*)
+  from unnest(array[1,1], array['a','b']) u(i,v)
+ group by rollup(i, v||'a') order by 1,3;
+ ?column? | case | count 
+----------+------+-------
+ aa       |    0 |     1
+ ba       |    0 |     1
+          |    1 |     2
+          |    1 |     2
+(4 rows)
+
+select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
+  from unnest(array[1,1], array['a','b']) u(i,v)
+ group by rollup(i, v||'a') order by 1,3;
+ ?column? | case | count 
+----------+------+-------
+ aa       |    0 |     1
+ ba       |    0 |     1
+          |    1 |     2
+          |    1 |     2
+(4 rows)
+
 -- end

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index c12a2d8b7b6ae79841383ea92583c6e6ced38810..a6310dd18f54abf4b3a82d8b3eccde276ce6c13e 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -612,3 +612,11 @@ drop view aggordview1;
 -- variadic aggregates
 select least_agg(q1,q2) from int8_tbl;
 select least_agg(variadic array[q1,q2]) from int8_tbl;
+
+-- check collation-sensitive matching between grouping expressions
+select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
+  from unnest(array['a','b']) u(v)
+ group by v||'a' order by 1;
+select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
+  from unnest(array['a','b']) u(v)
+ group by v||'a' order by 1;

diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 57047627058d1096a161cfda99119a6ea50f0fd0..ce920b79e410f22163d6ac331b95d74c6ab11b21 100644 (file)
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -248,4 +248,15 @@ select sum(ten) from onek group by rollup(four::text), two order by 1;
 select usort, grouping(usort), count(*) from (values ('1'::xid),('2'::xid),('1'::xid)) v(usort) group by rollup(usort);
 select id, usort, grouping(id,usort), count(*) from (values (1,'1'::xid),(1,'2'::xid),(2,'1'::xid)) v(id,usort) group by grouping sets ((id), (usort));
 
+-- check collation-sensitive matching between grouping expressions
+-- (similar to a check for aggregates, but there are additional code
+-- paths for GROUPING, so check again here)
+
+select v||'a', case grouping(v||'a') when 1 then 1 else 0 end, count(*)
+  from unnest(array[1,1], array['a','b']) u(i,v)
+ group by rollup(i, v||'a') order by 1,3;
+select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
+  from unnest(array[1,1], array['a','b']) u(i,v)
+ group by rollup(i, v||'a') order by 1,3;
+
 -- end