]> granicus.if.org Git - postgresql/commitdiff
Fix generation of MergeAppend plans for optimized min/max on expressions.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 7 Nov 2013 18:13:47 +0000 (13:13 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 7 Nov 2013 18:13:47 +0000 (13:13 -0500)
Before jamming a desired targetlist into a plan node, one really ought to
make sure the plan node can handle projections, and insert a buffering
Result plan node if not.  planagg.c forgot to do this, which is a hangover
from the days when it only dealt with IndexScan plan types.  MergeAppend
doesn't project though, not to mention that it gets unhappy if you remove
its possibly-resjunk sort columns.  The code accidentally failed to fail
for cases in which the min/max argument was a simple Var, because the new
targetlist would be equivalent to the original "flat" tlist anyway.
For any more complex case, it's been broken since 9.1 where we introduced
the ability to optimize min/max using MergeAppend, as reported by Raphael
Bauduin.  Fix by duplicating the logic from grouping_planner that decides
whether we need a Result node.

In 9.2 and 9.1, this requires back-porting the tlist_same_exprs() function
introduced in commit 4387cf956b9eb13aad569634e0c4df081d76e2e3, else we'd
uselessly add a Result node in cases that worked before.  It's rather
tempting to back-patch that whole commit so that we can avoid extra Result
nodes in mainline cases too; but I'll refrain, since that code hasn't
really seen all that much field testing yet.

src/backend/optimizer/plan/planagg.c
src/backend/optimizer/util/tlist.c
src/include/optimizer/tlist.h
src/test/regress/expected/inherit.out
src/test/regress/sql/inherit.sql

index f7ce9b12a920fdea355374a555afaa4b2db73a2e..88a6c99ea5063d2f4c7dbdb25020abaea1dbc121 100644 (file)
@@ -37,6 +37,7 @@
 #include "optimizer/paths.h"
 #include "optimizer/planmain.h"
 #include "optimizer/subselect.h"
+#include "optimizer/tlist.h"
 #include "parser/parsetree.h"
 #include "parser/parse_clause.h"
 #include "utils/lsyscache.h"
@@ -521,7 +522,27 @@ make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *mminfo)
         */
        plan = create_plan(subroot, mminfo->path);
 
-       plan->targetlist = subparse->targetList;
+       /*
+        * If the top-level plan node is one that cannot do expression evaluation
+        * and its existing target list isn't already what we need, we must insert
+        * a Result node to project the desired tlist.
+        */
+       if (!is_projection_capable_plan(plan) &&
+               !tlist_same_exprs(subparse->targetList, plan->targetlist))
+       {
+               plan = (Plan *) make_result(subroot,
+                                                                       subparse->targetList,
+                                                                       NULL,
+                                                                       plan);
+       }
+       else
+       {
+               /*
+                * Otherwise, just replace the subplan's flat tlist with the desired
+                * tlist.
+                */
+               plan->targetlist = subparse->targetList;
+       }
 
        plan = (Plan *) make_limit(plan,
                                                           subparse->limitOffset,
index b8149f7b461935409b7a9cd283ec89d40a2337d6..c98c767f310f3871dc5c3a3528471292e5ed9261 100644 (file)
@@ -188,6 +188,43 @@ get_tlist_exprs(List *tlist, bool includeJunk)
 }
 
 
