Schema changes

Posted by Freso on February 21, 2008 at 12:25am

Issue Summary

Since this is really a boolean field (0 or 1), there's no need for it to be a full INT. This patch changes the D5 MySQL to use bool and the D6 Schema API to use 'size' => 'tiny'.

Not tested.

Notes to D5 patch:
BOOL is older than <code>BOOLEAN, the latter having been used since MySQL 4.1.
The ALTER TABLE ... CHANGE syntax for the update is also the more backwards compatible version compared to ALTER TABLE ... MODIFY, the latter having been available since MySQL 3.22.16a.

Attachment	Size
cram.install.d5.use_tinyint.patch	1.03 KB
cram.install.d6.use_tinyint.patch	788 bytes

Comments

#1

Posted by Freso on February 21, 2008 at 12:53am

Please note that this isn't updated for #224646: PostgreSQL support for 5.x version, so if that happens to be committed first, please mark this CNW.

What is needed to make this patch work post-pgsql-patch:
valid int NOT NULL default '0', should become valid smallint NOT NULL default '0', (line 20), and the cram_update_2() of the patch should have this case added to the switch:

<?php
    case 'pgsql':
      db_change_column($ret,
        'cram_nonce', 'valid', 'valid', 'smallint',
        array('not null' => TRUE, 'default' => 0)
      );
      break;
?>

#2

Posted by Freso on February 21, 2008 at 1:16am

Note that the previous comment only applies to the 5.x patch. The D6 patch uses Schema API and is, as such, above the MySQL vs. PostgresQL problem (and should work for SQLite, Oracle, MS SQL, ... too).

And speaking of D6, I thought db_change_field() worked differently than it turns out. So here's an updated patch for 6.x-1.x-dev! (With a thoroughly tested update for pgsql.)

Attachment	Size
cram.install.d6.use_tinyint.patch	877 bytes

#3

Posted by Freso on February 21, 2008 at 1:37pm

Hm. Perhaps this patch should also make cram_nonce.issued unsigned? cram_nonce.issued is never less than 0, which means there's no reason for it to be signed.
Also, making it unsigned gives CRAM about 68 years of additional years to live, before needing a bigger int field! :D (It currently only has about 30 (29.9) years left to live!! :o (Which means that making this unsigned would increase the lifespan to about 98 years from now...))

Or do you want me to make a new issue for this? (Which is what I was doing before thinking that this issue could be re-purposed to a generic schema refresh.)

There's an attached patch with the unsigning on its own (for 6.x), which is not compatible with any of the TINYINT patches.

Attachment	Size
cram.d6.issued_unsigned.patch	909 bytes

#4

Posted by selmanj on February 21, 2008 at 4:53pm

Title:	Use TINYINT for cram_nonce.valid	» Schema changes
Status:	needs review	» needs work

Nope, this is now a generic schema-issue ticket :)

Also, regarding the use of bool, why not just use TINYINT for both d5 and d6? According to http://dev.mysql.com/doc/refman/5.1/en/data-types.html TINYINT seems to be the most portable option rather than bool or enum or anything like that.

Also, I appreciate the unsigned change; even though it is completely useless, it is a better descriptor of what values the field represents than leaving it signed.

#5

Posted by Freso on February 21, 2008 at 5:04pm

Let me reply in reverse order. :)

I thought I made it clear in the unsigned comments that I didn't think it was necessarily particularly useful, just a good idea. Which is also why I felt silly making a new issue for it, making another _update_N.

TINYINT doesn't exist in PostgresQL ('size'='tiny' makes pgsql use SMALLINT), and since we're using a switch-case anyway, that particular bit of code will only ever run in a MySQL[i] environment, which has had BOOL for a very, very long time. And yes, I know BOOL is just a synonym for TINYINT, but, semantically, BOOL is more correct.

I'll have a new patch ready in a moment!

#6

Posted by Freso on February 21, 2008 at 5:54pm

Version:	5.x-1.x-dev	» 6.x-1.x-dev
Status:	needs work	» needs review

Here's the 6.x patch, which has been tested on PostgresQL. Wouldn't hurt to test it on MySQL as well. (Check that issued and valid are regular integers (INT or INTEGER) before patch, and check that issued is unsigned, that valid is TINYINT, and that PRIMARY KEY still exists after patch and update.)

Attachment	Size
cram.d6.schema_changes.patch	1.27 KB

#7

Posted by Freso on February 21, 2008 at 6:21pm

And here's an untested patch for 5.x with MySQL. Please test. Same procedure as in #6.

Attachment	Size
cram.d5.schema_changes.patch	1.26 KB

#8

Posted by selmanj on March 6, 2008 at 4:54am

Status:

needs review

» needs work

D6 schema update fails with this error:

user warning: Multiple primary key defined query: ALTER TABLE cram_nonce ADD PRIMARY KEY (nonce, issued) in /var/www/dev/d6/includes/database.mysql-common.inc on line 374.

Maybe a recent update broke it?

#9

Posted by Freso on March 6, 2008 at 6:29am

Most probably it didn't, as the only D6 affecting update you've checked in since was the 'file path' one. Also, the reason the db_add_primary_key() is there in the first place is because it seemed to get lost in the db_change_field() processing with PostgreSQL. Would it be an okay solution to db_del_primary_key() before db_add_primary_key()?

#10

Posted by Freso on March 6, 2008 at 11:25pm

Note (to self?): #224646: PostgreSQL support for 5.x version is applied, so patch needs to take pgsql into account for 5.x.

Project:	CRAM (Challenge-Response Authentication Mechanism)
Version:	6.x-1.x-dev
Component:	Code
Category:	task
Priority:	normal
Assigned:	Unassigned
Status:	needs work

Jump to:

Issue Summary

Comments