(2018/12/28 17:28), Etsuro Fujita wrote:
> I noticed that I forgot to modify the cost for evaluating the PathTarget
> for each output row accordingly to this change :(. Attached is a patch
> for that.
On reflection, I noticed these on estimate_path_cost_size, other than that:
1) In the case of a foreign-grouping path, we failed to adjust the
PathTarget eval cost when using the remote estimates, which I think
would be needed because the PathTarget expressions cannot always be
pre-computed as entries of the fdw_scan_tlist for the foreign-grouping path.
2) We also failed to factor in the eval cost for the foreign-scan and
foreign-join cases, with/without the remote estimates; in the scan/join
cases, the PathTarget might contain PHVs, so the cost of evaluating PHVs
should be charged. Currently, PHVs are evaluated locally, so the cost
of PHV expressions should also be factored in when using the remote
estimates.
Here is a new version of the patch.
Best regards,
Etsuro Fujita
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2ac9d7b..3304ebe 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2667,6 +2667,9 @@ estimate_path_cost_size(PlannerInfo *root,
Cost total_cost;
Cost cpu_per_tuple;
+ /* Make sure the core code has set up the relation's reltarget */
+ Assert(foreignrel->reltarget);
+
/*
* If the table or the server is configured to use remote estimates,
* connect to the foreign server and execute EXPLAIN to estimate the
@@ -2744,6 +2747,25 @@ estimate_path_cost_size(PlannerInfo *root,
cost_qual_eval(&local_cost, local_param_join_conds, root);
startup_cost += local_cost.startup;
total_cost += local_cost.per_tuple * retrieved_rows;
+
+ /* Add in the tlist eval cost for each output row */
+ startup_cost += foreignrel->reltarget->cost.startup;
+ total_cost += foreignrel->reltarget->cost.per_tuple * rows;
+
+ /*
+ * In the case of a foreign-grouping path, the fdw_scan_tlist would
+ * have contained tlist expressions that will be pushed down to the
+ * remote as-is including at least grouping expressions; adjust the
+ * eval cost.
+ */
+ if (IS_UPPER_REL(foreignrel))
+ {
+ QualCost tlist_cost;
+
+ cost_qual_eval(&tlist_cost, fdw_scan_tlist, root);
+ startup_cost -= tlist_cost.startup;
+ total_cost -= tlist_cost.per_tuple * rows;
+ }
}
else
{
@@ -2842,19 +2864,19 @@ estimate_path_cost_size(PlannerInfo *root,
nrows = clamp_row_est(nrows * fpinfo->joinclause_sel);
run_cost += nrows * remote_conds_cost.per_tuple;
run_cost += fpinfo->local_conds_cost.per_tuple * retrieved_rows;
+
+ /* Add in the tlist eval cost for each output row */
+ startup_cost += foreignrel->reltarget->cost.startup;
+ run_cost += foreignrel->reltarget->cost.per_tuple * rows;
}
else if (IS_UPPER_REL(foreignrel))
{
PgFdwRelationInfo *ofpinfo;
- PathTarget *ptarget = foreignrel->reltarget;
AggClauseCosts aggcosts;
double input_rows;
int numGroupCols;
double numGroups = 1;
- /* Make sure the core code set the pathtarget. */
- Assert(ptarget != NULL);
-
/*
* This cost model is mixture of costing done for sorted and
* hashed aggregates in cost_agg(). We are not sure which
@@ -2918,26 +2940,22 @@ estimate_path_cost_size(PlannerInfo *root,
* Startup cost includes:
* 1. Startup cost for underneath input relation
* 2. Cost of performing aggregation, per cost_agg()
- * 3. Startup cost for PathTarget eval
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += (cpu_operator_cost * numGroupCols) * input_rows;
- startup_cost += ptarget->cost.startup;
/*-----
* Run time cost includes:
* 1. Run time cost of underneath input relation
* 2. Run time cost of performing aggregation, per cost_agg()
- * 3. PathTarget eval cost for each output row
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
run_cost += aggcosts.finalCost * numGroups;
run_cost += cpu_tuple_cost * numGroups;
- run_cost += ptarget->cost.per_tuple * numGroups;
/* Accout for the eval cost of HAVING quals, if any */
if (root->parse->havingQual)
@@ -2952,6 +2970,10 @@ estimate_path_cost_size(PlannerInfo *root,
startup_cost += fpinfo->local_conds_cost.startup;
run_cost += fpinfo->local_conds_cost.per_tuple * retrieved_rows;
}
+
+ /* Add in the tlist eval cost for each output row */
+ startup_cost += foreignrel->reltarget->cost.startup;
+ run_cost += foreignrel->reltarget->cost.per_tuple * rows;
}
else
{
@@ -2970,6 +2992,10 @@ estimate_path_cost_size(PlannerInfo *root,
startup_cost += foreignrel->baserestrictcost.startup;
cpu_per_tuple = cpu_tuple_cost + foreignrel->baserestrictcost.per_tuple;
run_cost += cpu_per_tuple * foreignrel->tuples;
+
+ /* Add int the tlist eval cost for each output row */
+ startup_cost += foreignrel->reltarget->cost.startup;
+ run_cost += foreignrel->reltarget->cost.per_tuple * rows;
}
/*
