Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit e74d8c5

Browse files
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 6afea53 commit e74d8c5

File tree

5 files changed

+75
-6
lines changed

5 files changed

+75
-6
lines changed

src/backend/parser/analyze.c

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -451,11 +451,13 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
451451
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
452452
qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
453453
qry->hasAggs = pstate->p_hasAggs;
454-
if (pstate->p_hasAggs)
455-
parseCheckAggregates(pstate, qry);
456454

457455
assign_query_collations(pstate, qry);
458456

457+
/* this must be done after collations, for reliable comparison of exprs */
458+
if (pstate->p_hasAggs)
459+
parseCheckAggregates(pstate, qry);
460+
459461
return qry;
460462
}
461463

@@ -1318,8 +1320,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
13181320
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
13191321
qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
13201322
qry->hasAggs = pstate->p_hasAggs;
1321-
if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
1322-
parseCheckAggregates(pstate, qry);
13231323

13241324
foreach(l, stmt->lockingClause)
13251325
{
@@ -1329,6 +1329,10 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
13291329

13301330
assign_query_collations(pstate, qry);
13311331

1332+
/* this must be done after collations, for reliable comparison of exprs */
1333+
if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
1334+
parseCheckAggregates(pstate, qry);
1335+
13321336
return qry;
13331337
}
13341338

@@ -1790,8 +1794,6 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
17901794
qry->hasWindowFuncs = pstate->p_hasWindowFuncs;
17911795
qry->hasTargetSRFs = pstate->p_hasTargetSRFs;
17921796
qry->hasAggs = pstate->p_hasAggs;
1793-
if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
1794-
parseCheckAggregates(pstate, qry);
17951797

17961798
foreach(l, lockingClause)
17971799
{
@@ -1801,6 +1803,10 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
18011803

18021804
assign_query_collations(pstate, qry);
18031805

1806+
/* this must be done after collations, for reliable comparison of exprs */
1807+
if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual)
1808+
parseCheckAggregates(pstate, qry);
1809+
18041810
return qry;
18051811
}
18061812

src/test/regress/expected/aggregates.out

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2116,3 +2116,22 @@ SELECT min(x ORDER BY y) FROM (VALUES(1, 2)) AS d(x,y);
21162116
1
21172117
(1 row)
21182118

2119+
-- check collation-sensitive matching between grouping expressions
2120+
select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
2121+
from unnest(array['a','b']) u(v)
2122+
group by v||'a' order by 1;
2123+
?column? | case | count
2124+
----------+------+-------
2125+
aa | 1 | 1
2126+
ba | 0 | 1
2127+
(2 rows)
2128+
2129+
select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
2130+
from unnest(array['a','b']) u(v)
2131+
group by v||'a' order by 1;
2132+
?column? | case | count
2133+
----------+------+-------
2134+
aa | 1 | 1
2135+
ba | 0 | 1
2136+
(2 rows)
2137+

src/test/regress/expected/groupingsets.out

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1540,4 +1540,29 @@ explain (costs off)
15401540
-> Seq Scan on tenk1
15411541
(12 rows)
15421542

1543+
-- check collation-sensitive matching between grouping expressions
1544+
-- (similar to a check for aggregates, but there are additional code
1545+
-- paths for GROUPING, so check again here)
1546+
select v||'a', case grouping(v||'a') when 1 then 1 else 0 end, count(*)
1547+
from unnest(array[1,1], array['a','b']) u(i,v)
1548+
group by rollup(i, v||'a') order by 1,3;
1549+
?column? | case | count
1550+
----------+------+-------
1551+
aa | 0 | 1
1552+
ba | 0 | 1
1553+
| 1 | 2
1554+
| 1 | 2
1555+
(4 rows)
1556+
1557+
select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
1558+
from unnest(array[1,1], array['a','b']) u(i,v)
1559+
group by rollup(i, v||'a') order by 1,3;
1560+
?column? | case | count
1561+
----------+------+-------
1562+
aa | 0 | 1
1563+
ba | 0 | 1
1564+
| 1 | 2
1565+
| 1 | 2
1566+
(4 rows)
1567+
15431568
-- end

src/test/regress/sql/aggregates.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -935,3 +935,11 @@ SELECT dense_rank(x) WITHIN GROUP (ORDER BY x) FROM (VALUES (1),(1),(2),(2),(3),
935935
-- 2a505161-2727-2473-7c46-591ed108ac52@email.cz
936936
SELECT min(x ORDER BY y) FROM (VALUES(1, NULL)) AS d(x,y);
937937
SELECT min(x ORDER BY y) FROM (VALUES(1, 2)) AS d(x,y);
938+
939+
-- check collation-sensitive matching between grouping expressions
940+
select v||'a', case v||'a' when 'aa' then 1 else 0 end, count(*)
941+
from unnest(array['a','b']) u(v)
942+
group by v||'a' order by 1;
943+
select v||'a', case when v||'a' = 'aa' then 1 else 0 end, count(*)
944+
from unnest(array['a','b']) u(v)
945+
group by v||'a' order by 1;

src/test/regress/sql/groupingsets.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,4 +415,15 @@ explain (costs off)
415415
count(*)
416416
from tenk1 group by grouping sets (unique1,twothousand,thousand,hundred,ten,four,two);
417417

418+
-- check collation-sensitive matching between grouping expressions
419+
-- (similar to a check for aggregates, but there are additional code
420+
-- paths for GROUPING, so check again here)
421+
422+
select v||'a', case grouping(v||'a') when 1 then 1 else 0 end, count(*)
423+
from unnest(array[1,1], array['a','b']) u(i,v)
424+
group by rollup(i, v||'a') order by 1,3;
425+
select v||'a', case when grouping(v||'a') = 1 then 1 else 0 end, count(*)
426+
from unnest(array[1,1], array['a','b']) u(i,v)
427+
group by rollup(i, v||'a') order by 1,3;
428+
418429
-- end

0 commit comments

Comments
 (0)