Message-ID: <5C485173.5040503@lab.ntt.co.jp>
Date: Wed, 23 Jan 2019 20:35:15 +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> <5C25DE9B.5010000@lab.ntt.co.jp>
 <5C2F449C.7010103@lab.ntt.co.jp>
In-Reply-To: <5C2F449C.7010103@lab.ntt.co.jp>
Content-Type: multipart/mixed;
 boundary="------------090903030608070704060301"
To: Pg Hackers <pgsql-hackers@postgresql.org>
Precedence: bulk

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

(2019/01/04 20:33), Etsuro Fujita wrote:
> Here is a new version of the patch.

Attached is an updated version of the patch.

Changes:
* Fixed a stupid bug in the case when use_remote_estimate
* Fixed a typo in a comment I added
* Modified comments I added a little bit further
* Added the commit message

If there are no objections, I'll push that.

Best regards,
Etsuro Fujita

--------------090903030608070704060301
Content-Type: text/x-patch;
 name="0001-postgres_fdw-Account-for-tlist-eval-costs-in-estimat.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-postgres_fdw-Account-for-tlist-eval-costs-in-estimat.pa";
 filename*1="tch"

From 3ff65a6f05a330140bf5de94fbcf6ab2a04f040b Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <efujita@postgresql.org>
Date: Wed, 23 Jan 2019 20:30:30 +0900
Subject: [PATCH] postgres_fdw: Account for tlist eval costs in
 estimate_path_cost_size()

Previously, estimate_path_cost_size() didn't account for tlist eval
costs, except when costing a foreign-grouping path using local
statistics, but such costs should be accounted for when costing that path
using remote estimates, because some of the tlist expressions might be
evaluated locally.  Also, such costs should be accounted for in the case
of a foreign-scan or foreign-join path, because currently, postgres_fdw
evaluates PlaceHolderVars (if any) locally.

This also fixes an oversight in my commit
f8f6e44676ef38fee7a5bbe4f256a34ea7799ac1.

Like that commit, apply this to HEAD only to avoid destabilizing existing
plan choices.

Author: Etsuro Fujita
Discussion: https://postgr.es/m/5BFD3EAD.2060301%40lab.ntt.co.jp
---
 contrib/postgres_fdw/postgres_fdw.c | 41 +++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d85a83abe9..f460fd2ec4 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2499,6 +2499,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
@@ -2576,6 +2579,24 @@ 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 tlist eval cost for each output row.  In case of an
+		 * aggregate, some of the tlist expressions such as grouping
+		 * exressions will be evaluated remotely, so adjust the costs.
+		 */
+		startup_cost += foreignrel->reltarget->cost.startup;
+		total_cost += foreignrel->reltarget->cost.startup;
+		total_cost += foreignrel->reltarget->cost.per_tuple * rows;
+		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.startup;
+			total_cost -= tlist_cost.per_tuple * rows;
+		}
 	}
 	else
 	{
@@ -2674,19 +2695,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 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
@@ -2750,26 +2771,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)
@@ -2784,6 +2801,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 tlist eval cost for each output row */
+			startup_cost += foreignrel->reltarget->cost.startup;
+			run_cost += foreignrel->reltarget->cost.per_tuple * rows;
 		}
 		else
 		{
@@ -2802,6 +2823,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 in tlist eval cost for each output row */
+			startup_cost += foreignrel->reltarget->cost.startup;
+			run_cost += foreignrel->reltarget->cost.per_tuple * rows;
 		}
 
 		/*
-- 
2.19.2


--------------090903030608070704060301--