This bug seems to have been introduced in #292984: Add cid to node_comment_statistics.

Instead of showing recent comments, it shows all comments (no matter how old) for the most recently-commented-on node, then the next newest node, and so on.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

FileSize
860 bytes

This is the current query:

+++ modules/comment/comment.module	10 Nov 2009 19:50:55 -0000
@@ -371,44 +371,29 @@ function comment_permalink($comment) {
+  $query = db_select('comment', 'c');
+  $query->innerJoin('node', 'n', 'n.nid = c.nid');
+  $query->innerJoin('node_comment_statistics', 'ncs', 'ncs.nid = c.nid');
+  $query->addTag('node_access');
+  $comments = $query
+    ->fields('c')
+    ->condition('ncs.comment_count', 0, '>')
+    ->condition('c.status', COMMENT_PUBLISHED)
+    ->condition('n.status', NODE_PUBLISHED)
+    ->orderBy('ncs.last_comment_timestamp', 'DESC')
+    ->orderBy('c.cid', 'DESC')
+    ->range(0, $number)
+    ->execute()
+    ->fetchAll();

Actually, upon looking at this again, I've no idea why this is joining on node_comment_statistics at all.

Someone should run the patched query against an SQL EXPLAIN. We might need an index on comment.created.

sun’s picture

Status: Active » Needs review
grendzy’s picture

Status: Needs review » Needs work
FileSize
12.5 KB

Here's the explain: I agree on both points, we should have an index, and I can't find a reason to join node_comment_statistics.

grendzy’s picture

Status: Needs work » Needs review
FileSize
15.5 KB
1.9 KB
1.63 KB
grendzy’s picture

FileSize
1.63 KB

go go gadget testbot!

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to fly for both D8 and D7 to me.

catch’s picture

Issue tags: +Needs backport to D7
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed these patches to 7.x and 8.x, respectively. Thanks all.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

catch’s picture

This patch is causing random test failures for both D7 and D8, not re-opening the issue, but #1188880: Comment module test fails randomly is where we're working on it.

catch’s picture

Status: Closed (fixed) » Active

Actually I am re-opening it, because that issue is critical, but we need to discuss whether that index is really necessary or not.

msonnabaum’s picture

It appears this change introduced a performance regression.

I was seeing this query take over a second, and I noticed that it hits the node table first, so it can't use comment's indexes for the sort, leading to "Using temporary; Using filesort", on some potentially very big tables.

Adding a STRAIGHT_JOIN reverses the order so that the comment indexes get used first, which removes the filesort and tmp table as well as only scans the 10 rows it needs. I just don't know how or if that's a possibility with DBTNG, so I'll need some help there.

grendzy’s picture

Odd. When I test, the filesort only occurs with #1188880: Comment module test fails randomly applied, and the results are different (the join order isn't affected, but I do get a filesort on the comment table) If I rollback #1188880 it's back to the EXPLAIN result in #4. Could this be caused by differences in the query optimizer between MariaDB and MySQL? I have 5.1.36.

Liam McDermott’s picture

I just tried this on MySQL 5.0.96 and 5.1.63 and am seeing a temp table and filesort, same as #12.

This is such a bad problem on the test site I'm helping a client with the temp table grows >2GB in size before it runs out of room in the /tmp partition, then throws an error that makes the site inaccessible (after waiting several minutes for the page to load). In case anyone is interested, the site has around 160,000 nodes and 800,000 comments. Once the query is cached however, the page loads in a second or two (sorry for the lack of hard figures, but the difference is loading times of seconds versus minutes so I haven't bothered with benchmarking).

It might seem like heresy to do it this way, but this query avoids filesorts:

EXPLAIN SELECT c.* FROM comment c where c.nid in (select n.nid from node n where n.nid = c.nid and n.status = '1') and  (c.status = '1') ORDER BY c.created DESC, c.cid DESC LIMIT 10 OFFSET 0;

idselect_typetabletypepossible_keyskeykey_lenrefrowsExtra
1PRIMARYcindexNULLcomment_created4NULL801179Using where
2DEPENDENT SUBQUERYneq_refPRIMARY,node_status_typePRIMARY4wabtest_drutest.c.nid1Using where

Adding a STRAIGHT_JOIN reverses the order so that the comment indexes get used first, which removes the filesort and tmp table as well as only scans the 10 rows it needs. I just don't know how or if that's a possibility with DBTNG, so I'll need some help there.

I'm pretty sure it's not possible with DBTNG, as it stands. That's what prompted me to start thinking of other ways of avoiding the creation of tmp tables and filesorts.

I can confirm your STRAIGHT_JOIN addition works for me, too.

alexpott’s picture

Issue summary: View changes
Status: Active » Closed (fixed)

This is now a view

lizzjoy’s picture

Issue summary: View changes
lizzjoy’s picture

I changed comments from Full to Filtered HTML since #2378211: no access to issue comments was posted.