postgres_fdw: More preliminary refactoring for upcoming join pushdown.

The code that generates a complete SQL query for a given foreign relation
was repeated in two places, and they didn't quite agree: the EXPLAIN case
left out the locking clause.  Centralize the code so we get the same
behavior everywhere, and adjust calling conventions and which functions
are static vs. extern accordingly .  Centralize the code so we get the same
behavior everywhere, and adjust calling conventions and which functions
are static vs. extern accordingly.

Ashutosh Bapat, reviewed and slightly adjusted by me.

contrib/postgres_fdw/deparse.c

@@ -141,6 +141,11 @@ static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod,
 				 deparse_expr_cxt *context);
 static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
 					   deparse_expr_cxt *context);
+static void deparseSelectSql(Bitmapset *attrs_used, List **retrieved_attrs,
+				 deparse_expr_cxt *context);
+static void deparseLockingClause(deparse_expr_cxt *context);
+static void appendWhereClause(List *exprs, deparse_expr_cxt *context);
+static void appendOrderByClause(List *pathkeys, deparse_expr_cxt *context);
 
 
 /*
@@ -697,6 +702,51 @@ deparse_type_name(Oid type_oid, int32 typemod)
 		return format_type_with_typemod_qualified(type_oid, typemod);
 }
 
+/*
+ * Deparse SELECT statement for given relation into buf.
+ *
+ * remote_conds is the list of conditions to be deparsed as WHERE clause.
+ *
+ * pathkeys is the list of pathkeys to order the result by.
+ *
+ * List of columns selected is returned in retrieved_attrs.
+ *
+ * If params_list is not NULL, it receives a list of Params and other-relation
+ * Vars used in the clauses; these values must be transmitted to the remote
+ * server as parameter values.
+ *
+ * If params_list is NULL, we're generating the query for EXPLAIN purposes,
+ * so Params and other-relation Vars should be replaced by dummy values.
+ */
+extern void
+deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel,
+						List *remote_conds, List *pathkeys,
+						List **retrieved_attrs, List **params_list)
+{
+	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) rel->fdw_private;
+	deparse_expr_cxt context;
+
+	/* Initialize params_list if caller needs one */
+	if (params_list)
+		*params_list = NIL;
+
+	context.buf = buf;
+	context.root = root;
+	context.foreignrel = rel;
+	context.params_list = params_list;
+
+	deparseSelectSql(fpinfo->attrs_used, retrieved_attrs, &context);
+
+	if (remote_conds)
+		appendWhereClause(remote_conds, &context);
+
+	/* Add ORDER BY clause if we found any useful pathkeys */
+	if (pathkeys)
+		appendOrderByClause(pathkeys, &context);
+
+	/* Add any necessary FOR UPDATE/SHARE. */
+	deparseLockingClause(&context);
+}
 
 /*
  * Construct a simple SELECT statement that retrieves desired columns
@@ -707,13 +757,13 @@ deparse_type_name(Oid type_oid, int32 typemod)
  * returned to *retrieved_attrs.
  */
 void
