split up the cache table

I think the logic behind this patch is quite sound, and the benefits might be significant. Good idea.

I would be OK with $domain being the 2nd param in cache_get() and defaulting it to 'default'. That way, you don't have to patch in a few places. No biggie.

This is very sound, later on we could do some magick so that we can memcached per domain. I run memcached for sessions, data cache, menu cache and url aliases, so i think this avenue worth pursuing.

Back to this patch here, I can't agree more.

This is one of the patches that need a fair amount of testing on busy sites. We need numbers so let's see if we can test it on drupal.org. If you run a busy Drupal site, please help test this patch. If there is no notable performance improvement, the patch probably isn't worth keeping. Point is, we need more than one website to evaluate this.

We'll want to collect information about MySQL's cache miss ratio. If you want to help, here is how you can compute your site's cache miss ratio. (There might be better ways to analyse this.)

mysql> show status like 'qc%';
+-------------------------+-----------+
| Variable_name           | Value     |
+-------------------------+-----------+
| Qcache_free_blocks      | 46898     |
| Qcache_free_memory      | 153045752 |
| Qcache_hits             | 599758178 |
| Qcache_inserts          | 168641389 |
| Qcache_lowmem_prunes    | 16025991  |
| Qcache_not_cached       | 2247680   |
| Qcache_queries_in_cache | 90020     |
| Qcache_total_blocks     | 228850    |
+-------------------------+-----------+

