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 cf1c123b724ee7960d34b104b53ebd6995de0d21..502b10368543e94933d9e2994df3bdb9331a1749 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -418,11 +418,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;
 }
 
@@ -1255,8 +1257,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)
        {
@@ -1266,6 +1266,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;
 }
 
@@ -1715,8 +1719,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)
        {
@@ -1726,6 +1728,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 e768b1fe6d59bb340005823b60c1a8c253528ef8..a1663f08c2270414616a9cf58cb36db659b19a57 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -2047,3 +2047,22 @@ SELECT balk(hundred) FROM tenk1;
 (1 row)
 
 ROLLBACK;
+-- 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 976050c2d07f242a147febefd89136d402198d7a..44fe5afcdcdc5e28ccf712f15b300f847b62697f 100644 (file)
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -921,4 +921,29 @@ select sum(ten) from onek group by rollup(four::text), two order by 1;
  2500
 (6 rows)
 
+-- 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 cb28f4cffedba4f30d4faef1d13be84bc9375940..7e129f74bc2de3b0814d61a094ab787083234d97 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -900,3 +900,11 @@ EXPLAIN (COSTS OFF) SELECT balk(hundred) FROM tenk1;
 SELECT balk(hundred) FROM tenk1;
 
 ROLLBACK;
+
+-- 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 77cdb5acba4afb5ee829fe98f02ef91113ec92a7..85830e92d26dbef5efd90f93b32fbb2532f392b0 100644 (file)
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -255,4 +255,15 @@ select array(select row(v.a,s1.*) from (select two,four, count(*) from onek grou
 select sum(ten) from onek group by two, rollup(four::text) order by 1;
 select sum(ten) from onek group by rollup(four::text), two order by 1;
 
+-- 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