Re: Protect syscache from bloating with negative cache entries - Mailing list pgsql-hackers

On 3/7/19 3:34 PM, Robert Haas wrote:
> 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?
> 

What I've done on v13 (and I don't think the results would be that
different on the current patch, but I may rerun it if needed) is a test
that creates large number of tables (up to 1M) and then accesses them
randomly. I don't know if it matches what you imagine, but see [1]

https://www.postgresql.org/message-id/74386116-0bc5-84f2-e614-0cff19aca2de%402ndquadrant.com

I don't think this shows any regression, but perhaps we should do a
microbenchmark isolating the syscache entirely?

>> 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.
> 

True, although it very much depends on how expensive it would be.

> 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.
> 

True.

>> 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.
> 

True.

>> 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.
> 

What I had in mind is more along these lines:

(a) track number of active syscache entries (increment when adding a new
one, decrement when evicting one)

(b) track peak number of active syscache entries

(c) after clock-sweep, if (peak > K*active) where K=2 or K=4 or so, do a
memory context swap, i.e. create a new context, copy active entries over
and destroy the old one

That would at least free() the memory. Of course, the syscache entries
may have different sizes, so tracking just numbers of entries is just an
approximation. But I think it'd be enough.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services