Database cache bins allow unlimited growth: cache DB tables of gigabytes!

I think making it per bin makes sense, as there might be other bins where this is useful.
Also having this as part of the cache configuration helps you to get an overview inside settings.php, in case needed.

I think if we do that, we should have also some pseudo-LRU:

If ($refresh_cache_time && $age_remaining < $refresh_cache_time) {
  // cache set again. (treat like cache miss, but do not rebuild only re-cache)
}

Proposed parameters are per bin:

- max_age: 86400
- max_age_jitter_percent: 5
- refresh_cache_time: 3600 // Do not let frequently accessed data expire, re-cache it instead.

< berdir> Fabianx-screen: that would be interesting, but then we need two max-ages.. since some items might have a "hard" max-age
< Fabianx-screen> berdir: Yes, but the cache backend can distinguish with a simple flag:
< Fabianx-screen> $data->is_default_max_age => TRUE / FALSE,

Either the render cache service or the database backend (per-bin) should support an upper limit for the max-age. E.g. a day or a week.

I don't think this actually fixes the problem.

We only check if a cache item is expired when retrieving that cache item. Which means that any render cache item that is accessed infrequently (or never again) also does not get removed.

The only way I think we can solve this is by evicting expired cache items on cron, i.e. implementing LRU anyway.

So berdir pointed out that we already do what I just described, in \Drupal\Core\Cache\DatabaseBackend::garbageCollection(), called by system_cron(). Ok :)

#5: system_cron() runs:

  foreach (Cache::getBins() as $cache_backend) {
    $cache_backend->garbageCollection();
  }

is that not sufficient?

Given that, +1 to the proposed solution in principle:

Either the render cache service or the database backend (per-bin) should support an upper limit for the max-age. E.g. a day or a week. It would probably be great to vary that slightly for every write, e.g. +/- 5% so not all cache entries expire at the same time.

… but I still have some concerns:

1. We want to prefer keeping render cache items of "bigger page components" cached, choosing to evict "smaller page components" from the cache instead. Because the smaller ones, when combined, result in the bigger ones being re-created in the cache anyway.
2. In the future we'll probably want to send the Age header. We're able to base that on the render cache items created timestamps, taking the maximum value of those. This change would mean the Age would never exceed a week, for no good reason other than sites with huge amounts of content getting into trouble.
3. I wonder if we can choose to not impose a maximum max-age, but instead specify a maximum number of cache items for a certain cache bin, or even the maximum bin size (see http://stackoverflow.com/questions/9620198/how-to-get-the-sizes-of-the-t...) . Given those restrictions, we'd then continue to do the same things as we do today in DatabaseBackend::garbageCollection(), but on top of that also delete the oldest cache items (by looking at the created timestamp) and delete, say, the 1% oldest cache items.

I don't think we can implement 1. I do think 2 is a useful thing to take into account, but it's not a solution. But 3 seems like a viable alternative solution?

I do think the already proposed resolution works, but then my second point is a concern. I wonder what you guys think of point 3 as an alternative solution? Just trying to make sure we end up with the best possible solution :)

Finally: what do other CMSes/web frameworks do for this?

At least typo3 just defines a default lifetime of an hour, see http://wiki.typo3.org/Caching_framework

#8:

I don't think it belongs in render cache at all.

It is a pure database backend cache implementation detail and should have no affect on other metadata that we store extra.

1. I think we should at least move cache.page to its own cache_page bin again.

2. For the age header we should not depend on internal data of the cache backend (that is an implementation detail and not part of the interface), but rather provide our own in #cache. (similar with downstream-ttl and ensuring proper max-age is working correctly).

3. I still think to automatically refresh still active cache items is the sanest solution to have pseudo-LRU in database backend - configurable via settings for only that backend. (so others that are likely LRU already can support it, but don't need to.)

That would also take care of #2 then as the about-to-expire cache item would be just refreshed (and retain its age), it is also in principle similar to how If-Modified-Since works - where the reverse proxy returns a 304 when the backend returned data that is still matching the cache and just returns its cached item.
( in this case the cache tags take care of that.)

And I don't think it is complex to implement.

It is one Settings call in __construct and 5 lines of code in get() and 5 lines of code in set plus test coverage.

---

Thinking about 1. again - keep smaller fragments for longer, keep larger fragments shorter.

That kind of policy is very opposite to LRU.

I am not sure, it is something we should think about, but it is out of scope here I think.

Thinking about 1. again - keep smaller fragments for longer, keep larger fragments shorter.

I said the opposite.

but it is out of scope here I think.

It is not, because it's related.

Everything I wrote in #8 was about this:

Just trying to make sure we end up with the best possible solution :)

i.e. making suring we take all things into account.

Okay, I actually would have thought the smaller fragments change less as they usually have less variation :p.

#11: The problem is 3) is not LRU and makes refreshing impossible, i.e. the oldest active caches are very likely the ones that have been created first on cold cache, but those are very likely front page and other things that are highly frequently used and that a LRU policy would not expire.

So we would do the exact opposite, while the goal is to expire things that have not been accessed since day X. (LRU)

But LRU is very costly, because you need a write for each cache_get().

The nice thing about the refresh-pseudo-LRU is that you only need a refresh when you are near the set expiration time.

e.g. for my own sites based on DB I would probably use:

expire=1 week (+/- 10% jitter)
working set=1 day

Everything that was accessed within 1 day would be kept permanently, which is the working set of the site.

So old GA crawled urls would expire, but my front page would not.

Edit:

This would also in general favor larger fragments (as they would get accessed more often) and expire the smaller fragments automatically.

e.g. if my front page does not change and all that is accessed is my front page, then all the block and entity caches would expire after one week.

If then my front page changes, obviously there is a larger cache miss, but that is the trade-off to make and would happen with any other LRU system. (interesting observation: LRU has problems with hierarchical cache items - need to some research if there is something in the literature on that).

However if there are uncached pages then obviously the most active blocks would still be refreshed (unless they depend on url / route obviously).

So I think the trade-off in general is okay.

---

Edit 2:

Putting a max number of items only works when there is a pressure to evict, e.g. that is making that again more complex.

Given our pseudo-LRU it would work like that, it would check the number of items configured and only expire those expired items until the threshold is reached again.

In this case however I would not restrict the working set, but rather only the number of expired items to keep.

So it is orthogonal to the approach and could help to keep some expired items around - however I don't think it is necessary.

In this aproach a user can define in settings.php a max age for the appropriate bin.

Ups forgot to change some stuff from earlier. here is the new one.

1) Moved loading of settings to DatabaseBackendFactory in constructor and now we are passing value for the bin if available.
Also changed to

if (isset($this->configuration['max_expire']) ...

2) Removed changes to this part. Calling now DatabaseBackend directly in tests and added 2 more tests like suggested

1. Updated explanation
2. Added explanation in default.settings.php. Not exactly sure for this kind of settings what information is needed but here is my first try.
Since i added the explanation in default.settings.php and I already added the settings with Settings::get('cache_database', []); i left it like that. If container parameters is better i can change...
3. .

Removing the "needs tests" tag, because it already has tests. I also changed the DatabaseBackend to make sure that anything passed in $configuration is an actual array.

Also changed all array definitions to short syntaxes.

