Memory leaks

Greg Copeland <greg@CopelandConsulting.Net> writes:

I've started playing a little with Postgres to determine if there were
memory leaks running around. After some very brief checking, I'm
starting[1] to think that the answer is yes. Has anyone already gone
through a significant effort to locate and eradicate memory leaks?

Yes, this has been dealt with before. Have you read
src/backend/utils/mmgr/README?

AFAIK the major problems these days are not in the backend as a whole,
but in the lesser-used PL language modules (cf recent discussions).
plpgsql has some issues too, I suspect, but not as bad as pltcl etc.
Possibly the best answer is to integrate the memory-context notion into
those modules; if they did most of their work in a temp context that
could be freed once per PL statement or so, the problems would pretty
much go away.

It's fairly difficult to get anywhere with standard leak-tracking tools,
since they don't know anything about palloc. What's worse, it is *not*
a bug for a routine to palloc space it never pfrees, if it knows that
it's palloc'ing in a short-lived memory context. The fact that a
context may be released with much still-allocated memory in it is not a
bug but a feature; but try teaching that to any standard leak checker...

regards, tom lane

On Tue, 2002-10-22 at 17:09, Tom Lane wrote:

Greg Copeland <greg@CopelandConsulting.Net> writes:

I've started playing a little with Postgres to determine if there were
memory leaks running around. After some very brief checking, I'm
starting[1] to think that the answer is yes. Has anyone already gone
through a significant effort to locate and eradicate memory leaks?

Yes, this has been dealt with before.

What tools, aside from noggin v1.0, did they use? Do we know?

Have you read
src/backend/utils/mmgr/README?

Yes...but it's been some time since I last opened it. It was because I
knew there were some caveats that I wasn't attempting to claim for sure
that there were leaks.

I then moved on to psql, again, just for fun. Here, I'm thinking that I
started to find some other leaks...but again, I've not spent any real
time on it. So again, I'm not really sure it they are meaningful at
this point.

AFAIK the major problems these days are not in the backend as a whole,
but in the lesser-used PL language modules (cf recent discussions).

Ya, that's what made me wonder about the topic on a larger scale.

plpgsql has some issues too, I suspect, but not as bad as pltcl etc.
Possibly the best answer is to integrate the memory-context notion into
those modules; if they did most of their work in a temp context that
could be freed once per PL statement or so, the problems would pretty
much go away.

Interesting. Having not looked at memory management schemes used in the
pl implementations, can you enlighten me by what you mean by "integrate
the memory-context notion"? Does that mean they are not using
palloc/pfree stuff?

It's fairly difficult to get anywhere with standard leak-tracking tools,
since they don't know anything about palloc. What's worse, it is *not*
a bug for a routine to palloc space it never pfrees, if it knows that
it's palloc'ing in a short-lived memory context. The fact that a
context may be released with much still-allocated memory in it is not a
bug but a feature; but try teaching that to any standard leak checker...

regards, tom lane

Well, the thing that really got my attention is that dmalloc is
reporting frees on null pointers. While that may be safe on specific
platforms, IIRC, it's actually undefined. Again, this is as reported by
dmalloc so I've yet to actually track it down to determine if it's
possibly something else going on (magic voodoo of some heap manager,
etc).

Greg

On 22 Oct 2002, Greg Copeland wrote:

On Tue, 2002-10-22 at 17:09, Tom Lane wrote:

plpgsql has some issues too, I suspect, but not as bad as pltcl etc.
Possibly the best answer is to integrate the memory-context notion into
those modules; if they did most of their work in a temp context that
could be freed once per PL statement or so, the problems would pretty
much go away.

Interesting. Having not looked at memory management schemes used in the
pl implementations, can you enlighten me by what you mean by "integrate
the memory-context notion"? Does that mean they are not using
palloc/pfree stuff?

I saw use of a couple of malloc (or Python specific malloc) calls the other day
but I also seem to recall that, after consideration, I decided the memory
needed to survive for the duration of the backend. Should I have created a new
child of the top context and changed these malloc calls?

I was going to ask about thoughts on redirecting malloc etc to palloc etc and
thereby intercepting memory allocation within the languages and automatically
bringing them into the memory context realm. However, that would just be making
life way too awkward, bearing in mind the above paragraph. Can't we get Sir
Mongle (or whatever the name was) to test these things under the auspices of
them being DoS attacks?

