Re: [HACKERS] aggregation memory leak and fix

> No apologies necessary.  Glad to have someone digging into that area of
> the code.  We will gladly apply your patches to 6.5.  However, I request
> that you send context diffs(diff -c).  Normal diffs are just too
> error-prone in application.   Send them, and I will apply them right
> away.
>  
Context diffs attached.  This was due to my ignorance of diff.  When I
made the other files, I though "hmm, these could be difficult to apply
if the code has changed a bit, wouldn't it be good if they included a
few lines before and after the fix".  Now I know "-c".

> Not sure why that is there?  Perhaps for GROUP BY processing?
>  
Right, it is a result of the Group processing requiring sorted input. 
Just that it doesn't "require" sorted input, it "could" be a little more
flexible and the sort wouldn't be necessary.  Essentially this would be
a single "AggSort" node that did the aggregation while sorting (probably
with replacement selection rather than quicksort).  This definitely
would require some code/smarts that isn't there today.

> > think I chased it down to the constvalue allocated in
> > execQual::ExecTargetList(), but I couldn't figure out where to properly
> > free it.  8 bytes leaked was much better than 750 bytes, so I stopped
> > banging my head on that particular item.
>  
> Can you give me the exact line?  Is it the palloc(1)?
>  
No, the 8 bytes seem to come from the ExecEvalExpr() call near line
1530.  Problem was when I tried to free these, I got "not in AllocSet"
errors, so something more complicated was going on.

Thanks.

Erik

-----------[aggregation_memory_patch.sh]----------------------

