Page and block caches are cleared the same way on cron as on content change

Does anyone have thoughts on this? Knowledge of why system_cron() clears caches at all?

It doesn't. system_cron() clears *temporary entries* from the cache table. It's a garbage collection mechanism that is very necessary.

Why pages and blocks are cached with CACHE_TEMPORARY is the actual question.

If page_cache_maximum_age is set, this is used as the value of $cache->expire, see http://api.drupal.org/api/function/drupal_page_set_cache/7

However block module always does CACHE_TEMPORARY - cache_set($cid, $array, 'cache_block', CACHE_TEMPORARY);

We could look at changing this to use variable_get('block_cache_maximum_age', $something), but that means either breaking string freeze and a new dropdown to the performance page, or a hidden variable.

I don't understand everything in this fread, but I'm experimenting performance issues on a drupal 6 website with very low frequentation. The first one to access the page will have to wait a while for the page to show up. Once that's done the other visitors have reasonably fast access to the website. I suspect that's due to the cache cleared by cron. If that's not totally out of subject here, could you please share hints about how to handle cache for a small D6 website ?

edit :
I updated to the latest version of boost, it has become very userfriendly and it seems to solve my problems.

Subscribing.

Yeah, I am scratching my head trying to think of why CACHE_TEMPORARY ever makes sense to use in cache_set()... it seems like it rarely would? (It's sort of like a declaration that "I don't trust my code to clear caches correctly when it's supposed to so instead I'm going let cron clean it up whenever it's run" which is just bad.)

Just a note that #4 was wrong, $cache->expire will be set if there's an Expires header, but not from page_cache_maximum_age.

I can confirm the behavior described by effulgentsia in the original post, and add that 'page_cache_maximum_age' is only used to set the 'Cache-Control' header "max-age" value in Drupal core. When you use something like Varnish, the max-age tells Varnish how long it should serve that page from its cache before checking the back end for an updated version. It also tells the browser how long to hold the cached page for. I cannot get Varnish to send cached pages unless max-age is set to something greater than 0. That said, I could configure Varnish to manipulate the Cache-Control header when it comes in and force it to cache objects, but it does by default respect this max-age setting, controlled by page_cache_maximum_age.

Do we need to change node_form_submit(), comment_form_submit(), and friends to cache_clear_all() with $wildcard=TRUE?

I also do not know a lot about the history of why the settings are the way they are. Another argument could be, just change CACHE_TEMPORARY to CACHE_PERMANENT in drupal_page_set_cache() and then some contrib module could take care of the user expectations that page caches should be cleared when nodes and comments are updated. Views, panels, and other sections and page callbacks in Drupal provided by contrib modules and composed of nodes or other entities will also come with this expectation. It seems like the API functions are there to let a module like http://drupal.org/project/cache_actions or http://drupal.org/project/expire or future module deal with that complexity. If core just clears pages aliased to nodes or when comments are added/updated on nodes aliased or not to pages (cids) it seems too specific where a more general module could take its place. It could all go into one of these contrib modules, as long as the pages are getting stored with the correct 'expire' value (not CACHE_TEMPORARY).

Patch attached to use CACHE_PERMANENT instead of CACHE_TEMPORARY

Looks like the testbot shows why that won't work (and it is pretty darn awesome that there is a test - in the poll module, no less!! - to catch that).

I think what this means, unfortunately, is that this bug will be difficult to fix in Drupal 7 without something resembling an API change. (Though you could certainly argue, as @effulgentsia says, that node, comment, as well as any contrib module should always have been clearing the entire page/block cache anyway in these situations, because you never know who else might have stored something permanent in there.)

I think we should consider setting an expires header if page_cache_maximum age is set, that would give some kind of sanity to this, although reading through the original reverse proxy issue there might be a problem with expires headers and certain proxies, but I'm not really up on exactly how much of a problem that is.

All of this can be done in contrib though, so I'd be tempted to move this to 'major' and D8, and solve it in contrib for D7, and there's already work started on that in performance_hacks module.

I think we should consider setting an expires header if page_cache_maximum age is set, that would give some kind of sanity to this, although reading through the original reverse proxy issue there might be a problem with expires headers and certain proxies, but I'm not really up on exactly how much of a problem that is.

That sounds like a decent plan to me.

As noted above, we also need to change the cache_clear_all() function to clear all the entries in block and page forcefully ($cid = '*' and $wildcard = TRUE).

+

Subscribing

memcache has various workarounds for this, the latest version is in #1134242: Review cache_lifetime behaviour..

While it is tempting to kill CACHE_TEMPORARY that needs some work and can't be done in a way that could be backported to Drupal 7, there's an interim measure we can do to improve things a bit before then though:

* add a garbage collection method to the cache interface.
* the core cache.inc will have the same code for content clears and garbage collection, at least for now, that's fine.
* add hook_cache_bins() - this would stop modules that do things other than declare cache bins in hook_flush_caches() from having their code executed every cron run.
* system_cron() would then get all cache bins, load the class that controls them, and run cache_gc() on each.
* this could be backported, we can partially workaround the cron issue for the core cache.inc by only running the garbage collection once every x hours, instead of every single cron run.

Also something like this may end up blocking a full memcache 7.x release - since the 6.x branch solves some of this by changing the logic in cache_clear_all() - cache_clear_all() itself is no longer pluggable in 7.x.

Subscribe

xref: #81461: Clean up the cache API (stop overloading function arguments, remove procedural wrappers)

This actually blocks a stable release of Drupal 7 memcache module now, so I'm bumping to major.

n Drupal 6, all of cache.inc was swappable, this meant cache backends could do their own logic in cache_clear_all().

In Drupal 7, cache_clear_all() is hard-coded in cache.inc, and just calls cache_clear_all(NULL, 'cache_page');.

That means there is no longer any way to differentiate between cache_clear_all() triggered by node or comment posting, and cache_clear_all() triggered by cron's garbage collection (see system_cron()).

In core/db caching, this means that the page cache gets wiped every cron run.

In memcache, it's the opposite - the cache doesn't get wiped on wildcard clears.

See #1246796: Memcache fails its own tests. and #1103478-16: Skip some wildcard fetches for background where this issue was fixed in memcache - which required doing custom logic in cache_clear_all() when called with no arguments.

The way I'd like to fix it is by adding a new method to the cache API (either to the interface or optional, depends how we feel about bc for other cache backends), which gets called when cache_clear_all() is called with no arguments.

In the database backend this could just do what cache_clear_all() does now, but for memcache it would allow the workarounds in the 6.x-1.x branch to be implemented.

That new method already exists in this patch #81461: Clean up the cache API (stop overloading function arguments, remove procedural wrappers), however that contains a lot of changes that aren't necessary to fix this bug, so bumping this one to major for the memcache blocker.

The fix proposed here of setting an explicit expire on page and block cache entries (more or less deprecating CACHE_TEMPORARY) will fix the database issue, but it won't fix it for memcache at all. We might need to split it out into two issues, but trying to keep it in one place for now.

I've recently come across the problem with Drupal 7 and memcache not clearing the page cache after the page cache maximum age is passed. After searching through the code I was surprised that the page cache maximum age is not used when adding the page into the cache.

To fix the memcache issue and other backends wouldn't it be sensible to set the expire time of the cache_set in drupal_page_set_cache (common.inc:4964) to expire at REQUEST_TIME + page_cache_maximum_age?

I may be misunderstanding this test or even the whole thread and where things stand, but just to weigh in:

NOT caching all pages because a poll wouldn't work properly from a cached page, to me, indicates that the poll implementation needs to be fixed to clear the cached page upon submission, or to ajax in the results, or to disallow a vote from anonymous based on a cookie, or something along those lines.

Nuking all pages in the cache every time cron runs so polls can work is swatting a fly with a cannon.

I've already had to patch core in D6 for this issue. Please consider this carefully for D7 and D8.

cache_page should honor cache_lifetime, imho, as that's the naive expectation of every user who visits the Performance page.

cache_page getting totally wiped out every time cron runs as default behaviour is definitely "unexpected behaviour"

Even Drupal community members who should know better got this wrong in the comments on the metaltoad page referenced. :-)

