Cleanup some aggregate code in the executor

Here we alter the code that calls build_pertrans_for_aggref() so that the
function no longer needs to special-case whether it's dealing with an
aggtransfn or an aggcombinefn.  This allows us to reuse the
build_aggregate_transfn_expr() function and just get rid of the
build_aggregate_combinefn_expr() completely.

All of the special case code that was in build_pertrans_for_aggref() has
been moved up to the calling functions.

This saves about a dozen lines of code in nodeAgg.c and a few dozen more
in parse_agg.c

Also, rename a few variables in nodeAgg.c to try to make it more clear
that we're working with either a aggtransfn or an aggcombinefn.  Some of
the old names would have you believe that we were always working with an
aggtransfn.

Discussion: https://postgr.es/m/CAApHDvptMQ9FmF0D67zC_w88yVnoNVR2+kkOQGUrCmdxWxLULQ@mail.gmail.com

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 8440a76fbdc9e60e83b6e60125ac7df510645ad0..914b02ceee48e240f50ff854980fad07a3833ee2 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -461,10 +461,11 @@ static void hashagg_tapeinfo_release(HashTapeInfo *tapeinfo, int tapenum);
 static Datum GetAggInitVal(Datum textInitVal, Oid transtype);
 static void build_pertrans_for_aggref(AggStatePerTrans pertrans,
                                      AggState *aggstate, EState *estate,
-                                     Aggref *aggref, Oid aggtransfn, Oid aggtranstype,
-                                     Oid aggserialfn, Oid aggdeserialfn,
-                                     Datum initValue, bool initValueIsNull,
-                                     Oid *inputTypes, int numArguments);
+                                     Aggref *aggref, Oid transfn_oid,
+                                     Oid aggtranstype, Oid aggserialfn,
+                                     Oid aggdeserialfn, Datum initValue,
+                                     bool initValueIsNull, Oid *inputTypes,
+                                     int numArguments);
 
 
 /*
@@ -3724,8 +3725,8 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
        Aggref     *aggref = lfirst(l);
        AggStatePerAgg peragg;
        AggStatePerTrans pertrans;
-       Oid         inputTypes[FUNC_MAX_ARGS];
-       int         numArguments;
+       Oid         aggTransFnInputTypes[FUNC_MAX_ARGS];
+       int         numAggTransFnArgs;
        int         numDirectArgs;
        HeapTuple   aggTuple;
        Form_pg_aggregate aggform;
@@ -3859,14 +3860,15 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
         * could be different from the agg's declared input types, when the
         * agg accepts ANY or a polymorphic type.
         */
-       numArguments = get_aggregate_argtypes(aggref, inputTypes);
+       numAggTransFnArgs = get_aggregate_argtypes(aggref,
+                                                  aggTransFnInputTypes);
 
        /* Count the "direct" arguments, if any */
        numDirectArgs = list_length(aggref->aggdirectargs);
 
        /* Detect how many arguments to pass to the finalfn */
        if (aggform->aggfinalextra)
-           peragg->numFinalArgs = numArguments + 1;
+           peragg->numFinalArgs = numAggTransFnArgs + 1;
        else
            peragg->numFinalArgs = numDirectArgs + 1;
 
