Thread: [BUGS] Combination of ordered-set aggregate function terminates JDBCconnection on PostgreSQL 9.6.5

Pantelis Theodosiou <ypercube@gmail.com> writes:
> - it can be seen with only rank and dense_rank, with any order by (asc,
> desc, null):
> select
>   rank(1) within group (order by a),
>   dense_rank(1) within group (order by a)
> from (values (1)) t(a) ;

Check ...

> - but it doesn't happen if   (values (1)) is replaced with a single row
> table.

It did for me.  I'm using a debug-enabled build, which typically helps
to make this sort of thing more reproducible.

regression=# create table t(a int) ;
CREATE TABLE
regression=# insert into t values(1);
INSERT 0 1
regression=# select rank(1) within group (order by a), dense_rank(1) within group (order by a)
from t;                 
server closed the connection unexpectedly
        regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Lukas Eder <lukas.eder@gmail.com> writes:
> [ this crashes: ]
> select
>   cume_dist(1) within group (order by a desc),
>   rank(1) within group (order by a desc),
>   dense_rank(1) within group (order by a asc),
>   percent_rank(1) within group (order by a asc)
> from (values(1)) t(a);
>
> The issue depends on a certain set of combinations of the above function
> calls. Each function can be called individually without problems. Some
> functions can be combined without problems as well.

So the problem arises when nodeAgg.c decides it can combine the transition
calculations for two different ordered-set aggregates, leading to the
final functions for those OSAs being invoked successively on the same
transition state.  The finalfns are not expecting that and the second
one crashes.  (Concretely, this happens because osastate->sortstate
has already been reset to null, after closing down the contained
tuplesort object.)

It seems like this is probably fixable by having the finalfns not do
tuplesort_end immediately, but rather track whether anyone's yet
done the sort, and then do something like
if (already_sorted)    tuplesort_rescan(osastate->sortstate);else    tuplesort_performsort(osastate->sortstate);

However, in order to make use of tuplesort_rescan, we'd have had
to pass randomAccess = true to tuplesort_begin_xxx, which would
be rather an annoying overhead for the majority case where there
isn't a potential for reuse.

