aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--src/backend/optimizer/plan/planner.c55
-rw-r--r--src/test/regress/expected/aggregates.out19
-rw-r--r--src/test/regress/sql/aggregates.sql11
3 files changed, 79 insertions, 6 deletions
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 044fb246665..093ace08644 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -3148,6 +3148,27 @@ reorder_grouping_sets(List *groupingSets, List *sortclause)
}
/*
+ * has_volatile_pathkey
+ * Returns true if any PathKey in 'keys' has an EquivalenceClass
+ * containing a volatile function. Otherwise returns false.
+ */
+static bool
+has_volatile_pathkey(List *keys)
+{
+ ListCell *lc;
+
+ foreach(lc, keys)
+ {
+ PathKey *pathkey = lfirst_node(PathKey, lc);
+
+ if (pathkey->pk_eclass->ec_has_volatile)
+ return true;
+ }
+
+ return false;
+}
+
+/*
* make_pathkeys_for_groupagg
* Determine the pathkeys for the GROUP BY clause and/or any ordered
* aggregates. We expect at least one of these here.
@@ -3168,6 +3189,19 @@ reorder_grouping_sets(List *groupingSets, List *sortclause)
*
* When the best pathkeys are found we also mark each Aggref that can use
* those pathkeys as aggpresorted = true.
+ *
+ * Note: When an aggregate function's ORDER BY / DISTINCT clause contains any
+ * volatile functions, we never make use of these pathkeys. We want to ensure
+ * that sorts using volatile functions are done independently in each Aggref
+ * rather than once at the query level. If we were to allow this then Aggrefs
+ * with compatible sort orders would all transition their rows in the same
+ * order if those pathkeys were deemed to be the best pathkeys to sort on.
+ * Whereas, if some other set of Aggref's pathkeys happened to be deemed
+ * better pathkeys to sort on, then the volatile function Aggrefs would be
+ * left to perform their sorts individually. To avoid this inconsistent
+ * behavior which could make Aggref results depend on what other Aggrefs the
+ * query contains, we always force Aggrefs with volatile functions to perform
+ * their own sorts.
*/
static List *
make_pathkeys_for_groupagg(PlannerInfo *root, List *groupClause, List *tlist,
@@ -3254,20 +3288,33 @@ make_pathkeys_for_groupagg(PlannerInfo *root, List *groupClause, List *tlist,
AggInfo *agginfo = list_nth_node(AggInfo, root->agginfos, i);
Aggref *aggref = linitial_node(Aggref, agginfo->aggrefs);
List *sortlist;
+ List *pathkeys;
if (aggref->aggdistinct != NIL)
sortlist = aggref->aggdistinct;
else
sortlist = aggref->aggorder;
+ pathkeys = make_pathkeys_for_sortclauses(root, sortlist,
+ aggref->args);
+
+ /*
+ * Ignore Aggrefs which have volatile functions in their ORDER BY
+ * or DISTINCT clause.
+ */
+ if (has_volatile_pathkey(pathkeys))
+ {
+ unprocessed_aggs = bms_del_member(unprocessed_aggs, i);
+ continue;
+ }
+
/*
* When not set yet, take the pathkeys from the first unprocessed
* aggregate.
*/
if (currpathkeys == NIL)
{
- currpathkeys = make_pathkeys_for_sortclauses(root, sortlist,
- aggref->args);
+ currpathkeys = pathkeys;
/* include the GROUP BY pathkeys, if they exist */
if (grouppathkeys != NIL)
@@ -3279,11 +3326,7 @@ make_pathkeys_for_groupagg(PlannerInfo *root, List *groupClause, List *tlist,
}
else
{
- List *pathkeys;
-
/* now look for a stronger set of matching pathkeys */
- pathkeys = make_pathkeys_for_sortclauses(root, sortlist,
- aggref->args);
/* include the GROUP BY pathkeys, if they exist */
if (grouppathkeys != NIL)
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 309c2dc8654..ae3b9053318 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1471,6 +1471,25 @@ group by ten;
-> Seq Scan on tenk1
(5 rows)
+-- Ensure that we never choose to provide presorted input to an Aggref with
+-- a volatile function in the ORDER BY / DISTINCT clause. We want to ensure
+-- these sorts are performed individually rather than at the query level.
+explain (costs off)
+select
+ sum(unique1 order by two), sum(unique1 order by four),
+ sum(unique1 order by four, two), sum(unique1 order by two, random()),
+ sum(unique1 order by two, random(), random() + 1)
+from tenk1
+group by ten;
+ QUERY PLAN
+----------------------------------
+ GroupAggregate
+ Group Key: ten
+ -> Sort
+ Sort Key: ten, four, two
+ -> Seq Scan on tenk1
+(5 rows)
+
-- Ensure no ordering is requested when enable_presorted_aggregate is off
set enable_presorted_aggregate to off;
explain (costs off)
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 15f6be8e15a..514e3b2b397 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -546,6 +546,17 @@ select
from tenk1
group by ten;
+-- Ensure that we never choose to provide presorted input to an Aggref with
+-- a volatile function in the ORDER BY / DISTINCT clause. We want to ensure
+-- these sorts are performed individually rather than at the query level.
+explain (costs off)
+select
+ sum(unique1 order by two), sum(unique1 order by four),
+ sum(unique1 order by four, two), sum(unique1 order by two, random()),
+ sum(unique1 order by two, random(), random() + 1)
+from tenk1
+group by ten;
+
-- Ensure no ordering is requested when enable_presorted_aggregate is off
set enable_presorted_aggregate to off;
explain (costs off)