Fix failure to mark all aggregates with appropriate transtype.

In commit 915b703e1 I gave get_agg_clause_costs() the responsibility of
marking Aggref nodes with the appropriate aggtranstype.  I failed to notice
that where it was being called from, it might see only a subset of the
Aggref nodes that were in the original targetlist.  Specifically, if there
are duplicate aggregate calls in the tlist, either make_sort_input_target
or make_window_input_target might put just a single instance into the
grouping_target, and then only that instance would get marked.  Fix by
moving the call back into grouping_planner(), before we start building
assorted PathTargets from the query tlist.  Per report from Stefan Huehner.

Report: <20160702131056.GD3165@huehner.biz>

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index a66317367c3435a5b92b8179e7732054812c1d8d..ddf1109de76f4a75601af0d2721f13aa3a8ee5c0 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -114,6 +114,7 @@ static Size estimate_hashagg_tablesize(Path *path,
 static RelOptInfo *create_grouping_paths(PlannerInfo *root,
                                          RelOptInfo *input_rel,
                                          PathTarget *target,
+                                         const AggClauseCosts *agg_costs,
                                          List *rollup_lists,
                                          List *rollup_groupclauses);
 static RelOptInfo *create_window_paths(PlannerInfo *root,
@@ -1499,6 +1500,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
                PathTarget *grouping_target;
                PathTarget *scanjoin_target;
                bool            have_grouping;
+               AggClauseCosts agg_costs;
                WindowFuncLists *wflists = NULL;
                List       *activeWindows = NIL;
                List       *rollup_lists = NIL;
@@ -1623,6 +1625,28 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
                 */
                root->processed_tlist = tlist;
 
+               /*
+                * Collect statistics about aggregates for estimating costs, and mark
+                * all the aggregates with resolved aggtranstypes.  We must do this
+                * before slicing and dicing the tlist into various pathtargets, else
+                * some copies of the Aggref nodes might escape being marked with the
+                * correct transtypes.
+                *
+                * Note: currently, we do not detect duplicate aggregates here.  This
+                * may result in somewhat-overestimated cost, which is fine for our
+                * purposes since all Paths will get charged the same.  But at some
+                * point we might wish to do that detection in the planner, rather
+                * than during executor startup.
+                */
+               MemSet(&agg_costs, 0, sizeof(AggClauseCosts));
+               if (parse->hasAggs)
+               {
+                       get_agg_clause_costs(root, (Node *) tlist, AGGSPLIT_SIMPLE,
+                                                                &agg_costs);
+                       get_agg_clause_costs(root, parse->havingQual, AGGSPLIT_SIMPLE,
+                                                                &agg_costs);
+               }
+
                /*
                 * Locate any window functions in the tlist.  (We don't need to look
                 * anywhere else, since expressions used in ORDER BY will be in there
@@ -1822,6 +1846,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
                        current_rel = create_grouping_paths(root,
                                                                                                current_rel,
                                                                                                grouping_target,
+                                                                                               &agg_costs,
                                                                                                rollup_lists,
                                                                                                rollup_groupclauses);
                }
@@ -3244,6 +3269,7 @@ estimate_hashagg_tablesize(Path *path, const AggClauseCosts *agg_costs,
  *
  * input_rel: contains the source-data Paths
  * target: the pathtarget for the result Paths to compute
+ * agg_costs: cost info about all aggregates in query (in AGGSPLIT_SIMPLE mode)
  * rollup_lists: list of grouping sets, or NIL if not doing grouping sets
  * rollup_groupclauses: list of grouping clauses for grouping sets,
  *             or NIL if not doing grouping sets
@@ -3260,6 +3286,7 @@ static RelOptInfo *
 create_grouping_paths(PlannerInfo *root,
                                          RelOptInfo *input_rel,
                                          PathTarget *target,
+                                         const AggClauseCosts *agg_costs,
                                          List *rollup_lists,
                                          List *rollup_groupclauses)
 {
@@ -3267,7 +3294,6 @@ create_grouping_paths(PlannerInfo *root,
        Path       *cheapest_path = input_rel->cheapest_total_path;
        RelOptInfo *grouped_rel;
        PathTarget *partial_grouping_target = NULL;
-       AggClauseCosts agg_costs;
        AggClauseCosts agg_partial_costs;       /* parallel only */
        AggClauseCosts agg_final_costs;         /* parallel only */
        Size            hashaggtablesize;
@@ -3364,20 +3390,6 @@ create_grouping_paths(PlannerInfo *root,
                return grouped_rel;
        }
 
-       /*
-        * Collect statistics about aggregates for estimating costs.  Note: we do
-        * not detect duplicate aggregates here; a somewhat-overestimated cost is
-        * okay for our purposes.
-        */
-       MemSet(&agg_costs, 0, sizeof(AggClauseCosts));
-       if (parse->hasAggs)
-       {
-               get_agg_clause_costs(root, (Node *) target->exprs, AGGSPLIT_SIMPLE,
-                                                        &agg_costs);
-               get_agg_clause_costs(root, parse->havingQual, AGGSPLIT_SIMPLE,
-                                                        &agg_costs);
-       }
-
        /*
         * Estimate number of groups.
         */
@@ -3414,7 +3426,7 @@ create_grouping_paths(PlannerInfo *root,
         */
        can_hash = (parse->groupClause != NIL &&
                                parse->groupingSets == NIL &&
-                               agg_costs.numOrderedAggs == 0 &&
+                               agg_costs->numOrderedAggs == 0 &&
                                grouping_is_hashable(parse->groupClause));
 
        /*
@@ -3446,7 +3458,7 @@ create_grouping_paths(PlannerInfo *root,
                /* We don't know how to do grouping sets in parallel. */
                try_parallel_aggregation = false;
        }
-       else if (agg_costs.hasNonPartial || agg_costs.hasNonSerial)
+       else if (agg_costs->hasNonPartial || agg_costs->hasNonSerial)
        {
                /* Insufficient support for partial mode. */
                try_parallel_aggregation = false;
@@ -3627,7 +3639,7 @@ create_grouping_paths(PlannerInfo *root,
                                                                                                  (List *) parse->havingQual,
                                                                                                          rollup_lists,
                                                                                                          rollup_groupclauses,
-                                                                                                         &agg_costs,
+                                                                                                         agg_costs,
                                                                                                          dNumGroups));
                                }
                                else if (parse->hasAggs)
@@ -3645,7 +3657,7 @@ create_grouping_paths(PlannerInfo *root,
                                                                                         AGGSPLIT_SIMPLE,
                                                                                         parse->groupClause,
                                                                                         (List *) parse->havingQual,
-                                                                                        &agg_costs,
+                                                                                        agg_costs,
                                                                                         dNumGroups));
                                }
                                else if (parse->groupClause)
