Message-ID: <5CD2503B.6010108@lab.ntt.co.jp>
Date: Wed, 08 May 2019 12:42:51 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: postgres_fdw: another oddity in costing aggregate pushdown paths
References: <5C66A056.60007@lab.ntt.co.jp> <29503.1550844622@localhost>
 <5C73CA75.8060005@lab.ntt.co.jp>
In-Reply-To: <5C73CA75.8060005@lab.ntt.co.jp>
Content-Type: multipart/mixed;
 boundary="------------090904060900090805000605"
To: Antonin Houska <ah@cybertec.at>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

This is a multi-part message in MIME format.
--------------090904060900090805000605
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

(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

--------------090904060900090805000605
Content-Type: text/x-patch;
 name="another-fix-postgres-fdw-grouping-path-cost-v3.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="another-fix-postgres-fdw-grouping-path-cost-v3.patch"

*** 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);
  

--------------090904060900090805000605--