Re: Statistical aggregate functions are not working with PARTIAL aggregation

Hi,

On 2019-05-20 17:27:10 +1200, David Rowley wrote:
> On Mon, 20 May 2019 at 13:20, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > How
> > about we have something roughly like:
> >
> > int numTransFnArgs = -1;
> > int numCombineFnArgs = -1;
> > Oid transFnInputTypes[FUNC_MAX_ARGS];
> > Oid combineFnInputTypes[2];
> >
> > if (DO_AGGSPLIT_COMBINE(...)
> > numCombineFnArgs = 1;
> > combineFnInputTypes = list_make2(aggtranstype, aggtranstype);
> > else
> > numTransFnArgs = get_aggregate_argtypes(aggref, transFnInputTypes);
> >
> > ...
> >
> > if (DO_AGGSPLIT_COMBINE(...))
> > build_pertrans_for_aggref(pertrans, aggstate, estate,
> > aggref, combinefn_oid, aggtranstype,
> > serialfn_oid, deserialfn_oid,
> > initValue, initValueIsNull,
> > combineFnInputTypes, numCombineFnArgs);
> > else
> > build_pertrans_for_aggref(pertrans, aggstate, estate,
> > aggref, transfn_oid, aggtranstype,
> > serialfn_oid, deserialfn_oid,
> > initValue, initValueIsNull,
> > transFnInputTypes, numTransFnArgs);
> >
> > seems like that'd make the code clearer?
>
> I think that might be a good idea... I mean apart from trying to
> assign a List to an array :) We still must call
> get_aggregate_argtypes() in order to determine the final function, so
> the code can't look exactly like you've written.
>
> > I wonder if we shouldn't
> > strive to have *no* DO_AGGSPLIT_COMBINE specific logic in
> > build_pertrans_for_aggref (except perhaps for an error check or two).
>
> Just so we have a hard copy to review and discuss, I think this would
> look something like the attached.
>
> We do miss out on a few very small optimisations, but I don't think
> they'll be anything we could measure. Namely
> build_aggregate_combinefn_expr() called make_agg_arg() once and used
> it twice instead of calling it once for each arg. I don't think
> that's anything we could measure, especially in a situation where
> two-stage aggregation is being used.
>
> I ended up also renaming aggtransfn to transfn_oid in
> build_pertrans_for_aggref(). Having it called aggtranfn seems a bit
> too close to the pg_aggregate.aggtransfn column which is confusion
> given that we might pass it the value of the aggcombinefn column.

Now that master is open for development, and you have a commit bit, are
you planning to go forward with this on your own?

Greetings,

Andres Freund