Skip to content

Commit 5d0731d

Browse files
tglsfdctglsfdc
committed
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 4387cf9, 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.
1 parent 66e6daa commit 5d0731d

File tree

3 files changed

+84
-1
lines changed

3 files changed

+84
-1
lines changed

‎src/backend/optimizer/plan/planagg.c

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
#include "optimizer/planmain.h"
4040
#include "optimizer/planner.h"
4141
#include "optimizer/subselect.h"
42+
#include "optimizer/tlist.h"
4243
#include "parser/parsetree.h"
4344
#include "parser/parse_clause.h"
4445
#include "utils/lsyscache.h"
@@ -526,7 +527,27 @@ make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *mminfo)
526527
*/
527528
plan = create_plan(subroot, mminfo->path);
528529

529-
plan->targetlist = subparse->targetList;
530+
/*
531+
* If the top-level plan node is one that cannot do expression evaluation
532+
* and its existing target list isn't already what we need, we must insert
533+
* a Result node to project the desired tlist.
534+
*/
535+
if (!is_projection_capable_plan(plan) &&
536+
!tlist_same_exprs(subparse->targetList, plan->targetlist))
537+
{
538+
plan = (Plan *) make_result(subroot,
539+
subparse->targetList,
540+
NULL,
541+
plan);
542+
}
543+
else
544+
{
545+
/*
546+
* Otherwise, just replace the subplan's flat tlist with the desired
547+
* tlist.
548+
*/
549+
plan->targetlist = subparse->targetList;
550+
}
530551

531552
plan = (Plan *) make_limit(plan,
532553
subparse->limitOffset,

‎src/test/regress/expected/inherit.out

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1207,6 +1207,28 @@ select * from matest0 order by 1-id;
12071207
1 | Test 1
12081208
(6 rows)
12091209

1210+
explain (verbose, costs off) select min(1-id) from matest0;
1211+
QUERY PLAN
1212+
----------------------------------------
1213+
Aggregate
1214+
Output: min((1 - matest0.id))
1215+
-> Append
1216+
-> Seq Scan on public.matest0
1217+
Output: matest0.id
1218+
-> Seq Scan on public.matest1
1219+
Output: matest1.id
1220+
-> Seq Scan on public.matest2
1221+
Output: matest2.id
1222+
-> Seq Scan on public.matest3
1223+
Output: matest3.id
1224+
(11 rows)
1225+
1226+
select min(1-id) from matest0;
1227+
min
1228+
-----
1229+
-5
1230+
(1 row)
1231+
12101232
reset enable_indexscan;
12111233
set enable_seqscan = off; -- plan with fewest seqscans should be merge
12121234
explain (verbose, costs off) select * from matest0 order by 1-id;
@@ -1238,6 +1260,42 @@ select * from matest0 order by 1-id;
12381260
1 | Test 1
12391261
(6 rows)
12401262

1263+
explain (verbose, costs off) select min(1-id) from matest0;
1264+
QUERY PLAN
1265+
--------------------------------------------------------------------------
1266+
Result
1267+
Output: $0
1268+
InitPlan 1 (returns $0)
1269+
-> Limit
1270+
Output: ((1 - matest0.id))
1271+
-> Result
1272+
Output: ((1 - matest0.id))
1273+
-> Merge Append
1274+
Sort Key: ((1 - matest0.id))
1275+
-> Index Scan using matest0i on public.matest0
1276+
Output: matest0.id, (1 - matest0.id)
1277+
Index Cond: ((1 - matest0.id) IS NOT NULL)
1278+
-> Index Scan using matest1i on public.matest1
1279+
Output: matest1.id, (1 - matest1.id)
1280+
Index Cond: ((1 - matest1.id) IS NOT NULL)
1281+
-> Sort
1282+
Output: matest2.id, ((1 - matest2.id))
1283+
Sort Key: ((1 - matest2.id))
1284+
-> Bitmap Heap Scan on public.matest2
1285+
Output: matest2.id, (1 - matest2.id)
1286+
Filter: ((1 - matest2.id) IS NOT NULL)
1287+
-> Bitmap Index Scan on matest2_pkey
1288+
-> Index Scan using matest3i on public.matest3
1289+
Output: matest3.id, (1 - matest3.id)
1290+
Index Cond: ((1 - matest3.id) IS NOT NULL)
1291+
(25 rows)
1292+
1293+
select min(1-id) from matest0;
1294+
min
1295+
-----
1296+
-5
1297+
(1 row)
1298+
12411299
reset enable_seqscan;
12421300
drop table matest0 cascade;
12431301
NOTICE: drop cascades to 3 other objects

‎src/test/regress/sql/inherit.sql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,11 +382,15 @@ insert into matest3 (name) values ('Test 6');
382382
set enable_indexscan = off; -- force use of seqscan/sort, so no merge
383383
explain (verbose, costs off) select * from matest0 order by 1-id;
384384
select * from matest0 order by 1-id;
385+
explain (verbose, costs off) select min(1-id) from matest0;
386+
select min(1-id) from matest0;
385387
reset enable_indexscan;
386388

387389
set enable_seqscan = off; -- plan with fewest seqscans should be merge
388390
explain (verbose, costs off) select * from matest0 order by 1-id;
389391
select * from matest0 order by 1-id;
392+
explain (verbose, costs off) select min(1-id) from matest0;
393+
select min(1-id) from matest0;
390394
reset enable_seqscan;
391395

392396
drop table matest0 cascade;

0 commit comments

Comments
 (0)