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.

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index e577a039566b4b3775828938276ccf1730258d4c..df3d1ee184689b72b8392f8f8b7e5dc531b52003 100644 (file)
--- a/contrib/postgres_fdw/deparse.c
+++ b/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

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 0aa7fbeac04b326faa3844c8cf73388bd52011db..2ab85f68a7507c8ae5885e03c558fbaa659df71c 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/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);

diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 0d8c271505c1038eb63773dd36e0bc0ccadafc2b..bf83c91481cc7bb5e913d7ce554683bc7a51bae6 100644 (file)
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/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);