Caching entire {menu_router} table causes MySQL error/slow rebuilds and slows menu_link_save

What are the steps to repeat this bug? When does it happen? What did you do to get rid of it?

My guess is that this is a general mysql issue for your site that happened to show up for comment_notify rather than being a comment_notify issue.

When you say "use the new content type" you mean visiting http://example.com/node/add/your_custom_type?

You can see that this is a fairly common issue across a variety of systems.

Looks like the problem happens when Drupal tries to cache the entire {menu_router} table in {cache_menu} (with cid = "router:").

MySQL has a default max_packet_size of 1M, which Drupal should respect.

See also #321154: can I disable cache_menu? how?.

Related forum posts: http://drupal.org/node/321210, and probably http://drupal.org/node/313887.

The same problem:

user warning: Got a packet bigger than 'max_allowed_packet' bytes query: UPDATE cache_menu SET data = 'a:1057:{s:8:\"mimemail\";a:26:{s:13:\"page callback\";s:13:\"mimemail_post\";s:15:\"access callback\";i:0;s:4:\"type\";i:4;s:6:\"module\";s:8:\"mimemail\";s:14:\"load_functions\";s:0:\"\";s:16:\"to_arg_functions\";s:0:\"\";s:5:\"title\";s:0:\"\";s:6:\"weight\";i:0;s:13:\"_number_parts\";i:1;s:6:\"_parts\";a:1:{i:0;s:8:\"mimemail\";}s:4:\"_fit\";i:1;s:8:\"_visible\";b:1;s:4:\"_tab\";b:0;s:10:\"tab_parent\";s:0:\"\";s:8:\"tab_root\";s:8:\"mimemail\";s:16:\"access arguments\";a:0:{}s:14:\"page arguments\";a:0:{}s:14:\"block callback\";s:0:\"\";s:15:\"title arguments\";a:0:{}s:14:\"title callback\";s:1:\"t\";s:11:\"description\";s:0:\"\";s:8:\"position\";s:0:\"\";s:4:\"path\";s:8:\"mimemail\";s:4:\"file\";s:0:\"\";s:9:\"file path\";s:0:\"\";s:12:\"include file\";s:0:\"\";}s:4:\"node\";a:26:{s:5:\"title\";s:7:\"Content\";s:13:\"page callback\";s:17:\"node_page_default\";s:16:\"access arguments\";a:1:{i:0;s:14:\"access 
...
e\";s:9:\"file path\";s:0:\"\";s:12:\"include file\";s:62:\"sites/all/modules/contributions/cck/includes/content.admin.inc\";}s:63:\"admin/content/node-type/szkola/fields/field_wiek/remove\";a:26:{s:5:\"title\";s:12:\"Remove field\";s:13:\"page callback\";s:15:\"drupal_get_form\";s:14:\"page arguments\";a:3:{i:0;s:25:\"content_field_remove_form\";i:1;s:6:\"szkola\";i:2;s:18:\"field_wiek\";}s:16:\"access arguments\";a:1:{i:0;s:24:\"administer content types\";}s:4:\"file\";s:26:\"includes/content.admin.inc\";s:4:\"type\";i:4;s:6:\"module\";s:7:\"content\";s:14:\"load_functions\";s:0:\"\";s:16:\"to_arg_functions\";s:0:\"\";s:6:\"weight\";i:0;s:13:\"_number_parts\";i:7;s:6:\"_parts\";a:7:{i:0;s:5:\"admin\";i:1;s:7:\"content\";i:2;s:9:\"node-type\";i:3;s:6:\"szkola\";i:4;s:6:\"fields\";i:5;s:18:\"field_wiek_uczniow\";i:6;s:6:\"remove\";}s:4:\"_fit\";i:127;s:8:\"_visible\";b:1;s:4:\"_tab\";b:0;s:10:\"tab_parent\";s:0:\"\";s:8:\"tab_root\";s:63:\"admin/content/node-type/szkola/fields/field_wiek/remove\";s:15:\"access callback\";s:11:\"user_access\";s:14:\"block callback\";s:0:\"\";s:15:\"title arguments\";a:0:{}s:14:\"title callback\";s:1:\"t\";s:11:\"description\";s:0:\"\";s:8:\"position\";s:0:\"\";s:4:\"path\";s:63:\"admin/content/node-type/szkola/fields/field_wiek_uczniow/remove\";s:9:\"file path\";s:0:\"\";s:12:\"include file\";s:62:\"sites/all/modules/contributions/cck/includes/content.admin.inc\";}}', created = 1231611827, expire = 0, headers = '', serialized = 1 WHERE cid = 'router:' in /home/sites/w/public_html/includes/cache.inc on line 109.

