From 187d15185200aabc22c5219bbe636bc950670a78 Mon Sep 17 00:00:00 2001 From: Alexander Pyhalov Date: Fri, 14 Jul 2023 16:34:02 +0300 Subject: [PATCH] For partial aggregation we can't rely on the fact that every var is a part of some GROUP BY expression --- .../postgres_fdw/expected/postgres_fdw.out | 120 ++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.c | 31 +++-- contrib/postgres_fdw/sql/postgres_fdw.sql | 9 ++ 3 files changed, 152 insertions(+), 8 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 8d80ba0a6be..31ee461045c 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9955,6 +9955,126 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1; 49 | 19.0000000000000000 | 29 | 60 (50 rows) +EXPLAIN (VERBOSE, COSTS OFF) +SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + QUERY PLAN +-------------------------------------------------------------------------------------------------------------------------------- + Sort + Output: ((pagg_tab.b / 2)), (avg(pagg_tab.a)), (max(pagg_tab.a)), (count(*)) + Sort Key: ((pagg_tab.b / 2)) + -> Finalize HashAggregate + Output: ((pagg_tab.b / 2)), avg(pagg_tab.a), max(pagg_tab.a), count(*) + Group Key: ((pagg_tab.b / 2)) + -> Append + -> Foreign Scan + Output: ((pagg_tab.b / 2)), (PARTIAL avg(pagg_tab.a)), (PARTIAL max(pagg_tab.a)), (PARTIAL count(*)) + Relations: Aggregate on (public.fpagg_tab_p1 pagg_tab) + Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p1 GROUP BY 1 + -> Foreign Scan + Output: ((pagg_tab_1.b / 2)), (PARTIAL avg(pagg_tab_1.a)), (PARTIAL max(pagg_tab_1.a)), (PARTIAL count(*)) + Relations: Aggregate on (public.fpagg_tab_p2 pagg_tab_1) + Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p2 GROUP BY 1 + -> Foreign Scan + Output: ((pagg_tab_2.b / 2)), (PARTIAL avg(pagg_tab_2.a)), (PARTIAL max(pagg_tab_2.a)), (PARTIAL count(*)) + Relations: Aggregate on (public.fpagg_tab_p3 pagg_tab_2) + Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p3 GROUP BY 1 +(19 rows) + +SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + ?column? | avg | max | count +----------+---------------------+-----+------- + 0 | 10.5000000000000000 | 21 | 120 + 1 | 12.5000000000000000 | 23 | 120 + 2 | 14.5000000000000000 | 25 | 120 + 3 | 16.5000000000000000 | 27 | 120 + 4 | 18.5000000000000000 | 29 | 120 + 5 | 10.5000000000000000 | 21 | 120 + 6 | 12.5000000000000000 | 23 | 120 + 7 | 14.5000000000000000 | 25 | 120 + 8 | 16.5000000000000000 | 27 | 120 + 9 | 18.5000000000000000 | 29 | 120 + 10 | 10.5000000000000000 | 21 | 120 + 11 | 12.5000000000000000 | 23 | 120 + 12 | 14.5000000000000000 | 25 | 120 + 13 | 16.5000000000000000 | 27 | 120 + 14 | 18.5000000000000000 | 29 | 120 + 15 | 10.5000000000000000 | 21 | 120 + 16 | 12.5000000000000000 | 23 | 120 + 17 | 14.5000000000000000 | 25 | 120 + 18 | 16.5000000000000000 | 27 | 120 + 19 | 18.5000000000000000 | 29 | 120 + 20 | 10.5000000000000000 | 21 | 120 + 21 | 12.5000000000000000 | 23 | 120 + 22 | 14.5000000000000000 | 25 | 120 + 23 | 16.5000000000000000 | 27 | 120 + 24 | 18.5000000000000000 | 29 | 120 +(25 rows) + +-- It's unsafe to pushdown variables if we need both variable and variable-based expression +EXPLAIN (VERBOSE, COSTS OFF) +SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + QUERY PLAN +---------------------------------------------------------------------------------------------------------------------------------------------- + Sort + Output: ((((pagg_tab.b / 2)))::numeric), (avg(pagg_tab.a)), (max(pagg_tab.a)), (count(*)), ((pagg_tab.b / 2)) + Sort Key: ((((pagg_tab.b / 2)))::numeric) + -> Finalize GroupAggregate + Output: (((pagg_tab.b / 2)))::numeric, avg(pagg_tab.a), max(pagg_tab.a), count(*), ((pagg_tab.b / 2)) + Group Key: ((pagg_tab.b / 2)) + -> Sort + Output: ((pagg_tab.b / 2)), pagg_tab.b, (PARTIAL avg(pagg_tab.a)), (PARTIAL max(pagg_tab.a)), (PARTIAL count(*)) + Sort Key: ((pagg_tab.b / 2)) + -> Append + -> Partial HashAggregate + Output: ((pagg_tab.b / 2)), pagg_tab.b, PARTIAL avg(pagg_tab.a), PARTIAL max(pagg_tab.a), PARTIAL count(*) + Group Key: (pagg_tab.b / 2) + -> Foreign Scan on public.fpagg_tab_p1 pagg_tab + Output: (pagg_tab.b / 2), pagg_tab.b, pagg_tab.a + Remote SQL: SELECT a, b FROM public.pagg_tab_p1 + -> Partial HashAggregate + Output: ((pagg_tab_1.b / 2)), pagg_tab_1.b, PARTIAL avg(pagg_tab_1.a), PARTIAL max(pagg_tab_1.a), PARTIAL count(*) + Group Key: (pagg_tab_1.b / 2) + -> Foreign Scan on public.fpagg_tab_p2 pagg_tab_1 + Output: (pagg_tab_1.b / 2), pagg_tab_1.b, pagg_tab_1.a + Remote SQL: SELECT a, b FROM public.pagg_tab_p2 + -> Partial HashAggregate + Output: ((pagg_tab_2.b / 2)), pagg_tab_2.b, PARTIAL avg(pagg_tab_2.a), PARTIAL max(pagg_tab_2.a), PARTIAL count(*) + Group Key: (pagg_tab_2.b / 2) + -> Foreign Scan on public.fpagg_tab_p3 pagg_tab_2 + Output: (pagg_tab_2.b / 2), pagg_tab_2.b, pagg_tab_2.a + Remote SQL: SELECT a, b FROM public.pagg_tab_p3 +(28 rows) + +SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + numeric | avg | max | count +---------+---------------------+-----+------- + 0 | 10.5000000000000000 | 21 | 120 + 1 | 12.5000000000000000 | 23 | 120 + 2 | 14.5000000000000000 | 25 | 120 + 3 | 16.5000000000000000 | 27 | 120 + 4 | 18.5000000000000000 | 29 | 120 + 5 | 10.5000000000000000 | 21 | 120 + 6 | 12.5000000000000000 | 23 | 120 + 7 | 14.5000000000000000 | 25 | 120 + 8 | 16.5000000000000000 | 27 | 120 + 9 | 18.5000000000000000 | 29 | 120 + 10 | 10.5000000000000000 | 21 | 120 + 11 | 12.5000000000000000 | 23 | 120 + 12 | 14.5000000000000000 | 25 | 120 + 13 | 16.5000000000000000 | 27 | 120 + 14 | 18.5000000000000000 | 29 | 120 + 15 | 10.5000000000000000 | 21 | 120 + 16 | 12.5000000000000000 | 23 | 120 + 17 | 14.5000000000000000 | 25 | 120 + 18 | 16.5000000000000000 | 27 | 120 + 19 | 18.5000000000000000 | 29 | 120 + 20 | 10.5000000000000000 | 21 | 120 + 21 | 12.5000000000000000 | 23 | 120 + 22 | 14.5000000000000000 | 25 | 120 + 23 | 16.5000000000000000 | 27 | 120 + 24 | 18.5000000000000000 | 29 | 120 +(25 rows) + -- Partial aggregates are safe to push down for all built-in aggregates EXPLAIN (VERBOSE, COSTS OFF) SELECT /* aggregate function <> aggpartialfunc */ diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 5c48c5d50dc..eec76a00488 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -6381,13 +6381,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, ListCell *lc; int i; List *tlist = NIL; + bool partial = false; /* We currently don't support pushing Grouping Sets. */ if (query->groupingSets) return false; + if (patype == PARTITIONWISE_AGGREGATE_PARTIAL) + partial = true; + /* It's unsafe to push having statements with partial aggregates */ - if ((patype == PARTITIONWISE_AGGREGATE_PARTIAL) && havingQual) + if (partial && havingQual) return false; /* Get the fpinfo of the underlying scan relation. */ @@ -6461,11 +6465,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, } else { + /* + * For partial aggregation we can't rely on the fact that every + * var is a part of some GROUP BY expression, so extract variables + * and if we found some, avoid to push it down. + */ + /* * Non-grouping expression we need to compute. Can we ship it * as-is to the foreign server? */ - if (is_foreign_expr(root, grouped_rel, expr) && + if (!partial && is_foreign_expr(root, grouped_rel, expr) && !is_foreign_param(root, grouped_rel, expr)) { /* Yes, so add to tlist as-is; OK to suppress duplicates */ @@ -6489,14 +6499,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, return false; /* - * Add aggregates, if any, into the targetlist. Plain Vars - * outside an aggregate can be ignored, because they should be + * Add aggregates, if any, into the targetlist. When dealing + * with usual aggregations, plain Vars outside an aggregate + * can be ignored, because they should be * either same as some GROUP BY column or part of some GROUP * BY expression. In either case, they are already part of - * the targetlist and thus no need to add them again. In fact - * including plain Vars in the tlist when they do not match a - * GROUP BY column would cause the foreign server to complain - * that the shipped query is invalid. + * the targetlist and thus no need to add them again. When + * looking at partial aggregation, we can get additional vars + * needed for partial aggregation itself, which do not match + * group by column. In fact including plain Vars in the tlist + * when they do not match a GROUP BY column would cause the + * foreign server to complain that the shipped query is invalid. */ foreach(l, aggvars) { @@ -6504,6 +6517,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, if (IsA(aggref, Aggref)) tlist = add_to_flat_tlist(tlist, list_make1(aggref)); + else if (partial) + return false; } } } diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 827637b001a..4f93e1931de 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3030,6 +3030,15 @@ EXPLAIN (VERBOSE, COSTS OFF) SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1; SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1; +EXPLAIN (VERBOSE, COSTS OFF) +SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; +SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + +-- It's unsafe to pushdown variables if we need both variable and variable-based expression +EXPLAIN (VERBOSE, COSTS OFF) +SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; +SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1; + -- Partial aggregates are safe to push down for all built-in aggregates EXPLAIN (VERBOSE, COSTS OFF) SELECT /* aggregate function <> aggpartialfunc */ -- 2.34.1