Re: Protect syscache from bloating with negative cache entries

On Wed, Mar 6, 2019 at 6:18 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
> I agree clock sweep might be sufficient, although the benchmarks done in
> this thread so far do not suggest the LRU approach is very expensive.

I'm not sure how thoroughly it's been tested -- has someone
constructed a benchmark that does a lot of syscache lookups and
measured how much slower they get with this new code?

> A simple true/false flag, as proposed by Robert, would mean we can only
> do the cleanup once per the catalog_cache_prune_min_age interval, so
> with the default value (5 minutes) the entries might be between 5 and 10
> minutes old. That's probably acceptable, although for higher values the
> range gets wider and wider ...

That's true, but I don't know that it matters.  I'm not sure there's
much of a use case for raising this parameter to some larger value,
but even if there is, is it really worth complicating the mechanism to
make sure that we throw away entries in a more timely fashion?  That's
not going to be cost-free, either in terms of CPU cycles or in terms
of code complexity.

Again, I think our goal should be to add the least mechanism here that
solves the problem.  If we can show that a true/false flag makes poor
decisions about which entries to evict and a smarter algorithm does
better, then it's worth considering.  However, my bet is that it makes
no meaningful difference.

> Which part of the LRU approach is supposedly expensive? Updating the
> lastaccess field or moving the entries to tail? I'd guess it's the
> latter, so perhaps we can keep some sort of age field, update it less
> frequently (once per minute?), and do the clock sweep?

Move to tail (although lastaccess would be expensive if too if it
involves an extra gettimeofday() call).  GCLOCK, like we use for
shared_buffers, is a common approximation of LRU which tends to be a
lot less expensive to implement.  We could do that here and it might
work well, but I think the question, again, is whether we really need
it.  I think our goal here should just be to jettison cache entries
that are clearly worthless.  It's expensive enough to reload cache
entries that any kind of aggressive eviction policy is probably a
loser, and if our goal is just to get rid of the stuff that's clearly
not being used, we don't need to be super-accurate about it.

> BTW wasn't one of the cases this thread aimed to improve a session that
> accesses a lot of objects in a short period of time? That balloons the
> syscache, and while this patch evicts the entries from memory, we never
> actually release the memory back (because AllocSet just moves it into
> the freelists) and it's unlikely to get swapped out (because other
> chunks on those memory pages are likely to be still used). I've proposed
> to address that by recreating the context if it gets too bloated, and I
> think Alvaro agreed with that. But I haven't seen any further discussion
> about that.

That's an interesting point.  It seems reasonable to me to just throw
away everything and release all memory if the session has been idle
for a while, but if the session is busy doing stuff, discarding
everything in bulk like that is going to cause latency spikes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company