Slow performance on upload_load() query [#73045]

The query in update_load() performs an unnecessary ORDER BY. The results are placed into an associative array where order is unimportant, and the ORDER BY clause causes mysql to create a temporary table to do the sort which costs a significant amount of time over a simple select without the sort.

Also, there is no index on the vid key in the file_revisions table, causing the query to run much slower. On powerful hardware (quad xeon running at 3 ghz with 4G ram), these load queries take 0.3 seconds to perform on mysql 5.0.22 without the help of the mysql query cache when the file revisions table has 60k entries.

I'm not sure how to submit a db schema adjustement for this so I will attach it to this issue. The code for mysql for the index that is needed is:

ALTER TABLE file_revisions ADD INDEX (vid);

Using the second half of the primary key is horrendously slow as compared to using a direct index into the table. These two changes combined cause this query to take less than 0.01 seconds.

Attached patch generated against drupal 4.7.0 using:
diff upload.module fixed_upload.module > upload_load_module.patch

Comment	File	Size	Author
#24	upload_load_fix.diff.txt	1.55 KB	davemicc
#18	upload_load_fix_1.patch	2.46 KB	jhenry
#18
#16	upload_load_fix_0.patch	796 bytes	jhenry
#16
#5	upload_load_fix.patch	1.54 KB	jhenry
#5
#3	updates_11.patch	437 bytes	jhenry
#3
#2	updates_10.patch	415 bytes	jhenry
#2
	upload_load_module.patch	290 bytes	jhenry

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dries CreditAttribution: Dries commented 10 July 2006 at 19:09

Status:

Reviewed & tested by the community

» Needs work

To submit a SQL schema update:

1. update the database schemas in the database directories.
2. add an entry to updates.inc.

Next, roll a patch.

Comment #2

jhenry CreditAttribution: jhenry commented 10 July 2006 at 19:23

File	Size
updates_10.patch	415 bytes

Patch rolled against 4.7.2 and attached.

Comment #3

jhenry CreditAttribution: jhenry commented 10 July 2006 at 19:26

File	Size
updates_11.patch	437 bytes

fixed sql patch, missing return.

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 10 July 2006 at 19:31

use unified diff format. see http://drupal.org/diffandpatch (and see the handbook pages above this one)

Comment #5

jhenry CreditAttribution: jhenry commented 10 July 2006 at 21:15

File	Size
upload_load_fix.patch	1.54 KB

udiff against current cvs attached

Comment #6

jhenry CreditAttribution: jhenry commented 10 July 2006 at 21:24

Status:

Needs work

» Needs review

Comment #7

dopry CreditAttribution: dopry commented 10 July 2006 at 21:28

RE: #5

+1.
Code style looks good. It is indeed a frivolous ORDER BY clause as far as I can tell.
And that indexed VID is a good idea.

Comment #8

drumm

he/him

NY, US

CreditAttribution: drumm commented 11 July 2006 at 07:22

Status:

Needs review

» Needs work

This is why the order by exists: http://drupal.org/node/57307. (Feel free to re-status and explain why if you think no ordering is still better.)

Comment #9

Dries CreditAttribution: Dries commented 11 July 2006 at 08:10

Maybe with the index, the query is already a lot faster? This is a simple query, and should be fast -- both with and without the 'order by'-clause.

Comment #10

jhenry CreditAttribution: jhenry commented 11 July 2006 at 14:42

The main problem with doing the order by in MySQL is that it creates a temporary table to do the sort because it doesn't use the file id index to generate the result set. It is much faster to do a ksort in php than to create the temporary table in MySQL.

Numbers on my hardware:
1. With no index and order by on the query: 0.3 seconds
2. With no index and no order by on the query: 0.12 seconds
3. With an index and no order by on the query: < 0.01 seconds

ksort in php: < 0.01 seconds

Comment #11

jhenry CreditAttribution: jhenry commented 11 July 2006 at 14:44

The last number was supposed to be less than 0.01 in my above comment. I guess the comment form doesn't like greater-than signs.

Comment #12

dopry CreditAttribution: dopry commented 11 July 2006 at 16:55

@jhenry: what is it with the index and order?

Comment #13

jhenry CreditAttribution: jhenry commented 11 July 2006 at 17:33

0.01 to 0.05 seconds depending on load (potentially much greater if the temp table memory cache is full, but that's a db issue). I'd be satisfied with only adding the index to the table for performance if the order is important to other users.

Comment #14

jhenry CreditAttribution: jhenry commented 13 July 2006 at 19:51

Does drupal want this database fix if I keep the ORDER BY?

Comment #15

dopry CreditAttribution: dopry commented 13 July 2006 at 22:06

I say index it, but keep the order by.

Comment #16

jhenry CreditAttribution: jhenry commented 14 July 2006 at 18:27

Status:

Needs work

» Needs review

File	Size
upload_load_fix_0.patch	796 bytes

New patch with order by optimization removed.

Comment #17

drumm

he/him

NY, US

CreditAttribution: drumm commented 14 July 2006 at 18:36

Version:	4.7.0	» x.y.z
Status:	Needs review	» Needs work

Looks good, but we need a change in either upload_install() or system_install() (wherever the CREATE TABLE lives)

Comment #18

jhenry CreditAttribution: jhenry commented 14 July 2006 at 18:49

Status:

Needs work

» Needs review

File	Size
upload_load_fix_1.patch	2.46 KB

updated schemas

Comment #19

jhenry CreditAttribution: jhenry commented 18 July 2006 at 16:04

Anyone have comments on the patch?

Comment #20

dopry CreditAttribution: dopry commented 18 July 2006 at 21:09

just tested on my local install. Update went smoothly nothing seems broken afterwards with mysql.

Comment #21

moshe weitzman CreditAttribution: moshe weitzman commented 19 July 2006 at 02:28

Status:

Needs review

» Reviewed & tested by the community

seems we have sufficient review here.

Comment #22

drumm

he/him

NY, US

CreditAttribution: drumm commented 19 July 2006 at 06:10

Status:

Reviewed & tested by the community

» Needs work

None of these files exist anymore. You are looking for modules/system/system.install.

Comment #23

dopry CreditAttribution: dopry commented 19 July 2006 at 22:11

arg. you're correct I'm testing on 4.7.2. sry.

Comment #24

davemicc CreditAttribution: davemicc commented 7 September 2006 at 00:17

Status:

Needs work

» Needs review

File	Size
upload_load_fix.diff.txt	1.55 KB

I believe this patch works against cvs.

Comment #25

dopry CreditAttribution: dopry commented 7 September 2006 at 05:40

Status:

Needs review

» Reviewed & tested by the community

The patch seems identical to the original which had been well recieved and reviewed already. It applies cleanly to HEAD. The index was properly added to mysql db after running the update. The pgsql looks about right.