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

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

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

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

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