Re: Statistical aggregate functions are not working with PARTIAL aggregation

On Mon, 20 May 2019 at 13:20, Andres Freund <andres@anarazel.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.

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