Doing bulk deletion of users in a cron job, I was only getting 290-300 users deleted per run (240 seconds). It turned out that 95% of the time was being spent in comment_user(), which updates {comments} and {node_comment_statistics} on their respective uid columns. This is in an environment with no comments, but 138,000 nodes (thus {node_comment_statistics} has 138,000 rows).

The attached patch indexes the respective uid columns (I know {comments}, being empty, didn't contribute to my issue, but adding that as well seems like a no-brainer). After applying the patch and running the update process, my next cron batch deleted 3900 users.

Files: 
CommentFileSizeAuthor
#21 comment_index_update_update.patch859 bytescatch
PASSED: [[SimpleTest]]: [MySQL] 37,558 pass(es).
[ View ]
#19 indexes.diff1.18 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch indexes.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 comment_index_D7.patch859 bytescatch
#16 comment_index_289504_D6.patch1.37 KBcatch
#14 comment_index_289504_D6.patch1.37 KBcatch
#14 comment_index_7.patch859 bytescatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_index_7.patch.
[ View ]
#11 comment_index_289504.patch1.54 KBmikeryan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_index_289504.patch.
[ View ]
#4 uid_index.diff1.9 KBmoshe weitzman
Passed: 11824 passes, 0 fails, 0 exceptions
[ View ]
#1 comment.install.patch1.19 KBmikeryan
Failed: Failed to apply patch.
[ View ]
comment.install.patch661 bytesmikeryan
Failed: Failed to apply patch.
[ View ]

Comments

mikeryan’s picture

StatusFileSize
new1.19 KB
Failed: Failed to apply patch.
[ View ]

Always forget to add my changes into the schema hook as well...

frankcarey’s picture

no comments? bump! any reason not to do this?

robertDouglass’s picture

Is this applicable to D7?

moshe weitzman’s picture

Version:6.3» 7.x-dev
Assigned:Unassigned» moshe weitzman
Issue tags:+Performance
StatusFileSize
new1.9 KB
Passed: 11824 passes, 0 fails, 0 exceptions
[ View ]

Here is a D7 patch. In addition to these 2 indexes, I added foreign keys to user.uid and moved the misplaced comment_update_7005() back into the updates defgroup where it belongs. no changes to comment_update_7005()

catch’s picture

subscribing, looks rtbc but could do with EXPLAIN output.

moshe weitzman’s picture

Here is an EXPLAIN for the delete query in comment_user_cancel() before and then after this patch.

1 SIMPLE c ALL NULL NULL NULL NULL 1 Using where

1 SIMPLE c ref comment_uid comment_uid 4 const 1 Using index

catch’s picture

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

Status:Reviewed & tested by the community» Fixed

Committed to HEAD! Thanks!

moshe weitzman’s picture

Version:7.x-dev» 6.x-dev
Status:Fixed» Reviewed & tested by the community

RTBC for D6. see patch in #1

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs work

The comment update functions should have a clear indication of when they were added. Ie. before Drupal 6.0, inbetween Drupal 6.0 to 7.0. etc. See http://drupal.org/node/278592#comment-2038486

mikeryan’s picture

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_index_289504.patch.
[ View ]

Rerolled with @defgroup updates-6.x-extra.

catch’s picture

Just caught user deletions scanning 21 million rows in node_comment_statistics. This no longer applies, re-roll forthcoming.

Status:Needs review» Needs work

The last submitted patch, comment_index_289504.patch, failed testing.

catch’s picture

Priority:Normal» Major
Status:Needs work» Needs review
Issue tags:+Needs forward port to D7
StatusFileSize
new859 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_index_7.patch.
[ View ]
new1.37 KB

Here's the re-roll. The index in D7 is called 'comment_uid', updated the patch to reflect this.

Also adding a D7 patch to ensure that upgrade does not attempt to add the index twice.

Upgrading to major, spammers get deleted a lot.

Status:Needs review» Needs work

The last submitted patch, comment_index_7.patch, failed testing.

catch’s picture

Status:Needs work» Needs review
StatusFileSize
new1.37 KB
new859 bytes

la la la.

moshe weitzman’s picture

Status:Needs review» Reviewed & tested by the community

Looks like a good backport to me.

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs work

Did not apply anymore due to #336483: Performance: SELECT MAX(comment_count) FROM node_comment_statistics does full table scan changing indexes also in node comment stats. Needs a reroll.

moshe weitzman’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch indexes.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled and tested a new install.

Gábor Hojtsy’s picture

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

Committed to Drupal 6. Needs updates in Drupal 7 to fix update path.

catch’s picture

Status:Patch (to be ported)» Needs review
Issue tags:-Needs forward port to D7+D7 upgrade path
StatusFileSize
new859 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,558 pass(es).
[ View ]

Re-rolled the D7 patch.

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.