Performance: change {system}.type: alter table system modify column type VARCHAR(32);
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | drupal.org upgrade, needs backport to D6, Novice |
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 bytesShortening 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: 0This 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'
#1
Rather 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);
mysql> CREATE UNIQUE INDEX name_type ON system (name (12),type);
Query OK, 44 rows affected (0.05 sec)
Records: 44 Duplicates: 0 Warnings: 0
mysql> explain 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 | const | name_type | name_type | 805 | const,const | 1 | |
+----+-------------+--------+-------+---------------+-----------+---------+-------------+------+-------+
1 row in set (0.00 sec)
#2
This 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.
#3
@jaydub: interesting suggestion with the index lenght. I still think varchar(255) is wasteful for the type column.
#5
Hmm. I forgot that I had suggested this. I still think it's a good idea.
#6
In particular I think we should shorten the field, so I'm changing the title.
#7
This sounds like a sensible fix...
#8
#9
Here's my first attempt at a novice patch. Tested both installing and upgrading.
#10
A 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:
<?php+ db_drop_index($ret, 'system', 'modules');
+ db_change_field($ret, 'system', 'type', 'type', array('type' => 'varchar', 'length' => 12, 'not null' => TRUE, 'default' => ''));
+ db_add_index($ret, 'system', 'modules', array('type', 'status', 'weight', 'filename'));
+ db_add_index($ret, 'system', 'type_name', array('type', 'name'));
?>
instead of:
<?php+ $spec = array(
+ 'description' => 'The type of the item, either module, theme, or theme_engine.',
+ 'type' => 'varchar',
+ 'length' => 32,
+ 'not null' => TRUE,
+ 'default' => '',
+ );
+
+ $indexes = array(
+ 'indexes' => array(
+ 'modules' => array(array('type', 12), 'status', 'weight', 'filename'),
+ ),
+ );
+
+ db_drop_index($ret, 'system', 'modules');
+ db_change_field($ret, 'system', 'type', 'type', $spec, $indexes);
?>
#11
Applied the changes suggested in #10 - type is now VARCHAR(12) - just enough to hold "theme_engine".
#12
Wow, yes that update function looks a whole lot better now. :) Now I'm just waiting for the testing bot to come back green.
#13
Marked #371504: Missing index in system table as a duplicate of this.
#14
If 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.
#15
I don't know, it's just a minor performance enhancement, should it be backported?
#16
We 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.
#17
Ok then, looks good to me. Applies cleanly, tested manually and testing bot gives the green.
#18
Nice work people. Every performance improvement that can be backported should be.
#19
Schema 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
#20
Committed to HEAD. :) Thanks!
#21
Here'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?
#22
Here 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
#23
Patch attached
#24
Why this patch still not commited to 6?
#25
I'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!
#26
Just one-line patch, drop old index before adding new one
#27
change status
#28
This 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().
#29
@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
<?php
function system_update_6053() {
$ret = array();
db_add_index($ret, 'system', 'type_name', array(array('type', 12), 'name'));
return $ret;
}
function system_update_7018() {
$ret = array();
db_drop_index($ret, 'system', 'modules');
db_change_field($ret, 'system', 'type', 'type', array('type' => 'varchar', 'length' => 12, 'not null' => TRUE, 'default' => ''));
db_add_index($ret, 'system', 'modules', array('type', 'status', 'weight', 'name'));
+ db_drop_index($ret, 'system', 'type_name');
db_add_index($ret, 'system', 'type_name', array('type', 'name'));
return $ret;
}
?>
#30
Sorry andy, that make complete sense. RTBC then.
#31
Just reorder for more readablity
#32
Committed to CVS HEAD. Thanks!
#33
No, 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.
#34
Suppose this issue is fixed, Let's back extra section in follow-ups at #278592: Sync 6.x extra updates with HEAD
#35
Automatically closed -- issue fixed for 2 weeks with no activity.