Re: Protect syscache from bloating with negative cache entries - Mailing list pgsql-hackers

Hello.

At Fri, 18 Jan 2019 17:09:41 -0800, "andres@anarazel.de" <andres@anarazel.de> wrote in
<20190119010941.6ruftewah7t3k3yk@alap3.anarazel.de>
> Hi,
> 
> On 2019-01-18 19:57:03 -0500, Robert Haas wrote:
> > On Fri, Jan 18, 2019 at 4:23 PM andres@anarazel.de <andres@anarazel.de> wrote:
> > > My proposal for this was to attach a 'generation' to cache entries. Upon
> > > access cache entries are marked to be of the current
> > > generation. Whenever existing memory isn't sufficient for further cache
> > > entries and, on a less frequent schedule, triggered by a timer, the
> > > cache generation is increased and th new generation's "creation time" is
> > > measured.  Then generations that are older than a certain threshold are
> > > purged, and if there are any, the entries of the purged generation are
> > > removed from the caches using a sequential scan through the cache.
> > >
> > > This outline achieves:
> > > - no additional time measurements in hot code paths

It is caused at every transaction start time and stored in
TimestampTz in this patch. No additional time measurement exists
already but cache puruing won't happen if a transaction lives for
a long time. Time-driven generation value, maybe with 10s-1min
fixed interval, is a possible option.

> > > - no need for a sequential scan of the entire cache when no generations
> > >   are too old

This patch didn't precheck against the oldest generation, but it
can be easily calculated. (But doesn't base on the creation time
but on the last-access time.) (Attached applies over the
v7-0001-Remove-entries-..patch)

Using generation time, entries are purged even if it is recently
accessed. I think last-accessed time is more sutable for the
purpse. On the other hand using last-accessed time, the oldest
generation can be stale by later access.

> > > - both size and time limits can be implemented reasonably cheaply
> > > - overhead when feature disabled should be close to zero

Overhead when disabled is already nothing since scanning is
inhibited when cache_prune_min_age is a negative value.

> > Seems generally reasonable.  The "whenever existing memory isn't
> > sufficient for further cache entries" part I'm not sure about.
> > Couldn't that trigger very frequently and prevent necessary cache size
> > growth?
> 
> I'm thinking it'd just trigger a new generation, with it's associated
> "creation" time (which is cheap to acquire in comparison to creating a
> number of cache entries) . Depending on settings or just code policy we
> can decide up to which generation to prune the cache, using that
> creation time.  I'd imagine that we'd have some default cache-pruning
> time in the minutes, and for workloads where relevant one can make
> sizing configurations more aggressive - or something like that.

The current patch uses last-accesed time by non-gettimeofday()
method. The genreation is fixed up to 3 and infrequently-accessed
entries are removed sooner. Generation interval is determined by
cache_prune_min_age.

Although this doesn't put a hard cap on memory usage, it is
indirectly and softly limited by the cache_prune_min_age and
cache_memory_target, which determins how large a cache can grow
until pruning happens. They are per-cache basis.

If we prefer to set a budget on all the syschaches (or even
including other caches), it would be more complex.

regares.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 4a3b3094a0..8274704af7 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -859,6 +859,7 @@ InitCatCache(int id,
     for (i = 0; i < nkeys; ++i)
         cp->cc_keyno[i] = key[i];
     cp->cc_tupsize = 0;
+    cp->cc_noprune_until = 0;
 
     /*
      * new cache is initialized as far as we can go for now. print some
@@ -898,6 +899,7 @@ CatCacheCleanupOldEntries(CatCache *cp)
     int            i;
     int            nremoved = 0;
     size_t        hash_size;
+    TimestampTz oldest_lastaccess = 0;
 #ifdef CATCACHE_STATS
     /* These variables are only for debugging purpose */
     int            ntotal = 0;
@@ -918,6 +920,10 @@ CatCacheCleanupOldEntries(CatCache *cp)
     if (cache_prune_min_age < 0)
         return false;
 
+    /* Return immediately if apparently no entry to remove */
+    if (cp->cc_noprune_until == 0 || catcacheclock <= cp->cc_noprune_until)
+        return false;
+
     /*
      * Return without pruning if the size of the hash is below the target.
      */
@@ -939,6 +945,7 @@ CatCacheCleanupOldEntries(CatCache *cp)
             CatCTup    *ct = dlist_container(CatCTup, cache_elem, iter.cur);
             long entry_age;
             int us;
+            bool removed = false;
 
 
             /*
@@ -982,12 +989,24 @@ CatCacheCleanupOldEntries(CatCache *cp)
                     {
                         CatCacheRemoveCTup(cp, ct);
                         nremoved++;
+                        removed = true;
                     }
                 }
             }
+
+            /* Take the oldest lastaccess among survived entries */
+            if (!removed &&
+                (oldest_lastaccess == 0 || ct->lastaccess < oldest_lastaccess))
+                oldest_lastaccess = ct->lastaccess;
         }
     }
 
+    /* Calculate the next pruning time if any entry remains */
+    if (oldest_lastaccess > 0)
+        oldest_lastaccess += cache_prune_min_age * USECS_PER_SEC;
+
+    cp->cc_noprune_until = oldest_lastaccess;
+
 #ifdef CATCACHE_STATS
     StaticAssertStmt(SYSCACHE_STATS_NAGECLASSES == 6,
                      "number of syscache age class must be 6");
@@ -1423,6 +1442,11 @@ SearchCatCacheInternal(CatCache *cache,
             ct->naccess++;
         ct->lastaccess = catcacheclock;
 
+        /* the first entry determines the next pruning time */
+        if (cache_prune_min_age >= 0 && cache->cc_noprune_until == 0)
+            cache->cc_noprune_until =
+                ct->lastaccess + cache_prune_min_age * USECS_PER_SEC;
+
         /*
          * If it's a positive entry, bump its refcount and return it. If it's
          * negative, we can report failure to the caller.
diff --git a/src/include/utils/catcache.h b/src/include/utils/catcache.h
index 4d51975920..1750919399 100644
--- a/src/include/utils/catcache.h
+++ b/src/include/utils/catcache.h
@@ -63,7 +63,8 @@ typedef struct catcache
     ScanKeyData cc_skey[CATCACHE_MAXKEYS];    /* precomputed key info for heap
                                              * scans */
     int            cc_tupsize;        /* total amount of catcache tuples */
-
+    TimestampTz    cc_noprune_until; /* Skip pruning until this time has passed
+                                   * zero means no entry lives in this cache */
     /*
      * Statistics entries
      */