On Wed, Jun 20, 2018 at 7:11 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
> On Tue, Jun 19, 2018 at 2:13 PM, Jeevan Chalke
> <jeevan.cha...@enterprisedb.com> wrote:
> >
> >
> > In the reported testcase, parallel_workers is set to 0 for all partition
> > (child) relations. Which means partial parallel paths are not possible
> for
> > child rels. However, the parent can have partial parallel paths. Thus,
> when
> > we have a full partitionwise aggregate possible (as the group by clause
> > matches with the partition key), we end-up in a situation where we do
> create
> > a partially_grouped_rel for the parent but there won't be any
> > partially_grouped_live_children.
> >
> > The current code was calling add_paths_to_append_rel() without making
> sure
> > of any live children present or not (sorry, it was my fault). This
> causes an
> > Assertion failure in add_paths_to_append_rel() as this function assumes
> that
> > it will have non-NIL live_childrels list.
> >
>
> Thanks for the detailed explanation.
>
> > I have fixed partitionwise aggregate code which is calling
> > add_paths_to_append_rel() by checking the live children list correctly.
> And
> > for robustness, I have also added an Assert() in
> add_paths_to_append_rel().
> >
> > I have verified the callers of add_paths_to_append_rel() and except one,
> all
> > are calling it by making sure that they have a non-NIL live children
> list.
>
> I actually thought about moving if(live_childrel != NIL) test inside
> this function, but then every caller is doing something different when
> that condition occurs. So doesn't help much.
>
> > The one which is calling add_paths_to_append_rel() directly is
> > set_append_rel_pathlist(). And I think, at that place, we will never
> have an
> > empty live children list,
>
> Yes, I think so too. That's because set_append_rel_size() should have
> marked such a parent as dummy and subsequent set_rel_pathlist() would
> not create any paths for it.
>
> Here are some review comments.
>
> + /* We should end-up here only when we have few live child rels. */
>
> I think the right wording is " ... we have at least one child." I was
> actually
> thinking if we should just return from here when live_children == NIL. But
> then
> we will loose an opportunity to catch a bug earlier than set_cheapest().
>
Done.
>
> + * However, if there are no live children, then we cannot create any
> append
> + * path.
>
> I think "no live children" is kind of misleading here. We usually use that
> term
> to mean at least one non-dummy child. But in this case there is no child at
> all, so we might want to better describe this situation. May be also
> explain
> when this condition can happen.
>
Done. Tried re-phrasing it. Please check.
> + if (patype == PARTITIONWISE_AGGREGATE_FULL && grouped_live_children
> != NIL)
>
> I think for this to happen, the parent grouped relation and the underlying
> scan
> itself should be dummy. So, would an Assert be better? I think we have
> discussed this earlier, but I can not spot the mail.
>
Yep, Assert() will be better. Done.
>
> +-- Test when partition tables has parallel_workers = 0 but not the parent
>
> Better be worded as "Test when parent can produce parallel paths but not
> any of
> its children". parallel_worker = 0 is just a means to test that. Although
> the
> EXPLAIN output below doesn't really reflect that parent can have parallel
> plans. Is it possible to create a scenario to show that.
>
Added test.
>
> I see that you have posted some newer versions of this patch, but I
> think those still need to address these comments.
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>
Thanks
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
From dd9951b3fd92a61d1b9fb02019a3af5b4d8e3b93 Mon Sep 17 00:00:00 2001
From: Jeevan Chalke <jeevan.cha...@enterprisedb.com>
Date: Wed, 20 Jun 2018 18:47:12 +0530
Subject: [PATCH] Make sure that we have live children before we append them.
In create_partitionwise_grouping_paths(), we were calling
add_paths_to_append_rel() on empty live children which causing
an Assertion failure inside it. Thus, prevent calling
add_paths_to_append_rel() when there are no live children.
In passing, add an Assert() in add_paths_to_append_rel() to
check that it receives a valid live children list.
Also, it is very well possible that we could not have a
partially_grouped_rel for some of the children for which aggregation
in partial is not feasible. In such cases, we will end up having
fewer children in the partially_grouped_live_children list. Don't
create append rel in that case.
Jeevan Chalke
---
src/backend/optimizer/path/allpaths.c | 3 +
src/backend/optimizer/plan/planner.c | 25 +++++-
src/test/regress/expected/partition_aggregate.out | 105 ++++++++++++++++++++++
src/test/regress/sql/partition_aggregate.sql | 24 +++++
4 files changed, 155 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 3ada379..9f3d725 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1391,6 +1391,9 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
bool build_partitioned_rels = false;
double partial_rows = -1;
+ /* We should end-up here only when we have at least one child. */
+ Assert(live_childrels != NIL);
+
/* If appropriate, consider parallel append */
pa_subpaths_valid = enable_parallel_append && rel->consider_parallel;
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 67a2c7a..9459b3c 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7011,12 +7011,14 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
int cnt_parts;
List *grouped_live_children = NIL;
List *partially_grouped_live_children = NIL;
+ int dummy_children_cnt;
PathTarget *target = grouped_rel->reltarget;
Assert(patype != PARTITIONWISE_AGGREGATE_NONE);
Assert(patype != PARTITIONWISE_AGGREGATE_PARTIAL ||
partially_grouped_rel != NULL);
+ dummy_children_cnt = 0;
/* Add paths for partitionwise aggregation/grouping. */
for (cnt_parts = 0; cnt_parts < nparts; cnt_parts++)
{
@@ -7075,6 +7077,8 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
if (IS_DUMMY_REL(child_input_rel))
{
mark_dummy_rel(child_grouped_rel);
+ /* Count dummy children. */
+ dummy_children_cnt++;
continue;
}
@@ -7114,8 +7118,16 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
* partitionwise aggregation, we might have paths in the partial_pathlist
* if parallel aggregation is possible. For partial partitionwise
* aggregation, we may have paths in both pathlist and partial_pathlist.
- */
- if (partially_grouped_rel)
+ *
+ * However, it is very well possible that we could not have a
+ * partially_grouped_rel for some of (or all) the children for which
+ * aggregation in partial is not feasible. In that case, we might have
+ * no child or fewer children to append. In case of no child, we cannot
+ * create any append path. Whereas when there are fewer children, then
+ * the children count should be number of partitions - dummy children.
+ */
+ if (partially_grouped_rel && partially_grouped_live_children != NIL &&
+ list_length(partially_grouped_live_children) == nparts - dummy_children_cnt)
{
add_paths_to_append_rel(root, partially_grouped_rel,
partially_grouped_live_children);
@@ -7130,7 +7142,16 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
/* If possible, create append paths for fully grouped children. */
if (patype == PARTITIONWISE_AGGREGATE_FULL)
+ {
+ /*
+ * In case of full partitionwise aggregation we should have grouped rel
+ * for each non-dummy child.
+ */
+ Assert(grouped_live_children != NIL &&
+ list_length(grouped_live_children) == nparts - dummy_children_cnt);
+
add_paths_to_append_rel(root, grouped_rel, grouped_live_children);
+ }
}
/*
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index 76a8209..498592f 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1394,3 +1394,108 @@ SELECT y, sum(x), avg(x), count(*) FROM pagg_tab_para GROUP BY y HAVING avg(x) <
11 | 16500 | 11.0000000000000000 | 1500
(4 rows)
+-- Test when parent can produce parallel paths but not any (or some) of its children
+ALTER TABLE pagg_tab_para_p1 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p2 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+ Sort Key: pagg_tab_para_p1.x, (sum(pagg_tab_para_p1.y)), (avg(pagg_tab_para_p1.y))
+ -> Finalize GroupAggregate
+ Group Key: pagg_tab_para_p1.x
+ Filter: (avg(pagg_tab_para_p1.y) < '7'::numeric)
+ -> Gather Merge
+ Workers Planned: 2
+ -> Sort
+ Sort Key: pagg_tab_para_p1.x
+ -> Partial HashAggregate
+ Group Key: pagg_tab_para_p1.x
+ -> Parallel Append
+ -> Seq Scan on pagg_tab_para_p1
+ -> Seq Scan on pagg_tab_para_p2
+ -> Parallel Seq Scan on pagg_tab_para_p3
+(15 rows)
+
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ x | sum | avg | count
+----+------+--------------------+-------
+ 0 | 5000 | 5.0000000000000000 | 1000
+ 1 | 6000 | 6.0000000000000000 | 1000
+ 10 | 5000 | 5.0000000000000000 | 1000
+ 11 | 6000 | 6.0000000000000000 | 1000
+ 20 | 5000 | 5.0000000000000000 | 1000
+ 21 | 6000 | 6.0000000000000000 | 1000
+(6 rows)
+
+ALTER TABLE pagg_tab_para_p3 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+ Sort Key: pagg_tab_para_p1.x, (sum(pagg_tab_para_p1.y)), (avg(pagg_tab_para_p1.y))
+ -> Finalize GroupAggregate
+ Group Key: pagg_tab_para_p1.x
+ Filter: (avg(pagg_tab_para_p1.y) < '7'::numeric)
+ -> Gather Merge
+ Workers Planned: 2
+ -> Sort
+ Sort Key: pagg_tab_para_p1.x
+ -> Partial HashAggregate
+ Group Key: pagg_tab_para_p1.x
+ -> Parallel Append
+ -> Seq Scan on pagg_tab_para_p1
+ -> Seq Scan on pagg_tab_para_p2
+ -> Seq Scan on pagg_tab_para_p3
+(15 rows)
+
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ x | sum | avg | count
+----+------+--------------------+-------
+ 0 | 5000 | 5.0000000000000000 | 1000
+ 1 | 6000 | 6.0000000000000000 | 1000
+ 10 | 5000 | 5.0000000000000000 | 1000
+ 11 | 6000 | 6.0000000000000000 | 1000
+ 20 | 5000 | 5.0000000000000000 | 1000
+ 21 | 6000 | 6.0000000000000000 | 1000
+(6 rows)
+
+-- Reset parallelism parameters to get partitionwise aggregation plan.
+RESET min_parallel_table_scan_size;
+RESET parallel_setup_cost;
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+ Sort Key: pagg_tab_para_p1.x, (sum(pagg_tab_para_p1.y)), (avg(pagg_tab_para_p1.y))
+ -> Append
+ -> HashAggregate
+ Group Key: pagg_tab_para_p1.x
+ Filter: (avg(pagg_tab_para_p1.y) < '7'::numeric)
+ -> Seq Scan on pagg_tab_para_p1
+ -> HashAggregate
+ Group Key: pagg_tab_para_p2.x
+ Filter: (avg(pagg_tab_para_p2.y) < '7'::numeric)
+ -> Seq Scan on pagg_tab_para_p2
+ -> HashAggregate
+ Group Key: pagg_tab_para_p3.x
+ Filter: (avg(pagg_tab_para_p3.y) < '7'::numeric)
+ -> Seq Scan on pagg_tab_para_p3
+(15 rows)
+
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+ x | sum | avg | count
+----+------+--------------------+-------
+ 0 | 5000 | 5.0000000000000000 | 1000
+ 1 | 6000 | 6.0000000000000000 | 1000
+ 10 | 5000 | 5.0000000000000000 | 1000
+ 11 | 6000 | 6.0000000000000000 | 1000
+ 20 | 5000 | 5.0000000000000000 | 1000
+ 21 | 6000 | 6.0000000000000000 | 1000
+(6 rows)
+
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index c60d7d2..53654f3 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -294,3 +294,27 @@ SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) <
EXPLAIN (COSTS OFF)
SELECT y, sum(x), avg(x), count(*) FROM pagg_tab_para GROUP BY y HAVING avg(x) < 12 ORDER BY 1, 2, 3;
SELECT y, sum(x), avg(x), count(*) FROM pagg_tab_para GROUP BY y HAVING avg(x) < 12 ORDER BY 1, 2, 3;
+
+-- Test when parent can produce parallel paths but not any (or some) of its children
+ALTER TABLE pagg_tab_para_p1 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p2 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+
+ALTER TABLE pagg_tab_para_p3 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+
+-- Reset parallelism parameters to get partitionwise aggregation plan.
+RESET min_parallel_table_scan_size;
+RESET parallel_setup_cost;
+
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x HAVING avg(y) < 7 ORDER BY 1, 2, 3;
--
1.8.3.1
