Repository navigation
Keep mapped object disk restores from sharing keys with the source table - #1570
Merged
Merged
Conversation
A table restored next to its source with --restore-database-mapping on the same S3 disk lost the source data on DROP DATABASE ... SYNC of the copy (Code: 499 The specified key does not exist). Three defects in downloadObjectDiskParts and its callers combined into that: The idempotency guard strings.Contains(objectPath, backupName) looked at the whole key, so a disk endpoint prefix which contains the backup name (backup `regression` on `.../regression-test/live/`) silently disabled the key rewrite and the copy referenced the source objects. HardlinkBackupPartsToStorage ran before the objects were copied and the keys rewritten, so an interrupted restore left `detached` (or the table directory with restore_as_attach) pointing at the source keys. The shadow metadata files are hardlinked into the restored table and WriteMetadataToFile wrote them in place, so a second mapped restore of the same local backup rewrote the parts of the first copy, and since the suffix carried only the backup name both copies shared their keys. splitRestoreObjectKeys now derives the source key from the backup key (a suffix left by a previous restore is stripped), the destination key carries `_<backup>_<crc32(db.table)>`, objects are copied before the parts are linked into the table and metadata files are written via a temporary file plus rename. TestRestoreMappingObjectDiskDropDst covers the plain, cache-wrapped tiered, prefix-contains-backup-name, interrupted and restore-twice cases; all but the first two fail on master. Verified on 26.8: TestRestoreMappingObjectDiskDropDst, TestRestoreMapping, TestCacheDiskBackup, TestS3. fix #1568 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CI of the previous commit failed in two ways. On ClickHouse 22.8 the object keys of an S3 disk are a bare random name without the `xxx/` directory component, and splitRestoreObjectKeys (like the code before it) skipped such keys, so the mapped copy still shared its objects with the source and TestRestoreMappingObjectDiskDropDst failed with the very error it guards against. The suffix now goes onto the only component when there is no directory. On 26.8 every embedded backup restore failed with `Cannot open file .../metadata/<db>/<table>.sql ... Permission denied` and its remote leftovers cascaded into the other tests of the same pooled environment. WriteMetadataToFile now creates the file via CreateTemp, which as root produces a root owned file, while the in-place os.Create kept the clickhouse owner of the existing file and its 0640 mode made it unreadable for the server. The owner of the replaced file is now copied onto the new one together with its mode. Verified on 22.8 (TestRestoreMappingObjectDiskDropDst) and 26.8 (TestRestoreMappingObjectDiskDropDst, TestEmbeddedS3, TestBwLimitEmbeddedS3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Report for CI Build 35323856779Coverage increased (+0.06%) to 67.514%Details
Uncovered Changes
Coverage Regressions22 previously-covered lines in 5 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1568
Problem
restore --restore-database-mapping=source:completedof a table on an S3 disk next to the still existing source, followed byDROP DATABASE completed SYNC, made the source table fail withCode: 499 ... The specified key does not exist (S3_ERROR). The same happened when an interrupted mapped restore was dropped.Root causes
strings.Contains(objectPath, backupName)examined the whole key, so a disk endpoint prefix containing the backup name (backupregression, disk.../regression-test/live/) silently disabled the rewrite and the copy referenced the source objects. This matches the completed-restore case of the report.HardlinkBackupPartsToStorageran before the objects were copied and the keys rewritten, so an interrupted restore leftdetached(or the table directory withrestore_as_attach) pointing at the source keys. This matches the SIGINT case of the report.WriteMetadataToFilewrote them in place, so a second mapped restore of the same local backup rewrote the parts of the first copy; the suffix carried only the backup name, so two copies of one backup shared their keys.Fix
splitRestoreObjectKeys: source key always derived from the backup key (a suffix left by a previous restore is stripped), destination key gets_<backup>_<crc32(db.table)>.WriteMetadataToFilewrites via temporary file and rename, so hardlinked parts of an already restored table are never modified.Tests
TestSplitRestoreObjectKeysunit test.TestRestoreMappingObjectDiskDropDst: plains3_only, cache-wrapped tiered policy withMOVE PARTITION(as in the report), backup name contained in the disk key prefix, interrupted restore (object copy fails after schema is created, emulated with a wrongS3_OBJECT_DISK_PATH), restore of the same local backup twice. The last three fail on master with the exact error from the issue.TestRestoreMappingObjectDiskDropDst,TestRestoreMapping,TestCacheDiskBackup,TestS3.🤖 Generated with Claude Code