database schema and operations clean up
ilo - March 12, 2009 - 07:53
| Project: | Login Security |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | ilo |
| Status: | closed |
Description
I've seen it's a must to perform a db clean up for the module (at least d6 and d7). Primary key is set to 'timestamp' field, instead of id. Due to this failing when two events happend in the same timestamp (quite easy to happend if two users meets in time, or one script trying to..) I'll propose changing this to 'id' field and creating the timestamp index. Also, a complete db code review will be performed to migrate from db_query to more drupal friendly drupal_write_record. The update part should also be complete for the .install file.
Subtask of #397890: create a stable version for the 6.x branch of the login_security module
#1
I need someone with pgsql to test this patch. Just do what stated in the subject, updates database schema, and changes db_query('INSERT to drupal_write_record.
#2
Previous patch didn't include the schema changes needed in the .install file for the drupal_write_record to find the schema.
#3
I'll be easier for most people to review this during development if it's in the snapshot, so I committed it.
#4
Thanks deekayen, I guess something went wrong because of the issue #492800: Incorrect table definition, not sure if this is fixed at all..
#5
I moved the index creation around in the initial commit because the API documentation says the keys should all be dropped before calling db_change_field, then re-created. That probably means the index creation probably needs a little more work since they are not re-created during change_field or after.
#492800: Incorrect table definition was caused by me moving the index creation down to the bottom of the update function. When I moved the creation down to the bottom, there was then no key for it to drop, which caused the error. The error was generated because #2 creates a key id then turns around and drops it on the next line. Why?
This is more like what I expected (untested):
<?phpfunction login_security_update_6001() {
$ret = array();
db_drop_primary_key($ret, 'login_security_track');
db_drop_index($ret, 'login_security_track', 'name');
db_change_field($ret, 'login_security_track', 'name', 'name', array(
'type' => 'varchar',
'length' => 64,
'not null' => TRUE,
'default' => '',
'description' => t("Username used in the login submission."),
)
);
db_change_field($ret, 'login_security_track', 'timestamp', 'timestamp', array(
'type' => 'int',
'not null' => TRUE,
'default' => 0,
'description' => t("Timestamp of the event."),
)
);
db_add_index($ret, 'login_security_track', 'name', array('name'));
db_add_index($ret, 'login_security_track', 'timestamp', array('timestamp'));
db_add_primary_key($ret, 'login_security_track', array('id'));
return $ret;
}
?>
#6
Not exactly, line 3 deletes the Index of the id field, not the key.. The problem here is just trying to alter the table (drop index id) with a serial field (id) and not primary key (just dropped in line 1).
Problem was: Previous database schema is wrong..
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/login_secur...
<?php'indexes' => array(
'id' => array('id'),
'name' => array('name'),
'host' => array('host')
),
'primary key' => array('timestamp')
?>
1) timestamp can't be primary key, two login attempts can happend at the same timestamp
2) id is indexed, timestamp is not indexed, but queries require tstamp index.
So, the tasks were:
- primary key should be remove from timestamp
- primary key should be created for ID
- index id is deleted
- index timestamp is created.
as shown in the final schema:
<?php'indexes' => array(
'name' => array('name'),
'host' => array('host'),
'timestamp' => array('timestamp'),
),
'primary key' => array('id')
?>
The howto was:
<?php// Remove current primary key
db_drop_primary_key($ret, 'login_security_track');
// Mysql needs a primary key to perform operations in the table. Create the new one
db_add_primary_key($ret, 'login_security_track', array('id'));
// drop the Watch out, it drops the current id INDEX, not the key
db_drop_index($ret, 'login_security_track', 'id');
// Create the new index for timestamp
db_add_index($ret, 'login_security_track', 'timestamp', array('timestamp'));
?>
We can just change the primary key and move the index part to the end as you did, or use the parameter $new_keys in the first alter.
http://api.drupal.org/api/function/db_change_field/6:
"On MySQL, all type 'serial' fields must be part of at least one key or index as soon as they are created. You cannot use db_add_{primary_key,unique_key,index}() for this purpose because the ALTER TABLE command will fail to add the column without a key or index specification. The solution is to use the optional $new_keys argument to create the key or index at the same time as field."
Your code should work (didn't test also, I'll give a try asap) just like this, leaving the db_add_primary_key at the begining..
<?phpfunction login_security_update_6001() {
$ret = array();
// step 1, change primary key in the table.
db_drop_primary_key($ret, 'login_security_track');
// mysql needs a key if there's a a serial field.
db_add_primary_key($ret, 'login_security_track', array('id'));
// step 2 remove index for name
db_drop_index($ret, 'login_security_track', 'name');
// Not sure if this is already dropped in all db engines because of the primary key drop, so I leave commented
// db_drop_index($ret, 'login_security_track', 'id');
db_change_field($ret, 'login_security_track', 'name', 'name', array(
'type' => 'varchar',
'length' => 64,
'not null' => TRUE,
'default' => '',
'description' => t("Username used in the login submission."),
)
);
db_change_field($ret, 'login_security_track', 'timestamp', 'timestamp', array(
'type' => 'int',
'not null' => TRUE,
'default' => 0,
'description' => t("Timestamp of the event."),
)
);
db_add_index($ret, 'login_security_track', 'name', array('name'));
db_add_index($ret, 'login_security_track', 'timestamp', array('timestamp'));
return $ret;
}
?>
Note that I just kept the primary key add before the alter. I guess, since we have a serial field, we should db_add_primary_key just inmediatly before the db_drop_primary_key.
#7
In fact, I've tested this:
- Uninstall module
- update login_security module to date 27-Feb-2009 (previous to db changes)
- Intall module
- Update login_security module to 6-1 branch
- Apply this patch
- perform update
And the result is:
The following queries were executed
login_security module
Update #6001
* ALTER TABLE {login_security_track} DROP PRIMARY KEY
* ALTER TABLE {login_security_track} ADD PRIMARY KEY (id)
* ALTER TABLE {login_security_track} DROP INDEX name
* ALTER TABLE {login_security_track} DROP INDEX id
* ALTER TABLE {login_security_track} CHANGE name `name` VARCHAR(64) NOT NULL DEFAULT ''
* ALTER TABLE {login_security_track} CHANGE timestamp `timestamp` INT NOT NULL DEFAULT 0
* ALTER TABLE {login_security_track} ADD INDEX name (name)
* ALTER TABLE {login_security_track} ADD INDEX timestamp (timestamp)
This is the update funtion
<?phpfunction login_security_update_6001() {
$ret = array();
// Change current primary key
db_drop_primary_key($ret, 'login_security_track');
db_add_primary_key($ret, 'login_security_track', array('id'));
// Drop indexes
db_drop_index($ret, 'login_security_track', 'name');
db_drop_index($ret, 'login_security_track', 'id');
db_change_field($ret, 'login_security_track', 'name', 'name', array(
'type' => 'varchar',
'length' => 64,
'not null' => TRUE,
'default' => '',
'description' => t("Username used in the login submission."),
)
);
db_change_field($ret, 'login_security_track', 'timestamp', 'timestamp', array(
'type' => 'int',
'not null' => TRUE,
'default' => 0,
'description' => t("Timestamp of the event."),
)
);
// Re-create indexes
db_add_index($ret, 'login_security_track', 'name', array('name'));
db_add_index($ret, 'login_security_track', 'timestamp', array('timestamp'));
return $ret;
}
?>
#8
FYI, I've been committing to both HEAD and DRUPAL-6--1.
#9
good!. Thanks for your help and support in the module's queue, deekayen.
#10
committed
#11
Automatically closed -- issue fixed for 2 weeks with no activity.