On 2017/01/12 18:25, Ashutosh Bapat wrote:
On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
On 2017/01/05 21:11, Ashutosh Bapat wrote:
IIUC, for a relation with use_remote_estimates we will deparse the
query twice and will build the targetlist twice.
While working on this, I noticed a weird case. Consider:
postgres=# explain verbose select 1 from ft1 left join ft2 on (ft1.a =
ft2.a) inner join test on (true);
QUERY PLAN
-------------------------------------------------------------------------------------------------
Nested Loop (cost=100.00..103.06 rows=1 width=4)
Output: 1
-> Foreign Scan (cost=100.00..102.04 rows=1 width=0)
Relations: (public.ft1) LEFT JOIN (public.ft2)
Remote SQL: SELECT NULL FROM (public.t1 r1 LEFT JOIN public.t2 r2
ON (((r1.a = r2.a))))
-> Seq Scan on public.test (cost=0.00..1.01 rows=1 width=0)
Output: test.a, test.b
(7 rows)
In this case the fpinfo->tlist of the foreign join is NIL, so whether or not
the tlist is already built cannot be discriminated by the fpinfo->tlist. We
might need another flag to show that the tlist has been built already.
Since this is an existing issue and we would need to give careful thought to
this, so I'd like to leave this for another patch.
I think in that case, relation's targetlist will also be NIL or
contain no Var node. It wouldn't be expensive to build it again and
again. That's better than maintaining a new flag.
I think that's ugly. A more clean way I'm thinking is: (1) in
postgresGetForeignJoinPaths(), create a tlist by
build_tlist_to_deparse() and save it in fpinfo->tlist before
estimate_path_cost_size() if use_remote_estimates=true, (2) in
estimate_path_cost_size(), use the fpinfo->tlist if
use_remote_estimates=true, and (3) in postgresGetForeignPlan(), use the
fpinfo->tlist as the fdw_scan_tlist if use_remote_estimates=true,
otherwise create a tlist as the fdw_scan_tlist by
build_tlist_to_deparse(), like the attached.
What do you think about that?
Another change is: I simplified build_tlist_to_deparse() a bit and put
that in postgres_fdw.c because that is used only in postgres_fdw.c.
I still think we should discuss this separately because this is an
existing issue and that makes it easy to review the patch. If the
attached is the right way to go, I'll update the join-pushdown patch on
top of the patch.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***************
*** 843,883 **** deparse_type_name(Oid type_oid, int32 typemod)
}
/*
- * 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. If foreignrel is an upper relation,
- * then the output targetlist can also contain expressions to be evaluated on
- * foreign server.
- */
- List *
- build_tlist_to_deparse(RelOptInfo *foreignrel)
- {
- List *tlist = NIL;
- PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
-
- /*
- * For an upper relation, we have already built the target list while
- * checking shippability, so just return that.
- */
- if (foreignrel->reloptkind == RELOPT_UPPER_REL)
- return fpinfo->grouped_tlist;
-
- /*
- * We require columns specified in foreignrel->reltarget->exprs and those
- * required for evaluating the local conditions.
- */
- tlist = add_to_flat_tlist(tlist,
- pull_var_clause((Node *) foreignrel->reltarget->exprs,
- PVC_RECURSE_PLACEHOLDERS));
- tlist = add_to_flat_tlist(tlist,
- pull_var_clause((Node *) fpinfo->local_conds,
- PVC_RECURSE_PLACEHOLDERS));
-
- return tlist;
- }
-
- /*
* Deparse SELECT statement for given relation into buf.
*
* tlist contains the list of desired columns to be fetched from foreign server.
--- 843,848 ----
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 406,411 **** static void conversion_error_callback(void *arg);
--- 406,412 ----
static bool foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel,
JoinType jointype, RelOptInfo *outerrel, RelOptInfo *innerrel,
JoinPathExtraData *extra);
+ static List *build_tlist_to_deparse(RelOptInfo *foreignrel);
static bool foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel);
static List *get_useful_pathkeys_for_relation(PlannerInfo *root,
RelOptInfo *rel);
***************
*** 1190,1197 **** 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);
/*
* Ensure that the outer plan produces a tuple whose descriptor
--- 1191,1209 ----
remote_conds = fpinfo->remote_conds;
local_exprs = fpinfo->local_conds;
! /* Get the list of columns to be fetched from the foreign server. */
! if ((fpinfo->use_remote_estimate &&
! foreignrel->reloptkind == RELOPT_JOINREL) ||
! foreignrel->reloptkind == RELOPT_UPPER_REL)
! {
! /*
! * We have already built the tlist for a join relation as well as
! * for an upper relation.
! */
! fdw_scan_tlist = fpinfo->tlist;
! }
! else
! fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
/*
* Ensure that the outer plan produces a tuple whose descriptor
***************
*** 2528,2537 **** estimate_path_cost_size(PlannerInfo *root,
classifyConditions(root, foreignrel, param_join_conds,
&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 ||
foreignrel->reloptkind == RELOPT_UPPER_REL)
! fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
else
fdw_scan_tlist = NIL;
--- 2540,2555 ----
classifyConditions(root, foreignrel, param_join_conds,
&remote_param_join_conds, &local_param_join_conds);
! /* Get the list of columns to be fetched from the foreign server. */
if (foreignrel->reloptkind == RELOPT_JOINREL ||
foreignrel->reloptkind == RELOPT_UPPER_REL)
! {
! /*
! * We have already built the tlist for a join relation as well as
! * for an upper relation.
! */
! fdw_scan_tlist = fpinfo->tlist;
! }
else
fdw_scan_tlist = NIL;
***************
*** 2708,2714 **** estimate_path_cost_size(PlannerInfo *root,
MemSet(&aggcosts, 0, sizeof(AggClauseCosts));
if (root->parse->hasAggs)
{
! get_agg_clause_costs(root, (Node *) fpinfo->grouped_tlist,
AGGSPLIT_SIMPLE, &aggcosts);
get_agg_clause_costs(root, (Node *) root->parse->havingQual,
AGGSPLIT_SIMPLE, &aggcosts);
--- 2726,2732 ----
MemSet(&aggcosts, 0, sizeof(AggClauseCosts));
if (root->parse->hasAggs)
{
! get_agg_clause_costs(root, (Node *) fpinfo->tlist,
AGGSPLIT_SIMPLE, &aggcosts);
get_agg_clause_costs(root, (Node *) root->parse->havingQual,
AGGSPLIT_SIMPLE, &aggcosts);
***************
*** 2718,2724 **** estimate_path_cost_size(PlannerInfo *root,
numGroupCols = list_length(root->parse->groupClause);
numGroups = estimate_num_groups(root,
get_sortgrouplist_exprs(root->parse->groupClause,
! fpinfo->grouped_tlist),
input_rows, NULL);
/*
--- 2736,2742 ----
numGroupCols = list_length(root->parse->groupClause);
numGroups = estimate_num_groups(root,
get_sortgrouplist_exprs(root->parse->groupClause,
! fpinfo->tlist),
input_rows, NULL);
/*
***************
*** 4276,4281 **** foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
--- 4294,4325 ----
return true;
}
+ /*
+ * 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.
+ */
+ static List *
+ build_tlist_to_deparse(RelOptInfo *foreignrel)
+ {
+ List *tlist = NIL;
+ PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
+
+ /*
+ * We require columns specified in foreignrel->reltarget->exprs and those
+ * required for evaluating the local conditions.
+ */
+ tlist = add_to_flat_tlist(tlist,
+ pull_var_clause((Node *) foreignrel->reltarget->exprs,
+ PVC_RECURSE_PLACEHOLDERS));
+ tlist = add_to_flat_tlist(tlist,
+ pull_var_clause((Node *) fpinfo->local_conds,
+ PVC_RECURSE_PLACEHOLDERS));
+
+ return tlist;
+ }
+
static void
add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel,
Path *epq_path)
***************
*** 4407,4412 **** postgresGetForeignJoinPaths(PlannerInfo *root,
--- 4451,4460 ----
0, fpinfo->jointype,
extra->sjinfo);
+ /* If we are going to estimate costs remotely, build the tlist here. */
+ if (fpinfo->use_remote_estimate)
+ fpinfo->tlist = build_tlist_to_deparse(joinrel);
+
/* Estimate costs for bare join relation */
estimate_path_cost_size(root, joinrel, NIL, NIL, &rows,
&width, &startup_cost, &total_cost);
***************
*** 4613,4619 **** foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
apply_pathtarget_labeling_to_tlist(tlist, grouping_target);
/* Store generated targetlist */
! fpinfo->grouped_tlist = tlist;
/* Safe to pushdown */
fpinfo->pushdown_safe = true;
--- 4661,4667 ----
apply_pathtarget_labeling_to_tlist(tlist, grouping_target);
/* Store generated targetlist */
! fpinfo->tlist = tlist;
/* Safe to pushdown */
fpinfo->pushdown_safe = true;
*** a/contrib/postgres_fdw/postgres_fdw.h
--- b/contrib/postgres_fdw/postgres_fdw.h
***************
*** 93,100 **** typedef struct PgFdwRelationInfo
JoinType jointype;
List *joinclauses;
! /* Grouping information */
! List *grouped_tlist;
} PgFdwRelationInfo;
/* in postgres_fdw.c */
--- 93,103 ----
JoinType jointype;
List *joinclauses;
! /*
! * Optional tlist describing the contents of the scan tuple from the
! * relation.
! */
! List *tlist;
} PgFdwRelationInfo;
/* in postgres_fdw.c */
***************
*** 158,164 **** 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 *foreignrel);
extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root,
RelOptInfo *foreignrel, List *tlist,
List *remote_conds, List *pathkeys,
--- 161,166 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