--
Nigel J. Andrews

"Nigel J. Andrews" <nandrews@investsystems.co.uk> writes:

I saw use of a couple of malloc (or Python specific malloc) calls the
other day but I also seem to recall that, after consideration, I
decided the memory needed to survive for the duration of the
backend. Should I have created a new child of the top context and
changed these malloc calls?

If there is truly no reason to release the memory before the backend
dies, then I see no reason to avoid malloc(). Basically what the memory
context stuff gives you is the ability to bunch allocations together and
release a whole tree of stuff for little work.

An example: currently, the plpgsql parser builds its parsed syntax tree
with a bunch of malloc's. It has no way to free a syntax tree, so that
tree lives till the backend exits, whether it's ever used again or not.
It would be better to build the tree via palloc's in a context created
specially for the specific function: then we could free the whole
context if we discovered that the function had gone away or been
redefined. (Compare what relcache does for rule syntaxtrees for rules
associated with relcache entries.) But right at the moment, there's no
mechanism to discover that situation, and so there's not now any direct
value in using palloc instead of malloc for plpgsql syntaxtrees. Yet
that's surely the direction to go in for the long term.

regards, tom lane

Greg Copeland <greg@copelandconsulting.net> writes:

On Tue, 2002-10-22 at 17:09, Tom Lane wrote:

Yes, this has been dealt with before.

What tools, aside from noggin v1.0, did they use? Do we know?

s/they/me/ ... none. But I don't know of any that I think would be
useful.

I then moved on to psql, again, just for fun. Here, I'm thinking that I
started to find some other leaks...but again, I've not spent any real
time on it. So again, I'm not really sure it they are meaningful at
this point.

psql might well have some internal leaks; the backend memory-context
design doesn't apply to it.

Possibly the best answer is to integrate the memory-context notion into
those modules; if they did most of their work in a temp context that
could be freed once per PL statement or so, the problems would pretty
much go away.

Interesting. Having not looked at memory management schemes used in the
pl implementations, can you enlighten me by what you mean by "integrate
the memory-context notion"? Does that mean they are not using
palloc/pfree stuff?

Not everywhere. plpgsql is full of malloc's and I think the other PL
modules are too --- and that's not to mention the allocation policies of
the perl, tcl, etc, language interpreters. We could use a thorough
review of that whole area.

Well, the thing that really got my attention is that dmalloc is
reporting frees on null pointers.

AFAIK that would dump core on many platforms (it sure does here...),
so I don't think I believe it without seeing chapter and verse. But
if you can point out where it's really happening, then we must fix it.

regards, tom lane

On Tue, Oct 22, 2002 at 11:28:23PM -0400, Tom Lane wrote:

I then moved on to psql, again, just for fun. Here, I'm thinking that I
started to find some other leaks...but again, I've not spent any real
time on it. So again, I'm not really sure it they are meaningful at
this point.

psql might well have some internal leaks; the backend memory-context
design doesn't apply to it.

But why? In the Mape project is used mmgr based on PostgreSQL's mmgr and
it's used for BE and FE. There is not problem with it (BTW backend is
multithread:-). IMHO use memory-context design for FE is good idea
if FE a lot works with memory. I already long time think about shared
lib with PostgreSQL mmgr...

Karel

--
Karel Zak <zakkr@zf.jcu.cz>
http://home.zf.jcu.cz/~zakkr/

C, PostgreSQL, PHP, WWW, http://docs.linux.cz, http://mape.jcu.cz

On Tue, 22 Oct 2002, Tom Lane wrote:

Greg Copeland <greg@copelandconsulting.net> writes:

Interesting. Having not looked at memory management schemes used in the
pl implementations, can you enlighten me by what you mean by "integrate
the memory-context notion"? Does that mean they are not using
palloc/pfree stuff?

Not everywhere. plpgsql is full of malloc's and I think the other PL
modules are too --- and that's not to mention the allocation policies of
the perl, tcl, etc, language interpreters...

I was going to make the suggestion that malloc et al. could be replaced with
palloc etc but then that raises too many complications without just shooving
everything into a long lived context anyway. Also I think we've got to rely on,
i.e. it is sensible to do so, the underlying language handling memory
correctly.

Hmmm...there do seem to be a few mallocs in plpython.c . I haven't looked very
closely but nothing jumped out at me as being obviously wrong from the grep
output.

