RE: Protect syscache from bloating with negative cache entries

From: "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>
To: 'Kyotaro HORIGUCHI' <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: "alvherre(at)2ndquadrant(dot)com" <alvherre(at)2ndquadrant(dot)com>, "tomas(dot)vondra(at)2ndquadrant(dot)com" <tomas(dot)vondra(at)2ndquadrant(dot)com>, "bruce(at)momjian(dot)us" <bruce(at)momjian(dot)us>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>, "robertmhaas(at)gmail(dot)com" <robertmhaas(at)gmail(dot)com>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Ideriha, Takeshi" <ideriha(dot)takeshi(at)jp(dot)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "michael(dot)paquier(at)gmail(dot)com" <michael(dot)paquier(at)gmail(dot)com>, "david(at)pgmasters(dot)net" <david(at)pgmasters(dot)net>, "craig(at)2ndquadrant(dot)com" <craig(at)2ndquadrant(dot)com>
Subject: RE: Protect syscache from bloating with negative cache entries
Date: 2019-02-12 01:02:39
Message-ID: 0A3221C70F24FB45833433255569204D1FB972A6@G01JPEXMBYT05
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

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

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

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

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

(5)
+ if (entry_age > cache_prune_min_age)

">=" instead of ">"?

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

(7)
CatalogCacheCreateEntry

+ int tupsize = 0;
if (ntp)
{
int i;
+ int tupsize;

tupsize is defined twice.

(8)
CatalogCacheCreateEntry

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

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

Regards
Takayuki Tsunakawa

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robbie Harwood 2019-02-12 01:16:00 Re: [PATCH v20] GSSAPI encryption support
Previous Message Chapman Flack 2019-02-12 00:59:12 Re: proposal: variadic argument support for least, greatest function