From cc592c48c58d9c1920f8e2063756dcbcce79e4dd Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Sat, 30 Jan 2016 10:32:38 -0500 Subject: [PATCH] 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 | 127 +++++++++++++++++----------- contrib/postgres_fdw/postgres_fdw.c | 38 +++------ contrib/postgres_fdw/postgres_fdw.h | 18 +--- 3 files changed, 93 insertions(+), 90 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index e577a03956..df3d1ee184 100644 --- 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 0aa7fbeac0..2ab85f68a7 100644 --- 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, ¶ms_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, + ¶ms_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 0d8c271505..bf83c91481 100644 --- 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); -- 2.40.0