Revert cache_menu patch removal, add a $conf setting instead (was: cache_menu: huge table size)

Subs

I have debugged this in D7, not sure if the same is true for D6. I have observed the same problem on a recently launched site with a small amount of data. After a while the cache_menu table was as big as the rest of the database. The majority of the cache entries are menu items (cid starts with menu_item:). The hash value in the cid is calculated from the normalized request path given to Drupal which is available in $_GET['q']. The menu system has an interesting feature:

When responding to a page request, the menu system looks to see if the path requested by the browser is registered as a menu item with a callback. If not, the system searches up the menu tree for the most complete match with a callback it can find. If the path a/b/i is requested in the tree above, the callback for a/b would be used.

api.drupal.org

This can lead to multiple cache_menu entries for the same menu_item: By appending a slash followed by a random string to the path of an existing page when making a request a different hash is produced.

Some more people should look at this issue, because I think it opens a possibility to attack a Drupal site. So I set the priority to major.

@Stefan, thanks for the steps to reproduce. This is will be #643984: Cache menu_get_item() most likely. Patch should be easy enough to revert locally to test if that helps.

Any progress on this item? :-)
seems to be under different issues but no final solution so far?
caching is great, but not if it fills the database with rubish in a couple of days.
Great to see that so many people are working on fixing this!
Keep up the good spirit!

It really is sad here that there is no way to say: cache_get_fast() because then one could just traverse the anchestors and return the first match, which is costly for DB cache, but really fast for memcache, APC, etc. and even better with a cache_multiple_get_fast.

And saved would be the actual router path (or the hash thereof), which takes care of the problem with the many entries.

That would be the proper solution here to still avoid DB queries alltogether.

Coming back after end of year vacation, the table has grown to 30 Mb. The database total size got to 102% of allowed space on my hosting which lead to buggy display of the site and warnings.

Please note that this site is (hopefully) not in production, but the table still grew up rapidly, even with very little traffic.

This is the second time it happens in few weeks. Our hosting allows 50Mb for database and the database size with Backup & Migrate module is less than 1Mb (gzipped).
This is a huge difference.

I'm using Drupal 7.9

#9 looks good to me.

Tagging.

Looks good for me, too, thanks! Would make sense to get this into the Feb 1st release.

Whoopsie. :P Nice catch.

I know catch normally tries to keep a 3-day minimum RTBC time, but that will of course blow the Feb 1 release deadline. I talked to him about it, and he pointed out that since this a straight rollback of another patch, it should leave us no worse off than core already was before.

Therefore, committed and pushed to 8.x. I tried to also commit/push to 7.x, but the patch did not apply. :( If we can get a re-roll in relatively short order, I might be able to squeeze it in tomorrow morning.

Moving back to 7.x since this is already in 8.x.

Sorry.

Only backport from #9 to D7

#18: 1234830-18.patch queued for re-testing.

Committed and pushed to 7.x. Thanks!

I applied #18 patch, but table cache_menu still grows. Is there anything else I need to do to fix table? I have menu with 1500 items, so cache_menu reaches 500Mb in 1 hour!

It seems I forgot to reopen issue. Table cache_menu still grows after patch #18.

Since this particular bug has been fixed, please open a new issue if you're still having problems, since it will be a different cause to the one dealt with here.

OK I see what happened, in between the original report and the bug being fixed, I'd missed that you're still having an issue in Drupal 6, since that's what the report was for it's OK to re-open this issue to keep looking at it, I just generally try to avoid re-opening issues at all cost since they get very long and unwieldy.

I think what might be useful would be if you could attach the full results of SELECT cid, LENGTH(data) as a .txt file to this issue.

Additionally an example of the identical cached data would be handy - since it's that bug that was originally fixed back in 2008.

schildi, thanks for hint :)

Just for record I repeat:
I'm using drupal 7.12 (so I changed issue version).
I applied patch #18 but table still grows.

I have big menu created by taxonomy_menu module from vocabulary with ~500 terms.
Site: http://nmkt.ru

Am I attached right files?

UPDATE: I've wrote next comment with some details and new files.

I figured that taxonomy_menu uses term description for title attribute in menu link.
And I have big descriptions for every term (~500 items).
So I disabled this behavior and reuploaded files. I think table will grow slower, but still be.

Every record in cache_menu has 'expire' field set to zero. Is it correct?

OK looking at these, the problem seems to mainly be that the indiividual cache entry for each menu tree is huge, rather than we're necessarily creating that many entries. i.e. if you have cache items that are 1mb+, then it doesn't take many of these to end up with a 50mb+ cache table.

