Conversation
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
filimonov
left a comment
There was a problem hiding this comment.
Two things I would change before merge, one is shape and one is a bug.
Shape: the generic API should not learn about envelopes. The PR touches 23 files outside ContentAddressed/ (IDisk, IMetadataStorage, IObjectStorage, S3ObjectStorage, copyS3File, four disk wrappers, ObjectStorageQueue). That is a rebase cost on every release, and the new offset is a defaulted argument that future upstream callers of copyObjectToAnotherObjectStorage / getBlobPath will not know about, so blobs they copy will carry the envelope silently once inline files stop failing first.
The root cause is that DataSourceDescription::operator== and sameKind compare only type, object storage type and endpoint, so a CAS disk looks identical to a plain s3 disk on the same endpoint. Fixing it there keeps everything inside code we already patch:
- In the
DiskObjectStorageconstructor (DiskObjectStorage.cpp:124), mark the description for a content-addressed metadata storage (suffix ondescription, CAS-gated). ThenDiskObjectStorage::copyFilefalls intoIDisk::copyThroughBuffers, and all four backup writers failsameKindand copy through buffers. Correct bytes, no generic diff. This also covers MOVE out of CAS (CAS-254). copyS3File::performCopyhas a real upstream bug:src_offsetis ignored when the single-operationCopyObjectis chosen. A 3-line fix (never single-op whenoffset != 0) is independent of CAS and can go upstream on its own.- If we want server-side copy for
S3(...)destinations, one CAS-gated branch inBackupWriterS3::copyFileFromDiskcan callcopyS3Filewith the payload offset as the existingsrc_offsetand a rawreadObjectfallback reader. Nosrc_object_offset, nogetObjectPayloadOffset, no wrapper changes.
Bug: zero-byte blob files. A wide part with an all-empty Array or all-NULL Nullable column has 0-byte arr.bin / n.bin (reproduced with the PR binary). partFileMustStayBlob keeps them blobs, every blob copy is ranged, and calculatePartSize(0) throws LOGICAL_ERROR. BACKUP skips empty files, IDisk::copyFile does not. Needs a size == 0 branch and a test.
Smaller items:
getObjectPayloadOffsetis only called on the non-CAS branches and always returns 0 there;readInlineDataToStringon CAS has no caller (the description sayscopyFileImpluses it, it usesgetContentAddressedFileCopySource).- The blob source is validated in the producer and again in both consumers;
src_blob != copy_source.objectandpayload_size != object.bytes_sizecannot be true by construction. - No gtest for the ranged copy, although
S3ObjectStorageConditionalOpsTestingtest_writebuffer_s3.cppalready has the mock and aLocalObjectStoragedestination (the baseIObjectStorageseek path is otherwise untested). - GCS:
supportsMultiPartCopyis false there, so every blob falls back to the buffered path. Correct, but logged only at TRACE and not tested; worth a line inbackend.md. - Cost: every blob is now Create + UploadPartCopy + Complete, even a 100-byte
primary.idx.
A column of all-empty `Array` values produces a zero-size `.bin`. Placement keys on the file name, so `partFileMustStayBlob` keeps it a blob, and a ranged server-side copy of a blob reaches `calculatePartSize(0)`, which throws. `BACKUP` never meets such a file when files are deduplicated, but `IDisk::copyFile` does, so `MOVE PARTITION` out of a CAS disk hits it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`DataSourceDescription::operator==` and `sameKind` compare the storage kind and the endpoint, so a content-addressed disk looks identical to a plain s3 disk on the same endpoint. Every server-side copy path then assumes a file is its whole object starting at byte 0, which is false on a CAS disk: a blob-backed file is a payload window behind a fixed-size envelope, and an inline file has no object of its own. Add `files_are_whole_objects` to `DataSourceDescription` and a predicate `canUseNativeCopyWith` that requires it from BOTH sides. A precondition holds on each side separately, so it is a conjunction rather than a comparison: two disks that both lack the property are not thereby able to use it. `operator==` and `sameKind` keep their meaning and stay reflexive, so callers that ask about disk identity are unaffected. The field is last in the struct because four call sites brace-initialize `DataSourceDescription` positionally. `BackupIO_AzureBlobStorage` does not use `sameKind` - it compares `object_storage_type` directly - so both of its conditions check the capability explicitly. This closes a reachable configuration rather than only future code: a read-only CAS mount over Azure is constructible, because `Pool::open` skips the conditional-write capability probe when the pool is opened read-only, and the Azure native path copies a whole blob with no range. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Konstantin Morozov <just.morozov.k@gmail.com>
Ten tests for the paths the capability gate touches, most of which no test exercised before. Three of them guard non-CAS behaviour, because the gate sits on a path every object-storage disk takes: - an ordinary s3 disk against an `encrypted` disk over the same s3. `sameKind` ignores `is_encrypted` and `DiskEncrypted` reports its delegate's description, so a predicate that replaced `operator==` rather than joining it would let the pair through and the cast to `DiskObjectStorage` would throw. - two ordinary s3 disks keep their server-side copy. The capability defaults to false, so any description that forgets to claim it loses the fast path in silence. - `BACKUP TO File(...)` keeps using `fs::copy`. Both sides take their description from one function, so a forgotten claim there gives false on both sides. The rest cover CAS: a cache disk over CAS stays out of the server-side copy, a zero-size file survives a backup when files are not deduplicated, an incremental backup exercises a non-zero `start_pos`, and a backup written into a CAS disk now succeeds where it used to be refused. The mechanism test now also asserts that something went through buffers, so it cannot pass when a part happens to hold no in-manifest entries. The unit test pins the predicate's truth table and, separately, that `operator==` stayed reflexive - the property that breaks if the conjunction is folded into it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`BACKUP` to an `S3(...)` destination on the pool's own endpoint copies a blob's payload without its header, and only `UploadPartCopy` can name a byte range. A store without it is still supported - the blob goes through the ClickHouse server - but the distinction belongs in the capability table next to the other store requirements, where an operator choosing a backend will look for it. `GCS` is the store this applies to today: its current XML API has no `UploadPartCopy`, so `Client::supportsMultiPartCopy` reports `false`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test_backup_to_s3_with_empty_array_column` and `test_incremental_backup_of_a_cas_table` repeat what `test_backup_to_s3_with_empty_arrays` and `test_incremental_backup_to_s3` already do, and the existing pair is stronger: the empty-file one asserts that a zero-size file actually reached the backup writer, rather than only checking the data after a restore. Two more cluster round-trips for no new coverage is time every CI run pays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three fixes from the whole-branch review. `BackupIO_AzureBlobStorage` went back to checking the capability flags directly instead of `canUseNativeCopyWith`. The predicate carries `sameKind`, which compares the description, and the two Azure sides build that string differently: a disk reports `Endpoint::getServiceEndpoint`, while the backup reports `ConnectionParams::getConnectionURL`, which for a `connection_string` disk parses that string and returns the service URL instead. Requiring them equal would have taken Azure's native copy away from a working configuration with no error and no log line. The capability is the only dimension this work argued about, so it is the only one the Azure conditions gained. `ReadOnlyDiskWrapper` now forwards `getS3StorageClient` and `tryGetS3StorageClient`. It already forwards `isContentAddressed` and `getMetadataStorage`, so a CAS disk behind the wrapper reaches the new backup branch, which then asks the disk for its client and got `NOT_IMPLEMENTED` from the base class - an exception that escapes the copy rather than falling back to buffers. `test_backup_to_file_keeps_fs_copy` asserted that the backup read zero bytes through a file descriptor. It cannot: `BackupFileInfo` checksums every entry without a precalculated hash, and `checksums.txt`, `columns.txt`, `count.txt` and the codec and version files have none. The assertion now compares what was read against the part's size on disk, which is what "the data did not go through the server" actually means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`BACKUP TABLE t TO Disk('<cas disk>', ...)` does not work, and the test that
claimed it does was wrong. A backup's own layout mirrors the table's data
directory - `.../data/<db>/<table>/all_1_1_0/<file>` - and
`Cas::isPartFilePath` looks for a part-directory component anywhere in the
path, so a backup file counts as a part file and the autocommit refusal
applies. The test now asserts the refusal, and the documented limitation says a
`CAS` disk cannot be a backup destination rather than claiming the opposite.
`test_backup_to_file_keeps_fs_copy` had its bound the wrong way round. Every
entry pays one checksum pass through `ReadBufferFromFileDescriptor`, so one
pass over the data is what a working `fs::copy` looks like, and the run that
failed - 6832069 bytes read against 6830303 on disk - was the fast path doing
its job. A second pass is what a lost `fs::copy` would cost, so the assertion
is now against one and a half passes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`test_backup_to_s3_with_empty_arrays` counted objects in the backup with `_size = 0` through the `s3` table function, which cannot report them: `s3_skip_empty_files` defaults to true, and the listing drops a zero-size object before the `WHERE` ever sees it. The assertion would have failed even with the file sitting in the backup. The query now turns that setting off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BACKUPof a table on a CAS to anS3destination that shares the pool's endpoint failed outright, and the code path that failed would have silently corrupted the backup had it not failed.BackupWriterS3::copyFileFromDiskasksS3to copy the source object server-side. That assumes the file is that object, whole, from byte 0. On a CAS disk neither half holds:A small per-part file (
checksums.txt,count.txt,columns.txt, ...) is an inline manifest entry with no object of its own.getStorageObjectsreturns a sized placeholder with an empty remote key, deliberately poisoned so a reader that bypasses the read path fails loudly rather than reading someone else's bytes. The copy passed that empty key toCopyObject, so everyBACKUPdied withInvalid argument- these files exist in every part.A blob object is
[envelope][payload]and the payload is the file.BlobLocationcarries the payload offset, butgetStorageObjectsreturns only key and length andgetBlobPathkeeps only the key, so the copy started at byte 0 and pulled the envelope into the backup.RESTOREthen failed.The first failure masked the second:
checksums.txtaborted the backup before any blob corruptioncould surface. Fixing only the empty key would have turned a loud failure into backups that report
success and cannot be restored.
The same bug was on the
BACKUP ... TO Disk(...)path (ans3/s3_plaindisk on the pool's endpoint):DiskObjectStorage::copyFile->copyFileImpl->S3ObjectStorage::copyObjectToAnotherObjectStorage.copyFileImplnow reads the bytes withreadInlineDataToString(implemented forCAS) and writes them to the destination.S3ObjectStoragethrowsLOGICAL_ERRORon an empty key.copyFileImplgets the offset from the newIMetadataStorage::getObjectPayloadOffsetand passes it asobject_from_offset.S3ObjectStoragepasses it tocopyS3Fileassrc_offset, so both the ranged copy and thefallback read only the payload.
RESTOREontoCASis not affected: it writes each part through oneCAStransaction.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed
BACKUPof a table on a CAS to anS3andDiskdestination on the same endpoint.Documentation entry for user-facing changes
Adds
docs/en/antalya/cas/operations/backup.mddescribing howBACKUP/RESTOREbehave on a content-addressed disk, and links it from the CAS index and roadmap.CI/CD Options
Exclude tests:
Regression jobs to run: