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.

regards, tom lane