Instead of this:

CREATE TABLE `uuid_node` (
  `nid` int(10) unsigned NOT NULL default '0',
  `uuid` varchar(64) NOT NULL default '',
  PRIMARY KEY  (`nid`),
  UNIQUE KEY `uuid_node_uuid_key` (`uuid`)
) ENGINE=MyISAM DEFAULT CHARSET=utf8;

It should be this:

CREATE TABLE `uuid_node` (
  `nid` int(10) unsigned NOT NULL default '0',
  `uuid` char(64) NOT NULL default '',
  PRIMARY KEY  (`nid`),
  UNIQUE KEY `uuid_node_uuid_key` (`uuid`)
) ENGINE=MyISAM DEFAULT CHARSET=utf8;

Ah! Much faster and smaller.

CommentFileSizeAuthor
#9 uuid-913820.patch1.53 KBapotek
#8 uuid-n913820-8.patch1.75 KBdamienmckenna
#6 uuid-913820.patch1.6 KBapotek
#3 uuid-913820.patch1.51 KBapotek
#2 uuid.install-uuid-col-char.patch1.17 KBapotek

Comments

apotek’s picture

apotek commented

Another way to say this is change the uuid => array() in this function


function uuid_table_schema($table, $key = 'key') {
  return array(
    'fields' => array(
      $key => array(
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
        'description' => 'The primary key of the record which this UUID was generated for.',
      ),
      'uuid' => array(
        'type' => 'char',
        'length' => 64,
        'not null' => TRUE,
        'default' => '',
        'description' => 'The Universally Unique Identifier.',
      ),
    ),
    'primary key' => array($key),
    'unique keys' => array(
      'uuid_'. $table .'_uuid_key' => array('uuid'),
    ),
  );
}

Of course, we'll also have to create an update hook.

apotek’s picture

apotek commented
Status: Active » Needs review
StatusFileSize
new uuid.install-uuid-col-char.patch1.17 KB

Okay, here's the patch.

apotek’s picture

apotek commented
StatusFileSize
new uuid-913820.patch1.51 KB

Regenerated the patch to be more properly formatted and to update smoother. Also wrapped the uuid_comment table create statement in a if table does not exist wrap.

This is a pretty atomic and specific patch.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. +1

recidive’s picture

recidive commented
Title: uuid schema: since uuids are *always* 64 characters long, save time and db space by defining the column as CHAR » Change UUID columns to char(36)
Version: 6.x-1.0-beta2 » 6.x-1.x-dev
Status: Reviewed & tested by the community » Needs review

Actually UUIDs are 36 chars long (32 digits and 4 hyphens).

In the long run, I think we should store only the 32 digits stripping out the hyphens in database only.

For now, I think converting the columns to 36 chars and use CHAR instead of VARCHAR would be an improvement.

Also, there are some code style issues with your patch:

1. There are tabs indentation and they are not accepted;
2. There is an inline comment with no comment at all;
3. Comments must start with a capital letter and end with a period;
4. We normally don't use "while(list(,$table) = each($tables))" constructors but "foreach ($tables as $table)".

apotek’s picture

apotek commented
StatusFileSize
new uuid-913820.patch1.6 KB

Thanks for the feedback. I'm confused, though, as it looks like you looked at the first version of the patch rather than the uuid-913820.patch version. The posted version has no tabs, and no inline comment with no comment in it.

I have created a new patch, with the column defined to 32 char. I also edited my comments to start with capital letters and end with periods.

> 4. We normally don't use "while(list(,$table) = each($tables))" constructors but "foreach ($tables as $table)".

If this is a blocker, I suppose I can roll another version of the patch to conform more the the Drupal way, it's just that the Drupal way is part of the reason why Drupal uses up so much more memory than necessary. As you know, foreach *copies* each variable rather than just iterating through them. Using a while() construct simply iterates through an array without the added expense of having to gobble up more memory.

recidive’s picture

recidive commented
Status: Needs review » Needs work

Just changing the column to CHAR(32) will not make it as the UUID including the hyphens are 36 chars long. So it should be 36 instead of 32.

Also, you should write uuid_update_6004() function to make you changes to database schema, since uuid_update_6003() will not run in sites running the latest schema.

That was the line I said there's a inline comment with no comment at all:

$tables = uuid_schema();//

And please change the while(list()) loop with foreach() for consistency.

I'm looking forward to commiting you patches. Thank you!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new uuid-n913820-8.patch1.75 KB

Here's an updated patch that cleans up some of the code and includes all of the requested changes from #7, and some others. Could someone please review it.

apotek’s picture

apotek commented
StatusFileSize
new uuid-913820.patch1.53 KB

Cleaned up as requested. Took a couple shortcuts from Damien as well. Thanks.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Reviewed & tested by the community

Worked correctly for me.

recidive’s picture

recidive commented

Thanks guys.

Just a question: what's the change in uuid_update_6003() about?

Is that part of the patch or was it mistakenly added?

apotek’s picture

apotek commented

Just put that in so that no errors are generated if the table already exists. It's unrelated, so feel free to rip it out, or I can rip it out of the patch and make a new issue for it, if you like. But is seems harmless where it is.

recidive’s picture

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

Commited! Thanks!

Status: Fixed » Closed (fixed)

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