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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wesley Tanaka’s picture

The addition of this index will optimize the query away:

ALTER TABLE node_comment_statistics ADD INDEX (comment_count);

brianV’s picture

Version: 6.6 » 7.x-dev
Status: Active » Needs review
FileSize
909 bytes

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

FiReaNGeL’s picture

Can we see the explain after the index addition Welsey?

brianV’s picture

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
brianV’s picture

Issue tags: +Performance

Just tagging

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
FiReaNGeL’s picture

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?

brianV’s picture

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.

FiReaNGeL’s picture

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

Dries’s picture

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

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

Oups.

brianV’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

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

Damien Tournoud’s picture

Status: Needs review » Needs work

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

brianV’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Damien Tournoud’s picture

Status: Fixed » Patch (to be ported)

Let's backport this.

Damien Tournoud’s picture

Status: Patch (to be ported) » Needs work

Why is the update function in system.install?

brianV’s picture

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?

catch’s picture

Component: search.module » comment.module
Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Quick fix
FileSize
1.34 KB

Yep.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks!

catch’s picture

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

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

brianV’s picture

Oops, forgot to attach the patch...

catch’s picture

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

catch’s picture

Status: Needs review » Needs work
brianV’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
1.34 KB

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.

catch’s picture

Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
FileSize
718 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.

catch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.37 KB

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

catch’s picture

Tagging for 'forward port'.

kehan’s picture

subscribing and hope this makes it into D7 soon

catch’s picture

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

Gábor Hojtsy’s picture

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.

catch’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D6, -Quick fix, -needs forward port to Drupal 7 +D7 upgrade path
FileSize
718 bytes

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

webchick’s picture

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.