Merge lp:~catch-drupal/pressflow/path_slow_query into lp:pressflow

[Nathaniel Catchpole]

Backporting selective full rebuilding of the path alias whitelist from Drupal 7.

 - update fixed an omission that meant the new code never actually stopped the slow query from running.
 - added a default argument for the whitelist rebuild.
 - remove path_alias_flush_caches() - the whitelist is stored in cache_path, and it doesn't matter if the other caches get stale. This was causing the whitelist query (which is very, very slow) to run every cron run.

[Narayan Newton (nnewton-drupal)]

Taking this to review, testing a deploy currently.

[Narayan Newton (nnewton-drupal)]

This looks good to me and has been working well on two client sites. Marking approved. This also merges cleanly with 6.20.

review: Approve

[David Strauss (davidstrauss)]

I'll interpret Narayan's approval as a sign I should check this out for
merging. Cool beans.
On Jan 5, 2011 4:01 PM, "Narayan Newton" <email address hidden> wrote:
> Review: Approve
> This looks good to me and has been working well on two client sites.
Marking approved. This also merges cleanly with 6.20.
> --
>
https://code.launchpad.net/~catch-drupal/pressflow/path_slow_query/+merge/42333
> You are requested to review the proposed merge of
lp:~catch-drupal/pressflow/path_slow_query into lp:pressflow.

[Nathaniel Catchpole (catch-drupal)]

Friendly bump, just found this causing issues on another client site, and thought this had been committed until I did a fresh checkout.

[Nathaniel Catchpole (catch-drupal)]

Another way to fix the hook_flush_caches() issue would be to put this into a variable instead of using cache_get()/cache_set() for the whitelist. This is was D7 does but it's more of a change, so leaving as it is for now.

[lp:~catch-drupal/pressflow/path_slow_query]

102. By Nathaniel <catch@catch-toshiba> on 2011-07-01

Merge in trunk.

103. By Nathaniel <catch@catch-toshiba> on 2011-07-01

Use a variable instead of cache system for the whitelist - the cache maintains itself, and the rebuild query is extremely slow on large data sets. Also patches Drupal 7 core version.

[Nathaniel Catchpole (catch-drupal)]

Just made a new push here to use a variable rather than the cache system. The cache maintains itself pretty well, and rebuilding is extremely expensive, so it is better to only rebuild it when absolutely requested rather than when {cache_path} might get wiped in general.

[Judah Anthony (judahanthony)]

May I ask the progress of this branch?

[Nathaniel Catchpole (catch-drupal)]

I'm now working in https://github.com/tag1consulting/pressflow6/tree/path_alias_slow_query - was getting hard to manage multiple branches in bzr.

review: Disapprove

[David Strauss (davidstrauss)]

Just to let folks here know, I've moved Pressflow 6 to GitHub: https://github.com/pressflow/6

Please post any new requested changes as pull requests there for review and integration.

[David Strauss (davidstrauss)]

Pressflow 6 is now maintained on GitHub, so I'm marking this merge as rejected.

Unmerged revisions

103. By Nathaniel <catch@catch-toshiba> on 2011-07-01

Use a variable instead of cache system for the whitelist - the cache maintains itself, and the rebuild query is extremely slow on large data sets. Also patches Drupal 7 core version.

102. By Nathaniel <catch@catch-toshiba> on 2011-07-01

Merge in trunk.

101. By Nathaniel Catchpole <catch@catch-laptop> on 2011-04-14

Remove path_alias_cache_flush_caches() - stops the whitelist being rebuilt every cron run.

100. By Nathaniel <catch@catch-toshiba> on 2010-12-01

Add default argument for path_alias_cache_path_whitelist_rebuild().

99. By Nathaniel <catch@catch-toshiba> on 2010-11-29

Minus the typo.

98. By Nathaniel <catch@catch-toshiba> on 2010-11-29

Actually stop the slow query from running.

97. By Nathaniel Catchpole <catch@catch-laptop> on 2010-11-08

#612878 - path alias cache slow query on node inserts.

=== modified file 'includes/common.inc'
--- includes/common.inc	2011-06-01 07:13:05 +0000
+++ includes/common.inc	2011-07-01 04:46:30 +0000
@@ -127,8 +127,13 @@
 /**
  * Reset the static variable which holds the aliases mapped for this request.
  */
-function drupal_clear_path_cache() {
+function drupal_clear_path_cache($path = NULL) {
   drupal_lookup_path('wipe');
+  $modules = module_implements('lookup_path');
+  if (!empty($modules)) {
+    $module = reset($modules);
+    module_invoke($module, 'path_alias_whitelist_rebuild', $path);
+  }
 }
 
 /*

=== modified file 'modules/path/path.module'
--- modules/path/path.module	2011-05-26 02:56:05 +0000
+++ modules/path/path.module	2011-07-01 04:46:30 +0000
@@ -120,7 +120,7 @@
       db_query("DELETE FROM {url_alias} WHERE src = '%s'", $path);
     }
   }
-  drupal_clear_path_cache();
+  drupal_clear_path_cache($path);
 }
 
 

=== modified file 'modules/path_alias_cache/path_alias_cache.module'
--- modules/path_alias_cache/path_alias_cache.module	2010-10-22 15:37:23 +0000
+++ modules/path_alias_cache/path_alias_cache.module	2011-07-01 04:46:30 +0000
@@ -1,15 +1,6 @@
 <?php
 
 /**
- * Implementation of hook_flush_caches().
- *
- * @return array
- */
-function path_alias_cache_flush_caches() {
-  return array('cache_path');
-}
-
-/**
  * Implementation of hook_lookup_path
  *
  * This version of lookup_path is almost exactly what is in core, but adds a cache table.
@@ -28,18 +19,17 @@
 
   // Retrieve the path alias whitelist.
   if (!isset($cache['whitelist'])) {
-    if ($cached = cache_get('path_alias_whitelist', 'cache_path')) {
-      $cache['whitelist'] = $cached->data;
-    } else {
-      $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
+    $whitelist = variable_get('path_alias_whitelist', NULL);
+    if (!isset($whitelist)) {
+      $whitelist = path_alias_cache_path_alias_whitelist_rebuild();
     }
+    $cache['whitelist'] = $whitelist;
   }
 
   $path_language = $path_language ? $path_language : $language->language;
 
   if ($action == 'wipe') {
     $cache = array();
-    $cache['whitelist'] = path_alias_cache_path_alias_whitelist_rebuild();
   }
   elseif ($cache['whitelist'] && $path != '') {
     if ($action == 'alias') {
@@ -198,7 +188,14 @@
  * @return
  *   An array containing a white list of path aliases.
  */
-function path_alias_cache_path_alias_whitelist_rebuild() {
+function path_alias_cache_path_alias_whitelist_rebuild($path  = NULL) {
+  // Don't rebuild the path alias whitelist for already whitelisted paths.
+  if (!empty($path) && $whitelist = variable_get('path_alias_whitelist', NULL)) {
+    if (isset($whitelist[strtok($path, '/')])) {
+      return TRUE;
+    }
+  }
+
   // For each alias in the database, get the top level component of the system
   // path it corresponds to. This is the portion of the path before the first "/"
   // if present, otherwise the whole path itself.
@@ -207,7 +204,7 @@
   while ($row = db_fetch_object($result)) {
     $whitelist[$row->path] = TRUE;
   }
-  cache_set('path_alias_whitelist', $whitelist, 'cache_path');
+  variable_set('path_alias_whitelist', $whitelist);
   return $whitelist;
 }