Re: [HACKERS] aggregation memory leak and fix

> Bruce Momjian <maillist(at)candle(dot)pha(dot)pa(dot)us> writes:
> >> What we need to do is work out what the best set of memory context
> >> definitions is, and then decide on a strategy for making sure that
> >> lower-level routines allocate their return values in the right context.
>
> > Let's suppose that we want to free all the memory used as expression
> > intermediate values after each row is processed.
> > It is my understanding that all these are created in utils/adt/*.c
> > files, and that the entry point to all those functions via
> > fmgr()/fmgr_c().
>
> That's probably the bulk of the specific calls of palloc(). Someone
> (Jan?) did a scan of the code a while ago looking for palloc() calls,
> and there aren't that many outside of the data-type-specific functions.
> But we'd have to look individually at all the ones that are elsewhere.
>
> > So, if we go into an expression memory context before calling
> > fmgr/fmgr_c in the executor, and return to the normal context after the
> > function call, all our intermediates are trapped in the expression
> > memory context.
>
> OK, so you're saying we leave the data-type-specific functions as is
> (calling palloc() to allocate their result areas), and make each call
> site specifically responsible for setting the context that palloc() will
> allocate from? That could work, I think. We'd need to see what side
> effects it'd have on other uses of palloc().
>
> What we'd probably want is to use a stack discipline for the current
> palloc-target memory context: when you set the context, you get back the
> ID of the old context, and you are supposed to restore that old context
> before returning.
>
> > At the end of each row, we just free the expression memory context. In
> > almost all cases, the data is stored in tuples, and we can free it. In
> > a few cases like aggregates, we have to save off the value we need to
> > keep before freeing the expression context.
>
> Actually, nodeAgg would just have to set an appropriate context before
> calling fmgr to execute the aggregate's transition functions, and then
> it wouldn't need an extra copy step. The results would come back in the
> right context already.
>
> > In fact, you could even optimize the cleanup to only do free'ing if
> > some expression memory was allocated. In most cases, it is not.
>
> Jan's stuff should already fall through pretty quickly if there's
> nothing in the context, I think. Note that what we want to do between
> tuples is a "context clear" of the expression context, not a "context
> delete" and then "context create" a new expression context. Context
> clear should be a pretty quick no-op if nothing's been allocated in that
> context...
>
> > In fact the nodeAgg.c patch that I backed out attempted to do that,
> > though because there wasn't code that checked if the Datum was
> > pg_type.typbyval, it didn't work 100%.
>
> Right. But if we approach it this way (clear the context at appropriate
> times) rather than thinking in terms of explicitly pfree'ing individual
> objects, life gets much simpler. Also, if we insist on being able to
> pfree individual objects inside a context, we can't use Jan's faster
> allocator! Remember, the reason it is faster and lower overhead is that
> it doesn't keep track of individual objects, only pools.
>
> I'd like to see us head in the direction of removing most of the
> explicit pfree calls that exist now, and instead rely on clearing
> memory contexts at appropriate times in order to manage memory.
> The fewer places where we need pfree, the more contexts can be run
> with the low-overhead space allocator. Also, the fewer explicit
> pfrees we need, the simpler and more reliable the code gets.

Tom, are you saying you agree with my approach, and I should give it a
try?

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