From db80acfc9d50ac56811d22802ab3d822ab313055 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 20 Dec 2016 09:20:17 +0200 Subject: [PATCH] Fix sharing Agg transition state of DISTINCT or ordered aggs. If a query contained two aggregates that could share the transition value, we would correctly collect the input into a tuplesort only once, but incorrectly run the transition function over the accumulated input twice, in finalize_aggregates(). That caused a crash, when we tried to call tuplesort_performsort() on an already-freed NULL tuplestore. Backport to 9.6, where sharing of transition state and this bug were introduced. Analysis by Tom Lane. Discussion: https://www.postgresql.org/message-id/ac5b0b69-744c-9114-6218-8300ac920e61@iki.fi --- src/backend/executor/nodeAgg.c | 21 ++++++++++++++++++--- src/test/regress/expected/aggregates.out | 9 +++++++++ src/test/regress/sql/aggregates.sql | 3 +++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index a093862f34..781938e2bb 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1575,16 +1575,19 @@ finalize_aggregates(AggState *aggstate, Datum *aggvalues = econtext->ecxt_aggvalues; bool *aggnulls = econtext->ecxt_aggnulls; int aggno; + int transno; Assert(currentSet == 0 || ((Agg *) aggstate->ss.ps.plan)->aggstrategy != AGG_HASHED); aggstate->current_set = currentSet; - for (aggno = 0; aggno < aggstate->numaggs; aggno++) + /* + * If there were any DISTINCT and/or ORDER BY aggregates, sort their + * inputs and run the transition functions. + */ + for (transno = 0; transno < aggstate->numtrans; transno++) { - AggStatePerAgg peragg = &peraggs[aggno]; - int transno = peragg->transno; AggStatePerTrans pertrans = &aggstate->pertrans[transno]; AggStatePerGroup pergroupstate; @@ -1603,6 +1606,18 @@ finalize_aggregates(AggState *aggstate, pertrans, pergroupstate); } + } + + /* + * Run the final functions. + */ + for (aggno = 0; aggno < aggstate->numaggs; aggno++) + { + AggStatePerAgg peragg = &peraggs[aggno]; + int transno = peragg->transno; + AggStatePerGroup pergroupstate; + + pergroupstate = &pergroup[transno + (currentSet * (aggstate->numtrans))]; if (DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit)) finalize_partialaggregate(aggstate, peragg, pergroupstate, diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 45208a6da6..fa1f5e7879 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1816,6 +1816,15 @@ NOTICE: avg_transfn called with 3 2 | 4 (1 row) +-- same as previous one, but with DISTINCT, which requires sorting the input. +select my_avg(distinct one),my_sum(distinct one) from (values(1),(3),(1)) t(one); +NOTICE: avg_transfn called with 1 +NOTICE: avg_transfn called with 3 + my_avg | my_sum +--------+-------- + 2 | 4 +(1 row) + -- shouldn't share states due to the distinctness not matching. select my_avg(distinct one),my_sum(one) from (values(1),(3)) t(one); NOTICE: avg_transfn called with 1 diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 430ac49385..2eeb3eedbd 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -727,6 +727,9 @@ select my_avg(one),my_avg(one) from (values(1),(3)) t(one); -- aggregate state should be shared as transfn is the same for both aggs. select my_avg(one),my_sum(one) from (values(1),(3)) t(one); +-- same as previous one, but with DISTINCT, which requires sorting the input. +select my_avg(distinct one),my_sum(distinct one) from (values(1),(3),(1)) t(one); + -- shouldn't share states due to the distinctness not matching. select my_avg(distinct one),my_sum(one) from (values(1),(3)) t(one);