Horiguchi-san, Bruce,
Thank you for telling me your ideas behind this feature. Frankly, I don't think I understood the proposed
specificationis OK, but I can't explain it well at this instant. So, let me discuss that in a subsequent mail.
Anyway, here are my review comments on 0001:
(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,
becauserelcache 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
releasedfor 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
