diff options
| -rw-r--r-- | contrib/postgres_fdw/deparse.c | 49 | ||||
| -rw-r--r-- | contrib/postgres_fdw/expected/postgres_fdw.out | 40 | ||||
| -rw-r--r-- | contrib/postgres_fdw/postgres_fdw.c | 32 | ||||
| -rw-r--r-- | contrib/postgres_fdw/postgres_fdw.h | 3 | ||||
| -rw-r--r-- | contrib/postgres_fdw/sql/postgres_fdw.sql | 10 |
5 files changed, 129 insertions, 5 deletions
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index d272719ff48..e7b3cf35eca 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -841,6 +841,55 @@ foreign_expr_walker(Node *node, } /* + * Returns true if given expr is something we'd have to send the value of + * to the foreign server. + * + * This should return true when the expression is a shippable node that + * deparseExpr would add to context->params_list. Note that we don't care + * if the expression *contains* such a node, only whether one appears at top + * level. We need this to detect cases where setrefs.c would recognize a + * false match between an fdw_exprs item (which came from the params_list) + * and an entry in fdw_scan_tlist (which we're considering putting the given + * expression into). + */ +bool +is_foreign_param(PlannerInfo *root, + RelOptInfo *baserel, + Expr *expr) +{ + if (expr == NULL) + return false; + + switch (nodeTag(expr)) + { + case T_Var: + { + /* It would have to be sent unless it's a foreign Var */ + Var *var = (Var *) expr; + PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) (baserel->fdw_private); + Relids relids; + + if (IS_UPPER_REL(baserel)) + relids = fpinfo->outerrel->relids; + else + relids = baserel->relids; + + if (bms_is_member(var->varno, relids) && var->varlevelsup == 0) + return false; /* foreign Var, so not a param */ + else + return true; /* it'd have to be a param */ + break; + } + case T_Param: + /* Params always have to be sent to the foreign server */ + return true; + default: + break; + } + return false; +} + +/* * Convert type OID + typmod info into a type name we can ship to the remote * server. Someplace else had better have verified that this type name is * expected to be known on the remote end. diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index c3e4c6849e8..f19f982e0ac 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -2827,6 +2827,46 @@ select sum(c1) from ft1 group by c2 having avg(c1 * (random() <= 1)::int) > 100 Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" (10 rows) +-- Remote aggregate in combination with a local Param (for the output +-- of an initplan) can be trouble, per bug #15781 +explain (verbose, costs off) +select exists(select 1 from pg_enum), sum(c1) from ft1; + QUERY PLAN +-------------------------------------------------- + Foreign Scan + Output: $0, (sum(ft1.c1)) + Relations: Aggregate on (public.ft1) + Remote SQL: SELECT sum("C 1") FROM "S 1"."T 1" + InitPlan 1 (returns $0) + -> Seq Scan on pg_catalog.pg_enum +(6 rows) + +select exists(select 1 from pg_enum), sum(c1) from ft1; + exists | sum +--------+-------- + t | 500500 +(1 row) + +explain (verbose, costs off) +select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1; + QUERY PLAN +--------------------------------------------------- + GroupAggregate + Output: ($0), sum(ft1.c1) + Group Key: $0 + InitPlan 1 (returns $0) + -> Seq Scan on pg_catalog.pg_enum + -> Foreign Scan on public.ft1 + Output: $0, ft1.c1 + Remote SQL: SELECT "C 1" FROM "S 1"."T 1" +(8 rows) + +select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1; + exists | sum +--------+-------- + t | 500500 +(1 row) + -- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates -- ORDER BY within aggregate, same column used to order explain (verbose, costs off) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index bfa9a1823b6..27d2b0a2ddf 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -5291,7 +5291,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) grouped_rel->fdw_private; PathTarget *grouping_target = grouped_rel->reltarget; PgFdwRelationInfo *ofpinfo; - List *aggvars; ListCell *lc; int i; List *tlist = NIL; @@ -5317,6 +5316,15 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, * server. All GROUP BY expressions will be part of the grouping target * and thus there is no need to search for them separately. Add grouping * expressions into target list which will be passed to foreign server. + * + * A tricky fine point is that we must not put any expression into the + * target list that is just a foreign param (that is, something that + * deparse.c would conclude has to be sent to the foreign server). If we + * do, the expression will also appear in the fdw_exprs list of the plan + * node, and setrefs.c will get confused and decide that the fdw_exprs + * entry is actually a reference to the fdw_scan_tlist entry, resulting in + * a broken plan. Somewhat oddly, it's OK if the expression contains such + * a node, as long as it's not at top level; then no match is possible. */ i = 0; foreach(lc, grouping_target->exprs) @@ -5338,6 +5346,13 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, return false; /* + * If it would be a foreign param, we can't put it into the tlist, + * so we have to fail. + */ + if (is_foreign_param(root, grouped_rel, expr)) + return false; + + /* * Pushable, so add to tlist. We need to create a TLE for this * expression and apply the sortgroupref to it. We cannot use * add_to_flat_tlist() here because that avoids making duplicate @@ -5352,9 +5367,11 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, else { /* - * Non-grouping expression we need to compute. Is it shippable? + * 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 (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 */ tlist = add_to_flat_tlist(tlist, list_make1(expr)); @@ -5362,12 +5379,16 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, else { /* Not pushable as a whole; extract its Vars and aggregates */ + List *aggvars; + aggvars = pull_var_clause((Node *) expr, PVC_INCLUDE_AGGREGATES); /* * If any aggregate expression is not shippable, then we - * cannot push down aggregation to the foreign server. + * cannot push down aggregation to the foreign server. (We + * don't have to check is_foreign_param, since that certainly + * won't return true for any such expression.) */ if (!is_foreign_expr(root, grouped_rel, (Expr *) aggvars)) return false; @@ -5454,7 +5475,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel, * If aggregates within local conditions are not safe to push * down, then we cannot push down the query. Vars are already * part of GROUP BY clause which are checked above, so no need to - * access them again here. + * access them again here. Again, we need not check + * is_foreign_param for a foreign aggregate. */ if (IsA(expr, Aggref)) { diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index a5d4011e8de..6d06421a162 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -140,6 +140,9 @@ extern void classifyConditions(PlannerInfo *root, extern bool is_foreign_expr(PlannerInfo *root, RelOptInfo *baserel, Expr *expr); +extern bool is_foreign_param(PlannerInfo *root, + RelOptInfo *baserel, + Expr *expr); extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte, Index rtindex, Relation rel, List *targetAttrs, bool doNothing, List *returningList, diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 613228fba8c..934b6ffaf2c 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -674,6 +674,16 @@ select count(*) from (select c5, count(c1) from ft1 group by c5, sqrt(c2) having explain (verbose, costs off) select sum(c1) from ft1 group by c2 having avg(c1 * (random() <= 1)::int) > 100 order by 1; +-- Remote aggregate in combination with a local Param (for the output +-- of an initplan) can be trouble, per bug #15781 +explain (verbose, costs off) +select exists(select 1 from pg_enum), sum(c1) from ft1; +select exists(select 1 from pg_enum), sum(c1) from ft1; + +explain (verbose, costs off) +select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1; +select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1; + -- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates |