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. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
cleanup_nodeagg_code.patch
Description: Binary data
