Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
as advised in #231587-48: cache_menu: data blobs identical and huge; the issue is now re-opened against D8.
Background: menu_table is growing. For details please compare #231587-45: cache_menu: data blobs identical and huge; and [#529060]
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff.txt | 753 bytes | Manuel Garcia |
#58 | revert_cache_menu_patch-1234830-58.patch | 1.69 KB | Manuel Garcia |
#54 | interdiff.txt | 564 bytes | Manuel Garcia |
#54 | revert_cache_menu_patch-1234830-54.patch | 1.65 KB | Manuel Garcia |
#52 | interdiff.txt | 561 bytes | Manuel Garcia |
Comments
Comment #1
schildi CreditAttribution: schildi commentedto provide some additional information I made a stack trace in function cache_set (D6: file includes/cache.inc) as follows
the first 10 levels are shown here:
as described in http://drupal.org/node/231587#comment-4804234 all entries have 0 as expiry (permanent). That results in being not purged by function cache_clear_all.
Please have a look at that issue because it is close to a show stopper (will crash a site in between several days).
Comment #2
TuWebO CreditAttribution: TuWebO commentedSubs
Comment #3
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedI 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:
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.
Comment #4
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedSome 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.
Comment #5
catch@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.
Comment #6
danny.ketelslegers CreditAttribution: danny.ketelslegers commentedAny 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!
Comment #7
StevenPatzComment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedoh dear. this is a lot worse than the original report.
$path, which is used in $cid, will be different for 'node/1', 'node/2' etc. and 'user/1', 'user/2' etc. etc. etc.
so we're caching router items like 'node/%' over, and over, and over, and over. and 'user/%', and 'term/%'. so much caching win.
we need to roll back #643984: Cache menu_get_item(), and rethink how we derive $cid from the request.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedpatch to roll back #643984: Cache menu_get_item().
Comment #10
Fabianx CreditAttribution: Fabianx commentedIt 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.
Comment #11
Ludo.RComing 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
Comment #12
catch#9 looks good to me.
Comment #13
catchTagging.
Comment #14
sunLooks good for me, too, thanks! Would make sense to get this into the Feb 1st release.
Comment #15
webchickWhoopsie. :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.
Comment #16
catchMoving back to 7.x since this is already in 8.x.
Comment #17
webchickSorry.
Comment #18
oriol_e9gOnly backport from #9 to D7
Comment #19
oriol_e9gComment #20
BTMash CreditAttribution: BTMash commented#18: 1234830-18.patch queued for re-testing.
Comment #21
webchickCommitted and pushed to 7.x. Thanks!
Comment #23
goyie CreditAttribution: goyie commentedI 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!
Comment #24
goyie CreditAttribution: goyie commentedIt seems I forgot to reopen issue. Table cache_menu still grows after patch #18.
Comment #25
catchSince 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.
Comment #26
schildi CreditAttribution: schildi commentedWhy do you think it is solved? The starting point was the observation that the table is growing endless in D6 (compare issue summary). For some reason the issue was opened in D8 and handled for D7. As far as I understood the patch #9 only rolls back a previous patch which didn't solve the situation either. So we are at the starting point again. I just checked (OK, just for D6, but that is where the issue came from originally):
Edit:
I think the issue is solved when the described erratic behavior was corrected - and not when some patch is applied. Users "Stefan Freudenberg" and "beejeebus" did some investigations - thanks a lot for that! - but possibly the offered patches did not help.
Comment #27
catchOK 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.
Comment #28
schildi CreditAttribution: schildi commentedSince I am still running (mainly) a D6 site I would like to ask goyie for this attachment. So this means to create two files. The first one is quite clear to me
For the second file you probably mean to extract one of the blob data?
select * from cache_menu where length(data) > 1000 limit 1;
Comment #29
goyie CreditAttribution: goyie commentedschildi, 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.
Comment #30
goyie CreditAttribution: goyie commentedI 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?
Comment #31
schildi CreditAttribution: schildi commentedPossibly it helps when also data of a D6 system are available (see attached files).
Comment #32
catchOK 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.
Comment #33
catchBack to 8.x
Comment #34
catchThis 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.
Comment #35
schildi CreditAttribution: schildi commentedas far as I remember there was a menu entry created automatically in D4 for each node created. Since the site where I am reporting the issue for is migrated from D4 --> D5 --> D6 these menu entries still exist. Is there a way to set up some kind of a batch to get rid of thousands of menu entries?
Comment #36
webchickThis seems worth calling out in the release notes for the patch that was committed.
Comment #36.0
webchickLinkify related issues.
Comment #37
knalstaaf CreditAttribution: knalstaaf commentedCould one (or a combination) of the following modules fix this for D7?
Or is that a different matter?
Comment #38
damiankloip CreditAttribution: damiankloip commentedMay want to repurpose this slightly now as cache_menu doesn't exist anymore in D8. See #1194136: Re-organise core cache bins.
Comment #39
joelpittetShould 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.
Comment #40
Fabianx CreditAttribution: Fabianx commentedWell, 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.
Comment #41
Fabianx CreditAttribution: Fabianx commentedWe 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.
Comment #42
Fabianx CreditAttribution: Fabianx commentedComment #43
kentr CreditAttribution: kentr commentedIn 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.
Comment #44
catch{cache_menu} doesn't exist in Drupal 7, but a version of this query does.
Comment #45
catchOpened #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.
Comment #46
Fabianx CreditAttribution: Fabianx commentedComment #48
Manuel Garcia CreditAttribution: Manuel Garcia commentedHere it is for D7 (reverts #18).
Comment #50
Manuel Garcia CreditAttribution: Manuel Garcia commentedIgnore the previous patch, don't know what I was thinking, let's try it again.
Comment #51
Fabianx CreditAttribution: Fabianx commentedNow make the cid creation part optional and disabled as before, like:
and it works out. Just need to find a good variable name.
Comment #52
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, used the variable name you suggested for now.
Comment #53
Fabianx CreditAttribution: Fabianx commentedNeed this, too ...
Comment #54
Manuel Garcia CreditAttribution: Manuel Garcia commentedDuh, sorry bout that.
Comment #55
Fabianx CreditAttribution: Fabianx commentedWhat is changed here? White space?
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 ...
Comment #56
Manuel Garcia CreditAttribution: Manuel Garcia commentedAbout 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?
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...Comment #57
Fabianx CreditAttribution: Fabianx commented1. 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. ++
Comment #58
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, here is the patch with the new if statement to make it easier to read.
Comment #59
Fabianx CreditAttribution: Fabianx commentedRTBC, looks great now!
Comment #60
joelpittetThis could use a comment, both explaining when to use and what LRU is maybe?
from @Fabianx
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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.
Comment #62
Fabianx CreditAttribution: Fabianx commentedAgree 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).
Comment #63
catchAlso 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.
Comment #66
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #69
yonailo CreditAttribution: yonailo as a volunteer commentedCould 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.
Comment #70
yfierro CreditAttribution: yfierro commentedSo 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?
Comment #71
munyivaAny solutions my cache_menu table keeps growing and the expiry is set to '0'
Comment #72
joseph.olstadBumping to 7.61. This didn't make it into 7.60.
Comment #73
XorunaI 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.
Comment #74
joseph.olstadComment #75
xrxphawxby CreditAttribution: xrxphawxby commentedThis really needs to get into the next release. Our site fell over today with a 22GB cache_menu.
Comment #76
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedI 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.
Comment #77
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commented#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.
Comment #78
joseph.olstadComment #79
MustangGB CreditAttribution: MustangGB commentedComment #80
MustangGB CreditAttribution: MustangGB commented