From 7097e6c4a48bc02155a9a52c570be67b30b30c9a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 8 Nov 2011 21:14:28 -0500 Subject: [PATCH] Wrap appendrel member outputs in PlaceHolderVars in additional cases. Add PlaceHolderVar wrappers as needed to make UNION ALL sub-select output expressions appear non-constant and distinct from each other. This makes the world safe for add_child_rel_equivalences to do what it does. Before, it was possible for that function to add identical expressions to different EquivalenceClasses, which logically should imply merging such ECs, which would be wrong; or to improperly add a constant to an EquivalenceClass, drastically changing its behavior. Per report from Teodor Sigaev. The only currently known consequence of this bug is "MergeAppend child's targetlist doesn't match MergeAppend" planner failures in 9.1 and later. I am suspicious that there may be other failure modes that could affect older release branches; but in the absence of any hard evidence, I'll refrain from back-patching further than 9.1. --- src/backend/optimizer/plan/planmain.c | 2 +- src/backend/optimizer/prep/prepjointree.c | 38 ++++++++++----- src/backend/optimizer/util/placeholder.c | 27 ++++++++--- src/include/optimizer/placeholder.h | 2 +- src/test/regress/expected/inherit.out | 58 +++++++++++++++++++++++ src/test/regress/sql/inherit.sql | 31 ++++++++++++ 6 files changed, 138 insertions(+), 20 deletions(-) diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c index ff39d5754d..d18e4c7a4c 100644 --- a/src/backend/optimizer/plan/planmain.c +++ b/src/backend/optimizer/plan/planmain.c @@ -188,7 +188,7 @@ query_planner(PlannerInfo *root, List *tlist, */ build_base_rel_tlists(root, tlist); - find_placeholders_in_jointree(root); + find_placeholders_in_query(root); joinlist = deconstruct_jointree(root); diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index ac622a34d9..4ff7f48977 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -785,22 +785,15 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, parse->havingQual = pullup_replace_vars(parse->havingQual, &rvcontext); /* - * Replace references in the translated_vars lists of appendrels. When - * pulling up an appendrel member, we do not need PHVs in the list of the - * parent appendrel --- there isn't any outer join between. Elsewhere, use - * PHVs for safety. (This analysis could be made tighter but it seems - * unlikely to be worth much trouble.) + * Replace references in the translated_vars lists of appendrels, too. + * We do it this way because we must preserve the AppendRelInfo structs. */ foreach(lc, root->append_rel_list) { AppendRelInfo *appinfo = (AppendRelInfo *) lfirst(lc); - bool save_need_phvs = rvcontext.need_phvs; - if (appinfo == containing_appendrel) - rvcontext.need_phvs = false; appinfo->translated_vars = (List *) pullup_replace_vars((Node *) appinfo->translated_vars, &rvcontext); - rvcontext.need_phvs = save_need_phvs; } /* @@ -1408,8 +1401,31 @@ pullup_replace_vars_callback(Var *var, if (newnode && IsA(newnode, Var) && ((Var *) newnode)->varlevelsup == 0) { - /* Simple Vars always escape being wrapped */ - wrap = false; + /* + * Simple Vars normally escape being wrapped. However, in + * wrap_non_vars mode (ie, we are dealing with an appendrel + * member), we must ensure that each tlist entry expands to a + * distinct expression, else we may have problems with + * improperly placing identical entries into different + * EquivalenceClasses. Therefore, we wrap a Var in a + * PlaceHolderVar if it duplicates any earlier entry in the + * tlist (ie, we've got "SELECT x, x, ..."). Since each PHV + * is distinct, this fixes the ambiguity. We can use + * tlist_member to detect whether there's an earlier + * duplicate. + */ + wrap = (rcon->wrap_non_vars && + tlist_member(newnode, rcon->targetlist) != tle); + } + else if (newnode && IsA(newnode, PlaceHolderVar) && + ((PlaceHolderVar *) newnode)->phlevelsup == 0) + { + /* + * No need to directly wrap a PlaceHolderVar with another one, + * either, unless we need to prevent duplication. + */ + wrap = (rcon->wrap_non_vars && + tlist_member(newnode, rcon->targetlist) != tle); } else if (rcon->wrap_non_vars) { diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c index 8b65245b51..3ae5154684 100644 --- a/src/backend/optimizer/util/placeholder.c +++ b/src/backend/optimizer/util/placeholder.c @@ -104,28 +104,41 @@ find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, } /* - * find_placeholders_in_jointree - * Search the jointree for PlaceHolderVars, and build PlaceHolderInfos + * find_placeholders_in_query + * Search the query for PlaceHolderVars, and build PlaceHolderInfos * - * We don't need to look at the targetlist because build_base_rel_tlists() - * will already have made entries for any PHVs in the tlist. + * We need to examine the jointree, but not the targetlist, because + * build_base_rel_tlists() will already have made entries for any PHVs + * in the targetlist. + * + * We also need to search for PHVs in AppendRelInfo translated_vars + * lists. In most cases, translated_vars entries aren't directly referenced + * elsewhere, but we need to create PlaceHolderInfo entries for them to + * support set_rel_width() calculations for the appendrel child relations. */ void -find_placeholders_in_jointree(PlannerInfo *root) +find_placeholders_in_query(PlannerInfo *root) { /* We need do nothing if the query contains no PlaceHolderVars */ if (root->glob->lastPHId != 0) { - /* Start recursion at top of jointree */ + /* Recursively search the jointree */ Assert(root->parse->jointree != NULL && IsA(root->parse->jointree, FromExpr)); (void) find_placeholders_recurse(root, (Node *) root->parse->jointree); + + /* + * Also search the append_rel_list for translated vars that are PHVs. + * Barring finding them elsewhere in the query, they do not need any + * ph_may_need bits, only to be present in the PlaceHolderInfo list. + */ + mark_placeholders_in_expr(root, (Node *) root->append_rel_list, NULL); } } /* * find_placeholders_recurse - * One recursion level of find_placeholders_in_jointree. + * One recursion level of jointree search for find_placeholders_in_query. * * jtnode is the current jointree node to examine. * diff --git a/src/include/optimizer/placeholder.h b/src/include/optimizer/placeholder.h index f112419fd5..5fbebfcb0c 100644 --- a/src/include/optimizer/placeholder.h +++ b/src/include/optimizer/placeholder.h @@ -21,7 +21,7 @@ extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr, Relids phrels); extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root, PlaceHolderVar *phv, bool create_new_ph); -extern void find_placeholders_in_jointree(PlannerInfo *root); +extern void find_placeholders_in_query(PlannerInfo *root); extern void mark_placeholder_maybe_needed(PlannerInfo *root, PlaceHolderInfo *phinfo, Relids relids); extern void update_placeholder_eval_levels(PlannerInfo *root, diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index d59ca449dc..efd6ac4fe0 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1242,3 +1242,61 @@ NOTICE: drop cascades to 3 other objects DETAIL: drop cascades to table matest1 drop cascades to table matest2 drop cascades to table matest3 +-- +-- Test merge-append for UNION ALL append relations +-- Check handling of duplicated, constant, or volatile targetlist items +-- +set enable_seqscan = off; +set enable_indexscan = on; +set enable_bitmapscan = off; +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT thousand, thousand FROM tenk1 +ORDER BY thousand, tenthous; + QUERY PLAN +---------------------------------------------------------------------- + Result + -> Merge Append + Sort Key: public.tenk1.thousand, public.tenk1.tenthous + -> Index Scan using tenk1_thous_tenthous on tenk1 + -> Sort + Sort Key: public.tenk1.thousand, public.tenk1.thousand + -> Seq Scan on tenk1 +(7 rows) + +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT 42, 42 FROM tenk1 +ORDER BY thousand, tenthous; + QUERY PLAN +---------------------------------------------------------------- + Result + -> Merge Append + Sort Key: public.tenk1.thousand, public.tenk1.tenthous + -> Index Scan using tenk1_thous_tenthous on tenk1 + -> Sort + Sort Key: (42), (42) + -> Seq Scan on tenk1 +(7 rows) + +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT thousand, random()::integer FROM tenk1 +ORDER BY thousand, tenthous; + QUERY PLAN +---------------------------------------------------------------------- + Result + -> Merge Append + Sort Key: public.tenk1.thousand, public.tenk1.tenthous + -> Index Scan using tenk1_thous_tenthous on tenk1 + -> Sort + Sort Key: public.tenk1.thousand, ((random())::integer) + -> Seq Scan on tenk1 +(7 rows) + +reset enable_seqscan; +reset enable_indexscan; +reset enable_bitmapscan; diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 3087a14b72..6e5a1d1c8e 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -406,3 +406,34 @@ select * from matest0 order by 1-id; reset enable_seqscan; drop table matest0 cascade; + +-- +-- Test merge-append for UNION ALL append relations +-- Check handling of duplicated, constant, or volatile targetlist items +-- + +set enable_seqscan = off; +set enable_indexscan = on; +set enable_bitmapscan = off; + +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT thousand, thousand FROM tenk1 +ORDER BY thousand, tenthous; + +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT 42, 42 FROM tenk1 +ORDER BY thousand, tenthous; + +explain (costs off) +SELECT thousand, tenthous FROM tenk1 +UNION ALL +SELECT thousand, random()::integer FROM tenk1 +ORDER BY thousand, tenthous; + +reset enable_seqscan; +reset enable_indexscan; +reset enable_bitmapscan; -- 2.40.0