I have been looking into why a reference to a nonexistent table, eg
	INSERT INTO nosuchtable VALUES(1);
leaks a small amount of memory per occurrence.  What I find is a
memory leak in the indexscan support.  Specifically,
RelationGetIndexScan in backend/access/index/genam.c palloc's both
an IndexScanDesc and some keydata storage.  The IndexScanDesc
block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
in backend/catalog/indexing.c.  But the keydata block is not.

This wouldn't matter so much if the palloc were coming from a
transaction-local context.  But what we're doing is a lookup in pg_class
on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
it's done a MemoryContextSwitchTo into the global CacheCxt before
starting the lookup.  Therefore, the un-pfreed block represents a
permanent memory leak.

In fact, *every* reference to a relation that is not already present in
the relcache causes a similar leak.  The error case is just the one that
is easiest to repeat.  The missing pfree of the keydata block is
probably causing a bunch of other short-term and long-term leaks too.

It seems to me there are two things to fix here: indexscan ought to
pfree everything it pallocs, and RelationBuildDesc ought to be warier
about how much work gets done with CacheCxt as the active palloc
context.  (Even if indexscan didn't leak anything ordinarily, there's
still the risk of elog(ERROR) causing an abort before the indexscan code
gets to clean up.)

Comments?  In particular, where is the cleanest place to add the pfree
of the keydata block?  I don't especially like the fact that callers
of index_endscan have to clean up the toplevel scan block; I think that
ought to happen inside index_endscan.

			regards, tom lane

Sat, 15 May 1999 23:09:46 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Memory leaks in relcache
> I have been looking into why a reference to a nonexistent table, eg
> 	INSERT INTO nosuchtable VALUES(1);
> leaks a small amount of memory per occurrence.  What I find is a
> memory leak in the indexscan support.  Specifically,
> RelationGetIndexScan in backend/access/index/genam.c palloc's both
> an IndexScanDesc and some keydata storage.  The IndexScanDesc
> block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
> in backend/catalog/indexing.c.  But the keydata block is not.
> 
> This wouldn't matter so much if the palloc were coming from a
> transaction-local context.  But what we're doing is a lookup in pg_class
> on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
> it's done a MemoryContextSwitchTo into the global CacheCxt before
> starting the lookup.  Therefore, the un-pfreed block represents a
> permanent memory leak.
> 
> In fact, *every* reference to a relation that is not already present in
> the relcache causes a similar leak.  The error case is just the one that
> is easiest to repeat.  The missing pfree of the keydata block is
> probably causing a bunch of other short-term and long-term leaks too.
> 
> It seems to me there are two things to fix here: indexscan ought to
> pfree everything it pallocs, and RelationBuildDesc ought to be warier
> about how much work gets done with CacheCxt as the active palloc
> context.  (Even if indexscan didn't leak anything ordinarily, there's
> still the risk of elog(ERROR) causing an abort before the indexscan code
> gets to clean up.)
> 
> Comments?  In particular, where is the cleanest place to add the pfree
> of the keydata block?  I don't especially like the fact that callers
> of index_endscan have to clean up the toplevel scan block; I think that
> ought to happen inside index_endscan.

You are certainly on to something.  Every call to index_endscan() either
calls pfree() just after the call to free the descriptor, or should.  I
recommend doing the pfree in the index_endscan, and removing the
individual pfree's after the index_endscan call.  I also recommend doing
pfree'ing the keys inside index_endscan() and see what happens.  The
regression test should show any problems.  I can easily do this is you
wish.


-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Mon, 17 May 1999 00:53:53 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] Memory leaks in relcache
> It seems to me there are two things to fix here: indexscan ought to
> pfree everything it pallocs, and RelationBuildDesc ought to be warier
> about how much work gets done with CacheCxt as the active palloc
> context.  (Even if indexscan didn't leak anything ordinarily, there's
> still the risk of elog(ERROR) causing an abort before the indexscan code
> gets to clean up.)

