Remove all optimistic lock code from Lisk

Summary: We never use this and almost certainly never will. It's been in Lisk for ~7 years but is a solution in search of a problem. It causes a conflict with any DAO that has a `version` column.

Test Plan: Browsed around, performed inserts and updates. Edited a Phriction document.

Reviewers: vrana, btrahan

Reviewed By: btrahan

CC: aran, leslie.chong

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

src/infrastructure/storage/lisk/LiskDAO.php

@@ -182,7 +182,6 @@
  */
 abstract class LiskDAO {
 
-  const CONFIG_OPTIMISTIC_LOCKS     = 'enable-locks';
   const CONFIG_IDS                  = 'id-mechanism';
   const CONFIG_TIMESTAMPS           = 'timestamps';
   const CONFIG_AUX_PHID             = 'auxiliary-phid';
@@ -211,7 +210,6 @@ abstract class LiskDAO {
 
   protected $id;
   protected $phid;
-  protected $version;
   protected $dateCreated;
   protected $dateModified;
 
@@ -329,7 +327,7 @@ abstract class LiskDAO {
 
 
   /**
-   * Change Lisk behaviors, like optimistic locks and timestamps. If you want
+   * Change Lisk behaviors, like ID configuration and timestamps. If you want
    * to change these behaviors, you should override this method in your child
    * class and change the options you're interested in. For example:
    *
@@ -341,21 +339,6 @@ abstract class LiskDAO {
    *
    * The available options are:
    *
-   * CONFIG_OPTIMISTIC_LOCKS
-   * Lisk automatically performs optimistic locking on objects, which protects
-   * you from read-modify-write concurrency problems. Lock failures are
-   * detected at write time and arise when two users read an object, then both
-   * save it. In theory, you should detect these failures and accommodate them
-   * in some sensible way (for instance, by showing the user differences
-   * between the original record and the copy they are trying to update, and
-   * prompting them to merge them). In practice, most Lisk tools are quick
-   * and dirty and don't get to that level of sophistication, but optimistic
-   * locks can still protect you from yourself sometimes. If you don't want
-   * to use optimistic locks, you can disable them. The performance cost of
-   * doing this locking is very very small (optimistic locks were chosen
-   * because they're simple and cheap, and highly optimized for the case where
-   * collisions are rare). By default, this option is OFF.
-   *
    * CONFIG_IDS
    * Lisk objects need to have a unique identifying ID. The three mechanisms
    * available for generating this ID are IDS_AUTOINCREMENT (default, assumes
@@ -405,7 +388,6 @@ abstract class LiskDAO {
    */
   protected function getConfiguration() {
     return array(
-      self::CONFIG_OPTIMISTIC_LOCKS         => false,
       self::CONFIG_IDS                      => self::IDS_AUTOINCREMENT,
       self::CONFIG_TIMESTAMPS               => true,
       self::CONFIG_PARTIAL_OBJECTS          => false,
@@ -605,9 +587,8 @@ abstract class LiskDAO {
 
   /**
    * Reload an object from the database, discarding any changes to persistent
-   * properties. If the object uses optimistic locks and you are in a locking
+   * properties. This is primarily useful after entering a transaction but
-   * mode while transactional, this will effectively synchronize the locks.
+   * before applying changes to an object.
-   * This is pretty heady. It is unlikely you need to use this method.
    *
    * @return this
    *
@@ -619,24 +600,13 @@ abstract class LiskDAO {
       throw new Exception("Unable to reload object that hasn't been loaded!");
     }
 
-    $use_locks = $this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS);
+    $result = $this->loadOneWhere(
-
+      '%C = %d',
-    if (!$use_locks) {
+      $this->getIDKeyForUse(),
-      $result = $this->loadOneWhere(
+      $this->getID());
-        '%C = %d',
-        $this->getIDKeyForUse(),
-        $this->getID());
-    } else {
-      $result = $this->loadOneWhere(
-        '%C = %d AND %C = %d',
-        $this->getIDKeyForUse(),
-        $this->getID(),
-        'version',
-        $this->getVersion());
-    }
 
     if (!$result) {
-      throw new AphrontQueryObjectMissingException($use_locks);
+      throw new AphrontQueryObjectMissingException();
     }
 
     return $this;
@@ -952,10 +922,6 @@ abstract class LiskDAO {
         unset($properties['id']);
       }
 
-      if (!$this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS)) {
-        unset($properties['version']);
-      }
-
       if (!$this->getConfigOption(self::CONFIG_TIMESTAMPS)) {
         unset($properties['datecreated']);
         unset($properties['datemodified']);
@@ -1155,7 +1121,6 @@ abstract class LiskDAO {
    */
   public function update() {
     $this->isEphemeralCheck();
-    $use_locks = $this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS);
 
     $this->willSaveObject();
     $data = $this->getPropertyValues();
@@ -1166,9 +1131,6 @@ abstract class LiskDAO {
 
     $map = array();
     foreach ($data as $k => $v) {
-      if ($use_locks && $k == 'version') {
-        continue;
-      }
       $map[$k] = $v;
     }
 
@@ -1179,31 +1141,16 @@ abstract class LiskDAO {
     }
     $map = implode(', ', $map);
 
-    if ($use_locks) {
+    $id = $this->getID();
-      $conn->query(
+    $conn->query(
-        'UPDATE %T SET %Q, version = version + 1 WHERE %C = %d AND %C = %d',
+      'UPDATE %T SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'),
-        $this->getTableName(),
+      $this->getTableName(),
-        $map,
+      $map,
-        $this->getIDKeyForUse(),
+      $this->getIDKeyForUse(),
-        $this->getID(),
+      $id);
-        'version',
+    // We can't detect a missing object because updating an object without
-        $this->getVersion());
+    // changing any values doesn't affect rows. We could jiggle timestamps
-      if ($conn->getAffectedRows() !== 1) {
+    // to catch this for objects which track them if we wanted.
-        throw new AphrontQueryObjectMissingException($use_locks);
-      }
-      $this->setVersion($this->getVersion() + 1);
-    } else {
-      $id = $this->getID();
-      $conn->query(
-        'UPDATE %T SET %Q WHERE %C = '.(is_int($id) ? '%d' : '%s'),
-        $this->getTableName(),
-        $map,
-        $this->getIDKeyForUse(),
-        $id);
-      // We can't detect a missing object because updating an object without
-      // changing any values doesn't affect rows. We could jiggle timestamps
-      // to catch this for objects which track them if we wanted.
-    }
 
     $this->didWriteData();
 
@@ -1274,10 +1221,6 @@ abstract class LiskDAO {
         throw new Exception('Unknown CONFIG_IDs mechanism!');
     }
 
-    if ($this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS)) {
-      $data['version'] = 0;
-    }
-
     $this->willWriteData($data);
 
     $conn = $this->establishConnection('w');
@@ -1296,11 +1239,6 @@ abstract class LiskDAO {
       $columns,
       $data);
 
-    // Update the object with the initial Version value
-    if ($this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS)) {
-      $this->setVersion(0);
-    }
-
     // Only use the insert id if this table is using auto-increment ids
     if ($id_mechanism === self::IDS_AUTOINCREMENT) {
       $this->setID($conn->getInsertID());
@@ -1323,24 +1261,12 @@ abstract class LiskDAO {
    */
   protected function shouldInsertWhenSaved() {
     $key_type = $this->getConfigOption(self::CONFIG_IDS);
-    $use_locks = $this->getConfigOption(self::CONFIG_OPTIMISTIC_LOCKS);
 
     if ($key_type == self::IDS_MANUAL) {
-      if ($use_locks) {
+      throw new Exception(
-        // If we are manually keyed and the object has a version (which means
+        'You are using manual IDs. You must override the '.
-        // that it has been saved to the DB before), do an update, otherwise
+        'shouldInsertWhenSaved() method to properly detect '.
-        // perform an insert.
+        'when to insert a new record.');
-        if ($this->getID() && $this->getVersion() !== null) {
-          return false;
-        } else {
-          return true;
-        }
-      } else {
-        throw new Exception(
-          'You are not using optimistic locks, but are using manual IDs. You '.
-          'must override the shouldInsertWhenSaved() method to properly '.
-          'detect when to insert a new record.');
-      }
     } else {
       return !$this->getID();
     }