Temporary cache table entries are not flushed

glad to know my cache_form growing to 4.4GB and cache_menu to 1.5GB helped find this bug :)

This is pretty important. Most of the caching code in FAPI3 was written before the addition of the AHAH code meant that forms would be thrashed a lot more -- the result is a much large number of cache entries. Even a small site with heavy use of an AHAH form (like, say, dragging around blocks a lot) could easily bloat up past the db size limits of many shared hosting accounts.

We're timestamping the entries, so calling cache_clear_all() on the form cache liberally will never hurt.

It's not as critical for the other tables that are hit here (page, block, etc) but the menu and form caches definitely hilight our need to regularly clear out those tables. We have the expire column, we need to use it.

Running this code in my own hook_cron implementation on my live site. seems to be working.

cache_form is well trimmed down to size (6MB as apposed to 4.4GB!), but cache_menu is 8889 odd items with expires = 0. My guess though is that this is just because I have loads of menu items (working off wild cards) and hence this is to be expected. Its 400MB in size, much better than the 1.5GB before :)

The tables end up with lots of MySQL overhead. Would it be worthwhile adding into hook_cron or elsewhere some optimize table statements? Or suggesting it in a readme somewhere perhaps?

Looks good, but this also seems like highly dependent on the intervals of cron runs, right?

Looks good, but this also seems like highly dependent on the intervals of cron runs, right?

Yes. That's one of my concerns as well, but I'm not sure that there's a good way around that. We already warn users if they don't run cron regularly, and there are contrib options like poormanscron.

I'm not sure about the menu system, but FormAPI's only other real option is to call cache_clear_all(NULL, 'cache_form') in a central function like drupal_get_form(), so that it's constantly cleaning up after itself.

I just ran into and filed a separate issue related to cache_clear_all only working for the first table if you have a cache_lifetime variable/value setup.

It would be nice if this used the hook that registers cache tables, as I really don't like to see that hardcoding of cache tables there.

Unfortunately, I just realized that is a devel.module hook. :/

So that means modules still have to handle their own flushing, making this a good patch for core but it's not very Drupally since it's a closed API.

This still looks like pretty too broad to me. update module implements hook_flush_caches() for example in core. Now if we add this, all cron runs will flush all update module data, which will make update module be without any data before the next cron, as far as update_cron() looks like being implemented.

/**
 * Implementation of hook_flush_caches().
 *
 * The function update.php (among others) calls this hook to flush the caches.
 * Since we're running update.php, we are likely to install a new version of
 * something, in which case, we want to check for available update data again.
 */
function update_flush_caches() {
  return array('cache_update');
}

This is just one core example of how hook_flush_caches() is supposed to work and not being called on all cron runs.

The note from drupal_flush_all_caches() also makes me wonder:

  // Don't clear cache_form - in-progress form submissions may break.
  // Ordered so clearing the page cache will always be the last action.
  $core = array('cache', 'cache_block', 'cache_filter', 'cache_page');

The proposed patch would break all forms being under submission when the cron run, right?

Eg. on Drupal.hu, we run cron every 5 minutes, which is very good for timed publication, and other timely operations. Breaking some user forms every 5 minutes and dropping away update data five minutes after update module obtained it does not sound like that attractive.

Am I missing or misunderstanding something here?

@merlinofchaos: hook_flush_caches() is that method provided by core, so this is not a closed API. And it is also what makes this a wide solution which needs more investigation.

Ahh, that hook appears to be undocumented at this time.

Breaking some user forms every 5 minutes and dropping away update data five minutes after update module obtained it does not sound like that attractive.

We set the expire column on form cache data, so forms in progress won't be broken unless the user has had them open and unsubmitted for over a day. Unless I'm missing something?

@Eaton, I checked cache_clear_all, and from the looks of the code, you seem to be right. That leaves the catch-all cache flush problem.

Yeah, i agree that is still worrisome. This is a fine solution for the form cache specifically but I'm not familiar enough with the nuances of the other caches to say that it wouldn't cause problems there.

chx: I provided an explanation above which Eaton understood and shared my concern. Maybe it was not detailed enough for you? Interesting that this makes this won't fix for you.