It happen always on: admin/build/modules page.
My provider heartinternet don't want to increase max_allowed_packet.
Any solution to that?

My issue described here: #357938: max_allowed_packet on admin/build/modules page

Please get a better provider. Having a max_allowed_packet of 1M, even if the MySQL default, makes strictly no sense. I've updated http://drupal.org/requirements to recommend max_allow_packet = 16M.

Thank you for advice.
Do you know some good hosting providers which offer server configuration with minimum requirements for Drupal websites?

http://drupal.org/hosting

See http://drupal.org/node/379976 for what looks to be a good workaround.

Three solutions for 6.x with max_package_size issue:
1. Increase your max_package_size in your MySQL config
2. Install www.drupal.org/project/fastpath_fscache module or different cache engine (APC/memcache/xcache), then Drupal will stop to use cache_menu table anymore.
3. Or try to install that one: #361967: Increase MAX_JOIN_SIZE and MAX_ALLOWED_PACKET settings in system.install

It has, but it's not commited yet.
But I've tested it and its working fine.
http://drupal.org/node/375293

Re-purposing this issue to deal with the actual bug.

I'm assuming we will backport this to 6.x, in which case we'll need a 6.x update function like:
cache_clear_all('router:', 'cache_menu', TRUE);

wrong placeholders before

Just to clarify: the goal of this patch is to remove the code that tries to cache the whole menu_router and then loads it from the cache as a single blob for every menu_link_save(). On sites with a lot of paths, this blob can be > 1 MB causing potential MySQL fgailures, and actually slowing the site down a lot. We get better performance by not caching it at all - basically this was a bad decision during the D6 menu-system rewrite that we never revisited to correct.

Now with 100% more DBTNG for D7

whitespace fix for the D7 patch, plus D6 backport.

@justinrandell - seems we cross-posted whitespace fix. This will have only a modest (if any) impact on the number of queries - there is a separate issue for that.

Effect on queries:

1. we omit an expensive cache_set() on every rebuild.
2. since aside from rebuilds we only usually save 1 menu_link per page load, we substitute a db_select for a cache_get (which is slow due to data size)

@justinrandell - please read the code in more detail - it does set the return value. In fact - that part of the code is not changed by this patch, so current D6/D7 does not work without it. Here's the other patch: http://drupal.org/node/302638

Re-rolling patches with a few more code comments/changes per discussion with Damien.

Some remarks I made to Peter on the IRC:

-  $menu = menu_router_build();
 
+  // Get the router if it's already in memory.
+  $menu = _menu_router_store();
   drupal_alter('menu_link', $item, $menu);

This is basically an API change: hook_menu_link_alter($item, $menu) will have an empty $menu in some cases. Peter said there are no implementation in contrib that makes use of that $menu parameter. For consistency, we should remove it from D7, and this needs more comments in D6.

