Would it be nuts to set fdw_scan_tlist in all cases? Would the code
> come out simpler than what we have now?
>
>
PFA the patch that can be applied on v9 patches.
If there is a whole-row reference for base relation, instead of adding that
as an additional column deparseTargetList() creates a list of all the
attributes of that foreign table . The whole-row reference gets constructed
during projection. This saves some network bandwidth while transferring the
data from the foreign server. If we build the target list for base
relation, we should include Vars for all the columns (like
deparseTargetList). Thus we borrow some code from deparseTargetList to get
all the attributes of a relation. I included this logic in function
build_tlist_from_attrs_used(), which is being called by
build_tlist_to_deparse(). So, before calling deparseSelectStmtForRel() the
callers can just call build_tlist_to_deparse() and pass the targetlist to
deparseSelectStmtForRel() and use the same to be handed over to the core
code as fdw_scan_tlist. build_tlist_to_deparse() also constructs
retrieved_attrs list, so that doesn't need to be passed around in deparse
routines.
But we now have similar code in three places deparseTargetList(),
deparseAnalyzeSql() and build_tlist_from_attrs_used(). So, I re-wrote
deparseTargetList() (which is now used to deparse returning list) to call
build_tlist_from_attrs_used() followed by deparseExplicitTargetList(). The
later and its minion deparseVar requires a deparse_expr_cxt to be passed.
deparse_expr_cxt has a member to store RelOptInfo of the relation being
deparsed. The callers of deparseReturningList() do not have it and thus
deparse_expr_cxt required changes, which in turn required changes in other
places. All in all, a larger refactoring. It touches more places than
necessary for foreign join push down. So, I think, we should try that as a
separate refactoring work. We may just do the work described in the first
paragraph for join pushdown, but we will then be left with duplicate code,
which I don't think is worth the output.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index fb72f45..7a2a67b 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -93,9 +93,10 @@ typedef struct foreign_loc_cxt
typedef struct deparse_expr_cxt
{
PlannerInfo *root; /* global planner state */
- RelOptInfo *foreignrel; /* the foreign relation we are planning
for */
+ Relids relids; /* Relids for which to deparse
*/
StringInfo buf; /* output buffer to append to */
List **params_list; /* exprs that will become remote Params
*/
+ bool is_joinrel; /* Are we deparsing for a join
relation */
} deparse_expr_cxt;
#define REL_ALIAS_PREFIX "r"
@@ -122,8 +123,7 @@ static void deparseTargetList(StringInfo buf,
Bitmapset *attrs_used,
List **retrieved_attrs,
bool qualify_col);
-static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
- deparse_expr_cxt *context);
+static void deparseExplicitTargetList(List *tlist, deparse_expr_cxt *context);
static void deparseReturningList(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
bool trig_after_row,
@@ -151,13 +151,15 @@ 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(List *tlist, List **retrieved_attrs,
- deparse_expr_cxt *context);
+static void deparseSelectSql(List *tlist, RelOptInfo *rel,
+ deparse_expr_cxt
*context);
static void deparseLockingClause(deparse_expr_cxt *context);
static void appendOrderByClause(List *pathkeys, deparse_expr_cxt *context);
static void appendConditions(List *exprs, deparse_expr_cxt *context);
static void deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
RelOptInfo *joinrel, bool use_alias,
List **params_list);
+static List *build_tlist_from_attrs_used(Index rtindex, Bitmapset *attrs_used,
+ PlannerInfo *root, List
**retrieved_attrs);
/*
@@ -715,26 +717,119 @@ deparse_type_name(Oid type_oid, int32 typemod)
}
/*
+ * Convert input bitmap representation of columns into targetlist of
+ * corresponding Var nodes.
+ *
+ * List of columns selected is returned in retrieved_attrs.
+ */
+List *
+build_tlist_from_attrs_used(Index rtindex, Bitmapset *attrs_used, PlannerInfo
*root,
+ List **retrieved_attrs)
+{
+ /*
+ * For a base relation fpinfo->attrs_used gives the list of columns
+ * required to be fetched from the foreign server.
+ */
+ RangeTblEntry *rte = planner_rt_fetch(rtindex, root);
+ /*
+ * Core code already has some lock on each rel being planned, so we
+ * can use NoLock here.
+ */
+ Relation rel = heap_open(rte->relid, NoLock);
+
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ bool have_wholerow;
+ int i;
+ List *var_list = NIL;
+
+ *retrieved_attrs = NIL;
+
+ /* If there's a whole-row reference, we'll need all the columns. */
+ have_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber,
+ attrs_used);
+
+ for (i = 1; i <= tupdesc->natts; i++)
+ {
+ Form_pg_attribute attr = tupdesc->attrs[i - 1];
+
+ /* Ignore dropped attributes. */
+ if (attr->attisdropped)
+ continue;
+
+ if (have_wholerow ||
+ bms_is_member(i - FirstLowInvalidHeapAttributeNumber,
+ attrs_used))
+ {
+ var_list = lappend(var_list, makeVar(rtindex, i,
attr->atttypid,
+
attr->atttypmod,
+
attr->attcollation, 0));
+ *retrieved_attrs = lappend_int(*retrieved_attrs, i);
+ }
+ }
+
+ /*
+ * Add ctid if needed. We currently don't support retrieving any other
+ * system columns.
+ */
+ if (bms_is_member(SelfItemPointerAttributeNumber -
FirstLowInvalidHeapAttributeNumber,
+ attrs_used))
+ {
+
+ var_list = lappend(var_list, makeVar(rtindex,
+
SelfItemPointerAttributeNumber,
+
TIDOID, -1, InvalidOid, 0));
+ *retrieved_attrs = lappend_int(*retrieved_attrs,
+
SelfItemPointerAttributeNumber);
+ }
+
+ heap_close(rel, NoLock);
+
+ return add_to_flat_tlist(NIL, var_list);
+}
+
+/*
* Build the targetlist for given relation to be deparsed as SELECT clause.
*
* The output targetlist contains the columns that need to be fetched from the
* foreign server for the given relation.
+ *
+ * List of columns selected is returned in retrieved_attrs.
*/
List *
-build_tlist_to_deparse(RelOptInfo *foreignrel)
+build_tlist_to_deparse(RelOptInfo *foreignrel, PlannerInfo *root,
+ List **retrieved_attrs)
{
List *tlist = NIL;
PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)
foreignrel->fdw_private;
- /*
- * We require columns specified in foreignrel->reltargetlist and those
- * required for evaluating the local conditions.
- */
- tlist = add_to_flat_tlist(tlist, foreignrel->reltargetlist);
- tlist = add_to_flat_tlist(tlist,
- pull_var_clause((Node
*) fpinfo->local_conds,
-
PVC_REJECT_AGGREGATES,
-
PVC_RECURSE_PLACEHOLDERS));
+ if (foreignrel->reloptkind == RELOPT_JOINREL)
+ {
+ ListCell *lc;
+ int i = 1;
+
+ /*
+ * We require columns specified in foreignrel->reltargetlist
and those
+ * required for evaluating the local conditions.
+ */
+ tlist = add_to_flat_tlist(tlist, foreignrel->reltargetlist);
+ tlist = add_to_flat_tlist(tlist,
+
pull_var_clause((Node *) fpinfo->local_conds,
+
PVC_REJECT_AGGREGATES,
+
PVC_RECURSE_PLACEHOLDERS));
+ /*
+ * For a join relation, retrieved attributes is nothing but
list of
+ * continously increasing integers starting from 1 upto the
number of
+ * entries in the targetlist, representing the attribute
numbers of the
+ * output of join.
+ */
+ *retrieved_attrs = NIL;
+ foreach (lc, tlist)
+ *retrieved_attrs = lappend_int(*retrieved_attrs, i++);
+ }
+ else
+ tlist = build_tlist_from_attrs_used(foreignrel->relid,
+
fpinfo->attrs_used, root,
+
retrieved_attrs);
return tlist;
}
@@ -756,13 +851,11 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
* so Params and other-relation Vars should be replaced by dummy values.
*
* pathkeys is the list of pathkeys to order the result by.
- *
- * List of columns selected is returned in retrieved_attrs.
*/
extern void
deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *rel,
List *tlist, List
*remote_conds, List *pathkeys,
- List **retrieved_attrs, List
**params_list)
+ List **params_list)
{
deparse_expr_cxt context;
@@ -774,11 +867,12 @@ deparseSelectStmtForRel(StringInfo buf, PlannerInfo
*root, RelOptInfo *rel,
/* Fill portions of context common to join and base relation */
context.buf = buf;
context.root = root;
- context.foreignrel = rel;
+ context.relids = rel->relids;
context.params_list = params_list;
+ context.is_joinrel = (rel->reloptkind == RELOPT_JOINREL);
/* Construct SELECT clause and FROM clause */
- deparseSelectSql(tlist, retrieved_attrs, &context);
+ deparseSelectSql(tlist, rel, &context);
/*
* Construct WHERE clause
@@ -802,55 +896,26 @@ deparseSelectStmtForRel(StringInfo buf, PlannerInfo
*root, RelOptInfo *rel,
* of the specified foreign table, and append it to "buf". The output
* contains just "SELECT ... FROM ....".
*
- * We also create an integer List of the columns being retrieved, which is
- * returned to *retrieved_attrs.
- *
* tlist is the list of desired columns. Read prologue of
* deparseSelectStmtForRel() for details.
*/
static void
-deparseSelectSql(List *tlist, List **retrieved_attrs, deparse_expr_cxt
*context)
+deparseSelectSql(List *tlist, RelOptInfo *rel, deparse_expr_cxt *context)
{
StringInfo buf = context->buf;
- RelOptInfo *foreignrel = context->foreignrel;
PlannerInfo *root = context->root;
- PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)
foreignrel->fdw_private;
/*
* Construct SELECT list
*/
appendStringInfoString(buf, "SELECT ");
-
- if (foreignrel->reloptkind == RELOPT_JOINREL)
- {
- /* For a join relation use the input tlist */
- deparseExplicitTargetList(tlist, retrieved_attrs, context);
- }
- else
- {
- /*
- * For a base relation fpinfo->attrs_used gives the list of
columns
- * required to be fetched from the foreign server.
- */
- RangeTblEntry *rte = planner_rt_fetch(foreignrel->relid, root);
-
- /*
- * Core code already has some lock on each rel being planned,
so we
- * can use NoLock here.
- */
- Relation rel = heap_open(rte->relid, NoLock);
-
- deparseTargetList(buf, root, foreignrel->relid, rel,
fpinfo->attrs_used,
- retrieved_attrs, false);
- heap_close(rel, NoLock);
- }
+ deparseExplicitTargetList(tlist, context);
/*
* Construct FROM clause
*/
appendStringInfoString(buf, " FROM ");
- deparseFromExprForRel(buf, root, foreignrel,
- (foreignrel->reloptkind ==
RELOPT_JOINREL),
+ deparseFromExprForRel(buf, root, rel, context->is_joinrel,
context->params_list);
}
@@ -872,62 +937,18 @@ deparseTargetList(StringInfo buf,
List **retrieved_attrs,
bool qualify_col)
{
- TupleDesc tupdesc = RelationGetDescr(rel);
- bool have_wholerow;
- bool first;
- int i;
-
- *retrieved_attrs = NIL;
-
- /* If there's a whole-row reference, we'll need all the columns. */
- have_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber,
- attrs_used);
-
- first = true;
- for (i = 1; i <= tupdesc->natts; i++)
- {
- Form_pg_attribute attr = tupdesc->attrs[i - 1];
-
- /* Ignore dropped attributes. */
- if (attr->attisdropped)
- continue;
-
- if (have_wholerow ||
- bms_is_member(i - FirstLowInvalidHeapAttributeNumber,
- attrs_used))
- {
- if (!first)
- appendStringInfoString(buf, ", ");
- first = false;
-
- deparseColumnRef(buf, rtindex, i, root, qualify_col);
-
- *retrieved_attrs = lappend_int(*retrieved_attrs, i);
- }
- }
-
- /*
- * Add ctid if needed. We currently don't support retrieving any other
- * system columns.
- */
- if (bms_is_member(SelfItemPointerAttributeNumber -
FirstLowInvalidHeapAttributeNumber,
- attrs_used))
- {
- if (!first)
- appendStringInfoString(buf, ", ");
- first = false;
-
- if (qualify_col)
- ADD_REL_QUALIFIER(buf, rtindex);
- appendStringInfoString(buf, "ctid");
+ deparse_expr_cxt context;
+ List *tlist = build_tlist_from_attrs_used(rtindex, attrs_used, root,
+
retrieved_attrs);
- *retrieved_attrs = lappend_int(*retrieved_attrs,
-
SelfItemPointerAttributeNumber);
- }
+ context.buf = buf;
+ context.root = root;
+ context.relids = bms_make_singleton(rtindex);
+ /* Columns need to be qualified for join relation */
+ context.is_joinrel = qualify_col;
+ context.params_list = NULL;
- /* Don't generate bad syntax if no undropped columns */
- if (first)
- appendStringInfoString(buf, "NULL");
+ deparseExplicitTargetList(tlist, &context);
}
/*
@@ -939,10 +960,10 @@ deparseLockingClause(deparse_expr_cxt *context)
{
StringInfo buf = context->buf;
PlannerInfo *root = context->root;
- RelOptInfo *rel = context->foreignrel;
+ Relids relids = context->relids;
int relid = -1;
- while ((relid = bms_next_member(rel->relids, relid)) >= 0)
+ while ((relid = bms_next_member(relids, relid)) >= 0)
{
/*
* Add FOR UPDATE/SHARE if appropriate. We apply locking
during the
@@ -963,7 +984,7 @@ deparseLockingClause(deparse_expr_cxt *context)
appendStringInfoString(buf, " FOR UPDATE");
/* Add the relation alias if we are here for a join
relation */
- if (rel->reloptkind == RELOPT_JOINREL)
+ if (context->is_joinrel)
appendStringInfo(buf, " OF %s%d",
REL_ALIAS_PREFIX, relid);
}
else
@@ -999,8 +1020,7 @@ deparseLockingClause(deparse_expr_cxt *context)
}
/* Add the relation alias if we are here for a
join relation */
- if (rel->reloptkind == RELOPT_JOINREL &&
- rc->strength != LCS_NONE)
+ if (context->is_joinrel && rc->strength !=
LCS_NONE)
appendStringInfo(buf, " OF %s%d",
REL_ALIAS_PREFIX, relid);
}
}
@@ -1084,20 +1104,14 @@ get_jointype_name(JoinType jointype)
* Deparse given targetlist and append it to context->buf.
*
* tlist is list of TargetEntry's which in turn contain Var nodes.
- *
- * retrieved_attrs is the list of continuously increasing integers starting
- * from 1. It has same number of entries as tlist.
*/
static void
-deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
- deparse_expr_cxt *context)
+deparseExplicitTargetList(List *tlist, deparse_expr_cxt *context)
{
ListCell *lc;
StringInfo buf = context->buf;
int i = 0;
- *retrieved_attrs = NIL;
-
foreach(lc, tlist)
{
TargetEntry *tle = (TargetEntry *) lfirst(lc);
@@ -1113,8 +1127,6 @@ deparseExplicitTargetList(List *tlist, List
**retrieved_attrs,
appendStringInfoString(buf, ", ");
deparseVar(var, context);
- *retrieved_attrs = lappend_int(*retrieved_attrs, i + 1);
-
i++;
}
@@ -1162,9 +1174,10 @@ deparseFromExprForRel(StringInfo buf, PlannerInfo *root,
RelOptInfo *foreignrel,
deparse_expr_cxt context;
context.buf = buf;
- context.foreignrel = foreignrel;
+ context.relids = foreignrel->relids;
context.root = root;
context.params_list = params_list;
+ context.is_joinrel = true;
appendStringInfo(buf, "(");
appendConditions(fpinfo->joinclauses, &context);
@@ -1688,9 +1701,9 @@ deparseExpr(Expr *node, deparse_expr_cxt *context)
static void
deparseVar(Var *node, deparse_expr_cxt *context)
{
- bool qualify_col = (context->foreignrel->reloptkind ==
RELOPT_JOINREL);
+ bool qualify_col = context->is_joinrel;
- if (bms_is_member(node->varno, context->foreignrel->relids) &&
+ if (bms_is_member(node->varno, context->relids) &&
node->varlevelsup == 0)
deparseColumnRef(context->buf, node->varno, node->varattno,
context->root, qualify_col);
@@ -2278,7 +2291,7 @@ appendOrderByClause(List *pathkeys, deparse_expr_cxt
*context)
ListCell *lcell;
int nestlevel;
char *delim = " ";
- RelOptInfo *baserel = context->foreignrel;
+ Relids relids = context->relids;
StringInfo buf = context->buf;
/* Make sure any constants in the exprs are printed portably */
@@ -2290,7 +2303,7 @@ appendOrderByClause(List *pathkeys, deparse_expr_cxt
*context)
PathKey *pathkey = lfirst(lcell);
Expr *em_expr;
- em_expr = find_em_expr_for_rel(pathkey->pk_eclass, baserel);
+ em_expr = find_em_expr_for_rel(pathkey->pk_eclass, relids);
Assert(em_expr != NULL);
appendStringInfoString(buf, delim);
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index 23b4b23..71466ba 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -691,7 +691,7 @@ get_useful_pathkeys_for_relation(PlannerInfo *root,
RelOptInfo *rel)
* checking ec_has_volatile here saves some cycles.
*/
if (pathkey_ec->ec_has_volatile ||
- !(em_expr = find_em_expr_for_rel(pathkey_ec,
rel)) ||
+ !(em_expr = find_em_expr_for_rel(pathkey_ec,
rel->relids)) ||
!is_foreign_expr(root, rel, em_expr))
{
query_pathkeys_ok = false;
@@ -744,7 +744,7 @@ get_useful_pathkeys_for_relation(PlannerInfo *root,
RelOptInfo *rel)
continue;
/* If no pushable expression for this rel, skip it. */
- em_expr = find_em_expr_for_rel(cur_ec, rel);
+ em_expr = find_em_expr_for_rel(cur_ec, rel->relids);
if (em_expr == NULL || !is_foreign_expr(root, rel, em_expr))
continue;
@@ -1084,10 +1084,11 @@ postgresGetForeignPlan(PlannerInfo *root,
remote_conds = fpinfo->remote_conds;
local_exprs = fpinfo->local_conds;
- /* Build the list of columns to be fetched from the foreign
server. */
- fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
}
+ /* Build the list of columns to be fetched from the foreign server. */
+ fdw_scan_tlist = build_tlist_to_deparse(foreignrel, root,
&retrieved_attrs);
+
/*
* Build the query string to be sent for execution, and identify
* expressions to be sent as parameters.
@@ -1095,7 +1096,7 @@ postgresGetForeignPlan(PlannerInfo *root,
initStringInfo(&sql);
deparseSelectStmtForRel(&sql, root, foreignrel, fdw_scan_tlist,
remote_conds,
best_path->path.pathkeys,
- &retrieved_attrs,
¶ms_list);
+ ¶ms_list);
/*
* Build the fdw_private list that will be available to the executor.
@@ -1108,6 +1109,11 @@ postgresGetForeignPlan(PlannerInfo *root,
if (foreignrel->reloptkind == RELOPT_JOINREL)
fdw_private = lappend(fdw_private,
makeString(fpinfo->relation_name->data));
+ else
+ {
+ /* We shouldn't set fdw_scan_tlist for a base relation */
+ fdw_scan_tlist = NIL;
+ }
/*
* Create the ForeignScan node for the given relation.
@@ -2070,7 +2076,7 @@ estimate_path_cost_size(PlannerInfo *root,
List *fdw_scan_tlist = NIL;
List *remote_conds;
- /* Required only to be passed to deparseSelectStmtForRel */
+ /* Required only to be passed to build_tlist_to_deparse(). */
List *retrieved_attrs;
/*
@@ -2081,10 +2087,8 @@ estimate_path_cost_size(PlannerInfo *root,
&remote_param_join_conds,
&local_param_join_conds);
/* Build the list of columns to be fetched from the foreign
server. */
- if (foreignrel->reloptkind == RELOPT_JOINREL)
- fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
- else
- fdw_scan_tlist = NIL;
+ fdw_scan_tlist = build_tlist_to_deparse(foreignrel, root,
+
&retrieved_attrs);
/*
* The complete list of remote conditions includes everything
from
@@ -2102,8 +2106,7 @@ estimate_path_cost_size(PlannerInfo *root,
initStringInfo(&sql);
appendStringInfoString(&sql, "EXPLAIN ");
deparseSelectStmtForRel(&sql, root, foreignrel, fdw_scan_tlist,
- remote_conds,
pathkeys, &retrieved_attrs,
- NULL);
+ remote_conds,
pathkeys, NULL);
/* Get the remote estimate */
conn = GetConnection(fpinfo->user, false);
@@ -3833,7 +3836,7 @@ conversion_error_callback(void *arg)
* the indicated relation.
*/
extern Expr *
-find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+find_em_expr_for_rel(EquivalenceClass *ec, Relids relids)
{
ListCell *lc_em;
@@ -3841,7 +3844,7 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo
*rel)
{
EquivalenceMember *em = lfirst(lc_em);
- if (bms_equal(em->em_relids, rel->relids))
+ if (bms_equal(em->em_relids, relids))
{
/*
* If there is more than one equivalence member whose
Vars are
diff --git a/contrib/postgres_fdw/postgres_fdw.h
b/contrib/postgres_fdw/postgres_fdw.h
index 4633d46..a39ad6b 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -138,12 +138,12 @@ extern void deparseAnalyzeSizeSql(StringInfo buf,
Relation rel);
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 List *build_tlist_to_deparse(RelOptInfo *foreign_rel);
+extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, Relids relids);
+extern List *build_tlist_to_deparse(RelOptInfo *foreign_rel, PlannerInfo *root,
+ List **retrieved_attrs);
extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
RelOptInfo *foreignrel, List
*tlist,
- List *remote_conds, List
*pathkeys,
- List **retrieved_attrs, List
**params_list);
+ List *remote_conds, List
*pathkeys, List **params_list);
/* in shippable.c */
extern bool is_builtin(Oid objectId);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
