From: Andrew Gierth Date: Thu, 17 Jan 2019 05:33:01 +0000 (+0000) Subject: Postpone aggregate checks until after collation is assigned. X-Git-Tag: REL9_5_16~33 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=91448e7dcc1feb14e7ee05c15b157796331fa9e9;p=postgresql 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 34472a893a..1155d7ad0a 100644 --- 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 99ca3271e2..0a675c9afb 100644 --- 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 42020592f8..30e780f0b8 100644 --- 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 c12a2d8b7b..a6310dd18f 100644 --- 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 5704762705..ce920b79e4 100644 --- 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