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:

Files: 
CommentFileSizeAuthor
#36 comment_count_update_update.patch718 bytescatch
PASSED: [[SimpleTest]]: [MySQL] 37,610 pass(es).
[ View ]
#31 336483-add-comment-count-index-rev2-d6.patch1.37 KBcatch
#29 comment_count_d7.patch718 bytescatch
#28 336483-add-comment-count-index-rev4.patch1.34 KBbrianV
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 336483-add-comment-count-index-rev4.patch.
[ View ]
#28 336483-add-comment-count-index-rev2-d6.patch1.37 KBbrianV
#24 336483-add-comment-count-index-d6.patch1.14 KBbrianV
#20 comment.patch1.34 KBcatch
Passed on all environments.
[ View ]
#14 336483-add-index-for-comment-count-rev3.patch1.68 KBbrianV
Passed: 11905 passes, 0 fails, 0 exceptions
[ View ]
#12 336483-add-index-for-comment-count-rev2.patch1.68 KBbrianV
Passed: 11876 passes, 0 fails, 0 exceptions
[ View ]
#2 336483-add-index-for-comment-count.patch909 bytesbrianV
Passed: 11905 passes, 0 fails, 0 exceptions
[ View ]

Comments

The addition of this index will optimize the query away:

ALTER TABLE node_comment_statistics ADD INDEX (comment_count);

Version:6.6» 7.x-dev
Status:Active» Needs review
StatusFileSize
new909 bytes
Passed: 11905 passes, 0 fails, 0 exceptions
[ View ]

Patch attached adds a hook_update_70XX() implementation which adds this index.

Can we see the explain after the index addition Welsey?

I'm not Wesley, but...

explain MAX(comment_count)
FROM d_node_comment_statistics \G
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: NULL
         type: NULL
possible_keys: NULL
          key: NULL
      key_len: NULL
          ref: NULL
         rows: NULL
        Extra: Select tables optimized away

Issue tags:+Performance

Just tagging

Status:Needs review» Reviewed & tested by the community

brianV, 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?

Fireangel,

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.

Excellent! No further objections / comments here, and it's already RTBC

Well, but shouldn't we update the schema definition in modules/comment/comment.install too? The index is not being created for new installs?

Status:Reviewed & tested by the community» Needs work

Oups.

Status:Needs work» Needs review
StatusFileSize
new1.68 KB
Passed: 11876 passes, 0 fails, 0 exceptions
[ View ]

Big time oops. Rerolled with the proper addition to comment_schema().

Status:Needs review» Needs work

Thanks Brian, but please remove the useless "&".

Status:Needs work» Needs review
StatusFileSize
new1.68 KB
Passed: 11905 passes, 0 fails, 0 exceptions
[ View ]

Wow - I am off my game this morning...

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Committed to CVS HEAD. Thanks!

Status:Fixed» Patch (to be ported)

Let's backport this.

Status:Patch (to be ported)» Needs work

Why is the update function in system.install?

Looks 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?

Component:search.module» comment.module
Status:Needs work» Needs review
Issue tags:+needs backport to D6, +Quick fix
StatusFileSize
new1.34 KB
Passed on all environments.
[ View ]

Yep.

Status:Needs review» Fixed

Committed to CVS HEAD. Thanks!

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

Status:Patch (to be ported)» Needs review

Thanks 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...)

StatusFileSize
new1.14 KB

Oops, forgot to attach the patch...

So 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()

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
new1.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 336483-add-comment-count-index-rev4.patch.
[ View ]

Ok, 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.

Priority:Normal» Major
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new718 bytes

The 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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 336483-add-comment-count-index-rev4.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.37 KB

Re-uploading the 6.x patch for the bot.

Tagging for 'forward port'.

subscribing and hope this makes it into D7 soon

This is already in D7. The d7 patch is for cleanup after a D6 commit.

Version:6.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Thanks, committed to D6. Back to D7 for cleanup.

Status:Patch (to be ported)» Needs review
Issue tags:-needs backport to D6, -Quick fix, -Needs forward port to D7+D7 upgrade path
StatusFileSize
new718 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,610 pass(es).
[ View ]

Re-uploading the 7.x patch. Still major - this will throw errors now if sites upgrade to 7.x from latest 6.x dev.

Status:Needs review» Fixed

/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!

Status:Fixed» Closed (fixed)
Issue tags:-Performance, -D7 upgrade path

Automatically closed -- issue fixed for 2 weeks with no activity.