Re: Protect syscache from bloating with negative cache entries

On Thu, Apr 4, 2019 at 8:53 AM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> So it seems to me that the simplest "Full" version wins. The
> attached is rebsaed version. dlist_move_head(entry) is removed as
> mentioned above in that patch.

1. I really don't think this patch has any business changing the
existing logic.  You can't just assume that the dlist_move_head()
operation is unimportant for performance.

2. This patch still seems to add a new LRU list that has to be
maintained.  That's fairly puzzling.  You seem to have concluded that
the version without the additional LRU wins, but the sent a new copy
of the version with the LRU version.

3. I don't think adding an additional call to GetCurrentTimestamp() in
start_xact_command() is likely to be acceptable.  There has got to be
a way to set this up so that the maximum number of new
GetCurrentTimestamp() is limited to once per N seconds, vs. the
current implementation that could do it many many many times per
second.

4. The code in CatalogCacheCreateEntry seems clearly unacceptable.  In
a pathological case where CatCacheCleanupOldEntries removes exactly
one element per cycle, it could be called on every new catcache
allocation.

I think we need to punt this patch to next release.  We're not
converging on anything committable very fast.

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