For more background, see http://www.metaltoad.com/blog/how-drupals-cron-killing-you-your-sleep-si..., which was written for D6, but the same problem exists in D7. Here's a summary:

  • system_cron() calls cache_clear_all(NULL, $table) for a bunch of tables, including 'cache_page'.
  • comment_form_submit(), node_form_submit() and a bunch of other functions call cache_clear_all() with no arguments, which results in cache_clear_all(NULL, 'cache_page') being called.
  • Records in cache_page have the expire field set to CACHE_TEMPORARY, because that's what drupal_page_set_cache() sets it to.
  • The default class that implements the 'cache_page' cache bin is DrupalDatabaseCache, whose clear() method when NULL is passed as $cid deletes all records with an expire of CACHE_TEMPORARY.
  • There is a cache_lifetime system variable, settable on the admin/config/performance page, that can be used to ensure that DrupalDatabaseCache::clear() doesn't run more than once within the corresponding interval, regardless of how often cron runs or website content gets changed.

This setup doesn't make sense to me. If I setup cron.php to run once an hour, why should my page cache be emptied each hour? Maybe I have a site whose content is only changed once per week on average: why should my page cache be emptied on cron at all? And while, yes, I can tune the cache_lifetime variable to be longer than my cron interval, doing so also means that when content is changed (i.e., a node or comment form is submitted), then my page cache really is stale for up to cache_lifetime.

And while, yes, I can override DrupalDatabaseCache with some other class, I'm not sure that helps, since I don't think it would have any way to distinguish whether clear() was called due to a node/comment submit or to a cron run.

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

Related issues:
#1279654: Page cache is CACHE_TEMPORARY and does not honor cache_lifetime

Files: 
CommentFileSizeAuthor
#110 d7-cache_garbage_collection-891600-110.patch10.53 KBbrad.bulger
#99 d7-cache_garbage_collection-891600-99.patch10.65 KBjackbravo
PASSED: [[SimpleTest]]: [MySQL] 39,916 pass(es). View
#88 d7-cache_garbage_collection-891600-88.patch10.65 KBiamEAP
PASSED: [[SimpleTest]]: [MySQL] 39,563 pass(es). View
#85 d7-cache_garbage_collection-891600-85.patch10.7 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/system/system.install. View
#80 d7-cache_garbage_collection-891600-80.patch10.69 KBiamEAP
PASSED: [[SimpleTest]]: [MySQL] 39,168 pass(es). View
#78 d7-cache_garbage_collection-891600-79.patch10.72 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] 39,038 pass(es), 60 fail(s), and 2 exception(s). View
#76 d7-cache_garbage_collection-891600-75.patch5.75 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/system/system.install. View
#73 d7-cache_garbage_collection-891600-73.patch5.75 KBiamEAP
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-cache_garbage_collection-891600-73.patch. Unable to apply patch. See the log in the details link for more information. View
#65 891600-65.patch9.53 KBno_commit_credit
PASSED: [[SimpleTest]]: [MySQL] 35,371 pass(es). View
#65 interdiff.txt2.88 KBno_commit_credit
#64 cache_garbage_collection.patch9.52 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,278 pass(es). View
#63 cache_garbage_collection.patch8.53 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,218 pass(es). View
#60 cache_garbage_collection.patch9.57 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,219 pass(es). View
#56 cache_garbage_collection.patch9.79 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,701 pass(es). View
#54 test_only.patch1.91 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 35,680 pass(es), 1 fail(s), and 0 exception(s). View
#54 cache_garbage_collection.patch7.34 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,673 pass(es). View
#52 test_only.patch2.33 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#52 cache_garbage_collection.patch7.76 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#51 cache_garbage_collection.patch5.27 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es). View
#49 cache_garbage_collection.patch5.26 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#48 core-page_cache_lifetime-891600-48.patch5.15 KBiamEAP
PASSED: [[SimpleTest]]: [MySQL] 35,259 pass(es). View
#44 core-page_cache_lifetime-891600-44.patch500 bytesiamEAP
PASSED: [[SimpleTest]]: [MySQL] 35,144 pass(es). View
#40 lifetime.patch5.05 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch lifetime.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#29 lifetime.patch4.99 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 33,309 pass(es). View
#28 garbage.patch5.11 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 32,884 pass(es). View
#26 garbage.patch5.11 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/cache.inc. View
#24 garbage.patch2.8 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 32,883 pass(es). View
#23 garbage.patch2.78 KBcatch
FAILED: [[SimpleTest]]: [MySQL] 32,825 pass(es), 1 fail(s), and 0 exception(es). View
#9 cache-page-temporary-permanent-891600-9.patch613 bytesIan Ward
FAILED: [[SimpleTest]]: [MySQL] 25,703 pass(es), 3 fail(s), and 0 exception(es). View

