The type column of the {system} table is currently a varchar(255) field. Yet in 99% of all Drupal installations it stores either the text 'module' or the text 'theme' and nothing else. This is already a strong case for making it an int column and defining constants for module and theme. Then, on every page, there is this query:
SELECT filename FROM system WHERE name = 'user' AND type = 'module';
This query always takes over 1 ms to run making it not a criminally slow query, but relatively slow, nonetheless. This is because the list of indexes available doesn't help the query much:
mysql> describe SELECT filename FROM system WHERE name = 'user' AND type = 'module';
+----+-------------+--------+------+-------------------+-----------+---------+-------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+--------+------+-------------------+-----------+---------+-------+------+-------------+
| 1 | SIMPLE | system | ref | modules,bootstrap | bootstrap | 38 | const | 54 | Using where |
+----+-------------+--------+------+-------------------+-----------+---------+-------+------+-------------+
Adding an index for these two columns won't work:
mysql> CREATE UNIQUE INDEX name_type ON system (name,type);
ERROR 1071 (42000): Specified key was too long; max key length is 1000 bytes
Shortening the potential length of type allows for making an index:
mysql> alter table system modify column type VARCHAR(32);
Query OK, 79 rows affected (0.03 sec)
Records: 79 Duplicates: 0 Warnings: 0
mysql> CREATE UNIQUE INDEX name_type ON system (name,type);
Query OK, 79 rows affected (0.02 sec)
Records: 79 Duplicates: 0 Warnings: 0
This allows the query to drop consistently below 1ms:
0.32 1 drupal_get_filename SELECT filename FROM system WHERE name = 'color' AND type = 'module'
0.24 1 drupal_get_filename SELECT filename FROM system WHERE name = 'comment' AND type = 'module'
0.25 1 drupal_get_filename SELECT filename FROM system WHERE name = 'dblog' AND type = 'module'
0.29 1 drupal_get_filename SELECT filename FROM system WHERE name = 'help' AND type = 'module'
0.37 1 drupal_get_filename SELECT filename FROM system WHERE name = 'openid' AND type = 'module'
0.34 1 drupal_get_filename SELECT filename FROM system WHERE name = 'search' AND type = 'module'
0.26 1 drupal_get_filename SELECT filename FROM system WHERE name = 'statistics' AND type = 'module'
0.27 1 drupal_get_filename SELECT filename FROM system WHERE name = 'taxonomy' AND type = 'module'
0.29 1 drupal_get_filename SELECT filename FROM system WHERE name = 'tracker' AND type = 'module'
0.22 1 drupal_get_filename SELECT filename FROM system WHERE name = 'update' AND type = 'module'
Comment | File | Size | Author |
---|---|---|---|
#31 | 215080_index_update.patch | 856 bytes | andypost |
#26 | 215080_index_update.patch | 823 bytes | andypost |
#23 | system-index-type-name-D6.patch | 984 bytes | andypost |
#21 | 215080-system-index-type-name-D6.patch | 1013 bytes | Dave Reid |
#11 | system-type-varchar12.patch | 1.61 KB | drifter |
Comments
Comment #1
jaydub CreditAttribution: jaydub commentedRather than changing the column def you can instead change the length of the index on the name or type column. So in order to avoid the max key length error:
CREATE UNIQUE mysql> CREATE UNIQUE INDEX name_type ON system (name (12),type);
Comment #2
jaydub CreditAttribution: jaydub commentedThis issue: http://drupal.org/node/185126 has some system table optimizations that I believe are already in D6. The index created in that issue covers 4 or 5 fields from the system table but not the 'name' field so perhaps this issue can still be considered.
Comment #3
robertDouglass CreditAttribution: robertDouglass commented@jaydub: interesting suggestion with the index lenght. I still think varchar(255) is wasteful for the type column.
Comment #5
robertDouglass CreditAttribution: robertDouglass commentedHmm. I forgot that I had suggested this. I still think it's a good idea.
Comment #6
robertDouglass CreditAttribution: robertDouglass commentedIn particular I think we should shorten the field, so I'm changing the title.
Comment #7
nicholasThompsonThis sounds like a sensible fix...
Comment #8
webchickComment #9
drifter CreditAttribution: drifter commentedHere's my first attempt at a novice patch. Tested both installing and upgrading.
Comment #10
Dave ReidA good start. I think we should actually put the type field at varchar(12). Do we really have any need for a type larger than 12 chars (profile, theme, engine, module, etc)? See {registry}.type - it's a varchar(9).
In system_schema(), you'll want to add another index
'type_name' => array('type', 'name'),
. We can probably changearray('type', 12)
to simply just'type'
. Make sure you make the same index addition/change in the system_update_7019.The first part of a PHP doc block of a function should fit to 80 characters. For system_update_7019, I'd recommend "Shorten the {system}.type column and add an index on type and name."
It's preferred in core to actually just do:
instead of:
Comment #11
drifter CreditAttribution: drifter commentedApplied the changes suggested in #10 - type is now VARCHAR(12) - just enough to hold "theme_engine".
Comment #12
Dave ReidWow, yes that update function looks a whole lot better now. :) Now I'm just waiting for the testing bot to come back green.
Comment #13
Dave ReidMarked #371504: Missing index in system table as a duplicate of this.
Comment #14
Dave ReidIf we want to backport this to 6.x, we should rename the update function system_update_6048() so that we won't be out of sync when we backport.
Comment #15
drifter CreditAttribution: drifter commentedI don't know, it's just a minor performance enhancement, should it be backported?
Comment #16
catchWe can commit as is, then if goba decides it's worth backporting, move the function back to 6048() in HEAD once that's done. This issue comes up every time with backported schema changes, dunno what a good solution is.
Comment #17
Dave ReidOk then, looks good to me. Applies cleanly, tested manually and testing bot gives the green.
Comment #18
robertDouglass CreditAttribution: robertDouglass commentedNice work people. Every performance improvement that can be backported should be.
Comment #19
Gábor HojtsySchema modifications would not be good for D6, but a short index can always be added. Gerhard suggested in an issue marked duplicate of this one to add an index on name in D6: #371504: Missing index in system table
Comment #20
webchickCommitted to HEAD. :) Thanks!
Comment #21
Dave ReidHere's the backport of just the index for type_name for D6. Not sure why we need to have a separate issue just for the D6 version. Why not reuse this thread?
Comment #22
andypostHere the new patch against DRUPAL-6
changed system_update_6050 because of system_update_6049 was for #363262: Missing index on url_alias table
So what with new D7 patch? - if update in d6 so it dowsn't needed in 7
Comment #23
andypostPatch attached
Comment #24
andypostWhy this patch still not commited to 6?
Comment #25
Gábor HojtsyI've modified the patch to add the system_update_6053() function, but apart from that, committed the 6.x patch as-is. Thanks! Now back to 7.x to fix the upgrade path!
Comment #26
andypostJust one-line patch, drop old index before adding new one
Comment #27
andypostchange status
Comment #28
catchThis doesn't look right to me - if system_update_6050() changes the column definition, we need to remove that from 7019() (and probably the entire function - in which case it should be emptied except for an inline comment explaining that it was moved to 6050().
Comment #29
andypost@catch updates are different, D6 have only index definition but D7 changes column, so we need only drop a previous index because it already exists from D6
Comment #30
catchSorry andy, that make complete sense. RTBC then.
Comment #31
andypostJust reorder for more readablity
Comment #32
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #33
Dave ReidNo, this path is broken now. Whenever we add an update function to D6, we need to add the same exact update function back into D7. That way people that are going to upgrade from D6.10 should still hit all the update functions in the latest D6 version. This current upgrade path is going to drop an index that does not exist for people that are not using the latest versions of D6, causing an error.
Comment #34
andypostSuppose this issue is fixed, Let's back extra section in follow-ups at #278592: Sync 6.x extra updates with HEAD
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedGoing to varchar(12) here may have been too short, given the way the 'type' column was used in D6 contrib. See #1987378: system_update_7018() fails on sites using the Drupal 6 "jQuery Plugin Handler" module because {system}.type is too short