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

tglsfdc · tglsfdc · commit 88e902b769e1 · 2017-04-11T13:53:21.000-04:00
Commit 0bf3ae8 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 28b0478, 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 28b0478, 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
@@ -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
@@ -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 */
