pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedpostgres_fdw: another oddity in costing aggregate pushdown paths
5+ messages / 3 participants
[nested] [flat]
* postgres_fdw: another oddity in costing aggregate pushdown paths
@ 2019-02-15 11:19 Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-02-22 14:10 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 5+ messages in thread
From: Etsuro Fujita @ 2019-02-15 11:19 UTC (permalink / raw)
To: pgsql-hackers
As mentioned in the near thread, I think there is another oversight in
the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
When costing an aggregate pushdown path using local statistics, we
re-use the estimated costs of implementing the underlying scan/join
relation, cached in the relation's PgFdwRelationInfo (ie,
rel_startup_cost and rel_total_cost). Since these costs wouldn't yet
contain the costs of evaluating the final scan/join target, as tlist
replacement by apply_scanjoin_target_to_paths() is performed afterwards.
So I think we need to adjust these costs so that the tlist eval costs
are included, but ISTM that estimate_path_cost_size() forgot to do so.
Attached is a patch for fixing this issue.
Best regards,
Etsuro Fujita
Attachments:
[text/x-patch] another-fix-postgres-fdw-grouping-path-cost.patch (3.1K, ../../5C66A056.60007@lab.ntt.co.jp/2-another-fix-postgres-fdw-grouping-path-cost.patch)
download | inline diff:
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 2705,2716 **** estimate_path_cost_size(PlannerInfo *root,
--- 2705,2719 ----
}
else if (IS_UPPER_REL(foreignrel))
{
+ RelOptInfo *outerrel = fpinfo->outerrel;
PgFdwRelationInfo *ofpinfo;
AggClauseCosts aggcosts;
double input_rows;
int numGroupCols;
double numGroups = 1;
+ Assert(outerrel);
+
/*
* This cost model is mixture of costing done for sorted and
* hashed aggregates in cost_agg(). We are not sure which
***************
*** 2719,2725 **** estimate_path_cost_size(PlannerInfo *root,
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
--- 2722,2728 ----
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
***************
*** 2772,2782 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
--- 2775,2787 ----
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
+ startup_cost += outerrel->reltarget->cost.startup;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
***************
*** 2784,2794 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
--- 2789,2801 ----
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
+ run_cost += outerrel->reltarget->cost.per_tuple * input_rows;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: postgres_fdw: another oddity in costing aggregate pushdown paths
2019-02-15 11:19 postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
@ 2019-02-22 14:10 ` Antonin Houska <ah@cybertec.at>
2019-02-25 10:59 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
0 siblings, 1 reply; 5+ messages in thread
From: Antonin Houska @ 2019-02-22 14:10 UTC (permalink / raw)
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>; +Cc: pgsql-hackers
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> As mentioned in the near thread, I think there is another oversight in
> the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
> When costing an aggregate pushdown path using local statistics, we
> re-use the estimated costs of implementing the underlying scan/join
> relation, cached in the relation's PgFdwRelationInfo (ie,
> rel_startup_cost and rel_total_cost). Since these costs wouldn't yet
> contain the costs of evaluating the final scan/join target, as tlist
> replacement by apply_scanjoin_target_to_paths() is performed afterwards.
> So I think we need to adjust these costs so that the tlist eval costs
> are included, but ISTM that estimate_path_cost_size() forgot to do so.
> Attached is a patch for fixing this issue.
I think the following comment in apply_scanjoin_target_to_paths() should
mention that FDWs rely on the new value of reltarget.
/*
* Update the reltarget. This may not be strictly necessary in all cases,
* but it is at least necessary when create_append_path() gets called
* below directly or indirectly, since that function uses the reltarget as
* the pathtarget for the resulting path. It seems like a good idea to do
* it unconditionally.
*/
rel->reltarget = llast_node(PathTarget, scanjoin_targets);
--
Antonin Houska
https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: postgres_fdw: another oddity in costing aggregate pushdown paths
2019-02-15 11:19 postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-02-22 14:10 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Antonin Houska <ah@cybertec.at>
@ 2019-02-25 10:59 ` Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-05-08 03:42 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
0 siblings, 1 reply; 5+ messages in thread
From: Etsuro Fujita @ 2019-02-25 10:59 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers
(2019/02/22 23:10), Antonin Houska wrote:
> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> wrote:
>> As mentioned in the near thread, I think there is another oversight in
>> the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
>> When costing an aggregate pushdown path using local statistics, we
>> re-use the estimated costs of implementing the underlying scan/join
>> relation, cached in the relation's PgFdwRelationInfo (ie,
>> rel_startup_cost and rel_total_cost). Since these costs wouldn't yet
>> contain the costs of evaluating the final scan/join target, as tlist
>> replacement by apply_scanjoin_target_to_paths() is performed afterwards.
>> So I think we need to adjust these costs so that the tlist eval costs
>> are included, but ISTM that estimate_path_cost_size() forgot to do so.
>> Attached is a patch for fixing this issue.
>
> I think the following comment in apply_scanjoin_target_to_paths() should
> mention that FDWs rely on the new value of reltarget.
>
> /*
> * Update the reltarget. This may not be strictly necessary in all cases,
> * but it is at least necessary when create_append_path() gets called
> * below directly or indirectly, since that function uses the reltarget as
> * the pathtarget for the resulting path. It seems like a good idea to do
> * it unconditionally.
> */
> rel->reltarget = llast_node(PathTarget, scanjoin_targets);
Agreed. How about mentioning that like the attached? In addition, I
added another assertion to estimate_path_cost_size() in that patch.
Thanks for the review!
Best regards,
Etsuro Fujita
Attachments:
[text/x-patch] another-fix-postgres-fdw-grouping-path-cost-v2.patch (4.4K, ../../5C73CA75.8060005@lab.ntt.co.jp/2-another-fix-postgres-fdw-grouping-path-cost-v2.patch)
download | inline diff:
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 2705,2716 **** estimate_path_cost_size(PlannerInfo *root,
--- 2705,2722 ----
}
else if (IS_UPPER_REL(foreignrel))
{
+ RelOptInfo *outerrel = fpinfo->outerrel;
PgFdwRelationInfo *ofpinfo;
AggClauseCosts aggcosts;
double input_rows;
int numGroupCols;
double numGroups = 1;
+ /* The upper relation should have its outer relation set */
+ Assert(outerrel);
+ /* The outer relation should have its reltarget set */
+ Assert(outerrel->reltarget);
+
/*
* This cost model is mixture of costing done for sorted and
* hashed aggregates in cost_agg(). We are not sure which
***************
*** 2719,2725 **** estimate_path_cost_size(PlannerInfo *root,
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
--- 2725,2731 ----
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
***************
*** 2772,2782 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
--- 2778,2790 ----
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
+ startup_cost += outerrel->reltarget->cost.startup;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
***************
*** 2784,2794 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
--- 2792,2804 ----
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
+ run_cost += outerrel->reltarget->cost.per_tuple * input_rows;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 6932,6941 **** apply_scanjoin_target_to_paths(PlannerInfo *root,
/*
* Update the reltarget. This may not be strictly necessary in all cases,
! * but it is at least necessary when create_append_path() gets called
* below directly or indirectly, since that function uses the reltarget as
! * the pathtarget for the resulting path. It seems like a good idea to do
! * it unconditionally.
*/
rel->reltarget = llast_node(PathTarget, scanjoin_targets);
--- 6932,6943 ----
/*
* Update the reltarget. This may not be strictly necessary in all cases,
! * but it is at least necessary 1) when create_append_path() gets called
* below directly or indirectly, since that function uses the reltarget as
! * the pathtarget for the resulting path, and/or 2) when
! * GetForeignUpperPaths() gets called later on the relation or its
! * children, since the FDW might reference the reltarget to do the work.
! * It seems like a good idea to do it unconditionally.
*/
rel->reltarget = llast_node(PathTarget, scanjoin_targets);
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: postgres_fdw: another oddity in costing aggregate pushdown paths
2019-02-15 11:19 postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-02-22 14:10 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Antonin Houska <ah@cybertec.at>
2019-02-25 10:59 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
@ 2019-05-08 03:42 ` Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-05-09 09:52 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Etsuro Fujita @ 2019-05-08 03:42 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers
(2019/02/25 19:59), Etsuro Fujita wrote:
> (2019/02/22 23:10), Antonin Houska wrote:
>> Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp> wrote:
>>> As mentioned in the near thread, I think there is another oversight in
>>> the cost estimation for aggregate pushdown paths in postgres_fdw, IIUC.
>>> When costing an aggregate pushdown path using local statistics, we
>>> re-use the estimated costs of implementing the underlying scan/join
>>> relation, cached in the relation's PgFdwRelationInfo (ie,
>>> rel_startup_cost and rel_total_cost). Since these costs wouldn't yet
>>> contain the costs of evaluating the final scan/join target, as tlist
>>> replacement by apply_scanjoin_target_to_paths() is performed afterwards.
>>> So I think we need to adjust these costs so that the tlist eval costs
>>> are included, but ISTM that estimate_path_cost_size() forgot to do so.
>>> Attached is a patch for fixing this issue.
>>
>> I think the following comment in apply_scanjoin_target_to_paths() should
>> mention that FDWs rely on the new value of reltarget.
>>
>> /*
>> * Update the reltarget. This may not be strictly necessary in all cases,
>> * but it is at least necessary when create_append_path() gets called
>> * below directly or indirectly, since that function uses the reltarget as
>> * the pathtarget for the resulting path. It seems like a good idea to do
>> * it unconditionally.
>> */
>> rel->reltarget = llast_node(PathTarget, scanjoin_targets);
>
> Agreed. How about mentioning that like the attached? In addition, I
> added another assertion to estimate_path_cost_size() in that patch.
This doesn't get applied cleanly after commit 1d33858406. Here is a
rebased version of the patch. I also modified the comments a little
bit. If there are no objections from Antonin or anyone else, I'll
commit the patch.
Best regards,
Etsuro Fujita
Attachments:
[text/x-patch] another-fix-postgres-fdw-grouping-path-cost-v3.patch (3.9K, ../../5CD2503B.6010108@lab.ntt.co.jp/2-another-fix-postgres-fdw-grouping-path-cost-v3.patch)
download | inline diff:
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 2842,2853 **** estimate_path_cost_size(PlannerInfo *root,
--- 2842,2859 ----
}
else if (IS_UPPER_REL(foreignrel))
{
+ RelOptInfo *outerrel = fpinfo->outerrel;
PgFdwRelationInfo *ofpinfo;
AggClauseCosts aggcosts;
double input_rows;
int numGroupCols;
double numGroups = 1;
+ /* The upper relation should have its outer relation set */
+ Assert(outerrel);
+ /* and that outer relation should have its reltarget set */
+ Assert(outerrel->reltarget);
+
/*
* This cost model is mixture of costing done for sorted and
* hashed aggregates in cost_agg(). We are not sure which
***************
*** 2856,2862 **** estimate_path_cost_size(PlannerInfo *root,
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
--- 2862,2868 ----
* and all finalization and run cost are added in total_cost.
*/
! ofpinfo = (PgFdwRelationInfo *) outerrel->fdw_private;
/* Get rows and width from input rel */
input_rows = ofpinfo->rows;
***************
*** 2909,2919 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
--- 2915,2927 ----
/*-----
* Startup cost includes:
! * 1. Startup cost for underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Cost of performing aggregation, per cost_agg()
*-----
*/
startup_cost = ofpinfo->rel_startup_cost;
+ startup_cost += outerrel->reltarget->cost.startup;
startup_cost += aggcosts.transCost.startup;
startup_cost += aggcosts.transCost.per_tuple * input_rows;
startup_cost += aggcosts.finalCost.startup;
***************
*** 2921,2931 **** estimate_path_cost_size(PlannerInfo *root,
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
--- 2929,2941 ----
/*-----
* Run time cost includes:
! * 1. Run time cost of underneath input relation, adjusted for
! * tlist replacement by apply_scanjoin_target_to_paths()
* 2. Run time cost of performing aggregation, per cost_agg()
*-----
*/
run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost;
+ run_cost += outerrel->reltarget->cost.per_tuple * input_rows;
run_cost += aggcosts.finalCost.per_tuple * numGroups;
run_cost += cpu_tuple_cost * numGroups;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 7113,7118 **** apply_scanjoin_target_to_paths(PlannerInfo *root,
--- 7113,7122 ----
* confused in createplan.c if they don't agree. We must do this now so
* that any append paths made in the next part will use the correct
* pathtarget (cf. create_append_path).
+ *
+ * Note that this is also necessary if GetForeignUpperPaths() gets called
+ * on the final scan/join relation or on any of its children, since the
+ * FDW might look at the rel's target to create ForeignPaths.
*/
rel->reltarget = llast_node(PathTarget, scanjoin_targets);
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: postgres_fdw: another oddity in costing aggregate pushdown paths
2019-02-15 11:19 postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-02-22 14:10 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Antonin Houska <ah@cybertec.at>
2019-02-25 10:59 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-05-08 03:42 ` Re: postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
@ 2019-05-09 09:52 ` Etsuro Fujita <etsuro.fujita@gmail.com>
0 siblings, 0 replies; 5+ messages in thread
From: Etsuro Fujita @ 2019-05-09 09:52 UTC (permalink / raw)
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>; +Cc: Antonin Houska <ah@cybertec.at>; pgsql-hackers
On Wed, May 8, 2019 at 12:45 PM Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> This doesn't get applied cleanly after commit 1d33858406. Here is a
> rebased version of the patch. I also modified the comments a little
> bit. If there are no objections from Antonin or anyone else, I'll
> commit the patch.
Pushed. Thanks for reviewing, Antonin!
Best regards,
Etsuro Fujita
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2019-05-09 09:52 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-02-15 11:19 postgres_fdw: another oddity in costing aggregate pushdown paths Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-02-22 14:10 ` Antonin Houska <ah@cybertec.at>
2019-02-25 10:59 ` Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-05-08 03:42 ` Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
2019-05-09 09:52 ` Etsuro Fujita <etsuro.fujita@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox