Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nnewton’s picture

+1 ing this. We have added an index on (pid,status) for drupal.org to speed up several queries across the comments table.

David Strauss’s picture

I've been told the query in question is "select count(cid) from comments where pid = X and status = X." If that's the case, I support this index. However, the better performance gain would be from keeping updated counts. MySQL still has to count the rows, even if an index can hand it the rows on a platter.

To put the performance significance of this into a metaphor:
Current status: Counting the yellow M&Ms in a jar with all colors.
Post-index status: Counting the yellow M&Ms with the yellow ones already organized in a separate pile.
Best status: Running tally of M&Ms by color.

meba’s picture

I support this...This really speeds some things up.

meba’s picture

Issue tags: +Performance
Dave Reid’s picture

Component: database system » comment.module

Marked #101322: comments are searched by not indexed field pid as a duplicate of this issue.

killes@www.drop.org’s picture

Status: Active » Needs review
FileSize
1.05 KB

Here's a patch. While I agree in principle with David, it doesn't hurt to pick some low hanging grapes sometimes.

catch’s picture

Status: Needs review » Needs work

The place this is used in core is comment_num_replies.

I'm wondering if we could get better queries for comment_num_new with nid, pid,

before.

mysql> EXPLAIN SELECT COUNT(cid) FROM comment WHERE pid = 1 AND status = 1;
+----+-------------+---------+-------------+---------------+------------+---------+------+------+-------------------------------------------------------+
| id | select_type | table   | type        | possible_keys | key        | key_len | ref  | rows | Extra                                                 |
+----+-------------+---------+-------------+---------------+------------+---------+------+------+-------------------------------------------------------+
|  1 | SIMPLE      | comment | index_merge | pid,status    | status,pid | 1,4     | NULL |    1 | Using intersect(status,pid); Using where; Using index | 
+----+-------------+---------+-------------+---------------+------------+---------+------+------+-------------------------------------------------------+
1 row in set (0.00 sec)
mysql> EXPLAIN SELECT COUNT(cid) FROM comment WHERE pid = 1 AND status = 1;
+----+-------------+---------+------+------------------------+--------------------+---------+-------------+------+-------------+
| id | select_type | table   | type | possible_keys          | key                | key_len | ref         | rows | Extra       |
+----+-------------+---------+------+------------------------+--------------------+---------+-------------+------+-------------+
|  1 | SIMPLE      | comment | ref  | pid,comment_pid_status | comment_pid_status | 5       | const,const |    2 | Using index | 
+----+-------------+---------+------+------------------------+--------------------+---------+-------------+------+-------------+
1 row in set (0.00 sec)

Looks better. Patch needs to be rolled from root though.

killes@www.drop.org’s picture

Status: Needs work » Needs review
FileSize
698 bytes

rerolled patch.

catch’s picture

FileSize
1.05 KB

If we're adding a pid, status index, I don't see any reason to keep the separate index on pid.

Don't want to hijack the issue so let me know if I should open a separate one, but comment_num_new looks like it could be simplified a bit as well:

mysql> EXPLAIN SELECT COUNT(c.cid) FROM node n INNER JOIN comment c ON n.nid = c.nid WHERE n.nid = 1 AND timestamp > '123456789' AND c.status = 1;
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
| id | select_type | table | type  | possible_keys | key     | key_len | ref   | rows | Extra       |
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
|  1 | SIMPLE      | n     | const | PRIMARY       | PRIMARY | 4       | const |    1 | Using index | 
|  1 | SIMPLE      | c     | ref   | nid,status    | nid     | 4       | const |    2 | Using where | 
+----+-------------+-------+-------+---------------+---------+---------+-------+------+-------------+
2 rows in set (0.03 sec)
mysql> EXPLAIN SELECT COUNT(cid) FROM comment WHERE nid = 1 AND timestamp > 123456789 AND status = 1;
+----+-------------+---------+------+---------------------+-----------------+---------+-------+------+--------------------------+
| id | select_type | table   | type | possible_keys       | key             | key_len | ref   | rows | Extra                    |
+----+-------------+---------+------+---------------------+-----------------+---------+-------+------+--------------------------+
|  1 | SIMPLE      | comment | ref  | nid,comment_num_new | comment_num_new | 4       | const |    2 | Using where; Using index | 
+----+-------------+---------+------+---------------------+-----------------+---------+-------+------+----
catch’s picture

FileSize
2.26 KB

wrong patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

minus the syntax error.

killes@www.drop.org’s picture

I agree with the added indices and the change of the query. We are already using a similar index as comment_num_new on drupal.org.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD.

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Moving back to D6 for consideration.

wedge’s picture

isn't there a small typo in comment_update_7003?
- db_drop_index($rest, 'comment', 'pid');
+ db_drop_index($ret, 'comment', 'pid');

catch’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
Issue tags: +Quick fix
FileSize
782 bytes

Yes there is.

Dries’s picture

Status: Needs review » Fixed

Thanks!

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)
Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.03 KB

D6 backport.

rjbrown99’s picture

#20 - I think it would be comment_update_6006(), no? In the patch it's 7006.

Albert Volkman’s picture

You are correct, sir.

rjbrown99’s picture

Patch 22 is incorrect - the table name in the patch is "comment" when it should be "comments".

Albert Volkman’s picture

Third time's a charm.

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.