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.

Comments

Freso’s picture

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:

    case 'pgsql':
      db_change_column($ret,
        'cram_nonce', 'valid', 'valid', 'smallint',
        array('not null' => TRUE, 'default' => 0)
      );
      break;
Freso’s picture

StatusFileSize
new877 bytes

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

Freso’s picture

StatusFileSize
new909 bytes

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.

selmanj’s picture

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.

Freso’s picture

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!

Freso’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new1.27 KB

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

Freso’s picture

StatusFileSize
new1.26 KB

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

selmanj’s picture

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?

Freso’s picture

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

Freso’s picture

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