Comments

effulgentsia’s picture

As an aside, someone mentioned the 'page_cache_maximum_age' system variable that we now have in D7, but from what I can tell, it doesn't affect this issue, since all it does is control external proxy caches, and not Drupal's cache_page table/bin.

Damien Tournoud’s picture

Title: system_cron() empties the page cache: why? » Page and blocks are cached with CACHE_TEMPORARY

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.

effulgentsia’s picture

Title: Page and blocks are cached with CACHE_TEMPORARY » Page and block caches are cleared the same way on cron as on content change

Thanks Damien. If CACHE_TEMPORARY is incorrect for page and block caches, then cache_clear_all() may need to be changed or else node_form_submit(), content_form_submit(), and friends may need to change how they call it. In any case, retitling issue to get at the heart of the problem from the perspective of a site owner.

catch’s picture

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.

effulgentsia’s picture

Does this mean if page_cache_maximum_age is 2 weeks, and node_form_submit() is called 1 week after a page is cached, then the node_form_submit() will fail to clear that page from the cache? Do we need to change node_form_submit(), comment_form_submit(), and friends to cache_clear_all() with $wildcard=TRUE? Sorry, I don't know much of the history and thinking that's gone into our page and block caching systems. My very naive assumption is that content changes should fully clear page and block caches regardless of their expire field. And I'm not clear on what purpose page_cache_maximum_age and (a hypothetical) block_cache_maximum_age serve. Why would you not want those to be very large, as long as you're emptying those caches when content changes invalidate them?

Jerome F’s picture

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.

David_Rothstein’s picture

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.)

catch’s picture

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.

Ian Ward’s picture

Status: Active » Needs review
FileSize
613 bytes
FAILED: [[SimpleTest]]: [MySQL] 25,703 pass(es), 3 fail(s), and 0 exception(es). View

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

Status: Needs review » Needs work

The last submitted patch, cache-page-temporary-permanent-891600-9.patch, failed testing.

David_Rothstein’s picture

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.)

catch’s picture

Component: cron system » base system

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.

Damien Tournoud’s picture

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).

ohnobinki’s picture

+

Agileware’s picture

Subscribing

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7

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.

catch’s picture

Component: base system » cache system
Fabianx’s picture

Subscribe

pillarsdotnet’s picture

catch’s picture

Priority: Normal » Major

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.

jamiecuthill’s picture

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?

RichardLynch’s picture

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" :-) :-) :-)

catch’s picture

Status: Needs work » Needs review
FileSize
2.78 KB
FAILED: [[SimpleTest]]: [MySQL] 32,825 pass(es), 1 fail(s), and 0 exception(es). View

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.

catch’s picture

FileSize
2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 32,883 pass(es). View

Use 'cron' as the flood identifier.

BTMash’s picture

Status: Needs review » Needs work

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.

catch’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/cache.inc. View

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.

Status: Needs review » Needs work

The last submitted patch, garbage.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
PASSED: [[SimpleTest]]: [MySQL] 32,884 pass(es). View

Well apart from that.

catch’s picture

FileSize
4.99 KB
PASSED: [[SimpleTest]]: [MySQL] 33,309 pass(es). View

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.

BTMash’s picture

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.

catch’s picture

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.

catch’s picture

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.

RichardLynch’s picture

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.

catch’s picture

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

BTMash’s picture

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.

RichardLynch’s picture

@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.

RichardLynch’s picture

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.

Fabianx’s picture

@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.

pillarsdotnet’s picture

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

catch’s picture

FileSize
5.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch lifetime.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Re-rolled for /core.

catch’s picture

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

Status: Needs review » Needs work
Issue tags: +Performance, +needs backport to D7

The last submitted patch, lifetime.patch, failed testing.

iamEAP’s picture

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

Where does this issue stand?

iamEAP’s picture

