Apply my original fix for Taiki Yamaguchi's bug report about DISTINCT MAX().

Add some regression tests for plausible failures in this area.

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 1bc6d15a3e83eaa1e70de86eb553e7436e3ec44c..b289ea6e65b37c998305cade838cd2aaf89d5f77 100644 (file)
--- 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.10 2008/03/31 16:59:26 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 e472e48ccb5aa5bbc5802d2bd347262be9bb3639..5bb92111f6065035a9758f81014cac061c743c54 100644 (file)
--- 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.37 2008/03/31 16:59:26 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 f3e50c5cbf96a40fb332c2fd1965092290b89ead..81e8089df169812fd758261dc8dbb52e1db9488c 100644 (file)
--- 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.104 2008/03/31 16:59:26 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 74635479e486e8c8c473c2e7513311eee883d617..ae65314166f06c753e210ee5b377b9faa943dc51 100644 (file)
--- 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 890aa8dea02d583d3112f46f778000091036494b..afa997e7ea7e8d0a608e9665386fcc47cc9d623d 100644 (file)
--- 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;