These are the current drupal.org cache statistics (not using Gerhard's patch). I'd think that 'Qcache_inserts' equals the absolute number of cache misses; after all, every cache miss results in the entry being inserted in the cache. Looking at these numbers, Drupal.org has a cache miss ratio of 21% (and a cache hit rate of 79%).

cache miss ratio = cache misses / total number of cache accesses 
                = Qcache_inserts / (Qcache_inserts + Qcache_hits)
                = 168641389 / (168641389 + 599758178)
                = .21

Of all the missers, 'Qcache_lowmem_prunes' seems to denote the 'capacitcy cache missers': 16025991 / (16025991 + 168641389) = .08. That is, 8% of all the misses are the result of the cache being too small. 92% of the cache misses are due to the queries not being in the cache (because the cache got flushed -- partially of in full).

Clearly, Gerhard's patch aims to make partial cache flushes 'smaller' (reduce the number of cache entries that get evicted), so if that 92% goes down, the total cache miss rate should go down as well, and performance should increase. At least, that is the theory, and ultimately, what we'd like to see confirmed.

Anyway, as for the patch itself, I'd document the rationale and workings in the code comments. Without the proper MySQL cache background, this code is plain weird. After all, we're working around a MySQL /limitation/ (i.e. MySQL's aggressive/conservative cache flusing behavior). :)

Hoping this gets some people up to speed with this patch.

please have a look at the pain people are experiencing at http://drupal.org/node/73604. I think the patch here can help them. Specifically, once we split out the filter and menu tables, we focus cache_clear_all() for thesze cases to perform a TRUNCATE TABLE instead of DELETE FROM which will be much easier on the DB.

I'm currently experiencing a problem as described here: http://drupal.org/node/74136

Do you think this patch could solve the issue?

Ooooh, I really like the sound of this one!

This would be a good change, and having the $domain present would make batch deletion from the filecache more efficient. It would also make implementing the object_cache much easier. (See http://drupal.org/node/74020)

One dumb question: why name the variable $domain? I find that variable name confusing. Wouldn't $type or $table be preferable?

One suggestion: For object caching (or other cache extensions), we would need an easy method for expanding the allowable $domain variables. Currently, I would need to patch this code by adding more switch statements to cache_get_table().

For people looking to test this:

http://cvs.drupal.org/viewcvs/drupal/contributions/modules/devel/generat...

Add pages.

Enable caching on your site. Then crawl the site to fill up the cache: wget -r --delete-after --cookies=off http://example.com

insert into cache (cid, data) select concat('filter:1:', MD5(RAND())), data from cache where cid like 'filter:';

I'll test more and report back.

@killes - are you planning on collecting measurements for this. the separate tables will make flushing the cache much faster. see #8

Found some evidence that this patch is a good thing, read here:

http://lists.drupal.org/archives/development/2006-08/msg00091.html

minor nit-pick:

Your suggested documentation for cache_get(), etc says:

 * @param $domain
 *   The cache table to store the data in. Valid values are 'filter',
 *   'locale', 'menu', 'page'.

However, your retrival of the cached variables uses:

cache_get('default', 'variables')

and the actual work is done by the function below, which sends everything other then the above 4 to the "default" cache. So, why not suggest/use NULL or '' as the first parameter to cache_get? The documentation for cach_get(), etc. needs to specify the case @param = none of these.

function cache_get_table($domain = '') {
  switch ($domain) {
    case 'filter':
    case 'menu':
    case 'page':
      $table = 'cache_'. $domain;
      break;
    default:
      $table = 'cache';
      break;
  }
  return $table;
}

i agree. this is not wishful thinking. the period after the patch demonstrates an average low that is unmatched in the lifetime of the chart.

as above, documentation is missing for: @param $domain == '', 'default', etc.

I agree this is a must. Hence the priority change.

A small code change, though is needed.

static $map;
if (!isset($map)) {
  $map = variable_get('cache_tables', array('filter' => 'cache_filter'....
}
return isset($map[foo]) ? $map[foo] : 'cache';

this lets you add cache tables. Maybe CCK or views wants its own cache table. Or another huge module will arise after we release... This is less code than a switch and faster...

modified patch as per chx's suggestion, tried to clean up Doxygen comments and caught a couple stray pre-patch uses of cache_set/get/clear_all

Hmmm, still needs work. The menu caching isn't working right. The cache doesn't seem to get clear appropriately. For example, i enabled the locale module, and the menu items didn't appear and weren't accessible.

Also, with this patch, can/should cache_clear_all() be modified to use TRUNCATE for totally clearing tables?

for some reason, the this doesn't seem to work: cache_clear_all('menu', '', TRUE);

It does seem to work in this form: cache_clear_all('menu', ':', TRUE);, since all the cache IDs include a colon.

Also, extended cache_clear_all() so that a call with no parameters clears expired/temporary entries from all the predefined tables.

PS, also changed the system update # istarting above since someone else had committed a duplicated.

looks like the update should really be numbered 1006. see: http://drupal.org/node/64212#comment-122346

apologies for cross posting. It sounds like this would help us out a lot if we're having the same issue as described here: http://drupal.org/node/73604 (my original forum post is here: http://drupal.org/node/78424)

We have a busy site (c.1.5 million pages/month although we've had a lot of downtime the past three months so probably more like 800k ish atm), and one that's experiencing '000s of 404s per day due to our being in the early stages of moving everything to drupal. Our drupal install has c.8000 nodes and c.100,000 comments. Cache table generates a lot of overhead and needs to be repaired and optimised every few hours before everything crashes.

Would be happy to help test the patch against 4.7.3 but won't be able to look at it until Sunday. I'd also be happy to give one of the developers working on this root access to the machine in order to try it out yourselves if you're so inclined - you'd get much better feedback that way since I'm very much a beginner at server administration. Will give it ago myself either way though when I get a chance and post up the results.

@killes - is this a 'feature' or a 'bug'.

Attached patch is updated to current HEAD (minimally tested, however). Also added check so that if $wildcard == TRUE and $cid == '*', the effect is to simply delete everything in the table, rather than doing the string match.

above you say:

cache_clear_all is now an alias to cache_clear_all('*', 'cache_page', TRUE);

Is that really the behavior you want? This would seem to mean that the code would no longer respect/use the minimum cache lifetime, right? Wouldn't you want cache_clear_all(NULL, 'cache_page')?

Reviewed the code, looks very good to me. Nothing stands out as needing anymore work. I'd say this is RTBC but perhaps another review is needed :-)

Latest patch looks a lot cleaner to me!

+ * - such smaller tables can be held in RAM by the SQL server

I wonder if that is true.

It would be good if someone could proof-read the phpDoc and double check whether the API is sound. Otherwise looks ready to be committed.

+1
Looks good. I love how this is setup allowing contrib to add cache tables and use the api. The docs seemed pretty clear to me. The only odd thing was in cache_clear_all the table arguement is mandatory but it says "if set".

Committed to CVS HEAD! Thanks.