As much as I love that a test caught this, I really do not want to maintain my one-liner D6 core patch when I install D7/D8, just for the sake of making a great test for a mis-implemented poll feature "pass" :-) :-) :-)

Here's a rough start on what I outlined in #20. The only core change is in system_cron() and DrupalDatabaseCache.

This should be more or less backportable as is to D7.

Cache backends may now specify a garbageCollection() method.

system_cron() by default only does garbage collection every 24 hours.

Not expecting this to pass tests but you never know.

Use 'cron' as the flood identifier.

Based off the discussion in #1279654: Page cache is CACHE_TEMPORARY and does not honor cache_lifetime, the issue with this patch would still be on content pages that are created prior to the content actually being expired (and the cache expiry time being set to CACHE_TEMPORARY (which is -1)) with the garbage collection running. For example:

• Page caching is set to expire every 6 hours.
• Garbage collection occurs every day at midnight.
• User creates page at 10pm.

Under this scenario, the content should not expire until atleast 4am. However, because the content has an expiry time set to -1, the page will still expire at midnight because its expiry time is less than the current time of garbage collection. I **think** one of the changes that should be happening is that the page expiry time should be set to the current time plus the page lifetime so the cache affects the relevant expired pages.

So the problem with using $cache_lifetime as the value for expire is that this turns it into a maximum - if your cache lifetime is 5 minutes then once #534092: cache_get() returns expired cache items is fixed you're in serious trouble since items will last shorter than CACHE_TEMPORARY items do now.

However you're right that even if we reduce the frequency of garbage collection we still have a problem, so here's a new patch.

Instead of using a variable prevent cache items from being cleared more often than cache_lifetime, it instead runs two queries. One to remove expired items, and one to remove CACHE_TEMPORARY items that were created longer ago than variable_get('cache_lifetime')

Still untested, however if that query is OK it should solve the two conflicting issues here.

Well apart from that.

Removed method_exists() since this is part of the interface in D8 now.

Note I am specifically not adding an update to remove the cache_flush_ . $bin variable since some alternate cache backends still use it.

