I'm running this via drush, more information forthcoming soon, but here's the output:

ALTER TABLE {login_security_track} DROP INDEX host                   [error]
ALTER TABLE {login_security_track} CHANGE `host` `host` VARCHAR(39)  [success]
NOT NULL DEFAULT ''
ALTER TABLE {login_security_track} ADD INDEX host (host)             [success]
ALTER TABLE {login_security_track} DROP PRIMARY KEY                  [error]
ALTER TABLE {login_security_track} ADD PRIMARY KEY (id)              [error]
ALTER TABLE {login_security_track} DROP INDEX name                   [error]
ALTER TABLE {login_security_track} DROP INDEX id                     [error]
ALTER TABLE {login_security_track} CHANGE `name` `name` VARCHAR(64)  [success]
NOT NULL DEFAULT ''
ALTER TABLE {login_security_track} CHANGE `timestamp` `timestamp` INT[success]
NOT NULL DEFAULT 0
ALTER TABLE {login_security_track} ADD INDEX name (name)             [success]
ALTER TABLE {login_security_track} ADD INDEX timestamp (timestamp)   [error]
CommentFileSizeAuthor
#4 593896_login_security_5_to_6_upgrade_1.patch1.23 KBilo
#2 593896_login_security_5_to_6_upgrade.patch1.22 KBilo

Comments

catch’s picture

catch
he/him
Primary language English
commented

Here's the errors from update.php:



    * user warning: Can't DROP 'host'; check that column/key exists query: ALTER TABLE login_security_track DROP INDEX host in /home/catch/www/6/includes/database.mysql-common.inc on line 448.
    * user warning: Incorrect table definition; there can be only one auto column and it must be defined as a key query: ALTER TABLE login_security_track DROP PRIMARY KEY in /home/catch/www/6/includes/database.mysql-common.inc on line 386.
    * user warning: Multiple primary key defined query: ALTER TABLE login_security_track ADD PRIMARY KEY (id) in /home/catch/www/6/includes/database.mysql-common.inc on line 374.
    * user warning: Can't DROP 'name'; check that column/key exists query: ALTER TABLE login_security_track DROP INDEX name in /home/catch/www/6/includes/database.mysql-common.inc on line 448.
    * user warning: Can't DROP 'id'; check that column/key exists query: ALTER TABLE login_security_track DROP INDEX id in /home/catch/www/6/includes/database.mysql-common.inc on line 448.
    * user warning: Duplicate key name 'timestamp' query: ALTER TABLE login_security_track ADD INDEX timestamp (timestamp) in /home/catch/www/6/includes/database.mysql-common.inc on line 434.
ilo’s picture

ilo commented
Assigned: Unassigned » ilo
Status: Active » Needs review
StatusFileSize
new 593896_login_security_5_to_6_upgrade.patch1.22 KB

Thanks for this catch, catch :)

I've been reviewing the upgrade path and is inacurate at all. First, the 'host' index will never be deleted because it doesn't exist, and second, the PRIMARY key can't be removed this way because it was created using three fields, and to be able to delete, the PRIMARY key should have just 1 field, and should be the autoincremented field: "there can be only one auto column and it must be defined as a key".

mysql only allows to alter a multiple primary key in two conditions:
- single sentence: ALTER TABLE xxx DROP PRIMARY KEY, ADD PRIMARY KEY (field)
or
- creating a index with the autoincrement field.
- droping current primary key.
- creating the new primary key.
- deleting the index of the autoincrement field.

As there's no Drupal API function to perform a DROP ADD in a single sentence, the new update function does the 2nd way..


function login_security_update_6000() {
  $ret = array();
  // Change current primary key
  db_add_index($ret, 'login_security_track', 'id', array('id'));
  db_drop_primary_key($ret, 'login_security_track');
  db_add_primary_key($ret, 'login_security_track', array('id'));
  db_drop_index($ret, 'login_security_track', 'id');
  db_change_field($ret, 'login_security_track', 'host', 'host', array(
    'type' => 'varchar',
    'length' => 39,
    'not null' => TRUE,
    'default' => '',
    'description' => t("The IP address of the request."),
    )
  );
  db_add_index($ret, 'login_security_track', 'host', array('host'));
  db_add_index($ret, 'login_security_track', 'name', array('name'));
  return $ret;
}

I've also included the 'name' index to leave 5.x to 6.x updated schema as defined for the installation.

I've not tested (apart than manually using a mysql client) but I guess this should work. Catch, could you give a try to the patch? It's for the 6.x-1.x-dev branch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Thanks for this, looks better but not quite there yet - only had time to run the drush update again with the patch:

ALTER TABLE {login_security_track} ADD INDEX id (id)                 [success]
ALTER TABLE {login_security_track} DROP PRIMARY KEY                  [success]
ALTER TABLE {login_security_track} ADD PRIMARY KEY (id)              [success]
ALTER TABLE {login_security_track} DROP INDEX id                     [success]
ALTER TABLE {login_security_track} CHANGE `host` `host` VARCHAR(39)  [success]
NOT NULL DEFAULT ''
ALTER TABLE {login_security_track} ADD INDEX host (host)             [success]
ALTER TABLE {login_security_track} ADD INDEX name (name)             [success]
ALTER TABLE {login_security_track} DROP PRIMARY KEY                  [error]
ALTER TABLE {login_security_track} ADD PRIMARY KEY (id)              [error]
ALTER TABLE {login_security_track} DROP INDEX name                   [success]
ALTER TABLE {login_security_track} DROP INDEX id                     [error]
ALTER TABLE {login_security_track} CHANGE `name` `name` VARCHAR(64)  [success]
NOT NULL DEFAULT ''
ALTER TABLE {login_security_track} CHANGE `timestamp` `timestamp` INT[success]
NOT NULL DEFAULT 0
ALTER TABLE {login_security_track} ADD INDEX name (name)             [success]
ALTER TABLE {login_security_track} ADD INDEX timestamp (timestamp)   [error]
ilo’s picture

ilo commented
Status: Needs work » Needs review
StatusFileSize
new 593896_login_security_5_to_6_upgrade_1.patch1.23 KB

Enough information, I guess.. Thanks for you time testing this.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

All pass updating via drush now. I didn't verify the resulting indexes against the schema or anything, but much better. Thanks!

ilo’s picture

ilo commented
Status: Reviewed & tested by the community » Fixed

Committed to 6.x-1.x-dev. Thanks catch for taking some time to test this, I know it takes sometime :)
I've already tested that final schema matches.

Status: Fixed » Closed (fixed)

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

  • ilo committed d93b6b5 on 6.x-1.x, 8.x-1.x 
    #593896 by catch and ilo: upgrade path from 5.x to 6.x didn't match...