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'
then
echo 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'
then
echo 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'
then
echo 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'
then
echo 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