Skip to content

Commit 3edc2db

Browse files
committed
Reduce overhead of cache-clobber testing in LookupOpclassInfo().
Commit 03ffc4d added logic to bypass all caching behavior in LookupOpclassInfo when CLOBBER_CACHE_ALWAYS is enabled. It doesn't look like I stopped to think much about what that would cost, but recent investigation shows that the cost is enormous: it roughly doubles the time needed for cache-clobber test runs. There does seem to be value in this behavior when trying to test the opclass-cache loading logic itself, but for other purposes the cost is excessive. Hence, let's back off to doing this only when debug_invalidate_system_caches_always is at least 3; or in older branches, when CLOBBER_CACHE_RECURSIVELY is defined. While here, clean up some other minor issues in LookupOpclassInfo. Re-order the code so we aren't left with broken cache entries (leading to later core dumps) in the unlikely case that we suffer OOM while trying to allocate space for a new entry. (That seems to be my oversight in 03ffc4d.) Also, in >= v13, stop allocating one array entry too many. That's evidently left over from sloppy reversion in 851b14b. Back-patch to all supported branches, mainly to reduce the runtime of cache-clobbering buildfarm animals. Discussion: https://postgr.es/m/[email protected]
1 parent 73ede4d commit 3edc2db

File tree

1 file changed

+23
-20
lines changed

1 file changed

+23
-20
lines changed

src/backend/utils/cache/relcache.c

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1612,15 +1612,15 @@ LookupOpclassInfo(Oid operatorClassOid,
16121612
/* First time through: initialize the opclass cache */
16131613
HASHCTL ctl;
16141614

1615+
/* Also make sure CacheMemoryContext exists */
1616+
if (!CacheMemoryContext)
1617+
CreateCacheMemoryContext();
1618+
16151619
MemSet(&ctl, 0, sizeof(ctl));
16161620
ctl.keysize = sizeof(Oid);
16171621
ctl.entrysize = sizeof(OpClassCacheEnt);
16181622
OpClassCache = hash_create("Operator class cache", 64,
16191623
&ctl, HASH_ELEM | HASH_BLOBS);
1620-
1621-
/* Also make sure CacheMemoryContext exists */
1622-
if (!CacheMemoryContext)
1623-
CreateCacheMemoryContext();
16241624
}
16251625

16261626
opcentry = (OpClassCacheEnt *) hash_search(OpClassCache,
@@ -1629,39 +1629,42 @@ LookupOpclassInfo(Oid operatorClassOid,
16291629

16301630
if (!found)
16311631
{
1632-
/* Need to allocate memory for new entry */
1632+
/* Initialize new entry */
16331633
opcentry->valid = false; /* until known OK */
16341634
opcentry->numSupport = numSupport;
1635-
1636-
if (numSupport > 0)
1637-
opcentry->supportProcs = (RegProcedure *)
1638-
MemoryContextAllocZero(CacheMemoryContext,
1639-
numSupport * sizeof(RegProcedure));
1640-
else
1641-
opcentry->supportProcs = NULL;
1635+
opcentry->supportProcs = NULL; /* filled below */
16421636
}
16431637
else
16441638
{
16451639
Assert(numSupport == opcentry->numSupport);
16461640
}
16471641

16481642
/*
1649-
* When testing for cache-flush hazards, we intentionally disable the
1650-
* operator class cache and force reloading of the info on each call. This
1651-
* is helpful because we want to test the case where a cache flush occurs
1652-
* while we are loading the info, and it's very hard to provoke that if
1653-
* this happens only once per opclass per backend.
1643+
* When aggressively testing cache-flush hazards, we disable the operator
1644+
* class cache and force reloading of the info on each call. This models
1645+
* no real-world behavior, since the cache entries are never invalidated
1646+
* otherwise. However it can be helpful for detecting bugs in the cache
1647+
* loading logic itself, such as reliance on a non-nailed index. Given
1648+
* the limited use-case and the fact that this adds a great deal of
1649+
* expense, we enable it only in CLOBBER_CACHE_RECURSIVELY mode.
16541650
*/
1655-
#if defined(CLOBBER_CACHE_ALWAYS)
1651+
#if defined(CLOBBER_CACHE_RECURSIVELY)
16561652
opcentry->valid = false;
16571653
#endif
16581654

16591655
if (opcentry->valid)
16601656
return opcentry;
16611657

16621658
/*
1663-
* Need to fill in new entry.
1664-
*
1659+
* Need to fill in new entry. First allocate space, unless we already did
1660+
* so in some previous attempt.
1661+
*/
1662+
if (opcentry->supportProcs == NULL && numSupport > 0)
1663+
opcentry->supportProcs = (RegProcedure *)
1664+
MemoryContextAllocZero(CacheMemoryContext,
1665+
numSupport * sizeof(RegProcedure));
1666+
1667+
/*
16651668
* To avoid infinite recursion during startup, force heap scans if we're
16661669
* looking up info for the opclasses used by the indexes we would like to
16671670
* reference here.

0 commit comments

Comments
 (0)