- Patch #105639 by David Strauss, catch, Berdir, drewish: tracker performance improvements.

3 files changed, 452 insertions, 68 deletions

diff --git a/modules/tracker/tracker.module b/modules/tracker/tracker.module
index 89ea0e826..c8a32ec57 100644
--- a/modules/tracker/tracker.module
+++ b/modules/tracker/tracker.module
@@ -36,10 +36,12 @@ function tracker_menu() {
   );
   $items['tracker/%user_uid_optional'] = array(
     'title' => 'My recent posts',
+    'page callback' => 'tracker_page',
     'access callback' => '_tracker_myrecent_access',
     'access arguments' => array(1),
     'page arguments' => array(1),
     'type' => MENU_LOCAL_TASK,
+    'file' => 'tracker.pages.inc',
   );
 
   $items['user/%user/track'] = array(
@@ -55,11 +57,91 @@ function tracker_menu() {
     'title' => 'Track posts',
     'type' => MENU_DEFAULT_LOCAL_TASK,
   );
+
   return $items;
 }
 
 /**
- * Access callback for tracker/%user_uid_optional
+ * Implement hook_cron().
+ */
+function tracker_cron() {
+  $max_nid = variable_get('tracker_index_nid', 0);
+  $batch_size = variable_get('tracker_batch_size', 1000);
+  if ($max_nid > 0) {
+    $last_nid = FALSE;
+    $result = db_query_range('SELECT nid, uid, status FROM {node} WHERE nid <= :max_nid ORDER BY nid DESC', array(':max_nid' => $max_nid), 0, $batch_size);
+
+    $count = 0;
+
+    foreach ($result as $row) {
+      // Calculate the changed timestamp for this node.
+      $changed = _tracker_calculate_changed($row->nid);
+
+      // Remove existing data for this node.
+      db_delete('tracker_node')
+        ->condition('nid', $row->nid)
+        ->execute();
+      db_delete('tracker_user')
+        ->condition('nid', $row->nid)
+        ->execute();
+
+      // Insert the node-level data.
+      db_insert('tracker_node')
+        ->fields(array(
+          'nid' => $row->nid,
+          'published' => $row->status,
+          'changed' => $changed,
+        ))
+        ->execute();
+
+      // Insert the user-level data for the node's author.
+      db_insert('tracker_user')
+        ->fields(array(
+          'nid' => $row->nid,
+          'published' => $row->status,
+          'changed' => $changed,
+          'uid' => $row->uid,
+        ))
+        ->execute();
+
+      $query = db_select('comment', 'c');
+      // Force PostgreSQL to do an implicit cast by adding 0.
+      $query->addExpression('0 + :changed', 'changed', array(':changed' => $changed));
+      $query->addField('c', 'status', 'published');
+      $query
+        ->distinct()
+        ->fields('c', array('uid', 'nid'))
+        ->condition('c.nid', $row->nid)
+        ->condition('c.uid', $row->uid, '<>')
+        ->condition('c.status', COMMENT_PUBLISHED);
+
+      // Insert the user-level data for the commenters (except if a commenter
+      // is the node's author).
+      db_insert('tracker_user')
+        ->from($query)
+        ->execute();
+
+      // Note that we have indexed at least one node.
+      $last_nid = $row->nid;
+
+      $count++;
+    }
+
+    if ($last_nid !== FALSE) {
+      // Prepare a starting point for the next run.
+      variable_set('tracker_index_nid', $last_nid - 1);
+
+      watchdog('tracker', t('Indexed %count nodes for tracking.', array('%count' => $count)));
+    }
+    else {
+      // If all nodes have been indexed, set to zero to skip future cron runs.
+      variable_set('tracker_index_nid', 0);
+    }
+  }
+}
+
+/**
+ * Access callback for tracker/%user_uid_optional.
  */
 function _tracker_myrecent_access($account) {
   // This path is only allowed for authenticated users looking at their own posts.
@@ -67,9 +149,210 @@ function _tracker_myrecent_access($account) {
 }
 
 /**
- * Access callback for user/%user/track
+ * Access callback for user/%user/track.
  */
 function _tracker_user_access($account) {
   return user_view_access($account) && user_access('access content');
 }
 
+/**
+ * Implement hook_nodeapi_insert().
+ */
+function tracker_node_insert($node, $arg = 0) {
+  _tracker_add($node->nid, $node->uid, $node->changed);
+}
+
+/**
+ * Implement hook_nodeapi_update().
+ */
+function tracker_node_update($node, $arg = 0) {
+  _tracker_add($node->nid, $node->uid, $node->changed);
+}
+
+/**
+ * Implement hook_nodeapi_delete().
+ */
+function tracker_node_delete($node, $arg = 0) {
+  _tracker_remove($node->nid, $node->uid, $node->changed);
+}
+
+/**
+ * Implement hook_comment_update().
+ *
+ * Comment module doesn't call hook_comment_unpublish() when saving individual
+ * comments so we need to check for those here.
+ */
+function tracker_comment_update($comment) {
+  $comment = (array) $comment;
+  // comment_save() calls hook_comment_publish() for all published comments
+  // so we to handle all other values here.
+  if ($comment['status'] != COMMENT_PUBLISHED) {
+    _tracker_remove($comment['nid'], $comment['uid'], $comment['timestamp']);
+  }
+}
+
+/**
+ * Implement hook_comment_publish().
+ *
+ * This actually handles the insert and update of published nodes since
+ * comment_save() calls hook_comment_publish() for all published comments.
+ */
+function tracker_comment_publish($comment) {
+  _tracker_add($comment->nid, $comment->uid, $comment->timestamp);
+}
+
+/**
+ * Implement hook_comment_unpublish().
+ */
+function tracker_comment_unpublish($comment) {
+  _tracker_remove($comment->nid, $comment->uid, $comment->timestamp);
+}
+
+/**
+ * Implement hook_comment_delete().
+ */
+function tracker_comment_delete($comment) {
+  _tracker_remove($comment->nid, $comment->uid, $comment->timestamp);
+}
+
+/**
+ * Update indexing tables when a node is added, updated or commented on.
+ *
+ * @param $nid
+ *   A node ID.
+ * @param $uid
+ *   The node or comment author.
+ * @param $changed
+ *   The node updated timestamp or comment timestamp.
+ */
+function _tracker_add($nid, $uid, $changed) {
+  $node = db_query('SELECT nid, status, uid, changed FROM {node} WHERE nid = :nid', array(':nid' => $nid))->fetchObject();
+
+  // Adding a comment can only increase the changed timestamp, so our
+  // calculation here is simple.
+  $changed = max($node->changed, $changed);
+
+  // Update the node-level data.
+  db_merge('tracker_node')
+    ->key(array('nid' => $nid))
+    ->fields(array(
+      'changed' => $changed,
+      'published' => $node->status,
+    ))
+    ->execute();
+
+  // Create or update the user-level data.
+  db_merge('tracker_user')
+    ->key(array(
+      'nid' => $nid,
+      'uid' => $uid,
+    ))
+    ->fields(array(
+      'changed' => $changed,
+      'published' => $node->status,
+    ))
+    ->execute();
+}
+
+/**
+ * Determine the max timestamp between $node->changed and the last comment.
+ *
+ * @param $nid
+ *   A node ID.
+ *
+ * @return
+ *  The $node->changed timestamp, or most recent comment timestamp, whichever
+ *  is the greatest.
+ */
+function _tracker_calculate_changed($nid) {
+  $changed = db_query('SELECT changed FROM {node} WHERE nid = :nid', array(':nid' => $nid))->fetchField();
+  $latest_comment = db_query_range('SELECT cid, timestamp FROM {comment} WHERE nid = :nid AND status = :status ORDER BY timestamp DESC', array(
+    ':nid' => $nid,
+    ':status' => COMMENT_PUBLISHED,
+  ), 0, 1)->fetchObject();
+  if ($latest_comment && $latest_comment->timestamp > $changed) {
+    $changed = $latest_comment->timestamp;
+  }
+  return $changed;
+}
+
+/**
+ * Clean up indexed data when nodes or comments are removed.
+ *
+ * @param $nid
+ *  The node ID.
+ * @param $uid
+ *   The author of the node or comment.
+ * @param $changed
+ *   The last changed timestamp of the node.
+ */
+function _tracker_remove($nid, $uid = NULL, $changed = NULL) {
+  $node = db_query('SELECT nid, status, uid, changed FROM {node} WHERE nid = :nid', array(':nid' => $nid))->fetchObject();
+
+  // The user only keeps his or her subscription if both of the following are true:
+  // (1) The node exists.
+  // (2) The user is either the node author or has commented on the node.
+  $keep_subscription = FALSE;
+
+  if ($node) {
+    // Self-authorship is one reason to keep the user's subscription.
+    $keep_subscription = ($node->uid == $uid);
+
+    // Comments are a second reason to keep the user's subscription.
+    if (!$keep_subscription) {
+      // Check if the user has commented at least once on the given nid
+      $keep_subscription = db_query_range('SELECT COUNT(*) FROM {comment} WHERE nid = :nid AND uid = :uid AND status = 0', array(
+        ':nid' => $nid,
+        ':uid' => $uid,
+      ), 0, 1)->fetchField();
+    }
+
+    // If we haven't found a reason to keep the user's subscription, delete it.
+    if (!$keep_subscription) {
+      db_delete('tracker_user')
+        ->condition('nid', $nid)
+        ->condition('uid', $uid)
+        ->execute();
+    }
+
+    // Now we need to update the (possibly) changed timestamps for other users
+    // and the node itself.
+
+    // We only need to do this if the removed item has a timestamp that equals
+    // or exceeds the listed changed timestamp for the node
+    $tracker_node = db_query('SELECT nid, changed FROM {tracker_node} WHERE nid = :nid', array(':nid' => $nid))->fetchObject();
+    if ($tracker_node && $changed >= $tracker_node->changed) {
+      // If we're here, the item being removed is *possibly* the item that
+      // established the node's changed timestamp.
+
+      // We just have to recalculate things from scratch.
+      $changed = _tracker_calculate_changed($nid);
+
+      // And then we push the out the new changed timestamp to our denormalized
+      // tables.
+      db_update('tracker_node')
+        ->fields(array(
+          'changed' => $changed,
+          'published' => $node->status,
+        ))
+        ->condition('nid', $nid)
+        ->execute();
+      db_update('tracker_node')
+        ->fields(array(
+          'changed' => $changed,
+          'published' => $node->status,
+        ))
+        ->condition('nid', $nid)
+        ->execute();
+   }
+  }
+  else {
+    // If the node doesn't exist, remove everything.
+    db_delete('tracker_node')
+      ->condition('nid', $nid)
+      ->execute();
+    db_delete('tracker_user')
+      ->condition('nid', $nid)
+      ->execute();
+  }
+}
diff --git a/modules/tracker/tracker.pages.inc b/modules/tracker/tracker.pages.inc
index 07226b8a8..b39b415a7 100644
--- a/modules/tracker/tracker.pages.inc
+++ b/modules/tracker/tracker.pages.inc
@@ -8,63 +8,67 @@
 
 
 /**
- * Menu callback. Prints a listing of active nodes on the site.
+ * Menu callback; prints a listing of active nodes on the site.
  */
 function tracker_page($account = NULL, $set_title = FALSE) {
-  // TODO: These queries are very expensive, see http://drupal.org/node/105639
-  $query = db_select('node', 'n', array('target' => 'slave'))->extend('PagerDefault');
-  $query->join('users', 'u', 'n.uid = u.uid');
-  $query->join('node_comment_statistics', 'l', 'n.nid = l.nid');
-  $query->addExpression('GREATEST(n.changed, l.last_comment_timestamp)', 'last_updated');
-  $query
-    ->distinct()
-    ->fields('n', array('nid', 'title', 'type', 'changed', 'uid'))
-    ->fields('u', array('name'))
-    ->fields('l', array('comment_count'))
-    ->condition('n.status', 1)
-    ->orderBy('last_updated', 'DESC')
-    ->addTag('node_access')
-    ->limit(25);
-
   if ($account) {
+    $query = db_select('tracker_user', 't')->extend('PagerDefault');
+    $query->condition('t.uid', $account->uid);
+
     if ($set_title) {
       // When viewed from user/%user/track, display the name of the user
       // as page title -- the tab title remains Track so this needs to be done
       // here and not in the menu definition.
       drupal_set_title($account->name);
     }
-    $query->leftJoin('comment', 'c', 'n.nid = c.nid AND (c.status = :status OR c.status IS NULL)', array(':status' => COMMENT_PUBLISHED));
-    $query->condition(db_or()
-      ->condition('n.uid', $account->uid)
-      ->condition('c.uid', $account->uid)
-    );
   }
+  else {
+    $query = db_select('tracker_node', 't')->extend('PagerDefault');
+  }
+
+  // This array acts as a placeholder for the data selected later
+  // while keeping the correct order.
+  $nodes = $query
+    ->addTag('node_access')
+    ->fields('t', array('nid', 'changed'))
+    ->condition('t.published', 1)
+    ->orderBy('t.changed', 'DESC')
+    ->limit(25)
+    ->execute()
+    ->fetchAllAssoc('nid');
 
-  $result = $query->execute();
+  if (!empty($nodes)) {
+    // Now, get the data and put into the placeholder array
+    $result = db_query('SELECT n.nid, n.title, n.type, n.changed, n.uid, u.name, l.comment_count FROM {node} n INNER JOIN {node_comment_statistics} l ON n.nid = l.nid INNER JOIN {users} u ON n.uid = u.uid WHERE n.nid IN (:nids)', array(':nids' => array_keys($nodes)));
+    foreach ($result as $node) {
+      $node->last_activity = $nodes[$node->nid]->changed;
+      $nodes[$node->nid] = $node;
+    }
 
-  $rows = array();
-  foreach ($result as $node) {
-    // Determine the number of comments:
-    $comments = 0;
-    if ($node->comment_count) {
-      $comments = $node->comment_count;
+    // Finally display the data
+    $rows = array();
+    foreach ($nodes as $node) {
+      // Determine the number of comments:
+      $comments = 0;
+      if ($node->comment_count) {
+        $comments = $node->comment_count;
 
-      if ($new = comment_num_new($node->nid)) {
-        $comments .= '<br />';
-        $comments .= l(format_plural($new, '1 new', '@count new'), "node/$node->nid", array('query' => comment_new_page_count($node->comment_count, $new, $node), 'fragment' => 'new'));
+        if ($new = comment_num_new($node->nid)) {
+          $comments .= '<br />';
+          $comments .= l(format_plural($new, '1 new', '@count new'), 'node/'. $node->nid, array('fragment' => 'new'));
+        }
       }
-    }
 
-    $rows[] = array(
-      check_plain(node_type_get_name($node->type)),
-      l($node->title, "node/$node->nid") . ' ' . theme('mark', node_mark($node->nid, $node->changed)),
-      theme('username', $node),
-      array('class' => array('replies'), 'data' => $comments),
-      t('!time ago', array('!time' => format_interval(REQUEST_TIME - $node->last_updated)))
-    );
+      $rows[] = array(
+        check_plain(node_type_get_name($node->type)),
+        l($node->title, 'node/' . $node->nid) . ' ' . theme('mark', node_mark($node->nid, $node->changed)),
+        theme('username', $node),
+        array('class' => array('replies'), 'data' => $comments),
+        t('!time ago', array('!time' => format_interval(REQUEST_TIME - $node->last_activity)))
+      );
+    }
   }
-
-  if (!$rows) {
+  else {
     $rows[] = array(array('data' => t('No posts available.'), 'colspan' => '5'));
   }
 
diff --git a/modules/tracker/tracker.test b/modules/tracker/tracker.test
index 9943c17aa..7aa215bd1 100644
--- a/modules/tracker/tracker.test
+++ b/modules/tracker/tracker.test
@@ -20,6 +20,9 @@ class TrackerTest extends DrupalWebTestCase {
     $permissions = array('access comments', 'create page content', 'post comments', 'post comments without approval');
     $this->user = $this->drupalCreateUser($permissions);
     $this->other_user = $this->drupalCreateUser($permissions);
+
+    // Make node preview optional.
+    variable_set('comment_preview_page', 0);
   }
 
   /**
@@ -28,20 +31,18 @@ class TrackerTest extends DrupalWebTestCase {
   function testTrackerAll() {
     $this->drupalLogin($this->user);
 
-    $page1 = array(
-      'title' => $this->randomName(8),
-      'status' => 1,
-    );
-    $page2 = array(
+    $unpublished = $this->drupalCreateNode(array(
       'title' => $this->randomName(8),
       'status' => 0,
-    );
-    $this->drupalCreateNode($page1);
-    $this->drupalCreateNode($page2);
+    ));
+    $published = $this->drupalCreateNode(array(
+      'title' => $this->randomName(8),
+      'status' => 1,
+    ));
 
     $this->drupalGet('tracker');
-    $this->assertText($page1['title'], t('Nodes show up in the tracker listing.'));
-    $this->assertNoText($page2['title'], t('Unpublished nodes do not show up in the tracker listing.'));
+    $this->assertNoText($unpublished->title, t('Unpublished node do not show up in the tracker listing.'));
+    $this->assertText($published->title, t('Published node show up in the tracker listing.'));
     $this->assertLink(t('My recent posts'), 0, t('User tab shows up on the global tracker page.'));
   }
 
@@ -51,22 +52,37 @@ class TrackerTest extends DrupalWebTestCase {
   function testTrackerUser() {
     $this->drupalLogin($this->user);
 
-    $page1 = array(
+    $unpublished = $this->drupalCreateNode(array(
       'title' => $this->randomName(8),
       'uid' => $this->user->uid,
-      'status' => 1,
-    );
-    $page2 = array(
+      'status' => 0,
+    ));
+    $my_published = $this->drupalCreateNode(array(
       'title' => $this->randomName(8),
       'uid' => $this->user->uid,
-      'status' => 0,
+      'status' => 1,
+    ));
+    $other_published_no_comment = $this->drupalCreateNode(array(
+      'title' => $this->randomName(8),
+      'uid' => $this->other_user->uid,
+      'status' => 1,
+    ));
+    $other_published_my_comment = $this->drupalCreateNode(array(
+      'title' => $this->randomName(8),
+      'uid' => $this->other_user->uid,
+      'status' => 1,
+    ));
+    $comment = array(
+      'subject' => $this->randomName(),
+      'comment' => $this->randomName(20),
     );
-    $this->drupalCreateNode($page1);
-    $this->drupalCreateNode($page2);
+    $this->drupalPost('comment/reply/' . $other_published_my_comment->nid, $comment, t('Save'));
 
     $this->drupalGet('user/' . $this->user->uid . '/track');
-    $this->assertText($page1['title'], t("Nodes show up in the author's tracker listing."));
-    $this->assertNoText($page2['title'], t("Unpublished nodes do not show up in the author's tracker listing."));
+    $this->assertNoText($unpublished->title, t("Unpublished nodes do not show up in the users's tracker listing."));
+    $this->assertText($my_published->title, t("Published nodes show up in the user's tracker listing."));
+    $this->assertNoText($other_published_no_comment->title, t("Other user's nodes do not show up in the user's tracker listing."));
+    $this->assertText($other_published_my_comment->title, t("Nodes that the user has commented on appear in the user's tracker listing."));
   }
 
   /**
@@ -100,16 +116,12 @@ class TrackerTest extends DrupalWebTestCase {
    * Test comment counters on the tracker listing.
    */
   function testTrackerNewComments() {
-    // Make node preview optional
-    variable_set('comment_preview_page', 0);
-
     $this->drupalLogin($this->user);
 
-    $edit = array(
+    $node = $this->drupalCreateNode(array(
       'comment' => 2,
       'title' => $this->randomName(),
-    );
-    $node = $this->drupalCreateNode($edit);
+    ));
 
     // Add a comment to the page.
     $comment = array(
@@ -137,4 +149,89 @@ class TrackerTest extends DrupalWebTestCase {
     $this->drupalGet('tracker');
     $this->assertText('1 new', t('New comments are counted on the tracker listing pages.'));
   }
+
+  /**
+   * Test that existing nodes are indexed by cron.
+   */
+  function testTrackerCronIndexing() {
+    $this->drupalLogin($this->user);
+
+    // Create 3 nodes.
+    $edits = array();
+    $nodes = array();
+    for ($i = 1; $i <= 3; $i++) {
+      $edits[$i] = array(
+        'comment' => 2,
+        'title' => $this->randomName(),
+      );
+      $nodes[$i] = $this->drupalCreateNode($edits[$i]);
+    }
+
+    // Add a comment to the last node as other user.
+    $this->drupalLogin($this->other_user);
+    $comment = array(
+      'subject' => $this->randomName(),
+      'comment' => $this->randomName(20),
+    );
+    $this->drupalPost('comment/reply/' . $nodes[3]->nid, $comment, t('Save'));
+
+    // Start indexing backwards from node 3.
+    variable_set('tracker_index_nid', 3);
+
+    // Clear the current tracker tables and rebuild them.
+    db_delete('tracker_node')
+      ->execute();
+    db_delete('tracker_user')
+      ->execute();
+    tracker_cron();
+
+    $this->drupalLogin($this->user);
+
+    // Fetch the user's tracker.
+    $this->drupalGet('tracker/' . $this->user->uid);
+
+    // Assert that all node titles are displayed.
+    foreach ($nodes as $i => $node) {
+      $this->assertText($node->title, t('Node @i is displayed on the tracker listing pages.', array('@i' => $i)));
+    }
+    $this->assertText('1 new', t('New comment is counted on the tracker listing pages.'));
+    $this->assertText('updated', t('Node is listed as updated'));
+
+
+    // Fetch the site-wide tracker.
+    $this->drupalGet('tracker');
+
+    // Assert that all node titles are displayed.
+    foreach ($nodes as $i => $node) {
+      $this->assertText($node->title, t('Node @i is displayed on the tracker listing pages.', array('@i' => $i)));
+    }
+    $this->assertText('1 new', t('New comment is counted on the tracker listing pages.'));
+  }
+
+  /**
+   * Test that publish/unpublish works at admin/content/node
+   */
+  function testTrackerAdminUnpublish() {
+    $admin_user = $this->drupalCreateUser(array('administer nodes'));
+    $this->drupalLogin($admin_user);
+
+    $node = $this->drupalCreateNode(array(
+      'comment' => 2,
+      'title' => $this->randomName(),
+    ));
+
+    // Assert that the node is displayed.
+    $this->drupalGet('tracker');
+    $this->assertText($node->title, t('Node is displayed on the tracker listing pages.'));
+
+    // Unpublish the node and ensure that it's no longer displayed.
+    $edit = array(
+      'operation' => 'unpublish',
+      'nodes[' . $node->nid . ']' => $node->nid,
+    );
+    $this->drupalPost('admin/content', $edit, t('Update'));
+
+    $this->drupalGet('tracker');
+    $this->assertText(t('No posts available.'), t('Node is displayed on the tracker listing pages.'));
+  }
 }