-deparseSelectSql(StringInfo buf,
-				 PlannerInfo *root,
-				 RelOptInfo *baserel,
-				 Bitmapset *attrs_used,
-				 List **retrieved_attrs)
+deparseSelectSql(Bitmapset *attrs_used, List **retrieved_attrs,
+				 deparse_expr_cxt *context)
 {
-	RangeTblEntry *rte = planner_rt_fetch(baserel->relid, root);
+	StringInfo	buf = context->buf;
+	RelOptInfo *foreignrel = context->foreignrel;
+	PlannerInfo *root = context->root;
+	RangeTblEntry *rte = planner_rt_fetch(foreignrel->relid, root);
 	Relation	rel;
 
 	/*
@@ -726,7 +776,7 @@ deparseSelectSql(StringInfo buf,
 	 * Construct SELECT list
 	 */
 	appendStringInfoString(buf, "SELECT ");
-	deparseTargetList(buf, root, baserel->relid, rel, attrs_used,
+	deparseTargetList(buf, root, foreignrel->relid, rel, attrs_used,
 					  retrieved_attrs);
 
 	/*
@@ -811,11 +861,15 @@ deparseTargetList(StringInfo buf,
 
 /*
  * Deparse the appropriate locking clause (FOR SELECT or FOR SHARE) for a
- * given relation.
+ * given relation (context->foreignrel).
  */
-void
-deparseLockingClause(StringInfo buf, PlannerInfo *root, RelOptInfo *rel)
+static void
+deparseLockingClause(deparse_expr_cxt *context)
 {
+	StringInfo	buf = context->buf;
+	PlannerInfo *root = context->root;
+	RelOptInfo *rel = context->foreignrel;
+
 	/*
 	 * Add FOR UPDATE/SHARE if appropriate.  We apply locking during the
 	 * initial row fetch, rather than later on as is done for local tables.
@@ -868,39 +922,16 @@ deparseLockingClause(StringInfo buf, PlannerInfo *root, RelOptInfo *rel)
 }
 
 /*
- * Deparse WHERE clauses in given list of RestrictInfos and append them to buf.
- *
- * baserel is the foreign table we're planning for.
- *
- * If no WHERE clause already exists in the buffer, is_first should be true.
- *
- * If params is not NULL, it receives a list of Params and other-relation Vars
- * used in the clauses; these values must be transmitted to the remote server
- * as parameter values.
- *
- * If params is NULL, we're generating the query for EXPLAIN purposes,
- * so Params and other-relation Vars should be replaced by dummy values.
+ * Deparse WHERE clauses in given list of RestrictInfos and append them to
+ * context->buf.
  */
-void
-appendWhereClause(StringInfo buf,
-				  PlannerInfo *root,
-				  RelOptInfo *baserel,
-				  List *exprs,
-				  bool is_first,
-				  List **params)
+static void
+appendWhereClause(List *exprs, deparse_expr_cxt *context)
 {
-	deparse_expr_cxt context;
 	int			nestlevel;
 	ListCell   *lc;
-
-	if (params)
-		*params = NIL;			/* initialize result list to empty */
-
-	/* Set up context struct for recursion */
-	context.root = root;
-	context.foreignrel = baserel;
-	context.buf = buf;
-	context.params_list = params;
+	bool		is_first = true;
+	StringInfo	buf = context->buf;
 
 	/* Make sure any constants in the exprs are printed portably */
 	nestlevel = set_transmission_modes();
@@ -916,7 +947,7 @@ appendWhereClause(StringInfo buf,
 			appendStringInfoString(buf, " AND ");
 
 		appendStringInfoChar(buf, '(');
-		deparseExpr(ri->clause, &context);
+		deparseExpr(ri->clause, context);
 		appendStringInfoChar(buf, ')');
 
 		is_first = false;
@@ -1946,20 +1977,14 @@ printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
  * relation. From given pathkeys expressions belonging entirely to the given
  * base relation are obtained and deparsed.
  */
-void
-appendOrderByClause(StringInfo buf, PlannerInfo *root, RelOptInfo *baserel,
-					List *pathkeys)
+static void
+appendOrderByClause(List *pathkeys, deparse_expr_cxt *context)
 {
 	ListCell   *lcell;
-	deparse_expr_cxt context;
 	int			nestlevel;
 	char	   *delim = " ";
-
-	/* Set up context struct for recursion */
-	context.root = root;
-	context.foreignrel = baserel;
-	context.buf = buf;
-	context.params_list = NULL;
+	RelOptInfo *baserel = context->foreignrel;
+	StringInfo	buf = context->buf;
 
 	/* Make sure any constants in the exprs are printed portably */
 	nestlevel = set_transmission_modes();
@@ -1974,7 +1999,7 @@ appendOrderByClause(StringInfo buf, PlannerInfo *root, RelOptInfo *baserel,
 		Assert(em_expr != NULL);
 
 		appendStringInfoString(buf, delim);
-		deparseExpr(em_expr, &context);
+		deparseExpr(em_expr, context);
 		if (pathkey->pk_strategy == BTLessStrategyNumber)
 			appendStringInfoString(buf, " ASC");
 		else

contrib/postgres_fdw/postgres_fdw.c

@@ -1003,19 +1003,9 @@ postgresGetForeignPlan(PlannerInfo *root,
 	 * expressions to be sent as parameters.
 	 */
 	initStringInfo(&sql);
-	deparseSelectSql(&sql, root, baserel, fpinfo->attrs_used,
-					 &retrieved_attrs);
-	if (remote_conds)
-		appendWhereClause(&sql, root, baserel, remote_conds,
-						  true, &params_list);
-
-	/* Add ORDER BY clause if we found any useful pathkeys */
-	if (best_path->path.pathkeys)
-		appendOrderByClause(&sql, root, baserel, best_path->path.pathkeys);
-
-	/* Add any necessary FOR UPDATE/SHARE. */
-	deparseLockingClause(&sql, root, baserel);
-
+	deparseSelectStmtForRel(&sql, root, baserel, remote_conds,
+							best_path->path.pathkeys, &retrieved_attrs,
+							&params_list);
 	/*
 	 * Build the fdw_private list that will be available to the executor.
 	 * Items in the list must match enum FdwScanPrivateIndex, above.
@@ -1909,6 +1899,7 @@ estimate_path_cost_size(PlannerInfo *root,
 		PGconn	   *conn;
 		Selectivity local_sel;
 		QualCost	local_cost;
+		List	   *remote_conds;
 
 		/*
 		 * join_conds might contain both clauses that are safe to send across,
@@ -1917,6 +1908,14 @@ estimate_path_cost_size(PlannerInfo *root,
 		classifyConditions(root, baserel, join_conds,
 						   &remote_join_conds, &local_join_conds);
 
+		/*
+		 * The complete list of remote conditions includes everything from
+		 * baserestrictinfo plus any extra join_conds relevant to this
+		 * particular path.
+		 */
+		remote_conds = list_concat(list_copy(remote_join_conds),
+								   fpinfo->remote_conds);
+
 		/*
 		 * Construct EXPLAIN query including the desired SELECT, FROM, and
 		 * WHERE clauses.  Params and other-relation Vars are replaced by
@@ -1924,17 +1923,8 @@ estimate_path_cost_size(PlannerInfo *root,
 		 */
 		initStringInfo(&sql);
 		appendStringInfoString(&sql, "EXPLAIN ");
-		deparseSelectSql(&sql, root, baserel, fpinfo->attrs_used,
-						 &retrieved_attrs);
-		if (fpinfo->remote_conds)
-			appendWhereClause(&sql, root, baserel, fpinfo->remote_conds,
-							  true, NULL);
-		if (remote_join_conds)
-			appendWhereClause(&sql, root, baserel, remote_join_conds,
-							  (fpinfo->remote_conds == NIL), NULL);
-
-		if (pathkeys)
-			appendOrderByClause(&sql, root, baserel, pathkeys);
+		deparseSelectStmtForRel(&sql, root, baserel, remote_conds, pathkeys,
+								&retrieved_attrs, NULL);
 
 		/* Get the remote estimate */
 		conn = GetConnection(fpinfo->user, false);

contrib/postgres_fdw/postgres_fdw.h

@@ -83,19 +83,6 @@ extern void classifyConditions(PlannerInfo *root,
 extern bool is_foreign_expr(PlannerInfo *root,
 				RelOptInfo *baserel,
 				Expr *expr);
-extern void deparseSelectSql(StringInfo buf,
-				 PlannerInfo *root,
-				 RelOptInfo *baserel,
-				 Bitmapset *attrs_used,
-				 List **retrieved_attrs);
-extern void deparseLockingClause(StringInfo buf,
-					 PlannerInfo *root, RelOptInfo *rel);
-extern void appendWhereClause(StringInfo buf,
-				  PlannerInfo *root,
-				  RelOptInfo *baserel,
-				  List *exprs,
-				  bool is_first,
-				  List **params);
 extern void deparseInsertSql(StringInfo buf, PlannerInfo *root,
 				 Index rtindex, Relation rel,
 				 List *targetAttrs, bool doNothing, List *returningList,
@@ -113,8 +100,9 @@ extern void deparseAnalyzeSql(StringInfo buf, Relation rel,
 				  List **retrieved_attrs);
 extern void deparseStringLiteral(StringInfo buf, const char *val);
 extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
-extern void appendOrderByClause(StringInfo buf, PlannerInfo *root,
-					RelOptInfo *baserel, List *pathkeys);
+extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
+					 RelOptInfo *baserel, List *remote_conds, List *pathkeys,
+						List **retrieved_attrs, List **params_list);
 
 /* in shippable.c */
 extern bool is_builtin(Oid objectId);