Provide an IDS_COUNTER mechanism for ID assignment

Summary: See D3912 for discussion. InnoDB may reuse autoincrement IDs after restart; provide a way to avoid it.

Test Plan: Unit tests. Scheduled and executed tasks through `drydock lease --type host` and `phd debug taskmaster`.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D3914

resources/sql/patches/liskcounters.php

@@ -0,0 +1,42 @@
+<?php
+
+// Switch PhabricatorWorkerActiveTask from autoincrement IDs to counter IDs.
+// Set the initial counter ID to be larger than any known task ID.
+
+$active_table = new PhabricatorWorkerActiveTask();
+$archive_table = new PhabricatorWorkerArchiveTask();
+
+$conn_w = $active_table->establishConnection('w');
+
+$active_auto = head(queryfx_one(
+  $conn_w,
+  'SELECT auto_increment FROM information_schema.tables
+    WHERE table_name = %s
+    AND table_schema = DATABASE()',
+  $active_table->getTableName()));
+
+$active_max = head(queryfx_one(
+  $conn_w,
+  'SELECT MAX(id) FROM %T',
+  $active_table->getTableName()));
+
+$archive_max = head(queryfx_one(
+  $conn_w,
+  'SELECT MAX(id) FROM %T',
+  $archive_table->getTableName()));
+
+$initial_counter = max((int)$active_auto, (int)$active_max, (int)$archive_max);
+
+queryfx(
+  $conn_w,
+  'INSERT IGNORE INTO %T (counterName, counterValue)
+    VALUES (%s, %d)',
+  LiskDAO::COUNTER_TABLE_NAME,
+  $active_table->getTableName(),
+  $initial_counter + 1);
+
+// Drop AUTO_INCREMENT from the ID column.
+queryfx(
+  $conn_w,
+  'ALTER TABLE %T CHANGE id id INT UNSIGNED NOT NULL',
+  $active_table->getTableName());

resources/sql/patches/liskcounters.sql

