From a87c729153e372f3731689a7be007bc2b53f1410 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 28 Mar 2014 11:50:01 -0400 Subject: [PATCH] Fix EquivalenceClass processing for nested append relations. The original coding of EquivalenceClasses didn't foresee that appendrel child relations might themselves be appendrels; but this is possible for example when a UNION ALL subquery scans a table with inheritance children. The oversight led to failure to optimize ordering-related issues very well for the grandchild tables. After some false starts involving explicitly flattening the appendrel representation, we found that this could be fixed easily by removing a few implicit assumptions about appendrel parent rels not being children themselves. Kyotaro Horiguchi and Tom Lane, reviewed by Noah Misch --- src/backend/optimizer/path/allpaths.c | 20 ++++++++--- src/backend/optimizer/path/equivclass.c | 12 ++++--- src/backend/optimizer/plan/createplan.c | 2 +- src/test/regress/expected/union.out | 47 +++++++++++++++++++++++++ src/test/regress/sql/union.sql | 29 +++++++++++++++ 5 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 03be7b1fe8..5777cb2ff0 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -1021,10 +1021,15 @@ get_cheapest_parameterized_child_path(PlannerInfo *root, RelOptInfo *rel, * accumulate_append_subpath * Add a subpath to the list being built for an Append or MergeAppend * - * It's possible that the child is itself an Append path, in which case - * we can "cut out the middleman" and just add its child paths to our - * own list. (We don't try to do this earlier because we need to - * apply both levels of transformation to the quals.) + * It's possible that the child is itself an Append or MergeAppend path, in + * which case we can "cut out the middleman" and just add its child paths to + * our own list. (We don't try to do this earlier because we need to apply + * both levels of transformation to the quals.) + * + * Note that if we omit a child MergeAppend in this way, we are effectively + * omitting a sort step, which seems fine: if the parent is to be an Append, + * its result would be unsorted anyway, while if the parent is to be a + * MergeAppend, there's no point in a separate sort on a child. */ static List * accumulate_append_subpath(List *subpaths, Path *path) @@ -1036,6 +1041,13 @@ accumulate_append_subpath(List *subpaths, Path *path) /* list_copy is important here to avoid sharing list substructure */ return list_concat(subpaths, list_copy(apath->subpaths)); } + else if (IsA(path, MergeAppendPath)) + { + MergeAppendPath *mpath = (MergeAppendPath *) path; + + /* list_copy is important here to avoid sharing list substructure */ + return list_concat(subpaths, list_copy(mpath->subpaths)); + } else return lappend(subpaths, path); } diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 35d2a83599..ac12f84fd5 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -1937,16 +1937,20 @@ add_child_rel_equivalences(PlannerInfo *root, if (cur_ec->ec_has_volatile) continue; - /* No point in searching if parent rel not mentioned in eclass */ - if (!bms_is_subset(parent_rel->relids, cur_ec->ec_relids)) + /* + * No point in searching if parent rel not mentioned in eclass; but + * we can't tell that for sure if parent rel is itself a child. + */ + if (parent_rel->reloptkind == RELOPT_BASEREL && + !bms_is_subset(parent_rel->relids, cur_ec->ec_relids)) continue; foreach(lc2, cur_ec->ec_members) { EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc2); - if (cur_em->em_is_const || cur_em->em_is_child) - continue; /* ignore consts and children here */ + if (cur_em->em_is_const) + continue; /* ignore consts here */ /* Does it reference parent_rel? */ if (bms_overlap(cur_em->em_relids, parent_rel->relids)) diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 184d37a881..784805fbf4 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -751,7 +751,7 @@ create_merge_append_plan(PlannerInfo *root, MergeAppendPath *best_path) /* Compute sort column info, and adjust MergeAppend's tlist as needed */ (void) prepare_sort_from_pathkeys(root, plan, pathkeys, - NULL, + best_path->path.parent->relids, NULL, true, &node->numCols, diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out index 6f9ee5eb47..6a7d0f2a88 100644 --- a/src/test/regress/expected/union.out +++ b/src/test/regress/expected/union.out @@ -502,9 +502,56 @@ explain (costs off) Index Cond: (ab = 'ab'::text) (7 rows) +-- +-- Test that ORDER BY for UNION ALL can be pushed down to inheritance +-- children. +-- +CREATE TEMP TABLE t1c (b text, a text); +ALTER TABLE t1c INHERIT t1; +CREATE TEMP TABLE t2c (primary key (ab)) INHERITS (t2); +INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd'), ('m', 'n'), ('e', 'f'); +INSERT INTO t2c VALUES ('vw'), ('cd'), ('mn'), ('ef'); +CREATE INDEX t1c_ab_idx on t1c ((a || b)); +set enable_seqscan = on; +set enable_indexonlyscan = off; +explain (costs off) + SELECT * FROM + (SELECT a || b AS ab FROM t1 + UNION ALL + SELECT ab FROM t2) t + ORDER BY 1 LIMIT 8; + QUERY PLAN +------------------------------------------------ + Limit + -> Merge Append + Sort Key: ((t1.a || t1.b)) + -> Index Scan using t1_ab_idx on t1 + -> Index Scan using t1c_ab_idx on t1c + -> Index Scan using t2_pkey on t2 + -> Index Scan using t2c_pkey on t2c +(7 rows) + + SELECT * FROM + (SELECT a || b AS ab FROM t1 + UNION ALL + SELECT ab FROM t2) t + ORDER BY 1 LIMIT 8; + ab +---- + ab + ab + cd + dc + ef + fe + mn + nm +(8 rows) + reset enable_seqscan; reset enable_indexscan; reset enable_bitmapscan; +reset enable_indexonlyscan; -- Test constraint exclusion of UNION ALL subqueries explain (costs off) SELECT * FROM diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql index d567cf1481..5205435e2f 100644 --- a/src/test/regress/sql/union.sql +++ b/src/test/regress/sql/union.sql @@ -198,9 +198,38 @@ explain (costs off) SELECT * FROM t2) t WHERE ab = 'ab'; +-- +-- Test that ORDER BY for UNION ALL can be pushed down to inheritance +-- children. +-- + +CREATE TEMP TABLE t1c (b text, a text); +ALTER TABLE t1c INHERIT t1; +CREATE TEMP TABLE t2c (primary key (ab)) INHERITS (t2); +INSERT INTO t1c VALUES ('v', 'w'), ('c', 'd'), ('m', 'n'), ('e', 'f'); +INSERT INTO t2c VALUES ('vw'), ('cd'), ('mn'), ('ef'); +CREATE INDEX t1c_ab_idx on t1c ((a || b)); + +set enable_seqscan = on; +set enable_indexonlyscan = off; + +explain (costs off) + SELECT * FROM + (SELECT a || b AS ab FROM t1 + UNION ALL + SELECT ab FROM t2) t + ORDER BY 1 LIMIT 8; + + SELECT * FROM + (SELECT a || b AS ab FROM t1 + UNION ALL + SELECT ab FROM t2) t + ORDER BY 1 LIMIT 8; + reset enable_seqscan; reset enable_indexscan; reset enable_bitmapscan; +reset enable_indexonlyscan; -- Test constraint exclusion of UNION ALL subqueries explain (costs off) -- 2.40.0