Prevent sharing transition states between ordered-set aggregates.

This ought to work, but the built-in OSAs are not capable of coping,
because their final-functions destructively modify their transition
state (specifically, the contained tuplesort object).  That was fine
when those functions were written, but commit 804163bc2 moved the
goalposts without telling orderedsetaggs.c.

We should fix the built-in OSAs to support this, but it will take
a little work, especially if we don't want to sacrifice performance
in the normal non-shared-state case.  Given that it took a year after
9.6 release for anyone to notice this bug, we should not prioritize
sharable-state over nonsharable-state performance.  And a proper fix
is likely to be more complicated than we'd want to back-patch, too.

Therefore, let's just put in this stop-gap patch to prevent nodeAgg.c
from choosing to use shared state for OSAs.  We can revert it in HEAD
when we get a better fix.

Report from Lukas Eder, diagnosis by me, patch by David Rowley.
Back-patch to 9.6 where the problem was introduced.

Discussion: https://postgr.es/m/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 8c6aa9a13e86456449d81b4ad7acfe7952cebd84..ed9e8b0396fafefc5b6b3d13cad722c9dd2352a3 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -3372,6 +3372,16 @@ find_compatible_pertrans(AggState *aggstate, Aggref *newagg,
 {
        ListCell   *lc;
 
+       /*
+        * For the moment, never try to share transition states between different
+        * ordered-set aggregates.  This is necessary because the finalfns of the
+        * built-in OSAs (see orderedsetaggs.c) are destructive of their
+        * transition states.  We should fix them so we can allow this, but not
+        * losing performance in the normal non-shared case will take some work.
+        */
+       if (AGGKIND_IS_ORDERED_SET(newagg->aggkind))
+               return -1;
+
        foreach(lc, transnos)
        {
                int                     transno = lfirst_int(lc);

diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index fa1f5e787988f7f02fbc095df7359c91c131ef00..c88a4e45687c971f3145ecd9fd396e1c407a6c12 100644 (file)
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1857,6 +1857,25 @@ NOTICE:  avg_transfn called with 3
       2 |      6
 (1 row)
 
+-- ideally these would share state, but we have to fix the OSAs first.
+select
+  percentile_cont(0.5) within group (order by a),
+  percentile_disc(0.5) within group (order by a)
+from (values(1::float8),(3),(5),(7)) t(a);
+ percentile_cont | percentile_disc 
+-----------------+-----------------
+               4 |               3
+(1 row)
+
+select
+  rank(4) within group (order by a),
+  dense_rank(4) within group (order by a)
+from (values(1),(3),(5),(7)) t(a);
+ rank | dense_rank 
+------+------------
+    3 |          3
+(1 row)
+
 -- test that aggs with the same sfunc and initcond share the same agg state
 create aggregate my_sum_init(int4)
 (

diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 2eeb3eedbdf543dc565f40e784fa582efe6cb985..77314522eb986b82f4d511e6220fa5255ad49013 100644 (file)
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -739,6 +739,17 @@ select my_avg(one) filter (where one > 1),my_sum(one) from (values(1),(3)) t(one
 -- this should not share the state due to different input columns.
 select my_avg(one),my_sum(two) from (values(1,2),(3,4)) t(one,two);
 
+-- ideally these would share state, but we have to fix the OSAs first.
+select
+  percentile_cont(0.5) within group (order by a),
+  percentile_disc(0.5) within group (order by a)
+from (values(1::float8),(3),(5),(7)) t(a);
+
+select
+  rank(4) within group (order by a),
+  dense_rank(4) within group (order by a)
+from (values(1),(3),(5),(7)) t(a);
+
 -- test that aggs with the same sfunc and initcond share the same agg state
 create aggregate my_sum_init(int4)
 (