On Tue, Apr 10, 2018 at 11:14 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke
> <jeevan.cha...@enterprisedb.com> wrote:
> > I actually wanted to have rel->consider_parallel in the condition (yes,
> for
> > additional safety) as we are adding a partial path into rel. But then
> > observed that it is same as that of final_rel->consider_parallel and thus
> > used it along with other condition.
> >
> > I have observed at many places that we do check consider_parallel flag
> > before adding a partial path to it. Thus for consistency added here too,
> but
> > yes, it just adds an additional safety here.
>
> Thanks to Andreas for reporting this issue and to Jeevan for fixing
> it. My commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 seems clearly
> to blame.
>
> The change to set_subquery_pathlist() looks correct, but can we add a
> simple test case?
I have tried adding simple testcase in this version of the patch. This test
hits the Assertion added in add_partial_path() like you have tried.
> Right now if I keep the new Assert() in
> add_partial_path() and leave out the rest of the changes, the
> regression tests still pass. That's not so good. Also, I think I
> would be inclined to wrap the if-statement around the rest of the
> function instead of returning early.
>
OK.
Wrapped if block instead of returning mid-way.
>
> The new Assert() in add_partial_path() is an excellent idea. I had
> the same thought before, but I didn't do anything about it. That was
> a bad idea; your plan is better. In fact, I suggest an additional
> Assert() that any relation to which we're adding a partial path is
> marked consider_parallel, like this:
>
> + /* Path to be added must be parallel safe. */
> + Assert(new_path->parallel_safe);
> +
> + /* Relation should be OK for parallelism, too. */
> + Assert(parent_rel->consider_parallel);
>
Yep.
Added this new one too.
>
> Regarding recurse_set_operations, since the rel->consider_parallel is
> always the same as final_rel->consider_parallel there's really no
> value in testing it. If it were possible for rel->consider_parallel
> to be false even when final_rel->consider_parallel were true then the
> test would be necessary for correctness. That might or might not
> happen in the future, so I guess it wouldn't hurt to include this for
> future-proofing,
In that case, we should have rel in a condition rather than final_rel as we
are adding a path into rel. For future-proofing added check on
lateral_relids too.
> but it's not actually a bug fix, so it also wouldn't
> hurt to leave it out. I could go either way, I guess.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
From 708026f34f6a5d087660930f9cb05aa3e08c130b Mon Sep 17 00:00:00 2001
From: Jeevan Chalke <jeevan.cha...@enterprisedb.com>
Date: Wed, 11 Apr 2018 14:08:13 +0530
Subject: [PATCH] Add partial path only when rel's consider_parallel is true.
Otherwise, it will result in an assertion failure later in the
create_gather_path().
---
src/backend/optimizer/path/allpaths.c | 41 +++++++++++++++------------
src/backend/optimizer/prep/prepunion.c | 3 +-
src/backend/optimizer/util/pathnode.c | 6 ++++
src/test/regress/expected/select_parallel.out | 19 +++++++++++++
src/test/regress/sql/select_parallel.sql | 6 ++++
5 files changed, 56 insertions(+), 19 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 3ba3f87..c3d0634 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2242,26 +2242,31 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
pathkeys, required_outer));
}
- /* If consider_parallel is false, there should be no partial paths. */
- Assert(sub_final_rel->consider_parallel ||
- sub_final_rel->partial_pathlist == NIL);
-
- /* Same for partial paths. */
- foreach(lc, sub_final_rel->partial_pathlist)
+ /* If outer rel allows parallelism, do same for partial paths. */
+ if (rel->consider_parallel && bms_is_empty(required_outer))
{
- Path *subpath = (Path *) lfirst(lc);
- List *pathkeys;
-
- /* Convert subpath's pathkeys to outer representation */
- pathkeys = convert_subquery_pathkeys(root,
- rel,
- subpath->pathkeys,
- make_tlist_from_pathtarget(subpath->pathtarget));
+ /* If consider_parallel is false, there should be no partial paths. */
+ Assert(sub_final_rel->consider_parallel ||
+ sub_final_rel->partial_pathlist == NIL);
- /* Generate outer path using this subpath */
- add_partial_path(rel, (Path *)
- create_subqueryscan_path(root, rel, subpath,
- pathkeys, required_outer));
+ /* Same for partial paths. */
+ foreach(lc, sub_final_rel->partial_pathlist)
+ {
+ Path *subpath = (Path *) lfirst(lc);
+ List *pathkeys;
+
+ /* Convert subpath's pathkeys to outer representation */
+ pathkeys = convert_subquery_pathkeys(root,
+ rel,
+ subpath->pathkeys,
+ make_tlist_from_pathtarget(subpath->pathtarget));
+
+ /* Generate outer path using this subpath */
+ add_partial_path(rel, (Path *)
+ create_subqueryscan_path(root, rel, subpath,
+ pathkeys,
+ required_outer));
+ }
}
}
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 8d86e98..61d0770 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -330,7 +330,8 @@ recurse_set_operations(Node *setOp, PlannerInfo *root,
* to build a partial path for this relation. But there's no point in
* considering any path but the cheapest.
*/
- if (final_rel->partial_pathlist != NIL)
+ if (rel->consider_parallel && bms_is_empty(rel->lateral_relids) &&
+ final_rel->partial_pathlist != NIL)
{
Path *partial_subpath;
Path *partial_path;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index bd9442c..561466c 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -770,6 +770,12 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path)
/* Check for query cancel. */
CHECK_FOR_INTERRUPTS();
+ /* Path to be added must be parallel safe. */
+ Assert(new_path->parallel_safe);
+
+ /* Relation should be OK for parallelism, too. */
+ Assert(parent_rel->consider_parallel);
+
/*
* As in add_path, throw out any paths which are dominated by the new
* path, but throw out the new path if some existing path dominates it.
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index fb209de..a07cd50 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -955,4 +955,23 @@ ORDER BY 1, 2, 3;
------------------------------+---------------------------+-------------+--------------
(0 rows)
+-- test interation between subquery and partial_paths
+SET LOCAL min_parallel_table_scan_size TO 0;
+CREATE VIEW tenk1_vw_sec WITH (security_barrier) AS SELECT * FROM tenk1;
+EXPLAIN (COSTS OFF)
+SELECT 1 FROM tenk1_vw_sec WHERE EXISTS (SELECT 1 WHERE unique1 = 0);
+ QUERY PLAN
+-------------------------------------------------------------------
+ Subquery Scan on tenk1_vw_sec
+ Filter: (alternatives: SubPlan 1 or hashed SubPlan 2)
+ -> Gather
+ Workers Planned: 4
+ -> Parallel Index Only Scan using tenk1_unique1 on tenk1
+ SubPlan 1
+ -> Result
+ One-Time Filter: (tenk1_vw_sec.unique1 = 0)
+ SubPlan 2
+ -> Result
+(10 rows)
+
rollback;
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index ac26d68..7db75b0 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -383,4 +383,10 @@ ORDER BY 1;
SELECT * FROM information_schema.foreign_data_wrapper_options
ORDER BY 1, 2, 3;
+-- test interation between subquery and partial_paths
+SET LOCAL min_parallel_table_scan_size TO 0;
+CREATE VIEW tenk1_vw_sec WITH (security_barrier) AS SELECT * FROM tenk1;
+EXPLAIN (COSTS OFF)
+SELECT 1 FROM tenk1_vw_sec WHERE EXISTS (SELECT 1 WHERE unique1 = 0);
+
rollback;
--
1.8.3.1