+/*
+ * tlist_same_exprs
+ *             Check whether two target lists contain the same expressions
+ *
+ * Note: this function is used to decide whether it's safe to jam a new tlist
+ * into a non-projection-capable plan node.  Obviously we can't do that unless
+ * the node's tlist shows it already returns the column values we want.
+ * However, we can ignore the TargetEntry attributes resname, ressortgroupref,
+ * resorigtbl, resorigcol, and resjunk, because those are only labelings that
+ * don't affect the row values computed by the node.  (Moreover, if we didn't
+ * ignore them, we'd frequently fail to make the desired optimization, since
+ * the planner tends to not bother to make resname etc. valid in intermediate
+ * plan nodes.)  Note that on success, the caller must still jam the desired
+ * tlist into the plan node, else it won't have the desired labeling fields.
+ */
+bool
+tlist_same_exprs(List *tlist1, List *tlist2)
+{
+       ListCell   *lc1,
+                          *lc2;
+
+       if (list_length(tlist1) != list_length(tlist2))
+               return false;                   /* not same length, so can't match */
+
+       forboth(lc1, tlist1, lc2, tlist2)
+       {
+               TargetEntry *tle1 = (TargetEntry *) lfirst(lc1);
+               TargetEntry *tle2 = (TargetEntry *) lfirst(lc2);
+
+               if (!equal(tle1->expr, tle2->expr))
+                       return false;
+       }
+
+       return true;
+}
+
+
 /*
  * Does tlist have same output datatypes as listed in colTypes?
  *
index 0edc8835fbd513b4897903081fa92a90aa6b8523..d172549a33298c093bdd03a8b1722d52ce5422bb 100644 (file)
@@ -26,6 +26,9 @@ extern List *flatten_tlist(List *tlist, PVCAggregateBehavior aggbehavior,
 extern List *add_to_flat_tlist(List *tlist, List *exprs);
 
 extern List *get_tlist_exprs(List *tlist, bool includeJunk);
+
+extern bool tlist_same_exprs(List *tlist1, List *tlist2);
+
 extern bool tlist_same_datatypes(List *tlist, List *colTypes, bool junkOK);
 extern bool tlist_same_collations(List *tlist, List *colCollations, bool junkOK);
 
index 36d90f5bba1dd8da2a22ff6a13ca5e0e8e286ca2..c79b8f950f13fa2ed431440188fda13f2ed8646a 100644 (file)
@@ -1203,6 +1203,28 @@ select * from matest0 order by 1-id;
   1 | Test 1
 (6 rows)
 
+explain (verbose, costs off) select min(1-id) from matest0;
+                   QUERY PLAN                   
+------------------------------------------------
+ Aggregate
+   Output: min((1 - public.matest0.id))
+   ->  Append
+         ->  Seq Scan on public.matest0
+               Output: public.matest0.id
+         ->  Seq Scan on public.matest1 matest0
+               Output: public.matest0.id
+         ->  Seq Scan on public.matest2 matest0
+               Output: public.matest0.id
+         ->  Seq Scan on public.matest3 matest0
+               Output: public.matest0.id
+(11 rows)
+
+select min(1-id) from matest0;
+ min 
+-----
+  -5
+(1 row)
+
 reset enable_indexscan;
 set enable_seqscan = off;  -- plan with fewest seqscans should be merge
 explain (verbose, costs off) select * from matest0 order by 1-id;
@@ -1236,6 +1258,41 @@ select * from matest0 order by 1-id;
   1 | Test 1
 (6 rows)
 
+explain (verbose, costs off) select min(1-id) from matest0;
+                                      QUERY PLAN                                      
+--------------------------------------------------------------------------------------
+ Result
+   Output: $0
+   InitPlan 1 (returns $0)
+     ->  Limit
+           Output: ((1 - public.matest0.id))
+           ->  Result
+                 Output: ((1 - public.matest0.id))
+                 ->  Merge Append
+                       Sort Key: ((1 - public.matest0.id))
+                       ->  Index Scan using matest0i on public.matest0
+                             Output: public.matest0.id, (1 - public.matest0.id)
+                             Index Cond: ((1 - public.matest0.id) IS NOT NULL)
+                       ->  Index Scan using matest1i on public.matest1 matest0
+                             Output: public.matest0.id, (1 - public.matest0.id)
+                             Index Cond: ((1 - public.matest0.id) IS NOT NULL)
+                       ->  Sort
+                             Output: public.matest0.id, ((1 - public.matest0.id))
+                             Sort Key: ((1 - public.matest0.id))
+                             ->  Seq Scan on public.matest2 matest0
+                                   Output: public.matest0.id, (1 - public.matest0.id)
+                                   Filter: ((1 - public.matest0.id) IS NOT NULL)
+                       ->  Index Scan using matest3i on public.matest3 matest0
+                             Output: public.matest0.id, (1 - public.matest0.id)
+                             Index Cond: ((1 - public.matest0.id) IS NOT NULL)
+(24 rows)
+
+select min(1-id) from matest0;
+ min 
+-----
+  -5
+(1 row)
+
 reset enable_seqscan;
 drop table matest0 cascade;
 NOTICE:  drop cascades to 3 other objects
index 6c7d809fb60a7fdc94d1beac23d665653014fcf1..3b3d71f040c6a6a260967f38974b9780b965bc40 100644 (file)
@@ -398,11 +398,15 @@ insert into matest3 (name) values ('Test 6');
 set enable_indexscan = off;  -- force use of seqscan/sort, so no merge
 explain (verbose, costs off) select * from matest0 order by 1-id;
 select * from matest0 order by 1-id;
+explain (verbose, costs off) select min(1-id) from matest0;
+select min(1-id) from matest0;
 reset enable_indexscan;
 
 set enable_seqscan = off;  -- plan with fewest seqscans should be merge
 explain (verbose, costs off) select * from matest0 order by 1-id;
 select * from matest0 order by 1-id;
+explain (verbose, costs off) select min(1-id) from matest0;
+select min(1-id) from matest0;
 reset enable_seqscan;
 
 drop table matest0 cascade;