Re: Protect syscache from bloating with negative cache entries

On 27/01/2021 03:13, Kyotaro Horiguchi wrote:
> At Thu, 14 Jan 2021 17:32:27 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> The commit 4656e3d668 (debug_invalidate_system_caches_always)
>> conflicted with this patch. Rebased.
> 
> At Wed, 27 Jan 2021 10:07:47 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> (I found a bug in a benchmark-aid function
>> (CatalogCacheFlushCatalog2), I repost an updated version soon.)
> 
> I noticed that a catcachebench-aid function
> CatalogCacheFlushCatalog2() allocates bucked array wrongly in the
> current memory context, which leads to a crash.
> 
> This is a fixed it then rebased version.

Thanks, with the scripts you provided, I was able to run the performance 
tests on my laptop, and got very similar results as you did.

The impact of v7-0002-Remove-dead-flag-from-catcache-tuple.patch is very 
small. I think I could see it in the tests, but only barely. And the 
tests did nothing else than do syscache lookups; in any real world 
scenario, it would be lost in noise. I think we can put that aside for 
now, and focus on v6-0001-CatCache-expiration-feature.patch:

The pruning is still pretty lethargic:

- Entries created in the same transactions are never pruned away

- The size of the hash table is never shrunk. So even though the patch 
puts a backstop to the hash table growing indefinitely, if you run one 
transaction that bloats the cache, it's bloated for the rest of the session.

I think that's OK. We might want to be more aggressive in the future, 
but for now it seems reasonable to lean towards the current behavior 
where nothing is pruned. Although I wonder if we should try to set 
'catcacheclock' more aggressively. I think we could set it whenever 
GetCurrentTimestamp() is called, for example.

Given how unaggressive this mechanism is, I think it should be safe to 
enable it by default. What would be a suitable default for 
catalog_cache_prune_min_age? 30 seconds?

Documentation needs to be updated for the new GUC.

Attached is a version with a few little cleanups:
- use TimestampTz instead of uint64 for the timestamps
- remove assign_catalog_cache_prune_min_age(). All it did was convert 
the GUC's value from seconds to microseconds, and stored it in a 
separate variable. Multiplication is cheap, so we can just do it when we 
use the GUC's value instead.

- Heikki