Inserting into a just created table fails when trying to set a value to a serial

This looks good to me. Could you just please move the try/catch into an override of PDO::lastInsertId()?

I'll write the tests.

re #4: I think I disagree that the try/catch should be in lastInsertId(). If you call lastInsertId() in a situation in which there is no valid last insert id, shouldn't it return an exception? The problem here is that InsertQuery::query() is calling lastInsertId() in just such a situation. InsertQuery::query() could be smart enough to detect that there is no last insert id and not try to return it, or if it does not want to deal with that alternatively it can catch the exception when it occurs and handle it. If anyone else calls lastInsertId() under the same conditions, they should have to make the same choice.

@jhedstrom: I did not mean to unassign the issue from you, it was a cross-post, but I cannot re-assign you, so you'll have to.

re #4: I think I disagree that the try/catch should be in lastInsertId(). If you call lastInsertId() in a situation in which there is no valid last insert id, shouldn't it return an exception? The problem here is that InsertQuery::query() is calling lastInsertId() in just such a situation. InsertQuery::query() could be smart enough to detect that there is no last insert id and not try to return it, or if it does not want to deal with that alternatively it can catch the exception when it occurs and handle it. If anyone else calls lastInsertId() under the same conditions, they should have to make the same choice.

No, typically lastInsertId() doesn't raise an exception. It does on PostgreSQL but not on the other database engines. This is something we want to solve directly there. Plus, we are only catching a very specific exception.

Here's a test. It currently fails because when a manual serial is inserted after nextval has been called, lastInsertId returns the previous auto-incremented ID instead of the new one (in the testcase, 5 is returned instead of the expected 7).

