From ea32c541e41797828e143092a5a431964eea910b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Oct 2012 13:55:43 -0700 Subject: [PATCH] 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 | 118 ++++---------------- 1 file changed, 22 insertions(+), 96 deletions(-) diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 86b61b58f2..47092515ea 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/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 - * mode while transactional, this will effectively synchronize the locks. - * This is pretty heady. It is unlikely you need to use this method. + * properties. This is primarily useful after entering a transaction but + * before applying changes to an object. * * @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); - - if (!$use_locks) { - $result = $this->loadOneWhere( - '%C = %d', - $this->getIDKeyForUse(), - $this->getID()); - } else { - $result = $this->loadOneWhere( - '%C = %d AND %C = %d', - $this->getIDKeyForUse(), - $this->getID(), - 'version', - $this->getVersion()); - } + $result = $this->loadOneWhere( + '%C = %d', + $this->getIDKeyForUse(), + $this->getID()); 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) { - $conn->query( - 'UPDATE %T SET %Q, version = version + 1 WHERE %C = %d AND %C = %d', - $this->getTableName(), - $map, - $this->getIDKeyForUse(), - $this->getID(), - 'version', - $this->getVersion()); - if ($conn->getAffectedRows() !== 1) { - 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. - } + $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) { - // If we are manually keyed and the object has a version (which means - // that it has been saved to the DB before), do an update, otherwise - // perform an insert. - 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.'); - } + throw new Exception( + 'You are using manual IDs. You must override the '. + 'shouldInsertWhenSaved() method to properly detect '. + 'when to insert a new record.'); } else { return !$this->getID(); }