From f82277c80de04a340b77daf930643af54b81400c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 31 Mar 2008 16:59:33 +0000 Subject: [PATCH] Apply my original fix for Taiki Yamaguchi's bug report about DISTINCT MAX(). Add some regression tests for plausible failures in this area. --- src/backend/optimizer/path/equivclass.c | 40 +++++++++++++++++++++++- src/backend/optimizer/plan/planagg.c | 14 ++++++++- src/include/optimizer/paths.h | 5 ++- src/test/regress/expected/aggregates.out | 33 +++++++++++++++++++ src/test/regress/sql/aggregates.sql | 7 +++++ 5 files changed, 96 insertions(+), 3 deletions(-) diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 1bc6d15a3e..0e36af637f 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -10,7 +10,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/path/equivclass.c,v 1.9 2008/01/09 20:42:27 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/path/equivclass.c,v 1.9.2.1 2008/03/31 16:59:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1638,6 +1638,44 @@ add_child_rel_equivalences(PlannerInfo *root, } +/* + * mutate_eclass_expressions + * Apply an expression tree mutator to all expressions stored in + * equivalence classes. + * + * This is a bit of a hack ... it's currently needed only by planagg.c, + * which needs to do a global search-and-replace of MIN/MAX Aggrefs + * after eclasses are already set up. Without changing the eclasses too, + * subsequent matching of ORDER BY clauses would fail. + * + * Note that we assume the mutation won't affect relation membership or any + * other properties we keep track of (which is a bit bogus, but by the time + * planagg.c runs, it no longer matters). Also we must be called in the + * main planner memory context. + */ +void +mutate_eclass_expressions(PlannerInfo *root, + Node *(*mutator) (), + void *context) +{ + ListCell *lc1; + + foreach(lc1, root->eq_classes) + { + EquivalenceClass *cur_ec = (EquivalenceClass *) lfirst(lc1); + ListCell *lc2; + + foreach(lc2, cur_ec->ec_members) + { + EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc2); + + cur_em->em_expr = (Expr *) + mutator((Node *) cur_em->em_expr, context); + } + } +} + + /* * find_eclass_clauses_for_index_join * Create joinclauses usable for a nestloop-with-inner-indexscan diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index e472e48ccb..dc3bf3b77c 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.36 2008/01/01 19:45:50 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.36.2.1 2008/03/31 16:59:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -187,6 +187,18 @@ optimize_minmax_aggregates(PlannerInfo *root, List *tlist, Path *best_path) hqual = replace_aggs_with_params_mutator(parse->havingQual, &aggs_list); + /* + * We have to replace Aggrefs with Params in equivalence classes too, + * else ORDER BY or DISTINCT on an optimized aggregate will fail. + * + * Note: at some point it might become necessary to mutate other + * data structures too, such as the query's sortClause or distinctClause. + * Right now, those won't be examined after this point. + */ + mutate_eclass_expressions(root, + replace_aggs_with_params_mutator, + &aggs_list); + /* * Generate the output plan --- basically just a Result */ diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index f3e50c5cbf..4bac7f4256 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/optimizer/paths.h,v 1.103 2008/01/01 19:45:58 momjian Exp $ + * $PostgreSQL: pgsql/src/include/optimizer/paths.h,v 1.103.2.1 2008/03/31 16:59:33 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -127,6 +127,9 @@ extern void add_child_rel_equivalences(PlannerInfo *root, AppendRelInfo *appinfo, RelOptInfo *parent_rel, RelOptInfo *child_rel); +extern void mutate_eclass_expressions(PlannerInfo *root, + Node *(*mutator) (), + void *context); extern List *find_eclass_clauses_for_index_join(PlannerInfo *root, RelOptInfo *rel, Relids outer_relids); diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 74635479e4..ae65314166 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -484,3 +484,36 @@ from int4_tbl; -2147483647 | 0 (5 rows) +-- check some cases that were handled incorrectly in 8.3.0 +select distinct max(unique2) from tenk1; + max +------ + 9999 +(1 row) + +select max(unique2) from tenk1 order by 1; + max +------ + 9999 +(1 row) + +select max(unique2) from tenk1 order by max(unique2); + max +------ + 9999 +(1 row) + +select max(unique2) from tenk1 order by max(unique2)+1; + max +------ + 9999 +(1 row) + +select max(unique2), generate_series(1,3) as g from tenk1 order by g desc; + max | g +------+--- + 9999 | 3 + 9999 | 2 + 9999 | 1 +(3 rows) + diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 890aa8dea0..afa997e7ea 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -217,3 +217,10 @@ select min(tenthous) from tenk1 where thousand = 33; -- check parameter propagation into an indexscan subquery select f1, (select min(unique1) from tenk1 where unique1 > f1) AS gt from int4_tbl; + +-- check some cases that were handled incorrectly in 8.3.0 +select distinct max(unique2) from tenk1; +select max(unique2) from tenk1 order by 1; +select max(unique2) from tenk1 order by max(unique2); +select max(unique2) from tenk1 order by max(unique2)+1; +select max(unique2), generate_series(1,3) as g from tenk1 order by g desc; -- GitLab