Closed #1947852: Database cache backend garbage collection does not work with permanent entries as a duplicate.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -150,6 +160,10 @@ protected function prepareItem($cache, $allow_invalid) {
      public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array()) {
   +    // Check if maximum expiration settings for cache bin is defined
   +    if (isset($this->configuration['max_expire']) && ($expire == Cache::PERMANENT || $expire > $this->configuration['max_expire'])) {
   +      $expire = REQUEST_TIME + $this->configuration['max_expire'];
   +    }
   

   $expire is a timestamp and configuration['max_expire'] is a duration, so I think we need to add REQUEST_TIME to $expire > $this->configuration['max_expire']?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -36,6 +44,7 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
   +    $this->configuration = Settings::get('cache_database', []);
   
   @@ -48,7 +57,8 @@ function __construct(Connection $connection, CacheTagsChecksumInterface $checksu
   +    $configuration['max_expire'] = isset($this->configuration[$bin]) ? (int) $this->configuration[$bin] : null;
   +    return new DatabaseBackend($this->connection, $this->checksumProvider, $bin, $configuration);
   

   So I would add $settings['cache_database']['default'] = 86400; to settings.php? How about something like $settings['cache_database']['default']['max_expire'] = ...?

3. +++ b/core/modules/system/src/Tests/Cache/DatabaseBackendUnitTest.php
   @@ -53,4 +53,23 @@ public function testSetGet() {
   +    $backend->set('test_cache1', 'foo');
   ...
   +    $backend->set('test_cache1', 'foo');
   ...
   +    $backend->set('test_cache1', 'foo');
   

   One of these could use a $expire param greater than the 'max_expire' setting.

#24:

cache_render grew to 1.3GB within days

Should we add a default?

#27

1. I think the tests prove that the code works, so I don't think this is needed.
2. @Denchev had already added this, but it looks like I had trouble applying his patch to settings.php locally. I added that code back to settings.php.
   No interdiff sadly.
3. Added a new test that does exactly that.
4. see .2, a default is added.

@Ollie
1. You are right.
The tests have some flaw i will update them and will see for your other suggestions.

1) Fixed set method.
2) Default value and bin now in settings and factory class.
3) Fixed tests.

I have made changes as specified in #31 and some more nitpicks I found. I added a test as pointed out in 3 but I am not sure if that would work the same way as the earlier test. From what I read of the code, I am expecting this to fail.

Attempting a fix. General observation: The code and tests assume that the max_expire setting would be defined in settings.php or overridden by the factory. What if both fail to set it? e.g., if settings.php just defines some other (future) element in the 'render' array (but not max_expire), then the default won't be set which means max_expire won't be set at all. In that case, there would be no upper limit at all. Is this what is intended?

Adding another test to see the effect for the scenario I described in #36.

