Fix AggGetAggref() so it won't lie to aggregate final functions.

tglsfdc · tglsfdc · commit aa1e9b3a4661 · 2017-10-12T15:20:04.000-04:00
If we merge the transition calculations for two different aggregates, it's reasonable to assume that the transition function should not care which of those Aggref structs it gets from AggGetAggref(). It is not reasonable to make the same assumption about an aggregate final function, however. Commit 804163b broke this, as it will pass whichever Aggref was first associated with the transition state in both cases. This doesn't create an observable bug so far as the core system is concerned, because the only existing uses of AggGetAggref() are in ordered-set aggregates that happen to not pay attention to anything but the input properties of the Aggref; and besides that, we disabled sharing of transition calculations for OSAs yesterday. Nonetheless, if some third-party code were using AggGetAggref() in a normal aggregate, they would be entitled to call this a bug. Hence, back-patch the fix to 9.6 where the problem was introduced. In passing, improve some of the comments about transition state sharing. Discussion: https://postgr.es/m/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
@@ -1372,8 +1372,8 @@ finalize_aggregate(AggState *aggstate,
 	{
 		int			numFinalArgs = peragg->numFinalArgs;
 
-		/* set up aggstate->curpertrans for AggGetAggref() */
-		aggstate->curpertrans = pertrans;
+		/* set up aggstate->curperagg for AggGetAggref() */
+		aggstate->curperagg = peragg;
 
 		InitFunctionCallInfoData(fcinfo, &peragg->finalfn,
 								 numFinalArgs,
@@ -1406,7 +1406,7 @@ finalize_aggregate(AggState *aggstate,
 			*resultVal = FunctionCallInvoke(&fcinfo);
 			*resultIsNull = fcinfo.isnull;
 		}
-		aggstate->curpertrans = NULL;
+		aggstate->curperagg = NULL;
 	}
 	else
 	{
@@ -2391,6 +2391,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	aggstate->current_set = 0;
 	aggstate->peragg = NULL;
 	aggstate->pertrans = NULL;
+	aggstate->curperagg = NULL;
 	aggstate->curpertrans = NULL;
 	aggstate->input_done = false;
 	aggstate->agg_done = false;
@@ -2659,27 +2660,29 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	 *
 	 * Scenarios:
 	 *
-	 * 1. An aggregate function appears more than once in query:
+	 * 1. Identical aggregate function calls appear in the query:
 	 *
 	 *	  SELECT SUM(x) FROM ... HAVING SUM(x) > 0
 	 *
-	 *	  Since the aggregates are the identical, we only need to calculate
-	 *	  the calculate it once. Both aggregates will share the same 'aggno'
-	 *	  value.
+	 *	  Since these aggregates are identical, we only need to calculate
+	 *	  the value once. Both aggregates will share the same 'aggno' value.
 	 *
 	 * 2. Two different aggregate functions appear in the query, but the
-	 *	  aggregates have the same transition function and initial value, but
-	 *	  different final function:
+	 *	  aggregates have the same arguments, transition functions and
+	 *	  initial values (and, presumably, different final functions):
 	 *
-	 *	  SELECT SUM(x), AVG(x) FROM ...
+	 *	  SELECT AVG(x), STDDEV(x) FROM ...
 	 *
 	 *	  In this case we must create a new peragg for the varying aggregate,
-	 *	  and need to call the final functions separately, but can share the
-	 *	  same transition state.
+	 *	  and we need to call the final functions separately, but we need
+	 *	  only run the transition function once.  (This requires that the
+	 *	  final functions be nondestructive of the transition state, but
+	 *	  that's required anyway for other reasons.)
 	 *
-	 * For either of these optimizations to be valid, the aggregate's
-	 * arguments must be the same, including any modifiers such as ORDER BY,
-	 * DISTINCT and FILTER, and they mustn't contain any volatile functions.
+	 * For either of these optimizations to be valid, all aggregate properties
+	 * used in the transition phase must be the same, including any modifiers
+	 * such as ORDER BY, DISTINCT and FILTER, and the arguments mustn't
+	 * contain any volatile functions.
 	 * -----------------
 	 */
 	aggno = -1;
@@ -3287,7 +3290,7 @@ GetAggInitVal(Datum textInitVal, Oid transtype)
  *
  * As a side-effect, this also collects a list of existing per-Trans structs
  * with matching inputs. If no identical Aggref is found, the list is passed
- * later to find_compatible_perstate, to see if we can at least reuse the
+ * later to find_compatible_pertrans, to see if we can at least reuse the
  * state value of another aggregate.
  */
 static int
@@ -3307,11 +3310,12 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
 
 	/*
 	 * Search through the list of already seen aggregates. If we find an
-	 * existing aggregate with the same aggregate function and input
-	 * parameters as an existing one, then we can re-use that one. While
+	 * existing identical aggregate call, then we can re-use that one. While
 	 * searching, we'll also collect a list of Aggrefs with the same input
 	 * parameters. If no matching Aggref is found, the caller can potentially
-	 * still re-use the transition state of one of them.
+	 * still re-use the transition state of one of them.  (At this stage we
+	 * just compare the parsetrees; whether different aggregates share the
+	 * same transition function will be checked later.)
 	 */
 	for (aggno = 0; aggno <= lastaggno; aggno++)
 	{
@@ -3360,7 +3364,7 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
  * struct
  *
  * Searches the list of transnos for a per-Trans struct with the same
- * transition state and initial condition. (The inputs have already been
+ * transition function and initial condition. (The inputs have already been
  * verified to match.)
  */
 static int
@@ -3406,16 +3410,16 @@ find_compatible_pertrans(AggState *aggstate, Aggref *newagg,
 			aggdeserialfn != pertrans->deserialfn_oid)
 			continue;
 
-		/* Check that the initial condition matches, too. */
+		/*
+		 * Check that the initial condition matches, too.
+		 */
 		if (initValueIsNull && pertrans->initValueIsNull)
 			return transno;
 
 		if (!initValueIsNull && !pertrans->initValueIsNull &&
 			datumIsEqual(initValue, pertrans->initValue,
 						 pertrans->transtypeByVal, pertrans->transtypeLen))
-		{
 			return transno;
-		}
 	}
 	return -1;
 }
@@ -3628,6 +3632,13 @@ AggCheckCallContext(FunctionCallInfo fcinfo, MemoryContext *aggcontext)
  * If the function is being called as an aggregate support function,
  * return the Aggref node for the aggregate call.  Otherwise, return NULL.
  *
+ * Aggregates sharing the same inputs and transition functions can get
+ * merged into a single transition calculation.  If the transition function
+ * calls AggGetAggref, it will get some one of the Aggrefs for which it is
+ * executing.  It must therefore not pay attention to the Aggref fields that
+ * relate to the final function, as those are indeterminate.  But if a final
+ * function calls AggGetAggref, it will get a precise result.
+ *
  * Note that if an aggregate is being used as a window function, this will
  * return NULL.  We could provide a similar function to return the relevant
  * WindowFunc node in such cases, but it's not needed yet.
@@ -3637,8 +3648,16 @@ AggGetAggref(FunctionCallInfo fcinfo)
 {
 	if (fcinfo->context && IsA(fcinfo->context, AggState))
 	{
+		AggStatePerAgg curperagg;
 		AggStatePerTrans curpertrans;
 
+		/* check curperagg (valid when in a final function) */
+		curperagg = ((AggState *) fcinfo->context)->curperagg;
+
+		if (curperagg)
+			return curperagg->aggref;
+
+		/* check curpertrans (valid when in a transition function) */
 		curpertrans = ((AggState *) fcinfo->context)->curpertrans;
 
 		if (curpertrans)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
@@ -1834,7 +1834,7 @@ typedef struct AggState
 	AggStatePerTrans pertrans;	/* per-Trans state information */
 	ExprContext **aggcontexts;	/* econtexts for long-lived data (per GS) */
 	ExprContext *tmpcontext;	/* econtext for input expressions */
-	AggStatePerTrans curpertrans;		/* currently active trans state */
+	AggStatePerTrans curpertrans;	/* currently active trans state, if any */
 	bool		input_done;		/* indicates end of input */
 	bool		agg_done;		/* indicates completion of Agg scan */
 	int			projected_set;	/* The last projected grouping set */
@@ -1857,6 +1857,7 @@ typedef struct AggState
 	List	   *hash_needed;	/* list of columns needed in hash table */
 	bool		table_filled;	/* hash table filled yet? */
 	TupleHashIterator hashiter; /* for iterating through hash table */
+	AggStatePerAgg curperagg;	/* currently active aggregate, if any */
 } AggState;
 
 /* ----------------
