I just took a look at the code for the 5.x branch and noticed that the allocation of local node id's is not atomic. On a server with heavy load you are likely to get multiple nodes with the same local node id.

One solution would be to mimic the Drupal db_next_id() functionality:

function db_next_id($name) {
  $name = db_prefix_tables($name);
  db_query('LOCK TABLES {sequences} WRITE');
  $id = db_result(db_query("SELECT id FROM {sequences} WHERE name = '%s'", $name)) + 1;
  db_query("REPLACE INTO {sequences} VALUES ('%s', %d)", $name, $id);
  db_query('UNLOCK TABLES');

  return $id;
}

And lock the table during allocation, or perhaps find a way to add local node ids to Drupals sequence table.

-Marty

Comments

jbrown’s picture

jbrown commented
Version: 5.x-1.3 » 6.x-1.2
Assigned: Unassigned » jbrown
Priority: Critical » Normal

You are quite correct. There is a race condition.

A better solution would be to use a database transaction.

This is not a critical issue.

MartysMind’s picture

MartysMind commented

I guess whether it was critical or not would depend on your servers load and if you actually expect your server to work ;-)

jbrown’s picture

jbrown commented
Status: Active » Postponed

I'll set it to postponed until someone creates a patch.

This module should really be implemented as a CCK field.

MartysMind’s picture

MartysMind commented

I have a theory...

If a bug causes a module to be unstable or intermittently fail.. then it is a critical bug.

Why? Simply put if I am selecting modules to implement a production web site I need to know if there are problems that may cause that website to fail unexpectedly.

Of course that may make me a bit of a heretic, since in my view MANY Drupal modules are in the same shape and should really be labeled "beta" instead of "production".

TapSkill’s picture

TapSkill commented

Just make it give the MySQL table an auto-increment. I'm not sure how to do this from within Drupal, so I won't bother. However, I thought that was how this worked, too. MySQL can have auto-incremented integers as value types. PHP just inserts a blank value and it auto-inserts the next number.

thekevinday’s picture

thekevinday commented

I would recommend avoiding certain auto-increment unless you are willing to write support for all SQL databases drupal supports.

For whatever reason, auto-increment does not seem to be an SQL standard.

Therefore, using this means extra code and extra bugs.

For help implementing this in multiple SQL languages, see:
- http://www.w3schools.com/SQL/sql_autoincrement.asp
- http://pointbeing.net/weblog/2008/03/mysql-versus-postgresql-adding-an-a... and/or http://siphu.wordpress.com/2009/08/10/add-auto-increment-column-in-postg...

For whatever reason, postgresql seems to use SERIAL for an equivalent auto-increment.

I strongly suspect Drupal does this already (somewhere), so you may be able to recycle the Drupal core database code to produce a portable solution.

That is, if you are going to use the autoincrement concept from #5.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 6.x-1.2 » 6.x-1.x-dev

Suppose 6.x version should use http://api.drupal.org/api/group/lock/6 this supported from 6.16

@jbrown Are sure you wanna make this module a cck field?