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 c601b6d40d16106e3c89c83c901bd13437963b75..3aa3d8a7f5f16fbd701b5d438db0f3913c8aefc6 100644 (file)
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -451,11 +451,13 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
    qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
    qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
    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;
 }
 
@@ -1318,8 +1320,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
    qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
    qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
    qry->hasAggs = pstate->p_hasAggs;
-   if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
-       parseCheckAggregates(pstate, qry);
 
    foreach(l, stmt->lockingClause)
    {
@@ -1329,6 +1329,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;
 }
 
@@ -1790,8 +1794,6 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
    qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
    qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
    qry->hasAggs = pstate->p_hasAggs;
-   if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
-       parseCheckAggregates(pstate, qry);
 
    foreach(l, lockingClause)
    {
@@ -1801,6 +1803,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 a21bf1dc6b88eb93e17af20d79303bed5616e279..deed5bef469f523c1d8884863e6db309cdbda17e 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -2116,3 +2116,22 @@ SELECT min(x ORDER BY y) FROM (VALUES(1, 2)) AS d(x,y);
    1
 (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 c7deec2ff402667fc2a21e990e4cabddf78c41bb..381ebce8a1e876b1d3fdf5babb95e7f473afa808 100644 (file)
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -1540,4 +1540,29 @@ explain (costs off)
          ->  Seq Scan on tenk1
 (12 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 b2d8583e09a8d6ad58792599fe60cec1cc95caed..a03f897bcc2d748e4d7c3342c6c7af6081251acb 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -935,3 +935,11 @@ SELECT dense_rank(x) WITHIN GROUP (ORDER BY x) FROM (VALUES (1),(1),(2),(2),(3),
 -- 2a505161-2727-2473-7c46-591ed108ac52@email.cz
 SELECT min(x ORDER BY y) FROM (VALUES(1, NULL)) AS d(x,y);
 SELECT min(x ORDER BY y) FROM (VALUES(1, 2)) AS d(x,y);
+
+-- 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 c32d23b8d72d9759469c25bf57509c65f5d1a29e..5d6485913b3220954f0846ab43f4059d021842fe 100644 (file)
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -415,4 +415,15 @@ explain (costs off)
          count(*)
     from tenk1 group by grouping sets (unique1,twothousand,thousand,hundred,ten,four,two);
 
+-- 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