Simplify handling of remote-qual pass-forward in postgres_fdw.

Commit 0bf3ae88a encountered a need to pass the finally chosen remote qual
conditions forward from postgresGetForeignPlan to postgresPlanDirectModify.
It solved that by sticking them into the plan node's fdw_private list,
which in hindsight was a pretty bad idea.  In the first place, there's no
use for those qual trees either in EXPLAIN or execution; indeed they could
never safely be used for any post-planning purposes, because they would not
get processed by setrefs.c.  So they're just dead weight to carry around in
the finished plan tree, plus being an attractive nuisance for somebody who
might get the idea that they could be used that way.  Secondly, because
those qual trees (sometimes) contained RestrictInfos, they created a
plan-transmission hazard for parallel query, which is how come we noticed a
problem.  We dealt with that symptom in commit 28b047875, but really a more
straightforward and more efficient fix is to pass the data through in a new
field of struct PgFdwRelationInfo.  So do it that way.  (There's no need
to revert 28b047875, as it has sufficient reason to live anyway.)

Per fuzz testing by Andreas Seltenreich.

Discussion: https://postgr.es/m/87tw5x4vcu.fsf@credativ.de

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 14189a07be199f9786ef885495f7f4895c8b359e..8d022437bebc9868445b6f6572ce0e4c8486ce9c 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -64,9 +64,6 @@ enum FdwScanPrivateIndex
 {
        /* SQL statement to execute remotely (as a String node) */
        FdwScanPrivateSelectSql,
-       /* List of qual clauses that can be executed remotely */
-       /* (DO NOT try to use these at runtime; see postgresGetForeignPlan) */
-       FdwScanPrivateRemoteConds,
        /* Integer list of attribute numbers retrieved by the SELECT */
        FdwScanPrivateRetrievedAttrs,
        /* Integer representing the desired fetch_size */
@@ -1262,12 +1259,14 @@ postgresGetForeignPlan(PlannerInfo *root,
                                                        remote_exprs, best_path->path.pathkeys,
                                                        false, &retrieved_attrs, &params_list);
 
+       /* Remember remote_exprs for possible use by postgresPlanDirectModify */
+       fpinfo->final_remote_exprs = remote_exprs;
+
        /*
         * Build the fdw_private list that will be available to the executor.
         * Items in the list must match order in enum FdwScanPrivateIndex.
         */
-       fdw_private = list_make4(makeString(sql.data),
-                                                        remote_exprs,
+       fdw_private = list_make3(makeString(sql.data),
                                                         retrieved_attrs,
                                                         makeInteger(fpinfo->fetch_size));
        if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
@@ -1280,13 +1279,6 @@ postgresGetForeignPlan(PlannerInfo *root,
         * Note that the remote parameter expressions are stored in the fdw_exprs
         * field of the finished plan node; we can't keep them in private state
         * because then they wouldn't be subject to later planner processing.
-        *
-        * We have some foreign qual conditions hidden away within fdw_private's
-        * FdwScanPrivateRemoteConds item, which would be unsafe per the above
-        * consideration. But those will only be used by postgresPlanDirectModify,
-        * which may extract them to use in a rewritten plan.  We assume that
-        * nothing will be done between here and there that would need to modify
-        * those expressions.
         */
        return make_foreignscan(tlist,
                                                        local_exprs,
@@ -2130,13 +2122,15 @@ postgresPlanDirectModify(PlannerInfo *root,
                                                 int subplan_index)
 {
        CmdType         operation = plan->operation;
-       Plan       *subplan = (Plan *) list_nth(plan->plans, subplan_index);
-       RangeTblEntry *rte = planner_rt_fetch(resultRelation, root);
+       Plan       *subplan;
+       RelOptInfo *foreignrel;
+       RangeTblEntry *rte;
+       PgFdwRelationInfo *fpinfo;
        Relation        rel;
        StringInfoData sql;
        ForeignScan *fscan;
        List       *targetAttrs = NIL;
-       List       *remote_conds;
+       List       *remote_exprs;
        List       *params_list = NIL;
        List       *returningList = NIL;
        List       *retrieved_attrs = NIL;
@@ -2155,8 +2149,10 @@ postgresPlanDirectModify(PlannerInfo *root,
         * It's unsafe to modify a foreign table directly if there are any local
         * joins needed.
         */
+       subplan = (Plan *) list_nth(plan->plans, subplan_index);
        if (!IsA(subplan, ForeignScan))
                return false;
+       fscan = (ForeignScan *) subplan;
 
        /*
         * It's unsafe to modify a foreign table directly if there are any quals
@@ -2168,17 +2164,20 @@ postgresPlanDirectModify(PlannerInfo *root,
        /*
         * We can't handle an UPDATE or DELETE on a foreign join for now.
         */
-       fscan = (ForeignScan *) subplan;
        if (fscan->scan.scanrelid == 0)
                return false;
 
+       /* Safe to fetch data about the target foreign rel */
+       foreignrel = root->simple_rel_array[resultRelation];
+       rte = root->simple_rte_array[resultRelation];
+       fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+
        /*
         * It's unsafe to update a foreign table directly, if any expressions to
         * assign to the target columns are unsafe to evaluate remotely.
         */
        if (operation == CMD_UPDATE)
        {
-               RelOptInfo *baserel = root->simple_rel_array[resultRelation];
                int                     col;
 
                /*
@@ -2201,7 +2200,7 @@ postgresPlanDirectModify(PlannerInfo *root,
                                elog(ERROR, "attribute number %d not found in subplan targetlist",
                                         attno);
 
-                       if (!is_foreign_expr(root, baserel, (Expr *) tle->expr))
+                       if (!is_foreign_expr(root, foreignrel, (Expr *) tle->expr))
                                return false;
 
                        targetAttrs = lappend_int(targetAttrs, attno);
@@ -2220,12 +2219,11 @@ postgresPlanDirectModify(PlannerInfo *root,
        rel = heap_open(rte->relid, NoLock);
 
        /*
-        * Extract the qual clauses that can be evaluated remotely.  (These are
+        * Recall the qual clauses that must be evaluated remotely.  (These are
         * bare clauses not RestrictInfos, but deparse.c's appendConditions()
         * doesn't care.)
         */
-       remote_conds = (List *) list_nth(fscan->fdw_private,
-                                                                        FdwScanPrivateRemoteConds);
+       remote_exprs = fpinfo->final_remote_exprs;
 
        /*
         * Extract the relevant RETURNING list if any.
@@ -2242,12 +2240,12 @@ postgresPlanDirectModify(PlannerInfo *root,
                        deparseDirectUpdateSql(&sql, root, resultRelation, rel,
                                                                   ((Plan *) fscan)->targetlist,
                                                                   targetAttrs,
-                                                                  remote_conds, &params_list,
+                                                                  remote_exprs, &params_list,
                                                                   returningList, &retrieved_attrs);
                        break;
                case CMD_DELETE:
                        deparseDirectDeleteSql(&sql, root, resultRelation, rel,
-                                                                  remote_conds, &params_list,
+                                                                  remote_exprs, &params_list,
                                                                   returningList, &retrieved_attrs);
                        break;
                default:

diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index df75518f465c16ab7eeb2a77a7e16c78e5c6d1a6..2bae799ccfe6c59a6c26852b8221475ddf6b20f1 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -22,7 +22,10 @@
 
 /*
  * FDW-specific planner information kept in RelOptInfo.fdw_private for a
- * foreign table.  This information is collected by postgresGetForeignRelSize.
+ * postgres_fdw foreign table.  For a baserel, this struct is created by
+ * postgresGetForeignRelSize, although some fields are not filled till later.
+ * postgresGetForeignJoinPaths creates it for a joinrel, and
+ * postgresGetForeignUpperPaths creates it for an upperrel.
  */
 typedef struct PgFdwRelationInfo
 {
@@ -40,6 +43,9 @@ typedef struct PgFdwRelationInfo
        List       *remote_conds;
        List       *local_conds;
 
+       /* Actual remote restriction clauses for scan (sans RestrictInfos) */
+       List       *final_remote_exprs;
+
        /* Bitmap of attr numbers we need to fetch from the remote server. */
        Bitmapset  *attrs_used;
 
@@ -83,6 +89,7 @@ typedef struct PgFdwRelationInfo
        RelOptInfo *outerrel;
        RelOptInfo *innerrel;
        JoinType        jointype;
+       /* joinclauses contains only JOIN/ON conditions for an outer join */
        List       *joinclauses;        /* List of RestrictInfo */
 
        /* Grouping information */