Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
comment_group_by.patch | 1.21 KB | robertDouglass | |
Comments
Comment #1
robertDouglass CreditAttribution: robertDouglass commentedThe 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:
Comment #2
ChrisKennedy CreditAttribution: ChrisKennedy commentedI 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:
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.
Comment #3
ChrisKennedy CreditAttribution: ChrisKennedy commentedIn other words: on December 30th the GROUP BY clause would have unnecessarily been in comment.module for five years.
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedif 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
Comment #5
chx CreditAttribution: chx commented*In a separate issue* it probably worth pursuing changing so an index is used.
Comment #6
robertDouglass CreditAttribution: robertDouglass commentedusing 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%.
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedyay! I'll backport as soon as this hits 5.x.
Comment #8
chx CreditAttribution: chx commentedRemoving unncessary code causes 24% speedup. I deem this critical AND ready.
Comment #9
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #10
robertDouglass CreditAttribution: robertDouglass commentedComment #11
robertDouglass CreditAttribution: robertDouglass commentedComment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commenteddone
Comment #13
ChrisKennedy CreditAttribution: ChrisKennedy commentedThis bug was reported and CRNed 3 months earlier btw: http://drupal.org/node/83560
Comment #14
(not verified) CreditAttribution: commented