Re: Protect syscache from bloating with negative cache entries

Thank you for the comment.

At Thu, 4 Apr 2019 15:44:35 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+TgmoZQx7pCcc=VO3WeDQNpco8h6MZN09KjcOMRRu_CrbeoSw(at)mail(dot)gmail(dot)com>
> On Thu, Apr 4, 2019 at 8:53 AM Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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.

Ok, it doesn't show significant performance gain so removed that.

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

Sorry, I attached wrong one. The attached is the right one, which
doesn't adds the new dlist.

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

The GetCurrentTimestamp() is called only once at very early in
the backend's life in InitPostgres. Not in
start_xact_command. What I did in the function is just copying
stmtStartTimstamp, not GetCurrentTimestamp().

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

It may be a problem, if just one entry was created in the
duration longer than by catalog_cache_prune_min_age and resize
interval, or all candidate entries except one are actually in use
at the pruning moment. Is it realistic?

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

Yeah, maybe right. This patch had several month silence several
times, got comments and modified taking in the comments for more
than two cycles, and finally had a death sentence (not literaly,
actually postpone) at very close to this third cycle end. I
anticipate the same continues in the next cycle.

By the way, I found the reason of the wrong result of the
previous benchmark. The test 3_0/1 needs to update catcacheclock
midst of the loop. I'm going to fix it and rerun it.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center