Anyway, since it looks like you set the status as if it was an indication of whether you are looking at this or not, let's allow others who have the time to follow the discussion and help work out a non-controversial solution to help get this in to Drupal 6.2.

The concern still being open is up above here in #10, and ends in "not being called on all cron runs."

chx: I think I have a valid concern, and Eaton shares it as well. The fact you have no interest in looking into it might make this a "black whole" for you, that's unfortunate, but you are not expected to deal with all issues passing your way, so such is life...

After getting weird errors when running this patch, I found that cache_clear_all has a serious bug in it:

db_query("DELETE FROM {". $table ."} WHERE expire != %d AND expire < %d", CACHE_PERMANENT, time());

... expire < time()? Nooooo... expire + created < time() is what it should be

expire is just e.g. 86400, i.e. 24 hours, rather than the whole timestamp.

patches for this against HEAD and DRUPAL-6 created in another thread I just opened: http://drupal.org/node/230374

EDIT: note that the above only applies to FAPI form_cache which was setting the expire column wrongly to seconds since created, whilst all other cache_ tables where using a complete timestamp

see also http://drupal.org/node/231587 which deals specifically with cache_menu which seems to never set expiry to anything other than 0, and which seems to have particularly fat blobs for the data fields

Then we need pre-requisite patches submitted before this patch lands to avoid breaking Drupal 6.

Hm, we went back to my first concerns at http://drupal.org/node/226728#comment-748819 with chx today, and discussed and reviewed the code being in question. Looks like this patch is not in error indeed, and in fact only removes expired entries (unlike the code comment suggests, which was written for one use case of hook_flush_caches()). Will test later on drupal.hu and check back with the results.

Bah, sorry for not getting back to here sooner. I just installed this patch on Drupal.hu today and watch how it goes. Tentatively marking RTBC, as there were no issues so far.

Found no problem in my testing, so committed. Thanks. Moving to 7.x.

I don't think will actually affect 6.x cache_menu (since I think the entries are all CACHE_PERMANENT), but this fix should: http://drupal.org/node/231587

This never got synced with D7 and still applies with offset.

The test failure doesn't get fixed by this patch, but I think[#276267] #277448: Cache system isn't doing CACHE_TEMPORARY like documented are pretty closely related to it - the cache.inc test was written without this bug in mind afaik.

chx's patch in #2 still applied to CVS HEAD.

Ok, I committed #2 so that at least 6.x and 7.x are on equal footing.

However, this patch still needs work:

a) drupal_flush_all_caches(), which has basically this identical code, does NOT in fact flush all caches; it needs the cache_form entry in its $core array.
b) system_cron() has an extra entry for cache_menu which it doesn't need; cache_menu is permanent storage.
c) In 7.x, I'd like to see an API function for this logic so that it can be called from both system_cron() and drupal_flush_all_caches().

Maybe cache_flush bug #227228: cache_clear_all and cache_get fail to clear caches when Minimum cache lifetime is on will be fixed too?

I have the same problem with Drupal 6.9. The cache_form table grew without limit. The cache was never truncated.
After a lot of debugging i discovered that the problem only occurs, when cache_lifetime is bigger then 0. Changing the Minimum cache lifetime to none solved the problem.

The reason lies in cache.inc, lines 152-162

      $cache_flush = variable_get('cache_flush', 0);
      if ($cache_flush == 0) {
        // This is the first request to clear the cache, start a timer.
        variable_set('cache_flush', time());
      }
      else if (time() > ($cache_flush + variable_get('cache_lifetime', 0))) {
        // Clear the cache for everyone, cache_flush_delay seconds have
        // passed since the first request to clear the cache.
        db_query("DELETE FROM {". $table ."} WHERE expire != %d AND expire < %d", CACHE_PERMANENT, time());
        variable_set('cache_flush', 0);
      }

The code always alters the variable cache_flush between 0 and the actual time, thus never reaching the delete statement in l. 160.
I don't have an idea how to solve the problem, but it must be there.

don't change version

#227228: cache_clear_all and cache_get fail to clear caches when Minimum cache lifetime is on is landed for 6 and 7

Note that in 7.x currently, block, field and image modules implement http://api.drupal.org/api/function/hook_flush_caches/7 and return the respective cache table name. In both 6.x and 7.x, update module implements update_flush_caches() which calls its own private cache flushing routine and returns an empty array, assuming that hook_flush_caches() will be invoked on each cron run, which it has since 6.2.

