diff --git a/lib/private/Files/Cache/Propagator.php b/lib/private/Files/Cache/Propagator.php index fdd926d89ac..1a6464d274c 100644 --- a/lib/private/Files/Cache/Propagator.php +++ b/lib/private/Files/Cache/Propagator.php @@ -9,12 +9,8 @@ declare(strict_types=1); */ namespace OC\Files\Cache; -use OC\DB\Exceptions\DbalException; -use OC\Files\Storage\Wrapper\Encryption; -use OCP\DB\QueryBuilder\ILiteral; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Cache\IPropagator; -use OCP\Files\Storage\IReliableEtagStorage; use OCP\Files\Storage\IStorage; use OCP\IDBConnection; use OCP\Server; @@ -23,9 +19,6 @@ use Psr\Clock\ClockInterface; use Psr\Log\LoggerInterface; class Propagator implements IPropagator { - public const MAX_RETRIES = 3; - - private bool $inBatch = false; private array $batch = []; private ClockInterface $clock; @@ -37,6 +30,10 @@ class Propagator implements IPropagator { $this->clock = Server::get(ClockInterface::class); } + public function __destruct() { + $this->commitBatch(); + } + #[Override] public function propagateChange(string $internalPath, int $time, int $sizeDifference = 0): void { // Do not propagate changes in ignored paths @@ -48,91 +45,10 @@ class Propagator implements IPropagator { $time = min($time, $this->clock->now()->getTimestamp()); - $storageId = $this->storage->getCache()->getNumericStorageId(); - $parents = $this->getParents($internalPath); - if ($this->inBatch) { - foreach ($parents as $parent) { - $this->addToBatch($parent, $time, $sizeDifference); - } - return; - } - - $parentHashes = array_map('md5', $parents); - sort($parentHashes); // Ensure rows are always locked in the same order - $etag = uniqid(); // since we give all folders the same etag we don't ask the storage for the etag - - $builder = $this->connection->getQueryBuilder(); - $hashParams = array_map(static fn (string $hash): ILiteral => $builder->expr()->literal($hash), $parentHashes); - - $builder->update('filecache') - ->set('mtime', $builder->func()->greatest('mtime', $builder->createNamedParameter($time, IQueryBuilder::PARAM_INT))) - ->where($builder->expr()->eq('storage', $builder->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) - ->andWhere($builder->expr()->in('path_hash', $hashParams)); - if (!$this->storage->instanceOfStorage(IReliableEtagStorage::class)) { - $builder->set('etag', $builder->createNamedParameter($etag, IQueryBuilder::PARAM_STR)); - } - - if ($sizeDifference !== 0) { - $hasCalculatedSize = $builder->expr()->gt('size', $builder->expr()->literal(-1, IQUeryBuilder::PARAM_INT)); - $sizeColumn = $builder->getColumnName('size'); - $newSize = $builder->func()->greatest( - $builder->func()->add('size', $builder->createNamedParameter($sizeDifference)), - $builder->createNamedParameter(-1, IQueryBuilder::PARAM_INT) - ); - - // Only update if row had a previously calculated size - $builder->set('size', $builder->createFunction("CASE WHEN $hasCalculatedSize THEN $newSize ELSE $sizeColumn END")); - - if ($this->storage->instanceOfStorage(Encryption::class)) { - // in case of encryption being enabled after some files are already uploaded, some entries will have an unencrypted_size of 0 and a non-zero size - $hasUnencryptedSize = $builder->expr()->neq('unencrypted_size', $builder->expr()->literal(0, IQueryBuilder::PARAM_INT)); - $sizeColumn = $builder->getColumnName('size'); - $unencryptedSizeColumn = $builder->getColumnName('unencrypted_size'); - $newUnencryptedSize = $builder->func()->greatest( - $builder->func()->add( - $builder->createFunction("CASE WHEN $hasUnencryptedSize THEN $unencryptedSizeColumn ELSE $sizeColumn END"), - $builder->createNamedParameter($sizeDifference) - ), - $builder->createNamedParameter(-1, IQueryBuilder::PARAM_INT) - ); - - // Only update if row had a previously calculated size - $builder->set('unencrypted_size', $builder->createFunction("CASE WHEN $hasCalculatedSize THEN $newUnencryptedSize ELSE $unencryptedSizeColumn END")); - } - } - - for ($i = 0; $i < self::MAX_RETRIES; $i++) { - try { - if ($this->connection->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { - $this->connection->beginTransaction(); - // Lock all the rows first with a SELECT FOR UPDATE ordered by path_hash - $forUpdate = $this->connection->getQueryBuilder(); - $forUpdate->select('fileid') - ->from('filecache') - ->where($forUpdate->expr()->eq('storage', $forUpdate->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))) - ->andWhere($forUpdate->expr()->in('path_hash', $hashParams)) - ->orderBy('path_hash') - ->forUpdate() - ->executeQuery(); - $builder->executeStatement(); - $this->connection->commit(); - } else { - $builder->executeStatement(); - } - break; - } catch (DbalException $e) { - if ($this->connection->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { - $this->connection->rollBack(); - } - if (!$e->isRetryable()) { - throw $e; - } - - $loggerInterface = Server::get(LoggerInterface::class); - $loggerInterface->warning('Retrying propagation query after retryable exception.', [ 'exception' => $e ]); - } + foreach ($parents as $parent) { + $this->addToBatch($parent, $time, $sizeDifference); } } @@ -152,7 +68,7 @@ class Propagator implements IPropagator { #[Override] public function beginBatch(): void { - $this->inBatch = true; + Server::get(LoggerInterface::class)->debug('Propagated changes are committed in batches automatically, so it is no longer necessary to begin a batch manually.'); } private function addToBatch(string $internalPath, int $time, int $sizeDifference): void { @@ -172,11 +88,6 @@ class Propagator implements IPropagator { #[Override] public function commitBatch(): void { - if (!$this->inBatch) { - throw new \BadMethodCallException('Not in batch'); - } - $this->inBatch = false; - // Ensure rows are always locked in the same order uasort($this->batch, static fn (array $a, array $b) => $a['hash'] <=> $b['hash']); diff --git a/lib/private/Files/Utils/Scanner.php b/lib/private/Files/Utils/Scanner.php index d93b8370d7a..5e531c9c023 100644 --- a/lib/private/Files/Utils/Scanner.php +++ b/lib/private/Files/Utils/Scanner.php @@ -147,7 +147,6 @@ class Scanner extends PublicEmitter { }); $propagator = $storage->getPropagator(); - $propagator->beginBatch(); $scanner->backgroundScan(); $propagator->commitBatch(); } catch (\Exception $e) { @@ -243,7 +242,6 @@ class Scanner extends PublicEmitter { } try { $propagator = $storage->getPropagator(); - $propagator->beginBatch(); try { $scanner->scan($relativePath, $recursive, \OC\Files\Cache\Scanner::REUSE_ETAG | \OC\Files\Cache\Scanner::REUSE_SIZE); } catch (LockedException $e) { @@ -275,16 +273,10 @@ class Scanner extends PublicEmitter { private function postProcessEntry(IStorage $storage, $internalPath) { $this->triggerPropagator($storage, $internalPath); - if ($this->useTransaction) { - $this->entriesToCommit++; - if ($this->entriesToCommit >= self::MAX_ENTRIES_TO_COMMIT) { - $propagator = $storage->getPropagator(); - $this->entriesToCommit = 0; - $this->db->commit(); - $propagator->commitBatch(); - $this->db->beginTransaction(); - $propagator->beginBatch(); - } + $this->entriesToCommit++; + if ($this->entriesToCommit >= self::MAX_ENTRIES_TO_COMMIT) { + $this->entriesToCommit = 0; + $storage->getPropagator()->commitBatch(); } } } diff --git a/lib/public/Files/Cache/IPropagator.php b/lib/public/Files/Cache/IPropagator.php index 620da29057c..12203262637 100644 --- a/lib/public/Files/Cache/IPropagator.php +++ b/lib/public/Files/Cache/IPropagator.php @@ -28,6 +28,7 @@ interface IPropagator { * before the batch is committed. * * @since 9.1.0 + * @depecated 34.0.0 Propagated changes are committed in batches automatically, so it is no longer necessary to begin a batch manually. */ public function beginBatch(): void; diff --git a/tests/lib/Files/Cache/PropagatorTest.php b/tests/lib/Files/Cache/PropagatorTest.php index 903915029d1..5355031e86a 100644 --- a/tests/lib/Files/Cache/PropagatorTest.php +++ b/tests/lib/Files/Cache/PropagatorTest.php @@ -40,6 +40,7 @@ class PropagatorTest extends TestCase { $paths = ['', 'foo', 'foo/bar']; $oldInfos = $this->getFileInfos($paths); $this->storage->getPropagator()->propagateChange('foo/bar/file.txt', time()); + $this->storage->getPropagator()->commitBatch(); $newInfos = $this->getFileInfos($paths); foreach ($oldInfos as $i => $oldInfo) { @@ -58,6 +59,7 @@ class PropagatorTest extends TestCase { $cache->put('foo/bar', ['mtime' => $oldTime]); $cache->put('foo/bar/file.txt', ['mtime' => $oldTime]); $this->storage->getPropagator()->propagateChange('foo/bar/file.txt', $targetTime); + $this->storage->getPropagator()->commitBatch(); $newInfos = $this->getFileInfos($paths); $this->assertEquals($targetTime, $newInfos['foo/bar']->getMTime()); @@ -71,6 +73,7 @@ class PropagatorTest extends TestCase { $paths = ['', 'foo', 'foo/bar']; $oldInfos = $this->getFileInfos($paths); $this->storage->getPropagator()->propagateChange('foo/bar/file.txt', time(), 10); + $this->storage->getPropagator()->commitBatch(); $newInfos = $this->getFileInfos($paths); foreach ($oldInfos as $i => $oldInfo) { @@ -82,10 +85,11 @@ class PropagatorTest extends TestCase { $paths = ['', 'foo', 'foo/bar']; $oldInfos = $this->getFileInfos($paths); $this->storage->getPropagator()->propagateChange('foo/bar/file.txt', time(), -100); + $this->storage->getPropagator()->commitBatch(); $newInfos = $this->getFileInfos($paths); foreach ($oldInfos as $i => $oldInfo) { - $this->assertEquals(-1, $newInfos[$i]->getSize()); + $this->assertEquals(-97, $newInfos[$i]->getSize()); } } @@ -101,7 +105,6 @@ class PropagatorTest extends TestCase { $oldInfos = $this->getFileInfos($paths); $propagator = $this->storage->getPropagator(); - $propagator->beginBatch(); $propagator->propagateChange('asd/file.txt', time(), 10); $propagator->propagateChange('foo/bar/file.txt', time(), 2);