--
Nigel J. Andrews

On Tue, 2002-10-22 at 22:28, Tom Lane wrote:

Greg Copeland <greg@copelandconsulting.net> writes:

So again, I'm not really sure it they are meaningful at
this point.

psql might well have some internal leaks; the backend memory-context
design doesn't apply to it.

Okay. Thanks. I'll probably take another look at it a little later and
report back if I find anything of value.

Does that mean they are not using
palloc/pfree stuff?

Not everywhere. plpgsql is full of malloc's and I think the other PL
modules are too --- and that's not to mention the allocation policies of
the perl, tcl, etc, language interpreters. We could use a thorough
review of that whole area.

Okay. I've started looking at plpython to better understand it's memory
needs. I'm seeing a mix of mallocs and PLy_malloc. The PLy version is
basically malloc which also checks and reports on memory allocation
errors. Anyone know if the cases where malloc was used was purposely
done so for performance reasons or simply the flavor or the day?

I thinking for starters, the plpython module could be normalized to use
the PLy_malloc stuff across the board. Then again, I still need to
spend some more time on it. ;)

Well, the thing that really got my attention is that dmalloc is
reporting frees on null pointers.

AFAIK that would dump core on many platforms (it sure does here...),
so I don't think I believe it without seeing chapter and verse.

I actually expected it to do that here on my x86-Linux platform but a
quick check showed that it was quiet happy with it. What platforms are
you using -- just curious?

But if you can point out where it's really happening, then we must fix it.

I'll trying track this down some more this coming week to see if this is
really occurring. After thinking about it, I'm not sure why dmalloc
would ever report a free on null if it were not actually occurring.
After all, any call to free should still be showing some memory address
(valid or otherwise). Off the top of my head, I can't think of an
artifact that would cause it to falsely report it.

Greg

"Nigel J. Andrews" <nandrews@investsystems.co.uk> writes:

On Tue, 22 Oct 2002, Tom Lane wrote:

Not everywhere. plpgsql is full of malloc's and I think the other PL
modules are too --- and that's not to mention the allocation policies of
the perl, tcl, etc, language interpreters...

I was going to make the suggestion that malloc et al. could be replaced with
palloc etc but then that raises too many complications without just shooving
everything into a long lived context anyway. Also I think we've got to rely on,
i.e. it is sensible to do so, the underlying language handling memory
correctly.

If the perl/tcl/etc interpreters have internal memory leaks, there's
little we can do about that except file bug reports. What I was
wondering about is whether there isn't deliberate action we need to
take to inform those interpreters when data is no longer required.

An example: when a procedure is updated with CREATE OR REPLACE FUNCTION,
the only thing pltcl does about it is a solitary Tcl_DeleteHashEntry();
it doesn't try to tell Tcl to delete the existing Tcl procedure. That
might happen for free (will we always regenerate the same Tcl procedure
name? not sure), but if the omission causes a leak it's surely not Tcl's
fault. That's on top of our own data structures about the pltcl
function (pltcl_proc_desc and related stuff), which are definitely
leaked in this scenario.

Sticking all the data about a given function into a memory context
that's specific to the function would make it easier to reclaim our own
memory in this scenario, but we'd still have to tell Tcl to clean up
its own memory.

Actually pltcl's internal structures about a function look simple enough
that it may not be worth using a context for them. It would definitely
be useful to do that for plpgsql, though, which builds an extremely
complicated structure for each function (and leaks it all on
function redefinition :-().

regards, tom lane

Greg Copeland <greg@copelandconsulting.net> writes:

Okay. I've started looking at plpython to better understand it's memory
needs. I'm seeing a mix of mallocs and PLy_malloc. The PLy version is
basically malloc which also checks and reports on memory allocation
errors. Anyone know if the cases where malloc was used was purposely
done so for performance reasons or simply the flavor or the day?

Probably either oversight or the result of different people's different
coding styles.

I thinking for starters, the plpython module could be normalized to use
the PLy_malloc stuff across the board. Then again, I still need to
spend some more time on it. ;)

Consistency is good. What I'd wonder about, though, is whether you
shouldn't be using palloc ;-). malloc, with or without a PLy_ wrapper,
doesn't provide any leverage to help you get rid of stuff when you don't
want it anymore.

Well, the thing that really got my attention is that dmalloc is
reporting frees on null pointers.

