Message-ID: <5C25DE9B.5010000@lab.ntt.co.jp>
Date: Fri, 28 Dec 2018 17:28:11 +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: oddity in costing aggregate pushdown paths
References: <5BFD3EAD.2060301@lab.ntt.co.jp> <5BFE1BDE.7010306@lab.ntt.co.jp>
 <5C010821.7050102@lab.ntt.co.jp> <5C051180.7090804@lab.ntt.co.jp>
 <5C0639B7.1080006@lab.ntt.co.jp>
In-Reply-To: <5C0639B7.1080006@lab.ntt.co.jp>
Content-Type: multipart/mixed;
 boundary="------------040505050602040000030100"
To: Pg Hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

This is a multi-part message in MIME format.
--------------040505050602040000030100
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit

(2018/12/04 17:24), Etsuro Fujita wrote:
> (2018/12/03 20:20), Etsuro Fujita wrote:
>> (2018/11/30 18:51), Etsuro Fujita wrote:
>>> (2018/11/28 13:38), Etsuro Fujita wrote:
>>>> BTW another thing I noticed is this comment on costing aggregate
>>>> pushdown paths using local statistics in estimate_path_cost_size:
>>>>
>>>>                  * Also, core does not care about costing HAVING expressions and
>>>>                  * adding that to the costs.  So similarly, here too we are not
>>>>                  * considering remote and local conditions for costing.
>>>>
>>>> I think this was true when aggregate pushdown went in, but isn't anymore
>>>> because of commit 7b6c07547190f056b0464098bb5a2247129d7aa2.  So we
>>>> should update estimate_path_cost_size so that it accounts for the
>>>> selectivity and cost of the HAVING expressions as well?
>>>
>>> There seems to be no objections, I updated the patch as such.  Attached
>>> is an updated version of the patch.
>>
>> I revised some comments a bit and added the commit message.  Attached is
>> an updated patch.  If there are no objections, I'll apply this to HEAD only.
> 
> Done after fixing typos in the commit message.

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.

Best regards,
Etsuro Fujita

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

*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 2937,2943 **** estimate_path_cost_size(PlannerInfo *root,
  			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)
--- 2937,2943 ----
  			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 * rows;
  
  			/* Accout for the eval cost of HAVING quals, if any */
  			if (root->parse->havingQual)

--------------040505050602040000030100--