(2014/02/18 12:37), Tom Lane wrote:
> Etsuro Fujita <[email protected]> writes:
>> (2014/02/18 12:03), Tom Lane wrote:
>>> The calling FDW is supposed to do that; note the header comment.
>
>> However, ISTM postgresGetForeignPaths() doesn't work like
>> that. It uses the same rowcount for all paths of a same parameterization?
>
> That's what we want no?
Maybe I'm missing something. But I don't think
postgresGetForeignPaths() marks the parameterized path with the correct
row estimate. Also, that function doesn't seem to estimate the cost of
the parameterized path correctly. Please find attached a patch.
> Anyway, the point of using ppi_rows would be to enforce that all the
> rowcount estimates for a given parameterized relation are the same.
> In the FDW case, all those estimates are the FDW's responsibility,
> and so making them consistent is also its responsibility IMO.
>
> Another way of looking at this is that none of the pathnode creation
> routines in pathnode.c are responsible for setting rowcount estimates.
> That's done by whatever is setting the cost estimate; this must be so,
> else the cost estimate is surely bogus. So any way you slice it, the
> FDW has to get it right.
Understood. Thank you for the analysis.
Sorry for the delay.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 295,300 **** static bool postgresAnalyzeForeignTable(Relation relation,
--- 295,301 ----
static void estimate_path_cost_size(PlannerInfo *root,
RelOptInfo *baserel,
List *join_conds,
+ List *param_conds,
double *p_rows, int *p_width,
Cost *p_startup_cost, Cost
*p_total_cost);
static void get_remote_estimate(const char *sql,
***************
*** 493,499 **** postgresGetForeignRelSize(PlannerInfo *root,
* values in fpinfo so we don't need to do it again to generate
the
* basic foreign path.
*/
! estimate_path_cost_size(root, baserel, NIL,
&fpinfo->rows,
&fpinfo->width,
&fpinfo->startup_cost, &fpinfo->total_cost);
--- 494,500 ----
* values in fpinfo so we don't need to do it again to generate
the
* basic foreign path.
*/
! estimate_path_cost_size(root, baserel, NIL, NIL,
&fpinfo->rows,
&fpinfo->width,
&fpinfo->startup_cost, &fpinfo->total_cost);
***************
*** 523,529 **** postgresGetForeignRelSize(PlannerInfo *root,
set_baserel_size_estimates(root, baserel);
/* Fill in basically-bogus cost estimates for use later. */
! estimate_path_cost_size(root, baserel, NIL,
&fpinfo->rows,
&fpinfo->width,
&fpinfo->startup_cost, &fpinfo->total_cost);
}
--- 524,530 ----
set_baserel_size_estimates(root, baserel);
/* Fill in basically-bogus cost estimates for use later. */
! estimate_path_cost_size(root, baserel, NIL, NIL,
&fpinfo->rows,
&fpinfo->width,
&fpinfo->startup_cost, &fpinfo->total_cost);
}
***************
*** 541,547 **** postgresGetForeignPaths(PlannerInfo *root,
--- 542,550 ----
PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private;
ForeignPath *path;
List *join_quals;
+ List *param_quals;
Relids required_outer;
+ ParamPathInfo *param_info;
double rows;
int width;
Cost startup_cost;
***************
*** 591,604 **** postgresGetForeignPaths(PlannerInfo *root,
if (!is_foreign_expr(root, baserel, rinfo->clause))
continue;
- /*
- * OK, get a cost estimate from the remote, and make a path.
- */
- join_quals = list_make1(rinfo);
- estimate_path_cost_size(root, baserel, join_quals,
- &rows, &width,
- &startup_cost,
&total_cost);
-
/* Must calculate required outer rels for this path */
required_outer = bms_union(rinfo->clause_relids,
baserel->lateral_relids);
--- 594,599 ----
***************
*** 608,613 **** postgresGetForeignPaths(PlannerInfo *root,
--- 603,622 ----
if (bms_is_empty(required_outer))
required_outer = NULL;
+ /*
+ * OK, get a cost estimate from the remote, and make a path.
+ */
+ join_quals = list_make1(rinfo);
+ param_info = get_baserel_parampathinfo(root, baserel,
required_outer);
+ param_quals = list_copy(param_info->ppi_clauses);
+ estimate_path_cost_size(root, baserel, join_quals, param_quals,
+ &rows, &width,
+ &startup_cost,
&total_cost);
+
+ /* Mark the path with the correct row estimate */
+ if (param_info)
+ rows = param_info->ppi_rows;
+
path = create_foreignscan_path(root, baserel,
rows,
startup_cost,
***************
*** 667,686 **** postgresGetForeignPaths(PlannerInfo *root,
if (!is_foreign_expr(root, baserel,
rinfo->clause))
continue;
/*
* OK, get a cost estimate from the remote, and
make a path.
*/
join_quals = list_make1(rinfo);
! estimate_path_cost_size(root, baserel,
join_quals,
&rows, &width,
&startup_cost, &total_cost);
! /* Must calculate required outer rels for this
path */
! required_outer = bms_union(rinfo->clause_relids,
!
baserel->lateral_relids);
! required_outer = bms_del_member(required_outer,
baserel->relid);
! if (bms_is_empty(required_outer))
! required_outer = NULL;
path = create_foreignscan_path(root, baserel,
rows,
--- 676,702 ----
if (!is_foreign_expr(root, baserel,
rinfo->clause))
continue;
+ /* Must calculate required outer rels for this
path */
+ required_outer = bms_union(rinfo->clause_relids,
+
baserel->lateral_relids);
+ required_outer = bms_del_member(required_outer,
baserel->relid);
+ if (bms_is_empty(required_outer))
+ required_outer = NULL;
+
/*
* OK, get a cost estimate from the remote, and
make a path.
*/
join_quals = list_make1(rinfo);
! param_info = get_baserel_parampathinfo(root,
baserel,
!
required_outer);
! param_quals =
list_copy(param_info->ppi_clauses);
! estimate_path_cost_size(root, baserel,
join_quals, param_quals,
&rows, &width,
&startup_cost, &total_cost);
! /* Mark the path with the correct row estimate
*/
! if (param_info)
! rows = param_info->ppi_rows;
path = create_foreignscan_path(root, baserel,
rows,
***************
*** 1662,1673 **** postgresExplainForeignModify(ModifyTableState *mtstate,
* Get cost and size estimates for a foreign scan
*
* We assume that all the baserestrictinfo clauses will be applied, plus
! * any join clauses listed in join_conds.
*/
static void
estimate_path_cost_size(PlannerInfo *root,
RelOptInfo *baserel,
List *join_conds,
double *p_rows, int *p_width,
Cost *p_startup_cost, Cost
*p_total_cost)
{
--- 1678,1691 ----
* Get cost and size estimates for a foreign scan
*
* We assume that all the baserestrictinfo clauses will be applied, plus
! * any join clauses listed in join_conds and all the extra join clauses
! * appearing in param_conds.
*/
static void
estimate_path_cost_size(PlannerInfo *root,
RelOptInfo *baserel,
List *join_conds,
+ List *param_conds,
double *p_rows, int *p_width,
Cost *p_startup_cost, Cost
*p_total_cost)
{
***************
*** 1723,1728 **** estimate_path_cost_size(PlannerInfo *root,
--- 1741,1764 ----
/* Add in the eval cost of the local_conds */
startup_cost += fpinfo->local_conds_cost.startup;
total_cost += fpinfo->local_conds_cost.per_tuple *
retrieved_rows;
+
+ /* Add in the eval cost of extra join clauses in the
param_conds */
+ if (param_conds)
+ {
+ List *extra_conds;
+ QualCost extra_conds_cost;
+
+ /*
+ * We approximate the list of all the extra join
clauses as
+ * param_conds minus join_conds. (We assume that
pointer
+ * equality is enough to recognize duplicate
RestrictInfos.)
+ */
+ extra_conds = list_difference_ptr(param_conds,
join_conds);
+ cost_qual_eval(&extra_conds_cost, extra_conds, root);
+
+ startup_cost += extra_conds_cost.startup;
+ total_cost += extra_conds_cost.per_tuple *
retrieved_rows;
+ }
}
else
{
***************
*** 1731,1736 **** estimate_path_cost_size(PlannerInfo *root,
--- 1767,1773 ----
* parameterized paths can be made).
*/
Assert(join_conds == NIL);
+ Assert(param_conds == NIL);
/* Use rows/width estimates made by set_baserel_size_estimates.
*/
rows = baserel->rows;
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