The Drupal 7 update here will be tricky since we need to find all cache_* tables and add an index to them, however I believe there are schema functions for this so it ought to be possible without invoking hooks.

After a few times of reading through and trying to understand what is going on, it finally makes sense to me :) However, is there a reason for garbageCollection() in the D8 version if its just a wrapper for expire()?

But aside from that, the code that actually matters looks sound. So its a +1 from me towards RTBC.

The reason to have the two methods is so that memcache and similar which handle expiry of cache items themselves are able to add a no-op method there. This could use some tests for the cron logic.

Wanted to see how it runs with a million items in cache_page so hacked cache.inc to use time() and made a script to put 1m items in there (after creating 100k with a different script first before I realised that probably wasn't enough).

That ends up with:

MariaDB [d8]> SELECT COUNT(DISTINCT created) FROM cache_page;
+-------------------------+
| COUNT(DISTINCT created) |
+-------------------------+
|                    1021 |
+-------------------------+
1 row in set (0.41 sec)

MariaDB [d8]> SELECT COUNT(*) FROM cache_page;
+----------+
| COUNT(*) |
+----------+
|  1065003 |
+----------+
1 row in set (0.15 sec)

Found a point just over half way:

MariaDB [d8]> SELECT COUNT(*) FROM cache_page WHERE expire = -1 AND created < 1317475580;
+----------+
| COUNT(*) |
+----------+
|   667187 |
+----------+
1 row in set (0.23 sec)

That query is nicely indexed:

MariaDB [d8]> EXPLAIN SELECT COUNT(*) FROM cache_page WHERE expire = -1 AND created < 1317475580;
+----+-------------+------------+-------+----------------+----------------+---------+------+--------+--------------------------+
| id | select_type | table      | type  | possible_keys  | key            | key_len | ref  | rows   | Extra                    |
+----+-------------+------------+-------+----------------+----------------+---------+------+--------+--------------------------+
|  1 | SIMPLE      | cache_page | range | expire_created | expire_created | 8       | NULL | 271206 | Using where; Using index |
+----+-------------+------------+-------+----------------+----------------+---------+------+--------+--------------------------+
1 row in set (0.00 sec)

Then deleted those:

MariaDB [d8]> DELETE FROM cache_page WHERE expire = -1 AND created < 1317475580;
Query OK, 667187 rows affected (16.62 sec)

then I tried deleting 500k items with the old query:

MariaDB [d8]> SELECT COUNT(*) FROM cache_page WHERE expire = -1;
+----------+
| COUNT(*) |
+----------+
|   497816 |
+----------+
1 row in set (0.12 sec)

MariaDB [d8]> DELETE FROM cache_page WHERE expire = -1;
Query OK, 497816 rows affected (9.01 sec)

So the new query is about 60% slower than the old one with 500-600k items (although I'm only running this once each on my laptop so there's a potentially wide margin of error, so main thing is that it's not magnitudes different). Either way the cache hit rate should be improved, and deleting 500k items all at the same time should be extremely rare.

I did a similar test with 100k rows in the table, deleting 20k, and that took 8ms to delete.

Maybe it's just me, but it seems like the CSS cache clearing routine, at least in D6, is quite brain-dead in regards to pages that might be cached with references to the CSS files in question...

Step 1) Get your Page to cache for a long time.
Step 2) Something calls drupal_clear_css_cache.
Step 3) Surf to a Page in the cache.
Step 4) View perfectly valid HTML that references 404 CSS files. Lovely.

Maybe you guys have this covered already in D8, but I'm still trying to get decent performance out of D6 page_cache...

I don't claim to know the right answer to this issue, but perhaps each CSS file should have a registry of cache_id values that reference it or something... Ouch.

@richardlynch that's off topic for this issue, and there are contrib modules like advagg that deal with it in D6.

My generate script for 1 million rows was:

$cache_lifetime = variable_get('cache_lifetime', 300);
for ($i = 0; $i < 1000000; $i++) {
  $rand = mt_rand(-1, 0);
  $text = 'lorem ipsum dolor sit amet';
  db_insert('cache_page')
    ->fields(array(
      'cid' => 'cache-test-'. $i,
      'data' => $text,
      'expire' => $rand,
      'created' => mt_rand($time - $cache_lifetime * 2, $time),
      'serialized' => 0,
    ))
    ->execute();
}

(note that I did not go through cache_set as this gave me a bit more control on the created time). I have to figure out and write a test to see that it works well (though running it through 1M items is a bad idea given it took nearly 20 minutes just to generate the items on my comp). But the tests by eye so far indicate that it is working as expected.

@catch
Unfortunately, my naive patch to just make page_set_cache honor cache_lifetime appears to run afoul of the CSS brain-dead clear-all...

We seem to be having a theme-less (CSS-less) page every couple days, and I believe my patch is the cause.

It's quite the crisis right now, and they're wanting a whole lot of analysis before I revert my patch in PRODUCTION...

The more I study D6 caching, the more it looks like spaghetti just waiting for a tiny blunder to cause an avalanche of issues.

If you can make the page cache lifetime longer, and *NOT* have the CSS required by that page disappear out from under it, you're a better man than I.

I'm just going to live with poor performance and let Drupal take the fall for it. Sorry.

