On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat
>
> GroupPathExtraData now completely absorbs members from and replaces
> OtherUpperPathExtraData. This means that we have to consider a way to
> pass GroupPathExtraData to FDWs through GetForeignUpperPaths(). I
> haven't tried it in this patch.
>
Initially, I was passing OtherUpperPathExtraData to FDW. But now we need to
pass GroupPathExtraData.
However, since GetForeignUpperPaths() is a generic function for all upper
relations, we might think of renaming this struct to UpperPathExtraData.
Add an UpperRelationKind member to it
Which will be used to distinguish the passed in extra data. But now we only
have extra data for grouping only, I chose not to do that here. But
someone, when needed, may choose this approach.
> With this patch there's a failure in partition_aggregation where the
> patch is creating paths with MergeAppend with GatherMerge underneath.
> I think this is related to the call
> add_paths_to_partial_grouping_rel() when try_parallel_aggregation is
> true. But I didn't investigate it further.
>
I fixed it. We need to pass is_partial_agg instead of
extra->partial_partitionwise_grouping while calling
add_paths_to_partial_grouping_rel() in case of parallelism.
> With those two things remaining I am posting this patch, so that
> Jeevan Chalke can merge this patch into his patches and also merge
> some of his changes related to mine and Robert's changes. Let me know
> if this refactoring looks good.
>
Will rebase my changes tomorrow.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 87ea18c..ed16f7d 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -353,7 +353,7 @@ static void postgresGetForeignUpperPaths(PlannerInfo *root,
UpperRelationKind stage,
RelOptInfo *input_rel,
RelOptInfo *output_rel,
- OtherUpperPathExtraData *extra);
+ GroupPathExtraData *group_extra);
/*
* Helper functions
@@ -429,7 +429,7 @@ static void add_paths_with_pathkeys_for_rel(PlannerInfo *root, RelOptInfo *rel,
static void add_foreign_grouping_paths(PlannerInfo *root,
RelOptInfo *input_rel,
RelOptInfo *grouped_rel,
- OtherUpperPathExtraData *extra);
+ GroupPathExtraData *group_extra);
static void apply_server_options(PgFdwRelationInfo *fpinfo);
static void apply_table_options(PgFdwRelationInfo *fpinfo);
static void merge_fdw_options(PgFdwRelationInfo *fpinfo,
@@ -5235,7 +5235,7 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
static void
postgresGetForeignUpperPaths(PlannerInfo *root, UpperRelationKind stage,
RelOptInfo *input_rel, RelOptInfo *output_rel,
- OtherUpperPathExtraData *extra)
+ GroupPathExtraData *group_extra)
{
PgFdwRelationInfo *fpinfo;
@@ -5255,7 +5255,7 @@ postgresGetForeignUpperPaths(PlannerInfo *root, UpperRelationKind stage,
fpinfo->pushdown_safe = false;
output_rel->fdw_private = fpinfo;
- add_foreign_grouping_paths(root, input_rel, output_rel, extra);
+ add_foreign_grouping_paths(root, input_rel, output_rel, group_extra);
}
/*
@@ -5268,7 +5268,7 @@ postgresGetForeignUpperPaths(PlannerInfo *root, UpperRelationKind stage,
static void
add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
RelOptInfo *grouped_rel,
- OtherUpperPathExtraData *extra)
+ GroupPathExtraData *group_extra)
{
Query *parse = root->parse;
PgFdwRelationInfo *ifpinfo = input_rel->fdw_private;
@@ -5288,16 +5288,16 @@ add_foreign_grouping_paths(PlannerInfo *root, RelOptInfo *input_rel,
* Store passed-in target and havingQual in fpinfo. If its a foreign
* partition, then path target and HAVING quals fetched from the root are
* not correct as Vars within it won't match with this child relation.
- * However, server passed them through extra and thus fetch from it.
+ * However, server passed them through group_extra and thus fetch from it.
*/
- if (extra)
+ if (group_extra)
{
/* Partial aggregates are not supported. */
- if (extra->isPartialAgg)
+ if (group_extra->partial_partitionwise_grouping)
return;
- fpinfo->grouped_target = extra->relTarget;
- fpinfo->havingQual = extra->havingQual;
+ fpinfo->grouped_target = group_extra->target;
+ fpinfo->havingQual = group_extra->havingQual;
}
else
{
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 1611975..9a34bf9 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -109,43 +109,6 @@ typedef struct
int *tleref_to_colnum_map;
} grouping_sets_data;
-/*
- * Struct for extra information passed to create_grouping_paths
- *
- * can_hash is true if hash-based grouping is possible, false otherwise.
- * can_sort is true if sort-based grouping is possible, false otherwise.
- * can_partial_agg is true if partial aggregation is possible, false otherwise.
- * partial_costs_set indicates whether agg_partial_costs and agg_final_costs
- * have valid costs set. Both of those are computed only when partial
- * aggregation is required.
- * agg_partial_costs gives partial aggregation costs.
- * agg_final_costs gives finalization costs.
- */
-typedef struct
-{
- /* Data which remains constant once set. */
- bool can_hash;
- bool can_sort;
- bool can_partial_agg;
- bool partial_costs_set;
- AggClauseCosts agg_partial_costs;
- AggClauseCosts agg_final_costs;
-
- /*
- * Data which depends upon the input and grouping relations and hence may
- * change across partitioning hierarchy
- */
- bool consider_parallel; /* It probably remains same across
- * partitioning hierarchy. But double
- * check.
- */
- bool try_parallel_aggregation;
- bool partitionwise_grouping;
- bool partial_partitionwise_grouping;
- Node *havingQual;
- PathTarget *target;
-} GroupPathExtraData;
-
/* Local functions */
static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind);
static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode);
@@ -3942,8 +3905,7 @@ populate_grouping_rels_with_paths(PlannerInfo *root,
if (extra->try_parallel_aggregation)
add_paths_to_partial_grouping_rel(root, input_rel,
partially_grouped_rel, gd, extra,
- true,
- extra->partial_partitionwise_grouping);
+ true, is_partial_agg);
/*
* Now generate non-parallel paths, partial or full aggregation paths as
diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h
index 1c2b1fc..6d94c73 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -63,7 +63,7 @@ typedef void (*GetForeignUpperPaths_function) (PlannerInfo *root,
UpperRelationKind stage,
RelOptInfo *input_rel,
RelOptInfo *output_rel,
- OtherUpperPathExtraData *extra);
+ GroupPathExtraData *group_extra);
typedef void (*AddForeignUpdateTargets_function) (Query *parsetree,
RangeTblEntry *target_rte,
diff --git a/src/include/nodes/relation.h b/src/include/nodes/relation.h
index 113e835..55edd7c 100644
--- a/src/include/nodes/relation.h
+++ b/src/include/nodes/relation.h
@@ -2296,19 +2296,41 @@ typedef struct JoinPathExtraData
} JoinPathExtraData;
/*
- * Struct for extra information passed to subroutines involving path creation
- * for other upper rels.
+ * Struct for extra information passed to create_grouping_paths
*
- * relTarget is the PathTarget for this upper rel
- * isPartialAgg is true if we are creating a partial aggregation path
- * havingQual is the quals applied to this upper rel
+ * can_hash is true if hash-based grouping is possible, false otherwise.
+ * can_sort is true if sort-based grouping is possible, false otherwise.
+ * can_partial_agg is true if partial aggregation is possible, false otherwise.
+ * partial_costs_set indicates whether agg_partial_costs and agg_final_costs
+ * have valid costs set. Both of those are computed only when partial
+ * aggregation is required.
+ * agg_partial_costs gives partial aggregation costs.
+ * agg_final_costs gives finalization costs.
*/
typedef struct
{
- PathTarget *relTarget;
- bool isPartialAgg;
+ /* Data which remains constant once set. */
+ bool can_hash;
+ bool can_sort;
+ bool can_partial_agg;
+ bool partial_costs_set;
+ AggClauseCosts agg_partial_costs;
+ AggClauseCosts agg_final_costs;
+
+ /*
+ * Data which depends upon the input and grouping relations and hence may
+ * change across partitioning hierarchy
+ */
+ bool consider_parallel; /* It probably remains same across
+ * partitioning hierarchy. But double
+ * check.
+ */
+ bool try_parallel_aggregation;
+ bool partitionwise_grouping;
+ bool partial_partitionwise_grouping;
Node *havingQual;
-} OtherUpperPathExtraData;
+ PathTarget *target;
+} GroupPathExtraData;
/*
* For speed reasons, cost estimation for join paths is performed in two
