New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backup/Restore for KeeperMap tables #56460
Conversation
This is an automated comment for commit 9bcedf3 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
6443211
to
9e91e4d
Compare
src/Backups/BackupImpl.cpp
Outdated
@@ -452,13 +452,15 @@ void BackupImpl::readBackupMetadata() | |||
size_of_entries = 0; | |||
|
|||
const auto * contents = config_root->getNodeByPath("contents"); | |||
std::vector<std::pair<String /*source*/, String /*target*/>> reference_files; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not necessary anymore.
src/Backups/BackupImpl.cpp
Outdated
if (!deduplicate_files) | ||
{ | ||
for (const auto & reference : info.reference_sources) | ||
writer->copyFile(reference, info.data_file_name, info.size - info.base_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. And it can be faster too because instead of copying the same file again to another destination you just copy it inside a backup.
target_info->reference_sources.push_back(reference.file_name); | ||
reference.size = target_info->size; | ||
total_size_of_files += reference.size; | ||
reference.checksum = target_info->checksum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encrypted_by_disk
should be copied too:
reference.encrypted_by_disk = target_info->encrypted_by_disk;
if (info.reference_target.empty()) | ||
{ | ||
file_name_to_info.emplace(info.file_name, &info); | ||
total_size_of_files += info.size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use this copy-inside-backup
approach for other copies too if we insert here something like:
SizeAndChecksum size_and_checksum{info.size, info.checksum};
auto [it, inserted] = data_file_index_by_checksum.emplace(size_and_checksum, i);
if (inserted)
{
info.data_file_name = info.file_name;
info.data_file_index = i;
}
else
{
/// specify that info must not be written in the usual way ...
info.data_file_name.clear();
info.data_file_index = -1;
/// ... because target_info will be copied to its place instead
auto & target_info = it->second;
target_info->reference_sources.push_back(info.file_name);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we are okay with using size and checksum in plain backups as unique id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A plain backup means every file is stored in its place: there can files of the same contents; empty files are stored too; and references to the base backup are not used. So a plain backup is about how it must look on Disk or S3 when it's completed. It's not about how we should write it - we can write it any way we like if the result is the same. I mean there is no difference in the result whether we
copy SRC TO DEST[0], DEST[1], ... DEST[N]
or
copy SRC TO DEST[0], then
copy DEST[0] to DEST[1], ... DEST[N]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but there is a difference if we use size and checksum
if files from different backup entries match in size and checksum this will produce a different result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if files from different backup entries match in size and checksum this will produce a different result
I didn't quite understand. Do you mean that two files of the same size but different by contents can have the same checksum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean chances are almost nonexistent for a collision to happen, especially with 128bits we use for checksum but just asking if we are okay with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formula is (https://en.wikipedia.org/wiki/Birthday_attack)
p(n, H) = 1-exp(-n^2 / 2 / H)
So if we used 128-bit hashes without size, for a backup with 1000000 files we would have the probability like 10^-27
that two hashes match. With sizes the probability must be even smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though that optimization is not very crucial anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me do this in a different PR so we are not blocking backups of KeeperMap and then we can discuss if we are okay with the solution.
LOG_TRACE(log, "Copying file inside backup from {} to {} ", source, destination); | ||
copyS3File( | ||
client, | ||
client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broke my local build:
/home/ubuntu/repo/ClickHouse/src/Backups/BackupIO_S3.cpp:255:5: error: no matching function for call to 'copyS3File'
255 | copyS3File(
| ^~~~~~~~~~
/home/ubuntu/repo/ClickHouse/src/IO/S3/copyS3File.h:31:6: note: candidate function not viable: no known conversion from 'std::shared_ptr<S3::Client>' to 'const String' (aka 'const basic_string<char, char_traits<char>, allocator<char>>') for 2nd argument
33 | void copyS3File(
| ^
34 | const std::shared_ptr<const S3::Client> & s3_client,
35 | const String & src_bucket,
| ~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[10301/12493] Building CXX object src/CMakeFiles/dbms.dir/Functions/CastOverloadResolver.cpp.o
ninja: build stopped: subcommand failed.
Fix: #56974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's this PR #56314 which modifies copyS3File
It didn't have the latest master so it didn't catch the new code that calls copyS3File
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add support for backing up and restoring tables using KeeperMap engine.
cc @vitlibar there are tests missing and some polishing is left but created a draft PR so you can take a look while I'm still working on it
Documentation entry for user-facing changes