Hi, On 2019-05-20 17:27:10 +1200, David Rowley wrote: > On Mon, 20 May 2019 at 13:20, Andres Freund <and...@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.
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
