From 6a0865e4bbf678516e7f2530a163857cd51ca676 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 23 Jul 2009 20:45:27 +0000
Subject: [PATCH] In a non-hashed Agg node, reset the "aggcontext" at group
 boundaries, instead of individually pfree'ing pass-by-reference transition
 values.  This should be at least as fast as the prior coding, and it has the
 major advantage of clearing out any working data an aggregate function may
 have stored in or underneath the aggcontext.  This avoids memory leakage when
 an aggregate such as array_agg() is used in GROUP BY mode.  Per report from
 Chris Spotts.

Back-patch to 8.4.  In principle the problem could arise in prior versions,
but since they didn't have array_agg the issue seems not critical.
---
 src/backend/executor/nodeAgg.c          | 26 +++++++++++--------------
 src/backend/utils/adt/array_userfuncs.c |  6 ++++--
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index a6b1f194ae..4548fe9a17 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -55,13 +55,15 @@
  *	  in either case its value need not be preserved.  See int8inc() for an
  *	  example.	Notice that advance_transition_function() is coded to avoid a
  *	  data copy step when the previous transition value pointer is returned.
+ *	  Also, some transition functions make use of the aggcontext to store
+ *	  working state.
  *
  *
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/executor/nodeAgg.c,v 1.167 2009/06/17 16:05:34 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/executor/nodeAgg.c,v 1.168 2009/07/23 20:45:27 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -272,18 +274,6 @@ initialize_aggregates(AggState *aggstate,
 									  work_mem, false);
 		}
 
-		/*
-		 * If we are reinitializing after a group boundary, we have to free
-		 * any prior transValue to avoid memory leakage.  We must check not
-		 * only the isnull flag but whether the pointer is NULL; since
-		 * pergroupstate is initialized with palloc0, the initial condition
-		 * has isnull = 0 and null pointer.
-		 */
-		if (!peraggstate->transtypeByVal &&
-			!pergroupstate->transValueIsNull &&
-			DatumGetPointer(pergroupstate->transValue) != NULL)
-			pfree(DatumGetPointer(pergroupstate->transValue));
-
 		/*
 		 * (Re)set transValue to the initial value.
 		 *
@@ -911,10 +901,15 @@ agg_retrieve_direct(AggState *aggstate)
 		}
 
 		/*
-		 * Clear the per-output-tuple context for each group
+		 * Clear the per-output-tuple context for each group, as well as
+		 * aggcontext (which contains any pass-by-ref transvalues of the
+		 * old group).  We also clear any child contexts of the aggcontext;
+		 * some aggregate functions store working state in such contexts.
 		 */
 		ResetExprContext(econtext);
 
+		MemoryContextResetAndDeleteChildren(aggstate->aggcontext);
+
 		/*
 		 * Initialize working state for a new input tuple group
 		 */
@@ -1234,7 +1229,8 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
 	 * structures and transition values.  NOTE: the details of what is stored
 	 * in aggcontext and what is stored in the regular per-query memory
 	 * context are driven by a simple decision: we want to reset the
-	 * aggcontext in ExecReScanAgg to recover no-longer-wanted space.
+	 * aggcontext at group boundaries (if not hashing) and in ExecReScanAgg
+	 * to recover no-longer-wanted space.
 	 */
 	aggstate->aggcontext =
 		AllocSetContextCreate(CurrentMemoryContext,
diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c
index a1f48d8784..5b1cdd877b 100644
--- a/src/backend/utils/adt/array_userfuncs.c
+++ b/src/backend/utils/adt/array_userfuncs.c
@@ -6,7 +6,7 @@
  * Copyright (c) 2003-2009, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/adt/array_userfuncs.c,v 1.31 2009/06/20 18:45:28 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/adt/array_userfuncs.c,v 1.32 2009/07/23 20:45:27 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -539,7 +539,9 @@ array_agg_finalfn(PG_FUNCTION_ARGS)
 
 	/*
 	 * Make the result.  We cannot release the ArrayBuildState because
-	 * sometimes aggregate final functions are re-executed.
+	 * sometimes aggregate final functions are re-executed.  Rather, it
+	 * is nodeAgg.c's responsibility to reset the aggcontext when it's
+	 * safe to do so.
 	 */
 	result = makeMdArrayResult(state, 1, dims, lbs,
 							   CurrentMemoryContext,