Performance: change {system}.type: alter table system modify column type VARCHAR(32);

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)

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.

This sounds like a sensible fix...

Here's my first attempt at a novice patch. Tested both installing and upgrading.

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 change array('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:

+  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:

+  $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);

Applied the changes suggested in #10 - type is now VARCHAR(12) - just enough to hold "theme_engine".

Wow, yes that update function looks a whole lot better now. :) Now I'm just waiting for the testing bot to come back green.

Marked #371504: Missing index in system table as a duplicate of this.

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.

I don't know, it's just a minor performance enhancement, should it be backported?

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.

Ok then, looks good to me. Applies cleanly, tested manually and testing bot gives the green.

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

Committed to HEAD. :) Thanks!

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?

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

Patch attached

Why this patch still not commited to 6?

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!

Just one-line patch, drop old index before adding new one

change status

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().

@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

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;
}

Sorry andy, that make complete sense. RTBC then.

Just reorder for more readablity

Committed to CVS HEAD. Thanks!

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.

Suppose this issue is fixed, Let's back extra section in follow-ups at #278592: Sync 6.x extra updates with HEAD

Going 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