From e7a9020939c9f09eddf6cf2f5477f82a1efcbf09 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 14 May 2014 14:55:50 -0400 Subject: [PATCH] Code review for recent changes in relcache.c. rd_replidindex should be managed the same as rd_oidindex, and rd_keyattr and rd_idattr should be managed like rd_indexattr. Omissions in this area meant that the bitmapsets computed for rd_keyattr and rd_idattr would be leaked during any relcache flush, resulting in a slow but permanent leak in CacheMemoryContext. There was also a tiny probability of relcache entry corruption if we ran out of memory at just the wrong point in RelationGetIndexAttrBitmap. Otherwise, the fields were not zeroed where expected, which would not bother the code any AFAICS but could greatly confuse anyone examining the relcache entry while debugging. Also, create an API function RelationGetReplicaIndex rather than letting non-relcache code be intimate with the mechanisms underlying caching of that value (we won't even mention the memory leak there). Also, fix a relcache flush hazard identified by Andres Freund: RelationGetIndexAttrBitmap must not assume that rd_replidindex stays valid across index_open. The aspects of this involving rd_keyattr date back to 9.3, so back-patch those changes. --- src/backend/utils/cache/relcache.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 8058169d067..48c0b7b865b 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -1829,6 +1829,7 @@ RelationDestroyRelation(Relation relation) FreeTupleDesc(relation->rd_att); list_free(relation->rd_indexlist); bms_free(relation->rd_indexattr); + bms_free(relation->rd_keyattr); FreeTriggerDesc(relation->trigdesc); if (relation->rd_options) pfree(relation->rd_options); @@ -3594,7 +3595,8 @@ insert_ordered_oid(List *list, Oid datum) * correctly with respect to the full index set. It is up to the caller * to ensure that a correct rd_indexattr set has been cached before first * calling RelationSetIndexList; else a subsequent inquiry might cause a - * wrong rd_indexattr set to get computed and cached. + * wrong rd_indexattr set to get computed and cached. Likewise, we do not + * touch rd_keyattr. */ void RelationSetIndexList(Relation relation, List *indexIds, Oid oidIndex) @@ -3875,10 +3877,16 @@ RelationGetIndexAttrBitmap(Relation relation, bool keyAttrs) list_free(indexoidlist); - /* Now save a copy of the bitmap in the relcache entry. */ + /* + * 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 + * the values; if we run out of memory before making that copy, we won't + * leave the relcache entry looking like the other ones are valid but + * empty. + */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); - relation->rd_indexattr = bms_copy(indexattrs); relation->rd_keyattr = bms_copy(uindexattrs); + relation->rd_indexattr = bms_copy(indexattrs); MemoryContextSwitchTo(oldcxt); /* We return our original working copy for caller to play with */ @@ -4423,6 +4431,7 @@ load_relcache_init_file(bool shared) rel->rd_indexvalid = 0; rel->rd_indexlist = NIL; rel->rd_indexattr = NULL; + rel->rd_keyattr = NULL; rel->rd_oidindex = InvalidOid; rel->rd_createSubid = InvalidSubTransactionId; rel->rd_newRelfilenodeSubid = InvalidSubTransactionId; -- 2.30.2