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