Re: Protect syscache from bloating with negative cache entries

At Tue, 12 Feb 2019 01:02:39 +0000, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote in <0A3221C70F24FB45833433255569204D1FB972A6(at)G01JPEXMBYT05>
> From: Kyotaro HORIGUCHI [mailto:horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp]
> > Recuded frequency of dlist_move_tail by taking 1ms interval between two
> > succesive updates on the same entry let the degradation dissapear.
> >
> > patched : 13720 tps (+2%)
>
> What do you think contributed to this performance increase? Or do you hink this is just a measurement variation?
>
> Most of my previous comments also seem to apply to v13, so let me repost them below:
>
>
> (1)
>
> (1)
> +/* GUC variable to define the minimum age of entries that will be cosidered to
> + /* initilize catcache reference clock if haven't done yet */
>
> cosidered -> considered
> initilize -> initialize

Fixed. I found "databsae", "temprary", "resturns",
"If'force'"(missing space), "aginst", "maintan". And all fixed.

> I remember I saw some other wrong spelling and/or missing words, which I forgot (sorry).

Thank you for pointing some of them.

> (2)
> Only the doc prefixes "sys" to the new parameter names. Other places don't have it. I think we should prefix sys, because relcache and plancache should be configurable separately because of their different usage patterns/lifecycle.

I tend to agree. They are already removed in this patchset. The
names are changed to "catalog_cache_*" in the new version.

> (3)
> The doc doesn't describe the unit of syscache_memory_target. Kilobytes?

Added.

> (4)
> + hash_size = cp->cc_nbuckets * sizeof(dlist_head);
> + tupsize = sizeof(CatCTup) + MAXIMUM_ALIGNOF + dtp->t_len;
> + tupsize = sizeof(CatCTup);
>
> GetMemoryChunkSpace() should be used to include the memory context overhead. That's what the files in src/backend/utils/sort/ do.

Thanks. Done. Include bucket and cache header part but still
excluding clist. Renamed from tupsize to memusage.

> (5)
> + if (entry_age > cache_prune_min_age)
>
> ">=" instead of ">"?

I didn't get it serious, but it is better. Fixed.

> (6)
> + if (!ct->c_list || ct->c_list->refcount == 0)
> + {
> + CatCacheRemoveCTup(cp, ct);
>
> It's better to write "ct->c_list == NULL" to follow the style in this file.
>
> "ct->refcount == 0" should also be checked prior to removing the catcache tuple, just in case the tuple hasn't been released for a long time, which might hardly happen.

Yeah, I fixed it in v12. This no longer removes an entry in
use. (if (c_list) is used in the file.)

> (7)
> CatalogCacheCreateEntry
>
> + int tupsize = 0;
> if (ntp)
> {
> int i;
> + int tupsize;
>
> tupsize is defined twice.

The second tupsize was bogus, but the first is removed in this
version. Now memory usage of an entry is calculated as a chunk
size.

> (8)
> CatalogCacheCreateEntry
>
> In the negative entry case, the memory allocated by CatCacheCopyKeys() is not counted. I'm afraid that's not negligible.

Right. Fixed.

> (9)
> The memory for CatCList is not taken into account for syscache_memory_target.

Yeah, this is intensional since CatCacheList is short lived. Comment added.

| * Don't waste a time by counting the list in catcache memory usage,
| * since a list doesn't persist for a long time
| */
| cl = (CatCList *)
| palloc(offsetof(CatCList, members) + nmembers * sizeof(CatCTup *));

Please fine the attached, which is the new version v14 addressing
Tomas', Ideriha-san and your comments.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center