[development] A gotcha with auto-increment and MySQL

[Barry Jaspan]

The most recent thread on using auto-incrementing columns in Drupal
seems to have led to the decision that it is okay to use them
(acknowledging that both our mysql and pgsql tables already do use
them), and the upcoming database schema data structure supports them.
However, while in my waking-up haze this morning, out of nowhere I
realized one of the reasons they were probably previously avoided.

On an auto-incrementing column, a race condition exists if
db_next_id() is sometimes used and sometimes not used.  The problem is
that while db_next_id() is atomic it does not alter MySQL's idea of
the next id to assign.  Suppose at T0 the maximum nid in the node
table is 10 and the sequences table reflects that.  At T1, one Drupal
process calls db_next_id('{node}_nid').  The sequences table is
atomically updated to 11 and the caller gets 11.  At T2, a second
Drupal process inserts a new row into the node table without providing
a nid; since the maximum nid is 10, the new row is assigned nid 11.
At T3, the first process tries to insert a row using the nid 11 it got
from db_next_id() and fails.

This problem does not occur with pgsql because the db_next_id() and
the auto-incrementing column both use the same underlying 'sequence'
object.

I see two ways to solve this problem:

1.  Disallow auto-incrementing columns everywhere in Drupal.  Note
that they are used on numerous core tables both with mysql and pgsql.
To remove them, we would have to verify that every insert into a table
uses db_next_id() to generate the new id.

2.  Just don't use db_next_id() and auto-increment on the same
columns, ever.  We could just declare this a bug and fix it when
discovered.  However, now that we (will soon) have a database schema,
we could also make db_next_id() actively warn about it:

function db_next_id($name) {
  list($table, $col) = preg_split_table_and_column_name($name);
  $schema = drupal_get_schema();
  if ($schema[$table]['cols'][$col]['type'] == 'serial') {
    // log an error here
  }

  // rest of db_next_id() here
}

Thoughts?

Barry