Fixing failures.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -150,6 +160,12 @@ protected function prepareItem($cache, $allow_invalid) {
   +      $expire = $this->configuration['max_expire'] + REQUEST_TIME;
   

   What do you think about renaming 'max_expire' to 'max_lifetime'?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -205,11 +221,21 @@ public function setMultiple(array $items) {
        foreach ($items as $cid => $item) {
   +      // Check if maximum expiration settings for cache bin is defined. If set,
   +      // enforce an upper limit for the expiration to allow for garbage
   +      // collection.
   +      $max_expire = isset($this->configuration['max_expire']) ? $this->configuration['max_expire'] + REQUEST_TIME : Cache::PERMANENT;
   

   $max_expire assignment can be moved outside the loop.

3. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -36,6 +44,9 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
   +    $this->configuration = Settings::get('cache_database', ['render' => ['max_expire' => 86400 * 7]]);
   
   +++ b/sites/default/default.settings.php
   @@ -691,3 +691,17 @@
   +$settings['cache_database']['render'] = [
   +  'max_expire' => 86400 * 7,
   +];
   

   Why do we need both? Isn't it enough to set it in default.settings.php?

#41.1 makes a lot of sense to me.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -150,6 +160,12 @@ protected function prepareItem($cache, $allow_invalid) {
+    if (isset($this->configuration['max_expire']) && ($expire == Cache::PERMANENT || $expire > $this->configuration['max_expire'] + REQUEST_TIME)) {

@@ -205,11 +221,21 @@ public function setMultiple(array $items) {
+      if ($max_expire != Cache::PERMANENT && ($item['expire'] == Cache::PERMANENT || $item['expire'] > $max_expire)) {

Let's use strict equality (===).

First rerolling for conflicts from #2336627: Deadlock on cache_config (DatabaseBackend::setMultiple()).

Fixing for comments in #41.

For point 3, I think we have the default in the constructor to make sure the value is there in case it is removed from settings.php. As @Berdir pointed out, we could comment it out.

For point 1, I am not sure if max_lifetime makes more sense than max_expiry but I have renamed it.

The patch in #45 wasn't complete (missed default.settings.php), but the interdiff was correct. I had missed the comment in #43 and fixing that now.

Sorry I meant just rename the settings 'max_expire', variable $max_expire is fine.

#42 So change the line in default.setting.php to

# $settings['cache_database']['render']['max_lifetime'] = 86400 * 7;

?

I think that setting (commented or not) should be before

# if (file_exists(__DIR__ . '/settings.local.php')) {
#   include __DIR__ . '/settings.local.php';
# }

, right?

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -42,6 +42,13 @@ class DatabaseBackend implements CacheBackendInterface {
      /**
   +   * Per-bin database backend configuration.
   +   *
   +   * @var array
   +   */
   +  protected $configuration;
   
   +++ b/sites/default/default.settings.php
   @@ -691,3 +691,17 @@
   +/**
   + * Per-bin database backend configuration.
   + *
   + * Configure settings for database backend. Available settings are:
   + * - max_expire: Specify the maximum expiry for an individual cache item in the
   + *   selected bin. Some cache bins can get very big and persistent cache entries
   + *   are never removed. Setting this value will allow them to be garbage
   + *   collected after the configured time, and will need to be rebuilt if the
   + *   item is requested again.
   + */
   +$settings['cache_database']['render'] = [
   +  'max_lifetime' => 86400 * 7,
   +];
   

   So this is an array, just in case additional settings are added in the future, right?

   Are we sure that's a good idea?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -191,14 +201,24 @@ public function setMultiple(array $items) {
   +    // Check if maximum expiration settings for cache bin is defined. If set,
   +    // enforce an upper limit for the expiration to allow for garbage
   +    // collection.
   ...
   +      // If maximum expiration is set, enforce an upper limit on the expiry.
   

   These comments are effectively saying the same thing.

   I think we can keep just the first comment.

   And I think this would be clearer:
   Check if a 'max_lifetime' setting is set for the current cache bin. If set, the lifetime of a cache item is limited to the specified 'max_lifetime', thus preventing unused cache items to linger forever.

3. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -191,14 +201,24 @@ public function setMultiple(array $items) {
   +    $max_expire = isset($this->configuration['max_lifetime']) ? $this->configuration['max_lifetime'] + REQUEST_TIME : Cache::PERMANENT;
   

   s/$max_expire?$expire/

4. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -36,6 +44,9 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
   +    // Load settings and if not set, default to maximum expiration of the render
   

   s/expiration/lifetime/

5. +++ b/core/modules/system/src/Tests/Cache/DatabaseBackendUnitTest.php
   @@ -53,4 +53,78 @@ public function testSetGet() {
   +    // Empty configuration would force DatabaseBackend to select permanent
   +    // lifetime for cache objects.
   

   No configuration (the default) allows cache items to live forever.

6. +++ b/core/modules/system/src/Tests/Cache/DatabaseBackendUnitTest.php
   @@ -53,4 +53,78 @@ public function testSetGet() {
   +    // Test set as well.
   

   s/set/set()/

Related to #51.1, there is one issue that would add a similar per bin setting #1944740: Do not garbage collect as soon as items expire.

#54: that's a good argument! :) However, that issue is going with a very different approach to creating a per-bin setting. Which one is preferable?

#54 #55:

I do think those two issues are orthogonal actually:

- This issue ensures that CACHE_PERMANENT or > expire is mapped to a max-expire value
- That issue ensures that expired items are not directly removed

While the other issue would potentially pro-long the time items of this issue are kept in the cache, I do not think it would help with the original problem.

Also staleness currently is not yet (widely?) used in core.

Train of unrelated thoughts

It would be very useful to return an old value, then setup a queue or after-request item to re-populate data, but thats not done currently.

Also likely would want to use a max-age=0 (or -2) then anyway, to avoid having an expired item be stored in another cache item.

Anyway.

TL;DR: This issue should continue as is.

#57: I agree with #57.

I also agree on that and that was my idea. I was just saying adding even more support for $allow_invalid = TRUE is likely not that helpful for core itself as there is no(t much) usage in core of that feature right now.

Oh, yes, I was not asking to bring more settings in! I was only making sure we made the right design choice for this setting: A) will we likely add another setting (yes), B) the other issue uses a different setting structure, which one do we want (this one).

So I think that effectively just leaves #51 to be addressed?

Fixed remarks from #51 and the test failures introduced in #45.

Looks great!

I'd like to see one more person's thoughts on max_lifetime vs. max_expire. It was olli's idea, I +1'd it, but I wonder if some people are not in favor.

#62.1: the first comment covers both, currently. But let's let the first then say "set the correct default value: the maximum lifetime", and the second can give the details of the overriding that are in the first comment currently.

removed debug code.
#62.2 commented out.
and some comments for #62.1

Alright, thanks for the many rerolls! :)

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -191,14 +201,25 @@ public function setMultiple(array $items) {
   +    $expire = isset($this->configuration['max_lifetime']) ? $this->configuration['max_lifetime'] + REQUEST_TIME : Cache::PERMANENT;
   

   When reading this it looks like it will be CACHE_PERMANENT unless configured.

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -36,6 +44,9 @@ class DatabaseBackendFactory implements CacheFactoryInterface {
   +    $this->configuration = Settings::get('cache_database', ['render' => ['max_lifetime' => 86400 * 7]]);
   

   But for cache.render we're always setting it - should we reference that in the comment above.

   If I was seeing the 7-day expiry, I'd probably check the backend first, then settings.php, then only after that find this in the factory if I was lucky.

   Also reading this, I wonder whether we couldn't put the default configuration into the container definition for cache.render, then it'd be kept closer to where the bin is defined - the factory doesn't know anything about specific bins in HEAD.

@catch: There is a small catch in the point 2 you have mentioned. It looks like we are always setting 'max_lifetime' even if not set in settings.php. That's not true though. If in settings.php, we set some other keys in that array but not max_lifetime, that value will no be set. That is where the CACHE_PERMANENT backup setting you pointed out in point 1 comes in.

@hussainweb - yes it's possible to not set it, I just think the factory is a strange place to put the default setting that most sites will end up using.

I'd very much prefer this to have in the container definition for a specific cache bin also. But if I remember correctly from an IRC conversation, that was considered impossible. I wish I wrote down on this issue why.

Coming back to this:

- I really think it should be per-bin. For example we'd want cache_bootstrap to stay with CACHE_PERMANENT, but maybe bring cache_render down to 12 hours.

- it's not really database specific, it's valuable for any cache backend that doesn't support LRU.

This means we need a term for non-LRU cache backends, I don't have one though.

There you have it: NonLruCacheInterface :).

Well there's that, but LRU is normally used as shorthand for 'storage size capped'.

APCu has a capped storage size, but this doesn't mean you can just throw items at it. Once it's full the behaviour depends on the ttl but the default is just to empty itself out entirely.

The way we deal with that is to not recommend APCu for any unbounded cache bins - instead it's restricted to ones where we expect the total size of cache items to be within a reasonable limit, so that it never gets full at all.

OK here's a suggestion that makes less assumptions about storage: 'configurable permanent ttl'.

If it's set, then cache storage can set 'permanent' cache entries to that length or higher, up to permanent again.

Here's an untested draft with the interface and CacheBackendInterface implementing it.

If the concept seems fine, we still need to sort out the configuration key for the bins, what it gets set to, and add tests.

I'm also going to bump this to critical, since the meta was triaged as critical and I don't think that meta gets fixed without this in some form.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -194,8 +199,10 @@ protected function doSetMultiple(array $items) {
+        $item['expire'] = $this->maximumPermanentTtl;

minimum vs. maximum mixup ...

----

I re-read my comments again:

1. I think this is a good case for a decorator class: e.g. BoundedDatabaseBackend + using a trait. then every cache backend can implement that by implementing the Interface + using the trait.

2. I still think that:

- a) Random Jitter is a good idea (it is horrible if all items expire at the same time)

- b) A mechanism for refreshing cache items that are accessed before the TTL expires is added (pseudo-LRU)

As the setting is per bin, a site owner can e.g. choose to use cache_page as Expire + Refresh to ensure larger items that are frequently accessed, stay in the cache, while smaller items that are not accessed at all would expire. (as Wim Leers wanted)

On the other hand someone that wants to cache smaller items, could just use a larger ttl for cache_render and a smaller one for cache_page.

To give my code some perspective.

Jitter is as simple as:

    foreach ($items as $cid => $item) {
+      if (!isset($item['expire'] || $item['expire'] === CacheBackendInterface::CACHE_PERMANENT))  {
+        $item['expire'] = $this->addTtlJitter($this->minimumPermanentTtl);
+      }
       $item += array(
-        'expire' => CacheBackendInterface::CACHE_PERMANENT,
         'tags' => array(),
       );
// ...

protected function addTtlJitter($ttl) {
  $diff = $ttl * $this->ttlJitter;
  return rand($ttl - $diff, $ttl + $diff);
}

While refreshTIme would work like:

protected function prepareItem($cache, $allow_invalid) {
  // ...

+  if ($this instanceof TtlAwareCacheBackendInterface && $cache->ttl === CacheBackendInterface::CACHE_PERMANENT && $cache->expire + $this->refreshTtl >= REQUEST_TIME) {
+   // Refresh the item in the cache, could also just update $expire.
+   $this->setMultiple([$cache]);
+ }

  return $cache;
}

I see the problem though that we have nowhere to save that the item should be cached permanently instead of an item that truly is time dependent - however I think we should save the ttl in any case in addition to the expire timestamp. That would solve that for all cache backends.

e.g. practical example: homepage should never be expired, is accessed several times per hour, permanent items are cached for a week, e.g.:

• - Set minimum Expire Ttl == 1 week
• - Set jitter = 10% = 0.10 (so that not all items expire at the same time)
• - Set refreshTime = 1 week - 1 day == 6 days (working set: 1 day)

So every item accessed within the delta of: minimumExpireTtl - refreshTime and originally cached CACHE::Permanent will be refreshed.

But it also happens only when the item is close to be expired.

And I do think this is superior than just expiring all content after 7 days when the last cache clear was, which is like a cache clear all (for cache_render e.g.).

And we all know that cache_clear_all can be problematic for little more frequently visited sites.

. I think this is a good case for a decorator class: e.g. BoundedDatabaseBackend + using a trait. then every cache backend can implement that by implementing the Interface + using the trait.

I struggle to see many people using this except the database backend, so not sure that's useful in practice. If the idea is to keep a reference implementation of the database backend then it seems good for that.

Random Jitter is a good idea (it is horrible if all items expire at the same time)

I also think that's a good idea, but it's 100% implementation - except for the new methods mentioning 'minimum' permanent ttl so it can vary upwards. I think we should add that before RTBC though yes.

The refresh idea is a good one, and I've done similar last-minute-update logic in custom code before, but it's going to require an additional column in the database schema and an update. Also we'd have to do that in a way that it doesn't blow up before the update runs which is not going to be straightforward - the column will be missing on any ->get() call.

Patch update that fixes the minor issues above, and doesn't explode doing drush cr.

If this passes, still needs tests, still needs jitter, and then we need to figure out whether to try the refresh or not here or move that to a follow-up.

Also not sure what to do about the change to CacheBackendFactoryInterface. We can't change the actual interface - as you can see from the patch I had to update the various factories including ones that don't need this.

We could allow the individual factories to add the optional parameter to the method, but then there's no interface to communicate what the param means.

Or possibly a new interface and something like addMethodCall..

I think addMethodCall is the way to go via the 'calls' property.

I see that it would be nicer to add as a parameter, but changing the interface is a no-go obviously.

And adding getWithTtl() feels just wrong.

Maybe we could use a 'tag', which is like a named parameter and a compiler pass to add the tag information as a method call?

--

On the other hand could we not introduce a new FactoryInterface that all core factories that want to support this would implement?

I think an entirely new factory interface might be the way to go. End of the day here but will have a look tomorrow.

Small update:

- added the new factory interface
- switched to an $options array for the service argument and new factory interface. That would potentially allow us to make things like the refresh and jitter configurable if we wanted to.

If that looks OK, will start looking at some of the rest from #81.

+++ b/core/core.services.yml
@@ -207,7 +207,7 @@ services:
+    arguments: [default, ['maximum_permanent_ttl', 86400]]

Shouldn't this be minimum_permanent_ttl?

Yes it should.

The setting of the expire time to the minimum ttl looks like it can be tested.

#88, #89: But is it not in fact a maximum?

e.g. PERMANENT == unlimited, while this is the maximum ttl time that is allowed to be set.

Though that is wrong, too. As you could set a higher value ...

I am still confused by the wording.

Agree with the tests.

I'm also confused. I'd also expect this to be called "maximum". But, really, it's not even maximum or minimum, it's just mapping PERMANENT to some configurable number. Perhaps a clearer name would therefore be overridePermanentMaxAge.

I'd also go with max-age rather than TTL because Cache-Control: max-age and because https://www.drupal.org/developing/api/8/cache/max-age.

Definitely needs tests - should be easy to add to the database backend tests.

On $minimum_permanent_ttl - I don't like the name either, clearly it's confusing even me.

In practice:

- it should be configurable via the container, as in the patch.
- backends should set the ttl no lower than the configured ttl, but for example if we use jitter, then they're going to set it higher than that (so it's not a maximum)
- backends could do other things like whitelist particular cids as permanent
- they could also ignore it and use permanent.

What about $overridePermamentTtl?

I'm not at all sure about max-age - we use expire in the database backend already which is already confusing, and this isn't going to be used for Cache-control header anywhere. When it's applied to the cache_render bin that may not be obvious at all.

TTL === max-age, no?

Yes it's the same thing, I just associate it with the Cache-control header more. Whereas ttl I think more of apcu or similar. I guess it's a case of whether that's a good thing or not that it has the association with the header.

Or $override_permanent ;)

   * @param int $expire
   *   One of the following values:
   *   - CacheBackendInterface::CACHE_PERMANENT: Indicates that the item should
   *     not be removed unless it is deleted explicitly.
   *   - A Unix timestamp: Indicates that the item will be considered invalid
   *     after this time, i.e. it will not be returned by get() unless
   *     $allow_invalid has been set to TRUE. When the item has expired, it may
   *     be permanently deleted by the garbage collector at any time.

Unfortunately the cache back-end interface is designed for absolute timestamps, so it's neither TTL nor max-age. Otherwise just following its existing terminology would've been perfect…

Well except the argument is a ttl - we convert it to an expiry in the backend. Similarly CACHE_PERMANENT is treated as a ttl - it really means never as opposed to just being a far future expiry.

Similarly CACHE_PERMANENT is treated as a ttl

It's not, it's an absolute time: "infinity". But PHP doesn't support infinity, so we map it to -1.

Well except the argument is a ttl - we convert it to an expiry in the backend.

True. But that's my point: we wouldn't need to discuss "TTL" vs "max-age" if we could just have be the same kind of thing as what we have, which is absolute numbers: timestamps.

So, we simply can't be consistent with what we have. If you then prefer TTL, works for me.

It's not, it's an absolute time: "infinity". But PHP doesn't support infinity, so we map it to -1.

Is now + infinity different to just infinity?

Added tests. Also added some basic jitter to the database backend.

Not doing anything with the wording yet - I'm not tied to ttl at all, but not sure we have a better option yet.

No interdiff - will start posting them from this point though since this is more or less minimally viable now and the patch is getting bigger.

Arggh missing new files.

Arggh missing new files.

Newb! :P

1. +++ b/core/core.services.yml
   @@ -207,7 +207,7 @@ services:
   +    arguments: [default, ['maximum_permanent_ttl', 86400]]
   
   +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -42,6 +42,11 @@ class DatabaseBackend implements CacheBackendInterface {
   +  protected $minimumPermanentTtl = CacheBackendInterface::CACHE_PERMANENT;
   

   Still inconsistent.

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -68,21 +68,16 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
   +        // @todo: check interface.
   

   Do you mean an assert($service instanceof SomeInterface)?

3. +++ b/core/lib/Drupal/Core/Cache/ConfigurablePermanentTtlInterface.php
   @@ -0,0 +1,21 @@
   + * collection. The actual TTL used may be the same or higher than configured.
   

   This last sentence is meant to communicate that there may be jitter? Not very clear to me.

   Also, why >=, and not <=? Documenting the rationale for that would be good I think?

4. +++ b/core/lib/Drupal/Core/Cache/ConfigurablePermanentTtlInterface.php
   @@ -0,0 +1,21 @@
   +  function setMinimumPermanentTtl($ttl);
   +}
   

   Missing newline between these two.

5. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -240,6 +252,14 @@ protected function doSetMultiple(array $items) {
   +   * Calculate permanent expiry.
   

   Missing @return.

6. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -240,6 +252,14 @@ protected function doSetMultiple(array $items) {
   +    return rand($minimum, $minimum * 1.5) + REQUEST_TIME;
   

   Shouldn't this 1.5 be a constant on the class as well, to make it clear what it's for?

7. +++ b/core/modules/system/src/Tests/Cache/DatabaseBackendUnitTest.php
   @@ -51,6 +52,21 @@ public function testSetGet() {
        $this->assertIdentical($cached_value_short, $backend->get($cid_short)->data, "Backend contains the correct value for short, non-ASCII cache id.");
   +
   +  }
   

   Extraneous \n.

Should cover everything in #104.

I think my vote is for override_permanent_maxage or override_permanent_ttl.

That says much clearer that this is an override for permanent cache items.

-    $consistent = $this->container->get($this-consistentServiceName);

+    $consistent = $this->container->get($this->consistentServiceName);

Was the test failure.

Switched to $override_permament_ttl for now.

Need to use the constant in the test also.

Opened a follow-up to discuss the refresh - that's not necessary to fix the critical for me, and having to do a schema update to cache tables feels very 8.2.x-only. #2705993: Add auto-refresh/quasi-LRU to database backend

Adds two new interfaces and updates the container, so tagging for rc target triage.

+++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
@@ -68,26 +68,34 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
+    $consistent = $this->container->get($this->consistentServiceName);
+    if ($consistent instanceof ConfigurableCacheBackendFactoryInterface) {
+      $consistent_backend = $consistent->get($bin, $options);
+    }
+    else {
+      $consistent_backend = $consistent->get($bin);
+    }

I think this would be much easier to read with a helper function.

$backend = $this->getCacheBackendWithOptions($name, $options);

The rest looks great to me.

Yes that's better.

RTBC - Looks great to me - except perhaps test coverage might need to be extended a little?

1. +++ b/core/core.services.yml
   @@ -207,31 +207,31 @@ services:
   +    arguments: [default, ['override_permanent_ttl', 86400]]
   ...
   +    arguments: [entity, ['override_permanent_ttl', 86400]]
   ...
   +    arguments: [menu, ['override_permanent_ttl', 86400]]
   ...
   +    arguments: [render, ['override_permanent_ttl', 86400]]
   ...
   +    arguments: [data, ['override_permanent_ttl', 86400]]
   

   Why not make this 86400 a container parameter itself, so you only have to override that one parameter on your sites if you need a different TTL?

2. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -68,26 +68,46 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
   +   * Get a cache backend with options if it supports it.
   

   s/Get/Gets/

3. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -68,26 +68,46 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
   +   * @return
   

   Missing type.

4. +++ b/core/lib/Drupal/Core/Cache/ConfigurableCacheFactoryInterface.php
   @@ -0,0 +1,23 @@
   +interface ConfigurableCacheFactoryInterface {
   

   Why not let this extend \Drupal\Core\Cache\CacheFactoryInterface?

5. +++ b/core/lib/Drupal/Core/Cache/ConfigurablePermanentTtlInterface.php
   @@ -0,0 +1,26 @@
   + * configured ttl could potentially be set very low for some use cases.
   

   s/ttl/TTL/

6. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -42,6 +49,11 @@ class DatabaseBackend implements CacheBackendInterface {
   +   * The overridden permanent TTL.
   

   "override" in variable name vs "overridden" in documentation.

   Note this is not even the overridden value, this is literally the value to use. It just might be overridden.

   So, I think $permanentTtl would be a better name.

7. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -240,6 +259,16 @@ protected function doSetMultiple(array $items) {
   +   * Calculate permanent expiry.
   

   s/Calculate/Calculates/

8. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -240,6 +259,16 @@ protected function doSetMultiple(array $items) {
   +   * @return an expires timestamp based on the override TTL with added 'jitter'.
   

   Wrong doxygen.

#1 done.

Minor points also done.

#6 done and wish we'd thought of that about 20 comments ago, much better.

#4 because not sure if it's clearer since they both have a single method with the same name and different arguments. But PHP allows it so why not, don't think it's less clear, just a bit neutral.

@Fabianx on tests. I thought about testing that the expires doesn't return a valid cache entry, but we already have tests for that, and it's tricky to test with REQUEST time without manipulating the value, which would then invalidate the test here. Could potentially test ChainedFast explicitly though.

Added explicit testing of the ChainedFastBackend when both the fast and consistent backends have permanent overridden.

That test passed but didn't test anything very interesting, better one.

That is enough on tests for me and all feedback has been addressed.

I think this should be ready now for core committer review.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -240,6 +259,17 @@ protected function doSetMultiple(array $items) {
+    return rand($minimum, $minimum * self::JITTER_MULTIPLIER) + REQUEST_TIME;

From https://secure.php.net/manual/en/function.rand.php

Note: On some platforms (such as Windows), getrandmax() is only 32767. If you require a range larger than 32767, specifying min and max will allow you to create a range larger than this, or consider using mt_rand() instead.

I think we need to use mt_rand() here. Patch attached does this.

Patch includes a few coding standards fixes too.

RTBC on the interdiff

Seems ready to me as well but I think we should improve some documentation.

1. +++ b/core/lib/Drupal/Core/Cache/ConfigurablePermanentTtlInterface.php
   @@ -0,0 +1,27 @@
   + * This allows cache backends with restrictions on storage space to set a
   + * shorter TTL for 'permanent' cache items, so that they get expired by garbage
   + * collection. The actual TTL used may be the same or higher than configured,
   + * to allow 'jitter' to be applied so that items set immediately after a cache
   + * clear do not all expire at the same time. Cache backends should assign jitter
   + * higher rather than lower to avoid the risk of cache stampedes, since the
   + * configured TTL could potentially be set very low for some use cases.
   

   I must say some awesome documentation. Can we add some of it above the JITTER_MULTIPLIER or at least add @see?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -235,6 +254,17 @@ protected function doSetMultiple(array $items) {
   +    return mt_rand($minimum, $minimum * self::JITTER_MULTIPLIER) + REQUEST_TIME;
   

   We can also explain this line as well as per #119.

1. +++ b/core/core.services.yml
   @@ -207,31 +208,31 @@ services:
   +    arguments: [default, ['override_permanent_ttl', '%cache.configuration.override_permanent_ttl%']]
   

   ', ' -> ': '?

2. +++ b/core/core.services.yml
   @@ -207,31 +208,31 @@ services:
        factory: cache_factory:get
   -    arguments: [render]
   +    arguments: [render, ['override_permanent_ttl', '%cache.configuration.override_permanent_ttl%']]
   

   This doesn't work with code that uses cache_factory directly like render_cache.

3. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -37,6 +44,11 @@ class DatabaseBackend implements CacheBackendInterface {
      /**
   +   * The permanent TTL.
   +   */
   +  protected $permanentTtl = CacheBackendInterface::CACHE_PERMANENT;
   

   missing @var?

4. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
   @@ -34,16 +34,14 @@ function __construct(Connection $connection, CacheTagsChecksumInterface $checksu
   +      $class->setOverridePermanentTtl($options['override_permanent_ttl']);
   

   ->setPermanentTtl(

Addressing #121 and #122.

However #122-2 is a good spot, and not sure what to do about that yet.

Rebased.

To be honest the more I work on this patch, the less I like it. For a site that doesn't get many 404s, it could degrade performance rather than improving it, and the back and forth here shows that the concept is quite hard to grasp. There are also other options here such as setting an arbitrary limit on cache items and pruning in garbageCollection if it goes over - I like that option less than this one though.

I'd be tempted to bump #2699613: Set a shorter TTL for 404 responses in page_cache module and #2699627: url.path cache context for breadcrumbs is unnecessarily granular to critical and mark this postponed on those. They'll benefit sites running any cache backend and if we can get them tested on a site like @casey's that's suffering from this issue, it'd then allow us to see whether this issue is really critical by itself.

Doing that for now.

What might make more sense is to add a "lurker" cron job, which expires cache entries that are actually expired per the cache tags.

Though I am not sure if our structure allows for that - given we only have a checksum in the DB backend. However for serialized render arrays we could do it at least as we there have the cache tags information.

I just ran into this after launching a new D8 site that uses the database backend (currently memcached or redis isn't an option); see #2814015: cache_render table becomes extremely large over time.

We don't have anything out of the ordinary, but there are a couple views exposed filters in blocks, and those blocks have 100,000+ rows in the cache_render table (one per filter set and url permutation—the block only appears on one route, but the path can have filter arguments like ?abc-xyz), which never expire unless I run drush cr manually (then they just fill up again until the next cache clear).

We were dealing with a database around 10-15 MB total before launching the prod site; now that there's live traffic, the database balloons out to 10+ GB every day until we clear the cache. Backups and full database copies take 30+ minutes instead of 30s...

I believe I have seen maybe 5-10% of all Drupal sites I've ever worked with using a cache backend other than the database, and I see no reason that will change with Drupal 8—this could be a huge problem for anyone trying to run a small-to-medium sized Drupal 8 site on shared hosting with even pretty basic sites.

Closed #2814015: cache_render table becomes extremely large over time as a duplicate.

And yes, more common... but at least 96% of the sites I've ever dealt with are not on Pantheon, Platform.sh, Acquia, etc. There's a silent majority of Drupal sites out there running on Hostgator, Dreamhost, preconfigured DigitalOcean VPSes, etc., and this problem (along with some others) doesn't help silence the mantra of "Drupal 8 is not for shared hosting"...

I agree with a minimal fix for now—the jitter doesn't seem (to me) like it would be a major issue/blocker.

#138: I do agree that some kind of expiration would be nice.

However please be aware that there is a working contrib backend by larowlan already:

https://github.com/larowlan/slushi_cache

I still think that using jitter is simple and can be achieved easily (and not sure what catch' last patch is missing anyway except for reviews and tests?) and we should:

- a) make simple changes
- b) ensure it will work for more complex cases, too.

Core has a higher responsibility in that regard.

--

To the issue above:

Likely it is indeed best to max-age=0 the view result with that many permutations (are they all valid?) and have the whole block be placeholder-ed automatically.

I have upgraded the site to 8.2 version, it shows issue with the size of cache table and also deadlock error.Any help is appreciated

With the rise of container based hosting like pantheon, platform.sh, acquia and so on which afaik all offer redis/memcache, I do think that using such a backend will be more common than it used to be.

This is a bold statement!

100% of the Drupal sites I am working with are not on those platforms.

We don't have anything out of the ordinary, but there are a couple views exposed filters in blocks, and those blocks have 100,000+ rows in the cache_render table (one per filter set and url permutation—the block only appears on one route, but the path can have filter arguments like ?abc-xyz)

Do you have 10s/100s of thousands of requests changing those filters then? I wonder a bit whether caching with views exposed filters wouldn't benefit better from some flood control or something - stop entries getting there in the first place. Would have to do the flood control when rendering the view and then set max_age=0 when flood control kicks in.

Backups and full database copies take 30+ minutes instead of 30s...

Well, that is a huge flaw in your backup system: you should never ever back up the cache_* tables.

But of course we should still solve the problem described in this issue.

Unassigning me since I'm not actively working on this at the moment.

AFAICT we have agreement this is a good idea.

It seems the last point of disagreement is the inclusion of jitter or not?

This problem also came up in #2828639: [BUGFIX] getRelated is using wrong cache tags.

And I know that some Acquia customers have run into this very problem too.

FWIW Slushi cache solves this via contrib in the meantime.

I observed a similar issue on shared hosting, which did not seem to be addressed by installing Slushi cache.

Some shared hosting providers will shut sites down with minimal warning if database sizes grow too large - as geerlingguy says in #138, issues like this are likely to push more small sites away from Drupal.

We experienced this error when a google bot was crawling our facetted search, the cache_render table was growing to a couple of GB within minutes. We had to put our site in maintenance mode and exclude the facetted search in robots.txt to deal with it. Site is up and running again now, with a cronjob clearing the cache every 6 hours just to be sure... Not exaclty ideal. We thought about putting varnish in front of it, but since the varnish bin is shared with other websites it might compromise those if it grows this fast...

@malcomio Slushi cache should work fine for shared hosting, what makes you think it wouldn't? You'd have to configure the TTL time quite low if you're getting crawled though.

@catch on the side project site where I experienced this issue, it still occurred after installing Slushi cache, although looking at settings.php now, I think that it may not have the correct configuration. I'll check and if I'm still having problems, I'll create an issue on the module.

Subscribing, experiencing this for a site i've built. Slushi cache won't help (or i'm probably setting it wrong). DB without cache is about 75 MB big, it grows to 250+ MB in one day. And then the hosting provider just shuts down database user access, so i get a understandably angry client on the phone and i'm getting tired of manualy clearing cache x times a day. Bigger database won't fix this, it will just take a little longer to fill up the DB to max size.

@Jelle_S: Faceted search is the sort of module that should take responsibility to not cache every single variation. Because it's a module that allows insane numbers of variations. So of course it does not make sense to cache them all.

Just registering a real-life example with a 8.2 site. PHP 5.6, core modules + Honeypot and a custom multimedia-related module, no memcache or alternate cache backend.

+--------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in abcdefgh_db    | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+--------------------------+--------------+------------+--------+-------------+---------------------+
| cache_render             | 8312.00 MB   | 7978.88 MB | 220276 | 49.84 MB    | 0.01                |
| cache_dynamic_page_cache | 2924.00 MB   | 2706.72 MB | 43703  | 24.00 MB    | 0.01                |
+--------------------------+--------------+------------+--------+-------------+---------------------+

Rebuilt caches / Truncated tables

+--------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in abcdefgh_db    | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+--------------------------+--------------+------------+--------+-------------+---------------------+
| cache_render             | 176.00 MB    | 156.70 MB  | 7444   | 2.55 MB     | 0.02                |
| cache_dynamic_page_cache | 96.00 MB     | 78.52 MB   | 1406   | 1.52 MB     | 0.02                |
+--------------------------+--------------+------------+--------+-------------+---------------------+

4 days after:

+--------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in abcdefgh_db    | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+--------------------------+--------------+------------+--------+-------------+---------------------+
| cache_render             | 5120.00 MB   | 4689.84 MB | 116175 | 33.98 MB    | 0.01                |
| cache_dynamic_page_cache | 2348.00 MB   | 2207.66 MB | 39439  | 18.86 MB    | 0.01                |
+--------------------------+--------------+------------+--------+-------------+---------------------+

The Local Actions and Local Tasks blocks also vary by URL, and they're enabled for all users by default, even though most sites only need those blocks for authenticated users (because it's possible that they're used on a wiki-like site that allows anonymous users to use them).

Those two blocks in particular can cause enormous amounts of cache entries. Which further confirms the need for this issue to be solved.

#156 should the default for Local Actions and Local Tasks be to not show up for anonymous users, then? If yes, perhaps that should be a follow-up issue.

I just noticed one site (the one that originally surfaced this issue for me) hit a full disk on the database server (100 GB) from the cache_render table expanding again. A drush cr fixed it for now, but this happened because for a few weeks, no code deployments were done (thus no cache rebuilds took place).

The entire database (besides cache_render) is under 1 GB :-/

We have set Local Actions and Local Tasks blocks on the site mentioned by Aurelian above to 'authenticated' only. I am monitoring to see if this actually fixes the issue.

I'm OK with this going in without jitter, we can just move that to a follow-up and it's only an implementation detail in the database cache backend.

The only really reliable solution is using a cache backend with a fxed size like redis or memcache.

This. I already wrote this in #8.3, almost two years ago.

Caching with unbounded storage makes no sense. In practice, every cache has a limited size. A database isn't designed to store caches, but if it is, then we should limit the number of items we store.

What I still don't understand is why we can't just move forward with one simple setting as initially proposed, that's not perfect but it's simple and fairly reliable. I think that sites that grow so quickly that this isn't enough really shouldn't use the database backend anyway. i can work on updating the patch for that if we can agree on going forward with that.

+1. If you update the patch, I promise swift reviews. Let's finally get this done. This is biting many D8 sites.

+1 for #163

+1 Just discovered my fairly small site with database of 15gb when it's really just 50mb.

Agreed. +1 for #163

Amazing that this issue is actually running for so many years.

+1 just discovered my brand new flashy bling bling high traffic optimized website of 42.9mb on Friday night went onto 1GB over the weekend. Thanks cache render

Do we actually have a way to solve this that is practical and long term?

Can someone clarify which of these patches may be workable at this point and what need to be fixed on them to get a working solution? Happy to apply a patch and test, but the existing patches are old.

No movement in a month :( I promised swift reviews in #163 of a reroll. So I'm going to turn it around: I'll do a reroll, hopefully swift reviews will follow!

To do what I said, the "reroll of the simple approach", I had to first find the simple approach. I went back all the way to the beginning.

• The patch that I originally RTBC'd (on August 12, 2015) in #66 seems to be close to what we want. It was un-RTBC'd because there was discussion A) about it being specific to the database cache back-end, B) to make it a per-bin setting.
• To address A, @catch rolled #79, as a PoC. He made it an actual working patch in #82 and #87.
• Then @Fabianx started talking in #81 about "bounded DB backend", jitter and pseudo-LRU.
• @catch added jitter support in #104.
• @catch continued to refine the patch in subsequent comments, but it remained fundamentally the same. It was RTBC'd in #118 by @Fabianx.
• @olli spotted a critical flaw that all of us missed, in #122.2. All prior patches fail for cases where code is using the cache factory directly!
• Then in #129 @catch comes to the realization that he doesn't like this approach/patch. He thinks postponing this on #2699613: Set a shorter TTL for 404 responses in page_cache module and #2699627: url.path cache context for breadcrumbs is unnecessarily granular would be good,
  because those would already solve the problem for many sites.
• Since then, both #2699613 and #2699627 landed, and the problems still exist. We see this on Acquia Cloud. And @Berdir also said it in #131.
• @Berdir in #137 and again in #161 asks why we can't solve this with a simple setting. But of course, @olli spotted that critical flaw. So I started looking at how slushi_cache solves this. Turns out it solves this in a very simple way: by accepting a single "permanent TTL" setting instead of one per bin (Slushi Cache calls this the "melt time" 👏), it avoids the problem that @olli pointed out. And it is a simple decorator of the existing code. We could literally adopt Slushi cache as-is IMO. Or we could merge its code into DatabaseBackend.
  It does NOT meet two requirements that @catch posed though: A) per-bin configurability, B) being generic, instead of specific to the DatabaseBackend. But that's the thing, this problem is pretty much specific to the DatabaseBackend!
• However, #152 and #153 show that they don't fix the entire problem. But they sure are reducing the problem's impact. The problem is that we don't know it will be enough per se.

So, if we want "the simple approach", I think we basically want to move Slushi Cache into core? 2526150-slushi_cache-170.patch does this. With two added features: per-bin overrides (but in a way that it won't break like @olli pointed out for earlier patches) + allowing to specify 0 or -1 to opt out (that was a flaw in Slushi Cache).

However … it looks like @Berdir and I are thinking something similar: the root cause is much simpler. @Berdir in #161:

The only really reliable solution is using a cache backend with a fxed size like redis or memcache. A max age size as proposed here would help at least small to medium sized sites to not grow too large over time.

I in #163:

Caching with unbounded storage makes no sense. In practice, every cache has a limited size. A database isn't designed to store caches, but if it is, then we should limit the number of items we store.

So here's a provocative thought… what if we do limit the amount of space that database tables can consume? LRU requires bookkeeping, so that's off the table. If you want LRU, use Redis or Memcached. But we can easily achieve fixed-size DB cache bins:

1. Add a serial field to DatabaseBackend::schemaDefinition()
2. Do something like DELETE FROM cache_data WHERE id <= ((SELECT MAX(fixed_size_id) FROM cache_data) - 1000)

So here's a patch implementing that. Including an upgrade path even. 2526150-bounded_database_cache_bins__count-170.patch

But then I wondered whether we could do even better: what if we could actually assign size limitations not by count but by space?
Turns out that MySQL at least allows this: https://stackoverflow.com/a/18238810 — you can query information_schema. And then if we construct the right query, we can get the number of megabytes consumed by a given table: https://stackoverflow.com/a/9620273. What remains then of course, is removing a number of rows to make it fit in that size again. We can't know the size of an individual record, so we'd have to assume they're all sized the same, and then remove the percentage that would make it fit within the budget again. Before you get too excited: this would then be MySQL-specific, which would be … sad. Anyway, very very rough PoC for this attached.

So, #170 contains 3 approaches, all fairly simple:

1. slushi_cache: moves https://www.drupal.org/project/slushi_cache into core, with minimal adjustments — this is probably the "simple" approach that was being asked for in
2. count: makes the DB cache bins bounded (fixed size), by row count
3. size: makes the DB cache bins bounded (fixed size), by disk space

Options 1 and 2 are database engine-agnostic. Option 3 is database engine-specific, and therefore an unlikely choice.

Option 1 does not protect against surges in activity, e.g. bots crawling your site, or malicious users attacking your site, option 2 does.

And finally, a provocative related question: #2888838: Optimize render caching.

And I just realized this was made major instead of critical in #130 because it was though that two other issues would help (see #129), but they did not. Therefore the original criticalness still applies. Back to critical.

The current title implies one particular solution. As of #170, there is a very different option. So, retitling for clarity & simplicity.

So here's a provocative thought… what if we do limit the amount of space that database tables can consume? LRU requires bookkeeping, so that's off the table. If you want LRU, use Redis or Memcached. But we can easily achieve fixed-size DB cache bins:

I was thinking about this issue this morning, and had a similar but different thought:

- have a per-bin settings-configurable number of rows
- when the size of the bin exceeds the number of rows, find the median created timestamp per-bin and delete all items created before that timestamp (i.e. about 50% of the table).
- do this on cron so there's no cost with cache sets.

This will clear some space without resulting in a complete cache clear, tables will be able to go over the limit for up to three hours.

My concern with both that approach and the one suggested by Wim is that it could lead to thrashing, but again memcache is the real answer to that.

Haven't reviewed the patches yet.

For what it's worth, I like #176 a lot!

My concern with both that approach and the one suggested by Wim is that it could lead to thrashing

Really? What's a scenario in which #176 would result in thrashing? It's true that frequently used items could get purged shortly after their last use, but never shortly after their first use.

#176 is a more advanced (and complicated) version of 2526150-bounded_database_cache_bins__count-170.patch. Why do we want to do stuff with median timestamp etc? Why not just look at the row count versus allowed row count, and nothing else?

I'm probably missing something :)

@effulgentsia, I think @catch is saying that large sets of cache items could get cleared at once, and then regenerated at once (see jitter). It would probably depend on the cron interval, content, and access patterns.

The median timestamp approach in #176 appears intended to better preserve recently-created cache entries, but could also still allow large cache table size, and doesn't really approximate LRU. For example, if you get traffic that maxes the cache out during each cron interval, you cycle between ~50% and 100% of the peak cache size, assuming evenly-distributed timestamps. The cache hit ratio and % cleared might be unpredictable if the requests during a cron interval are front- or back-loaded. If you're getting the kind of traffic that makes the above concerns valid maybe you are using another cache backend, though.

The simple row-count approach provides a more predictable upper bound on DB size, which is the main issue here.

@catch, Wim's row-count approach only runs on cron, as you suggested.

Really? What's a scenario in which #176 would result in thrashing? It's true that frequently used items could get purged shortly after their last use, but never shortly after their first use.

A bad but relatively common scenario I have in my head is something like this:

Cron run every ten minutes.

There 2 requests per second to unique pages from a crawler.

Mean of 20 render cache items per page.

This means:

1,200 unique pages visited in ten minutes.

24,000 cache items created in ten minutes.

Cache item limit of 10,000.

Whether you go down to 10,000 or 5,000 items, you're going to be deleting tens of thousands of rows per hour then recreating them again.

If we take a Drupal.org node with 300 comments by say 50 unique users, there could easily by 4-500 individual render cache items per page on a cold render cache, so you could hit a high row limit from a much smaller number of pages.

@tedfordgif thanks for answering Wim's 178 that's exactly the design and drawbacks - this wasn't a competitor to Wim's suggestions it was just what I was mulling when I saw this morning, and haven't really thought through beyond that (hence why I didn't post anything).

Why do we want to do stuff with median timestamp etc? Why not just look at the row count versus allowed row count, and nothing else?

I think doing rows is fine. But why add the bounded_id column? Why not COUNT and ORDER BY created? Which I think would have the same effect of deleting the oldest first, which is the key feature in both #170 and #176. Or is the problem that adding an index on a decimal field is less efficient than on a serial field?

24,000 cache items created in ten minutes...Cache item limit of 10,000.

I think that's the problem with the #180 scenario. You shouldn't have a site that, as part of its normal operation, creates more cache items between cron runs than your limit. If it's an occasional blip, that's one thing, but if it's happening all the time, it means you need to do one of:

• Increase the limit
• Switch to a more advanced cache backend
• Disable render caching
• Write a module to disable caching conditionally (e.g., during crawler requests)

You shouldn't have a site that, as part of its normal operation, creates more cache items between cron runs than your limit. If it's an occasional blip, that's one thing, but if it's happening all the time,

Question for me is - how would you find that out without manual auditing of cache tables? With the current issue you find out because your database is huge. One thing we could do is track on cron runs how many items were deleted vs. how many deleted last cron run or similar and log it, then at least there's an indication what's happening.

#178: Exactly, it could still allow a large cache table size. Rather than a simple "FIXED SIZE, PERIOD", it's "while limit exceeded, delete ~50% and hope that will keep it within bounds". So I agree with The simple row-count approach provides a more predictable upper bound on DB size, which is the main issue here. — my intuition says it's better to KISS to solve the problem for now, then we can introduce more fancy algorithms later. "fixed size but only kinda and with deletion based on median timestamp" is conceptually a lot harder to understand than just "fixed size". So I fear doing this would introduce another Drupalism.

#180: I think this is about solving the common case. The default could be 10,000 entries. But for bigger sites, you'd want to tweak that number.

#181:

1. Because I forgot that created even existed :) Happy to reroll if that approach is what we end up agreeing on.
2. I agree that it's a matter of tweaking for those sites that see a lot of traffic. The default value should cater to small and medium traffic sites.

#182: We could do tracking, but yesterday's traffic is not necessarily an indication of today's traffic. Plus, then we're making it pretty complex.

It sounds to me like we've entered a bikeshed where the color is replaced with What should the default DB cache bin row count limit be? as the primary question and Ohhh do we want special weighted ways for making it fit within the limit? as the secondary question. The problem is that "the best weighted way" will depend on the particular site. That's why I'm arguing to go with the simplest possible solution.

Rather than a simple "FIXED SIZE, PERIOD", it's "while limit exceeded, delete ~50% and hope that will keep it within bounds".

The row limit patch isn't a fixed size though, it's trimming down to the maximum size/row every cron run - i.e. it will immediately exceed the fixed size as soon as a new cache entry is created, until the next cron run.

Given that, and unless we run those queries on insert which would be a performance hit, I'm not sure what this gets us beyond the configurable TTL for permanent entries - with that the longevity of a cache entry is predictable at least and it'll prevent unbounded growth.

Given that […] I'm not sure what this gets us beyond the configurable TTL for permanent entries - with that the longevity of a cache entry is predictable at least and it'll prevent unbounded growth.

With a configurable TTL for permanent entries, you still have an unbounded number of cache items. If your site has a traffic surge, or is DDoSsed or spidered or something else, then the gigabytes of data will set there a full day (assuming that configurable permanent TTL defaults to 86400 as in today's patches).

With a fixed size, you prevent that.

i.e. it will immediately exceed the fixed size as soon as a new cache entry is created, until the next cron run.

This is correct. But I think this is less problematic than the problem I described above.

I've laid out my thinking, but I don't feel strongly which approach should be the one. Either one is a big step forward. I'll let @catch choose.

2526150-bounded_database_cache_bins__count-170.patch being tested now. Applied without error, no other apparent issues on the site. After only about an hour, I can say that the DB size has remained stable on a site which bloats (very) rapidly. As of right now:

| Tables in dbstage1 | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+-----------------------+--------------+------------+--------+-------------+---------------------+
| search_index          | 52.00 MB     | 20.58 MB   | 381744 | 22.58 MB    | 1.10                |
| search_dataset        | 18.00 MB     | 10.52 MB   | 1068   | 0.00 MB     | 0.00                |
| comment_field_data    | 15.00 MB     | 2.52 MB    | 10184  | 4.50 MB     | 1.79                |
| comment__comment_body | 15.00 MB     | 6.52 MB    | 8523   | 0.91 MB     | 0.14                |
| node_revision__body   | 12.00 MB     | 4.52 MB    | 1062   | 0.22 MB     | 0.05                |
+-----------------------+--------------+------------+--------+-------------+---------------------+
+-----------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in dbstage2 | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+-----------------------------+--------------+------------+--------+-------------+---------------------+
| search_index                | 36.00 MB     | 14.55 MB   | 331914 | 13.55 MB    | 0.93                |
| cache_field                 | 29.00 MB     | 21.56 MB   | 9688   | 0.34 MB     | 0.02                |
| field_data_comment_body     | 27.00 MB     | 12.52 MB   | 17760  | 6.55 MB     | 0.52                |
| watchdog                    | 25.00 MB     | 16.52 MB   | 11584  | 0.56 MB     | 0.03                |
| search_dataset              | 17.00 MB     | 9.52 MB    | 1054   | 0.00 MB     | 0.00                |

For comparison, this is the same DB before a 'drush cr':

--------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in dbprod1    | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+--------------------------+--------------+------------+--------+-------------+---------------------+
| cache_render             | 4912.00 MB   | 4580.58 MB | 91155  | 22.72 MB    | 0.00                |
| cache_dynamic_page_cache | 1692.00 MB   | 1482.86 MB | 29326  | 16.84 MB    | 0.01                |
| cache_data               | 232.00 MB    | 196.89 MB  | 72672  | 10.58 MB    | 0.05                |
| search_index             | 76.00 MB     | 40.48 MB   | 380034 | 24.33 MB    | 0.60                |
| cache_entity             | 76.00 MB     | 64.66 MB   | 7341   | 1.52 MB     | 0.02                |
+--------------------------+--------------+------------+--------+-------------+---------------------+
+----------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in dbprod2 | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+----------------------------+--------------+------------+--------+-------------+---------------------+
| search_index               | 36.00 MB     | 14.55 MB   | 332827 | 13.55 MB    | 0.93                |
| cache_field                | 29.00 MB     | 21.56 MB   | 10739  | 0.34 MB     | 0.02                |
| field_data_comment_body    | 27.00 MB     | 12.52 MB   | 17835  | 6.55 MB     | 0.52                |
| watchdog                   | 25.00 MB     | 16.52 MB   | 11312  | 0.56 MB     | 0.03                |
| search_dataset             | 17.00 MB     | 9.52 MB    | 912    | 0.00 MB     | 0.00                |
+----------------------------+--------------+------------+--------+-------------+---------------------+

I will ping back later today or tomorrow with a new size report.

@mariagwyn, it would be helpful to see the size of and number of rows in your cache_render table. How often do you run cron?

If you're using MySql, the following query might be helpful (replace 'drupal' with your database name):

SELECT table_name AS `Table`, TABLE_ROWS as 'Num. of Rows', round(((data_length + index_length) / 1024 / 1024), 2) `Size in MB`  FROM information_schema.TABLES where table_schema = 'drupal' and table_name = 'cache_render'  ORDER BY (data_length + index_length) DESC limit 10;

I've laid out my thinking, but I don't feel strongly which approach should be the one.

Seems to me the only difference between the two approaches is: when num_items_during_cron > max_items, should we prune to max_items (suggested by Wim) or to max_items/2 (suggested by catch)? However, the "2" in catch's formula seems pretty arbitrary. So I'd like to propose a 3rd option: prune to max_items * (max_items / num_items_during_cron).

What this does is make it so that we create ahead of time an amount of free space that's a function of how much we're likely to need.

In other words, if our limit is 10,000 but we're currently at 11,000, then we can prune to 10K * 10K/11K = 9091, which leaves nearly 1K for future growth prior to hitting the limit again. On the other hand, if we're currently at 40,000 (got to 4x our limit since the last cron run), then we'd prune down to 2,500. Of course, if >30K items got added since last cron run, then the freed up 7.5K will likely get used up pretty quickly, and we'll be back in a situation of exceeding our limit, but per #181, I think such a large disparity between the limit and what the site uses within a single cron interval is a bigger problem than this issue's scope should be.

@tedforgif:

For comparison, here is prod. Note that it was cleared earlier today but has been growing in size:

--------------------------+--------------+------------+--------+-------------+---------------------+
| Tables in prodDB    | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+--------------------------+--------------+------------+--------+-------------+---------------------+
| cache_render             | 360.00 MB    | 299.92 MB  | 12937  | 3.53 MB     | 0.01                |
| cache_dynamic_page_cache | 120.00 MB    | 102.72 MB  | 2989   | 1.55 MB     | 0.02                |
| search_index             | 76.00 MB     | 40.48 MB   | 380034 | 24.33 MB    | 0.60                |
| cache_entity             | 68.00 MB     | 59.64 MB   | 7326   | 0.47 MB     | 0.01                |
| cache_data               | 32.00 MB     | 20.59 MB   | 7405   | 1.52 MB     | 0.07                |
+--------------------------+--------------+------------+--------+-------------+---------------------+

Num rows:

+--------------+--------------+------------+
| Table        | Num. of Rows | Size in MB |
+--------------+--------------+------------+
| cache_render |        12983 |     303.45 |
+--------------+--------------+------------+
1 row in set (0.00 sec)

On Testing site/DB, with db copied immediately after 'drush cr' on prod:

DB Size:

| Tables in dbstage1 | Size on Disk | Size in DB | Rows   | Index in DB | Index-to-data ratio |
+-----------------------+--------------+------------+--------+-------------+---------------------+
| search_index          | 52.00 MB     | 20.58 MB   | 381744 | 22.58 MB    | 1.10                |
| search_dataset        | 18.00 MB     | 10.52 MB   | 1068   | 0.00 MB     | 0.00                |
| comment_field_data    | 15.00 MB     | 2.52 MB    | 10184  | 4.50 MB     | 1.79                |
| comment__comment_body | 15.00 MB     | 6.52 MB    | 8523   | 0.91 MB     | 0.14                |
| node_revision__body   | 12.00 MB     | 4.52 MB    | 1062   | 0.22 MB     | 0.05                |

And number of rows:

+--------------+--------------+------------+
| Table        | Num. of Rows | Size in MB |
+--------------+--------------+------------+
| cache_render |           64 |       1.53 |
+--------------+--------------+------------+

Seems to me the only difference between the two approaches is

Indeed. Well, roughly, because @catch's proposal is more complex than max_items/2.

In other words, if our limit is 10,000 but we're currently at 11,000, then we can prune to 10K * 10K/11K = 9091, which leaves nearly 1K for future growth prior to hitting the limit again.

I think this is simple enough to be "KISS". But I need to repeat my warning from #183: We could do tracking, but yesterday's traffic is not necessarily an indication of today's traffic.

@mariagwyn Thanks for sharing those numbers. It confirms that this patch is solving the symptoms. But your data doesn't yet tell us whether this would negatively impact response times. How did cache hit ratios change?

I'm still in favor of the simple row limit. We're not trying to eliminate the (render_)cache entirely in this ticket, so emptying the cache to some percent of the max rows setting is not a goal. If we're getting the same traffic pattern during each cron interval, any cache entries we remove will be recreated, so we might as well just leave them in the cache if they fit in the "limit".

I initially liked the approach in #188 for having math, and I thought some more about the various ways of calculating the average size of the cache items, but I'm still firmly in the camp of KI(really)SS, meaning simple row limit. It's tunable both by cron frequency and row limit, and doesn't try too hard.

As mentioned previously, future gold-plating options include creating a db_cache_profiler module, which would help you tune those two parameters by sampling cache get/set, or db_cache_tuner, which keeps things tuned in real time. But the right approach is to get a real cache outside Drupal.

For what it's worth, I agree. I think simply pruning to max_items is fine for resolving this critical. It means we're always over the limit, and therefore, we might need to figure out a good setting name for it ("max", "limit", and "bound" aren't entirely accurate), but I think that's ok.

+1 for the simple approach - solves the problem and is really simple to understand. re #192, perhaps an example in default.settings.php with help text along the lines of Set an upper row limit on the database cache bin, enforced on cron run. would suffice?