@@ -0,0 +1,9 @@
+CREATE TABLE `{$NAMESPACE}_harbormaster`.`lisk_counter` (
+  counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY,
+  counterValue BIGINT UNSIGNED NOT NULL
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+
+CREATE TABLE `{$NAMESPACE}_worker`.`lisk_counter` (
+  counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY,
+  counterValue BIGINT UNSIGNED NOT NULL
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;

src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php

@@ -11,6 +11,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask {
 
   public function getConfiguration() {
     return array(
+      self::CONFIG_IDS => self::IDS_COUNTER,
       self::CONFIG_TIMESTAMPS => false,
     ) + parent::getConfiguration();
   }

src/infrastructure/storage/lisk/LiskDAO.php

@@ -177,9 +177,12 @@ abstract class LiskDAO {
   const SERIALIZATION_PHP           = 'php';
 
   const IDS_AUTOINCREMENT           = 'ids-auto';
+  const IDS_COUNTER                 = 'ids-counter';
   const IDS_PHID                    = 'ids-phid';
   const IDS_MANUAL                  = 'ids-manual';
 
+  const COUNTER_TABLE_NAME          = 'lisk_counter';
+
   private $__dirtyFields            = array();
   private $__missingFields          = array();
   private static $processIsolationLevel       = 0;
@@ -327,8 +330,23 @@ abstract class LiskDAO {
    * Lisk objects need to have a unique identifying ID. The three mechanisms
    * available for generating this ID are IDS_AUTOINCREMENT (default, assumes
    * the ID column is an autoincrement primary key), IDS_PHID (to generate a
-   * unique PHID for each object) or IDS_MANUAL (you are taking full
+   * unique PHID for each object), IDS_MANUAL (you are taking full
-   * responsibility for ID management).
+   * responsibility for ID management), or IDS_COUNTER (see below).
+   *
+   * InnoDB does not persist the value of `auto_increment` across restarts,
+   * and instead initializes it to `MAX(id) + 1` during startup. This means it
+   * may reissue the same autoincrement ID more than once, if the row is deleted
+   * and then the database is restarted. To avoid this, you can set an object to
+   * use a counter table with IDS_COUNTER. This will generally behave like
+   * IDS_AUTOINCREMENT, except that the counter value will persist across
+   * restarts and inserts will be slightly slower. If a database stores any
+   * DAOs which use this mechanism, you must create a table there with this
+   * schema:
+   *
+   *   CREATE TABLE lisk_counter (
+   *     counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY,
+   *     counterValue BIGINT UNSIGNED NOT NULL
+   *   ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
    *
    * CONFIG_TIMESTAMPS
    * Lisk can automatically handle keeping track of a `dateCreated' and
@@ -365,7 +383,6 @@ abstract class LiskDAO {
    * directly access or assign protected members of your class (use the getters
    * and setters).
    *
-   *
    * @return dictionary  Map of configuration options to values.
    *
    * @task   config
@@ -1181,7 +1198,6 @@ abstract class LiskDAO {
     return $this;
   }
 
-
   /**
    * Internal implementation of INSERT and REPLACE.
    *
@@ -1193,6 +1209,8 @@ abstract class LiskDAO {
     $this->willSaveObject();
     $data = $this->getPropertyValues();
 
+    $conn = $this->establishConnection('w');
+
     $id_mechanism = $this->getConfigOption(self::CONFIG_IDS);
     switch ($id_mechanism) {
       case self::IDS_AUTOINCREMENT:
@@ -1204,6 +1222,17 @@ abstract class LiskDAO {
           unset($data[$id_key]);
         }
         break;
+      case self::IDS_COUNTER:
+        // If we are using counter IDs, assign a new ID if we don't already have
+        // one.
+        $id_key = $this->getIDKeyForUse();
+        if (empty($data[$id_key])) {
+          $counter_name = $this->getTableName();
+          $id = self::loadNextCounterID($conn, $counter_name);
+          $this->setID($id);
+          $data[$id_key] = $id;
+        }
+        break;
       case self::IDS_PHID:
         if (empty($data[$this->getIDKeyForUse()])) {
           $phid = $this->generatePHID();
@@ -1219,8 +1248,6 @@ abstract class LiskDAO {
 
     $this->willWriteData($data);
 
-    $conn = $this->establishConnection('w');
-
     $columns = array_keys($data);
 
     foreach ($data as $key => $value) {
@@ -1761,4 +1788,37 @@ abstract class LiskDAO {
     $this->$name = $value;
   }
 
+  /**
+   * Increments a named counter and returns the next value.
+   *
+   * @param   AphrontDatabaseConnection   Database where the counter resides.
+   * @param   string                      Counter name to create or increment.
+   * @return  int                         Next counter value.
+   *
+   * @task util
+   */
+  public static function loadNextCounterID(
+    AphrontDatabaseConnection $conn_w,
+    $counter_name) {
+
+    // NOTE: If an insert does not touch an autoincrement row or call
+    // LAST_INSERT_ID(), MySQL normally does not change the value of
+    // LAST_INSERT_ID(). This can cause a counter's value to leak to a
+    // new counter if the second counter is created after the first one is
+    // updated. To avoid this, we insert LAST_INSERT_ID(1), to ensure the
+    // LAST_INSERT_ID() is always updated and always set correctly after the
+    // query completes.
+
+    queryfx(
+      $conn_w,
+      'INSERT INTO %T (counterName, counterValue) VALUES
+          (%s, LAST_INSERT_ID(1))
+        ON DUPLICATE KEY UPDATE
+          counterValue = LAST_INSERT_ID(counterValue + 1)',
+      self::COUNTER_TABLE_NAME,
+      $counter_name);
+
+    return $conn_w->getInsertID();
+  }
+
 }

src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php

@@ -95,5 +95,40 @@ final class LiskFixtureTestCase extends PhabricatorTestCase {
     $this->assertEqual(true, (bool)$load->load((string)$id));
   }
 
+  public function testCounters() {
+    $obj = new HarbormasterObject();
+    $conn_w = $obj->establishConnection('w');
+
+    // Test that the counter bascially behaves as expected.
+    $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'a'));
+    $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'a'));
+    $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_w, 'a'));
+
+    // This first insert is primarily a test that the previous LAST_INSERT_ID()
+    // value does not bleed into the creation of a new counter.
+    $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'b'));
+    $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'b'));
+
+    // These inserts alternate database connections. Since unit tests are
+    // transactional by default, we need to break out of them or we'll deadlock
+    // since the transactions don't normally close until we exit the test.
+    LiskDAO::endIsolateAllLiskEffectsToTransactions();
+    try {
+
+      $conn_1 = $obj->establishConnection('w', $force_new = true);
+      $conn_2 = $obj->establishConnection('w', $force_new = true);
+
+      $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_1, 'z'));
+      $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_2, 'z'));
+      $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_1, 'z'));
+      $this->assertEqual(4, LiskDAO::loadNextCounterID($conn_2, 'z'));
+      $this->assertEqual(5, LiskDAO::loadNextCounterID($conn_1, 'z'));
+
+      LiskDAO::beginIsolateAllLiskEffectsToTransactions();
+    } catch (Exception $ex) {
+      LiskDAO::beginIsolateAllLiskEffectsToTransactions();
+      throw $ex;
+    }
+  }
 
 }

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1012,6 +1012,14 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'sql',
         'name'    => $this->getPatchPath('drydockresourcetype.sql'),
       ),
+      'liskcounters.sql' => array(
+        'type'    => 'sql',
+        'name'    => $this->getPatchPath('liskcounters.sql'),
+      ),
+      'liskcounters.php' => array(
+        'type'    => 'php',
+        'name'    => $this->getPatchPath('liskcounters.php'),
+      ),
     );
   }