Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
An issue when running Drupal with millions of nodes:
This query in node_update_index() does a full table scan
mysql> EXPLAIN SELECT MAX(comment_count) FROM d_node_comment_statistics \G
*************************** 1. row ***************************
id: 1
select_type: SIMPLE
table: d_node_comment_statistics
type: ALL
possible_keys: NULL
key: NULL
key_len: NULL
ref: NULL
rows: 2435440
Extra:
Comment | File | Size | Author |
---|---|---|---|
#36 | comment_count_update_update.patch | 718 bytes | catch |
#31 | 336483-add-comment-count-index-rev2-d6.patch | 1.37 KB | catch |
#29 | comment_count_d7.patch | 718 bytes | catch |
#28 | 336483-add-comment-count-index-rev4.patch | 1.34 KB | brianV |
#28 | 336483-add-comment-count-index-rev2-d6.patch | 1.37 KB | brianV |
Comments
Comment #1
Wesley Tanaka CreditAttribution: Wesley Tanaka commentedThe addition of this index will optimize the query away:
ALTER TABLE node_comment_statistics ADD INDEX (comment_count);
Comment #2
brianV CreditAttribution: brianV commentedPatch attached adds a hook_update_70XX() implementation which adds this index.
Comment #3
FiReaNGeL CreditAttribution: FiReaNGeL commentedCan we see the explain after the index addition Welsey?
Comment #4
brianV CreditAttribution: brianV commentedI'm not Wesley, but...
Comment #5
brianV CreditAttribution: brianV commentedJust tagging
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #7
FiReaNGeL CreditAttribution: FiReaNGeL commentedbrianV, was that EXPLAIN on a table with a significant amount of nodes / comments? To me it looks like on a brand new installation, which doesnt mean much?
Comment #8
brianV CreditAttribution: brianV commentedFireangel,
That was taken from Wesley Tanaka's page on the subject. I am providing his data, which he says is based upon a node database of 2.4million nodes.
See http://wtanaka.com/drupal/million-nodes-6 - there is an entry for this bug.
Comment #9
FiReaNGeL CreditAttribution: FiReaNGeL commentedExcellent! No further objections / comments here, and it's already RTBC
Comment #10
Dries CreditAttribution: Dries commentedWell, but shouldn't we update the schema definition in modules/comment/comment.install too? The index is not being created for new installs?
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedOups.
Comment #12
brianV CreditAttribution: brianV commentedBig time oops. Rerolled with the proper addition to comment_schema().
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks Brian, but please remove the useless "&".
Comment #14
brianV CreditAttribution: brianV commentedWow - I am off my game this morning...
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedComment #16
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #17
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's backport this.
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhy is the update function in system.install?
Comment #19
brianV CreditAttribution: brianV commentedLooks like I am more off my game than I thought today.
There is absolutely no good reason why I put this in system.install instead of comment.install.
What's the best way to correct this? Move the hook_update_N to the correct file in another patch?
Comment #20
catchYep.
Comment #21
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #22
catchComment #23
brianV CreditAttribution: brianV commentedThanks for cleaning that up, catch.
I've attached a patch for D6, but this obviously has implications for D7. Ie, if the index already exists, calling the 'CREATE INDEX' again during the upgrade to D7 will throw errors.
So, I assume we should also remove comment_update_7011() altogether if this goes into D6? (This is assuming that all the hook_update_60XX() functions are run before the hook_update_70XX() functions...)
Comment #24
brianV CreditAttribution: brianV commentedOops, forgot to attach the patch...
Comment #25
catchSo the situation with 6/7 updates is this:
1. Commit to D7
2. Commit to D6 in a 6.x-7.x extras defgroup (so #24 is CNW unfortunately)
3. Move back to D7 - patch would need to add the update added in D6 and remove comment_update_7011()
Comment #27
catchComment #28
brianV CreditAttribution: brianV commentedOk, as per catch's instructions, I've added updated D6 and D7 patches.
D6 patch:
1. Created a defgroup' updates-6.x-extra Extra system updates for 6.x'
2. Added this changes as comment_update_6004()
3. Added the index to the comment_node_statistics schema definition.
D7 patch:
1. Added the same defgroup I added to D6, with comment_update_6004()
2. Removed comment_update_7011() completely.
Comment #29
catchThe Drupal 6 patch still applies and looks good.
The D7 patch needed updating, fortunately since this issue was last updated we have the lovely new db_index_exists() function, so we can easily account for whether the index exists or not without any messing about.
Marking this RTBC for 6. Once it's committed, we'll need to move the issue to D7 to apply the follow-up patch there.
Comment #31
catchRe-uploading the 6.x patch for the bot.
Comment #32
catchTagging for 'forward port'.
Comment #33
kehan CreditAttribution: kehan commentedsubscribing and hope this makes it into D7 soon
Comment #34
catchThis is already in D7. The d7 patch is for cleanup after a D6 commit.
Comment #35
Gábor HojtsyThanks, committed to D6. Back to D7 for cleanup.
Comment #36
catchRe-uploading the 7.x patch. Still major - this will throw errors now if sites upgrade to 7.x from latest 6.x dev.
Comment #37
webchick/me violates the cardinal rule of committing patches from needs review :\
However, it seems we need this to avoid causing problems in the upgrade path from the latest version of D6 to D7. The patch is simply wrapping a part of the upgrade path in in "if exists" check so "should" be safe (famous last words...)
Committed and pushed to 7.x. Thanks!