In the function comment_render(), a large and expensive GROUP BY clause exists that would usually serve to make sure only one copy of each comment is selected. The query doesn't use any LEFT JOIN or other construct that would return multiple comment results, and the query functions the same without the GROUP BY as it does with. Here is an example of the query:

SELECT c.cid as cid, c.pid, c.nid, c.subject, c.comment, c.format, c.timestamp, c.name, c.mail, c.homepage, u.uid, u.name AS registered_name, u.picture, u.data, c.score, c.users, c.thread, c.status 
FROM comments c 
INNER JOIN users u ON c.uid = u.uid 
WHERE c.nid = 1 
GROUP BY c.cid, c.pid, c.nid, c.subject, c.comment, c.format, c.timestamp, c.name, c.mail, u.picture, c.homepage, u.uid, u.name, u.picture, u.data, c.score, c.users, c.thread, c.status 
ORDER BY c.timestamp

I've not benchmarked but it seems obvious that if we can run the query without that clause, we should.

CommentFileSizeAuthor
comment_group_by.patch1.21 KBrobertDouglass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

robertDouglass’s picture

The query has some variations, depending on the circumstances and comment settings, but they all revolve around the ORDER BY clause. Here are some other possibilities:

ORDER BY c.timestamp DESC
ORDER BY c.thread DESC
ORDER BY c.timestamp
ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1))
ChrisKennedy’s picture

I was interested in why this was the case, so I tracked each revision to that line back to the original GROUP BY commit, which is here:
http://cvs.drupal.org/viewcvs/drupal/drupal/includes/comment.inc?r1=1.55...

It appears to be in there to allow moderation votes to be summed, as the relevant code is:

function comment_query($lid, $order, $pid = -1) {

  $query .= "SELECT u.*, c.*, SUM(m.score) / COUNT(m.cid) AS score, COUNT(m.cid) AS votes FROM comments c LEFT JOIN user u ON c.author = u.uid LEFT JOIN moderate m ON c.cid = m.cid WHERE c.lid = '$lid'";

The other main change is at http://cvs.drupal.org/viewcvs/drupal/drupal/includes/comment.inc?r1=1.60... where the grouping is expanded from cid to "c.cid, c.pid, c.lid, c.subject, c.comment, c.timestamp, u.uid, u.name". The comment for that commit is "Fixed the remaining SQL problems when running MySQL in ANSI mode. (moderation/comment related)". In ANSI SQL a GROUP BY clause must include all columns that may differ.

The GROUP BY on each column unfortunately stayed in even after the aggregation functions were removed along with comment scores back in December 2001: http://cvs.drupal.org/viewcvs/drupal/drupal/modules/comment/comment.modu....

So it looks like removing the GROUP BY is a good catch.

ChrisKennedy’s picture

In other words: on December 30th the GROUP BY clause would have unnecessarily been in comment.module for five years.

killes@www.drop.org’s picture

if I do an explain on the queries then the "bad stuff" changes from

Using where; Using temporary; Using filesort

to

Using where; Using filesort

which is probably an improvement

chx’s picture

*In a separate issue* it probably worth pursuing changing so an index is used.

robertDouglass’s picture

using ab -kc 10 -t 30 on a page displaying a node that has 100 comments, 50 of which are shown, here are the results:

Average With GROUP BY: 16.94 requests per second
Average without GROUP BY: 20.90 requests per second

That's a speed increase of 23.4%.

killes@www.drop.org’s picture

Status: Needs review » Reviewed & tested by the community

yay! I'll backport as soon as this hits 5.x.

chx’s picture

Priority: Normal » Critical

Removing unncessary code causes 24% speedup. I deem this critical AND ready.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

robertDouglass’s picture

Status: Fixed » Patch (to be ported)
robertDouglass’s picture

Version: 5.x-dev » 4.7.x-dev
killes@www.drop.org’s picture

Status: Patch (to be ported) » Fixed

done

ChrisKennedy’s picture

This bug was reported and CRNed 3 months earlier btw: http://drupal.org/node/83560

Anonymous’s picture

Status: Fixed » Closed (fixed)