Performance: SELECT MAX(comment_count) FROM node_comment_statistics does full table scan

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

Just tagging

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?

Oups.

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

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

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

Committed to CVS HEAD. Thanks!

Let's backport this.

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?

Yep.

Committed to CVS HEAD. Thanks!

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

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

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.

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.

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.

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

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

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