As far as cleaning up from an elog, my only idea would be to have a
global List that contains pointers that should be freed from any elog().
The cache code would lconc() any of its pointers onto the list, and an
elog() would check the list and free anything on there.  The problem is
that many times the palloc's happen in non-cache functions, so the cache
code may not have access to the palloc address, and if we put it
everywhere, we are doing this for non-cache calls, which may be too much
overhead.  We could also try clearing the cache on an elog() but that
seems extreme too.

ie, cache function calls a function that allocates memory then calls
another function that fails.  The memory is in cache context, but the
cache function never saw a return from it's first call, so it couldn't
add it to the elog global free list.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Mon, 17 May 1999 01:21:11 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] Memory leaks in relcache

Tom, where are we on this.  As I remember, it is still an open issue,
right?  I can add it to the TODO list.


> I have been looking into why a reference to a nonexistent table, eg
> 	INSERT INTO nosuchtable VALUES(1);
> leaks a small amount of memory per occurrence.  What I find is a
> memory leak in the indexscan support.  Specifically,
> RelationGetIndexScan in backend/access/index/genam.c palloc's both
> an IndexScanDesc and some keydata storage.  The IndexScanDesc
> block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
> in backend/catalog/indexing.c.  But the keydata block is not.
> 
> This wouldn't matter so much if the palloc were coming from a
> transaction-local context.  But what we're doing is a lookup in pg_class
> on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
> it's done a MemoryContextSwitchTo into the global CacheCxt before
> starting the lookup.  Therefore, the un-pfreed block represents a
> permanent memory leak.
> 
> In fact, *every* reference to a relation that is not already present in
> the relcache causes a similar leak.  The error case is just the one that
> is easiest to repeat.  The missing pfree of the keydata block is
> probably causing a bunch of other short-term and long-term leaks too.
> 
> It seems to me there are two things to fix here: indexscan ought to
> pfree everything it pallocs, and RelationBuildDesc ought to be warier
> about how much work gets done with CacheCxt as the active palloc
> context.  (Even if indexscan didn't leak anything ordinarily, there's
> still the risk of elog(ERROR) causing an abort before the indexscan code
> gets to clean up.)
> 
> Comments?  In particular, where is the cleanest place to add the pfree
> of the keydata block?  I don't especially like the fact that callers
> of index_endscan have to clean up the toplevel scan block; I think that
> ought to happen inside index_endscan.
> 
> 			regards, tom lane
> 
> 


-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Wed, 7 Jul 1999 04:16:51 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] Memory leaks in relcache
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Tom, where are we on this.  As I remember, it is still an open issue,
> right?  I can add it to the TODO list.

I have not done anything about it yet; it ought to be in TODO.

I'm also aware of two or three other sources of small but permanent
memory leaks, btw; have them in my todo list.

			regards, tom lane

>> I have been looking into why a reference to a nonexistent table, eg
>> INSERT INTO nosuchtable VALUES(1);
>> leaks a small amount of memory per occurrence.  What I find is a
>> memory leak in the indexscan support.  Specifically,
>> RelationGetIndexScan in backend/access/index/genam.c palloc's both
>> an IndexScanDesc and some keydata storage.  The IndexScanDesc
>> block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
>> in backend/catalog/indexing.c.  But the keydata block is not.
>> 
>> This wouldn't matter so much if the palloc were coming from a
>> transaction-local context.  But what we're doing is a lookup in pg_class
>> on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
>> it's done a MemoryContextSwitchTo into the global CacheCxt before
>> starting the lookup.  Therefore, the un-pfreed block represents a
>> permanent memory leak.
>> 
>> In fact, *every* reference to a relation that is not already present in
>> the relcache causes a similar leak.  The error case is just the one that
>> is easiest to repeat.  The missing pfree of the keydata block is
>> probably causing a bunch of other short-term and long-term leaks too.
>> 
>> It seems to me there are two things to fix here: indexscan ought to
>> pfree everything it pallocs, and RelationBuildDesc ought to be warier
>> about how much work gets done with CacheCxt as the active palloc
>> context.  (Even if indexscan didn't leak anything ordinarily, there's
>> still the risk of elog(ERROR) causing an abort before the indexscan code
>> gets to clean up.)
>> 
>> Comments?  In particular, where is the cleanest place to add the pfree
>> of the keydata block?  I don't especially like the fact that callers
>> of index_endscan have to clean up the toplevel scan block; I think that
>> ought to happen inside index_endscan.
>> 
>> regards, tom lane
>> 
>> 


