Restore nodeAgg.c's ability to check for improperly-nested aggregates.

While poking around in the aggregate logic, I noticed that commit
8ed3f11bb broke the logic in nodeAgg.c that purports to detect nested
aggregates, by moving initialization of regular aggregate argument
expressions out of the code segment that checks for that.

You could argue that this check is unnecessary, but it's not much code
so I'm inclined to keep it as a backstop against parser and planner
bugs.  However, there's certainly zero value in checking only some of
the subexpressions.

We can make the check complete again, and as a bonus make it a good
deal more bulletproof against future mistakes of the same ilk, by
moving it out to the outermost level of ExecInitAgg.  This means we
need to check only once per Agg node not once per aggregate, which
also seems like a good thing --- if the check does find something
wrong, it's not urgent that we report it before the plan node
initialization finishes.

Since this requires remembering the original length of the aggs list,
I deleted a long-obsolete stanza that changed numaggs from 0 to 1.
That's so old it predates our decision that palloc(0) is a valid
operation, in (digs...) 2004, see commit 24a1e20f1.

In passing improve a few comments.

Back-patch to v10, just in case.

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 40d8ec9db4631b63625e8527a97f7ce6b309ac2c..f62b3b22ba928da08b5515d7ea023aef71603678 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -268,7 +268,12 @@ typedef struct AggStatePerTransData
         */
        int                     numInputs;
 
-       /* offset of input columns in AggState->evalslot */
+       /*
+        * At each input row, we evaluate all argument expressions needed for all
+        * the aggregates in this Agg node in a single ExecProject call.  inputoff
+        * is the starting index of this aggregate's argument expressions in the
+        * resulting tuple (in AggState->evalslot).
+        */
        int                     inputoff;
 
        /*
@@ -2809,10 +2814,11 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
        /*
         * initialize child expressions
         *
-        * We rely on the parser to have checked that no aggs contain other agg
-        * calls in their arguments.  This would make no sense under SQL semantics
-        * (and it's forbidden by the spec).  Because it is true, we don't need to
-        * worry about evaluating the aggs in any particular order.
+        * We expect the parser to have checked that no aggs contain other agg
+        * calls in their arguments (and just to be sure, we verify it again while
+        * initializing the plan node).  This would make no sense under SQL
+        * semantics, and it's forbidden by the spec.  Because it is true, we
+        * don't need to worry about evaluating the aggs in any particular order.
         *
         * Note: execExpr.c finds Aggrefs for us, and adds their AggrefExprState
         * nodes to aggstate->aggs.  Aggrefs in the qual are found here; Aggrefs
@@ -2851,17 +2857,6 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
         */
        numaggs = aggstate->numaggs;
        Assert(numaggs == list_length(aggstate->aggs));
-       if (numaggs <= 0)
-       {
-               /*
-                * This is not an error condition: we might be using the Agg node just
-                * to do hash-based grouping.  Even in the regular case,
-                * constant-expression simplification could optimize away all of the
-                * Aggrefs in the targetlist and qual.  So keep going, but force local
-                * copy of numaggs positive so that palloc()s below don't choke.
-                */
-               numaggs = 1;
-       }
 
        /*
         * For each phase, prepare grouping set data and fmgr lookup data for
@@ -3364,19 +3359,19 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
        }
 
        /*
-        * Update numaggs to match the number of unique aggregates found. Also set
-        * numstates to the number of unique aggregate states found.
+        * Update aggstate->numaggs to be the number of unique aggregates found.
+        * Also set numstates to the number of unique transition states found.
         */
        aggstate->numaggs = aggno + 1;
        aggstate->numtrans = transno + 1;
 
        /*
         * Build a single projection computing the aggregate arguments for all
-        * aggregates at once, that's considerably faster than doing it separately
-        * for each.
+        * aggregates at once; if there's more than one, that's considerably
+        * faster than doing it separately for each.
         *
-        * First create a targetlist combining the targetlist of all the
-        * transitions.
+        * First create a targetlist combining the targetlists of all the
+        * per-trans states.
         */
        combined_inputeval = NIL;
        column_offset = 0;
@@ -3385,10 +3380,14 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
                AggStatePerTrans pertrans = &pertransstates[transno];
                ListCell   *arg;
 
+               /*
+                * Mark this per-trans state with its starting column in the combined
+                * slot.
+                */
                pertrans->inputoff = column_offset;
 
                /*
-                * Adjust resno in a copied target entries, to point into the combined
+                * Adjust resnos in the copied target entries to match the combined
                 * slot.
                 */
                foreach(arg, pertrans->aggref->args)
@@ -3405,7 +3404,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
                column_offset += list_length(pertrans->aggref->args);
        }
 
-       /* and then create a projection for that targetlist */
+       /* Now create a projection for the combined targetlist */
        aggstate->evaldesc = ExecTypeFromTL(combined_inputeval, false);
        aggstate->evalslot = ExecInitExtraTupleSlot(estate);
        aggstate->evalproj = ExecBuildProjectionInfo(combined_inputeval,
@@ -3415,6 +3414,21 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
                                                                                                 NULL);
        ExecSetSlotDescriptor(aggstate->evalslot, aggstate->evaldesc);
 
+       /*
+        * Last, check whether any more aggregates got added onto the node while
+        * we processed the expressions for the aggregate arguments (including not
+        * only the regular arguments handled immediately above, but any FILTER
+        * expressions and direct arguments we might've handled earlier).  If so,
+        * we have nested aggregate functions, which is semantically nonsensical,
+        * so complain.  (This should have been caught by the parser, so we don't
+        * need to work hard on a helpful error message; but we defend against it
+        * here anyway, just to be sure.)
+        */
+       if (numaggs != list_length(aggstate->aggs))
+               ereport(ERROR,
+                               (errcode(ERRCODE_GROUPING_ERROR),
+                                errmsg("aggregate function calls cannot be nested")));
+
        return aggstate;
 }
 
@@ -3444,7 +3458,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
        List       *sortlist;
        int                     numSortCols;
        int                     numDistinctCols;
-       int                     naggs;
        int                     i;
 
        /* Begin filling in the pertrans data */
@@ -3586,22 +3599,11 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
        }
 
        /* Initialize the input and FILTER expressions */
-       naggs = aggstate->numaggs;
        pertrans->aggfilter = ExecInitExpr(aggref->aggfilter,
                                                                           (PlanState *) aggstate);
        pertrans->aggdirectargs = ExecInitExprList(aggref->aggdirectargs,
                                                                                           (PlanState *) aggstate);
 
-       /*
-        * Complain if the aggregate's arguments contain any aggregates; nested
-        * agg functions are semantically nonsensical.  (This should have been
-        * caught earlier, but we defend against it here anyway.)
-        */
-       if (naggs != aggstate->numaggs)
-               ereport(ERROR,
-                               (errcode(ERRCODE_GROUPING_ERROR),
-                                errmsg("aggregate function calls cannot be nested")));
-
        /*
         * If we're doing either DISTINCT or ORDER BY for a plain agg, then we
         * have a list of SortGroupClause nodes; fish out the data in them and