From 5d0731da521f090f80ea39529fe274ac6d6bffa1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 7 Nov 2013 13:13:15 -0500 Subject: [PATCH] Fix generation of MergeAppend plans for optimized min/max on expressions. 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 | 23 ++++++++++- src/test/regress/expected/inherit.out | 58 +++++++++++++++++++++++++++ src/test/regress/sql/inherit.sql | 4 ++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index 090ae0b494..e5c57280ba 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -39,6 +39,7 @@ #include "optimizer/planmain.h" #include "optimizer/planner.h" #include "optimizer/subselect.h" +#include "optimizer/tlist.h" #include "parser/parsetree.h" #include "parser/parse_clause.h" #include "utils/lsyscache.h" @@ -526,7 +527,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, diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index a2ef7ef7cd..bfe67337b7 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1207,6 +1207,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 - matest0.id)) + -> Append + -> Seq Scan on public.matest0 + Output: matest0.id + -> Seq Scan on public.matest1 + Output: matest1.id + -> Seq Scan on public.matest2 + Output: matest2.id + -> Seq Scan on public.matest3 + Output: matest3.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; @@ -1238,6 +1260,42 @@ 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 - matest0.id)) + -> Result + Output: ((1 - matest0.id)) + -> Merge Append + Sort Key: ((1 - matest0.id)) + -> Index Scan using matest0i on public.matest0 + Output: matest0.id, (1 - matest0.id) + Index Cond: ((1 - matest0.id) IS NOT NULL) + -> Index Scan using matest1i on public.matest1 + Output: matest1.id, (1 - matest1.id) + Index Cond: ((1 - matest1.id) IS NOT NULL) + -> Sort + Output: matest2.id, ((1 - matest2.id)) + Sort Key: ((1 - matest2.id)) + -> Bitmap Heap Scan on public.matest2 + Output: matest2.id, (1 - matest2.id) + Filter: ((1 - matest2.id) IS NOT NULL) + -> Bitmap Index Scan on matest2_pkey + -> Index Scan using matest3i on public.matest3 + Output: matest3.id, (1 - matest3.id) + Index Cond: ((1 - matest3.id) IS NOT NULL) +(25 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 diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 86376554b0..747aa88666 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -382,11 +382,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; -- 2.40.0