]> granicus.if.org Git - postgresql/commitdiff
Wrap appendrel member outputs in PlaceHolderVars in additional cases.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 9 Nov 2011 02:14:28 +0000 (21:14 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 9 Nov 2011 02:14:28 +0000 (21:14 -0500)
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
src/backend/optimizer/prep/prepjointree.c
src/backend/optimizer/util/placeholder.c
src/include/optimizer/placeholder.h
src/test/regress/expected/inherit.out
src/test/regress/sql/inherit.sql

index ff39d5754dc989a087102523ca77adca79a89e17..d18e4c7a4cff4aa7283ceb71e3c49609020fbae1 100644 (file)
@@ -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);
 
index ac622a34d9d96e726af679479e69e7b7d7eaa65a..4ff7f48977bc1d288177dd17b150bd0b1c082f9d 100644 (file)
@@ -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)
                        {
index 8b65245b5107595da7e15d09d009446ca0d701c7..3ae5154684db810f992f5da5d1fbc2b4acc7e4a1 100644 (file)
@@ -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.
  *
index f112419fd5885e8a41e08014c7c0c4257de524c4..5fbebfcb0c8ddd4dd9cbf21560333783b6c5a001 100644 (file)
@@ -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,
index d59ca449dc4fcbdd391f021d3d2ad52e23f4c068..efd6ac4fe084af55fc03843b7e27831ca367b22b 100644 (file)
@@ -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;
index 3087a14b729f99082fb72fa5c98e7b4292bda9b3..6e5a1d1c8eeaef0973ae213bffc8a064e5cf238b 100644 (file)
@@ -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;