NOTE:
If you can keep your page lifetime smaller than CSS lifetime, you should be fine...

But I was wanting both to be like 6 months or a year, and only managed to make a page out-live its CSS and JS requisites.

Ouch.

@RichardLynch: That is still off-topic here. As catch already said advagg deals with that in D6 very nicely. Boost also has solutions for that (re-caching CSS files at another place), so please use one of those solutions and be happy :-).

To the original issue: Nice progress here. I've also been bitten by the cache clear on cron, but with Varnish in front its not as bad.

#29: lifetime.patch queued for re-testing.

Re-rolled for /core.

#40: lifetime.patch queued for re-testing.

Looks like there have been significant changes to core's cache.inc since last post.

Where does this issue stand?

I don't intend to hijack this thread, but I'm coming to this issue with my primary concern being that page cache entries last only as long as the time of the last cron run + cache_lifetime. (Many issues relating to that one issue are marked as dupes of this, which is why I'm here.)

A can't find a concise, accurate summary of this anywhere, so here's my crack at it:

In the worst case, that means the page cache is wiped on every cron run. If an administrator sets their cache_lifetime to, e.g. 6 hours, then cache entries are wiped every six hours, regardless of when the page cache entry was created (so even if a cache entry is created at 5:50pm, if cron runs at 6:00pm and its been cache_lifetime since the last time page cache was flushed, the 10-minute-old entry will also be wiped too). As has been mentioned before, this is unintuitive and very bad for performance.

Rather than making very large changes to how the cache clear happens, I think it makes more sense to make very small changes to how drupal_page_set_cache operates.

Catch's workaround proposed in #12 (setting an expires header in contrib based on page_cache_max_age) is almost workable, but I can think of cases where decoupling external cache lifetime and internal page cache lifetime is desirable.

Why not just make drupal_page_set_cache() set a default expiration of REQUEST_TIME + cache_lifetime if a cache_lifetime exists, otherwise default to CACHE_TEMPORARY?

Patch for D8 supplied knowing nothing about D8, but hoping for a direct backport to 7.

iamEAP, that's patch is not going to get committed - it means the minimum and maximum cache lifetime becomes the same, which is not the right fix.

#40 could do with a re-roll though, the actual code hasn't really changed much, just moved around.

Could you clarify how the minimum would also become the maximum cache lifetime in that situation? Also, is it even relevant? All the minimum cache lifetime guarantees is a minimum cache lifetime. Beyond the lifetime, nothing can or should be guaranteed.

The text on the performance page (in 7) reads as follows:

Cached pages will not be re-created until at least this much time has elapsed.

As noted many times in this thread, this is not actually happening in any current version of Drupal. The patch in #44 makes the least number of changes (no API changes, unless I'm misunderstanding "API" in this context), stays consistent with the text on the page, clarifies the meaning of cache_lifetime, and could easily be written to apply to all current versions of Drupal, whereas the approach from #40 makes more significant API changes and doesn't have as clear a backport to 7 (let alone 6).

if $expire = $minimum_cache_lifetime + REQUEST_TIME, then as soon as that timestamp is reached, the item will be invalid, hence, it becomes the maximum. This will actually make things considerably worse rather than better for sites that have a short cache lifetime and longer cron intervals.

Re-roll of #40 (due to #22336: Move all core Drupal files under a /core folder to improve usability and upgrades).

Moved the 24 hours logic out of system cron to the database backend itself, some other small cleanup as well.

This should fix the install failure.

Here's a test that fails without the patch, and passes with it.

Also a new patch because I hadn't updated properly for the config conversion.

.htaccess cruft.

Re-rolled after #1015946: Eliminate $user->cache and {session}.cache in favor of $_SESSION['cache_expiration'][$bin].

With the attachment.

(Mostly) code style review.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -113,10 +113,31 @@ class DatabaseBackend implements CacheBackendInterface {
+  }
+ ¶

Trailing space here.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -113,10 +113,31 @@ class DatabaseBackend implements CacheBackendInterface {
+    // When setting cache items, clean up old session data in case it's
+    // stale.

AFAIK it's should be "it is" as short versions shoudn't be used. At least for interface text, but I guess that applies for comments as well.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -184,29 +205,22 @@ class DatabaseBackend implements CacheBackendInterface {
+    if ($cache_lifetime = config('system.performance')->get('cache_lifetime')) {
+      // Clear expired items from the cache. ¶
+      db_delete($this->bin) ¶
+        ->condition('expire', CACHE_PERMANENT, '>') ¶
+        ->condition('expire', REQUEST_TIME, '<') ¶
+        ->execute(); ¶
+      variable_set('cache_flush_' . $this->bin, 0); ¶
+ ¶
+      // Clear CACHE_TEMPORARY items that are older than the minimum cache lifetime. ¶
+      db_delete($this->bin) ¶
+        ->condition('expire', CACHE_TEMPORARY) ¶
+        ->condition('created', REQUEST_TIME - $cache_lifetime, '<') ¶
+        ->execute(); ¶
+     } ¶
+     else { ¶

No idea why, but every single line here has exactly one trailing space. Weird.

+++ b/core/lib/Drupal/Core/Cache/InstallBackend.phpundefined
@@ -32,6 +32,12 @@ use Exception;
 class InstallBackend extends DatabaseBackend {
+  function __construct($bin) {
+    if ($bin != 'cache') {
+      $bin = 'cache_' . $bin;
+    }
+    $this->bin = $bin;
+  }

That should probably have a Overrides DatabaseBackend::__construct() docblock.

+++ b/core/modules/system/system.installundefined
@@ -682,7 +682,7 @@ function system_schema() {
     'indexes' => array(
-      'expire' => array('expire'),
+      'expire_created' => array('expire', 'created'),

I guess this needs an update function?

I believe the plan for D8 page cache via WSCCI is to replace it with Symfony Kernel's HttpCache.

Given that work would almost definitely not be back-portable, would it be worth splitting this into separate issues where this one focuses on a "quick" fix for D7 (utilizing work from the existing patch), and we create a separate issue for D8 for page cache re-implementation via Symfony HttpCache?

Yes, the possibility of switching to Symfony's HttpCache definitely sounds like something that should be discussed in a separate issue, rather than here.

But this issue would still need to be fixed in D8 first (before backporting to D7).

Yes discussing HttpCache is completely out of scope here, this is a straight bugfix for backport and even if the page cache is completely replaced, that wouldn't necessarily touch the code here. There's too much coupling in core still between the page and block cache and the cache API (i.e. the fact that cache_clear_all() hasn't been fully removed yet), but theoretically CACHE_TEMPORARY and minimum lifetime which are fixed here is an independent part of the API from the block and page caches, and I've seen them used as such in custom code before.

Attached re-roll deals with Berdir's review I think, two things not included:

- rather than the 'overrides' docblock for InstallBackend::__construct() I just removed the function since it's the same as the DatabaseBackend anyway.

- there's no update in the 8.x patch, one will need to be added for the D7 backport though.

Adding 'contributed project blocker' since memcache is currently having to differentiate between these via debug_backtrace().

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -199,29 +215,24 @@ class DatabaseBackend implements CacheBackendInterface {
-      $_SESSION['cache_expiration'][$this->bin] = REQUEST_TIME;

Where did this go?

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -199,29 +215,24 @@ class DatabaseBackend implements CacheBackendInterface {
+      variable_set('cache_flush_' . $this->bin, 0);

What is this for?

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -199,29 +215,24 @@ class DatabaseBackend implements CacheBackendInterface {
+      // lifetime. ¶

Trailing space.

Thanks! Actually all the session stuff is completely unnecessary now, as is the variable_set(), so those are removed from the patch, I'd forgotten how much clean-up this allows. Also fixed the trailing whitespace.

Hmm ovesslin pointed out by e-mail that we can't remove the $_SESSION stuff here, got over-enthusiastic:

Before patch:
- CACHE_TEMPORARY items are stored until either cron runs or cache_clear_all() is called, as soon as that happens they're deleted.
- if minimum cache lifetime is enabled, then for the current user they're flushed immediately due to a timestamp stored in $_SESSION, for all other users they're flushed when the current timestamp is greater than variable_get('cache_flush_' . $bin); + $cache_lifetime

After patch:
- CACHE TEMPORARY items are stored until cache_clear_all() is called, or for up to 24 hours (older items are cleared out on cron).
- if minimum cache lifetime is enabled, then items older than the minimum cache lifetime are deleted immediately (no variable tracking, just deleted), for the current user, all items are marked as invalid via the $_SESSION variable. Items are remove by garbage collection on cron when they're older than 24 hours or minimum cache lifetime, whichever is the greatest.

This just removes the stray variable_set() and the trailing whitespace per #62.

Comment nitpicks.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -199,29 +215,23 @@ class DatabaseBackend implements CacheBackendInterface {
-      $_SESSION['cache_expiration'][$this->bin] = REQUEST_TIME;

Still missing... Tests could check for it.

When minimum cache lifetime is enabled and there's two comments/content updates in $cache_lifetime seconds, then anonymous users
- before: will see both changes in $cache_lifetime seconds
- after: will see both changes in 24 hours
Is this correct?

+++ b/core/modules/system/system.module
+++ b/core/modules/system/system.module
@@ -3068,7 +3068,7 @@ function system_cron() {

@@ -3068,7 +3068,7 @@ function system_cron() {
   $core = array('cache', 'path', 'filter', 'page', 'form', 'menu');
   $cache_bins = array_merge(module_invoke_all('flush_caches'), $core);
   foreach ($cache_bins as $bin) {
-    cache($bin)->expire();
+    cache($bin)->garbageCollection();
   }

Should we add a way for modules to figure out in hook_flush_caches() that we're just collecting garbage?

When minimum cache lifetime is enabled and there's two comments/content updates in $cache_lifetime seconds, then anonymous users
- before: will see both changes in $cache_lifetime seconds
- after: will see both changes in 24 hours
Is this correct?

Not quite:

Before: will see both changes in $cache_lifetime seconds (kinda, there's some edge cases in the current code)
After: will see changes immediately for any cache item that's older than cache lifetime. Cache items that are younger will 'survive' until they're either garbage collected or if there's a content change once cache lifetime has passed for them.

With hook_flush_caches() do you mean for modules that are doing something other than returning cache bins? While hook_flush_caches() is completely abused for various purposes and calling it during cron is a bug, that's a bit different to this issue, however it's partly dealt with in #1167144: Make cache backends responsible for their own storage and I think a separate issue to add hook_cache_bin_info() to decouple it from hook_flush_caches() would be a good idea.

I'll try to re-roll shortly. We added some tests for cache lifetime recently, so I'm surprised they're all passing with the broken patches, but yeah would be good to add to those a bit.

Yes, that's what I mean with hook_flush_caches(): field and features do system_rebuild_module_data() in system_cron() while update and ctools introduce workarounds. Decoupling sounds good.

separate issue to add hook_cache_bin_info() to decouple it from hook_flush_caches()

Also note that we're attempting a major decoupling in #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding - might possibly resolve this idea already, not sure.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -127,9 +122,30 @@ class DatabaseBackend implements CacheBackendInterface {
   function set($cid, $data, $expire = CACHE_PERMANENT, array $tags = array()) {
+    // When setting cache items, clean up old session data in case it is stale.
+    $this->cleanSession();

In some cases it might take time to clean the session because page cache is disabled if session is not empty.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -234,38 +244,24 @@ class DatabaseBackend implements CacheBackendInterface {
+    // Clear expired items from the cache.
+    db_delete($this->bin)
+      ->condition('expire', CACHE_PERMANENT, '>')
+      ->condition('expire', REQUEST_TIME, '<')
+      ->execute();

For items with expire > 0 caches are still cleared the same way on cron as on content change.

+++ b/core/modules/system/tests/cache.test
@@ -317,6 +317,41 @@ class CacheClearCase extends CacheTestCase {
+  function testCacheTemporary() {
+    $cache = cache($this->default_bin);
+    // Set a permanent and temporary cache item.
+    $cache->set('test_cid_clear1', $this->default_value, CACHE_TEMPORARY);
+    $cache->set('test_cid_clear2', $this->default_value);

It would be good to add an expired item (expire < REQUEST_TIME) and one that is not expired to the test.

+++ b/core/modules/system/tests/cache.test
@@ -317,6 +317,41 @@ class CacheClearCase extends CacheTestCase {
+    // Now after expiring the bin, neither the temporary nor permanent item
+    // should be removed.
+    $cache->expire();
+    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value));

For current user the temporary item should be removed.

To prevent younger items surviving, could we have "the timestamp of the last global flush" just like memcache module?

Dries committed #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support on Wed, 13 Jun 2012.
Therefore, according to Catch, this is now a 7.x-only issue.

Does this warrant another round of discussion now that we don't need to worry about a backport, or is a straight port of the D8 patch in progress sufficient?

Complete shot in the dark. Mostly what's found above sans tests, plus the update function.

@iamEAP, I was writing the 8.x patch explicitly for backport to D7, and with the longer term aim of adding cache tags in mind, so apart from the bugs that olli identified personally I think it's fine as a straight port.

Same...

Edit: Good to know. Thanks, catch.

Edit again: missing semi-colon...

Alright. First stab at a more or less direct port of #65.

Additionally...

• Adding expired and as yet unexpired cache items to the test as per #70
• Adding the update function to system for the new index on cache bins

Adding an explicit check in the update function to replace the index if it actually dropped it. Also, minor comment cleanup in system.module.

If it's reviewed (see http://drupal.org/patch/review) and a maintainer commits it in time, yes.

There are some problems with the last patch:

• On a minor note, the call to flood_register_event has the wrong number of parameters.
• Relatedly (and more importantly), cron will only flush the cache once every twenty four hours, regardless of how your minimum cache lifetime is set. Probably not intended.
• The update function may miss some cache tables.

I've also started a contrib module to address this issue: http://www.drupal.org/project/adbc

+ $core = array('cache', 'cache_form', 'cache_path', 'cache_filter', 'cache_bootstrap', 'cache_page');
+ $cache_tables = array_merge(module_invoke_all('flush_caches'), $core);

This update could run during an update from Drupal 7 to Drupal 8 (it's unlikely but at the moment it could), this and any disabled modules may mean that module_invoke_all('flush_caches') doesn't return a complete list.

The cache tags patch already had to deal with this issue for 8.x, and the 7.x code should be the same:

/**
 * Modifies existing cache tables, adding support for cache tags.
 */
function system_update_8007() {
  // Find all potential cache tables.
  $tables = db_find_tables(Database::getConnection()->prefixTables('{cache}') . '%');

  foreach ($tables as $table) {
     // Assume we have a valid cache table if there is both 'cid' and 'data'
     // columns.
     if (db_field_exists($table, 'cid') && db_field_exists($table, 'data')) {
       // Do schema changes here.
     }
  }
}

Fixed the update function according to catch's comments in #84. Also fixed my remaining comments from #83:

• Flood registers the event correctly now (previously, it gave a window of 1s due to a typo).
• Instead of taking the maximum of cache lifetime and the garbage collection frequency variable, we're taking cache lifetime if it's set, and garbage collection otherwise.

Clearing on cron every 24 hours is by design, minimum cache lifetime is supposed to be a minimum, not a maximum, and the cache will still be cleared if there's a content change.

I suppose it is configurable. Here's #80 with flood_register_events fix and the new update function.

I'm a little concerned there are some contrib modules that don't use the word "cache" in their cache table names (which I don't believe is explicitly required), but I would imagine that number is small enough not to worry too much.

Clearing cache every 24 hours is better than art every cron run. But one question of the original issue is not answered yet imho: Why has system_cron() to clear caches at all?

system_cron() is supposed to only do 'garbage collection' for non-permanent cache items when they've expired. If an item has an explicit TTL then that's OK, the problem is with CACHE_TEMPORARY which has always already expired as soon as it's created (apart from minimum cache lifetime which is disabled by default and not very well understood).

The bug here is that core never differentiated between garbage collection and explicitly clearing temporary items due to content changes, as well as expiring CACHE_TEMPORARY items too frequently during garbage collection (i.e. every cron run).

Do I understand this right: With this patch only the temporary cache will be cleared every 24 hours. But because e.g. the page cache is now permanently it will be only cleared if there's a content change.

Where in D7 does the cache get cleared on content change? I see in D6 there's an empty call to cache_clear_all(), at the end of node_save, but nothing of the sort in D7.

In Drupal 7 those cache clears were moved to submit handlers, mainly so that things like mass imports of tens of thousands of nodes can handle cache clearing themselves at the end instead of once per node.

Works for me, although it did not apply cleanly.

#88: d7-cache_garbage_collection-891600-88.patch queued for re-testing.

We worked around this in memcache by running a debug_backtrace() to determine what fired the cache clear. That's considerably less than ideal, but given the workaround exists I'm going to demote this from major.

#88: d7-cache_garbage_collection-891600-88.patch queued for re-testing.

This is a re-roll of #88. Work great for me.

So this works fine for Fidelix, iamEAP and myself. Do we need more people to test it before marking as RTBC?

#99: d7-cache_garbage_collection-891600-99.patch queued for re-testing.

marking as RTBC.

Why 24 hours? What do I do If I want to have a longer minimum cache lifetime than that - not that I see the use case in doing so, but what justifies adding a constant here?

Nevermind. I read the logic backward. Makes significantly more sense now.

Overall this seems to take Drupal 7 caching into a much nicer direction, but I saw some issues:

1. The update function number needs to be bumped. (I would have fixed this on commit if it weren't for the other things below.)

2. -        // We store the time in the current user's session. We then simulate
   -        // that the cache was flushed for this user by not returning cached
   -        // data that was cached before the timestamp.
   -        $_SESSION['cache_expiration'][$this->bin] = REQUEST_TIME;
   

   I don't understand why this is being removed (and if it is being removed, why the other code that uses $_SESSION['cache_expiration'] isn't).

   If you remove it, don't you run into the exact problem that it was designed to solve? (Example: Configure the site to cache pages and to have a minimum cache lifetime of 10 minutes, then give anonymous users permission to edit content. After the patch, an anonymous user who edits content won't see their own edits fully take effect right away; instead they'll have to wait 10 minutes.)

3.    function getMultiple(&$cids) {
        try {
   -      // Garbage collection necessary when enforcing a minimum cache lifetime.
   -      $this->garbageCollection($this->bin);
   

   Why is this being removed from getMultiple() but left behind in isEmpty()? Doesn't that mean cache_get() and cache_is_empty() can return inconsistent results?

4.    protected function garbageCollection() {
   ...
   -    $cache_flush = variable_get('cache_flush_' . $this->bin, 0);
   -    if ($cache_flush && ($cache_flush + $cache_lifetime <= REQUEST_TIME)) {
   -      // Reset the variable immediately to prevent a meltdown in heavy load situations.
   -      variable_set('cache_flush_' . $this->bin, 0);
   

   Partially related to (or perhaps dependent on) the answer to the above question... but aren't we worried about these meltdowns? After the patch, garbage collection will do a db_delete() twice every single time it is called, and it can easily be called on normal page requests.

5. This patch seems to have a side effect unrelated to CACHE_TEMPORARY which I'm not sure was discussed above. If I'm reading the code right, then before the patch, for a cache_set() that was done with a specific expiration timestamp (let's say 5 minutes from now), and if the site had the minimum cache lifetime set to something longer (let's say 10 minutes), it would be forced to stay in the cache for at least 10 minutes.

   After the patch, it can disappear after 5 minutes if a cache clear happens then.

   That may very well be the correct behavior, but it could use some discussion. (For example, I wonder if it could cause issues on sites that may be relying on long minimum cache times for everything.)

Also a few other issues that are more minor:

1. +    // Assume a valid cache table if both 'cid' and 'data' columns exist.
   +    if (db_field_exists($table, 'cid') && db_field_exists($table, 'data')) {
   +      if (db_drop_index($table, 'expire')) {
   +        db_add_index($table, 'expire_created', array('expire', 'created'));
   +      }
   +    }
   

   I'm all in favor of checking 'cid' and 'data', but why aren't we checking 'expire' and 'created' too (since those are the actual columns that the code below relies on being there)? Probabaly not too likely to matter in practice, but seems like an obvious omission nonetheless.

2. @@ -3025,7 +3025,15 @@ function system_cron() {
   ....
   -    cache_clear_all(NULL, $table);
   +    // Expire all temporary and expired cache items. Limit this to once every 24
   +    // hours by default to avoid wiping page and block caches every cron run.
   +    $cache = _cache_get_object($table);
   .....
   +      $cache->clear();
   

   Why are we switching to the private _cache_get_object() function all the sudden? Doesn't cache_clear_all(NULL, $table) still work?

3. +  function testCacheTemporary() {
   +    $cache = _cache_get_object($this->default_bin);
   +    // Set a permanent and temporary cache item.
   +    $cache->set('test_cid_clear1', $this->default_value, CACHE_TEMPORARY);
   

   Similar question here (and actually throughout the test): Why is this using _cache_get_object() rather than the standard API?

Is this still an issue in D8?

No this has been fixed in 8.x. Via #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support which isn't backportable.

Updated issue summary.

So, what is the current recommended approach for D7?

Use some Contrib modules that expire way less:

e.g.

- advanced page cache
- boost
- blockcache_alter (for blocks)
- render_cache (for full nodes)

Then on a cache miss you at least don't have to rebuild everything.

--

However this patch could still have a chance to go in overall.

reroll of #99 against current 7.x (as of 7.43)

Needs to be rerolled for the latest stable drupal version, 7.54, because of a conflict with update 7081.

Patch in #113 needs to be re-rolled for the latest stable drupal version because of a conflict with system_update_7082().

Meanwhile it is still not clear what is the best approach for D7. Comment #107 states that the fix in 8.x is cannot be back-ported and comment #109 suggests using contrib modules that "expire less".

Attached is the patch in #113 re-rolled against the latest drupal 7.68 we have been using the patch in its different rolls for sometime now. It seems better than the default.

hi
@izmeez patch is working for me on config below:
--Drupal 7.69
--Install--profile--Panopoly (panopoly-7.x-1.72)
-- php 7.1.31

Thanks

reroll for 7.70 to avoid conflict with system_update_7083() - we'll just keep doing this, i guess

Re-roll for drupal 7.71 to avoid conflict with system_update_7084()

Yet another reroll to bump the fake update hook number for 7.95.

Is continually bumping up this fake update_N hook in the system module preventing the real hooks from running? Like, d7-cache_garbage_collection-891600-119.patch defined system_update_7086(). Now core Drupal 7.98 has a real system_update_7086() function. But won't my installation think that it has already run?

Yes, that is a good point, thanks!

In case someone has run the update hook from this patch, it is likely that the real system_update_7086 introduced by #2164025: Improve security of session ID against DB exposure or SQL injection in Drupal 7.98 will not run. This will cause that these sites would not use the security improvement, because the session hashing will not be enabled. See: https://www.drupal.org/node/3364841

I recommend that all sites using this patch should carefully check that change record and a committed patch and take measures to ensure that they have the session hashing enabled.

I've never been quite clear about the system_update_N function in this patch. It tries to find all existing cache tables and replaces any existing "expire" index with an "expire_created" index. Running in an update function like this, that's a one-time check, right? If new cache tables are created with an "expire" index, this would do nothing about that. I don't know if that would happen. But if a one-time check will suffice, then it seems like instead of interfering with the real update hooks of the system module, that check should move somewhere else.

There are two changes in the system.install file. Change in system_schema:

function system_schema() {
     ...
     'indexes' => array(
-      'expire' => array('expire'),
+      'expire_created' => array('expire', 'created'),

And the new update function system_update_7086().

These two changes cover two usecases - if the site already has cache tables, then the update hook will update the indexes there. On installation (or creation of a new cache_xx table) the system_schema() comes to play and it will ensure that all new tables would have expire_created index. This is because all other cache tables are using the main cache table definition. See:

$schema['cache_block'] = drupal_get_schema_unprocessed('system', 'cache');

$schema['cache_form'] = $schema['cache'];

So both changes in the install file are needed - one for new installations (and new cache tables) and the update hook for existing tables.

OK, so if future cache table use (or are supposed to use) the basic cache table schema, then there's no need to redo any of these changes for future updates. Is that right?

It seems like putting the update hook into a new module would be a better way to handle that. There are some alternate caching modules mentioned above, it's over my head to evaluate them though.

Yes, that should be correct. In case the full patch was/is still applied, all existing cache tables should have been updated in the past and new are created by the system_schema() code. So there is no need to run the update hook again. Sites just needs to ensure that the new system_update_7086() from D7.98 have run.

I've moved the code to replace 'expire' indexes with 'expire_create' from the fake hook_update_N function into system_requirements(). In the runtime phase (eg when you load the Status report on your site), it will report a warning that the update script needs to be run if it finds at least one 'expire' index on a cache table. In the update or install phases, if it finds an 'expire' index on a cache table, it will drop it and then create an 'expire_create' index if one does not already exist, reporting all of these actions to the user. So running drush updatedb or the update.php page should clean up the indexes even if there are no update hooks needing to be run.

I reset the system module schema number back to before the first hook from this patch that we applied to our site - 7074 - and reran all of the real system module hooks since then, without much incident. YMMV