Status: Needs work » Needs review
FileSize
500 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,144 pass(es). View

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.

catch’s picture

Status: Needs review » Needs work

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.

iamEAP’s picture

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).

catch’s picture

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.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
PASSED: [[SimpleTest]]: [MySQL] 35,259 pass(es). View
catch’s picture

FileSize
5.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, cache_garbage_collection.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
PASSED: [[SimpleTest]]: [MySQL] 35,683 pass(es). View

This should fix the install failure.

catch’s picture

FileSize
7.76 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
2.33 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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.

Status: Needs review » Needs work

The last submitted patch, cache_garbage_collection.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
7.34 KB
PASSED: [[SimpleTest]]: [MySQL] 35,673 pass(es). View
1.91 KB
FAILED: [[SimpleTest]]: [MySQL] 35,680 pass(es), 1 fail(s), and 0 exception(s). View

.htaccess cruft.

catch’s picture

catch’s picture

FileSize
9.79 KB
PASSED: [[SimpleTest]]: [MySQL] 35,701 pass(es). View

With the attachment.

Berdir’s picture

Status: Needs review » Needs work

(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?

iamEAP’s picture

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?

David_Rothstein’s picture

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).

catch’s picture

Status: Needs work » Needs review
FileSize
9.57 KB
PASSED: [[SimpleTest]]: [MySQL] 35,219 pass(es). View

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.

catch’s picture

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

olli’s picture

Status: Needs review » Needs work
+++ 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.

catch’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [MySQL] 35,218 pass(es). View

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.

catch’s picture

FileSize
9.52 KB
PASSED: [[SimpleTest]]: [MySQL] 35,278 pass(es). View

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.

no_commit_credit’s picture

FileSize
2.88 KB
9.53 KB
PASSED: [[SimpleTest]]: [MySQL] 35,371 pass(es). View

Comment nitpicks.

olli’s picture

Status: Needs review » Needs work
+++ 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?

catch’s picture

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.

olli’s picture

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.

sun’s picture

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.

olli’s picture

+++ 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?

pillarsdotnet’s picture

iamEAP’s picture

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?

iamEAP’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-cache_garbage_collection-891600-73.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, d7-cache_garbage_collection-891600-73.patch, failed testing.

catch’s picture

@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.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/system/system.install. View

Same...

Edit: Good to know. Thanks, catch.

Edit again: missing semi-colon...

The last submitted patch, d7-cache_garbage_collection-891600-75.patch, failed testing.

iamEAP’s picture

FileSize
10.72 KB
FAILED: [[SimpleTest]]: [MySQL] 39,038 pass(es), 60 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, d7-cache_garbage_collection-891600-79.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
10.69 KB
PASSED: [[SimpleTest]]: [MySQL] 39,168 pass(es). View

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

Anonymous’s picture

Just a quick question. Will this patch be implemented in the next Drupal 7 release?

joachim’s picture

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

iamEAP’s picture

Status: Needs review » Needs work

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

catch’s picture

+ $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.
     }
  }
}
iamEAP’s picture

Status: Needs work » Needs review
FileSize
10.7 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/system/system.install. View

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.

Status: Needs review » Needs work

The last submitted patch, d7-cache_garbage_collection-891600-85.patch, failed testing.

catch’s picture

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.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
10.65 KB
PASSED: [[SimpleTest]]: [MySQL] 39,563 pass(es). View

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

iamEAP’s picture

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.

smitty’s picture

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?

catch’s picture

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).

smitty’s picture

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.

iamEAP’s picture

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.

catch’s picture

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.

Fidelix’s picture

Works for me, although it did not apply cleanly.

iamEAP’s picture

catch’s picture

Priority: Major » Normal

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.

mgifford’s picture

jackbravo’s picture

FileSize
10.65 KB
PASSED: [[SimpleTest]]: [MySQL] 39,916 pass(es). View

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

jackbravo’s picture

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

jackbravo’s picture

jackbravo’s picture

Status: Needs review » Reviewed & tested by the community

marking as RTBC.

lotyrin’s picture

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?

lotyrin’s picture

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

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

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?

caktux’s picture

Is this still an issue in D8?

catch’s picture

catch’s picture

Issue summary: View changes

Updated issue summary.

deanflory’s picture

So, what is the current recommended approach for D7?

Fabianx’s picture

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.

brad.bulger’s picture

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