Re: [HACKERS] [sqlsmith] ERROR: badly formatted node string "RESTRICTINFO... - Mailing list pgsql-hackers

I wrote:
> Apparently, postgres_fdw is trying to store RestrictInfos in the
> fdw_private field of a ForeignScan node.  That won't do; those aren't
> supposed to be present in a finished plan tree, so there's no readfuncs.c
> support for them (and we're not adding it).

> Don't know if this is a new bug, or ancient but not previously reachable.
> It seems to be nearly the inverse of the problem you found yesterday,
> in which postgres_fdw was stripping RestrictInfos sooner than it really
> ought to.  Apparently that whole business needs a fresh look.

Attached is a proposed patch that cleans up the mess here --- and it was
a mess.  The comments for PgFdwRelationInfo claimed that the remote_conds
and local_conds fields contained bare clauses, but actually they could
contain either bare clauses or RestrictInfos or both, depending on where
the clauses had come from.  And there was some seriously obscure and
undocumented logic around how the fdw_recheck_quals got set up for a
foreign join node.  (BTW, said logic is assuming that no EPQ recheck is
needed for a foreign join.  I commented it to that effect, but I'm not
very sure that I believe it.  If there's a bug there, though, it's
independent of the immediate problem.)

Anybody want to review this, or shall I just push it?

(BTW, I've not yet looked to see if this needs to be back-ported.)

            regards, tom lane

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index c5149a0..1d5aa83 100644
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
*************** classifyConditions(PlannerInfo *root,
*** 210,216 ****

      foreach(lc, input_conds)
      {
!         RestrictInfo *ri = (RestrictInfo *) lfirst(lc);

          if (is_foreign_expr(root, baserel, ri->clause))
              *remote_conds = lappend(*remote_conds, ri);
--- 210,216 ----

      foreach(lc, input_conds)
      {
!         RestrictInfo *ri = lfirst_node(RestrictInfo, lc);

          if (is_foreign_expr(root, baserel, ri->clause))
              *remote_conds = lappend(*remote_conds, ri);
*************** build_tlist_to_deparse(RelOptInfo *forei
*** 869,874 ****
--- 869,875 ----
  {
      List       *tlist = NIL;
      PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+     ListCell   *lc;

      /*
       * For an upper relation, we have already built the target list while
*************** build_tlist_to_deparse(RelOptInfo *forei
*** 884,892 ****
      tlist = add_to_flat_tlist(tlist,
                         pull_var_clause((Node *) foreignrel->reltarget->exprs,
                                         PVC_RECURSE_PLACEHOLDERS));
!     tlist = add_to_flat_tlist(tlist,
!                               pull_var_clause((Node *) fpinfo->local_conds,
!                                               PVC_RECURSE_PLACEHOLDERS));

      return tlist;
  }
--- 885,898 ----
      tlist = add_to_flat_tlist(tlist,
                         pull_var_clause((Node *) foreignrel->reltarget->exprs,
                                         PVC_RECURSE_PLACEHOLDERS));
!     foreach(lc, fpinfo->local_conds)
!     {
!         RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
!
!         tlist = add_to_flat_tlist(tlist,
!                                   pull_var_clause((Node *) rinfo->clause,
!                                                   PVC_RECURSE_PLACEHOLDERS));
!     }

      return tlist;
  }
*************** deparseSelectSql(List *tlist, bool is_su
*** 1049,1054 ****
--- 1055,1061 ----
   * "buf".
   *
   * quals is the list of clauses to be included in the WHERE clause.
+  * (These may or may not include RestrictInfo decoration.)
   */
  static void
  deparseFromExpr(List *quals, deparse_expr_cxt *context)
*************** deparseLockingClause(deparse_expr_cxt *c
*** 1262,1267 ****
--- 1269,1277 ----
   *
   * The conditions in the list are assumed to be ANDed. This function is used to
   * deparse WHERE clauses, JOIN .. ON clauses and HAVING clauses.
+  *
+  * Depending on the caller, the list elements might be either RestrictInfos
+  * or bare clauses.
   */
  static void
  appendConditions(List *exprs, deparse_expr_cxt *context)
*************** appendConditions(List *exprs, deparse_ex
*** 1278,1293 ****
      {
          Expr       *expr = (Expr *) lfirst(lc);

!         /*
!          * Extract clause from RestrictInfo, if required. See comments in
!          * declaration of PgFdwRelationInfo for details.
!          */
          if (IsA(expr, RestrictInfo))
!         {
!             RestrictInfo *ri = (RestrictInfo *) expr;
!
!             expr = ri->clause;
!         }

          /* Connect expressions with "AND" and parenthesize each condition. */
          if (!is_first)
--- 1288,1296 ----
      {
          Expr       *expr = (Expr *) lfirst(lc);

!         /* Extract clause from RestrictInfo, if required */
          if (IsA(expr, RestrictInfo))
!             expr = ((RestrictInfo *) expr)->clause;

          /* Connect expressions with "AND" and parenthesize each condition. */
          if (!is_first)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 6670df5..28a945f 100644
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
*************** enum FdwScanPrivateIndex
*** 64,70 ****
  {
      /* SQL statement to execute remotely (as a String node) */
      FdwScanPrivateSelectSql,
!     /* List of restriction clauses that can be executed remotely */
      FdwScanPrivateRemoteConds,
      /* Integer list of attribute numbers retrieved by the SELECT */
      FdwScanPrivateRetrievedAttrs,
--- 64,71 ----
  {
      /* 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,
*************** postgresGetForeignRelSize(PlannerInfo *r
*** 576,582 ****
                     &fpinfo->attrs_used);
      foreach(lc, fpinfo->local_conds)
      {
!         RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);

          pull_varattnos((Node *) rinfo->clause, baserel->relid,
                         &fpinfo->attrs_used);
--- 577,583 ----
                     &fpinfo->attrs_used);
      foreach(lc, fpinfo->local_conds)
      {
!         RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);

          pull_varattnos((Node *) rinfo->clause, baserel->relid,
                         &fpinfo->attrs_used);
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1119,1132 ****
      PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
      Index        scan_relid;
      List       *fdw_private;
-     List       *remote_conds = NIL;
      List       *remote_exprs = NIL;
      List       *local_exprs = NIL;
      List       *params_list = NIL;
      List       *retrieved_attrs;
      StringInfoData sql;
      ListCell   *lc;
-     List       *fdw_scan_tlist = NIL;

      /*
       * For base relations, set scan_relid as the relid of the relation. For
--- 1120,1133 ----
      PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
      Index        scan_relid;
      List       *fdw_private;
      List       *remote_exprs = NIL;
      List       *local_exprs = NIL;
      List       *params_list = NIL;
+     List       *fdw_scan_tlist = NIL;
+     List       *fdw_recheck_quals = NIL;
      List       *retrieved_attrs;
      StringInfoData sql;
      ListCell   *lc;

      /*
       * For base relations, set scan_relid as the relid of the relation. For
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1163,1171 ****
       *
       * This code must match "extract_actual_clauses(scan_clauses, false)"
       * except for the additional decision about remote versus local execution.
-      * Note however that we don't strip the RestrictInfo nodes from the
-      * remote_conds list, since appendWhereClause expects a list of
-      * RestrictInfos.
       */
      foreach(lc, scan_clauses)
      {
--- 1164,1169 ----
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1176,1201 ****
              continue;

          if (list_member_ptr(fpinfo->remote_conds, rinfo))
-         {
-             remote_conds = lappend(remote_conds, rinfo);
              remote_exprs = lappend(remote_exprs, rinfo->clause);
-         }
          else if (list_member_ptr(fpinfo->local_conds, rinfo))
              local_exprs = lappend(local_exprs, rinfo->clause);
          else if (is_foreign_expr(root, foreignrel, rinfo->clause))
-         {
-             remote_conds = lappend(remote_conds, rinfo);
              remote_exprs = lappend(remote_exprs, rinfo->clause);
-         }
          else
              local_exprs = lappend(local_exprs, rinfo->clause);
      }

      if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
      {
!         /* For a join relation, get the conditions from fdw_private structure */
!         remote_conds = fpinfo->remote_conds;
!         local_exprs = fpinfo->local_conds;

          /* Build the list of columns to be fetched from the foreign server. */
          fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
--- 1174,1198 ----
              continue;

          if (list_member_ptr(fpinfo->remote_conds, rinfo))
              remote_exprs = lappend(remote_exprs, rinfo->clause);
          else if (list_member_ptr(fpinfo->local_conds, rinfo))
              local_exprs = lappend(local_exprs, rinfo->clause);
          else if (is_foreign_expr(root, foreignrel, rinfo->clause))
              remote_exprs = lappend(remote_exprs, rinfo->clause);
          else
              local_exprs = lappend(local_exprs, rinfo->clause);
      }

      if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
      {
!         /*
!          * For a join relation, get the conditions from fdw_private structure.
!          * Note that we leave fdw_recheck_quals empty in this case, since
!          * there is no need for EPQ recheck at a join (and Vars or Aggrefs in
!          * the qual might not be available locally anyway).
!          */
!         remote_exprs = extract_actual_clauses(fpinfo->remote_conds, false);
!         local_exprs = extract_actual_clauses(fpinfo->local_conds, false);

          /* Build the list of columns to be fetched from the foreign server. */
          fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1238,1243 ****
--- 1235,1248 ----
              }
          }
      }
+     else
+     {
+         /*
+          * For a base-relation scan, we have to support EPQ recheck, which
+          * should recheck all remote quals
+          */
+         fdw_recheck_quals = remote_exprs;
+     }

      /*
       * Build the query string to be sent for execution, and identify
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1245,1251 ****
       */
      initStringInfo(&sql);
      deparseSelectStmtForRel(&sql, root, foreignrel, fdw_scan_tlist,
!                             remote_conds, best_path->path.pathkeys,
                              false, &retrieved_attrs, ¶ms_list);

      /*
--- 1250,1256 ----
       */
      initStringInfo(&sql);
      deparseSelectStmtForRel(&sql, root, foreignrel, fdw_scan_tlist,
!                             remote_exprs, best_path->path.pathkeys,
                              false, &retrieved_attrs, ¶ms_list);

      /*
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1253,1259 ****
       * Items in the list must match order in enum FdwScanPrivateIndex.
       */
      fdw_private = list_make4(makeString(sql.data),
!                              remote_conds,
                               retrieved_attrs,
                               makeInteger(fpinfo->fetch_size));
      if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
--- 1258,1264 ----
       * Items in the list must match order in enum FdwScanPrivateIndex.
       */
      fdw_private = list_make4(makeString(sql.data),
!                              remote_exprs,
                               retrieved_attrs,
                               makeInteger(fpinfo->fetch_size));
      if (IS_JOIN_REL(foreignrel) || IS_UPPER_REL(foreignrel))
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1266,1271 ****
--- 1271,1283 ----
       * 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,
*************** postgresGetForeignPlan(PlannerInfo *root
*** 1273,1279 ****
                              params_list,
                              fdw_private,
                              fdw_scan_tlist,
!                             remote_exprs,
                              outer_plan);
  }

--- 1285,1291 ----
                              params_list,
                              fdw_private,
                              fdw_scan_tlist,
!                             fdw_recheck_quals,
                              outer_plan);
  }

*************** postgresPlanDirectModify(PlannerInfo *ro
*** 2199,2205 ****
      rel = heap_open(rte->relid, NoLock);

      /*
!      * Extract the baserestrictinfo clauses that can be evaluated remotely.
       */
      remote_conds = (List *) list_nth(fscan->fdw_private,
                                       FdwScanPrivateRemoteConds);
--- 2211,2219 ----
      rel = heap_open(rte->relid, NoLock);

      /*
!      * Extract the qual clauses that can 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);
*************** foreign_join_ok(PlannerInfo *root, RelOp
*** 4094,4102 ****
      if (fpinfo_o->local_conds || fpinfo_i->local_conds)
          return false;

!     /* Separate restrict list into join quals and quals on join relation */
      if (IS_OUTER_JOIN(jointype))
!         extract_actual_join_clauses(extra->restrictlist, &joinclauses, &otherclauses);
      else
      {
          /*
--- 4108,4128 ----
      if (fpinfo_o->local_conds || fpinfo_i->local_conds)
          return false;

!     /* Separate restrict list into join quals and pushed-down (other) quals */
      if (IS_OUTER_JOIN(jointype))
!     {
!         /* Grovel through the clauses to separate into two lists */
!         joinclauses = otherclauses = NIL;
!         foreach(lc, extra->restrictlist)
!         {
!             RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
!
!             if (rinfo->is_pushed_down)
!                 otherclauses = lappend(otherclauses, rinfo);
!             else
!                 joinclauses = lappend(joinclauses, rinfo);
!         }
!     }
      else
      {
          /*
*************** foreign_join_ok(PlannerInfo *root, RelOp
*** 4106,4121 ****
           * a join down to the foreign server even if some of its join quals
           * are not safe to pushdown.
           */
!         otherclauses = extract_actual_clauses(extra->restrictlist, false);
          joinclauses = NIL;
      }

      /* Join quals must be safe to push down. */
      foreach(lc, joinclauses)
      {
!         Expr       *expr = (Expr *) lfirst(lc);

!         if (!is_foreign_expr(root, joinrel, expr))
              return false;
      }

--- 4132,4147 ----
           * a join down to the foreign server even if some of its join quals
           * are not safe to pushdown.
           */
!         otherclauses = extra->restrictlist;
          joinclauses = NIL;
      }

      /* Join quals must be safe to push down. */
      foreach(lc, joinclauses)
      {
!         RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);

!         if (!is_foreign_expr(root, joinrel, rinfo->clause))
              return false;
      }

*************** foreign_join_ok(PlannerInfo *root, RelOp
*** 4152,4163 ****
       */
      foreach(lc, otherclauses)
      {
!         Expr       *expr = (Expr *) lfirst(lc);

!         if (!is_foreign_expr(root, joinrel, expr))
!             fpinfo->local_conds = lappend(fpinfo->local_conds, expr);
          else
!             fpinfo->remote_conds = lappend(fpinfo->remote_conds, expr);
      }

      fpinfo->outerrel = outerrel;
--- 4178,4189 ----
       */
      foreach(lc, otherclauses)
      {
!         RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);

!         if (is_foreign_expr(root, joinrel, rinfo->clause))
!             fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo);
          else
!             fpinfo->local_conds = lappend(fpinfo->local_conds, rinfo);
      }

      fpinfo->outerrel = outerrel;
*************** foreign_grouping_ok(PlannerInfo *root, R
*** 4631,4641 ****
          foreach(lc, (List *) query->havingQual)
          {
              Expr       *expr = (Expr *) lfirst(lc);

!             if (!is_foreign_expr(root, grouped_rel, expr))
!                 fpinfo->local_conds = lappend(fpinfo->local_conds, expr);
              else
!                 fpinfo->remote_conds = lappend(fpinfo->remote_conds, expr);
          }
      }

--- 4657,4681 ----
          foreach(lc, (List *) query->havingQual)
          {
              Expr       *expr = (Expr *) lfirst(lc);
+             RestrictInfo *rinfo;

!             /*
!              * Currently, the core code doesn't wrap havingQuals in
!              * RestrictInfos, so we must make our own.
!              */
!             Assert(!IsA(expr, RestrictInfo));
!             rinfo = make_restrictinfo(expr,
!                                       true,
!                                       false,
!                                       false,
!                                       root->qual_security_level,
!                                       grouped_rel->relids,
!                                       NULL,
!                                       NULL);
!             if (is_foreign_expr(root, grouped_rel, expr))
!                 fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo);
              else
!                 fpinfo->local_conds = lappend(fpinfo->local_conds, rinfo);
          }
      }

*************** foreign_grouping_ok(PlannerInfo *root, R
*** 4645,4653 ****
       */
      if (fpinfo->local_conds)
      {
          ListCell   *lc;
!         List       *aggvars = pull_var_clause((Node *) fpinfo->local_conds,
!                                               PVC_INCLUDE_AGGREGATES);

          foreach(lc, aggvars)
          {
--- 4685,4701 ----
       */
      if (fpinfo->local_conds)
      {
+         List       *aggvars = NIL;
          ListCell   *lc;
!
!         foreach(lc, fpinfo->local_conds)
!         {
!             RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
!
!             aggvars = list_concat(aggvars,
!                                   pull_var_clause((Node *) rinfo->clause,
!                                                   PVC_INCLUDE_AGGREGATES));
!         }

          foreach(lc, aggvars)
          {
*************** foreign_grouping_ok(PlannerInfo *root, R
*** 4664,4670 ****
                  if (!is_foreign_expr(root, grouped_rel, expr))
                      return false;

!                 tlist = add_to_flat_tlist(tlist, aggvars);
              }
          }
      }
--- 4712,4718 ----
                  if (!is_foreign_expr(root, grouped_rel, expr))
                      return false;

!                 tlist = add_to_flat_tlist(tlist, list_make1(expr));
              }
          }
      }
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 57dbb79..df75518 100644
*** a/contrib/postgres_fdw/postgres_fdw.h
--- b/contrib/postgres_fdw/postgres_fdw.h
*************** typedef struct PgFdwRelationInfo
*** 34,49 ****

      /*
       * Restriction clauses, divided into safe and unsafe to pushdown subsets.
!      *
!      * For a base foreign relation this is a list of clauses along-with
!      * RestrictInfo wrapper. Keeping RestrictInfo wrapper helps while dividing
!      * scan_clauses in postgresGetForeignPlan into safe and unsafe subsets.
!      * Also it helps in estimating costs since RestrictInfo caches the
!      * selectivity and qual cost for the clause in it.
!      *
!      * For a join relation, however, they are part of otherclause list
!      * obtained from extract_actual_join_clauses, which strips RestrictInfo
!      * construct. So, for a join relation they are list of bare clauses.
       */
      List       *remote_conds;
      List       *local_conds;
--- 34,41 ----

      /*
       * Restriction clauses, divided into safe and unsafe to pushdown subsets.
!      * All entries in these lists should have RestrictInfo wrappers; that
!      * improves efficiency of selectivity and cost estimation.
       */
      List       *remote_conds;
      List       *local_conds;
*************** typedef struct PgFdwRelationInfo
*** 91,97 ****
      RelOptInfo *outerrel;
      RelOptInfo *innerrel;
      JoinType    jointype;
!     List       *joinclauses;

      /* Grouping information */
      List       *grouped_tlist;
--- 83,89 ----
      RelOptInfo *outerrel;
      RelOptInfo *innerrel;
      JoinType    jointype;
!     List       *joinclauses;    /* List of RestrictInfo */

      /* Grouping information */
      List       *grouped_tlist;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers