Skip to content

Commit 887a232

Browse files
david-rowleydavid-rowley
committed
Fix issue with ORDER BY / DISTINCT aggregates and FILTER
1349d27 added support so that aggregate functions with an ORDER BY or DISTINCT clause could make use of presorted inputs to avoid an implicit sort within nodeAgg.c. That commit failed to consider that a FILTER clause may exist that filters rows before the aggregate function arguments are evaluated. That can be problematic if an aggregate argument contains an expression which could error out during evaluation. It's perfectly valid to want to have a FILTER clause which eliminates such values, and with the pre-sorted path added in 1349d27, it was possible that the planner would produce a plan with a Sort node above the Aggregate to perform the sort on the aggregate's arguments long before the Aggregate node would filter out the non-matching values. Here we fix this by inspecting ORDER BY / DISTINCT aggregate functions which have a FILTER clause to see if the aggregate's arguments are anything more complex than a Var or a Const. Evaluating these isn't going to cause an error. If we find any non-Var, non-Const parameters then the planner will now opt to perform the sort in the Aggregate node for these aggregates, i.e. disable the presorted aggregate optimization. An alternative fix would have been to completely disallow the presorted optimization for Aggrefs with any FILTER clause, but that wasn't done as that could cause large performance regressions for queries that see significant gains from 1349d27 due to presorted results coming in from an Index Scan. Backpatch to 16, where 1349d27 was introduced Author: David Rowley <dgrowleyml@gmail.com> Reported-by: Kaimeh <kkaimeh@gmail.com> Diagnosed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CAK-%2BJz9J%3DQ06-M7cDJoPNeYbz5EZDqkjQbJnmRyQyzkbRGsYkA%40mail.gmail.com Backpatch-through: 16
1 parent 2d33cf7 commit 887a232

File tree

4 files changed

+115
-16
lines changed

4 files changed

+115
-16
lines changed

‎src/backend/optimizer/plan/planner.c

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3240,10 +3240,53 @@ adjust_group_pathkeys_for_groupagg(PlannerInfo *root)
32403240
if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
32413241
continue;
32423242

3243-
/* only add aggregates with a DISTINCT or ORDER BY */
3244-
if (aggref->aggdistinct != NIL || aggref->aggorder != NIL)
3245-
unprocessed_aggs = bms_add_member(unprocessed_aggs,
3246-
foreach_current_index(lc));
3243+
/* Skip unless there's a DISTINCT or ORDER BY clause */
3244+
if (aggref->aggdistinct == NIL && aggref->aggorder == NIL)
3245+
continue;
3246+
3247+
/* Additional safety checks are needed if there's a FILTER clause */
3248+
if (aggref->aggfilter != NULL)
3249+
{
3250+
ListCell *lc2;
3251+
bool allow_presort = true;
3252+
3253+
/*
3254+
* When the Aggref has a FILTER clause, it's possible that the
3255+
* filter removes rows that cannot be sorted because the
3256+
* expression to sort by results in an error during its
3257+
* evaluation. This is a problem for presorting as that happens
3258+
* before the FILTER, whereas without presorting, the Aggregate
3259+
* node will apply the FILTER *before* sorting. So that we never
3260+
* try to sort anything that might error, here we aim to skip over
3261+
* any Aggrefs with arguments with expressions which, when
3262+
* evaluated, could cause an ERROR. Vars and Consts are ok. There
3263+
* may be more cases that should be allowed, but more thought
3264+
* needs to be given. Err on the side of caution.
3265+
*/
3266+
foreach(lc2, aggref->args)
3267+
{
3268+
TargetEntry *tle = (TargetEntry *) lfirst(lc2);
3269+
Expr *expr = tle->expr;
3270+
3271+
while (IsA(expr, RelabelType))
3272+
expr = (Expr *) (castNode(RelabelType, expr))->arg;
3273+
3274+
/* Common case, Vars and Consts are ok */
3275+
if (IsA(expr, Var) || IsA(expr, Const))
3276+
continue;
3277+
3278+
/* Unsupported. Don't try to presort for this Aggref */
3279+
allow_presort = false;
3280+
break;
3281+
}
3282+
3283+
/* Skip unsupported Aggrefs */
3284+
if (!allow_presort)
3285+
continue;
3286+
}
3287+
3288+
unprocessed_aggs = bms_add_member(unprocessed_aggs,
3289+
foreach_current_index(lc));
32473290
}
32483291

