Make sequence API work on non-MySQL databases

I like David's suggestion here. So we could write something like:

public function nextId() {
    return variable_get('sequence_minimum', 0) + $this->query("INSERT INTO {sequences} (value) VALUES (default)");
}

Which would even suit PostgreSQL :)

Well, we could automate it ourselves:

public function nextId($existing_id = 0) {
    $transaction = $this->startTransaction();
    $id = $this->query("INSERT INTO {sequences} (value) VALUES (default)");
    if ((variable_get('sequence_minimum', 0) + $id) < $existing_id)) {
        $sequence_minimum = $existing_id - $id - 1;
        variable_set('sequence_minimum', $sequence_minimum);
    }
    return variable_get('sequence_minimum', 0) + $id;
}

Don't we have a lock API now too? we can make this atomic with that no? If need be we could set the offset as a property in the class definition and update it by calling variable_set on __destruct

actually that whole destruct idea wouldn't work. It would virtually turn Drupal into a single thread system

We can't actually use variable_get/set because they cache and break atomicity so how about this instead: We add another column to the sequences table that stores the offset and defaults to 0:

public function nextId($existing_id = 0) {
    $transaction = $this->startTransaction();
    // Returning NULL prevents PostgreSQL from doing an additional query.
    $this->query("INSERT INTO {sequences} (value, seq_min) VALUES (default, (SELECT MAX(seq_min) FROM {sequences}))", array(), array('return' => Database::RETURN_NULL));
    $id = $this->query("SELECT (MAX(seq_min) + MAX(value) FROM {sequences}")->fetchField();
    if ($id <= $existing) {
        $this->query("UPDATE {sequences} SET seq_min = (:existing - MAX(value)) WHERE value = MAX(value)", array(':existing' => $existing_id));
        $id = $this->query("SELECT (MAX(seq_min) + MAX(value)) FROM {sequences}")->fetchField();
    }
    return $id;
}

The transaction should mean we don't need to take out a lock. This uses a maximum of 4 queries and a minimum of 2.

You can INSERT into the same table you SELECT from? Wow

What is that suppose to mean? PostgreSQL does a SELECT to get the last insert id (thats inside the PDO driver).

So any objections with the implementation? If not I'll write a patch. Will also truncate the table as all we need is sequence and offset.

here is the PostgreSQL implementation as suggested by chx and DamZ. Notice I'm not using the Drupal transaction API because postgreSQL needs to commit the transaction to release the table lock (PostgreSQL allows you to commit a transaction within a transaction :))

I haven't tested this but I will try get some time to do so

whoops, lets try that again.

I'm using this to create my diff files:

git diff --no-prefix origin/master

has worked in the passed

attached is patch that works for PostgreSQL, need sqlite implementation added to it though. Chx, can you do that part?

I actually found some time to test my patch - I had a few syntax issues with DBTNG - but I got the sequences API working on PostgreSQL. Still need someone to make it work on SQLite.

cool, attached are some better comments.

According to my research (http://www.postgresql.org/docs/8.1/static/sql-savepoint.html) and talking in #postgres, SAVEPOINT nextid allows PostgreSQL to rollback to that point without failing the transaction using ROLLBACK TO SAVEPOINT nextid. Because the savepoint is labeled the same, a new savepoint (calling nextid again, and the situation occuring again) will overwrite the old savepoint. Savepoints are released on COMMIT

How can this be tested? Does simpletest support race conditions? I've tested PostgreSQL on the sequences API tests, and my patch passes it. If you wanted to test the sequences API within a transaction, we could do this:

  /**
   * Test that the Sequences API works within a transaction.
   */
  function testDbNextIdTransaction() {
    $first = db_next_id();
    
    $transaction = db_transaction();

    $second = db_next_id($first);
    $this->assertEqual($first + 1, $second, t('Sequence provides a larger number than the existing ID within a transaction.'));
    
    // When the transaction fails, the sequence will be reset the first id.
    $transaction->rollback();
    unset($transaction);
    
    $third = db_next_id();
    $this->assertEqual($second, $third, t('Sequence is rolled back to a value before the transaction.'));
  }

But would it pass on MySQL. Crell, webchick - back to you.

I've re-written the patch for a few reasons:

1. PostgreSQL needed to clean up the sequences table similar to what MySQL does. Then I realised PostgreSQL doesn't actually need to insert into the table at all but merely use it as a lock when altering the sequence. So now, my patch retrieves the next value in the sequences directly from the sequence within PostgreSQL. This also prevents having to register a shutdown function.
2. PostgreSQL sequences live outside the scope of transactions. If a sequence is altered within a transaction and the transaction is rolled back, the sequence remains altered. So long our logic raised the sequence value rather than lower it. I don't see this as being an issue. However, this did raise the question if a SAVEPOINT was needed. They are however, in case a LOCK on the table cannot be acquired.

I've tested this against simpletests sequence API tests and it works fine. I don't believe more tests are needed to prove functionality.
Lets get this in ASAP so I can move on with fix the other areas of Drupal that are broken in PostgreSQL.

After talking with DamZ in IRC, we implemented a better locking system with PostgreSQL Advisory Locks which are designed to be used by applications rather than designed to lock tables (though they can). Advisory locks require a numeric id to hold as the lock identifier I've created the POSTGRESQL_NEXTID_LOCK constant to hold this value.

I've tested and confirmed that #42 is a good solution. DamZ agreed with me. Lets get this in and move on to Sqlite implementation.

done. Also pushed the implemented default nextID() to the sqlite driver since it didn't have an implementation. This is a TODO for chx or DamZ

cool, test bot is happy.