Re: [BUGS] Combination of ordered-set aggregate function terminates JDBC connection on PostgreSQL 9.6.5 - Mailing list 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