diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 566ce5b3cb4..682c5107e4b 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -3201,9 +3201,58 @@ adjust_group_pathkeys_for_groupagg(PlannerInfo *root) if (AGGKIND_IS_ORDERED_SET(aggref->aggkind)) continue; - /* only add aggregates with a DISTINCT or ORDER BY */ - if (aggref->aggdistinct != NIL || aggref->aggorder != NIL) - unprocessed_aggs = bms_add_member(unprocessed_aggs, + /* skip unless there's a DISTINCT or ORDER BY clause */ + if (aggref->aggdistinct == NIL && aggref->aggorder == NIL) + continue; + + if (aggref->aggfilter != NULL) + { + ListCell *lc2; + bool allow_presort = true; + + /* + * When the Aggref has a FILTER clause, it's possible that the + * filter removes rows that cannot be sorted because the + * expression to sort by results in an error during its + * evaluation. This is a problem for presorting as that happens + * before the FILTER, whereas without presorting the Agg node will + * apply the FILTER *before* sorting. So that we never try to + * sort anything that might error, here we aim to skip over any + * Aggrefs with arguments with expressions which, when evaluated, + * could cause an ERROR. Vars and Consts are ok. Also we can + * ignore any implicit casts as these should also never cause an + * error. There may be more cases that should be allowed, but + * more thought needs to be given. We err on the side of caution. + */ + foreach(lc2, aggref->args) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc2); + Expr *expr = tle->expr; + + /* Common case, vars and Consts are ok */ + if (IsA(expr, Var) || IsA(expr, Const)) + continue; + + /* + * implicit casts should be safe, try removing those and + * check for Vars and Consts again. + */ + expr = (Expr *) strip_implicit_coercions((Node *) expr); + + if (IsA(expr, Var) || IsA(expr, Const)) + continue; + + /* nope. Can't do it. */ + allow_presort = false; + break; + } + + /* did we find something unsupported? */ + if (!allow_presort) + continue; + } + + unprocessed_aggs = bms_add_member(unprocessed_aggs, foreach_current_index(lc)); } diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 6b6371c3e74..3949c42ea58 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1678,6 +1678,44 @@ select sum(two order by two) from tenk1; (2 rows) reset enable_presorted_aggregate; +-- +-- Test cases with FILTER clause +-- +-- Ensure we presort when the aggregate contains plain Vars +explain (costs off) +select sum(reltuples order by reltuples) filter (where reltuples >= 0) +from pg_class; + QUERY PLAN +---------------------------------- + Aggregate + -> Sort + Sort Key: reltuples + -> Seq Scan on pg_class +(4 rows) + +-- Ensure we presort when there's an implicit cast on a Var +explain (costs off) +select string_agg(distinct relname, ',') filter (where relname like 'pg%') +from pg_class; + QUERY PLAN +------------------------------------------------- + Aggregate + -> Sort + Sort Key: ((relname)::text) COLLATE "C" + -> Seq Scan on pg_class +(4 rows) + +-- Ensure we don't presort when the aggregate's argument contains an +-- explicit cast. +explain (costs off) +select string_agg(distinct relname::varchar(10), ',') filter (where relname like 'pg%') +from pg_class; + QUERY PLAN +---------------------------- + Aggregate + -> Seq Scan on pg_class +(2 rows) + -- -- Test combinations of DISTINCT and/or ORDER BY -- diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 2c47a462b7e..af8e8f48cd6 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -633,6 +633,26 @@ explain (costs off) select sum(two order by two) from tenk1; reset enable_presorted_aggregate; +-- +-- Test cases with FILTER clause +-- + +-- Ensure we presort when the aggregate contains plain Vars +explain (costs off) +select sum(reltuples order by reltuples) filter (where reltuples >= 0) +from pg_class; + +-- Ensure we presort when there's an implicit cast on a Var +explain (costs off) +select string_agg(distinct relname, ',') filter (where relname like 'pg%') +from pg_class; + +-- Ensure we don't presort when the aggregate's argument contains an +-- explicit cast. +explain (costs off) +select string_agg(distinct relname::varchar(10), ',') filter (where relname like 'pg%') +from pg_class; + -- -- Test combinations of DISTINCT and/or ORDER BY --