Review cache_lifetime behaviour. [#1134242]

I went through the queue the other day and concluded that reports about stale data are primarily issues with css/js aggregation and the page cache.

However thinking about it more, and reviewing the core logic again, I think there is a genuine bug in how we currently handle this.

The main difference between memcache's handling and cores's, is that memcache respects cache_lifetime for both cache_clear_all('*', $bin, TRUE); as well as cache_clear_all();

For core, cache_lifetime only takes effect for cache_clear_all(NULL, $bin); - if you do cache_clear_all('*', $bin, TRUE); it truncates the table altogether.

This means that a full cache flush from the performance screen doesn't do a full clear in memcache, whereas it does in core.

There are three cases we need to account for:

1. cache_clear_all() - this is triggered by content updates, since there are no arguments, we can detect that kind of flush easily.
2. cache_clear_all('*', $bin, TRUE); this is also triggered 'on purpose' - so we can assume it's correct (except for the recursive calls in memcache.inc which this patch tweaks).
3. Calls to cache_clear_all(NULL, $bin) in system_cron() and elsewhere - these are for garbage collection (and core is wrong to do these on cron when it uses CACHE_TEMPORARY for page and block caches), and they should be ignored by memcache which doesn't need garbage collection.

Attached patch does the following:

- adds a new constant MEMCACHE_CONTENT_FLUSH - when cache_clear_all() is called, it converts this to cache_clear_all(MEMCACHE_CONTENT_FLUSH, 'cache_block') and cache_clear_all(MEMCACHE_CONTENT_FLUSH, 'cache_page');

- I found another issue related to this, in that we were expiring entries set to CACHE_PERMANENT in the block or page bins after cache_clear_all(), now we ignore these on MEMCACHE_CONTENT_FLUSH same as core does.

- adds a new variable, variable_get('cache_flush_content_$bin'); when this is set, we check against minimum cache lifetime.

- when cache_clear_all('*', $bin, TRUE); is called, we don't do anything with minimum cache lifetime - this now means every cached item in the bin is invalidated (although stampede protection will still kick in if enabled).

- cache_clear_all(NULL, $bin); called directly remains a no-op - no change from now.

Since the tests here were written to the current functionality, there is one fail if you only apply the patch to memcache.inc - since cache_clear_all('*', $bin, TRUE); now no longer takes into account minimum cache lifetime.

As well as modifying this, I added tests for CACHE_PERMANENT vs. cache_clear_all() - these should fail unless the patch is applied and a couple of others related to this.

Comment	File	Size	Author
#3	lifetime.patch	12.09 KB	catch
	lifetime.patch	11.92 KB	catch

Comments

Comment #1

catch

he/him

English

commented 21 April 2011 at 08:39

Just a note this passes simpletests, but I haven't run the benchmarks yet, will do later.

Comment #2

catch

he/him

English

commented 21 April 2011 at 13:00

Before:

STAT total_connections 11894
STAT cmd_get 120360
STAT cmd_set 57337
STAT get_hits 62915
STAT get_misses 57445
STAT incr_hits 0
STAT bytes_read 67239525
STAT bytes_written 167285060
STAT evictions 5864
STAT total_items 57337

Hit rate: 52.2700%
Miss rate: 47.7200%

HTTP return codes:  20x(11881) 30x(1308) 40x(2)  50x(0)
 200: 11881
 302: 1308

Pages per second: 20.83

After:

STAT total_connections 12133
STAT cmd_get 122737
STAT cmd_set 58178
STAT get_hits 64447
STAT get_misses 58290
STAT incr_hits 0
STAT bytes_read 68368013
STAT bytes_written 170674792
STAT evictions 7189
STAT total_items 58178

Hit rate: 52.5000%
Miss rate: 47.4900%

HTTP return codes:  20x(12120) 30x(1337) 40x(2)  50x(0)
 200: 12120
 302: 1337

Pages per second: 21.26

Benchmarks come out the same, so this isn't affected either way.

Grepping core, there is one place in locale module where it calls cache_clear_all('*', 'cache_page', TRUE); otherwise this only happen in drupal_flush_all_caches(). Scanning contrib modules on a couple of client code bases, some are doing this on their own cache tables or in updates, but it's not common. So I think it makes sense to ignore cache_lifetime in this case and stick closer to the core behaviour.