Handle restriction clause lists more uniformly in postgres_fdw.

Clauses in the lists retained by postgres_fdw during planning were
sometimes bare boolean clauses, sometimes RestrictInfos, and sometimes
a mixture of the two in the same list.  The comment about that situation
didn't come close to telling the full truth, either.  Aside from being
confusing, this had a couple of bad practical consequences:
* waste of planning cycles due to inability to cache per-clause selectivity
and cost estimates;
* sometimes, RestrictInfos would sneak into the fdw_private list of a
finished Plan node, causing failures if, for example, we tried to ship
the Plan tree to a parallel worker.
(It may well be that it's a bug in the parallel-query logic that we
would ever try to ship such a plan to a parallel worker, but in any
case this deserves to be cleaned up.)

To fix, rearrange so that clause lists in PgFdwRelationInfo are always
lists of RestrictInfos, and then strip the RestrictInfos at the last
minute when making a Plan node.  In passing do a bit of refactoring and
comment cleanup in postgresGetForeignPlan and foreign_join_ok.

Although the messiness here dates back at least to 9.6, there's no evidence
that it causes anything worse than wasted planning cycles in 9.6, so no
back-patch for now.

Per fuzz testing by Andreas Seltenreich.

Tom Lane and Ashutosh Bapat

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

contrib/postgres_fdw/deparse.c

@@ -210,7 +210,7 @@ classifyConditions(PlannerInfo *root,
 
 	foreach(lc, input_conds)
 	{
-		RestrictInfo *ri = (RestrictInfo *) lfirst(lc);
+		RestrictInfo *ri = lfirst_node(RestrictInfo, lc);
 
 		if (is_foreign_expr(root, baserel, ri->clause))
 			*remote_conds = lappend(*remote_conds, ri);
@@ -869,6 +869,7 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
 {
 	List	   *tlist = NIL;
 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+	ListCell   *lc;
 
 	/*
 	 * For an upper relation, we have already built the target list while
@@ -884,9 +885,14 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
 	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 *) fpinfo->local_conds,
+								  pull_var_clause((Node *) rinfo->clause,
 												  PVC_RECURSE_PLACEHOLDERS));
+	}
 
 	return tlist;
 }
@@ -1049,6 +1055,7 @@ deparseSelectSql(List *tlist, bool is_subquery, List **retrieved_attrs,
  * "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)
@@ -1262,6 +1269,9 @@ deparseLockingClause(deparse_expr_cxt *context)
  *
  * 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)
@@ -1278,16 +1288,9 @@ appendConditions(List *exprs, deparse_expr_cxt *context)
 	{
 		Expr	   *expr = (Expr *) lfirst(lc);
 
-		/*
-		 * Extract clause from RestrictInfo, if required. See comments in
-		 * declaration of PgFdwRelationInfo for details.
-		 */
+		/* Extract clause from RestrictInfo, if required */
 		if (IsA(expr, RestrictInfo))
-		{
-			RestrictInfo *ri = (RestrictInfo *) expr;
-
-			expr = ri->clause;
-		}
+			expr = ((RestrictInfo *) expr)->clause;
 
 		/* Connect expressions with "AND" and parenthesize each condition. */
 		if (!is_first)

contrib/postgres_fdw/postgres_fdw.h

@@ -34,16 +34,8 @@ typedef struct PgFdwRelationInfo
 
 	/*
 	 * 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.
+	 * All entries in these lists should have RestrictInfo wrappers; that
+	 * improves efficiency of selectivity and cost estimation.
 	 */
 	List	   *remote_conds;
 	List	   *local_conds;
@@ -91,7 +83,7 @@ typedef struct PgFdwRelationInfo
 	RelOptInfo *outerrel;
 	RelOptInfo *innerrel;
 	JoinType	jointype;
-	List	   *joinclauses;
+	List	   *joinclauses;	/* List of RestrictInfo */
 
 	/* Grouping information */
 	List	   *grouped_tlist;