On Mon, Jun 18, 2018 at 9:27 PM, Andres Freund <and...@anarazel.de> wrote:
> On 2018-06-18 17:10:12 +0530, Jeevan Chalke wrote:
> > On Mon, Jun 18, 2018 at 5:02 PM, Rajkumar Raghuwanshi <
> > rajkumar.raghuwan...@enterprisedb.com> wrote:
> >
> > > Hi,
> > >
> > > Below test case crashed, when set enable_partitionwise_aggregate to
> true.
> > >
> >
> > I will have a look over this.
>
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.
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. 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, I may be wrong though. And if that's the case
then newly added Assert() in add_paths_to_append_rel() will fail anyway to
catch any programming error in that code path.
Attached patch fixing the crash and also added a simple test-coverage for
that.
Let me know if I missed any.
Thanks
> >
> > Thanks for reporting.
>
> I've added an v11 open-items entry.
>
> - Andres
>
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
From e969ef65e7ccebaa4d068a09ac9db812ee0793ab Mon Sep 17 00:00:00 2001
From: Jeevan Chalke <jeevan.cha...@enterprisedb.com>
Date: Tue, 19 Jun 2018 10:44:33 +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.
Jeevan Chalke
---
src/backend/optimizer/path/allpaths.c | 3 ++
src/backend/optimizer/plan/planner.c | 6 ++--
src/test/regress/expected/partition_aggregate.out | 40 +++++++++++++++++++++++
src/test/regress/sql/partition_aggregate.sql | 14 ++++++++
4 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 477b9f7..4d8e7a0 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 few live child rels. */
+ Assert(live_childrels != NIL);
+
/*
* AppendPath generated for partitioned tables must record the RT indexes
* of partitioned tables that are direct or indirect children of this
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 67a2c7a..5e75f7f 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -7114,8 +7114,10 @@ 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.
+ * However, if there are no live children, then we cannot create any append
+ * path.
*/
- if (partially_grouped_rel)
+ if (partially_grouped_rel && partially_grouped_live_children != NIL)
{
add_paths_to_append_rel(root, partially_grouped_rel,
partially_grouped_live_children);
@@ -7129,7 +7131,7 @@ create_partitionwise_grouping_paths(PlannerInfo *root,
}
/* If possible, create append paths for fully grouped children. */
- if (patype == PARTITIONWISE_AGGREGATE_FULL)
+ if (patype == PARTITIONWISE_AGGREGATE_FULL && grouped_live_children != NIL)
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..bfde035 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -1394,3 +1394,43 @@ 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 partition tables has parallel_workers = 0 but not the parent
+ALTER TABLE pagg_tab_para_p1 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p2 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p3 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+-- 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..a1aacf5 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -294,3 +294,17 @@ 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 partition tables has parallel_workers = 0 but not the parent
+ALTER TABLE pagg_tab_para_p1 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p2 SET (parallel_workers = 0);
+ALTER TABLE pagg_tab_para_p3 SET (parallel_workers = 0);
+ANALYZE pagg_tab_para;
+
+-- 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
