Skip to content

Commit 174fab9

Browse files
RhodiumToadRhodiumToad
committed
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
1 parent feea385 commit 174fab9

File tree

3 files changed

+39
-6
lines changed

3 files changed

+39
-6
lines changed

‎src/backend/parser/analyze.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -391,11 +391,13 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
391391
qry->hasSubLinks = pstate->p_hasSubLinks;
392392
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
393393
qry->hasAggs = pstate->p_hasAggs;
394-
if (pstate->p_hasAggs)
395-
parseCheckAggregates(pstate, qry);
396394

397395
assign_query_collations(pstate, qry);
398396

397+
/* this must be done after collations, for reliable comparison of exprs */
398+
if (pstate->p_hasAggs)
399+
parseCheckAggregates(pstate, qry);
400+
399401
return qry;
400402
}
401403

@@ -1010,8 +1012,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
10101012
qry->hasSubLinks = pstate->p_hasSubLinks;
10111013
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
10121014
qry->hasAggs = pstate->p_hasAggs;
1013-
if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
1014-
parseCheckAggregates(pstate, qry);
10151015

10161016
foreach(l, stmt->lockingClause)
10171017
{
@@ -1021,6 +1021,10 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
10211021

10221022
assign_query_collations(pstate, qry);
10231023

1024+
/* this must be done after collations, for reliable comparison of exprs */
1025+
if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
1026+
parseCheckAggregates(pstate, qry);
1027+
10241028
return qry;
10251029
}
10261030

@@ -1470,8 +1474,6 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
14701474
qry->hasSubLinks = pstate->p_hasSubLinks;
14711475
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
14721476
qry->hasAggs = pstate->p_hasAggs;
1473-
if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
1474-
parseCheckAggregates(pstate, qry);
14751477

14761478
foreach(l, lockingClause)
14771479
{
@@ -1481,6 +1483,10 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
14811483

14821484
assign_query_collations(pstate, qry);
14831485

1486+
/* this must be done after collations, for reliable comparison of exprs */
1487+
if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
1488+
parseCheckAggregates(pstate, qry);
1489+
14841490
return qry;
14851491
}
14861492

‎src/test/regress/expected/aggregates.out

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,3 +1655,22 @@ select least_agg(variadic array[q1,q2]) from int8_tbl;
16551655
-4567890123456789
16561656
(1 row)
16571657

1658+
-- check collation-sensitive matching between grouping expressions
1659+
select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
1660+
from unnest(array['a','b']) u(v)
1661+
group by v||'a' order by 1;
1662+
?column? | case | count
1663+
----------+------+-------
1664+
aa | 1 | 1
1665+
ba | 0 | 1
1666+
(2 rows)
1667+
1668+
select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
1669+
from unnest(array['a','b']) u(v)
1670+
group by v||'a' order by 1;
1671+
?column? | case | count
1672+
----------+------+-------
1673+
aa | 1 | 1
1674+
ba | 0 | 1
1675+
(2 rows)
1676+

‎src/test/regress/sql/aggregates.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,3 +612,11 @@ drop view aggordview1;
612612
-- variadic aggregates
613613
select least_agg(q1,q2) from int8_tbl;
614614
select least_agg(variadic array[q1,q2]) from int8_tbl;
615+
616+
-- check collation-sensitive matching between grouping expressions
617+
select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
618+
from unnest(array['a','b']) u(v)
619+
group by v||'a' order by 1;
620+
select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
621+
from unnest(array['a','b']) u(v)
622+
group by v||'a' order by 1;

0 commit comments

Comments
 (0)