Fix two ancient memory-leak bugs in relcache.c.

RelationCacheInsert() ignored the possibility that hash_search(HASH_ENTER)
might find a hashtable entry already present for the same OID.  However,
that can in fact occur during recursive relcache load scenarios.  When it
did happen, we overwrote the pointer to the pre-existing Relation, causing
a session-lifespan leakage of that entire structure.  As far as is known,
the pre-existing Relation would always have reference count zero by the
time we arrive back at the outer insertion, so add code that deletes the
pre-existing Relation if so.  If by some chance its refcount is positive,
elog a WARNING and allow the pre-existing Relation to be leaked as before.

Also, AttrDefaultFetch() was sloppy about leaking the cstring form of the
pg_attrdef.adbin value it's copying into the relcache structure.  This is
only a query-lifespan leakage, and normally not very significant, but it
adds up during CLOBBER_CACHE testing.

These bugs are of very ancient vintage, but I'll refrain from back-patching
since there's no evidence that these leaks amount to anything in ordinary
usage.
This commit is contained in:
Tom Lane 2014-05-18 16:51:46 -04:00
parent 44cd47c1d4
commit 078b2ed291
1 changed files with 48 additions and 21 deletions

View File

@ -171,24 +171,36 @@ static int NextEOXactTupleDescNum = 0;
static int EOXactTupleDescArrayLen = 0; static int EOXactTupleDescArrayLen = 0;
/* /*
* macros to manipulate the lookup hashtables * macros to manipulate the lookup hashtable
*/ */
#define RelationCacheInsert(RELATION) \ #define RelationCacheInsert(RELATION, replace_allowed) \
do { \ do { \
RelIdCacheEnt *idhentry; bool found; \ RelIdCacheEnt *hentry; bool found; \
idhentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \ hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
(void *) &(RELATION->rd_id), \ (void *) &((RELATION)->rd_id), \
HASH_ENTER, &found); \ HASH_ENTER, &found); \
/* used to give notice if found -- now just keep quiet */ \ if (found) \
idhentry->reldesc = RELATION; \ { \
/* this can happen, see comments in RelationBuildDesc */ \
Relation _old_rel = hentry->reldesc; \
Assert(replace_allowed); \
hentry->reldesc = (RELATION); \
if (RelationHasReferenceCountZero(_old_rel)) \
RelationDestroyRelation(_old_rel, false); \
else \
elog(WARNING, "leaking still-referenced relcache entry for \"%s\"", \
RelationGetRelationName(_old_rel)); \
} \
else \
hentry->reldesc = (RELATION); \
} while(0) } while(0)
#define RelationIdCacheLookup(ID, RELATION) \ #define RelationIdCacheLookup(ID, RELATION) \
do { \ do { \
RelIdCacheEnt *hentry; \ RelIdCacheEnt *hentry; \
hentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \ hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
(void *) &(ID), \ (void *) &(ID), \
HASH_FIND, NULL); \ HASH_FIND, NULL); \
if (hentry) \ if (hentry) \
RELATION = hentry->reldesc; \ RELATION = hentry->reldesc; \
else \ else \
@ -197,12 +209,13 @@ do { \
#define RelationCacheDelete(RELATION) \ #define RelationCacheDelete(RELATION) \
do { \ do { \
RelIdCacheEnt *idhentry; \ RelIdCacheEnt *hentry; \
idhentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \ hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
(void *) &(RELATION->rd_id), \ (void *) &((RELATION)->rd_id), \
HASH_REMOVE, NULL); \ HASH_REMOVE, NULL); \
if (idhentry == NULL) \ if (hentry == NULL) \
elog(WARNING, "trying to delete a rd_id reldesc that does not exist"); \ elog(WARNING, "failed to delete relcache entry for OID %u", \
(RELATION)->rd_id); \
} while(0) } while(0)
@ -982,9 +995,18 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
/* /*
* Insert newly created relation into relcache hash table, if requested. * Insert newly created relation into relcache hash table, if requested.
*
* There is one scenario in which we might find a hashtable entry already
* present, even though our caller failed to find it: if the relation is a
* system catalog or index that's used during relcache load, we might have
* recursively created the same relcache entry during the preceding steps.
* So allow RelationCacheInsert to delete any already-present relcache
* entry for the same OID. The already-present entry should have refcount
* zero (else somebody forgot to close it); in the event that it doesn't,
* we'll elog a WARNING and leak the already-present entry.
*/ */
if (insertIt) if (insertIt)
RelationCacheInsert(relation); RelationCacheInsert(relation, true);
/* It's fully valid */ /* It's fully valid */
relation->rd_isvalid = true; relation->rd_isvalid = true;
@ -1599,7 +1621,7 @@ formrdesc(const char *relationName, Oid relationReltype,
/* /*
* add new reldesc to relcache * add new reldesc to relcache
*/ */
RelationCacheInsert(relation); RelationCacheInsert(relation, false);
/* It's fully valid */ /* It's fully valid */
relation->rd_isvalid = true; relation->rd_isvalid = true;
@ -2841,7 +2863,7 @@ RelationBuildLocalRelation(const char *relname,
/* /*
* Okay to insert into the relcache hash tables. * Okay to insert into the relcache hash tables.
*/ */
RelationCacheInsert(rel); RelationCacheInsert(rel, false);
/* /*
* Flag relation as needing eoxact cleanup (to clear rd_createSubid). We * Flag relation as needing eoxact cleanup (to clear rd_createSubid). We
@ -3489,8 +3511,13 @@ AttrDefaultFetch(Relation relation)
NameStr(relation->rd_att->attrs[adform->adnum - 1]->attname), NameStr(relation->rd_att->attrs[adform->adnum - 1]->attname),
RelationGetRelationName(relation)); RelationGetRelationName(relation));
else else
attrdef[i].adbin = MemoryContextStrdup(CacheMemoryContext, {
TextDatumGetCString(val)); /* detoast and convert to cstring in caller's context */
char *s = TextDatumGetCString(val);
attrdef[i].adbin = MemoryContextStrdup(CacheMemoryContext, s);
pfree(s);
}
break; break;
} }
@ -4717,7 +4744,7 @@ load_relcache_init_file(bool shared)
*/ */
for (relno = 0; relno < num_rels; relno++) for (relno = 0; relno < num_rels; relno++)
{ {
RelationCacheInsert(rels[relno]); RelationCacheInsert(rels[relno], false);
/* also make a list of their OIDs, for RelationIdIsInInitFile */ /* also make a list of their OIDs, for RelationIdIsInInitFile */
if (!shared) if (!shared)
initFileRelationIds = lcons_oid(RelationGetRelid(rels[relno]), initFileRelationIds = lcons_oid(RelationGetRelid(rels[relno]),