@33 c) "In 7.x, I'd like to see an API function for this logic so that it can be called from both system_cron() and drupal_flush_all_caches().", note that system_cron() doesn't do a complete "flush" of the caches, it just removes expired and temporary entries (subject to the cache_lifetime variable). drupal_flush_all_caches() *does* completely empty the tables though, which is why it doesn't touch cache_form. An API function would need to distinguish between a partial and a complete flush. Maybe hook_flush_caches also needs to make this distinction?

@33 b), In 7.x, system_cron() no longer needs to handle cache_block since block.module now does that as already noted.

Perhaps the remaining tables listed in system_cron() should be returned by a new system_flush_caches() rather than being coded directly into system_cron()? (And then if hook_flush_caches() had byref argument $tables - rather than returning them - it would be easier to manipulate the behavior than at present...)

Note that in 5.x there is no regular cache flushing of this sort, and my 5.x sites with only occasional content updates fly along for anonymous visitors since cache_page and cache_filter fill up with loads of "expired" or temporary entries (anon visitors are probably 95% of the total)... though this doesn't causes stale content to be seen. But a 6.x site is forever emptying its page cache and correspondingly much slower (i.e. page caching does much less for site performance/responsiveness than in 5.x). So I'd actually prefer the page cache not to be cleared on each cron ... but this will vary from site to site.

Setting an expiration time on entries in cache_page does not seem to be an option either - updating a page or a block essentially invalidates the entire page cache .. which is presumably why CACHE_TEMPORARY is used at present (note the only uses of timestamps in the expire column in core that I am aware of at present are cache_form: 6 hours from creation, which seems reasonable, cache_filter: 24 hours from creation, which seems arbitrary though not entirely unreasonable (see also #198855: check_markup() hardcodes cache expiration, which is a bit off the mark I think), and cache_update: between 1 hour and 1 week depending on various things).

See also #256416: Editing a node does not wipe its cache entry
#409682: Improve page caching system

Final note: The initial motivation for this patch was ever-growing cache_menu and cache_form. Although this patch did not actually affect cache_menu, the problem with cache_form was addressed by this patch, though it wasn't finally fixed until #227228: cache_clear_all and cache_get fail to clear caches when Minimum cache lifetime is on got in. IMO the jury is still out on the desirability of *always* flushing *all* the core cache tables on every cron run. For some sites e.g. those that display time-dependent info (e.g. they use forum.module or date.module) it probably makes sense; for busy sites that have frequent content updates and use cache_lifetime > 0 it possibly makes little difference (e.g. on d.o there are always pages/comments being added/edited so the page/block caches will always be being cleared anyway); for others it does not seem so ideal.

[update: minor clarifications to this rambling comment 29 Sep 09]

Instead of hard coding a list of core cache tables, we should probably add an optional item to table definition in schema api where a table can say that it is a cache table.

@moshe Very good idea! most of cache tales are defined as

  $schema['cache_block'] = drupal_get_schema_unprocessed('system', 'cache');
  $schema['cache_block']['description'] = 'Cache table for the Block module to store already built blocks, identified by module, delta, and various contexts which may change the block, such as theme, locale, and caching mode defined for the block.';

So just

- $core = array('cache', 'cache_filter', 'cache_bootstrap', 'cache_page');
+ $core = drupal_get_schema(); 
+ foreach ($core as $table => $info...

But this brings a little performance loss

From webchick's list:

a) drupal_flush_all_caches(), which has basically this identical code, does NOT in fact flush all caches; it needs the cache_form entry in its $core array.

There's now an explicit comment in drupal_flush_all_caches() not to include cache_form() because that will wipe in-progress form submissions, let's leave that as it is.

b) system_cron() has an extra entry for cache_menu which it doesn't need; cache_menu is permanent storage.

Removed in this patch.

c) In 7.x, I'd like to see an API function for this logic so that it can be called from both system_cron() and drupal_flush_all_caches().

See you in #636454: Cache tag support for D8 I hope.

This has been fixed in D7 already, moving to D8.

Closing this then, let's start fresh or with existing cleanup issues in D8.