> -- 
>   Bruce Momjian                        |  http://www.op.net/~candle
>   maillist@candle.pha.pa.us            |  (610) 853-3000
>   +  If your life is a hard drive,     |  830 Blythe Avenue
>   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Wed, 07 Jul 1999 10:24:31 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] Memory leaks in relcache 
Added to TODO:

* fix indexscan() so it does leak memory by not requiring caller to free
* improve dynamic memory allocation by introducing tuple-context memory
  allocation
* fix memory leak in cache code when non-existant table is referenced

> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> > Tom, where are we on this.  As I remember, it is still an open issue,
> > right?  I can add it to the TODO list.
> 
> I have not done anything about it yet; it ought to be in TODO.
> 
> I'm also aware of two or three other sources of small but permanent
> memory leaks, btw; have them in my todo list.
> 
> 			regards, tom lane
> 
> >> I have been looking into why a reference to a nonexistent table, eg
> >> INSERT INTO nosuchtable VALUES(1);
> >> leaks a small amount of memory per occurrence.  What I find is a
> >> memory leak in the indexscan support.  Specifically,
> >> RelationGetIndexScan in backend/access/index/genam.c palloc's both
> >> an IndexScanDesc and some keydata storage.  The IndexScanDesc
> >> block is eventually pfree'd, at the bottom of CatalogIndexFetchTuple
> >> in backend/catalog/indexing.c.  But the keydata block is not.
> >> 
> >> This wouldn't matter so much if the palloc were coming from a
> >> transaction-local context.  But what we're doing is a lookup in pg_class
> >> on behalf of RelationBuildDesc in backend/utils/cache/relcache.c, and
> >> it's done a MemoryContextSwitchTo into the global CacheCxt before
> >> starting the lookup.  Therefore, the un-pfreed block represents a
> >> permanent memory leak.
> >> 
> >> In fact, *every* reference to a relation that is not already present in
> >> the relcache causes a similar leak.  The error case is just the one that
> >> is easiest to repeat.  The missing pfree of the keydata block is
> >> probably causing a bunch of other short-term and long-term leaks too.
> >> 
> >> It seems to me there are two things to fix here: indexscan ought to
> >> pfree everything it pallocs, and RelationBuildDesc ought to be warier
> >> about how much work gets done with CacheCxt as the active palloc
> >> context.  (Even if indexscan didn't leak anything ordinarily, there's
> >> still the risk of elog(ERROR) causing an abort before the indexscan code
> >> gets to clean up.)
> >> 
> >> Comments?  In particular, where is the cleanest place to add the pfree
> >> of the keydata block?  I don't especially like the fact that callers
> >> of index_endscan have to clean up the toplevel scan block; I think that
> >> ought to happen inside index_endscan.
> >> 
> >> regards, tom lane
> >> 
> >> 
> 
> 
> > -- 
> >   Bruce Momjian                        |  http://www.op.net/~candle
> >   maillist@candle.pha.pa.us            |  (610) 853-3000
> >   +  If your life is a hard drive,     |  830 Blythe Avenue
> >   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
> 


-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@candle.pha.pa.us            |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Wed, 7 Jul 1999 23:27:26 -0400 (EDT)
Bruce Momjian <maillist@candle.pha.pa.us>
Re: [HACKERS] Memory leaks in relcache