-function menu_router_build($reset = FALSE) {
-  static $menu;
+function menu_router_build() {

The signature of menu_router_build() changes, but apparently PHP happily accepts supplementary parameters passed to a function, even in E_ALL.

-function _menu_find_router_path($menu, $link_path) {
-  $parts = explode('/', $link_path, MENU_MAX_PARTS);
+function _menu_find_router_path($link_path) {
+  $menu = _menu_router_store();

This private core function changes too. I grep the whole contribution repository, and there is no match for that function, except in one early version of i18n_menu.

@justinrandell - no, those lines are essential. Please work through the code. Adding more comments here.

I like this change.

Can someone please provide an overview of what this patch is doing? That might help make reviews easier.

Summary:

this patch removes the code that caches the whole menu_router and similarly removes the code to load it from the cache as a single blob for every menu_link_save().

Justification:

On sites with a lot of paths, this menu router data can be > 1 MB causing potential MySQL failures for those with a default 'max_allowed_packet' setting, and actually slowing the site down a lot due to the overhead of this slow query and the cost of serializing/unserializing a MB+ of array data.

Because of the above problems, we get better performance and solve the potential MySQL bug by not caching it at all - basically this caching was a bad design decision during the D6 menu-system rewrite that we never revisited to correct, but happily we can make this change now with little to no impact on the developer-facing API.

I reviewed the D7 patch, and the code and it looks solid. Since chx also approves, the RTBC here is quite justified.

Reviewed and it looks good. Committed to D7. Changing version to D6 for Gabor. Thanks all.

I am a bit appalled that this was committed without any serious performance tests.

http://ftp.de.debian.org/debian/pool/main/m/mysql-dfsg-5.0/mysql-dfsg-5....

According to this Debian changes the MySQL default to 16M (a much more sane value).

Thanks to Gerhard's suggestion, I found that actually there was a flaw - the module page is as or faster than before unless admin_menu is enabled (or any other module that calls menu_router_build()), in which case an extra router rebuild was being performed. So, for insurance we need to add back the reset flag + static var in menu_router_build().

Also, to clarify in terms of performance, this change only matters during a menu rebuild or when a menu link is saved - this code is never run on a normal page view.

The patch from #27 that was committed changed the call to hook_menu_link_alter() (it removed the $menu parameter) but did not update the documentation. This should be corrected as part of the follow-up.

Edit: See #423120: Missing argument 2 for devel_menu_link_alter() for an instance where this caused problems.

ok, sorry, I hadn't seen that this cache had a rather limited use case.

@drewish - sure - but please don't set back unless you have a problem with the follow-up patch that's pending.

Did you mean this documentaiton, or documentation in the code?

http://drupal.org/node/224333#hook_menu_link_alter

ok, new patch with menu.api.php change plus drupal_static() conversion.

looks good to me. Pretty straight forward.

Before committing this -- wouldn't it be better to fix menu_admin not to call this expensive function twice? I personally don't like it that when I call menu_router_build(), it only works the first time around. There might be legitimate reasons to call it twice, and in that case, I'd hope it has the desired effect.

@Dries - In D6 we have a $reset flag. The already committed patch totally removed that, but running into this made me realize we should put it back.

admin_menu in D6 calls it after core does a menu rebuild in order to more-or-less recapitulate what menu_navigation_links_rebuild() does. Note - in D6 admin_menu omits the $resst parameter and does not expect to force fresh data.

So, the D7 follow-up patch is, in effect, a partial reversion of the first patch, plus conversion of the affected code to the new statics API, plus api docs cleanup.

I still don't understand why we need to have the static variable in Drupal 7. It makes the API unpredictable as explained in #43. Can you try to explain differently why we're adding the static caching to menu_router_build() in Drupal 7 -- instead of fixing the caller?

@Dries - we are putting back the static caching that the first patch removed. The data generated by this function (calling all hook menu implementations) should never change during a page load, so it's a very natural thing to have a static cache for.

We could omit this is D7 and just leave the static in D6, since admin_menu is can certainly be changed before D7. An alternative/addition would be to have a wrapper function or make public in D7 the helper function added in the patch which would be more natural place to have the static cache.

Ok, maybe this is more in line with what you are suggesting:

function _menu_router_build is split into function _menu_router_build and _menu_router_save. Because of this, menu_router_build() actually lives up to its name and builds the router and masks, but does not make any changes to the database.

A new API function is added: menu_get_router() which uses the data in the static variable cache if menu_router_build() has run already.

Note that a big chunk of the patch is just that the

$insert = db_insert('menu_router')

code is moved around a little due to the function split.

missed changing one function name + re-rolled D6 patch.

I like this change too :D simple copy paste mostly really.

This patch took me about 2.5 hours to review because I had to come up to speed on menu system internals.

Ok, so it looks like in the old flow of menu_rebuild:

1. Removes the variable that indicates it's required so it doesn't keep getting rebuilt each time menu_execute_active_handler runs.
2. Clears the menu cache with menu_cache_clear_all which empties the cache_menu table, which holds various serialized arrays, such as of all the items in the various menus (navigation, user_menu, etc.).
3. Then calls menu_router_rebuild which goes through, calls hook_menu and hook_menu_alter, then calls:
a) _menu_router_rebuild which populates the extended internal menu info (load_functions and stuff like that) and chucks it in {menu_router}
b) _menu_router_store which statically caches the menu.
4. _menu_navigation_links_rebuild which goes through and populates the navigation menu's menu links.
5. _menu_clear_page_cache which "clear the page and block caches at most twice per page load." (Um. What!? Anyway, not the problem of this patch ;))

This patch:

1. Moves the menu_cache_clear_all() *after* the rebuilding to avoid potential race conditions where a subsequent page request tries to re-fill the {cache_menu} table while it's still in the middle of figuring out what $menu actually is.
2. Moves the {menu_router} insertion logic from _menu_router_build to a new function called _menu_router_save, and calls it from menu_rebuild(). Now menu_router_build() *only* builds the menu (and now the map too) and doesn't try and save it as well.
3. Renames _menu_router_store() to _menu_router_cache() (+1!) and puts static caching back in since there's not much point in re-generating this twice if you don't need to.
4. Creates a new function called menu_get_router() which modules such as admin menu can call if what they really mean is "I want to see a copy of what menu links exist" and not "I want to rebuild the whole fricking menu again."

I think this resolves Dries's concern, which is that sometimes menu_rebuild() would not do what you think it does. I know it's expensive, but I have had cause for calling this a couple of times in a single request in install profiles and the like, so I agree with that.

One question: Why bother with _menu_router_cache() at all? Why not merge that logic in with menu_get_router()?

- * @param $menu
- *   Associative array containg the menu router returned from menu_router_build().
  * @return
  *   None.
  */
-function hook_menu_link_alter(&$item, $menu) {
+function hook_menu_link_alter(&$item) {

I could see there being a use case for this in custom modules, if not in contrib. For example, if you wanted to do something fancy to everything under the "User" menu. But you can also use regex on $items, and this only fires on menu rebuild (right?) so I think we're ok here. Hey, at least we got rid of a typo in the docs (containg) ;)

@webchick - the caching wrapper function is different from menu_get_router() since in menu_link_save() we only want to use the router if it's in memory, otherwise we don't want to force it to be built.

I'm not sure if there is a better approach to make this work - the advantage of using the router when it's in memory (generally only when rebuilding the system navigation links) is that we avoid a query for each link by using the data in memory - this is surely much faster nad less load on the DB server.

The last change above (the link_alter) is necessary unless we want to jsut pass the NULL in when there is no available router in memory - no known implementations of this hook in D6 actually use the 2nd parameter for anything, and discussing with chx, there was not any specific reason to pass it, we just did that since we thought we'd have it available (in D6). Note that the actual change to that hook invocation is already in D7 - the patch here is just making the API docs consistent with the actual core code.

I spent another 30 minutes reviewing this patch and it does what I suggested. Committed to CVS HEAD. Updating version number.

great - thanks.

Regarding the patch in #48 for D6... there are whitespaces in empty line in new function _menu_router_cache().

The D6 patch is a straight backport of the one that got in D7. Applied to my D6 development environment, it worked flawlessly. I support its inclusion in D6.

this patch is identical to the last D6, except it removes the trailing whitespace on one line of function _menu_router_cache() per #54

Actually the D6 should have one more minor change - a re-ordering of operations in function menu_rebuild() (as was in the last D7 fu patch) to reduce race conditions a little - though it won't matter much since all the menu_link_save() calls already will be clearing parts of the cached data.

subscribing

Hm, I am not going to pretend I understand what happens in the patch :) If it is a straight backport of the D7 patch and works for Damien, that is great. It would be good to have one more tester at least, given that we have no automated test system here.

One big hunk of the patch is basically just an indentation change. The changes here are actually a little less than in D7 (I omitted splitting one helper function into two) and the overall effect is pretty simple:

1) we no longer store the menu router (i.e. an entire DB table worth of data) as one cache entry
2) when we save a menu link, we use the router if it's in memory, otherwise we now have to query the {menu_router} table

