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".

Applied.

> > 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.

I think you will find make_groupPlan adds the sort as needed by the
GROUP BY.  I assume you are suggesting to do the aggregate/GROUP on unsorted
data, which is hard to do in a flexible way.

> > > 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.

Yes, if you look inside ExecEvalExpr(), you will see it tries to get a
value for the expression(Datum).  It may return an int, float4, or a
string.  In the last case, that is actually a pointer and not a specific
value.

So, in some cases, the value can just be thrown away, or it may be a
pointer to memory that can be freed after the call to heap_formtuple()
later in the function.  The trick is to find the function call in
ExecEvalExpr() that is allocating something, and conditionally free
values[] after the call to heap_formtuple().  If you don't want find it,
perhaps you can send me enough info so I can see it here.

I wonder whether it is the call to CreateTupleDescCopy() inside
ExecEvalVar()?

Another problem I just fixed is that fjIsNull was not being pfree'ed if
it was used with >64 targets, but I don't think that affects you.

I also assume you have run your recent patch through the the
test/regression tests, so see it does not cause some other area to fail,
right?

--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026