Обсуждение: BUG #4496: Memory leak in pg_dump.c?
The following bug has been logged online: Bug reference: 4496 Logged by: Email address: dvice_null@yahoo.com PostgreSQL version: Latest cvs Operating system: Error in source code Description: Memory leak in pg_dump.c? Details: In file src/bin/pg_dump/pg_dump.c:3741 It seems like variables "constrinfo" and "indxinfo" leak memory e.g. at line 3741 and also on many other similar functions. If the memory is released somewhere I failed to find the location where it is released. So please either fix the leak if there is a leak or document better where the memory is released.
dvice_null@yahoo.com napsal(a):
> The following bug has been logged online:
>
> Bug reference: 4496
> Logged by:
> Email address: dvice_null@yahoo.com
> PostgreSQL version: Latest cvs
> Operating system: Error in source code
> Description: Memory leak in pg_dump.c?
> Details:
>
> In file src/bin/pg_dump/pg_dump.c:3741
>
> It seems like variables "constrinfo" and "indxinfo" leak memory e.g. at line
> 3741 and also on many other similar functions. If the memory is released
> somewhere I failed to find the location where it is released. So please
> either fix the leak if there is a leak or document better where the memory
> is released.
>
I think all information is collected and they are used for all pg_dump run. It
does not make sense to free them. See line 725.
Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql
--- Zdenek Kotala <Zdenek.Kotala@Sun.COM> wrote: > dvice_null@yahoo.com napsal(a): > > The following bug has been logged online: > > > > Bug reference: 4496 > > Logged by: > > Email address: dvice_null@yahoo.com > > PostgreSQL version: Latest cvs > > Operating system: Error in source code > > Description: Memory leak in pg_dump.c? > > Details: > > > > In file src/bin/pg_dump/pg_dump.c:3741 > > > > It seems like variables "constrinfo" and > "indxinfo" leak memory e.g. at line > > 3741 and also on many other similar functions. If > the memory is released > > somewhere I failed to find the location where it > is released. So please > > either fix the leak if there is a leak or document > better where the memory > > is released. > > > > I think all information is collected and they are > used for all pg_dump run. It > does not make sense to free them. See line 725. > > Zdenek But is the memory freed at some point? E.g. when program shuts down? If it is not freed ever, then it is a memory leak. I'm interested in this, because either you have a memory leak or these is a bug in another application called cppcheck which claims that you have a memory leak. But if it is too difficult to find out, I won't bother you about this anymore as it is most likely a minor issue if even that.
Aggro wrote: > > --- Zdenek Kotala <Zdenek.Kotala@Sun.COM> wrote: > > > I think all information is collected and they are > > used for all pg_dump run. It > > does not make sense to free them. See line 725. > > But is the memory freed at some point? E.g. when > program shuts down? If it is not freed ever, then it > is a memory leak. A pg_dump run is comparatively short-lived, so if Zdenek is right then there's no important leak here -- we're counting on program exit to release the memory. There's probably little point in releasing things earlier than that. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Aggro <dvice_null@yahoo.com> writes:
> But is the memory freed at some point? E.g. when
> program shuts down? If it is not freed ever, then it
> is a memory leak.
Program-local memory is *always* freed at process termination. Any
operating system that failed to handle this would be broken beyond
usability, because any simple program crash would lead to loss of
usable memory. Eventually you'd be forced to reboot the OS just
because of application-level bugs.
Now, leakage of the Postgres shared memory segment after a postmaster
crash is a genuine issue. We have some code that tries to detect and
reclaim an orphaned segment, but it's not bulletproof and sometimes
manual cleanup (or a reboot) is needed. (It can't be too aggressive
because the opposite risk, reclaiming a segment that *is* still in
use, would have far worse consequences than a mere memory leak.)
> I'm interested in this, because
> either you have a memory leak or these is a bug in
> another application called cppcheck which claims that
> you have a memory leak.
We don't put too much stock in automatic leak analysis, because
there aren't any automated checkers that understand Postgres'
memory context mechanism. They don't know about shmem either ...
regards, tom lane
> A pg_dump run is comparatively short-lived, so if Zdenek is right then > there's no important leak here -- we're counting on program exit to > release the memory. There's probably little point in releasing things > earlier than that. Well, I'd tend to consider any logical part of a program that fails to release the memory it uses to be bad coding practice. You never know when you're going to need to shuffle things around, change the context of the code in a way that makes it long-lived, in turn causing the leak to become a real problem. Also, don't you like seeing the free()s paired to their mallocs()s in a way that makes the allocations intuitively correct? :) -- Tomáš Szépe <szepe@pinerecords.com>
Tomáš Szépe wrote: >> A pg_dump run is comparatively short-lived, so if Zdenek is right then >> there's no important leak here -- we're counting on program exit to >> release the memory. There's probably little point in releasing things >> earlier than that. > > Well, I'd tend to consider any logical part of a program that fails to > release the memory it uses to be bad coding practice. You never know > when you're going to need to shuffle things around, change the context > of the code in a way that makes it long-lived, in turn causing the leak > to become a real problem. I find that documenting where alloations are not correspondingly free()'d is usually sufficient for this. Personally I usually don't bother freeing some allocations in short-lived programs, but I make sure I know what they are and I'll usually have code in there to free them if the binary is built for debugging mostly to stop people reporting bogus memory leaks. There are good reasons not to free memory if a program will shortly be terminating and thus all its resources will be freed by the operating system. Sometimes explicitly freeing large piles of memory, especially if there's any sort of automatic cleanup associated, can take real time that you don't really want to waste when you're about to terminate anyway. Additionally, sometimes it's hard to know when it's safe to free a structure, but you always know it's safe for the OS to release the program's memory mappings after it terminates. This is a particular issue in libraries that use various caching and reuse mechanisms, have long-lived service providers, or do other things that may not be visible to the program using the library. It's a good idea to provide a libraryname_cleanup() call or similar, but it's probably only going to be useful if the app is being run under a memory leak checker or the like, and even then it's only useful to reduce noise. Memory leaks that actually matter are where memory is allocated and not freed as part of a process that is repeated many times in a program, or that consumes a huge amount of memory. > Also, don't you like seeing the free()s paired > to their mallocs()s in a way that makes the allocations intuitively > correct? :) No - I like to use lexically scoped instances of useful classes to hide the minutae of memory management behind a nice RAII interface. Think std::vector<T>. When forced, I like to use std::auto_ptr<> or std::tr1::shared_ptr<> (or boost::shared_ptr<> if TR1 is not available) as appropriate to manage the block. I in fact loathe seeing malloc() and free() [ or operator new() and operator delete() ] pairs, because every one is a potential leak or double free bug. Then again, I use C++ by preference, where you have those more sophisticated resource management options available to you. In the case of PostgreSQL code, of course, you just use palloc() and let it's management of memory contexts take care of everything for you. That sort of domain-specific smart memory management seems to be the best way to go whenever you can use it - see also, for example, Samba's talloc() and GhostScript's reference counting allocator. So ... in short, I see malloc() and free() as rather low level, and not something I want to see at all in the significant parts of any nontrivial program. -- Craig Ringer
Aggro napsal(a):
>> I think all information is collected and they are
>> used for all pg_dump run. It
>> does not make sense to free them. See line 725.
>>
>> Zdenek
>
> But is the memory freed at some point? E.g. when
> program shuts down? If it is not freed ever, then it
> is a memory leak. I'm interested in this, because
> either you have a memory leak or these is a bug in
> another application called cppcheck which claims that
> you have a memory leak. But if it is too difficult to
> find out, I won't bother you about this anymore as it
> is most likely a minor issue if even that.
Keep in mind that onetime allocated local memory is not reclaimed to OS until
process finished. See sbrk(2) It does not make sense to free memory at the end
(in pg_dump) because it does not have any effect on available memory in OS and
OS clean it soon anyway. I agree that from general point of view it is good to
release unused memory, because process can reuse it. But when you know what you
do then you can break a rule.
Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql
On Thursday 30 October 2008, Tom=C3=A1=C5=A1 Sz=C3=A9pe wrote: > > A pg_dump run is comparatively short-lived, so if Zdenek is right then > > there's no important leak here -- we're counting on program exit to > > release the memory. There's probably little point in releasing things > > earlier than that. > > Well, I'd tend to consider any logical part of a program that fails to > release the memory it uses to be bad coding practice. You never know > when you're going to need to shuffle things around, change the context > of the code in a way that makes it long-lived, in turn causing the leak > to become a real problem. Also, don't you like seeing the free()s paired > to their mallocs()s in a way that makes the allocations intuitively > correct? :) Unfreed memory is not the same as leaked memory. Leaked memory is unfreed= =20 memory which should have been freed. Sometimes it's best not to do it. If= =20 done it purposefully some times. Once I had to build a HUGE tree of data ju= st=20 to print some reports. I alloced the nodes ( by carving small chunks from b= ig=20 malloced blocks, but this was an optimization ) incrementally, finished=20 input, wrote output and just exit(), because I knew the OS will free the=20 memory, and keeping track of all the malloced blocks just to free them befo= re=20 exiting to the OS is just wasting memory, and freeing them wastes time. The= =20 OS guarantees on freeing memory, closing files and other autmoatic resource= =20 releasing are there to be used when needed. Similarly nearly nobody bothers= =20 to fclose() stdin/out/err or close 0/1/2, the OS will do it if needed. And= =20 many malloc implementations grow the data segment using sbrk(), but don't= =20 reduce it to the minimum when exiting to the OS. Francisco Olarte.
>>> Francisco Olarte Sanz <folarte@peoplecall.com> wrote: > Similarly nearly nobody bothers > to fclose() stdin/out/err On that one, maybe it should be done more often. In writing pg_clearxlogtail I found that closing stdout improved performance markedly. This was a filter piping from disk into gzip. -Kevin