32493292
/*

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1586,6 +1586,43 @@ select sum(two order by two) from tenk1;
15861586
(2 rows)
15871587

15881588
reset enable_presorted_aggregate;
1589+
--
1590+
-- Test cases with FILTER clause
1591+
--
1592+
-- Ensure we presort when the aggregate contains plain Vars
1593+
explain (costs off)
1594+
select sum(two order by two) filter (where two > 1) from tenk1;
1595+
QUERY PLAN
1596+
-------------------------------
1597+
Aggregate
1598+
-> Sort
1599+
Sort Key: two
1600+
-> Seq Scan on tenk1
1601+
(4 rows)
1602+
1603+
-- Ensure we presort for RelabelType'd Vars
1604+
explain (costs off)
1605+
select string_agg(distinct f1, ',') filter (where length(f1) > 1)
1606+
from varchar_tbl;
1607+
QUERY PLAN
1608+
-------------------------------------
1609+
Aggregate
1610+
-> Sort
1611+
Sort Key: f1
1612+
-> Seq Scan on varchar_tbl
1613+
(4 rows)
1614+
1615+
-- Ensure we don't presort when the aggregate's argument contains an
1616+
-- explicit cast.
1617+
explain (costs off)
1618+
select string_agg(distinct f1::varchar(2), ',') filter (where length(f1) > 1)
1619+
from varchar_tbl;
1620+
QUERY PLAN
1621+
-------------------------------
1622+
Aggregate
1623+
-> Seq Scan on varchar_tbl
1624+
(2 rows)
1625+
15891626
--
15901627
-- Test combinations of DISTINCT and/or ORDER BY
15911628
--

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -533,22 +533,22 @@ SELECT
533533
FROM
534534
(VALUES (NULL), (3), (1), (NULL), (NULL), (5), (2), (4), (NULL)) foo(bar);
535535
-[ RECORD 1 ]--------------------+-------------------------------------------------------------------------------------------------------------------------
536-
no_options | [1, 2, 3, 4, 5]
537-
returning_jsonb | [1, 2, 3, 4, 5]
538-
absent_on_null | [1, 2, 3, 4, 5]
539-
absentonnull_returning_jsonb | [1, 2, 3, 4, 5]
540-
null_on_null | [1, 2, 3, 4, 5, null, null, null, null]
541-
nullonnull_returning_jsonb | [1, 2, 3, 4, 5, null, null, null, null]
542-
row_no_options | [{"bar":1}, +
543-
| {"bar":2}, +
536+
no_options | [3, 1, 5, 2, 4]
537+
returning_jsonb | [3, 1, 5, 2, 4]
538+
absent_on_null | [3, 1, 5, 2, 4]
539+
absentonnull_returning_jsonb | [3, 1, 5, 2, 4]
540+
null_on_null | [null, 3, 1, null, null, 5, 2, 4, null]
541+
nullonnull_returning_jsonb | [null, 3, 1, null, null, 5, 2, 4, null]
542+
row_no_options | [{"bar":null}, +
544543
| {"bar":3}, +
545-
| {"bar":4}, +
546-
| {"bar":5}, +
547-
| {"bar":null}, +
544+
| {"bar":1}, +
548545
| {"bar":null}, +
549546
| {"bar":null}, +
547+
| {"bar":5}, +
548+
| {"bar":2}, +
549+
| {"bar":4}, +
550550
| {"bar":null}]
551-
row_returning_jsonb | [{"bar": 1}, {"bar": 2}, {"bar": 3}, {"bar": 4}, {"bar": 5}, {"bar": null}, {"bar": null}, {"bar": null}, {"bar": null}]
551+
row_returning_jsonb | [{"bar": null}, {"bar": 3}, {"bar": 1}, {"bar": null}, {"bar": null}, {"bar": 5}, {"bar": 2}, {"bar": 4}, {"bar": null}]
552552
row_filtered_agg | [{"bar":3}, +
553553
| {"bar":4}, +
554554
| {"bar":5}]

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,25 @@ explain (costs off)
595595
select sum(two order by two) from tenk1;
596596
reset enable_presorted_aggregate;
597597

598+
--
599+
-- Test cases with FILTER clause
600+
--
601+
602+
-- Ensure we presort when the aggregate contains plain Vars
603+
explain (costs off)
604+
select sum(two order by two) filter (where two > 1) from tenk1;
605+
606+
-- Ensure we presort for RelabelType'd Vars
607+
explain (costs off)
608+
select string_agg(distinct f1, ',') filter (where length(f1) > 1)
609+
from varchar_tbl;
610+
611+
-- Ensure we don't presort when the aggregate's argument contains an
612+
-- explicit cast.
613+
explain (costs off)
614+
select string_agg(distinct f1::varchar(2), ',') filter (where length(f1) > 1)
615+
from varchar_tbl;
616+
598617
--
599618
-- Test combinations of DISTINCT and/or ORDER BY
600619
--

0 commit comments

Comments
 (0)