diff --git a/resources/sql/patches/liskcounters.php b/resources/sql/patches/liskcounters.php new file mode 100644 index 0000000000..1cfac2c99c --- /dev/null +++ b/resources/sql/patches/liskcounters.php @@ -0,0 +1,42 @@ +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()); diff --git a/resources/sql/patches/liskcounters.sql b/resources/sql/patches/liskcounters.sql new file mode 100644 index 0000000000..f617a9699f --- /dev/null +++ b/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; diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index 0364091cb7..37c750f9e8 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/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(); } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 954191ef25..b78a90ebda 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/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 - * responsibility for ID management). + * unique PHID for each object), IDS_MANUAL (you are taking full + * 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(); + } + } diff --git a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php index 0f3b1db99d..25e0208ebb 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php +++ b/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; + } + } } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 625572b3be..9a048650c1 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/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'), + ), ); }