@@ -3727,7 +3739,7 @@ create_grouping_paths(PlannerInfo *root,
        if (can_hash)
        {
                hashaggtablesize = estimate_hashagg_tablesize(cheapest_path,
-                                                                                                         &agg_costs,
+                                                                                                         agg_costs,
                                                                                                          dNumGroups);
 
                /*
@@ -3751,7 +3763,7 @@ create_grouping_paths(PlannerInfo *root,
                                                                         AGGSPLIT_SIMPLE,
                                                                         parse->groupClause,
                                                                         (List *) parse->havingQual,
-                                                                        &agg_costs,
+                                                                        agg_costs,
                                                                         dNumGroups));
                }
 

diff --git a/src/test/regress/expected/limit.out b/src/test/regress/expected/limit.out
index 659a1015b48f375cb3ccc29f9b3afe1fd8523b9b..9c3eecfc3bd0c544b0ba8196444bd9b97cd00430 100644 (file)
--- a/src/test/regress/expected/limit.out
+++ b/src/test/regress/expected/limit.out
@@ -296,3 +296,27 @@ order by s2 desc;
   0 |  0
 (3 rows)
 
+-- test for failure to set all aggregates' aggtranstype
+explain (verbose, costs off)
+select sum(tenthous) as s1, sum(tenthous) + random()*0 as s2
+  from tenk1 group by thousand order by thousand limit 3;
+                                                    QUERY PLAN                                                     
+-------------------------------------------------------------------------------------------------------------------
+ Limit
+   Output: (sum(tenthous)), (((sum(tenthous))::double precision + (random() * '0'::double precision))), thousand
+   ->  GroupAggregate
+         Output: sum(tenthous), ((sum(tenthous))::double precision + (random() * '0'::double precision)), thousand
+         Group Key: tenk1.thousand
+         ->  Index Only Scan using tenk1_thous_tenthous on public.tenk1
+               Output: thousand, tenthous
+(7 rows)
+
+select sum(tenthous) as s1, sum(tenthous) + random()*0 as s2
+  from tenk1 group by thousand order by thousand limit 3;
+  s1   |  s2   
+-------+-------
+ 45000 | 45000
+ 45010 | 45010
+ 45020 | 45020
+(3 rows)
+

diff --git a/src/test/regress/sql/limit.sql b/src/test/regress/sql/limit.sql
index ef5686c520b3c5dbe2d3cecdb4c743efccb9f0ca..8015f81fc2b3b7a3520966ffb7bc4b2af72c4813 100644 (file)
--- a/src/test/regress/sql/limit.sql
+++ b/src/test/regress/sql/limit.sql
@@ -91,3 +91,11 @@ order by s2 desc;
 
 select generate_series(0,2) as s1, generate_series((random()*.1)::int,2) as s2
 order by s2 desc;
+
+-- test for failure to set all aggregates' aggtranstype
+explain (verbose, costs off)
+select sum(tenthous) as s1, sum(tenthous) + random()*0 as s2
+  from tenk1 group by thousand order by thousand limit 3;
+
+select sum(tenthous) as s1, sum(tenthous) + random()*0 as s2
+  from tenk1 group by thousand order by thousand limit 3;