diff --git a/CHANGELOG.md b/CHANGELOG.md index 34f51d0..cd58819 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ the app store accepts a `-beta` suffix, but Nextcloud servers then only offer th beta/daily update channel, which would hide the app from most admins. The stability stage is instead carried by the app description, the git tag, the release, and this file. +## [0.9.3-beta.4] - 2026-10-08 + +Bugfix release. + +### Fixed +- A backup failed with "cannot execute INSERT in a read-only transaction" on PostgreSQL as soon + as the database dump filled a whole pack (32 MiB): the pack's index cache row was written + inside the read-only dump transaction. On other databases that row was rolled back silently. +- Backups to a remote location slowed down as the repository grew: every backup step listed the + repository's index with a round trip per file. Over SFTP to a Hetzner Storage Box a 60 GiB + repository already lost a large part of each step to it; backing up continuously + (`occ backup:run`) went from 45 KiB/s to 5.8 MB/s with the fix. + ## [0.9.2-beta.3] - 2026-10-08 Bugfix release. @@ -122,6 +135,7 @@ and not implemented yet. - See the "Storage backend experiences" section of the README for provider-specific quirks found so far (e.g. Hetzner Storage Box sub-accounts). +[0.9.3-beta.4]: https://github.com/dillardblom/ng_backup/releases/tag/v0.9.3-beta.4 [0.9.2-beta.3]: https://github.com/dillardblom/ng_backup/releases/tag/v0.9.2-beta.3 [0.9.1-beta.2]: https://github.com/dillardblom/ng_backup/releases/tag/v0.9.1-beta.2 [0.9.0-beta.1]: https://github.com/dillardblom/ng_backup/releases/tag/v0.9.0-beta.1 diff --git a/README.md b/README.md index d7691f3..7aa4343 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ installations and hosted Nextcloud. (Choosing SMB as a storage backend is the on see "Storage backend experiences" below — that requirement comes from files_external, not from ng_backup, and applies to any Nextcloud app that writes to an SMB share.) -**Status: beta (0.9.2).** Tested thoroughly, usable alongside your current backup, but not yet a +**Status: beta (0.9.3).** Tested thoroughly, usable alongside your current backup, but not yet a drop-in replacement for it. See "Known limitations" below, `PLAN.md` for the design and `docs/HOWTO.md` for a step-by-step guide from setup to a tested restore. diff --git a/appinfo/info.xml b/appinfo/info.xml index 14e41ff..3f4f45e 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -9,7 +9,7 @@ directory to S3, SFTP, WebDAV, FTP, SMB or a local path, with restore of files a single users (via user_migration) and of the whole server onto a fresh installation. Tested thoroughly and usable alongside your current backup, but not yet a drop-in replacement for it. See the README for known limitations and the recovery kit before relying on it.]]> - 0.9.2 + 0.9.3 agpl Dillard Blom NgBackup diff --git a/lib/Backend/IBackend.php b/lib/Backend/IBackend.php index 131b293..2d7eb6d 100644 --- a/lib/Backend/IBackend.php +++ b/lib/Backend/IBackend.php @@ -37,5 +37,15 @@ interface IBackend { /** @return list object paths below $prefix (recursive) */ public function list(string $prefix): array; + /** + * Object paths directly in $dir, for a folder that only holds objects (index/, snapshots/, + * catalog/, trash/info/). One listing, without checking the type of every entry: on a remote + * storage list() costs a round trip per entry, which for index/ (one file per pack) grows + * with the size of the repository. + * + * @return list + */ + public function listFiles(string $dir): array; + public function delete(string $path): void; } diff --git a/lib/Backend/LocalBackend.php b/lib/Backend/LocalBackend.php index 1805768..1a577bb 100644 --- a/lib/Backend/LocalBackend.php +++ b/lib/Backend/LocalBackend.php @@ -107,6 +107,25 @@ final class LocalBackend implements IBackend { return $result; } + public function listFiles(string $dir): array { + $base = $this->abs($dir); + if (!is_dir($base)) { + return []; + } + $result = []; + $names = scandir($base); + if ($names === false) { + throw new BackendException('Cannot list ' . $dir); + } + foreach ($names as $name) { + if ($name !== '.' && $name !== '..' && !StorageBackend::isTemporary($name)) { + $result[] = trim($dir, '/') . '/' . $name; + } + } + sort($result); + return $result; + } + public function delete(string $path): void { @unlink($this->abs($path)); } diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index aff055f..58f53e2 100644 --- a/lib/Backend/StorageBackend.php +++ b/lib/Backend/StorageBackend.php @@ -133,6 +133,28 @@ final class StorageBackend implements IBackend { } } + public function listFiles(string $dir): array { + $abs = $this->abs($dir); + if (!$this->storage->is_dir($abs)) { + return []; // not created yet + } + $dh = $this->storage->opendir($abs); + if ($dh === false) { + // An existing folder read as empty would look like a new repository. + throw new BackendException('Cannot list ' . $dir); + } + $cut = $this->base === '' ? 0 : strlen($this->base) + 1; + $result = []; + while (($name = readdir($dh)) !== false) { + if ($name !== '.' && $name !== '..' && !self::isTemporary($name)) { + $result[] = substr($abs . '/' . $name, $cut); + } + } + closedir($dh); + sort($result); + return $result; + } + private function listInto(string $dir, array &$result): void { if (!$this->storage->is_dir($dir)) { return; diff --git a/lib/Db/DbDumper.php b/lib/Db/DbDumper.php index 2202a4c..1010cdd 100644 --- a/lib/Db/DbDumper.php +++ b/lib/Db/DbDumper.php @@ -72,50 +72,57 @@ final class DbDumper { public function dumpStep(Repository $repo, array $state, float $deadline, array &$stats): array { $state['steps']++; $sm = $this->db->createSchemaManager(); - $this->db->setTransactionIsolation(TransactionIsolationLevel::REPEATABLE_READ); - $this->db->beginTransaction(); + // A pack that fills up during the dump is uploaded right away; its index cache row must + // wait until the read-only transaction is over. + $repo->holdIndexCache(); try { - if ($this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { - $this->db->executeStatement('SET TRANSACTION READ ONLY'); - } - while ($state['i'] < count($state['tables'])) { - $table = $state['tables'][$state['i']]; - $schema = $sm->introspectTable($table); - $writer = $repo->blobWriter($stats); - $hash = $state['hash'] !== null ? unserialize(base64_decode($state['hash']), ['allowed_classes' => [\HashContext::class]]) : hash_init('sha256'); - if ($state['after'] === null && $state['segments'] === []) { - $writer->write($this->header($schema) . "\n"); + $this->db->setTransactionIsolation(TransactionIsolationLevel::REPEATABLE_READ); + $this->db->beginTransaction(); + try { + if ($this->db->getDatabasePlatform() instanceof \Doctrine\DBAL\Platforms\PostgreSQLPlatform) { + $this->db->executeStatement('SET TRANSACTION READ ONLY'); } - $complete = true; - foreach ($this->rowLines($schema, $state['after']) as [$key, $line]) { - $writer->write($line . "\n"); - hash_update($hash, $line . "\n"); - $state['rows']++; - $state['after'] = $key; - if (microtime(true) >= $deadline) { - $complete = false; + while ($state['i'] < count($state['tables'])) { + $table = $state['tables'][$state['i']]; + $schema = $sm->introspectTable($table); + $writer = $repo->blobWriter($stats); + $hash = $state['hash'] !== null ? unserialize(base64_decode($state['hash']), ['allowed_classes' => [\HashContext::class]]) : hash_init('sha256'); + if ($state['after'] === null && $state['segments'] === []) { + $writer->write($this->header($schema) . "\n"); + } + $complete = true; + foreach ($this->rowLines($schema, $state['after']) as [$key, $line]) { + $writer->write($line . "\n"); + hash_update($hash, $line . "\n"); + $state['rows']++; + $state['after'] = $key; + if (microtime(true) >= $deadline) { + $complete = false; + break; + } + } + $state['segments'][] = $writer->finish(); + $state['bytes'] += $writer->bytes(); + if (!$complete) { + $state['hash'] = base64_encode(serialize($hash)); + break; + } + $state['result'][$table] = ['blobs' => array_merge(...$state['segments']), 'rows' => $state['rows'], + 'bytes' => $state['bytes'], 'sha256' => hash_final($hash)]; + $state = array_merge($state, ['i' => $state['i'] + 1, 'after' => null, 'segments' => [], 'rows' => 0, 'bytes' => 0, 'hash' => null]); + if (microtime(true) >= $deadline && $state['i'] < count($state['tables'])) { break; } } - $state['segments'][] = $writer->finish(); - $state['bytes'] += $writer->bytes(); - if (!$complete) { - $state['hash'] = base64_encode(serialize($hash)); - break; + if ($state['i'] >= count($state['tables'])) { + $state['sequences'] = $this->sequenceStates($state['tables']); + $state['done'] = true; } - $state['result'][$table] = ['blobs' => array_merge(...$state['segments']), 'rows' => $state['rows'], - 'bytes' => $state['bytes'], 'sha256' => hash_final($hash)]; - $state = array_merge($state, ['i' => $state['i'] + 1, 'after' => null, 'segments' => [], 'rows' => 0, 'bytes' => 0, 'hash' => null]); - if (microtime(true) >= $deadline && $state['i'] < count($state['tables'])) { - break; - } - } - if ($state['i'] >= count($state['tables'])) { - $state['sequences'] = $this->sequenceStates($state['tables']); - $state['done'] = true; + } finally { + $this->db->rollBack(); } } finally { - $this->db->rollBack(); + $repo->releaseIndexCache(); } $repo->flushPacks(); return $state; diff --git a/lib/Repository/BlobIndex.php b/lib/Repository/BlobIndex.php index cae45cf..eb7269e 100644 --- a/lib/Repository/BlobIndex.php +++ b/lib/Repository/BlobIndex.php @@ -25,6 +25,8 @@ final class BlobIndex { private array $pending = []; private ?IIndexCache $cache = null; + /** @var list<\Closure():void>|null cache writes held back by holdCache(), or null when writing directly */ + private ?array $heldCacheWrites = null; private string $repositoryId = ''; /** @var int index files downloaded by the last load() (for tests and diagnostics) */ public int $downloaded = 0; @@ -40,6 +42,46 @@ final class BlobIndex { $this->repositoryId = $repositoryId; } + /** + * Hold back cache writes until releaseCache(). The database dump reads inside a read-only + * transaction on the same connection that the cache writes to: a write there fails on + * PostgreSQL and would be rolled back with the dump transaction elsewhere. + */ + public function holdCache(): void { + $this->heldCacheWrites ??= []; + } + + /** Write the cache changes held back since holdCache(). */ + public function releaseCache(): void { + $writes = $this->heldCacheWrites ?? []; + $this->heldCacheWrites = null; + foreach ($writes as $write) { + $write(); + } + } + + /** @param list $entries */ + private function cachePut(string $packId, array $entries): void { + if ($this->heldCacheWrites !== null) { + $this->heldCacheWrites[] = function () use ($packId, $entries): void { + $this->cache?->put($this->repositoryId, $packId, $entries); + }; + return; + } + $this->cache?->put($this->repositoryId, $packId, $entries); + } + + /** @param list $packIds */ + private function cacheRemove(array $packIds): void { + if ($this->heldCacheWrites !== null) { + $this->heldCacheWrites[] = function () use ($packIds): void { + $this->cache?->remove($this->repositoryId, $packIds); + }; + return; + } + $this->cache?->remove($this->repositoryId, $packIds); + } + /** * Load the index: one listing of index/, entries from the local cache where available, * and only the index files that are not cached yet are downloaded. Cached packs that no @@ -50,7 +92,7 @@ final class BlobIndex { $cached = $verify ? [] : ($this->cache?->all($this->repositoryId) ?? []); $present = []; $this->downloaded = 0; - foreach ($this->backend->list('index') as $path) { + foreach ($this->backend->listFiles('index') as $path) { $packId = basename($path); $present[$packId] = true; if (isset($cached[$packId])) { @@ -112,7 +154,7 @@ final class BlobIndex { fwrite($stream, $this->cipher->encryptString(json_encode($entries, JSON_THROW_ON_ERROR), 'index:' . $packId)); rewind($stream); $this->backend->put('index/' . $packId, $stream); - $this->cache?->put($this->repositoryId, $packId, $entries); + $this->cachePut($packId, $entries); foreach ($entries as $e) { $this->blobs[$e['id']] = [$packId, $e['offset'], $e['length'], $e['raw'], $e['flags']]; unset($this->pending[$e['id']]); @@ -131,7 +173,7 @@ final class BlobIndex { /** Forget a pack: delete its index file and cache row. Blobs that now live elsewhere are kept. */ public function removePack(string $packId): void { $this->backend->delete('index/' . $packId); - $this->cache?->remove($this->repositoryId, [$packId]); + $this->cacheRemove([$packId]); foreach ($this->blobs as $id => $loc) { if ($loc[0] === $packId) { unset($this->blobs[$id]); diff --git a/lib/Repository/Catalog.php b/lib/Repository/Catalog.php index b8a3c47..7dae152 100644 --- a/lib/Repository/Catalog.php +++ b/lib/Repository/Catalog.php @@ -128,7 +128,7 @@ final class Catalog { /** @return list */ private function generations(): array { - return array_map(fn ($p) => (int)basename($p), $this->backend->list('catalog')); + return array_map(fn ($p) => (int)basename($p), $this->backend->listFiles('catalog')); } private function path(int $gen): string { diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index 9a383ae..d6c4f1d 100644 --- a/lib/Repository/Repository.php +++ b/lib/Repository/Repository.php @@ -92,8 +92,8 @@ final class Repository { private function loadCatalog(?ICatalogAnchor $anchor): void { $this->catalog = new Catalog($this->backend, $this->cipher, $this->repositoryId, $anchor); $this->catalog->load( - fn () => array_map('basename', $this->backend->list('snapshots')), - fn () => array_map('basename', $this->backend->list('trash/info')), + fn () => array_map('basename', $this->backend->listFiles('snapshots')), + fn () => array_map('basename', $this->backend->listFiles('trash/info')), fn () => $this->backend->list('users')); } @@ -262,6 +262,15 @@ final class Repository { }); } + /** See BlobIndex::holdCache(): no database writes until releaseIndexCache(). */ + public function holdIndexCache(): void { + $this->index->holdCache(); + } + + public function releaseIndexCache(): void { + $this->index->releaseCache(); + } + public function flushPacks(): void { $this->runPacks?->flush(); } @@ -634,7 +643,7 @@ final class Repository { * @return list */ public function listSnapshots(): array { - $present = array_flip(array_map('basename', $this->backend->list('snapshots'))); + $present = array_flip(array_map('basename', $this->backend->listFiles('snapshots'))); $listed = array_keys($this->catalog->snapshots); foreach ($listed as $id) { if (!isset($present[$id])) { diff --git a/tests/Unit/Backend/StorageBackendListTest.php b/tests/Unit/Backend/StorageBackendListTest.php new file mode 100644 index 0000000..da2b93e --- /dev/null +++ b/tests/Unit/Backend/StorageBackendListTest.php @@ -0,0 +1,57 @@ +tempDir(); + foreach (['b1', 'a2', 'c3.part-0123abcd'] as $name) { + touch("$dir/$name"); + } + $isDirCalls = 0; + $storage = $this->createMock(IStorage::class); + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturnCallback(function (string $path) use (&$isDirCalls): bool { + $isDirCalls++; + return $path === 'repo' || $path === 'repo/index'; + }); + $storage->method('opendir')->willReturnCallback(fn (string $path) => $path === 'repo/index' ? opendir($dir) : false); + + $backend = new StorageBackend($storage, 'repo'); + $isDirCalls = 0; // the constructor checks the base folder + $this->assertSame(['index/a2', 'index/b1'], $backend->listFiles('index')); + $this->assertSame(1, $isDirCalls); + + $isDirCalls = 0; + $this->assertSame(['index/a2', 'index/b1'], $backend->list('index')); + $this->assertSame(4, $isDirCalls, 'list() checks every entry: the reason listFiles() exists'); + } + + public function testListFilesOfAMissingFolderIsEmpty(): void { + $storage = $this->createMock(IStorage::class); + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturnCallback(fn (string $path): bool => $path === 'repo'); + $storage->expects($this->never())->method('opendir'); + $this->assertSame([], (new StorageBackend($storage, 'repo'))->listFiles('snapshots')); + } + + public function testAnUnreadableFolderIsAnErrorNotEmpty(): void { + $storage = $this->createMock(IStorage::class); + $storage->method('instanceOfStorage')->willReturn(false); + $storage->method('is_dir')->willReturn(true); + $storage->method('opendir')->willReturn(false); + $this->expectException(BackendException::class); + (new StorageBackend($storage, 'repo'))->listFiles('catalog'); + } +} diff --git a/tests/Unit/Repository/IndexCacheTest.php b/tests/Unit/Repository/IndexCacheTest.php index c2b0c5b..73ecdd3 100644 --- a/tests/Unit/Repository/IndexCacheTest.php +++ b/tests/Unit/Repository/IndexCacheTest.php @@ -51,4 +51,41 @@ class IndexCacheTest extends TestCase { $fourth->restore($r1['snapshot'], $target); $this->assertCount(12, self::hashTree($target)); } + + /** The database dump reads in a read-only transaction: a pack that fills up then must not write the cache yet. */ + public function testHeldCacheWritesWaitForRelease(): void { + $cache = new class implements IIndexCache { + public array $data = []; + public bool $readOnly = false; + public function all(string $r): array { return $this->data[$r] ?? []; } + public function put(string $r, string $p, array $e): void { + if ($this->readOnly) { + throw new \RuntimeException('cannot execute INSERT in a read-only transaction'); + } + $this->data[$r][$p] = $e; + } + public function remove(string $r, array $ps): void { foreach ($ps as $p) { unset($this->data[$r][$p]); } } + }; + $backend = new LocalBackend($this->tempDir()); + $keys = KeyRing::generate(); + Repository::initWithKey($backend, $keys, $keys->wrap('pw')); + $repo = Repository::openWithKey($backend, $keys, $cache); + + $stats = []; + $repo->holdIndexCache(); + $cache->readOnly = true; + $writer = $repo->blobWriter($stats); + for ($i = 0; $i < 40; $i++) { + $writer->write(random_bytes(1048576)); // more than one 32 MiB pack + } + $writer->finish(); + $this->assertGreaterThan(0, count($backend->list('index')), 'a full pack was uploaded during the hold'); + $this->assertSame([], $cache->data); + + $cache->readOnly = false; + $repo->releaseIndexCache(); + $repo->flushPacks(); + $this->assertCount(count($backend->list('index')), array_merge(...array_values($cache->data))); + $this->assertSame(0, Repository::openWithKey($backend, $keys, $cache)->indexDownloads()); + } } diff --git a/tests/Unit/Repository/VerifyTest.php b/tests/Unit/Repository/VerifyTest.php index 117321f..fccdcb7 100644 --- a/tests/Unit/Repository/VerifyTest.php +++ b/tests/Unit/Repository/VerifyTest.php @@ -166,6 +166,10 @@ final class FlakyRangeBackend implements IBackend { return $this->inner->list($prefix); } + public function listFiles(string $dir): array { + return $this->inner->listFiles($dir); + } + public function delete(string $path): void { $this->inner->delete($path); } diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 06fdcb4..63f664b 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -3,3 +3,13 @@ declare(strict_types=1); require __DIR__ . '/../vendor/autoload.php'; + +// nextcloud/ocp ships the public interfaces without an autoloader; tests that mock them need one. +spl_autoload_register(static function (string $class): void { + if (str_starts_with($class, 'OCP\\')) { + $file = __DIR__ . '/../vendor/nextcloud/ocp/' . str_replace('\\', '/', $class) . '.php'; + if (is_file($file)) { + require $file; + } + } +});