Обсуждение: Memory leaks in relcache
I have been looking into why a reference to a nonexistent table, egINSERT 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
> 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, Pennsylvania19026
> 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, Pennsylvania19026
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, Pennsylvania19026
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
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, Pennsylvania19026