I wrote: > "Tomas Vondra" <t...@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
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers