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(at)candle(dot)pha(dot)pa(dot)us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026