AFAIK that would dump core on many platforms (it sure does here...),

I have to take that back: I was thinking about pfree() not free().
The ANSI C spec says that free(NULL) is a legal no-op, and there are
just a few ancient C libraries (perhaps none anymore) where it'll crash.
I tend to do "if (ptr) free(ptr)" from force of habit, but I notice that
psql (among other places) relies heavily on the ANSI behavior. It's
probably pointless to try to convince people to change that coding style.

regards, tom lane

On Wed, 2002-10-23 at 08:48, Tom Lane wrote:

Greg Copeland <greg@copelandconsulting.net> writes:

Okay. I've started looking at plpython to better understand it's memory
needs. I'm seeing a mix of mallocs and PLy_malloc. The PLy version is
basically malloc which also checks and reports on memory allocation
errors. Anyone know if the cases where malloc was used was purposely
done so for performance reasons or simply the flavor or the day?

Probably either oversight or the result of different people's different
coding styles.

My local copy has this changed to PLy stuff now. Testing shows it's
good...then again, I didn't really expect it to change anything. I'll
submit patches later.

I thinking for starters, the plpython module could be normalized to use
the PLy_malloc stuff across the board. Then again, I still need to
spend some more time on it. ;)

Consistency is good. What I'd wonder about, though, is whether you
shouldn't be using palloc ;-). malloc, with or without a PLy_ wrapper,
doesn't provide any leverage to help you get rid of stuff when you don't
want it anymore.

Ya, I'm currently looking to see how the memory is being used and why.
I'm trying to better understand it's life cycle. You implying that even
the short term memory should be using the palloc stuff? What about long
term? Blanket statement that pretty much all the PLy stuff should
really be using palloc?

Well, the thing that really got my attention is that dmalloc is
reporting frees on null pointers.

AFAIK that would dump core on many platforms (it sure does here...),

I have to take that back: I was thinking about pfree() not free().
The ANSI C spec says that free(NULL) is a legal no-op, and there are

Oh really. I didn't realize that. I've been using the "if( ptr ) "
stuff for so long I didn't realize I didn't need to anymore. Thanks for
the update. That was, of course, the cause for alarm.

It's
probably pointless to try to convince people to change that coding style.

Well at this late time, I think it's safe to say that it's not causing
problems for anyone on any of the supported platforms. So I'll not
waste time looking for it even though I happen think it's a poor
practice just the same.

Thanks,

Greg

Greg Copeland <greg@copelandconsulting.net> writes:

Ya, I'm currently looking to see how the memory is being used and why.
I'm trying to better understand it's life cycle. You implying that even
the short term memory should be using the palloc stuff? What about long
term? Blanket statement that pretty much all the PLy stuff should
really be using palloc?

Short-term stuff almost certainly should be using palloc, IMHO; anything
that is not going to survive the current function invocation should be
palloc'd in CurrentMemoryContext. The main reason for this is that you
don't need to fear leaking such memory if the function is aborted by
elog(). Depending on what you are doing, you may not have to bother
with explicit pfree's at all for such stuff. (In a PL handler you could
probably get away with omitting pfree's for stuff allocated once per
call, but not for stuff allocated once per statement. Unless you were to
make a new context that gets reset for each statement ... which might be
a good idea.)

For stuff that is going to live a long time and then be explicitly
freed, I don't think there's a hard-and-fast rule about which to use.
If you are building a complex data structure (parsetree, say) then it's
going to be easier to build it in a memory context and then just free
the context rather than tearing down the data structure piece-by-piece.
But when you are talking about a single object, there's not a heck of a
lot of difference between malloc() and palloc in TopMemoryContext.

I'd lean towards using the palloc routines anyway, for consistency of
coding style, but that's a judgment call not a must-do thing.

regards, tom lane

Greg Copeland wrote:

I've started playing a little with Postgres to determine if there were
memory leaks running around. After some very brief checking, I'm
starting[1] to think that the answer is yes. Has anyone already gone
through a significant effort to locate and eradicate memory leaks? Is
this done periodically? If so, what tools are others using? I'm
currently using dmalloc for my curiosity.

[1] Not sure yet as I'm really wanting to find culumative leaks rather
than one shot allocations which are simply never freed prior to process
termination.

While all leaks should be fixed, obviously, this is one of the "good"
things in the parennial "Thread vs process" argument for processes.