Skip to content

Commit 4cf85b2

Browse files
vitlibarvitlibar
authored
Merge pull request ClickHouse#80172 from vitlibar/revert-s3-native-copy-only-if-same-creds
Revert PR 77401 "Use s3 native copy only if source and destination credentials are same"
2 parents 7b78f58 + e8704ef commit 4cf85b2

File tree

12 files changed

+23
-55
lines changed

12 files changed

+23
-55
lines changed

‎src/Backups/BackupFactory.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class BackupFactory : boost::noncopyable
3939
std::shared_ptr<IBackupCoordination> backup_coordination;
4040
std::optional<UUID> backup_uuid;
4141
bool deduplicate_files = true;
42-
std::optional<bool> allow_s3_native_copy = true;
42+
bool allow_s3_native_copy = true;
4343
bool allow_azure_native_copy = true;
4444
bool use_same_s3_credentials_for_base_backup = false;
4545
bool use_same_password_for_base_backup = false;

‎src/Backups/BackupIO_S3.cpp‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ namespace S3AuthSetting
5050

5151
namespace S3RequestSetting
5252
{
53-
extern const S3RequestSettingsBoolAuto allow_native_copy;
53+
extern const S3RequestSettingsBool allow_native_copy;
5454
extern const S3RequestSettingsString storage_class_name;
5555
extern const S3RequestSettingsUInt64 http_max_fields;
5656
extern const S3RequestSettingsUInt64 http_max_field_name_size;
@@ -150,7 +150,7 @@ BackupReaderS3::BackupReaderS3(
150150
const S3::URI & s3_uri_,
151151
const String & access_key_id_,
152152
const String & secret_access_key_,
153-
std::optional<bool> allow_s3_native_copy,
153+
bool allow_s3_native_copy,
154154
const ReadSettings & read_settings_,
155155
const WriteSettings & write_settings_,
156156
const ContextPtr & context_,
@@ -168,9 +168,7 @@ BackupReaderS3::BackupReaderS3(
168168
}
169169

170170
s3_settings.request_settings.updateFromSettings(context_->getSettingsRef(), /* if_changed */true);
171-
172-
if (allow_s3_native_copy)
173-
s3_settings.request_settings[S3RequestSetting::allow_native_copy] = *allow_s3_native_copy;
171+
s3_settings.request_settings[S3RequestSetting::allow_native_copy] = allow_s3_native_copy;
174172

175173
client = makeS3Client(s3_uri_, access_key_id_, secret_access_key_, s3_settings, context_);
176174

@@ -249,7 +247,7 @@ BackupWriterS3::BackupWriterS3(
249247
const S3::URI & s3_uri_,
250248
const String & access_key_id_,
251249
const String & secret_access_key_,
252-
std::optional<bool> allow_s3_native_copy,
250+
bool allow_s3_native_copy,
253251
const String & storage_class_name,
254252
const ReadSettings & read_settings_,
255253
const WriteSettings & write_settings_,
@@ -269,10 +267,7 @@ BackupWriterS3::BackupWriterS3(
269267
}
270268

271269
s3_settings.request_settings.updateFromSettings(context_->getSettingsRef(), /* if_changed */true);
272-
273-
if (allow_s3_native_copy)
274-
s3_settings.request_settings[S3RequestSetting::allow_native_copy] = *allow_s3_native_copy;
275-
270+
s3_settings.request_settings[S3RequestSetting::allow_native_copy] = allow_s3_native_copy;
276271
s3_settings.request_settings[S3RequestSetting::storage_class_name] = storage_class_name;
277272

278273
client = makeS3Client(s3_uri_, access_key_id_, secret_access_key_, s3_settings, context_);

‎src/Backups/BackupIO_S3.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ class BackupReaderS3 : public BackupReaderDefault
2323
const S3::URI & s3_uri_,
2424
const String & access_key_id_,
2525
const String & secret_access_key_,
26-
std::optional<bool> allow_s3_native_copy,
26+
bool allow_s3_native_copy,
2727
const ReadSettings & read_settings_,
2828
const WriteSettings & write_settings_,
2929
const ContextPtr & context_,
@@ -54,7 +54,7 @@ class BackupWriterS3 : public BackupWriterDefault
5454
const S3::URI & s3_uri_,
5555
const String & access_key_id_,
5656
const String & secret_access_key_,
57-
std::optional<bool> allow_s3_native_copy,
57+
bool allow_s3_native_copy,
5858
const String & storage_class_name,
5959
const ReadSettings & read_settings_,
6060
const WriteSettings & write_settings_,

‎src/Backups/BackupSettings.cpp‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ namespace ErrorCodes
2727
M(Bool, async) \
2828
M(Bool, decrypt_files_from_encrypted_disks) \
2929
M(Bool, deduplicate_files) \
30+
M(Bool, allow_s3_native_copy) \
3031
M(Bool, allow_azure_native_copy) \
3132
M(Bool, use_same_s3_credentials_for_base_backup) \
3233
M(Bool, use_same_password_for_base_backup) \
@@ -63,13 +64,7 @@ BackupSettings BackupSettings::fromBackupQuery(const ASTBackupQuery & query)
6364
else
6465

6566
LIST_OF_BACKUP_SETTINGS(GET_BACKUP_SETTINGS_FROM_QUERY)
66-
67-
if (setting.name == "allow_s3_native_copy")
68-
{
69-
SettingFieldBoolAuto bool_auto{setting.value};
70-
res.allow_s3_native_copy = bool_auto.is_auto ? std::nullopt : std::make_optional(bool_auto.base.value);
71-
}
72-
else
67+
/// else
7368
{
7469
/// (if setting.name is not the name of a field of BackupSettings)
7570
res.core_settings.emplace_back(setting);
@@ -111,9 +106,6 @@ void BackupSettings::copySettingsToQuery(ASTBackupQuery & query) const
111106

112107
LIST_OF_BACKUP_SETTINGS(COPY_BACKUP_SETTINGS_TO_QUERY)
113108

114-
if (allow_s3_native_copy)
115-
query_settings->changes.emplace_back("allow_s3_native_copy", static_cast<Field>(SettingFieldBool{*allow_s3_native_copy}));
116-
117109
/// Copy the core settings to the query too.
118110
query_settings->changes.insert(query_settings->changes.end(), core_settings.begin(), core_settings.end());
119111

‎src/Backups/BackupSettings.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,8 @@ struct BackupSettings
4242
/// Whether the BACKUP will omit similar files (within one backup only).
4343
bool deduplicate_files = true;
4444

45-
/// Whether S3 native copy is allowed.
46-
/// If not set, then S3 native copy will be allowed only if the source and destination credentials are the same.
47-
std::optional<bool> allow_s3_native_copy;
45+
/// Whether native copy is allowed (optimization for cloud storages, that sometimes could have bugs)
46+
bool allow_s3_native_copy = true;
4847

4948
/// Whether native copy is allowed (optimization for cloud storages, that sometimes could have bugs)
5049
bool allow_azure_native_copy = true;

‎src/Backups/RestoreSettings.cpp‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ namespace
164164
M(Bool, skip_unresolved_access_dependencies) \
165165
M(Bool, update_access_entities_dependents) \
166166
M(RestoreUDFCreationMode, create_function) \
167+
M(Bool, allow_s3_native_copy) \
167168
M(Bool, use_same_s3_credentials_for_base_backup) \
168169
M(Bool, use_same_password_for_base_backup) \
169170
M(Bool, restore_broken_parts_as_detached) \
@@ -188,14 +189,9 @@ RestoreSettings RestoreSettings::fromRestoreQuery(const ASTBackupQuery & query)
188189
else
189190

190191
LIST_OF_RESTORE_SETTINGS(GET_RESTORE_SETTINGS_FROM_QUERY)
191-
192-
if (setting.name == "allow_s3_native_copy")
193-
{
194-
SettingFieldBoolAuto bool_auto{setting.value};
195-
res.allow_s3_native_copy = bool_auto.is_auto ? std::nullopt : std::make_optional(bool_auto.base.value);
196-
}
192+
/// else
197193
/// `allow_unresolved_access_dependencies` is an obsolete name.
198-
else if (setting.name == "allow_unresolved_access_dependencies")
194+
if (setting.name == "allow_unresolved_access_dependencies")
199195
{
200196
res.skip_unresolved_access_dependencies = SettingFieldBool{setting.value}.value;
201197
}
@@ -230,9 +226,6 @@ void RestoreSettings::copySettingsToQuery(ASTBackupQuery & query) const
230226

231227
LIST_OF_RESTORE_SETTINGS(COPY_RESTORE_SETTINGS_TO_QUERY)
232228

233-
if (allow_s3_native_copy)
234-
query_settings->changes.emplace_back("allow_s3_native_copy", static_cast<Field>(SettingFieldBool{*allow_s3_native_copy}));
235-
236229
/// Copy the core settings to the query too.
237230
query_settings->changes.insert(query_settings->changes.end(), core_settings.begin(), core_settings.end());
238231

‎src/Backups/RestoreSettings.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,8 @@ struct RestoreSettings
124124
/// How the RESTORE command will handle if a user-defined function which it's going to restore already exists.
125125
RestoreUDFCreationMode create_function = RestoreUDFCreationMode::kCreateIfNotExists;
126126

127-
/// Whether S3 native copy is allowed.
128-
/// If not set, then S3 native copy will be allowed only if the source and destination credentials are the same.
129-
std::optional<bool> allow_s3_native_copy;
127+
/// Whether native copy is allowed (optimization for cloud storages, that sometimes could have bugs)
128+
bool allow_s3_native_copy = true;
130129

131130
/// Whether base backup from S3 should inherit credentials from the RESTORE query.
132131
bool use_same_s3_credentials_for_base_backup = false;

‎src/IO/S3/copyS3File.cpp‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ namespace ErrorCodes
5050

5151
namespace S3RequestSetting
5252
{
53-
extern const S3RequestSettingsBoolAuto allow_native_copy;
53+
extern const S3RequestSettingsBool allow_native_copy;
5454
extern const S3RequestSettingsBool check_objects_after_upload;
5555
extern const S3RequestSettingsUInt64 max_part_number;
5656
extern const S3RequestSettingsBool allow_multipart_copy;
@@ -932,17 +932,9 @@ void copyS3File(
932932
object_metadata);
933933
};
934934

935-
/// Check whether the native copy is allowed and possible.
936-
const auto & allow_native_copy = settings[S3RequestSetting::allow_native_copy];
937-
if (!allow_native_copy.is_auto && (allow_native_copy.base.value == false))
935+
if (!settings[S3RequestSetting::allow_native_copy])
938936
{
939-
LOG_TRACE(getLogger("copyS3File"), "Native copy is disabled for {}", src_key);
940-
fallback_method();
941-
return;
942-
}
943-
if (allow_native_copy.is_auto && (src_s3_client->getCredentials() != dest_s3_client->getCredentials()))
944-
{
945-
LOG_TRACE(getLogger("copyS3File"), "Native copy is not possible for {} because credentials are different", src_key);
937+
LOG_TRACE(getLogger("copyS3File"), "Native copy is disable for {}", src_key);
946938
fallback_method();
947939
return;
948940
}

‎src/IO/S3Defines.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ inline static constexpr uint64_t DEFAULT_MAX_UNEXPECTED_WRITE_ERROR_RETRIES = 4;
3535
inline static constexpr uint64_t DEFAULT_MAX_REDIRECTS = 10;
3636
inline static constexpr uint64_t DEFAULT_RETRY_ATTEMPTS = 100;
3737

38-
inline static constexpr std::string_view DEFAULT_ALLOW_NATIVE_COPY = "auto";
38+
inline static constexpr bool DEFAULT_ALLOW_NATIVE_COPY = true;
3939
inline static constexpr bool DEFAULT_CHECK_OBJECTS_AFTER_UPLOAD = false;
4040

4141
}

‎src/IO/S3RequestSettings.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ namespace ErrorCodes
3333
DECLARE(UInt64, max_single_read_retries, 4, "", 0) \
3434
DECLARE(UInt64, request_timeout_ms, S3::DEFAULT_REQUEST_TIMEOUT_MS, "", 0) \
3535
DECLARE(UInt64, list_object_keys_size, S3::DEFAULT_LIST_OBJECT_KEYS_SIZE, "", 0) \
36-
DECLARE(BoolAuto, allow_native_copy, S3::DEFAULT_ALLOW_NATIVE_COPY, "", 0) \
36+
DECLARE(Bool, allow_native_copy, S3::DEFAULT_ALLOW_NATIVE_COPY, "", 0) \
3737
DECLARE(Bool, check_objects_after_upload, S3::DEFAULT_CHECK_OBJECTS_AFTER_UPLOAD, "", 0) \
3838
DECLARE(Bool, throw_on_zero_files_match, false, "", 0) \
3939
DECLARE(Bool, allow_multipart_copy, true, "", 0) \

0 commit comments

Comments
 (0)