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