Really what we need to move towards doing is caching the rendered HTML for menus rather than the links arrays, but that's a very large problem to solve due to access checks, I'm not sure how fixable this is going to be in Drupal 7 let alone Drupal 6, but if the entries are that big then it's worth looking at.

cache_menu items are supposed to be kept permanently (which in practice means until the next menu link is saved or the menu router gets rebuilt). We might want to consider setting 'permanent' to mean a configurable value defaulting to a month or so for the database backend, so that stale items can be removed on sites that aren't otherwise triggering cache clears.

Back to 8.x

This only appears to be affecting people with very, very large menu trees, and there's not a lot that can be done about that, so I'm going to downgrade from major.

This seems worth calling out in the release notes for the patch that was committed.

Linkify related issues.

Could one (or a combination) of the following modules fix this for D7?

• Alternative Database Cache,
• Alternative PHP Cache,
• Boost,
• Memcache API

Or is that a different matter?

May want to repurpose this slightly now as cache_menu doesn't exist anymore in D8. See #1194136: Re-organise core cache bins.

Should this just be bumped back to d7 then?

@knalstaaf not really for the memcacheAPI that's actually what brought me here. There is a set limit of 1MB default in memcache which causes fatals.

Well, it is certainly possible to fix this in D6 to D8, but you need a different cache paradigm:

A LRU (least-recently-used) cache.

That means your most frequent pages like the homepage would be cached, but depending on how many entries you have other pages would quickly vanish from the cache.

There is already an issue by @alexpott to convert CacheArray to LRUCacheArray, so if someone really wants to see this:

It is easily possible, but it needs work.

We saw a site almost go down due to this query (query cache lock) during this voting week, therefore we need to revert the revert and instead just add a little $conf setting so that high traffic sites using memcache (that use LRU by default) can still opt-in to use this functionality.

It is impossible to fix this without patching core.

In the case Fabianx mentioned, the DB was a slave dedicated pretty much entirely to this and a couple of other menu queries, and serving 25K read (SELECT) queries / second according to New Relic...

This should be the patch we used for D6. Based on what nnewton said was a patch from Catch.

{cache_menu} doesn't exist in Drupal 7, but a version of this query does.

Opened #2480811: Cache incoming path processing and route matching for 8.0.x since the code is very different from 7.x now.

Moving back to 7.

