From dc1057fcd878d5c062c5c4c2b548af2be513b6ab Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Wed, 25 Apr 2018 15:14:14 -0400 Subject: [PATCH] Prevent generation of bogus subquery scan paths. Commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 didn't check that consider_parallel was set for the target relation or account for the possibility that required_outer might be non-empty. To prevent future bugs of this ilk, add some assertions to add_partial_path and do a bit of future-proofing of the code recently added to recurse_set_operations. Report by Andreas Seltenreich. Patch by Jeevan Chalke. Review by Amit Kapila and by me. Discussion: http://postgr.es/m/CAM2+6=U+9otsyF2fYB8x_2TBeHTR90itarqW=qAEjN-kHaC7kw@mail.gmail.com --- 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 9ed73da0f7..afc663cfd8 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2243,26 +2243,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 8d86e98adc..61d0770f10 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 776c592dbe..e190ad49d1 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 fb209de5e9..a07cd50fcb 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 ac26d68053..7db75b0d22 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; -- 2.40.0