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

tglsfdc · tglsfdc · commit cb591fcbfbba · 2017-10-15T19:19:19.000-04:00
While poking around in the aggregate logic, I noticed that commit 8ed3f11 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 24a1e20. 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
@@ -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;
 
 	/*
@@ -2799,10 +2804,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
@@ -2841,17 +2847,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
@@ -3343,19 +3338,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;
@@ -3364,10 +3359,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)
@@ -3384,7 +3383,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,
@@ -3394,6 +3393,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;
 }
 
@@ -3423,7 +3437,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
 	List	   *sortlist;
 	int			numSortCols;
 	int			numDistinctCols;
-	int			naggs;
 	int			i;
 
 	/* Begin filling in the pertrans data */
@@ -3565,22 +3578,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
