I was trying to figure out why file deletions were so slow on my site. After running SHOW PROCESSLIST a few times it became apparent that there were a lot of queries like UPDATE users SET picture='0' WHERE (picture = '98529') that were taking multiple seconds. I traced the source of that back to user_file_delete() which tries to make sure there are no user records pointing at the file. Sadly there's no index on the picture column so this takes for ever.

I think there's two approaches the simplest it to just add the index. I've got a patch for that for 7.x. The other would be to try to look up the uid from the fid in {file_usage}. I'm not certain that would work and it seems a bit more complicated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

FileSize
1.03 KB
drewish’s picture

Status: Active » Needs review
FileSize
825 bytes

Here's one for D8 but since I've got no idea where updates have gone to in D8 so it probably needs work.

drewish’s picture

FileSize
825 bytes

I guess the D8 doesn't work quite right... come on test bot!

moshe weitzman’s picture

Status: Needs review » Needs work

Nice patch ... Needs an update function.

drewish’s picture

Status: Needs work » Needs review
FileSize
1.12 KB

Went ahead and rolled a patch with the update but I'm not totally sure it's necessary. Upgraded sites should get the index added in D7, new sites should have it added when the schema is defined. At least back in D6 we didn't updates from the last major version, not HEAD to HEAD updates...

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

By using !db_index_exists(), this update function can be in both D7 and D8 with no problem.

catch’s picture

Version: 8.x-dev » 7.x-dev
Priority: Normal » Critical
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Novice

I've committed/pushed #4 to 8.x, while we can use db_index_exists() for index additions safely, there's no need for the 8.x update until we support head-to-head updates.

Moving to D7 and bumping as critical so this gets resolved before we have to re-introduce the update in 8.x though. Backport should be straightforward so tagging as novice.

tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
839 bytes

I think this is the right group.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Assume this will come back green....

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, drupal-1348758-8.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7

#8: drupal-1348758-8.patch queued for re-testing.

tim.plunkett’s picture

FYI: The failure in #10 was "Drupal installation failed" and a retest worked fine.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Needs tests.

marcingy’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
marcingy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

cross post

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.48 KB
2.07 KB

Okay, here's an update test.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

The upgrade path test looks great.

I also tested the patch manually:

  1. Generated 50 users manually with devel.
  2. Applied the patch.
  3. Ran update.php.
  4. Confirmed that the user picture data was intact and that there was then an index on the column.
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, an upgrade path + tests + manual tests. It's like xmas in April! :D Awesome work, folks!

Committed and pushed to 7.x. Thanks! :)

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

Anonymous’s picture

Issue summary: View changes

Closing my a tag.