Re: Excessive cost of OpClassCache flushes in CLOBBER_CACHE_ALWAYS mode - Mailing list pgsql-hackers

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