Reviewed the discussion and the patch. Committed.

did this go into the core? If so what version?

If you've Got a packet bigger than 'max_allowed_packet' in other cases than menu_router, you may try:
http://drupal.org/project/db_tweaks

@redhatmatt It's in 6.12

running 6.12 and I still get this error (I think)

user warning: Got a packet bigger than 'max_allowed_packet' bytes query: UPDATE d6_cache_menu SET data = 'a:2:{s:4:\"tree\";a:243:{i:1;a:2:{s:4:\"link\";a:37:{s:14:\"load_functions\";s:0:\"\";s:16:\"to_arg_functions\";s:0:\"\";s:15:\"access_callback\";s:1:\"1\";s:16:\"access_arguments\";s:6:\"a:0:{}\";s:13:\"page_callback\";s:17:\"system_batch_page\";s:14:\"page_arguments\";s:6:\"a:0:{}\";s:5:\"title\";s:0:\"\";s:14:\"title_callback\";s:1:\"t\";s:15:\"title_arguments\";s:0:\"\";s:4:\"type\";s:1:\"4\";s:11:\"description\";s:0:\"\";s:9:\"menu_name\";s:10:\"navigation\";s:4:\"mlid\";s:1:\"1\";s:4:\"plid\";s:1:\"0\";s:9:\"link_path\";s:5:\"batch\";s:11:\"router_path\";s:5:\"batch\";s:10:\"link_title\";s:0:\"\";s:7:\"options\";s:6:\"a:0:{}\";s:6:\"module\";s:6:\"system\";s:6:\"hidden\";s:2:\"-1\";s:8:\"external\";s:1:\"0\";s:12:\"has_children\";s:1:\"0\";s:8:\"expanded\";s:1:\"0\";s:6:\"weight\";s:1:\"0\";s:5:\"depth\";s:1:\"1\";s:10:\"customized\";s:1:\"0\";s:2:\"p1\";s:1:\"1\";s:2:\"p2\";s:1:\"0\";s:2:\"p3\";s:1:\"0\";s:2:\"p4\";s:1:\"0\";s:2:\"p5\";s:1:\"0\";s:2:\"p6\";s:1:\"0\";s:2:\"p7\";s:1:\"0\";s:2:\"p8\";s:1:\"0\";s:2:\"p9\";s:1:\"0\";s:7:\"updated\";s:1:\"0\";s:15:\"in_active_trail\";b:0;}s:5:\"below\";b:0;}i:3;a:2:{s:4:\"link\";a:37:{s:14:\"load_functions\";s:0:\"\";s:16:\"to_arg_functions\";s:0:\"\";s:15:\"access_callback\";s:11:\"user_access\";s:16:\"access_arguments\";s:32:\"a:1:{i:0;s:14:\"access content\";}\";s:13:\"page_callback\";s:17:\"node_page_default\";s:14:\"page_arguments\";s:6:\"a:0:{}\";s:5:\"title\";s:7:\"Content\";s:14:\"title_callback\";s:1:\"t\";s:15:\"title_arguments\";s:0:\"\";s:4:\"type\";s:1:\"4\";s:11:\"description\";s:0:\"\";s:9:\"menu_name\";s:10:\"navigation\";s:4:\"mlid\";s:1:\"3\";s:4:\"plid\";s:1:\"0\";s:9:\"link_path\";s:4:\"node\";s:11:\"router_path\"; ...

after the snip there are hundreds of further occurances of "router_path" (but no menu_router)

Is this a different issue?

In my {cache_menu} (running 6.10, i.e. before this fix) the router: entry is 568K but there is also a links:navigation-tree:data:**hash** of 416K. I suspect you have something like this. Either speak to your host about increasing the global max_allowed_packet or else use "db_tweaks" linked just above, or else do something similar yourself (basically you'd need to increase max_allowed_packed in hook_init()). Although accounts on shared servers won't be able to change the global value they will often have sufficient permissions to change it just for the duration of the current DB session, i.e. by running a query such as db_query(SET SESSION max_allowed_packet=16*1024*1024) which would set it to 16M.

The following worked for me on Ubuntu 8.04.x LTS desktop edition http://drupal.org/node/541396