+++ includes/database/pgsql/database.inc
@@ -188,6 +188,30 @@ class DatabaseConnection_pgsql extends DatabaseConnection {
+        // This fail if a value was inserted into the serial column and

Nit: "fails"

+++ includes/database/pgsql/query.inc
@@ -69,6 +69,26 @@ class InsertQuery_pgsql extends InsertQuery {
+    if (!empty($table_information->serial_fields)) {
+      ¶
+      // If the first serial field is in the insert fields, update the

Nit: whitespace.

+++ includes/database/pgsql/query.inc
@@ -69,6 +69,26 @@ class InsertQuery_pgsql extends InsertQuery {
+      if (in_array($table_information->serial_fields[0], $this->insertFields)) {
+        $last_insert_id = $insert_values[array_search($table_information->serial_fields[0], $this->insertFields)];

This does an array search twice. Because FALSE is not a valid value for a serial column, we can replace this with one call to array_search, then test !== FALSE.

+++ includes/database/pgsql/query.inc
@@ -69,6 +69,26 @@ class InsertQuery_pgsql extends InsertQuery {
+      foreach ($table_information->serial_fields as $index => $serial_field) {

So, ummmm... I wanted to see if we could avoid the in_array on every element of serial_fields, because that is O(N^2) (though granted we almost never have more than one serial field, in fact I'm not sure even that is legal).

However, I cannot find anywhere in HEAD that uses or defines serial_fields at all. Does this property even exist?

+++ modules/simpletest/tests/database_test.test
@@ -561,6 +561,27 @@ class DatabaseInsertTestCase extends DatabaseTestCase {
+    // Test a manual insert.

It seems like we should have a test for inserting an explicit value, then verifying that an auto-increment value does not cause a unique key constraint.

Powered by Dreditor.

I am updating the patch as the styling suggestions, but this still needs review about the other logic suggestions.

BTW I tried the relevant tests on postgres and sqlite3 and they work fine in both databases types.

for the record, pay attention to the patch in #15, since my patch is a subset of that :-p

Ran the database tests on the last patch with sqlite; everything passed except DatabaseTransactionTestCase->testCommittedTransaction() and DatabaseTransactionTestCase->testTransactionRollBackSupported() which give General error: 5 database is locked.

The patch in #19 looks good to me, and the new tests pass on mysql and (bedir asserts) pgsql, thus fixing the pgsql issue. The new tests apparently reveal an existing bug in the sqlite driver. I believe our policy is not to hold up a commit due to problems on non-mysql drivers. Thus, RTBC.

SQLite is not affected by this patch. #20 is probably due to an old version of SQLite being used.

I'm not confortable with this part of the patch:

+          $new_value = $this->connection->query("SELECT NEXTVAL('" . $table_information->sequences[$index] . "') AS nextval, MAX(" . $serial_field . ") AS max FROM {" . $this->table . "}")->fetchObject();
+          // Set the current sequence value to the bigger of those two values.
+          $curval = $new_value->nextval > $new_value->max ? $new_value->nextval : $new_value->max;
+          $this->connection->query("SELECT SETVAL('" . $table_information->sequences[$index] . "', :curval)", array(':curval' => $curval));

... as this is prone to race conditions. At the minimum we should use only one query here, but we should study if an atomic solution is possible.

I reran the database transaction tests with sqlite 3.6.10, and am getting the same errors. I will try to update to 3.7.x and repeat

Same errors with sqlite 3.7.0.1

I found that "SELECT SETVAL('foo', (SELECT 30))" sets the counter to 30. So now I just need to find the syntax for selecting the max of (curval and max of column values) in a single expression... but at the moment I cannot access my pgsql server.

We discussed that with @berdir today, and the only way we can think of to make this purely atomic is to implement a while loop:

if (in_array($serial_field, $this->insertFields)) {
  // Load the next sequence value and the max value of the table.
  while ($this->connection->query("SELECT curval('" . $table_information->sequences[$index] . "') > (SELECT MAX(" . $table_information->sequences[$index] . ") FROM {" . $this->table . "})")->fetchField()) {
    $this->connection->query("SELECT nextval('" . $table_information->sequences[$index] . "')");
  }
}

It will be slow and clunky, but it's the only way I can see to avoid race-conditions. Most of the cases, it should not be a concern because this feature (inserting values into a serial field) is only used during imports, and most of the time the loop should only be used once.

okay

Wasn't this the problem with db_next_id? can't we reuse that code?

What if we always called setval('seq', 1, false) after creating a table with a sequence? This ensures that currval() and nextval() will work, and the "false" argument means that the next call to nextval() will return 1 instead of 2 so we won't change expected behavior.

We could do a similar trick during update on all such tables that have not yet been inserted into.

My suggestion in #30 would avoid the initial problem, generating an exception when calling db_insert() with an explicit value for a serial column that has never been auto-inserted into, but does nothing to help the followup problem of the key collision when the sequence catches up to explicitly inserted value.

chx just approached me in the coder lounger to ask why db_next_id() doesn't solve the problem. berdir is concerned about concurrency issues, but the implementation of nextId() for pgsql seems to address that by getting an advisory lock on the sequence and incrementing it until it exceeds a specified value. When we insert an explicit value, we could call db_next_id($explicit_value) to make sure that particular explicit value is exceeded by the sequence.

Oh, now I see berdir's concern. While nextId() is iterating, unrelated inserts can insert their own value at whatever the then-currval of the sequence is, which might happen to be exactly the explicit value we just inserted. This can happen because insert queries do not get the advisory lock that nextId() does. So, can we resolve this by having insert queries with explicit values for serial columns get the same advisory lock that nextId() does?

Here's another option: When we are about to insert an explicit value, increment the sequence *before* the insert until it exceeds the value we are inserting. Then do the insert. If two different inserts are running concurrently, presumably for different values (if they are trying to insert the same explicit value one of them would fail in any case), the sequence will first be incremented past the first one (which will then insert) and then past the second one (which will then insert), and they will both be happy.

Just discussed with berdir the following approach: *Before* the INSERT (say with id 42), do the following:

SELECT setval('bla_id_seq', GREATEST(MAX(id), 42)) from bla;
INSERT INTO bla(id, ...) VALUES(42, ...);

This will

• make the INSERT itself conflict only if there is already a row with id 42 (which must not happen anyway)
• any INSERT coming after the setval() start with id>=43

.

Patch follows.

You and bedir appear to have hit on the same idea I suggested in #31 in the last paragraph, "Here's another option...". So obviously I like the approach. :-)

+++ includes/database/pgsql/query.inc
@@ -43,6 +43,24 @@ class InsertQuery_pgsql extends InsertQuery {
+            // Force $last_insert_id to the specified value. This is only done
+            // if $index is 0.
+            if ($index == 0) {
+              $last_insert_id = $serial_value;
+            }

This confused me at first b/c I thought index 0 referred to the serial column's position in the table, not its position in the $table_information->serial_fields array. I suggest the comment should say: "This is only done for the first serial column in the table, because that is the column whose newly inserted value is returned as the last insert value."

Assuming that statement is true, of course...

+++ includes/database/pgsql/query.inc
@@ -43,6 +43,24 @@ class InsertQuery_pgsql extends InsertQuery {
+            $this->connection->query("SELECT setval('" . $table_information->sequences[$index] . "', GREATEST(MAX(" . $serial_field . "), :serial_value)) FROM {" . $this->table . "}", array(':serial_value' => (int)$serial_value));

See my suggestion in #30. If we use the optional third arg false to setval(), the next call to nextval() will return the value we are setting here.

Actually, now that you've gone and read #30, what we actually need to do is document for future that we CANNOT use that approach! I just realized it reintroduces a race condition. If we did that here, and another insert into the table occurred before the actual insert query lower in this function, the value we set here would be returned in the other query and the other query's value would be returned here. Wouldn't that be a fun bug to debug?

Let's document why we must do it the way this patch does it, and not using the third arg to setval().

I expect this will be RTBC when the comments are improved. Actually, I'll go improve them now and re-post.

Powered by Dreditor.

New patch. The only changes are comments, so I'm marking RTBC based on the code in the previous patch.

Same issue.

setval(sequence, GREATEST(MAX(field), value))

Is not concurrency safe. setval() never is, as far as I know. I believe that #26 is the only way forward.

Committed to HEAD.