Here it is for D7 (reverts #18).

Ignore the previous patch, don't know what I was thinking, let's try it again.

+++ b/includes/menu.inc
@@ -462,9 +462,18 @@ function menu_get_item($path = NULL, $router_item = NULL) {
+    if ($cached = cache_get($cid, 'cache_menu')) {

Now make the cid creation part optional and disabled as before, like:

$cid = variable_get('use_menu_router_cache_for_lru_backend', FALSE) ? 'menu_item:' . hash('sha256', $path) : FALSE;
if ($cid && $cached = ...) {

and it works out. Just need to find a good variable name.

OK, used the variable name you suggested for now.

+++ b/includes/menu.inc
@@ -462,9 +462,18 @@ function menu_get_item($path = NULL, $router_item = NULL) {
+      cache_set($cid, $router_item, 'cache_menu');

if ($cid) {
  cache_set(...);
}

Need this, too ...

Duh, sorry bout that.

1. +++ b/includes/menu.inc
   @@ -309,7 +309,7 @@ define('MENU_PREFERRED_LINK', '1cf698d64d1aa4b83907cf6ed55db3a7f8e92c91');
   - *   An array of path parts; for the above example, 
   + *   An array of path parts; for the above example,
   

   What is changed here? White space?

2. +++ b/includes/menu.inc
   @@ -462,9 +462,20 @@ function menu_get_item($path = NULL, $router_item = NULL) {
   +    $cid = variable_get('use_menu_router_cache_for_lru_backend', FALSE) ? 'menu_item:' . hash('sha256', $path) : FALSE;
   

   I think its better to use () around the 'menu_item' . hash(...) as it looks better even though this works.

   Unless I miss some coding standard on this.

   Or maybe we use some if statement and isset($cid)?

   Not sure ...

About 1. I haven't touched those lines, not sure why its showing up on git diff.

On 2. I agree that line is not very intuitive to read, perhaps something like this?

    $cid = FALSE;
    if (variable_get('use_menu_router_cache_for_lru_backend', FALSE)) {
      $cid = 'menu_item:' . hash('sha256', $path);
    }

In any case, $cid will always be set and not NULL (will either be a string or FALSE), so I think we are safe to keep the if statement as is. Unless I'm missing some coding standard...

1. Probably auto-correction of your editor. I do not care if we fix some coding standards violations on the way ...
2. Yep, lets do the if statement as its easier to read. ++

OK, here is the patch with the new if statement to make it easier to read.

RTBC, looks great now!

+++ b/includes/menu.inc
@@ -462,9 +462,23 @@ function menu_get_item($path = NULL, $router_item = NULL) {
+    if (variable_get('use_menu_router_cache_for_lru_backend', FALSE)) {

This could use a comment, both explaining when to use and what LRU is maybe?

A LRU (least-recently-used) cache.

from @Fabianx

So it was mentioned above that a similar query exists in Drupal 8 (I think it's Drupal\Core\Routing\RouteProvider::getRoutesByPath()). #2480811: Cache incoming path processing and route matching is clearly a separate D8 issue, but shouldn't we be committing a version of this patch to Drupal 8 first (acknowledging it's not an ideal solution, and that it could potentially be removed later in the other issue)?

That's my understanding of how to avoid D7->D8 regressions per https://www.drupal.org/core/backport-policy, to make sure there's some fix for this in Drupal 8 even if the more far-reaching issue never makes it.

Or are they so different for other reasons that a similar fix as in this patch would never work?

I also tend to agree regarding the code comment; it's not clear to me from the code what 'use_menu_router_cache_for_lru_backend' means or how/when the variable is supposed to be used.

Agree we need a better variable name and code comments.

Any suggestions?

And would we document that in default.settings.php or where else?

--

Spoke with catch and the solution in 8.x would look different as it would introduce a different LRU cache backend interface. Also this was a very late D7 revert, therefore part of the original code base.

And the revert should have never happened in this way (we just did not see this solution back then).

Also with 8.0.x the most likely place to cache would be \Drupal\Core\Routing\Router\AccessAwareRoute::matchRequest() which would mean we also skip the path alias lookup.

I thought about this again:

- a) This should live in its own cache bin, so its easily seperatable (e.g. cache_menu_router)
- b) The variable name is too long and should just be $conf['cache_menu_router'] = TRUE.
- c) The new setting needs to be added to settings.php and there the documentation needs to state that this should only be used with backends like redis or memcache.
- d) The cache item needs to be expired when the menu router table changes, which happened explicitly in the patch here as cache_menu was cleared.

I don't think D8 would profit from this as it has used a different direction and won't fixed the similar issue.

Given this was in D7 before and was reverted instead of just opt-ed out, I still stand by my opinion that this should be eligible to go back in. Even more so as D8 decided to won't fix the relevant partner issue in the mean time.

Could anyone move forward this issue ? (applying @Fabianx latest comments ?).

Fixing this issue could have an impact on performance, and it would be nice to let users opt-in to it.

So how am I supposed to run a production site with a bug like this? It is the 3rd time that menu_cache.ibd grows to 25GB and brings down the server!!!! Is anyone doing anything about it?

Any solutions my cache_menu table keeps growing and the expiry is set to '0'

Bumping to 7.61. This didn't make it into 7.60.

I have the same issue and it drives me crazy to figure out to solve this. Should we revert to Drupal to 7.59 waiting for a new release fixing this issue?

Edit: A possible solution is to use the File Cache module.

This really needs to get into the next release. Our site fell over today with a 22GB cache_menu.

+-----------------+--------------+-------------+-------+-------------+---------------------+
| Tables          | Size on Disk | Size in DB  | Rows  | Index in DB | Index-to-data ratio |
+-----------------+--------------+-------------+-------+-------------+---------------------+
| cache_menu      | 22568.00 MB  | 22225.27 MB | 45595 | 8.56 MB     | 0.00                |
| cache_page      | 4780.00 MB   | 4468.50 MB  | 37109 | 6.58 MB     | 0.00                |
| cache_form      | 1772.00 MB   | 364.05 MB   | 5668  | 1.02 MB     | 0.00                |
| cache_field     | 256.00 MB    | 236.83 MB   | 61828 | 4.52 MB     | 0.02                |
| cache_views     | 144.00 MB    | 133.58 MB   | 1645  | 0.19 MB     | 0.00                |
+-----------------+--------------+-------------+-------+-------------+---------------------+

I am happy to get this in given the router has changed so much, but someone needs to address #66 and create a new patch for D7.

#70 #71 #75:

The code in question is not yet in D7, so you have the opposite problem.

Please open a bug for huge cache_menu caches, which is likely indeed menus on the site and not the issue here.