#! /bin/sh
# This is a shell archive, meaning:
# 1. Remove everything above the #! /bin/sh line.
# 2. Save the resulting text in a file.
# 3. Execute the file with /bin/sh (not csh) to create:
#	execMain.c.diff
#	execUtils.c.diff
#	nodeAgg.c.diff
#	nodeResult.c.diff
# This archive created: Fri Mar 19 19:35:42 1999
export PATH; PATH=/bin:/usr/bin:$PATH
if test -f 'execMain.c.diff'
thenecho shar: "will not over-write existing file 'execMain.c.diff'"
else
cat << \SHAR_EOF > 'execMain.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
execMain.c	Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
execMain.c	Fri Mar 19 15:03:28 1999
***************
*** 394,401 ****
--- 394,419 ----  	EndPlan(queryDesc->plantree, estate); 
+ 	/* XXX - clean up some more from ExecutorStart() - er1p */
+ 	if (NULL == estate->es_snapshot) {
+ 	  /* nothing to free */
+ 	} else {
+ 	  if (estate->es_snapshot->xcnt > 0) { 
+ 	    pfree(estate->es_snapshot->xip);
+ 	  }
+ 	  pfree(estate->es_snapshot);
+ 	}
+ 
+ 	if (NULL == estate->es_param_exec_vals) {
+ 	  /* nothing to free */
+ 	} else {
+ 	  pfree(estate->es_param_exec_vals);
+ 	  estate->es_param_exec_vals = NULL;
+ 	}
+  	/* restore saved refcounts. */ 	BufferRefCountRestore(estate->es_refcount);
+  }  void
***************
*** 580,586 **** 	/* 	 *	initialize result relation stuff 	 */
!  	if (resultRelation != 0 && operation != CMD_SELECT) 	{ 		/*
--- 598,604 ---- 	/* 	 *	initialize result relation stuff 	 */
! 	 	if (resultRelation != 0 && operation != CMD_SELECT) 	{ 		/*
SHAR_EOF
fi
if test -f 'execUtils.c.diff'
thenecho shar: "will not over-write existing file 'execUtils.c.diff'"
else
cat << \SHAR_EOF > 'execUtils.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
execUtils.c	Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
execUtils.c	Fri Mar 19 14:55:59 1999
***************
*** 272,277 ****
--- 272,278 ---- #endif 		i++; 	}
+  	if (len > 0) 	{ 		ExecAssignResultType(commonstate,
***************
*** 366,371 ****
--- 367,419 ----  	pfree(projInfo); 	commonstate->cs_ProjInfo = NULL;
+ }
+ 
+ /* ----------------
+  *		ExecFreeExprContext
+  * ----------------
+  */
+ void
+ ExecFreeExprContext(CommonState *commonstate)
+ {
+ 	ExprContext *econtext;
+ 
+ 	/* ----------------
+ 	 *	get expression context.  if NULL then this node has
+ 	 *	none so we just return.
+ 	 * ----------------
+ 	 */
+ 	econtext = commonstate->cs_ExprContext;
+ 	if (econtext == NULL)
+ 		return;
+ 
+ 	/* ----------------
+ 	 *	clean up memory used.
+ 	 * ----------------
+ 	 */
+ 	pfree(econtext);
+ 	commonstate->cs_ExprContext = NULL;
+ }
+ 
+ /* ----------------
+  *		ExecFreeTypeInfo
+  * ----------------
+  */
+ void
+ ExecFreeTypeInfo(CommonState *commonstate)
+ {
+ 	TupleDesc tupDesc;
+ 
+ 	tupDesc = commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor;
+ 	if (tupDesc == NULL)
+ 		return;
+ 
+ 	/* ----------------
+ 	 *	clean up memory used.
+ 	 * ----------------
+ 	 */
+ 	FreeTupleDesc(tupDesc);
+ 	commonstate->cs_ResultTupleSlot->ttc_tupleDescriptor = NULL; }  /* ----------------------------------------------------------------
SHAR_EOF
fi
if test -f 'nodeAgg.c.diff'
thenecho shar: "will not over-write existing file 'nodeAgg.c.diff'"
else
cat << \SHAR_EOF > 'nodeAgg.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
nodeAgg.c	Thu Mar 11 23:59:11 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
nodeAgg.c	Fri Mar 19 15:01:21 1999
***************
*** 110,115 ****
--- 110,116 ---- 				isNull2 = FALSE; 	bool		qual_result; 
+ 	Datum  oldVal = (Datum) NULL;  /* XXX - so that we can save and free
on each iteration - er1p */  	/* --------------------- 	 *	get state info from node
***************
*** 372,379 ****
--- 373,382 ---- 						 */ 						args[0] = value1[aggno]; 						args[1] = newVal;
+ 						oldVal = value1[aggno]; /* XXX - save so we can free later - er1p */ 						value1[aggno] =	(Datum) fmgr_c(&aggfns->xfn1, 										   (FmgrValues *) args, &isNull1);
+ 						pfree(oldVal); /* XXX - new, let's free the old datum - er1p */ 						Assert(!isNull1); 					} 				}
SHAR_EOF
fi
if test -f 'nodeResult.c.diff'
thenecho shar: "will not over-write existing file 'nodeResult.c.diff'"
else
cat << \SHAR_EOF > 'nodeResult.c.diff'
***
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/611/src/backend/executor/
nodeResult.c	Thu Mar 11 23:59:12 1999
---
/afs/ece.cmu.edu/project/lcs/lcs-004/er1p/postgres/612/src/backend/executor/
nodeResult.c	Fri Mar 19 14:57:26 1999
***************
*** 263,268 ****
--- 263,270 ---- 	 *		  is freed at end-transaction time.  -cim 6/2/91 	 * ---------------- 	 */
+ 	ExecFreeExprContext(&resstate->cstate); /* XXX - new for us - er1p */
+ 	ExecFreeTypeInfo(&resstate->cstate); /* XXX - new for us - er1p */ 	ExecFreeProjectionInfo(&resstate->cstate);  	/* ----------------
***************
*** 276,281 ****
--- 278,284 ---- 	 * ---------------- 	 */ 	ExecClearTuple(resstate->cstate.cs_ResultTupleSlot);
+ 	pfree(resstate); node->resstate = NULL; /* XXX - new for us - er1p */ }  void
SHAR_EOF
fi
exit 0
#	End of shell archive