@@ -3880,7 +3882,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
         */
        if (OidIsValid(finalfn_oid))
        {
-           build_aggregate_finalfn_expr(inputTypes,
+           build_aggregate_finalfn_expr(aggTransFnInputTypes,
                                         peragg->numFinalArgs,
                                         aggtranstype,
                                         aggref->aggtype,
@@ -3911,7 +3913,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
            /*
             * If this aggregation is performing state combines, then instead
             * of using the transition function, we'll use the combine
-            * function
+            * function.
             */
            if (DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
            {
@@ -3924,8 +3926,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
            else
                transfn_oid = aggform->aggtransfn;
 
-           aclresult = pg_proc_aclcheck(transfn_oid, aggOwner,
-                                        ACL_EXECUTE);
+           aclresult = pg_proc_aclcheck(transfn_oid, aggOwner, ACL_EXECUTE);
            if (aclresult != ACLCHECK_OK)
                aclcheck_error(aclresult, OBJECT_FUNCTION,
                               get_func_name(transfn_oid));
@@ -3943,11 +3944,72 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
            else
                initValue = GetAggInitVal(textInitVal, aggtranstype);
 
-           build_pertrans_for_aggref(pertrans, aggstate, estate,
-                                     aggref, transfn_oid, aggtranstype,
-                                     serialfn_oid, deserialfn_oid,
-                                     initValue, initValueIsNull,
-                                     inputTypes, numArguments);
+           if (DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
+           {
+               Oid         combineFnInputTypes[] = {aggtranstype,
+               aggtranstype};
+
+               /*
+                * When combining there's only one input, the to-be-combined
+                * transition value.  The transition value is not counted
+                * here.
+                */
+               pertrans->numTransInputs = 1;
+
+               /* aggcombinefn always has two arguments of aggtranstype */
+               build_pertrans_for_aggref(pertrans, aggstate, estate,
+                                         aggref, transfn_oid, aggtranstype,
+                                         serialfn_oid, deserialfn_oid,
+                                         initValue, initValueIsNull,
+                                         combineFnInputTypes, 2);
+
+               /*
+                * Ensure that a combine function to combine INTERNAL states
+                * is not strict. This should have been checked during CREATE
+                * AGGREGATE, but the strict property could have been changed
+                * since then.
+                */
+               if (pertrans->transfn.fn_strict && aggtranstype == INTERNALOID)
+                   ereport(ERROR,
+                           (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+                            errmsg("combine function with transition type %s must not be declared STRICT",
+                                   format_type_be(aggtranstype))));
+           }
+           else
+           {
+               /* Detect how many arguments to pass to the transfn */
+               if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
+                   pertrans->numTransInputs = list_length(aggref->args);
+               else
+                   pertrans->numTransInputs = numAggTransFnArgs;
+
+               build_pertrans_for_aggref(pertrans, aggstate, estate,
+                                         aggref, transfn_oid, aggtranstype,
+                                         serialfn_oid, deserialfn_oid,
+                                         initValue, initValueIsNull,
+                                         aggTransFnInputTypes,
+                                         numAggTransFnArgs);
+
+               /*
+                * If the transfn is strict and the initval is NULL, make sure
+                * input type and transtype are the same (or at least
+                * binary-compatible), so that it's OK to use the first
+                * aggregated input value as the initial transValue.  This
+                * should have been checked at agg definition time, but we
+                * must check again in case the transfn's strictness property
+                * has been changed.
+                */
+               if (pertrans->transfn.fn_strict && pertrans->initValueIsNull)
+               {
+                   if (numAggTransFnArgs <= numDirectArgs ||
+                       !IsBinaryCoercible(aggTransFnInputTypes[numDirectArgs],
+                                          aggtranstype))
+                       ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+                                errmsg("aggregate %u needs to have compatible input type and transition type",
+                                       aggref->aggfnoid)));
+               }
+           }
        }
        else
            pertrans->aggshared = true;
@@ -4039,20 +4101,24 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
  * Build the state needed to calculate a state value for an aggregate.
  *
  * This initializes all the fields in 'pertrans'. 'aggref' is the aggregate
- * to initialize the state for. 'aggtransfn', 'aggtranstype', and the rest
+ * to initialize the state for. 'transfn_oid', 'aggtranstype', and the rest
  * of the arguments could be calculated from 'aggref', but the caller has
  * calculated them already, so might as well pass them.
+ *
+ * 'transfn_oid' may be either the Oid of the aggtransfn or the aggcombinefn.
  */
 static void
 build_pertrans_for_aggref(AggStatePerTrans pertrans,
                          AggState *aggstate, EState *estate,
                          Aggref *aggref,
-                         Oid aggtransfn, Oid aggtranstype,
+                         Oid transfn_oid, Oid aggtranstype,
                          Oid aggserialfn, Oid aggdeserialfn,
                          Datum initValue, bool initValueIsNull,
                          Oid *inputTypes, int numArguments)
 {
    int         numGroupingSets = Max(aggstate->maxsets, 1);
+   Expr       *transfnexpr;
+   int         numTransArgs;
    Expr       *serialfnexpr = NULL;
    Expr       *deserialfnexpr = NULL;
    ListCell   *lc;
@@ -4067,7 +4133,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
    pertrans->aggref = aggref;
    pertrans->aggshared = false;
    pertrans->aggCollation = aggref->inputcollid;
-   pertrans->transfn_oid = aggtransfn;
+   pertrans->transfn_oid = transfn_oid;
    pertrans->serialfn_oid = aggserialfn;
    pertrans->deserialfn_oid = aggdeserialfn;
    pertrans->initValue = initValue;
@@ -4081,111 +4147,34 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
 
    pertrans->aggtranstype = aggtranstype;
 
+   /* account for the current transition state */
+   numTransArgs = pertrans->numTransInputs + 1;
+
    /*
-    * When combining states, we have no use at all for the aggregate
-    * function's transfn. Instead we use the combinefn.  In this case, the
-    * transfn and transfn_oid fields of pertrans refer to the combine
-    * function rather than the transition function.
+    * Set up infrastructure for calling the transfn.  Note that invtrans is
+    * not needed here.
     */
-   if (DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
-   {
-       Expr       *combinefnexpr;
-       size_t      numTransArgs;
-
-       /*
-        * When combining there's only one input, the to-be-combined added
-        * transition value from below (this node's transition value is
-        * counted separately).
-        */
-       pertrans->numTransInputs = 1;
-
-       /* account for the current transition state */
-       numTransArgs = pertrans->numTransInputs + 1;
-
-       build_aggregate_combinefn_expr(aggtranstype,
-                                      aggref->inputcollid,
-                                      aggtransfn,
-                                      &combinefnexpr);
-       fmgr_info(aggtransfn, &pertrans->transfn);
-       fmgr_info_set_expr((Node *) combinefnexpr, &pertrans->transfn);
-
-       pertrans->transfn_fcinfo =
-           (FunctionCallInfo) palloc(SizeForFunctionCallInfo(2));
-       InitFunctionCallInfoData(*pertrans->transfn_fcinfo,
-                                &pertrans->transfn,
-                                numTransArgs,
-                                pertrans->aggCollation,
-                                (void *) aggstate, NULL);
-
-       /*
-        * Ensure that a combine function to combine INTERNAL states is not
-        * strict. This should have been checked during CREATE AGGREGATE, but
-        * the strict property could have been changed since then.
-        */
-       if (pertrans->transfn.fn_strict && aggtranstype == INTERNALOID)
-           ereport(ERROR,
-                   (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-                    errmsg("combine function with transition type %s must not be declared STRICT",
-                           format_type_be(aggtranstype))));
-   }
-   else
-   {
-       Expr       *transfnexpr;
-       size_t      numTransArgs;
-
-       /* Detect how many arguments to pass to the transfn */
-       if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
-           pertrans->numTransInputs = numInputs;
-       else
-           pertrans->numTransInputs = numArguments;
+   build_aggregate_transfn_expr(inputTypes,
+                                numArguments,
+                                numDirectArgs,
+                                aggref->aggvariadic,
+                                aggtranstype,
+                                aggref->inputcollid,
+                                transfn_oid,
+                                InvalidOid,
+                                &transfnexpr,
+                                NULL);
 
-       /* account for the current transition state */
-       numTransArgs = pertrans->numTransInputs + 1;
+   fmgr_info(transfn_oid, &pertrans->transfn);
+   fmgr_info_set_expr((Node *) transfnexpr, &pertrans->transfn);
 
-       /*
-        * Set up infrastructure for calling the transfn.  Note that
-        * invtransfn is not needed here.
-        */
-       build_aggregate_transfn_expr(inputTypes,
-                                    numArguments,
-                                    numDirectArgs,
-                                    aggref->aggvariadic,
-                                    aggtranstype,
-                                    aggref->inputcollid,
-                                    aggtransfn,
-                                    InvalidOid,
-                                    &transfnexpr,
-                                    NULL);
-       fmgr_info(aggtransfn, &pertrans->transfn);
-       fmgr_info_set_expr((Node *) transfnexpr, &pertrans->transfn);
-
-       pertrans->transfn_fcinfo =
-           (FunctionCallInfo) palloc(SizeForFunctionCallInfo(numTransArgs));
-       InitFunctionCallInfoData(*pertrans->transfn_fcinfo,
-                                &pertrans->transfn,
-                                numTransArgs,
-                                pertrans->aggCollation,
-                                (void *) aggstate, NULL);
-
-       /*
-        * If the transfn is strict and the initval is NULL, make sure input
-        * type and transtype are the same (or at least binary-compatible), so
-        * that it's OK to use the first aggregated input value as the initial
-        * transValue.  This should have been checked at agg definition time,
-        * but we must check again in case the transfn's strictness property
-        * has been changed.
-        */
-       if (pertrans->transfn.fn_strict && pertrans->initValueIsNull)
-       {
-           if (numArguments <= numDirectArgs ||
-               !IsBinaryCoercible(inputTypes[numDirectArgs],
-                                  aggtranstype))
-               ereport(ERROR,
-                       (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-                        errmsg("aggregate %u needs to have compatible input type and transition type",
-                               aggref->aggfnoid)));
-       }
-   }
+   pertrans->transfn_fcinfo =
+       (FunctionCallInfo) palloc(SizeForFunctionCallInfo(numTransArgs));
+   InitFunctionCallInfoData(*pertrans->transfn_fcinfo,
+                            &pertrans->transfn,
+                            numTransArgs,
+                            pertrans->aggCollation,
+                            (void *) aggstate, NULL);
 
    /* get info about the state value's datatype */
    get_typlenbyval(aggtranstype,
@@ -4276,6 +4265,9 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
         */
        Assert(aggstate->aggstrategy != AGG_HASHED && aggstate->aggstrategy != AGG_MIXED);
 
+       /* ORDER BY aggregates are not supported with partial aggregation */
+       Assert(!DO_AGGSPLIT_COMBINE(aggstate->aggsplit));
+
        /* If we have only one input, we need its len/byval info. */
        if (numInputs == 1)
        {

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index a25f8d5b98991d15059a0df46d6fdb43a6dc56be..24268eb5024df5413013377e487018c5df2b0b2e 100644 (file)
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -1959,6 +1959,11 @@ resolve_aggregate_transtype(Oid aggfuncid,
  * latter may be InvalidOid, however if invtransfn_oid is set then
  * transfn_oid must also be set.
  *
+ * transfn_oid may also be passed as the aggcombinefn when the *transfnexpr is
+ * to be used for a combine aggregate phase.  We expect invtransfn_oid to be
+ * InvalidOid in this case since there is no such thing as an inverse
+ * combinefn.
+ *
  * Pointers to the constructed trees are returned into *transfnexpr,
  * *invtransfnexpr. If there is no invtransfn, the respective pointer is set
  * to NULL.  Since use of the invtransfn is optional, NULL may be passed for
@@ -2021,35 +2026,6 @@ build_aggregate_transfn_expr(Oid *agg_input_types,
    }
 }
 
-/*
- * Like build_aggregate_transfn_expr, but creates an expression tree for the
- * combine function of an aggregate, rather than the transition function.
- */
-void
-build_aggregate_combinefn_expr(Oid agg_state_type,
-                              Oid agg_input_collation,
-                              Oid combinefn_oid,
-                              Expr **combinefnexpr)
-{
-   Node       *argp;
-   List       *args;
-   FuncExpr   *fexpr;
-
-   /* combinefn takes two arguments of the aggregate state type */
-   argp = make_agg_arg(agg_state_type, agg_input_collation);
-
-   args = list_make2(argp, argp);
-
-   fexpr = makeFuncExpr(combinefn_oid,
-                        agg_state_type,
-                        args,
-                        InvalidOid,
-                        agg_input_collation,
-                        COERCE_EXPLICIT_CALL);
-   /* combinefn is currently never treated as variadic */
-   *combinefnexpr = (Expr *) fexpr;
-}
-
 /*
  * Like build_aggregate_transfn_expr, but creates an expression tree for the
  * serialization function of an aggregate.

diff --git a/src/include/parser/parse_agg.h b/src/include/parser/parse_agg.h
index 4dea01752af3a5283039fa1cb141e50c81fccbe8..bffbb82df66cd9ebaf96687fc52f6772e0b1cda2 100644 (file)
--- a/src/include/parser/parse_agg.h
+++ b/src/include/parser/parse_agg.h
@@ -46,11 +46,6 @@ extern void build_aggregate_transfn_expr(Oid *agg_input_types,
                                         Expr **transfnexpr,
                                         Expr **invtransfnexpr);
 
-extern void build_aggregate_combinefn_expr(Oid agg_state_type,
-                                          Oid agg_input_collation,
-                                          Oid combinefn_oid,
-                                          Expr **combinefnexpr);
-
 extern void build_aggregate_serialfn_expr(Oid serialfn_oid,
                                          Expr **serialfnexpr);