Re: BUG #15781: subselect on foreign table (postgres_fdw) can crash (segfault) - Mailing list pgsql-bugs

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> (2019/04/26 3:24), Tom Lane wrote:
>> If we do leave it like this, then the only way for postgres_fdw to
>> avoid trouble is to not have any entries in fdw_exprs that exactly
>> match entries in fdw_scan_tlist.  So that pretty much devolves back
>> to what I said before: don't ship values to the far end that are
>> just going to be fed back as-is.  But now it's a correctness
>> requirement not just an optimization.

> I worked on the ORDERED/FINAL-upperrel pushdown for PG12, but I don't 
> think that that's directly related to this issue, because this arises in 
> PG11 already.  Maybe I'm missing something, but the 
> UPPERREL_GROUP_AGG-upperrel pushdown added in PG10 is likely to be 
> related to this.  I'll work on this issue unless somebody wants to.  But 
> I'll take a 10-day vocation from tomorrow, so I don't think I'll be able 
> to fix this in the next minor release...

Well, the releases are coming up fast, so I spent some time on this.
If we don't want to change what the core code does with fdw_exprs,
I think the only way to fix it is to hack postgres_fdw so that it
won't generate plans involving the problematic case.  See attached.

We end up with slightly weird-looking plans if the troublesome Param
is actually a GROUP BY expression, but if it's not, I think things
are fine.  Maybe we could do something smarter about the GROUP BY case,
but it seems weird enough to maybe not be worth additional trouble.

            regards, tom lane

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 079406f..c4e3311 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -842,6 +842,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 48ea67a..e034b03 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2707,6 +2707,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 794363c..6c9e320 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5486,7 +5486,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;
@@ -5512,6 +5511,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)
@@ -5533,6 +5541,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
@@ -5547,9 +5562,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));
@@ -5557,12 +5574,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;
@@ -5649,7 +5670,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 b382945..3e4603d 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -146,6 +146,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,
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 127cd30..73852f1 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -685,6 +685,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