What I think we should do about this is introduce another aggregate
API function, a bit like AggGetAggref or AggCheckCallContext,
that an aggregate function could call to find out whether there is
any possibility of multiple invocation of finalfns on the same
transition state.  For the moment I'd just be worried about making
it work for ordered-set aggs, which are the only case where we don't
(er, didn't) require that to work anyway.  But potentially we could
extend it to work for all agg cases and then finalfns could be
optimized in cases where it's safe for them to be destructive
of the transition state value.

Speaking of AggGetAggref, there's another thing that I think 804163bc2
did wrong for ordered-set aggregates: it can return the wrong Aggref
when two aggregates' intermediate states have been merged.  I do not
think it's appropriate to say "well, you shouldn't care which of the
Aggrefs you get".  It looks like this accidentally fails to fail
for the current OSAs, because indeed they do only look at the input-
related fields of the Aggref, but surely that's not something to
rely on.  It's most certainly not acceptable that the function's
documentation doesn't mention that its result may be a lie.

This might be a bigger change than we want to push into the back
branches.  In that case probably a back-patchable fix is to hack
nodeAgg.c so it will never combine input states for OSAs.
        regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 12 October 2017 at 08:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So the problem arises when nodeAgg.c decides it can combine the transition
> calculations for two different ordered-set aggregates, leading to the
> final functions for those OSAs being invoked successively on the same
> transition state.  The finalfns are not expecting that and the second
> one crashes.  (Concretely, this happens because osastate->sortstate
> has already been reset to null, after closing down the contained
> tuplesort object.)

Hmm, yeah. It was all coded with the assumption that final functions
never modify the transaction state.

> It seems like this is probably fixable by having the finalfns not do
> tuplesort_end immediately, but rather track whether anyone's yet
> done the sort, and then do something like
>
>         if (already_sorted)
>                 tuplesort_rescan(osastate->sortstate);
>         else
>                 tuplesort_performsort(osastate->sortstate);
>
> However, in order to make use of tuplesort_rescan, we'd have had
> to pass randomAccess = true to tuplesort_begin_xxx, which would
> be rather an annoying overhead for the majority case where there
> isn't a potential for reuse.
>
> What I think we should do about this is introduce another aggregate
> API function, a bit like AggGetAggref or AggCheckCallContext,
> that an aggregate function could call to find out whether there is
> any possibility of multiple invocation of finalfns on the same
> transition state.  For the moment I'd just be worried about making
> it work for ordered-set aggs, which are the only case where we don't
> (er, didn't) require that to work anyway.  But potentially we could
> extend it to work for all agg cases and then finalfns could be
> optimized in cases where it's safe for them to be destructive
> of the transition state value.

Yeah maybe if core tracked the total number of references in
AggStatePerTrans, and finalize_aggregate() incremented another counter
to track how many times the final function had been called on this
state, then if there was some way to expose that information to the
final function, it would know if it was the first or the last final
function to use the state. I'm just not sure how much we'd want to
allow the final function to see. Would we expose the whole
AggStatePerTrans? or just invent API functions to allow them to know
if they're the first or last?

> Speaking of AggGetAggref, there's another thing that I think 804163bc2
> did wrong for ordered-set aggregates: it can return the wrong Aggref
> when two aggregates' intermediate states have been merged.  I do not
> think it's appropriate to say "well, you shouldn't care which of the
> Aggrefs you get".  It looks like this accidentally fails to fail
> for the current OSAs, because indeed they do only look at the input-
> related fields of the Aggref, but surely that's not something to
> rely on.  It's most certainly not acceptable that the function's
> documentation doesn't mention that its result may be a lie.
>
> This might be a bigger change than we want to push into the back
> branches.  In that case probably a back-patchable fix is to hack
> nodeAgg.c so it will never combine input states for OSAs.

I've attached a patch which does this.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

David Rowley <david.rowley@2ndquadrant.com> writes:
> On 12 October 2017 at 08:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, in order to make use of tuplesort_rescan, we'd have had
>> to pass randomAccess = true to tuplesort_begin_xxx, which would
>> be rather an annoying overhead for the majority case where there
>> isn't a potential for reuse.

> Yeah maybe if core tracked the total number of references in
> AggStatePerTrans, and finalize_aggregate() incremented another counter
> to track how many times the final function had been called on this
> state, then if there was some way to expose that information to the
> final function, it would know if it was the first or the last final
> function to use the state.

That seems kind of irrelevant, at least for the existing OSAs.
To know what value of randomAccess to pass to the tuplesort setup,
we have to know *at the first transition-function call* whether
there may be multiple final-function calls coming up.  So what
what I'm imagining is a simple boolean result "yes, there will be
only one finalfn call, so it can destructively modify the transition
state", or "there might be more than one finalfn call, so the finalfn(s)
must preserve transition state".  And this info has to be available
throughout the aggregate run.

>> This might be a bigger change than we want to push into the back
>> branches.  In that case probably a back-patchable fix is to hack
>> nodeAgg.c so it will never combine input states for OSAs.

> I've attached a patch which does this.

Needs to reject plain OSAs too, not just hypotheticals.  Pushed
with that fix and some test cases.
        regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

On 10/12/2017 05:27 AM, Tom Lane wrote:
>>> This might be a bigger change than we want to push into the back
>>> branches.  In that case probably a back-patchable fix is to hw ck
>>> nodeAgg.c so it will never combine input states for OSAs.
> 
>> I've attached a patch which does this.
> 
> Needs to reject plain OSAs too, not just hypotheticals.  Pushed
> with that fix and some test cases.

Thanks!

> Speaking of AggGetAggref, there's another thing that I think 804163bc2
> did wrong for ordered-set aggregates: it can return the wrong Aggref
> when two aggregates' intermediate states have been merged.  I do not
> think it's appropriate to say "well, you shouldn't care which of the
> Aggrefs you get".  It looks like this accidentally fails to fail
> for the current OSAs, because indeed they do only look at the input-
> related fields of the Aggref, but surely that's not something to
> rely on.  It's most certainly not acceptable that the function's
> documentation doesn't mention that its result may be a lie.

Hmm. All the fields except for aggfnoid, aggtype and aggcollid are 
related to the inputs or the transition function, so all the other 
fields would be the same between two shared transition states. But yes, 
this really should be documented. Perhaps AggGetAggref() should return 
an Aggref with those fields set to InvalidOid, to make it clear that 
they should not be looked at?

Conceivably we could have another function like AggGetAggref() that 
returns all of the Aggrefs. But I don't think it's worth the 
complication. If the transition function needs to do something different 
depending on the aggregate it's for, well, don't do that. Define a 
different transition function for both aggregates.

- Heikki


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 10/12/2017 05:27 AM, Tom Lane wrote:
>> Speaking of AggGetAggref, there's another thing that I think 804163bc2
>> did wrong for ordered-set aggregates: it can return the wrong Aggref
>> when two aggregates' intermediate states have been merged.

> Conceivably we could have another function like AggGetAggref() that 
> returns all of the Aggrefs. But I don't think it's worth the 
> complication. If the transition function needs to do something different 
> depending on the aggregate it's for, well, don't do that. Define a 
> different transition function for both aggregates.

Thinking about it more clearly, if a transition function is being run
on behalf of several different Aggrefs (with identical input states),
then no, the transition function should not care which Aggref it
looks at.  As you say it mustn't do anything different on the basis of
the finalfn-related fields.  The problem occurs when a finalfn calls
AggGetAggref --- then, I think that the finalfn is entirely entitled
to expect that it will see its own Aggref, not some other one that
happens to share input+transition.

So the issue is that we need some different behavior during
finalize_aggregate than during the transition function calls.
I'm inclined to put back the curperagg field we had before,
and populate that during finalize_aggregate.  Then, AggGetAggref
would look at either curperagg or curpertrans, whichever is set.
        regards, tom lane


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

I wrote:
> Thinking about it more clearly, if a transition function is being run
> on behalf of several different Aggrefs (with identical input states),
> then no, the transition function should not care which Aggref it
> looks at.  As you say it mustn't do anything different on the basis of
> the finalfn-related fields.  The problem occurs when a finalfn calls
> AggGetAggref --- then, I think that the finalfn is entirely entitled
> to expect that it will see its own Aggref, not some other one that
> happens to share input+transition.

Concretely, I think we need to do the attached.  This seems like
a bug fix to me, so I'm inclined to back-patch it.  In the back
branches we could put the extra AggState field at the end, to
minimize ABI-break hazards.

BTW ... I was quite surprised to notice that the aggdirectargs
are treated as a property that has to be matched in order to
combine transition states.  They aren't used during the transition
phase, so this seems like a pointless constraint.  We could move
the aggdirectargs ExprState list to AggStatePerAggData and treat
the aggdirectargs as part of the finalfn-related data, instead.
As long as we're not merging states at all for OSAs, this is
moot, but it seems like something to fix along with that.

            regards, tom lane

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 1a1aebe..c804a61 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*************** finalize_aggregate(AggState *aggstate,
*** 1528,1535 ****
      {
          int            numFinalArgs = peragg->numFinalArgs;

!         /* set up aggstate->curpertrans for AggGetAggref() */
!         aggstate->curpertrans = pertrans;

          InitFunctionCallInfoData(fcinfo, &peragg->finalfn,
                                   numFinalArgs,
--- 1528,1535 ----
      {
          int            numFinalArgs = peragg->numFinalArgs;

!         /* set up aggstate->curperagg for AggGetAggref() */
!         aggstate->curperagg = peragg;

          InitFunctionCallInfoData(fcinfo, &peragg->finalfn,
                                   numFinalArgs,
*************** finalize_aggregate(AggState *aggstate,
*** 1562,1568 ****
              *resultVal = FunctionCallInvoke(&fcinfo);
              *resultIsNull = fcinfo.isnull;
          }
!         aggstate->curpertrans = NULL;
      }
      else
      {
--- 1562,1568 ----
              *resultVal = FunctionCallInvoke(&fcinfo);
              *resultIsNull = fcinfo.isnull;
          }
!         aggstate->curperagg = NULL;
      }
      else
      {
*************** ExecInitAgg(Agg *node, EState *estate, i
*** 2712,2717 ****
--- 2712,2718 ----
      aggstate->current_set = 0;
      aggstate->peragg = NULL;
      aggstate->pertrans = NULL;
+     aggstate->curperagg = NULL;
      aggstate->curpertrans = NULL;
      aggstate->input_done = false;
      aggstate->agg_done = false;
*************** AggCheckCallContext(FunctionCallInfo fci
*** 4070,4075 ****
--- 4071,4083 ----
   * If the function is being called as an aggregate support function,
   * return the Aggref node for the aggregate call.  Otherwise, return NULL.
   *
+  * Aggregates sharing the same inputs and transition functions can get
+  * merged into a single transition calculation.  If the transition function
+  * calls AggGetAggref, it will get some one of the Aggrefs for which it is
+  * executing.  It must therefore not pay attention to the Aggref fields that
+  * relate to the final function, as those are indeterminate.  But if a final
+  * function calls AggGetAggref, it will get a precise result.
+  *
   * Note that if an aggregate is being used as a window function, this will
   * return NULL.  We could provide a similar function to return the relevant
   * WindowFunc node in such cases, but it's not needed yet.
*************** AggGetAggref(FunctionCallInfo fcinfo)
*** 4079,4086 ****
--- 4087,4102 ----
  {
      if (fcinfo->context && IsA(fcinfo->context, AggState))
      {
+         AggStatePerAgg curperagg;
          AggStatePerTrans curpertrans;

+         /* check curperagg (valid when in a final function) */
+         curperagg = ((AggState *) fcinfo->context)->curperagg;
+
+         if (curperagg)
+             return curperagg->aggref;
+
+         /* check curpertrans (valid when in a transition function) */
          curpertrans = ((AggState *) fcinfo->context)->curpertrans;

          if (curpertrans)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index c461134..d4ce8d8 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct AggState
*** 1808,1814 ****
      ExprContext **aggcontexts;    /* econtexts for long-lived data (per GS) */
      ExprContext *tmpcontext;    /* econtext for input expressions */
      ExprContext *curaggcontext; /* currently active aggcontext */
!     AggStatePerTrans curpertrans;    /* currently active trans state */
      bool        input_done;        /* indicates end of input */
      bool        agg_done;        /* indicates completion of Agg scan */
      int            projected_set;    /* The last projected grouping set */
--- 1808,1815 ----
      ExprContext **aggcontexts;    /* econtexts for long-lived data (per GS) */
      ExprContext *tmpcontext;    /* econtext for input expressions */
      ExprContext *curaggcontext; /* currently active aggcontext */
!     AggStatePerAgg curperagg;    /* currently active aggregate, if any */
!     AggStatePerTrans curpertrans;    /* currently active trans state, if any */
      bool        input_done;        /* indicates end of input */
      bool        agg_done;        /* indicates completion of Agg scan */
      int            projected_set;    /* The last projected grouping set */

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

I wrote:
> To know what value of randomAccess to pass to the tuplesort setup,
> we have to know *at the first transition-function call* whether
> there may be multiple final-function calls coming up.  So what
> what I'm imagining is a simple boolean result "yes, there will be
> only one finalfn call, so it can destructively modify the transition
> state", or "there might be more than one finalfn call, so the finalfn(s)
> must preserve transition state".  And this info has to be available
> throughout the aggregate run.

Attached is a proposed patch to make the ordered-set aggregates
safe for state merging.  I've not tested it really thoroughly,
but it passes the regression cases added in 52328727b.

            regards, tom lane

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 40d8ec9..1718285 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*************** typedef struct AggStatePerTransData
*** 255,260 ****
--- 255,265 ----
      Aggref       *aggref;

      /*
+      * Is this state value actually being shared by more than one Aggref?
+      */
+     bool        aggshared;
+
+     /*
       * Nominal number of arguments for aggregate function.  For plain aggs,
       * this excludes any ORDER BY expressions.  For ordered-set aggs, this
       * counts both the direct and aggregated (ORDER BY) arguments.
*************** ExecInitAgg(Agg *node, EState *estate, i
*** 3345,3353 ****
          {
              /*
               * Existing compatible trans found, so just point the 'peragg' to
!              * the same per-trans struct.
               */
              pertrans = &pertransstates[existing_transno];
              peragg->transno = existing_transno;
          }
          else
--- 3350,3359 ----
          {
              /*
               * Existing compatible trans found, so just point the 'peragg' to
!              * the same per-trans struct, and mark the trans state as shared.
               */
              pertrans = &pertransstates[existing_transno];
+             pertrans->aggshared = true;
              peragg->transno = existing_transno;
          }
          else
*************** build_pertrans_for_aggref(AggStatePerTra
*** 3449,3454 ****
--- 3455,3461 ----

      /* Begin filling in the pertrans data */
      pertrans->aggref = aggref;
+     pertrans->aggshared = false;
      pertrans->aggCollation = aggref->inputcollid;
      pertrans->transfn_oid = aggtransfn;
      pertrans->serialfn_oid = aggserialfn;
*************** AggGetAggref(FunctionCallInfo fcinfo)
*** 4105,4121 ****
  {
      if (fcinfo->context && IsA(fcinfo->context, AggState))
      {
          AggStatePerAgg curperagg;
          AggStatePerTrans curpertrans;

          /* check curperagg (valid when in a final function) */
!         curperagg = ((AggState *) fcinfo->context)->curperagg;

          if (curperagg)
              return curperagg->aggref;

          /* check curpertrans (valid when in a transition function) */
!         curpertrans = ((AggState *) fcinfo->context)->curpertrans;

          if (curpertrans)
              return curpertrans->aggref;
--- 4112,4129 ----
  {
      if (fcinfo->context && IsA(fcinfo->context, AggState))
      {
+         AggState   *aggstate = (AggState *) fcinfo->context;
          AggStatePerAgg curperagg;
          AggStatePerTrans curpertrans;

          /* check curperagg (valid when in a final function) */
!         curperagg = aggstate->curperagg;

          if (curperagg)
              return curperagg->aggref;

          /* check curpertrans (valid when in a transition function) */
!         curpertrans = aggstate->curpertrans;

          if (curpertrans)
              return curpertrans->aggref;
*************** AggGetTempMemoryContext(FunctionCallInfo
*** 4146,4151 ****
--- 4154,4197 ----
  }

  /*
+  * AggStateIsShared - find out whether transition state is shared
+  *
+  * If the function is being called as an aggregate support function,
+  * return TRUE if the aggregate's transition state is shared across
+  * multiple aggregates, FALSE if it is not.
+  *
+  * Returns TRUE if not called as an aggregate support function.
+  * This is intended as a conservative answer, ie "no you'd better not
+  * scribble on your input".  In particular, will return TRUE if the
+  * aggregate is being used as a window function, which is a scenario
+  * in which changing the transition state is a bad idea.  We might
+  * want to refine the behavior for the window case in future.
+  */
+ bool
+ AggStateIsShared(FunctionCallInfo fcinfo)
+ {
+     if (fcinfo->context && IsA(fcinfo->context, AggState))
+     {
+         AggState   *aggstate = (AggState *) fcinfo->context;
+         AggStatePerAgg curperagg;
+         AggStatePerTrans curpertrans;
+
+         /* check curperagg (valid when in a final function) */
+         curperagg = aggstate->curperagg;
+
+         if (curperagg)
+             return aggstate->pertrans[curperagg->transno].aggshared;
+
+         /* check curpertrans (valid when in a transition function) */
+         curpertrans = aggstate->curpertrans;
+
+         if (curpertrans)
+             return curpertrans->aggshared;
+     }
+     return true;
+ }
+
+ /*
   * AggRegisterCallback - register a cleanup callback for an aggregate
   *
   * This is useful for aggs to register shutdown callbacks, which will ensure
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 25905a3..1e323d9 100644
*** a/src/backend/utils/adt/orderedsetaggs.c
--- b/src/backend/utils/adt/orderedsetaggs.c
***************
*** 40,53 ****
   * create just once per query because they will not change across groups.
   * The per-query struct and subsidiary data live in the executor's per-query
   * memory context, and go away implicitly at ExecutorEnd().
   */

  typedef struct OSAPerQueryState
  {
!     /* Aggref for this aggregate: */
      Aggref       *aggref;
      /* Memory context containing this struct and other per-query data: */
      MemoryContext qcontext;

      /* These fields are used only when accumulating tuples: */

--- 40,61 ----
   * create just once per query because they will not change across groups.
   * The per-query struct and subsidiary data live in the executor's per-query
   * memory context, and go away implicitly at ExecutorEnd().
+  *
+  * These structs are set up during the first call of the transition function.
+  * Because we allow nodeAgg.c to merge ordered-set aggregates (but not
+  * hypothetical aggregates) with identical inputs and transition functions,
+  * this info must not depend on the particular aggregate (ie, particular
+  * final-function), nor on the direct argument(s) of the aggregate.
   */

  typedef struct OSAPerQueryState
  {
!     /* Representative Aggref for this aggregate: */
      Aggref       *aggref;
      /* Memory context containing this struct and other per-query data: */
      MemoryContext qcontext;
+     /* Do we expect multiple final-function calls within one group? */
+     bool        rescan_needed;

      /* These fields are used only when accumulating tuples: */

*************** typedef struct OSAPerGroupState
*** 91,96 ****
--- 99,106 ----
      Tuplesortstate *sortstate;
      /* Number of normal rows inserted into sortstate: */
      int64        number_of_rows;
+     /* Have we already done tuplesort_performsort? */
+     bool        sort_done;
  } OSAPerGroupState;

  static void ordered_set_shutdown(Datum arg);
*************** ordered_set_startup(FunctionCallInfo fci
*** 146,151 ****
--- 156,164 ----
          qstate->aggref = aggref;
          qstate->qcontext = qcontext;

+         /* We need to support rescans if the trans state is shared */
+         qstate->rescan_needed = AggStateIsShared(fcinfo);
+
          /* Extract the sort information */
          sortlist = aggref->aggorder;
          numSortCols = list_length(sortlist);
*************** ordered_set_startup(FunctionCallInfo fci
*** 277,291 ****
                                                     qstate->sortOperators,
                                                     qstate->sortCollations,
                                                     qstate->sortNullsFirsts,
!                                                    work_mem, false);
      else
          osastate->sortstate = tuplesort_begin_datum(qstate->sortColType,
                                                      qstate->sortOperator,
                                                      qstate->sortCollation,
                                                      qstate->sortNullsFirst,
!                                                     work_mem, false);

      osastate->number_of_rows = 0;

      /* Now register a shutdown callback to clean things up at end of group */
      AggRegisterCallback(fcinfo,
--- 290,307 ----
                                                     qstate->sortOperators,
                                                     qstate->sortCollations,
                                                     qstate->sortNullsFirsts,
!                                                    work_mem,
!                                                    qstate->rescan_needed);
      else
          osastate->sortstate = tuplesort_begin_datum(qstate->sortColType,
                                                      qstate->sortOperator,
                                                      qstate->sortCollation,
                                                      qstate->sortNullsFirst,
!                                                     work_mem,
!                                                     qstate->rescan_needed);

      osastate->number_of_rows = 0;
+     osastate->sort_done = false;

      /* Now register a shutdown callback to clean things up at end of group */
      AggRegisterCallback(fcinfo,
*************** ordered_set_startup(FunctionCallInfo fci
*** 306,319 ****
   * group) by ExecutorEnd.  But we must take care to release any potential
   * non-memory resources.
   *
!  * This callback is arguably unnecessary, since we don't support use of
!  * ordered-set aggs in AGG_HASHED mode and there is currently no non-error
!  * code path in non-hashed modes wherein nodeAgg.c won't call the finalfn
!  * after calling the transfn one or more times.  So in principle we could rely
!  * on the finalfn to delete the tuplestore etc.  However, it's possible that
!  * such a code path might exist in future, and in any case it'd be
!  * notationally tedious and sometimes require extra data copying to ensure
!  * we always delete the tuplestore in the finalfn.
   */
  static void
  ordered_set_shutdown(Datum arg)
--- 322,333 ----
   * group) by ExecutorEnd.  But we must take care to release any potential
   * non-memory resources.
   *
!  * In the case where we're not expecting multiple finalfn calls, we could
!  * arguably rely on the finalfn to clean up; but it's easier and more testable
!  * if we just do it the same way in either case.  Note that many of the
!  * finalfns could *not* free the tuplesort object, at least not without extra
!  * data copying, because what they return is a pointer to a datum inside the
!  * tuplesort object.
   */
  static void
  ordered_set_shutdown(Datum arg)
*************** percentile_disc_final(PG_FUNCTION_ARGS)
*** 436,443 ****
      if (osastate->number_of_rows == 0)
          PG_RETURN_NULL();

!     /* Finish the sort */
!     tuplesort_performsort(osastate->sortstate);

      /*----------
       * We need the smallest K such that (K/N) >= percentile.
--- 450,463 ----
      if (osastate->number_of_rows == 0)
          PG_RETURN_NULL();

!     /* Finish the sort, or rescan if we already did */
!     if (!osastate->sort_done)
!     {
!         tuplesort_performsort(osastate->sortstate);
!         osastate->sort_done = true;
!     }
!     else
!         tuplesort_rescan(osastate->sortstate);

      /*----------
       * We need the smallest K such that (K/N) >= percentile.
*************** percentile_disc_final(PG_FUNCTION_ARGS)
*** 457,469 ****
      if (!tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, NULL))
          elog(ERROR, "missing row in percentile_disc");

-     /*
-      * Note: we *cannot* clean up the tuplesort object here, because the value
-      * to be returned is allocated inside its sortcontext.  We could use
-      * datumCopy to copy it out of there, but it doesn't seem worth the
-      * trouble, since the cleanup callback will clear the tuplesort later.
-      */
-
      /* We shouldn't have stored any nulls, but do the right thing anyway */
      if (isnull)
          PG_RETURN_NULL();
--- 477,482 ----
*************** percentile_cont_final_common(FunctionCal
*** 543,550 ****

      Assert(expect_type == osastate->qstate->sortColType);

!     /* Finish the sort */
!     tuplesort_performsort(osastate->sortstate);

      first_row = floor(percentile * (osastate->number_of_rows - 1));
      second_row = ceil(percentile * (osastate->number_of_rows - 1));
--- 556,569 ----

      Assert(expect_type == osastate->qstate->sortColType);

!     /* Finish the sort, or rescan if we already did */
!     if (!osastate->sort_done)
!     {
!         tuplesort_performsort(osastate->sortstate);
!         osastate->sort_done = true;
!     }
!     else
!         tuplesort_rescan(osastate->sortstate);

      first_row = floor(percentile * (osastate->number_of_rows - 1));
      second_row = ceil(percentile * (osastate->number_of_rows - 1));
*************** percentile_cont_final_common(FunctionCal
*** 575,587 ****
          val = lerpfunc(first_val, second_val, proportion);
      }

-     /*
-      * Note: we *cannot* clean up the tuplesort object here, because the value
-      * to be returned may be allocated inside its sortcontext.  We could use
-      * datumCopy to copy it out of there, but it doesn't seem worth the
-      * trouble, since the cleanup callback will clear the tuplesort later.
-      */
-
      PG_RETURN_DATUM(val);
  }

--- 594,599 ----
*************** percentile_disc_multi_final(PG_FUNCTION_
*** 779,786 ****
       */
      if (i < num_percentiles)
      {
!         /* Finish the sort */
!         tuplesort_performsort(osastate->sortstate);

          for (; i < num_percentiles; i++)
          {
--- 791,804 ----
       */
      if (i < num_percentiles)
      {
!         /* Finish the sort, or rescan if we already did */
!         if (!osastate->sort_done)
!         {
!             tuplesort_performsort(osastate->sortstate);
!             osastate->sort_done = true;
!         }
!         else
!             tuplesort_rescan(osastate->sortstate);

          for (; i < num_percentiles; i++)
          {
*************** percentile_disc_multi_final(PG_FUNCTION_
*** 804,814 ****
          }
      }

-     /*
-      * We could clean up the tuplesort object after forming the array, but
-      * probably not worth the trouble.
-      */
-
      /* We make the output array the same shape as the input */
      PG_RETURN_POINTER(construct_md_array(result_datum, result_isnull,
                                           ARR_NDIM(param),
--- 822,827 ----
*************** percentile_cont_multi_final_common(Funct
*** 902,909 ****
       */
      if (i < num_percentiles)
      {
!         /* Finish the sort */
!         tuplesort_performsort(osastate->sortstate);

          for (; i < num_percentiles; i++)
          {
--- 915,928 ----
       */
      if (i < num_percentiles)
      {
!         /* Finish the sort, or rescan if we already did */
!         if (!osastate->sort_done)
!         {
!             tuplesort_performsort(osastate->sortstate);
!             osastate->sort_done = true;
!         }
!         else
!             tuplesort_rescan(osastate->sortstate);

          for (; i < num_percentiles; i++)
          {
*************** percentile_cont_multi_final_common(Funct
*** 962,972 ****
          }
      }

-     /*
-      * We could clean up the tuplesort object after forming the array, but
-      * probably not worth the trouble.
-      */
-
      /* We make the output array the same shape as the input */
      PG_RETURN_POINTER(construct_md_array(result_datum, result_isnull,
                                           ARR_NDIM(param),
--- 981,986 ----
*************** mode_final(PG_FUNCTION_ARGS)
*** 1043,1050 ****

      shouldfree = !(osastate->qstate->typByVal);

!     /* Finish the sort */
!     tuplesort_performsort(osastate->sortstate);

      /* Scan tuples and count frequencies */
      while (tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, &abbrev_val))
--- 1057,1070 ----

      shouldfree = !(osastate->qstate->typByVal);

!     /* Finish the sort, or rescan if we already did */
!     if (!osastate->sort_done)
!     {
!         tuplesort_performsort(osastate->sortstate);
!         osastate->sort_done = true;
!     }
!     else
!         tuplesort_rescan(osastate->sortstate);

      /* Scan tuples and count frequencies */
      while (tuplesort_getdatum(osastate->sortstate, true, &val, &isnull, &abbrev_val))
*************** mode_final(PG_FUNCTION_ARGS)
*** 1097,1109 ****
      if (shouldfree && !last_val_is_mode)
          pfree(DatumGetPointer(last_val));

-     /*
-      * Note: we *cannot* clean up the tuplesort object here, because the value
-      * to be returned is allocated inside its sortcontext.  We could use
-      * datumCopy to copy it out of there, but it doesn't seem worth the
-      * trouble, since the cleanup callback will clear the tuplesort later.
-      */
-
      if (mode_freq)
          PG_RETURN_DATUM(mode_val);
      else
--- 1117,1122 ----
*************** hypothetical_rank_common(FunctionCallInf
*** 1174,1179 ****
--- 1187,1195 ----

      hypothetical_check_argtypes(fcinfo, nargs, osastate->qstate->tupdesc);

+     /* because we need a hypothetical row, we can't share transition state */
+     Assert(!osastate->sort_done);
+
      /* insert the hypothetical row into the sort */
      slot = osastate->qstate->tupslot;
      ExecClearTuple(slot);
*************** hypothetical_rank_common(FunctionCallInf
*** 1190,1195 ****
--- 1206,1212 ----

      /* finish the sort */
      tuplesort_performsort(osastate->sortstate);
+     osastate->sort_done = true;

      /* iterate till we find the hypothetical row */
      while (tuplesort_gettupleslot(osastate->sortstate, true, true, slot, NULL))
*************** hypothetical_rank_common(FunctionCallInf
*** 1207,1216 ****

      ExecClearTuple(slot);

-     /* Might as well clean up the tuplesort object immediately */
-     tuplesort_end(osastate->sortstate);
-     osastate->sortstate = NULL;
-
      return rank;
  }

--- 1224,1229 ----
*************** hypothetical_dense_rank_final(PG_FUNCTIO
*** 1329,1334 ****
--- 1342,1350 ----
      /* Get short-term context we can use for execTuplesMatch */
      tmpcontext = AggGetTempMemoryContext(fcinfo);

+     /* because we need a hypothetical row, we can't share transition state */
+     Assert(!osastate->sort_done);
+
      /* insert the hypothetical row into the sort */
      slot = osastate->qstate->tupslot;
      ExecClearTuple(slot);
*************** hypothetical_dense_rank_final(PG_FUNCTIO
*** 1345,1350 ****
--- 1361,1367 ----

      /* finish the sort */
      tuplesort_performsort(osastate->sortstate);
+     osastate->sort_done = true;

      /*
       * We alternate fetching into tupslot and extraslot so that we have the
*************** hypothetical_dense_rank_final(PG_FUNCTIO
*** 1391,1400 ****

      ExecDropSingleTupleTableSlot(extraslot);

-     /* Might as well clean up the tuplesort object immediately */
-     tuplesort_end(osastate->sortstate);
-     osastate->sortstate = NULL;
-
      rank = rank - duplicate_count;

      PG_RETURN_INT64(rank);
--- 1408,1413 ----
diff --git a/src/include/catalog/pg_aggregate.h b/src/include/catalog/pg_aggregate.h
index 5769f64..13f1bce 100644
*** a/src/include/catalog/pg_aggregate.h
--- b/src/include/catalog/pg_aggregate.h
*************** DATA(insert ( 3267    n 0 jsonb_agg_transfn
*** 318,330 ****
  DATA(insert ( 3270    n 0 jsonb_object_agg_transfn jsonb_object_agg_finalfn    -    -    -    -                -
         -            f f r r 0    2281    0    0        0    _null_ _null_ )); 

  /* ordered-set and hypothetical-set aggregates */
! DATA(insert ( 3972    o 1 ordered_set_transition            percentile_disc_final                    -    -    -    -
      -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3974    o 1 ordered_set_transition            percentile_cont_float8_final            -    -    -    -
     -        -        f f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3976    o 1 ordered_set_transition            percentile_cont_interval_final            -    -    -
-       -        -        f f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3978    o 1 ordered_set_transition            percentile_disc_multi_final                -    -    -
-       -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3980    o 1 ordered_set_transition            percentile_cont_float8_multi_final        -    -    -
-       -        -        f f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3982    o 1 ordered_set_transition            percentile_cont_interval_multi_final    -    -    -    -
     -        -        f f w w 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3984    o 0 ordered_set_transition            mode_final                                -    -    -
-       -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3986    h 1 ordered_set_transition_multi    rank_final                                -    -    -    -
     -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3988    h 1 ordered_set_transition_multi    percent_rank_final                        -    -    -    -
     -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3990    h 1 ordered_set_transition_multi    cume_dist_final                            -    -    -    -
      -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
--- 318,330 ----
  DATA(insert ( 3270    n 0 jsonb_object_agg_transfn jsonb_object_agg_finalfn    -    -    -    -                -
         -            f f r r 0    2281    0    0        0    _null_ _null_ )); 

  /* ordered-set and hypothetical-set aggregates */
! DATA(insert ( 3972    o 1 ordered_set_transition            percentile_disc_final                    -    -    -    -
      -        -        t f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3974    o 1 ordered_set_transition            percentile_cont_float8_final            -    -    -    -
     -        -        f f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3976    o 1 ordered_set_transition            percentile_cont_interval_final            -    -    -
-       -        -        f f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3978    o 1 ordered_set_transition            percentile_disc_multi_final                -    -    -
-       -        -        t f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3980    o 1 ordered_set_transition            percentile_cont_float8_multi_final        -    -    -
-       -        -        f f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3982    o 1 ordered_set_transition            percentile_cont_interval_multi_final    -    -    -    -
     -        -        f f s s 0    2281    0    0        0    _null_ _null_ )); 
! DATA(insert ( 3984    o 0 ordered_set_transition            mode_final                                -    -    -
-       -        -        t f s s 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3986    h 1 ordered_set_transition_multi    rank_final                                -    -    -    -
     -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3988    h 1 ordered_set_transition_multi    percent_rank_final                        -    -    -    -
     -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
  DATA(insert ( 3990    h 1 ordered_set_transition_multi    cume_dist_final                            -    -    -    -
      -        -        t f w w 0    2281    0    0        0    _null_ _null_ )); 
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index b604a5c..a68ec91 100644
*** a/src/include/fmgr.h
--- b/src/include/fmgr.h
*************** extern int AggCheckCallContext(FunctionC
*** 698,703 ****
--- 698,704 ----
                      MemoryContext *aggcontext);
  extern fmAggrefPtr AggGetAggref(FunctionCallInfo fcinfo);
  extern MemoryContext AggGetTempMemoryContext(FunctionCallInfo fcinfo);
+ extern bool AggStateIsShared(FunctionCallInfo fcinfo);
  extern void AggRegisterCallback(FunctionCallInfo fcinfo,
                      fmExprContextCallbackFunction func,
                      Datum arg);

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

I wrote:
> BTW ... I was quite surprised to notice that the aggdirectargs
> are treated as a property that has to be matched in order to
> combine transition states.  They aren't used during the transition
> phase, so this seems like a pointless constraint.  We could move
> the aggdirectargs ExprState list to AggStatePerAggData and treat
> the aggdirectargs as part of the finalfn-related data, instead.

Here's a pretty-lightly-tested patch for that.

            regards, tom lane

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 807822b..8f5f7f1 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*************** typedef struct AggStatePerTransData
*** 260,272 ****
      bool        aggshared;

      /*
-      * Nominal number of arguments for aggregate function.  For plain aggs,
-      * this excludes any ORDER BY expressions.  For ordered-set aggs, this
-      * counts both the direct and aggregated (ORDER BY) arguments.
-      */
-     int            numArguments;
-
-     /*
       * Number of aggregated input columns.  This includes ORDER BY expressions
       * in both the plain-agg and ordered-set cases.  Ordered-set direct args
       * are not counted, though.
--- 260,265 ----
*************** typedef struct AggStatePerTransData
*** 300,308 ****
      /* Oid of state value's datatype */
      Oid            aggtranstype;

!     /* ExprStates of the FILTER and argument expressions. */
!     ExprState  *aggfilter;        /* state of FILTER expression, if any */
!     List       *aggdirectargs;    /* states of direct-argument expressions */

      /*
       * fmgr lookup data for transition function or combine function.  Note in
--- 293,300 ----
      /* Oid of state value's datatype */
      Oid            aggtranstype;

!     /* ExprState for the FILTER expression, if any. */
!     ExprState  *aggfilter;

      /*
       * fmgr lookup data for transition function or combine function.  Note in
*************** typedef struct AggStatePerAggData
*** 442,447 ****
--- 434,442 ----
       */
      int            numFinalArgs;

+     /* ExprStates for any direct-argument expressions */
+     List       *aggdirectargs;
+
      /*
       * We need the len and byval info for the agg's result data type in order
       * to know how to copy/delete values.
*************** finalize_aggregate(AggState *aggstate,
*** 1526,1532 ****
       * for the transition state value.
       */
      i = 1;
!     foreach(lc, pertrans->aggdirectargs)
      {
          ExprState  *expr = (ExprState *) lfirst(lc);

--- 1521,1527 ----
       * for the transition state value.
       */
      i = 1;
!     foreach(lc, peragg->aggdirectargs)
      {
          ExprState  *expr = (ExprState *) lfirst(lc);

*************** ExecInitAgg(Agg *node, EState *estate, i
*** 3293,3298 ****
--- 3288,3297 ----
          else
              peragg->numFinalArgs = numDirectArgs + 1;

+         /* Initialize any direct-argument expressions */
+         peragg->aggdirectargs = ExecInitExprList(aggref->aggdirectargs,
+                                                  (PlanState *) aggstate);
+
          /*
           * build expression trees using actual argument & result types for the
           * finalfn, if it exists and is required.
*************** build_pertrans_for_aggref(AggStatePerTra
*** 3605,3615 ****

      }

!     /* Initialize the input and FILTER expressions */
      pertrans->aggfilter = ExecInitExpr(aggref->aggfilter,
                                         (PlanState *) aggstate);
-     pertrans->aggdirectargs = ExecInitExprList(aggref->aggdirectargs,
-                                                (PlanState *) aggstate);

      /*
       * If we're doing either DISTINCT or ORDER BY for a plain agg, then we
--- 3604,3612 ----

      }

!     /* Initialize the FILTER expression */
      pertrans->aggfilter = ExecInitExpr(aggref->aggfilter,
                                         (PlanState *) aggstate);

      /*
       * If we're doing either DISTINCT or ORDER BY for a plain agg, then we
*************** find_compatible_peragg(Aggref *newagg, A
*** 3793,3799 ****
              newagg->aggstar != existingRef->aggstar ||
              newagg->aggvariadic != existingRef->aggvariadic ||
              newagg->aggkind != existingRef->aggkind ||
-             !equal(newagg->aggdirectargs, existingRef->aggdirectargs) ||
              !equal(newagg->args, existingRef->args) ||
              !equal(newagg->aggorder, existingRef->aggorder) ||
              !equal(newagg->aggdistinct, existingRef->aggdistinct) ||
--- 3790,3795 ----
*************** find_compatible_peragg(Aggref *newagg, A
*** 3803,3809 ****
          /* if it's the same aggregate function then report exact match */
          if (newagg->aggfnoid == existingRef->aggfnoid &&
              newagg->aggtype == existingRef->aggtype &&
!             newagg->aggcollid == existingRef->aggcollid)
          {
              list_free(*same_input_transnos);
              *same_input_transnos = NIL;
--- 3799,3806 ----
          /* if it's the same aggregate function then report exact match */
          if (newagg->aggfnoid == existingRef->aggfnoid &&
              newagg->aggtype == existingRef->aggtype &&
!             newagg->aggcollid == existingRef->aggcollid &&
!             equal(newagg->aggdirectargs, existingRef->aggdirectargs))
          {
              list_free(*same_input_transnos);
              *same_input_transnos = NIL;

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs