From 20a9c8d24ed85238b0a79cecf68a2a6b949457a5 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 29 Jun 2019 10:26:37 -0700 Subject: [PATCH] bagostuff: optimize SqlBagOStuff and fix failing segmentation tests In SqlBagOStuff: * Add modifyMulti() helper method to reduce code duplication * Improve atomicity of add(), cas(), and changeTTL() queries * Avoid integer serialization and improve atomicity of incr() * Optimize new BagOStuff::changeTTLMulti() method In BagOStuff: * Add changeTTLMulti() method for subclasses to optimize * Make set() ignore WRITE_ALLOW_SEGMENTS for integers so incr() works * Strip WRITE_ALLOW_SEGMENTS flag from the setMulti() call in set() to avoid triggering bogus sanity check exceptions * Fix BagOStuffTest::testSetSegmentable failures via the above changes * Enforce WRITE_ALLOW_SEGMENTS sanity check in setMulti() for all the subclasses by using a final wrapper method * Add WRITE_ALLOW_SEGMENTS sanity check to deleteMulti() Bug: T113916 Change-Id: I25d1790fa9b0d1837643efccfa94a12043cfbf42 --- includes/libs/objectcache/BagOStuff.php | 84 ++++- .../objectcache/MemcachedPeclBagOStuff.php | 6 +- .../objectcache/MemcachedPhpBagOStuff.php | 2 +- .../libs/objectcache/MultiWriteBagOStuff.php | 8 +- includes/libs/objectcache/RedisBagOStuff.php | 6 +- .../libs/objectcache/ReplicatedBagOStuff.php | 8 +- includes/objectcache/SqlBagOStuff.php | 349 +++++++++--------- .../libs/objectcache/BagOStuffTest.php | 51 +++ 8 files changed, 310 insertions(+), 204 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index 7ebbe8bcb7..d13626ae1b 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -258,6 +258,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar */ public function set( $key, $value, $exptime = 0, $flags = 0 ) { if ( + is_int( $value ) || // avoid breaking incr()/decr() ( $flags & self::WRITE_ALLOW_SEGMENTS ) != self::WRITE_ALLOW_SEGMENTS || is_infinite( $this->segmentationSize ) ) { @@ -294,6 +295,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar $segmentHashes[] = $hash; } + $flags &= ~self::WRITE_ALLOW_SEGMENTS; // sanity $ok = $this->setMulti( $chunksByKey, $exptime, $flags ); if ( $ok ) { // Only when all segments are stored should the main key be changed @@ -503,6 +505,16 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar * @since 1.28 */ public function changeTTL( $key, $exptime = 0, $flags = 0 ) { + return $this->doChangeTTL( $key, $exptime, $flags ); + } + + /** + * @param string $key + * @param int $exptime + * @param int $flags + * @return bool + */ + protected function doChangeTTL( $key, $exptime, $flags ) { $expiry = $this->convertToExpiry( $exptime ); $delete = ( $expiry != 0 && $expiry < $this->getCurrentTime() ); @@ -673,7 +685,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar * Get an associative array containing the item for each of the keys that have items. * @param string[] $keys List of keys * @param int $flags Bitfield; supports READ_LATEST [optional] - * @return array + * @return array Map of (key => value) for existing keys */ public function getMulti( array $keys, $flags = 0 ) { $valuesBykey = $this->doGetMulti( $keys, $flags ); @@ -689,7 +701,7 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar * Get an associative array containing the item for each of the keys that have items. * @param string[] $keys List of keys * @param int $flags Bitfield; supports READ_LATEST [optional] - * @return array + * @return array Map of (key => value) for existing keys */ protected function doGetMulti( array $keys, $flags = 0 ) { $res = []; @@ -714,11 +726,21 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar * @return bool Success * @since 1.24 */ - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { + final public function setMulti( array $data, $exptime = 0, $flags = 0 ) { if ( ( $flags & self::WRITE_ALLOW_SEGMENTS ) === self::WRITE_ALLOW_SEGMENTS ) { throw new InvalidArgumentException( __METHOD__ . ' got WRITE_ALLOW_SEGMENTS' ); } + return $this->doSetMulti( $data, $exptime, $flags ); + } + + /** + * @param mixed[] $data Map of (key => value) + * @param int $exptime Either an interval in seconds or a unix timestamp for expiry + * @param int $flags Bitfield of BagOStuff::WRITE_* constants + * @return bool Success + */ + protected function doSetMulti( array $data, $exptime = 0, $flags = 0 ) { $res = true; foreach ( $data as $key => $value ) { $res = $this->doSet( $key, $value, $exptime, $flags ) && $res; @@ -737,7 +759,20 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar * @return bool Success * @since 1.33 */ - public function deleteMulti( array $keys, $flags = 0 ) { + final public function deleteMulti( array $keys, $flags = 0 ) { + if ( ( $flags & self::WRITE_ALLOW_SEGMENTS ) === self::WRITE_ALLOW_SEGMENTS ) { + throw new InvalidArgumentException( __METHOD__ . ' got WRITE_ALLOW_SEGMENTS' ); + } + + return $this->doDeleteMulti( $keys, $flags ); + } + + /** + * @param string[] $keys List of keys + * @param int $flags Bitfield of BagOStuff::WRITE_* constants + * @return bool Success + */ + protected function doDeleteMulti( array $keys, $flags = 0 ) { $res = true; foreach ( $keys as $key ) { $res = $this->doDelete( $key, $flags ) && $res; @@ -746,6 +781,26 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar return $res; } + /** + * Change the expiration of multiple keys that exist + * + * @see BagOStuff::changeTTL() + * + * @param string[] $keys List of keys + * @param int $exptime TTL or UNIX timestamp + * @param int $flags Bitfield of BagOStuff::WRITE_* constants (since 1.33) + * @return bool Success + * @since 1.34 + */ + public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) { + $res = true; + foreach ( $keys as $key ) { + $res = $this->doChangeTTL( $key, $exptime, $flags ) && $res; + } + + return $res; + } + /** * Increase stored value of $key by $value while preserving its TTL * @param string $key Key to increase @@ -898,16 +953,23 @@ abstract class BagOStuff implements IExpiringStore, IStoreKeyEncoder, LoggerAwar } /** - * Convert an optionally relative time to an absolute time - * @param int $exptime + * Convert an optionally relative timestamp to an absolute time + * + * The input value will be cast to an integer and interpreted as follows: + * - zero: no expiry; return zero (e.g. TTL_INDEFINITE) + * - negative: relative TTL; return UNIX timestamp offset by this value + * - positive (< 10 years): relative TTL; return UNIX timestamp offset by this value + * - positive (>= 10 years): absolute UNIX timestamp; return this value + * + * @param int $exptime Absolute TTL or 0 for indefinite * @return int */ protected function convertToExpiry( $exptime ) { - if ( $this->expiryIsRelative( $exptime ) ) { - return (int)$this->getCurrentTime() + $exptime; - } else { - return $exptime; - } + $exptime = (int)$exptime; // sanity + + return $this->expiryIsRelative( $exptime ) + ? (int)$this->getCurrentTime() + $exptime + : $exptime; } /** diff --git a/includes/libs/objectcache/MemcachedPeclBagOStuff.php b/includes/libs/objectcache/MemcachedPeclBagOStuff.php index 43cebd3255..ea0090f528 100644 --- a/includes/libs/objectcache/MemcachedPeclBagOStuff.php +++ b/includes/libs/objectcache/MemcachedPeclBagOStuff.php @@ -259,7 +259,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff { return $this->checkResult( false, $result ); } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { + public function doSetMulti( array $data, $exptime = 0, $flags = 0 ) { $this->debug( 'setMulti(' . implode( ', ', array_keys( $data ) ) . ')' ); foreach ( array_keys( $data ) as $key ) { $this->validateKeyEncoding( $key ); @@ -268,7 +268,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff { return $this->checkResult( false, $result ); } - public function deleteMulti( array $keys, $flags = 0 ) { + public function doDeleteMulti( array $keys, $flags = 0 ) { $this->debug( 'deleteMulti(' . implode( ', ', $keys ) . ')' ); foreach ( $keys as $key ) { $this->validateKeyEncoding( $key ); @@ -284,7 +284,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff { return $this->checkResult( false, $ok ); } - public function changeTTL( $key, $exptime = 0, $flags = 0 ) { + protected function doChangeTTL( $key, $exptime, $flags ) { $this->debug( "touch($key)" ); $result = $this->client->touch( $key, $exptime ); return $this->checkResult( $key, $result ); diff --git a/includes/libs/objectcache/MemcachedPhpBagOStuff.php b/includes/libs/objectcache/MemcachedPhpBagOStuff.php index ea73cbaee7..5a67a0d2a3 100644 --- a/includes/libs/objectcache/MemcachedPhpBagOStuff.php +++ b/includes/libs/objectcache/MemcachedPhpBagOStuff.php @@ -101,7 +101,7 @@ class MemcachedPhpBagOStuff extends MemcachedBagOStuff { return ( $n !== false && $n !== null ) ? $n : false; } - public function changeTTL( $key, $exptime = 0, $flags = 0 ) { + protected function doChangeTTL( $key, $exptime, $flags ) { return $this->client->touch( $this->validateKeyEncoding( $key ), $this->fixExpiry( $exptime ) diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index 4c6750fcbe..2df8f0c630 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -236,7 +236,7 @@ class MultiWriteBagOStuff extends BagOStuff { return $res; } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { + public function doSetMulti( array $data, $exptime = 0, $flags = 0 ) { return $this->doWrite( $this->cacheIndexes, $this->usesAsyncWritesGivenFlags( $flags ), @@ -245,7 +245,7 @@ class MultiWriteBagOStuff extends BagOStuff { ); } - public function deleteMulti( array $data, $flags = 0 ) { + public function doDeleteMulti( array $data, $flags = 0 ) { return $this->doWrite( $this->cacheIndexes, $this->usesAsyncWritesGivenFlags( $flags ), @@ -362,6 +362,10 @@ class MultiWriteBagOStuff extends BagOStuff { throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); } + protected function doChangeTTL( $key, $exptime, $flags ) { + throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); + } + protected function doGetMulti( array $keys, $flags = 0 ) { throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); } diff --git a/includes/libs/objectcache/RedisBagOStuff.php b/includes/libs/objectcache/RedisBagOStuff.php index 743b9eba2c..f67b887335 100644 --- a/includes/libs/objectcache/RedisBagOStuff.php +++ b/includes/libs/objectcache/RedisBagOStuff.php @@ -185,7 +185,7 @@ class RedisBagOStuff extends BagOStuff { return $result; } - public function setMulti( array $data, $expiry = 0, $flags = 0 ) { + public function doSetMulti( array $data, $expiry = 0, $flags = 0 ) { $batches = []; $conns = []; foreach ( $data as $key => $value ) { @@ -229,7 +229,7 @@ class RedisBagOStuff extends BagOStuff { return $result; } - public function deleteMulti( array $keys, $flags = 0 ) { + public function doDeleteMulti( array $keys, $flags = 0 ) { $batches = []; $conns = []; foreach ( $keys as $key ) { @@ -325,7 +325,7 @@ class RedisBagOStuff extends BagOStuff { return $result; } - public function changeTTL( $key, $exptime = 0, $flags = 0 ) { + protected function doChangeTTL( $key, $exptime, $flags ) { list( $server, $conn ) = $this->getConnection( $key ); if ( !$conn ) { return false; diff --git a/includes/libs/objectcache/ReplicatedBagOStuff.php b/includes/libs/objectcache/ReplicatedBagOStuff.php index 8502ce2704..8c2b3f685b 100644 --- a/includes/libs/objectcache/ReplicatedBagOStuff.php +++ b/includes/libs/objectcache/ReplicatedBagOStuff.php @@ -126,11 +126,11 @@ class ReplicatedBagOStuff extends BagOStuff { : $this->readStore->getMulti( $keys, $flags ); } - public function setMulti( array $data, $exptime = 0, $flags = 0 ) { + public function doSetMulti( array $data, $exptime = 0, $flags = 0 ) { return $this->writeStore->setMulti( $data, $exptime, $flags ); } - public function deleteMulti( array $keys, $flags = 0 ) { + public function doDeleteMulti( array $keys, $flags = 0 ) { return $this->writeStore->deleteMulti( $keys, $flags ); } @@ -181,6 +181,10 @@ class ReplicatedBagOStuff extends BagOStuff { throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); } + protected function doChangeTTL( $key, $exptime, $flags ) { + throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); + } + protected function doGetMulti( array $keys, $flags = 0 ) { throw new LogicException( __METHOD__ . ': proxy class does not need this method.' ); } diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index c3a5897daa..8bc053d01e 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -68,7 +68,16 @@ class SqlBagOStuff extends BagOStuff { protected $connFailureErrors = []; /** @var int */ - const GARBAGE_COLLECT_DELAY_SEC = 1; + private static $GARBAGE_COLLECT_DELAY_SEC = 1; + + /** @var string */ + private static $OP_SET = 'set'; + /** @var string */ + private static $OP_ADD = 'add'; + /** @var string */ + private static $OP_TOUCH = 'touch'; + /** @var string */ + private static $OP_DELETE = 'delete'; /** * Constructor. Parameters are: @@ -311,7 +320,7 @@ class SqlBagOStuff extends BagOStuff { if ( isset( $dataRows[$key] ) ) { // HIT? $row = $dataRows[$key]; $this->debug( "get: retrieved data; expiry time is " . $row->exptime ); - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $row->serverIndex ); if ( $this->isExpired( $db, $row->exptime ) ) { // MISS @@ -330,59 +339,51 @@ class SqlBagOStuff extends BagOStuff { return $values; } - public function setMulti( array $data, $expiry = 0, $flags = 0 ) { - return $this->insertMulti( $data, $expiry, $flags, true ); + public function doSetMulti( array $data, $exptime = 0, $flags = 0 ) { + return $this->modifyMulti( $data, $exptime, $flags, self::$OP_SET ); } - private function insertMulti( array $data, $expiry, $flags, $replace ) { + /** + * @param mixed[]|null[] $data Map of (key => new value or null) + * @param int $exptime UNIX timestamp, TTL in seconds, or 0 (no expiration) + * @param int $flags Bitfield of BagOStuff::WRITE_* constants + * @param string $op Cache operation + * @return bool + */ + private function modifyMulti( array $data, $exptime, $flags, $op ) { $keysByTable = []; foreach ( $data as $key => $value ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); $keysByTable[$serverIndex][$tableName][] = $key; } + $exptime = $this->convertToExpiry( $exptime ); + $result = true; - $exptime = (int)$expiry; /** @noinspection PhpUnusedLocalVariableInspection */ $silenceScope = $this->silenceTransactionProfiler(); foreach ( $keysByTable as $serverIndex => $serverKeys ) { - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); - $this->occasionallyGarbageCollect( $db ); + $this->occasionallyGarbageCollect( $db ); // expire old entries if any + $dbExpiry = $exptime ? $db->timestamp( $exptime ) : $this->getMaxDateTime( $db ); } catch ( DBError $e ) { $this->handleWriteError( $e, $db, $serverIndex ); $result = false; continue; } - if ( $exptime < 0 ) { - $exptime = 0; - } - - if ( $exptime == 0 ) { - $encExpiry = $this->getMaxDateTime( $db ); - } else { - $exptime = $this->convertToExpiry( $exptime ); - $encExpiry = $db->timestamp( $exptime ); - } foreach ( $serverKeys as $tableName => $tableKeys ) { - $rows = []; - foreach ( $tableKeys as $key ) { - $rows[] = [ - 'keyname' => $key, - 'value' => $db->encodeBlob( $this->serialize( $data[$key] ) ), - 'exptime' => $encExpiry, - ]; - } - try { - if ( $replace ) { - $db->replace( $tableName, [ 'keyname' ], $rows, __METHOD__ ); - } else { - $db->insert( $tableName, $rows, __METHOD__, [ 'IGNORE' ] ); - $result = ( $db->affectedRows() > 0 && $result ); - } + $result = $this->updateTableKeys( + $op, + $db, + $tableName, + $tableKeys, + $data, + $dbExpiry + ) && $result; } catch ( DBError $e ) { $this->handleWriteError( $e, $db, $serverIndex ); $result = false; @@ -398,37 +399,88 @@ class SqlBagOStuff extends BagOStuff { return $result; } - protected function doSet( $key, $value, $exptime = 0, $flags = 0 ) { - $ok = $this->insertMulti( [ $key => $value ], $exptime, $flags, true ); + /** + * @param string $op + * @param IDatabase $db + * @param string $table + * @param string[] $tableKeys Keys in $data to update + * @param mixed[]|null[] $data Map of (key => new value or null) + * @param string $dbExpiry DB-encoded expiry + * @return bool + * @throws DBError + * @throws InvalidArgumentException + */ + private function updateTableKeys( $op, $db, $table, $tableKeys, $data, $dbExpiry ) { + $success = true; - return $ok; + if ( $op === self::$OP_ADD ) { + $rows = []; + foreach ( $tableKeys as $key ) { + $rows[] = [ + 'keyname' => $key, + 'value' => $db->encodeBlob( $this->serialize( $data[$key] ) ), + 'exptime' => $dbExpiry + ]; + } + $db->delete( + $table, + [ + 'keyname' => $tableKeys, + 'exptime <= ' . $db->addQuotes( $db->timestamp() ) + ], + __METHOD__ + ); + $db->insert( $table, $rows, __METHOD__, [ 'IGNORE' ] ); + + $success = ( $db->affectedRows() == count( $rows ) ); + } elseif ( $op === self::$OP_SET ) { + $rows = []; + foreach ( $tableKeys as $key ) { + $rows[] = [ + 'keyname' => $key, + 'value' => $db->encodeBlob( $this->serialize( $data[$key] ) ), + 'exptime' => $dbExpiry + ]; + } + $db->replace( $table, [ 'keyname' ], $rows, __METHOD__ ); + } elseif ( $op === self::$OP_DELETE ) { + $db->delete( $table, [ 'keyname' => $tableKeys ], __METHOD__ ); + } elseif ( $op === self::$OP_TOUCH ) { + $db->update( + $table, + [ 'exptime' => $dbExpiry ], + [ + 'keyname' => $tableKeys, + 'exptime > ' . $db->addQuotes( $db->timestamp() ) + ], + __METHOD__ + ); + + $success = ( $db->affectedRows() == count( $tableKeys ) ); + } else { + throw new InvalidArgumentException( "Invalid operation '$op'" ); + } + + return $success; } - public function add( $key, $value, $exptime = 0, $flags = 0 ) { - $added = $this->insertMulti( [ $key => $value ], $exptime, $flags, false ); + protected function doSet( $key, $value, $exptime = 0, $flags = 0 ) { + return $this->modifyMulti( [ $key => $value ], $exptime, $flags, self::$OP_SET ); + } - return $added; + public function add( $key, $value, $exptime = 0, $flags = 0 ) { + return $this->modifyMulti( [ $key => $value ], $exptime, $flags, self::$OP_ADD ); } protected function cas( $casToken, $key, $value, $exptime = 0, $flags = 0 ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); - $db = null; + $exptime = $this->convertToExpiry( $exptime ); + /** @noinspection PhpUnusedLocalVariableInspection */ $silenceScope = $this->silenceTransactionProfiler(); + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); - $exptime = intval( $exptime ); - - if ( $exptime < 0 ) { - $exptime = 0; - } - - if ( $exptime == 0 ) { - $encExpiry = $this->getMaxDateTime( $db ); - } else { - $exptime = $this->convertToExpiry( $exptime ); - $encExpiry = $db->timestamp( $exptime ); - } // (T26425) use a replace if the db supports it instead of // delete/insert to avoid clashes with conflicting keynames $db->update( @@ -436,11 +488,14 @@ class SqlBagOStuff extends BagOStuff { [ 'keyname' => $key, 'value' => $db->encodeBlob( $this->serialize( $value ) ), - 'exptime' => $encExpiry + 'exptime' => $exptime + ? $db->timestamp( $exptime ) + : $this->getMaxDateTime( $db ) ], [ 'keyname' => $key, - 'value' => $db->encodeBlob( $casToken ) + 'value' => $db->encodeBlob( $casToken ), + 'exptime > ' . $db->addQuotes( $db->timestamp() ) ], __METHOD__ ); @@ -453,102 +508,51 @@ class SqlBagOStuff extends BagOStuff { return (bool)$db->affectedRows(); } - public function deleteMulti( array $keys, $flags = 0 ) { - return $this->purgeMulti( $keys, $flags ); - } - - public function purgeMulti( array $keys, $flags = 0 ) { - $keysByTable = []; - foreach ( $keys as $key ) { - list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); - $keysByTable[$serverIndex][$tableName][] = $key; - } - - $result = true; - /** @noinspection PhpUnusedLocalVariableInspection */ - $silenceScope = $this->silenceTransactionProfiler(); - foreach ( $keysByTable as $serverIndex => $serverKeys ) { - $db = null; - try { - $db = $this->getDB( $serverIndex ); - } catch ( DBError $e ) { - $this->handleWriteError( $e, $db, $serverIndex ); - $result = false; - continue; - } - - foreach ( $serverKeys as $tableName => $tableKeys ) { - try { - $db->delete( $tableName, [ 'keyname' => $tableKeys ], __METHOD__ ); - } catch ( DBError $e ) { - $this->handleWriteError( $e, $db, $serverIndex ); - $result = false; - } - - } - } - - if ( ( $flags & self::WRITE_SYNC ) == self::WRITE_SYNC ) { - $result = $this->waitForReplication() && $result; - } - - return $result; + public function doDeleteMulti( array $keys, $flags = 0 ) { + return $this->modifyMulti( + array_fill_keys( $keys, null ), + 0, + $flags, + self::$OP_DELETE + ); } protected function doDelete( $key, $flags = 0 ) { - $ok = $this->purgeMulti( [ $key ], $flags ); - - return $ok; + return $this->modifyMulti( [ $key => null ], 0, $flags, self::$OP_DELETE ); } public function incr( $key, $step = 1 ) { list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); - $db = null; + + $newCount = false; /** @noinspection PhpUnusedLocalVariableInspection */ $silenceScope = $this->silenceTransactionProfiler(); + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); - $step = intval( $step ); - $row = $db->selectRow( - $tableName, - [ 'value', 'exptime' ], - [ 'keyname' => $key ], - __METHOD__, - [ 'FOR UPDATE' ] - ); - if ( $row === false ) { - // Missing - return false; - } - $db->delete( $tableName, [ 'keyname' => $key ], __METHOD__ ); - if ( $this->isExpired( $db, $row->exptime ) ) { - // Expired, do not reinsert - return false; - } - - $oldValue = intval( $this->unserialize( $db->decodeBlob( $row->value ) ) ); - $newValue = $oldValue + $step; - $db->insert( + $encTimestamp = $db->addQuotes( $db->timestamp() ); + $db->update( $tableName, - [ - 'keyname' => $key, - 'value' => $db->encodeBlob( $this->serialize( $newValue ) ), - 'exptime' => $row->exptime - ], - __METHOD__, - [ 'IGNORE' ] + [ 'value = value + ' . (int)$step ], + [ 'keyname' => $key, "exptime > $encTimestamp" ], + __METHOD__ ); - - if ( $db->affectedRows() == 0 ) { - // Race condition. See T30611 - $newValue = false; + if ( $db->affectedRows() > 0 ) { + $newValue = $db->selectField( + $tableName, + 'value', + [ 'keyname' => $key, "exptime > $encTimestamp" ], + __METHOD__ + ); + if ( $this->isInteger( $newValue ) ) { + $newCount = (int)$newValue; + } } } catch ( DBError $e ) { $this->handleWriteError( $e, $db, $serverIndex ); - return false; } - return $newValue; + return $newCount; } public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) { @@ -560,41 +564,17 @@ class SqlBagOStuff extends BagOStuff { return $ok; } - public function changeTTL( $key, $exptime = 0, $flags = 0 ) { - list( $serverIndex, $tableName ) = $this->getTableByKey( $key ); - $db = null; - /** @noinspection PhpUnusedLocalVariableInspection */ - $silenceScope = $this->silenceTransactionProfiler(); - try { - $db = $this->getDB( $serverIndex ); - if ( $exptime == 0 ) { - $timestamp = $this->getMaxDateTime( $db ); - } else { - $timestamp = $db->timestamp( $this->convertToExpiry( $exptime ) ); - } - $db->update( - $tableName, - [ 'exptime' => $timestamp ], - [ 'keyname' => $key, 'exptime > ' . $db->addQuotes( $db->timestamp( time() ) ) ], - __METHOD__ - ); - if ( $db->affectedRows() == 0 ) { - $exists = (bool)$db->selectField( - $tableName, - 1, - [ 'keyname' => $key, 'exptime' => $timestamp ], - __METHOD__, - [ 'FOR UPDATE' ] - ); - - return $exists; - } - } catch ( DBError $e ) { - $this->handleWriteError( $e, $db, $serverIndex ); - return false; - } + public function changeTTLMulti( array $keys, $exptime, $flags = 0 ) { + return $this->modifyMulti( + array_fill_keys( $keys, null ), + $exptime, + $flags, + self::$OP_TOUCH + ); + } - return true; + protected function doChangeTTL( $key, $exptime, $flags ) { + return $this->modifyMulti( [ $key => null ], $exptime, $flags, self::$OP_TOUCH ); } /** @@ -634,7 +614,7 @@ class SqlBagOStuff extends BagOStuff { // Only purge on one in every $this->purgePeriod writes mt_rand( 0, $this->purgePeriod - 1 ) == 0 && // Avoid repeating the delete within a few seconds - ( time() - $this->lastGarbageCollect ) > self::GARBAGE_COLLECT_DELAY_SEC + ( time() - $this->lastGarbageCollect ) > self::$GARBAGE_COLLECT_DELAY_SEC ) { $garbageCollector = function () use ( $db ) { $this->deleteServerObjectsExpiringBefore( $db, time(), null, $this->purgeLimit ); @@ -668,7 +648,7 @@ class SqlBagOStuff extends BagOStuff { $keysDeletedCount = 0; foreach ( $serverIndexes as $numServersDone => $serverIndex ) { - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); $this->deleteServerObjectsExpiringBefore( @@ -773,7 +753,7 @@ class SqlBagOStuff extends BagOStuff { /** @noinspection PhpUnusedLocalVariableInspection */ $silenceScope = $this->silenceTransactionProfiler(); for ( $serverIndex = 0; $serverIndex < $this->numServers; $serverIndex++ ) { - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); for ( $i = 0; $i < $this->shards; $i++ ) { @@ -800,7 +780,7 @@ class SqlBagOStuff extends BagOStuff { list( $serverIndex ) = $this->getTableByKey( $key ); - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); $ok = $db->lock( $key, __METHOD__, $timeout ); @@ -832,7 +812,7 @@ class SqlBagOStuff extends BagOStuff { list( $serverIndex ) = $this->getTableByKey( $key ); - $db = null; + $db = null; // in case of connection failure try { $db = $this->getDB( $serverIndex ); $ok = $db->unlock( $key, __METHOD__ ); @@ -846,11 +826,11 @@ class SqlBagOStuff extends BagOStuff { $this->handleWriteError( $e, $db, $serverIndex ); $ok = false; } - } else { - $ok = false; + + return $ok; } - return $ok; + return true; } /** @@ -859,16 +839,19 @@ class SqlBagOStuff extends BagOStuff { * in storage requirements. * * @param mixed $data - * @return string + * @return string|int */ protected function serialize( $data ) { - $serial = serialize( $data ); + if ( is_int( $data ) ) { + return $data; + } + $serial = serialize( $data ); if ( function_exists( 'gzdeflate' ) ) { - return gzdeflate( $serial ); - } else { - return $serial; + $serial = gzdeflate( $serial ); } + + return $serial; } /** @@ -877,6 +860,10 @@ class SqlBagOStuff extends BagOStuff { * @return mixed */ protected function unserialize( $serial ) { + if ( $this->isInteger( $serial ) ) { + return (int)$serial; + } + if ( function_exists( 'gzinflate' ) ) { Wikimedia\suppressWarnings(); $decomp = gzinflate( $serial ); @@ -887,9 +874,7 @@ class SqlBagOStuff extends BagOStuff { } } - $ret = unserialize( $serial ); - - return $ret; + return unserialize( $serial ); } /** diff --git a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php index 1d11fd8ef2..98849873ca 100644 --- a/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/BagOStuffTest.php @@ -129,6 +129,56 @@ class BagOStuffTest extends MediaWikiTestCase { $this->assertFalse( $this->cache->get( $key ) ); } + /** + * @covers BagOStuff::changeTTLMulti + */ + public function testChangeTTLMulti() { + $key1 = $this->cache->makeKey( 'test-key1' ); + $key2 = $this->cache->makeKey( 'test-key2' ); + $key3 = $this->cache->makeKey( 'test-key3' ); + $key4 = $this->cache->makeKey( 'test-key4' ); + + // cleanup + $this->cache->delete( $key1 ); + $this->cache->delete( $key2 ); + $this->cache->delete( $key3 ); + $this->cache->delete( $key4 ); + + $ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], 30 ); + $this->assertFalse( $ok, "No keys found" ); + $this->assertFalse( $this->cache->get( $key1 ) ); + $this->assertFalse( $this->cache->get( $key2 ) ); + $this->assertFalse( $this->cache->get( $key3 ) ); + + $this->cache->setMulti( [ + $key1 => 1, + $key2 => 2, + $key3 => 3 + ] ); + + $ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], 300 ); + $this->assertTrue( $ok, "TTL bumped for all keys" ); + $this->assertEquals( 1, $this->cache->get( $key1 ) ); + $this->assertEquals( 2, $this->cache->get( $key2 ) ); + $this->assertEquals( 3, $this->cache->get( $key3 ) ); + + $ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3 ], time() + 86400 ); + $this->assertTrue( $ok, "Expiry set for all keys" ); + + $ok = $this->cache->changeTTLMulti( [ $key1, $key2, $key3, $key4 ], 300 ); + $this->assertFalse( $ok, "One key missing" ); + + $this->assertEquals( 2, $this->cache->incr( $key1 ) ); + $this->assertEquals( 3, $this->cache->incr( $key2 ) ); + $this->assertEquals( 4, $this->cache->incr( $key3 ) ); + + // cleanup + $this->cache->delete( $key1 ); + $this->cache->delete( $key2 ); + $this->cache->delete( $key3 ); + $this->cache->delete( $key4 ); + } + /** * @covers BagOStuff::add */ @@ -304,6 +354,7 @@ class BagOStuffTest extends MediaWikiTestCase { $this->cache->set( $key, 666, 10, BagOStuff::WRITE_ALLOW_SEGMENTS ); + $this->assertEquals( 666, $this->cache->get( $key ) ); $this->assertEquals( 667, $this->cache->incr( $key ) ); $this->assertEquals( 667, $this->cache->get( $key ) ); -- 2.20.1