Fix SELECT DISTINCT with index-optimized MIN/MAX on inheritance trees.

In a query such as "SELECT DISTINCT min(x) FROM tab", the DISTINCT is
pretty useless (there being only one output row), but nonetheless it
shouldn't fail.  But it could fail if "tab" is an inheritance parent,
because planagg.c's code for fixing up equivalence classes after making the
index-optimized MIN/MAX transformation wasn't prepared to find child-table
versions of the aggregate expression.  The least ugly fix seems to be
to add an option to mutate_eclass_expressions() to skip child-table
equivalence class members, which aren't used anymore at this stage of
planning so it's not really necessary to fix them.  Since child members
are ignored in many cases already, it seems plausible for
mutate_eclass_expressions() to have an option to ignore them too.

Per bug #7703 from Maxim Boguk.

Back-patch to 9.1.  Although the same code exists before that, it cannot
encounter child-table aggregates AFAICS, because the index optimization
transformation cannot succeed on inheritance trees before 9.1 (for lack
of MergeAppend).

diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 349bd537e5e791bd89d2efff3f747ecc8e496eda..70e048c6e9c93283d35154b3ff9f43846a276af0 100644 (file)
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -1973,12 +1973,12 @@ add_child_rel_equivalences(PlannerInfo *root,
 /*
  * mutate_eclass_expressions
  *   Apply an expression tree mutator to all expressions stored in
- *   equivalence classes.
+ *   equivalence classes (but ignore child exprs unless include_child_exprs).
  *
  * 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.
+ * subsequent matching of ORDER BY and DISTINCT 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
@@ -1988,7 +1988,8 @@ add_child_rel_equivalences(PlannerInfo *root,
 void
 mutate_eclass_expressions(PlannerInfo *root,
                          Node *(*mutator) (),
-                         void *context)
+                         void *context,
+                         bool include_child_exprs)
 {
    ListCell   *lc1;
 
@@ -2001,6 +2002,9 @@ mutate_eclass_expressions(PlannerInfo *root,
        {
            EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc2);
 
+           if (cur_em->em_is_child && !include_child_exprs)
+               continue;       /* ignore children unless requested */
+
            cur_em->em_expr = (Expr *)
                mutator((Node *) cur_em->em_expr, context);
        }

diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index 0bbca071fb01897032b1951ba1a03b7b6586bc05..cf2ab9ea840f7f5cd9ad9a8c0cabad9ee8298843 100644 (file)
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -256,7 +256,10 @@ optimize_minmax_aggregates(PlannerInfo *root, List *tlist,
 
    /*
     * We have to replace Aggrefs with Params in equivalence classes too, else
-    * ORDER BY or DISTINCT on an optimized aggregate will fail.
+    * ORDER BY or DISTINCT on an optimized aggregate will fail.  We don't
+    * need to process child eclass members though, since they aren't of
+    * interest anymore --- and replace_aggs_with_params_mutator isn't able
+    * to handle Aggrefs containing translated child Vars, anyway.
     *
     * Note: at some point it might become necessary to mutate other data
     * structures too, such as the query's sortClause or distinctClause. Right
@@ -264,7 +267,8 @@ optimize_minmax_aggregates(PlannerInfo *root, List *tlist,
     */
    mutate_eclass_expressions(root,
                              replace_aggs_with_params_mutator,
-                             (void *) root);
+                             (void *) root,
+                             false);
 
    /*
     * Generate the output plan --- basically just a Result

diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index b6fb8ee5ce93cda5de8a824528378921f43f0584..ab04d5e6047341e1ae98d893c7785455654693c0 100644 (file)
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -124,7 +124,8 @@ extern void add_child_rel_equivalences(PlannerInfo *root,
                           RelOptInfo *child_rel);
 extern void mutate_eclass_expressions(PlannerInfo *root,
                          Node *(*mutator) (),
-                         void *context);
+                         void *context,
+                         bool include_child_exprs);
 extern List *generate_implied_equalities_for_indexcol(PlannerInfo *root,
                                         IndexOptInfo *index,
                                         int indexcol);

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 5678f066cbf9b07f2d5bc9384e65e92906cc1e42..b6d79ca387ffb3fb09eb76ba6c9a0ffb10461c31 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -740,6 +740,45 @@ select min(f1), max(f1) from minmaxtest;
   11 |  18
 (1 row)
 
+-- DISTINCT doesn't do anything useful here, but it shouldn't fail
+explain (costs off)
+  select distinct min(f1), max(f1) from minmaxtest;
+                                        QUERY PLAN                                         
+-------------------------------------------------------------------------------------------
+ HashAggregate
+   InitPlan 1 (returns $0)
+     ->  Limit
+           ->  Merge Append
+                 Sort Key: public.minmaxtest.f1
+                 ->  Index Only Scan using minmaxtesti on minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan using minmaxtest1i on minmaxtest1 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan Backward using minmaxtest2i on minmaxtest2 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan using minmaxtest3i on minmaxtest3 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+   InitPlan 2 (returns $1)
+     ->  Limit
+           ->  Merge Append
+                 Sort Key: public.minmaxtest.f1
+                 ->  Index Only Scan Backward using minmaxtesti on minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan Backward using minmaxtest1i on minmaxtest1 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan using minmaxtest2i on minmaxtest2 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+                 ->  Index Only Scan Backward using minmaxtest3i on minmaxtest3 minmaxtest
+                       Index Cond: (f1 IS NOT NULL)
+   ->  Result
+(26 rows)
+
+select distinct min(f1), max(f1) from minmaxtest;
+ min | max 
+-----+-----
+  11 |  18
+(1 row)
+
 drop table minmaxtest cascade;
 NOTICE:  drop cascades to 3 other objects
 DETAIL:  drop cascades to table minmaxtest1

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index d1c74720d3722cf633b9f063355360689944b1c8..e33b36bad11c7a625fe711f02a9189a59861c4b3 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -278,6 +278,11 @@ explain (costs off)
   select min(f1), max(f1) from minmaxtest;
 select min(f1), max(f1) from minmaxtest;
 
+-- DISTINCT doesn't do anything useful here, but it shouldn't fail
+explain (costs off)
+  select distinct min(f1), max(f1) from minmaxtest;
+select distinct min(f1), max(f1) from minmaxtest;
+
 drop table minmaxtest cascade;
 
 --