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.

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 0d3ef20c0666b9f36dbc2b8c1c833cf7165b2f47..e6fc6a005895c3e9b02bc630621030a599c3cadf 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -171,24 +171,36 @@ static int        NextEOXactTupleDescNum = 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 { \
-       RelIdCacheEnt *idhentry; bool found; \
-       idhentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \
-                                                                                  (void *) &(RELATION->rd_id), \
+       RelIdCacheEnt *hentry; bool found; \
+       hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
+                                                                                  (void *) &((RELATION)->rd_id), \
                                                                                   HASH_ENTER, &found); \
-       /* used to give notice if found -- now just keep quiet */ \
-       idhentry->reldesc = RELATION; \
+       if (found) \
+       { \
+               /* 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)
 
 #define RelationIdCacheLookup(ID, RELATION) \
 do { \
        RelIdCacheEnt *hentry; \
-       hentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \
-                                                                                (void *) &(ID), \
-                                                                                HASH_FIND, NULL); \
+       hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
+                                                                                  (void *) &(ID), \
+                                                                                  HASH_FIND, NULL); \
        if (hentry) \
                RELATION = hentry->reldesc; \
        else \
@@ -197,12 +209,13 @@ do { \
 
 #define RelationCacheDelete(RELATION) \
 do { \
-       RelIdCacheEnt *idhentry; \
-       idhentry = (RelIdCacheEnt*)hash_search(RelationIdCache, \
-                                                                                  (void *) &(RELATION->rd_id), \
+       RelIdCacheEnt *hentry; \
+       hentry = (RelIdCacheEnt *) hash_search(RelationIdCache, \
+                                                                                  (void *) &((RELATION)->rd_id), \
                                                                                   HASH_REMOVE, NULL); \
-       if (idhentry == NULL) \
-               elog(WARNING, "trying to delete a rd_id reldesc that does not exist"); \
+       if (hentry == NULL) \
+               elog(WARNING, "failed to delete relcache entry for OID %u", \
+                        (RELATION)->rd_id); \
 } while(0)
 
 
@@ -982,9 +995,18 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
 
        /*
         * 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)
-               RelationCacheInsert(relation);
+               RelationCacheInsert(relation, true);
 
        /* It's fully valid */
        relation->rd_isvalid = true;
@@ -1599,7 +1621,7 @@ formrdesc(const char *relationName, Oid relationReltype,
        /*
         * add new reldesc to relcache
         */
-       RelationCacheInsert(relation);
+       RelationCacheInsert(relation, false);
 
        /* It's fully valid */
        relation->rd_isvalid = true;
@@ -2841,7 +2863,7 @@ RelationBuildLocalRelation(const char *relname,
        /*
         * Okay to insert into the relcache hash tables.
         */
-       RelationCacheInsert(rel);
+       RelationCacheInsert(rel, false);
 
        /*
         * 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),
                                         RelationGetRelationName(relation));
                        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;
                }
 
@@ -4717,7 +4744,7 @@ load_relcache_init_file(bool shared)
         */
        for (relno = 0; relno < num_rels; relno++)
        {
-               RelationCacheInsert(rels[relno]);
+               RelationCacheInsert(rels[relno], false);
                /* also make a list of their OIDs, for RelationIdIsInInitFile */
                if (!shared)
                        initFileRelationIds = lcons_oid(RelationGetRelid(rels[relno]),