Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
the comment table can use an indes on status and pid.
Comment | File | Size | Author |
---|---|---|---|
#24 | comment_indexes-243093-d6-24.patch | 2.03 KB | Albert Volkman |
#22 | comment_indexes-243093-d6-22.patch | 2.03 KB | Albert Volkman |
#20 | comment_indexes-243093-d6-20.patch | 2.03 KB | Albert Volkman |
#17 | typo.patch | 782 bytes | catch |
#12 | comment_indexes.patch | 2.25 KB | catch |
Comments
Comment #1
nnewton CreditAttribution: nnewton commented+1 ing this. We have added an index on (pid,status) for drupal.org to speed up several queries across the comments table.
Comment #2
David StraussI'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.
Comment #3
meba CreditAttribution: meba commentedI support this...This really speeds some things up.
Comment #4
meba CreditAttribution: meba commentedComment #5
Dave ReidMarked #101322: comments are searched by not indexed field pid as a duplicate of this issue.
Comment #6
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedHere's a patch. While I agree in principle with David, it doesn't hurt to pick some low hanging grapes sometimes.
Comment #7
catchThe 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.
Looks better. Patch needs to be rolled from root though.
Comment #8
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedrerolled patch.
Comment #9
catchIf 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:
Comment #10
catchwrong patch.
Comment #12
catchminus the syntax error.
Comment #13
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI 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.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #15
catchMoving back to D6 for consideration.
Comment #16
wedge CreditAttribution: wedge commentedisn't there a small typo in comment_update_7003?
- db_drop_index($rest, 'comment', 'pid');
+ db_drop_index($ret, 'comment', 'pid');
Comment #17
catchYes there is.
Comment #18
Dries CreditAttribution: Dries commentedThanks!
Comment #19
catchComment #20
Albert Volkman CreditAttribution: Albert Volkman commentedD6 backport.
Comment #21
rjbrown99 CreditAttribution: rjbrown99 commented#20 - I think it would be comment_update_6006(), no? In the patch it's 7006.
Comment #22
Albert Volkman CreditAttribution: Albert Volkman commentedYou are correct, sir.
Comment #23
rjbrown99 CreditAttribution: rjbrown99 commentedPatch 22 is incorrect - the table name in the patch is "comment" when it should be "comments".
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedThird time's a charm.