From f6569b886cd4db9b7bb1de65b530a692dd6dc734 Mon Sep 17 00:00:00 2001 From: Jeevan Chalke Date: Thu, 29 Mar 2018 18:04:05 +0530 Subject: [PATCH 3/3] Add agg_costs in GroupPathExtraData so that FDW can use it. We do pass GroupPathExtraData to postgres_fdw, thus add agg_costs too in the struct so that we can use these cost estimates in postgres_fdw too avoiding recalculations. In passing, updated couple of function signatures where we were passing both agg_costs and "extra" so that it just passes the "extra" parameter. --- contrib/postgres_fdw/postgres_fdw.c | 43 +++++++++++++----------------------- src/backend/optimizer/plan/planner.c | 31 ++++++++++++-------------- src/include/nodes/relation.h | 3 +++ 3 files changed, 32 insertions(+), 45 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index dbebbda..6d85569 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -362,6 +362,7 @@ static void estimate_path_cost_size(PlannerInfo *root, RelOptInfo *foreignrel, List *param_join_conds, List *pathkeys, + const AggClauseCosts *agg_costs, double *p_rows, int *p_width, Cost *p_startup_cost, Cost *p_total_cost); static void get_remote_estimate(const char *sql, @@ -614,7 +615,7 @@ 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, NIL, + estimate_path_cost_size(root, baserel, NIL, NIL, NULL, &fpinfo->rows, &fpinfo->width, &fpinfo->startup_cost, &fpinfo->total_cost); @@ -645,7 +646,7 @@ 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, NIL, + estimate_path_cost_size(root, baserel, NIL, NIL, NULL, &fpinfo->rows, &fpinfo->width, &fpinfo->startup_cost, &fpinfo->total_cost); } @@ -1076,7 +1077,7 @@ postgresGetForeignPaths(PlannerInfo *root, /* Get a cost estimate from the remote */ estimate_path_cost_size(root, baserel, - param_info->ppi_clauses, NIL, + param_info->ppi_clauses, NIL, NULL, &rows, &width, &startup_cost, &total_cost); @@ -2588,6 +2589,7 @@ estimate_path_cost_size(PlannerInfo *root, RelOptInfo *foreignrel, List *param_join_conds, List *pathkeys, + const AggClauseCosts *agg_costs, double *p_rows, int *p_width, Cost *p_startup_cost, Cost *p_total_cost) { @@ -2779,7 +2781,6 @@ estimate_path_cost_size(PlannerInfo *root, { PgFdwRelationInfo *ofpinfo; PathTarget *ptarget = foreignrel->reltarget; - AggClauseCosts aggcosts; double input_rows; int numGroupCols; double numGroups = 1; @@ -2808,23 +2809,6 @@ estimate_path_cost_size(PlannerInfo *root, input_rows = ofpinfo->rows; width = ofpinfo->width; - /* Collect statistics about aggregates for estimating costs. */ - MemSet(&aggcosts, 0, sizeof(AggClauseCosts)); - if (root->parse->hasAggs) - { - get_agg_clause_costs(root, (Node *) fpinfo->grouped_tlist, - AGGSPLIT_SIMPLE, &aggcosts); - - /* - * Cost of aggregates within the HAVING qual will remain same - * for both parent and a child. Thus, in case of child upper - * rel, it is not necessary to consider translated HAVING qual - * here. Hence use having qual from the root itself. - */ - get_agg_clause_costs(root, (Node *) root->parse->havingQual, - AGGSPLIT_SIMPLE, &aggcosts); - } - /* Get number of grouping columns and possible number of groups */ numGroupCols = list_length(root->parse->groupClause); numGroups = estimate_num_groups(root, @@ -2838,6 +2822,9 @@ estimate_path_cost_size(PlannerInfo *root, */ rows = retrieved_rows = numGroups; + /* agg_costs should not be null while grouping. */ + Assert(agg_costs); + /*----- * Startup cost includes: * 1. Startup cost for underneath input * relation @@ -2846,8 +2833,8 @@ estimate_path_cost_size(PlannerInfo *root, *----- */ startup_cost = ofpinfo->rel_startup_cost; - startup_cost += aggcosts.transCost.startup; - startup_cost += aggcosts.transCost.per_tuple * input_rows; + startup_cost += agg_costs->transCost.startup; + startup_cost += agg_costs->transCost.per_tuple * input_rows; startup_cost += (cpu_operator_cost * numGroupCols) * input_rows; startup_cost += ptarget->cost.startup; @@ -2859,7 +2846,7 @@ estimate_path_cost_size(PlannerInfo *root, *----- */ run_cost = ofpinfo->rel_total_cost - ofpinfo->rel_startup_cost; - run_cost += aggcosts.finalCost * numGroups; + run_cost += agg_costs->finalCost * numGroups; run_cost += cpu_tuple_cost * numGroups; run_cost += ptarget->cost.per_tuple * numGroups; } @@ -4761,7 +4748,7 @@ add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel, List *useful_pathkeys = lfirst(lc); Path *sorted_epq_path; - estimate_path_cost_size(root, rel, NIL, useful_pathkeys, + estimate_path_cost_size(root, rel, NIL, useful_pathkeys, NULL, &rows, &width, &startup_cost, &total_cost); /* @@ -4993,7 +4980,7 @@ postgresGetForeignJoinPaths(PlannerInfo *root, extra->sjinfo); /* Estimate costs for bare join relation */ - estimate_path_cost_size(root, joinrel, NIL, NIL, &rows, + estimate_path_cost_size(root, joinrel, NIL, NIL, NULL, &rows, &width, &startup_cost, &total_cost); /* Now update this information in the joinrel */ joinrel->rows = rows; @@ -5325,8 +5312,8 @@ add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, return; /* Estimate the cost of push down */ - estimate_path_cost_size(root, grouped_rel, NIL, NIL, &rows, - &width, &startup_cost, &total_cost); + estimate_path_cost_size(root, grouped_rel, NIL, NIL, extra->agg_costs, + &rows, &width, &startup_cost, &total_cost); /* Now update this information in the fpinfo */ fpinfo->rows = rows; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index c5fef61..32b2dc8 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -152,7 +152,6 @@ static RelOptInfo *make_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, static void create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, - const AggClauseCosts *agg_costs, grouping_sets_data *gd, GroupPathExtraData *extra, RelOptInfo **partially_grouped_rel_p); @@ -207,7 +206,6 @@ static void adjust_paths_for_srfs(PlannerInfo *root, RelOptInfo *rel, static void add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, RelOptInfo *partially_grouped_rel, - const AggClauseCosts *agg_costs, grouping_sets_data *gd, double dNumGroups, GroupPathExtraData *extra); @@ -229,7 +227,6 @@ static void create_partitionwise_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, RelOptInfo *partially_grouped_rel, - const AggClauseCosts *agg_costs, grouping_sets_data *gd, PartitionwiseAggregateType patype, GroupPathExtraData *extra); @@ -3689,6 +3686,12 @@ create_grouping_paths(PlannerInfo *root, GroupPathExtraData extra; /* + * Set agg_costs into the extra so that other routines like FDW can use + * it directly rather than computing it again. + */ + extra.agg_costs = agg_costs; + + /* * Determine whether it's possible to perform sort-based * implementations of grouping. (Note that if groupClause is empty, * grouping_is_sortable() is trivially true, and all the @@ -3751,9 +3754,8 @@ create_grouping_paths(PlannerInfo *root, else extra.patype = PARTITIONWISE_AGGREGATE_NONE; - create_ordinary_grouping_paths(root, input_rel, grouped_rel, - agg_costs, gd, &extra, - &partially_grouped_rel); + create_ordinary_grouping_paths(root, input_rel, grouped_rel, gd, + &extra, &partially_grouped_rel); } set_cheapest(grouped_rel); @@ -3907,9 +3909,7 @@ create_degenerate_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, */ static void create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, - RelOptInfo *grouped_rel, - const AggClauseCosts *agg_costs, - grouping_sets_data *gd, + RelOptInfo *grouped_rel, grouping_sets_data *gd, GroupPathExtraData *extra, RelOptInfo **partially_grouped_rel_p) { @@ -3979,8 +3979,8 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, /* Apply partitionwise aggregation technique, if possible. */ if (patype != PARTITIONWISE_AGGREGATE_NONE) create_partitionwise_grouping_paths(root, input_rel, grouped_rel, - partially_grouped_rel, agg_costs, - gd, patype, extra); + partially_grouped_rel, gd, patype, + extra); /* If we are doing partial aggregation only, return. */ if (extra->patype == PARTITIONWISE_AGGREGATE_PARTIAL) @@ -4010,8 +4010,7 @@ create_ordinary_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, /* Build final grouping paths */ add_paths_to_grouping_rel(root, input_rel, grouped_rel, - partially_grouped_rel, agg_costs, gd, - dNumGroups, extra); + partially_grouped_rel, gd, dNumGroups, extra); /* Give a helpful error if we failed to find any implementation */ if (grouped_rel->pathlist == NIL) @@ -6189,7 +6188,6 @@ static void add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, RelOptInfo *partially_grouped_rel, - const AggClauseCosts *agg_costs, grouping_sets_data *gd, double dNumGroups, GroupPathExtraData *extra) { @@ -6199,6 +6197,7 @@ add_paths_to_grouping_rel(PlannerInfo *root, RelOptInfo *input_rel, bool can_hash = (extra->flags & GROUPING_CAN_USE_HASH) != 0; bool can_sort = (extra->flags & GROUPING_CAN_USE_SORT) != 0; List *havingQual = (List *) extra->havingQual; + const AggClauseCosts *agg_costs = extra->agg_costs; AggClauseCosts *agg_final_costs = &extra->agg_final_costs; if (can_sort) @@ -6906,7 +6905,6 @@ create_partitionwise_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel, RelOptInfo *grouped_rel, RelOptInfo *partially_grouped_rel, - const AggClauseCosts *agg_costs, grouping_sets_data *gd, PartitionwiseAggregateType patype, GroupPathExtraData *extra) @@ -7006,8 +7004,7 @@ create_partitionwise_grouping_paths(PlannerInfo *root, /* Create grouping paths for this child relation. */ create_ordinary_grouping_paths(root, child_input_rel, - child_grouped_rel, - agg_costs, gd, &child_extra, + child_grouped_rel, gd, &child_extra, &child_partially_grouped_rel); if (child_partially_grouped_rel) diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h index 2b4f773..4ed3eb7 100644 --- a/src/include/nodes/relation.h +++ b/src/include/nodes/relation.h @@ -2335,6 +2335,8 @@ typedef enum /* * Struct for extra information passed to subroutines of create_grouping_paths * + * agg_costs gives cost info about all aggregates in query (in AGGSPLIT_SIMPLE + * mode) * flags indicating what kinds of grouping are possible. * partial_costs_set is true if the agg_partial_costs and agg_final_costs * have been initialized. @@ -2348,6 +2350,7 @@ typedef enum typedef struct { /* Data which remains constant once set. */ + const AggClauseCosts *agg_costs; int flags; bool partial_costs_set; AggClauseCosts agg_partial_costs; -- 1.8.3.1