Subject: Re: Excessive cost of OpClassCache flushes in CLOBBER_CACHE_ALWAYS
 mode
To: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers@lists.postgresql.org
References: <1370856.1625428625@sss.pgh.pa.us>
From: Andrew Dunstan <andrew@dunslane.net>
Message-ID: <8a392a8d-d520-d8d0-5e11-ff421a4c8e23@dunslane.net>
Date: Mon, 5 Jul 2021 07:32:08 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <1370856.1625428625@sss.pgh.pa.us>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/8a392a8d-d520-d8d0-5e11-ff421a4c8e23%40dunslane.net>
Precedence: bulk


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