Re: failures on barnacle (CLOBBER_CACHE_RECURSIVELY) because of memory leaks - Mailing list pgsql-hackers

I wrote:
> "Tomas Vondra" <tv@fuzzy.cz> writes:
>> So after 83 days, the regression tests on barnacle completed,

> Hah, that's perseverance!

>> and it
>> smells like another memory leak in CacheMemoryContext, similar to those
>> fixed in 078b2ed on May 18.

> Ugh, will look.

I've been experimenting by running the create_view test in isolation
(it's not quite self-contained, but close enough) and I find that there
are two memory leaks exposed here:

1. The relcache.c functions that provide on-demand caching, namely
RelationGetIndexList and RelationGetIndexAttrBitmap, are not careful
to free the old values (if any) of the relcache fields they fill.
I think we believed that the old values would surely always be null ...
but that's not necessarily the case when doing a recursive cache reload
of a system catalog, because we might've used/filled the fields while
fetching the data needed to fill them.  This results in a session-lifespan
leak of data in CacheMemoryContext, which is what Tomas saw.  (I'm not
real sure that this is a live issue for RelationGetIndexAttrBitmap, but
it demonstrably is for RelationGetIndexList.)

2. reloptions.c's parseRelOptions() leaks memory when disassembling a
reloptions array.  The leak is in the caller's memory context which
is typically query-lifespan, so under normal circumstances this is not
significant.  However, a CLOBBER_CACHE_RECURSIVELY build can call this
an awful lot of times within a single query.  The queries in create_view
that operate on views with security_barrier reloptions manage to eat
quite a lot of memory this way.

The attached patch fixes both of these things.  I'm inclined to think
it is not worth back-patching because neither effect could amount to
anything noticeable without CLOBBER_CACHE_RECURSIVELY.  Anyone think
otherwise?

            regards, tom lane

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c7ad6f9..e5f0240 100644
*** a/src/backend/access/common/reloptions.c
--- b/src/backend/access/common/reloptions.c
*************** parseRelOptions(Datum options, bool vali
*** 912,925 ****
      /* Done if no options */
      if (PointerIsValid(DatumGetPointer(options)))
      {
!         ArrayType  *array;
          Datum       *optiondatums;
          int            noptions;

-         array = DatumGetArrayTypeP(options);
-
-         Assert(ARR_ELEMTYPE(array) == TEXTOID);
-
          deconstruct_array(array, TEXTOID, -1, false, 'i',
                            &optiondatums, NULL, &noptions);

--- 912,921 ----
      /* Done if no options */
      if (PointerIsValid(DatumGetPointer(options)))
      {
!         ArrayType  *array = DatumGetArrayTypeP(options);
          Datum       *optiondatums;
          int            noptions;

          deconstruct_array(array, TEXTOID, -1, false, 'i',
                            &optiondatums, NULL, &noptions);

*************** parseRelOptions(Datum options, bool vali
*** 959,964 ****
--- 955,965 ----
                           errmsg("unrecognized parameter \"%s\"", s)));
              }
          }
+
+         /* It's worth avoiding memory leaks in this function */
+         pfree(optiondatums);
+         if (((void *) array) != DatumGetPointer(options))
+             pfree(array);
      }

      *numrelopts = numoptions;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 10d300a..fd84853 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
*************** RelationGetIndexList(Relation relation)
*** 3646,3651 ****
--- 3646,3652 ----
      ScanKeyData skey;
      HeapTuple    htup;
      List       *result;
+     List       *oldlist;
      char        replident = relation->rd_rel->relreplident;
      Oid            oidIndex = InvalidOid;
      Oid            pkeyIndex = InvalidOid;
*************** RelationGetIndexList(Relation relation)
*** 3737,3742 ****
--- 3738,3744 ----

      /* Now save a copy of the completed list in the relcache entry. */
      oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+     oldlist = relation->rd_indexlist;
      relation->rd_indexlist = list_copy(result);
      relation->rd_oidindex = oidIndex;
      if (replident == REPLICA_IDENTITY_DEFAULT && OidIsValid(pkeyIndex))
*************** RelationGetIndexList(Relation relation)
*** 3748,3753 ****
--- 3750,3758 ----
      relation->rd_indexvalid = 1;
      MemoryContextSwitchTo(oldcxt);

+     /* Don't leak the old list, if there is one */
+     list_free(oldlist);
+
      return result;
  }

*************** RelationGetIndexAttrBitmap(Relation rela
*** 4141,4146 ****
--- 4146,4159 ----

      list_free(indexoidlist);

+     /* Don't leak any old values of these bitmaps */
+     bms_free(relation->rd_indexattr);
+     relation->rd_indexattr = NULL;
+     bms_free(relation->rd_keyattr);
+     relation->rd_keyattr = NULL;
+     bms_free(relation->rd_idattr);
+     relation->rd_idattr = NULL;
+
      /*
       * Now save copies of the bitmaps in the relcache entry.  We intentionally
       * set rd_indexattr last, because that's the one that signals validity of