On 7/4/21 3:57 PM, Tom Lane wrote: > Over in [1] it is demonstrated that with CLOBBER_CACHE_ALWAYS enabled, > initdb accounts for a full 50% of the runtime of "make check-world" > (well, actually of the buildfarm cycle, which is not quite exactly > that but close). Since initdb certainly doesn't cost that much > normally, I wondered why it is so negatively affected by CCA. Some > perf measuring led me to LookupOpclassInfo, and specifically this bit: > > /* > * When testing for cache-flush hazards, we intentionally disable the > * operator class cache and force reloading of the info on each call. This > * is helpful because we want to test the case where a cache flush occurs > * while we are loading the info, and it's very hard to provoke that if > * this happens only once per opclass per backend. > */ > #ifdef CLOBBER_CACHE_ENABLED > if (debug_invalidate_system_caches_always > 0) > opcentry->valid = false; > #endif > > Diking that out halves initdb's CCA runtime. Turns out it also > roughly halves the runtime of the core regression tests under CCA, > so this doesn't explain why initdb seems so disproportionately > affected by CCA. > > However, seeing that this single choice is accounting for half the > cost of CCA testing, we really have to ask whether it's worth that. > This code was added by my commit 03ffc4d6d of 2007-11-28, about which > I wrote: > > Improve test coverage of CLOBBER_CACHE_ALWAYS by having it also force > reloading of operator class information on each use of LookupOpclassInfo. > Had this been in place a year ago, it would have helped me find a bug > in the then-new 'operator family' code. Now that we have a build farm > member testing CLOBBER_CACHE_ALWAYS on a regular basis, it seems worth > expending a little bit of effort here. > > I'm now a little dubious about my claim that this would have helped find > any bugs. Invalidating a finished OpClassCache entry does not model any > real-world scenario, because as noted elsewhere in LookupOpclassInfo, > once such a cache entry is populated it is kept for the rest of the > session. Also, the claim in the comment that we need this to test > a cache flush during load seems like nonsense: if we have > debug_invalidate_system_caches_always turned on, then we'll test > the effects of such flushes throughout the initial population of > a cache entry. Doing it over and over again adds nothing. > > So I'm now fairly strongly tempted to remove this code outright > (effectively reverting 03ffc4d6d). Another possibility now that > we have debug_invalidate_system_caches_always is to increase the > threshold at which this happens, making it more like > CLOBBER_CACHE_RECURSIVE. > > Thoughts? > >
If we don't think it's adding anything useful just rip it out. We don't generally keep code hanging around just on the off chance it might be useful some day. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
