Skip to content

Commit 6e2c762

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 aad87e3 commit 6e2c762

File tree

5 files changed

+123
-1
lines changed

5 files changed

+123
-1
lines changed

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
#include "optimizer/paths.h"
3838
#include "optimizer/planmain.h"
3939
#include "optimizer/subselect.h"
40+
#include "optimizer/tlist.h"
4041
#include "parser/parsetree.h"
4142
#include "parser/parse_clause.h"
4243
#include "utils/lsyscache.h"
@@ -521,7 +522,27 @@ make_agg_subplan(PlannerInfo *root, MinMaxAggInfo *mminfo)
521522
*/
522523
plan = create_plan(subroot, mminfo->path);
523524

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

526547
plan = (Plan *) make_limit(plan,
527548
subparse->limitOffset,

‎src/backend/optimizer/util/tlist.c

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,43 @@ get_tlist_exprs(List *tlist, bool includeJunk)
188188
}
189189

190190

191+
/*
192+
* tlist_same_exprs
193+
* Check whether two target lists contain the same expressions
194+
*
195+
* Note: this function is used to decide whether it's safe to jam a new tlist
196+
* into a non-projection-capable plan node. Obviously we can't do that unless
197+
* the node's tlist shows it already returns the column values we want.
198+
* However, we can ignore the TargetEntry attributes resname, ressortgroupref,
199+
* resorigtbl, resorigcol, and resjunk, because those are only labelings that
200+
* don't affect the row values computed by the node. (Moreover, if we didn't
201+
* ignore them, we'd frequently fail to make the desired optimization, since
202+
* the planner tends to not bother to make resname etc. valid in intermediate
203+
* plan nodes.) Note that on success, the caller must still jam the desired
204+
* tlist into the plan node, else it won't have the desired labeling fields.
205+
*/
206+
bool
207+
tlist_same_exprs(List *tlist1, List *tlist2)
208+
{
209+
ListCell *lc1,
210+
*lc2;
211+
212+
if (list_length(tlist1) != list_length(tlist2))
213+
return false; /* not same length, so can't match */
214+
215+
forboth(lc1, tlist1, lc2, tlist2)
216+
{
217+
TargetEntry *tle1 = (TargetEntry *) lfirst(lc1);
218+
TargetEntry *tle2 = (TargetEntry *) lfirst(lc2);
219+
220+
if (!equal(tle1->expr, tle2->expr))
221+
return false;
222+
}
223+
224+
return true;
225+
}
226+
227+
191228
/*
192229
* Does tlist have same output datatypes as listed in colTypes?
193230
*

‎src/include/optimizer/tlist.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@ extern List *flatten_tlist(List *tlist, PVCAggregateBehavior aggbehavior,
2626
extern List *add_to_flat_tlist(List *tlist, List *exprs);
2727

2828
extern List *get_tlist_exprs(List *tlist, bool includeJunk);
29+
30+
extern bool tlist_same_exprs(List *tlist1, List *tlist2);
31+
2932
extern bool tlist_same_datatypes(List *tlist, List *colTypes, bool junkOK);
3033
extern bool tlist_same_collations(List *tlist, List *colCollations, bool junkOK);
3134

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,6 +1203,28 @@ select * from matest0 order by 1-id;
12031203
1 | Test 1
12041204
(6 rows)
12051205

1206+
explain (verbose, costs off) select min(1-id) from matest0;
1207+
QUERY PLAN
1208+
------------------------------------------------
1209+
Aggregate
1210+
Output: min((1 - public.matest0.id))
1211+
-> Append
1212+
-> Seq Scan on public.matest0
1213+
Output: public.matest0.id
1214+
-> Seq Scan on public.matest1 matest0
1215+
Output: public.matest0.id
1216+
-> Seq Scan on public.matest2 matest0
1217+
Output: public.matest0.id
1218+
-> Seq Scan on public.matest3 matest0
1219+
Output: public.matest0.id
1220+
(11 rows)
1221+
1222+
select min(1-id) from matest0;
1223+
min
1224+
-----
1225+
-5
1226+
(1 row)
1227+
12061228
reset enable_indexscan;
12071229
set enable_seqscan = off; -- plan with fewest seqscans should be merge
12081230
explain (verbose, costs off) select * from matest0 order by 1-id;
@@ -1236,6 +1258,41 @@ select * from matest0 order by 1-id;
12361258
1 | Test 1
12371259
(6 rows)
12381260

1261+
explain (verbose, costs off) select min(1-id) from matest0;
1262+
QUERY PLAN
1263+
--------------------------------------------------------------------------------------
1264+
Result
1265+
Output: $0
1266+
InitPlan 1 (returns $0)
1267+
-> Limit
1268+
Output: ((1 - public.matest0.id))
1269+
-> Result
1270+
Output: ((1 - public.matest0.id))
1271+
-> Merge Append
1272+
Sort Key: ((1 - public.matest0.id))
1273+
-> Index Scan using matest0i on public.matest0
1274+
Output: public.matest0.id, (1 - public.matest0.id)
1275+
Index Cond: ((1 - public.matest0.id) IS NOT NULL)
1276+
-> Index Scan using matest1i on public.matest1 matest0
1277+
Output: public.matest0.id, (1 - public.matest0.id)
1278+
Index Cond: ((1 - public.matest0.id) IS NOT NULL)
1279+
-> Sort
1280+
Output: public.matest0.id, ((1 - public.matest0.id))
1281+
Sort Key: ((1 - public.matest0.id))
1282+
-> Seq Scan on public.matest2 matest0
1283+
Output: public.matest0.id, (1 - public.matest0.id)
1284+
Filter: ((1 - public.matest0.id) IS NOT NULL)
1285+
-> Index Scan using matest3i on public.matest3 matest0
1286+
Output: public.matest0.id, (1 - public.matest0.id)
1287+
Index Cond: ((1 - public.matest0.id) IS NOT NULL)
1288+
(24 rows)
1289+
1290+
select min(1-id) from matest0;
1291+
min
1292+
-----
1293+
-5
1294+
(1 row)
1295+
12391296
reset enable_seqscan;
12401297
drop table matest0 cascade;
12411298
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
@@ -398,11 +398,15 @@ insert into matest3 (name) values ('Test 6');
398398
set enable_indexscan = off; -- force use of seqscan/sort, so no merge
399399
explain (verbose, costs off) select * from matest0 order by 1-id;
400400
select * from matest0 order by 1-id;
401+
explain (verbose, costs off) select min(1-id) from matest0;
402+
select min(1-id) from matest0;
401403
reset enable_indexscan;
402404

403405
set enable_seqscan = off; -- plan with fewest seqscans should be merge
404406
explain (verbose, costs off) select * from matest0 order by 1-id;
405407
select * from matest0 order by 1-id;
408+
explain (verbose, costs off) select min(1-id) from matest0;
409+
select min(1-id) from matest0;
406410
reset enable_seqscan;
407411

408412
drop table matest0 cascade;

0 commit comments

Comments
 (0)