From 0c7a4142e40b186da12c3ac3f0664cb3a94e979f Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 24 Jun 2023 20:57:39 +0200 Subject: [PATCH 1/3] Use separate default settings for clickhouse-local There are already two of them: - storage_file_read_method can use mmap method for clickhouse-local - there is no sense in disabling allow_introspection_functions for clickhouse-local since it can hurt only itself And likely there will be more, once the infrastructure will be there. Signed-off-by: Azat Khuzhin --- src/Core/Settings.h | 2 +- src/Core/SettingsOverridesLocal.cpp | 13 +++++++++++++ src/Core/SettingsOverridesLocal.h | 11 +++++++++++ src/Interpreters/Context.cpp | 3 +++ ...2800_clickhouse_local_default_settings.reference | 2 ++ .../02800_clickhouse_local_default_settings.sh | 8 ++++++++ 6 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 src/Core/SettingsOverridesLocal.cpp create mode 100644 src/Core/SettingsOverridesLocal.h create mode 100644 tests/queries/0_stateless/02800_clickhouse_local_default_settings.reference create mode 100755 tests/queries/0_stateless/02800_clickhouse_local_default_settings.sh diff --git a/src/Core/Settings.h b/src/Core/Settings.h index 3d42bd582ed..c51076f3237 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -657,7 +657,7 @@ class IColumn; M(UInt64, function_range_max_elements_in_block, 500000000, "Maximum number of values generated by function 'range' per block of data (sum of array sizes for every row in a block, see also 'max_block_size' and 'min_insert_block_size_rows'). It is a safety threshold.", 0) \ M(ShortCircuitFunctionEvaluation, short_circuit_function_evaluation, ShortCircuitFunctionEvaluation::ENABLE, "Setting for short-circuit function evaluation configuration. Possible values: 'enable' - use short-circuit function evaluation for functions that are suitable for it, 'disable' - disable short-circuit function evaluation, 'force_enable' - use short-circuit function evaluation for all functions.", 0) \ \ - M(LocalFSReadMethod, storage_file_read_method, LocalFSReadMethod::mmap, "Method of reading data from storage file, one of: read, pread, mmap. The mmap method does not apply to clickhouse-server (it's intended for clickhouse-local).", 0) \ + M(LocalFSReadMethod, storage_file_read_method, LocalFSReadMethod::pread, "Method of reading data from storage file, one of: read, pread, mmap. The mmap method does not apply to clickhouse-server (it's intended for clickhouse-local).", 0) \ M(String, local_filesystem_read_method, "pread_threadpool", "Method of reading data from local filesystem, one of: read, pread, mmap, io_uring, pread_threadpool. The 'io_uring' method is experimental and does not work for Log, TinyLog, StripeLog, File, Set and Join, and other tables with append-able files in presence of concurrent reads and writes.", 0) \ M(String, remote_filesystem_read_method, "threadpool", "Method of reading data from remote filesystem, one of: read, threadpool.", 0) \ M(Bool, local_filesystem_read_prefetch, false, "Should use prefetching when reading data from local filesystem.", 0) \ diff --git a/src/Core/SettingsOverridesLocal.cpp b/src/Core/SettingsOverridesLocal.cpp new file mode 100644 index 00000000000..2beb560ece2 --- /dev/null +++ b/src/Core/SettingsOverridesLocal.cpp @@ -0,0 +1,13 @@ +#include +#include + +namespace DB +{ + +void applySettingsOverridesForLocal(Settings & settings) +{ + settings.allow_introspection_functions = true; + settings.storage_file_read_method = LocalFSReadMethod::mmap; +} + +} diff --git a/src/Core/SettingsOverridesLocal.h b/src/Core/SettingsOverridesLocal.h new file mode 100644 index 00000000000..89b79f4ad55 --- /dev/null +++ b/src/Core/SettingsOverridesLocal.h @@ -0,0 +1,11 @@ +#pragma once + +namespace DB +{ + +struct Settings; + +/// Update some settings defaults for clickhouse-local +void applySettingsOverridesForLocal(Settings & settings); + +} diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 106264320b2..dccdf4efca0 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -48,6 +48,7 @@ #include #include #include +#include #include #include #include @@ -3646,6 +3647,8 @@ void Context::setDefaultProfiles(const Poco::Util::AbstractConfiguration & confi setCurrentProfile(shared->system_profile_name); applySettingsQuirks(settings, &Poco::Logger::get("SettingsQuirks")); + if (shared->application_type == ApplicationType::LOCAL) + applySettingsOverridesForLocal(settings); shared->buffer_profile_name = config.getString("buffer_profile", shared->system_profile_name); buffer_context = Context::createCopy(shared_from_this()); diff --git a/tests/queries/0_stateless/02800_clickhouse_local_default_settings.reference b/tests/queries/0_stateless/02800_clickhouse_local_default_settings.reference new file mode 100644 index 00000000000..0f18d1a3897 --- /dev/null +++ b/tests/queries/0_stateless/02800_clickhouse_local_default_settings.reference @@ -0,0 +1,2 @@ +allow_introspection_functions 1 +storage_file_read_method mmap diff --git a/tests/queries/0_stateless/02800_clickhouse_local_default_settings.sh b/tests/queries/0_stateless/02800_clickhouse_local_default_settings.sh new file mode 100755 index 00000000000..792e187fc51 --- /dev/null +++ b/tests/queries/0_stateless/02800_clickhouse_local_default_settings.sh @@ -0,0 +1,8 @@ +#!/usr/bin/env bash +# Tags: no-random-settings, no-random-merge-tree-settings + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +$CLICKHOUSE_LOCAL -q "select name, value from system.settings where changed" From a7b14f87e0b43f02fac2cd216e906b045dbbfa42 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 24 Jun 2023 21:14:28 +0200 Subject: [PATCH 2/3] Throw an error instead of silenty ignore storage_file_read_method=mmap in server Signed-off-by: Azat Khuzhin --- src/Storages/StorageFile.cpp | 8 ++++---- .../0_stateless/02497_storage_file_reader_selection.sh | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Storages/StorageFile.cpp b/src/Storages/StorageFile.cpp index ff67272e542..5301b159f96 100644 --- a/src/Storages/StorageFile.cpp +++ b/src/Storages/StorageFile.cpp @@ -205,7 +205,7 @@ std::unique_ptr selectReadBuffer( { auto read_method = context->getSettingsRef().storage_file_read_method; - /** But using mmap on server-side is unsafe for the following reasons: + /** Using mmap on server-side is unsafe for the following reasons: * - concurrent modifications of a file will result in SIGBUS; * - IO error from the device will result in SIGBUS; * - recovery from this signal is not feasible even with the usage of siglongjmp, @@ -214,10 +214,10 @@ std::unique_ptr selectReadBuffer( * * But we keep this mode for clickhouse-local as it is not so bad for a command line tool. */ + if (context->getApplicationType() == Context::ApplicationType::SERVER && read_method == LocalFSReadMethod::mmap) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Using storage_file_read_method=mmap is not safe in server mode. Consider using pread."); - if (S_ISREG(file_stat.st_mode) - && context->getApplicationType() != Context::ApplicationType::SERVER - && read_method == LocalFSReadMethod::mmap) + if (S_ISREG(file_stat.st_mode) && read_method == LocalFSReadMethod::mmap) { try { diff --git a/tests/queries/0_stateless/02497_storage_file_reader_selection.sh b/tests/queries/0_stateless/02497_storage_file_reader_selection.sh index 20bde68718d..25387e61db6 100755 --- a/tests/queries/0_stateless/02497_storage_file_reader_selection.sh +++ b/tests/queries/0_stateless/02497_storage_file_reader_selection.sh @@ -13,4 +13,6 @@ $CLICKHOUSE_LOCAL --storage_file_read_method=mmap --print-profile-events -q "SEL $CLICKHOUSE_LOCAL --storage_file_read_method=pread --print-profile-events -q "SELECT * FROM file($DATA_FILE) FORMAT Null" 2>&1 | grep -F -c "CreatedReadBufferMMap" $CLICKHOUSE_LOCAL --storage_file_read_method=pread --print-profile-events -q "SELECT * FROM file($DATA_FILE) FORMAT Null" 2>&1 | grep -F -c "CreatedReadBufferOrdinary" +$CLICKHOUSE_CLIENT --storage_file_read_method=mmap -nq "SELECT * FROM file('/dev/null', 'LineAsString') FORMAT Null -- { serverError BAD_ARGUMENTS }" + rm $DATA_FILE From 59f11863d7776134c383b168a1ec7ff2acc8bc16 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Sat, 24 Jun 2023 21:41:33 +0200 Subject: [PATCH 3/3] Simplify settings overrides or clickhouse-local Signed-off-by: Azat Khuzhin --- programs/local/LocalServer.cpp | 16 +++++++++++++++- src/Core/SettingsOverridesLocal.cpp | 13 ------------- src/Core/SettingsOverridesLocal.h | 11 ----------- src/Interpreters/Context.cpp | 3 --- 4 files changed, 15 insertions(+), 28 deletions(-) delete mode 100644 src/Core/SettingsOverridesLocal.cpp delete mode 100644 src/Core/SettingsOverridesLocal.h diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index caca7cfb50d..033d2b91ec6 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -71,6 +71,15 @@ namespace ErrorCodes extern const int FILE_ALREADY_EXISTS; } +void applySettingsOverridesForLocal(ContextMutablePtr context) +{ + Settings settings = context->getSettings(); + + settings.allow_introspection_functions = true; + settings.storage_file_read_method = LocalFSReadMethod::mmap; + + context->setSettings(settings); +} void LocalServer::processError(const String &) const { @@ -657,6 +666,12 @@ void LocalServer::processConfig() CompiledExpressionCacheFactory::instance().init(compiled_expression_cache_size, compiled_expression_cache_elements_size); #endif + /// NOTE: it is important to apply any overrides before + /// setDefaultProfiles() calls since it will copy current context (i.e. + /// there is separate context for Buffer tables). + applySettingsOverridesForLocal(global_context); + applyCmdOptions(global_context); + /// Load global settings from default_profile and system_profile. global_context->setDefaultProfiles(config()); @@ -671,7 +686,6 @@ void LocalServer::processConfig() std::string default_database = config().getString("default_database", "_local"); DatabaseCatalog::instance().attachDatabase(default_database, std::make_shared(default_database, global_context)); global_context->setCurrentDatabase(default_database); - applyCmdOptions(global_context); if (config().has("path")) { diff --git a/src/Core/SettingsOverridesLocal.cpp b/src/Core/SettingsOverridesLocal.cpp deleted file mode 100644 index 2beb560ece2..00000000000 --- a/src/Core/SettingsOverridesLocal.cpp +++ /dev/null @@ -1,13 +0,0 @@ -#include -#include - -namespace DB -{ - -void applySettingsOverridesForLocal(Settings & settings) -{ - settings.allow_introspection_functions = true; - settings.storage_file_read_method = LocalFSReadMethod::mmap; -} - -} diff --git a/src/Core/SettingsOverridesLocal.h b/src/Core/SettingsOverridesLocal.h deleted file mode 100644 index 89b79f4ad55..00000000000 --- a/src/Core/SettingsOverridesLocal.h +++ /dev/null @@ -1,11 +0,0 @@ -#pragma once - -namespace DB -{ - -struct Settings; - -/// Update some settings defaults for clickhouse-local -void applySettingsOverridesForLocal(Settings & settings); - -} diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index dccdf4efca0..106264320b2 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -48,7 +48,6 @@ #include #include #include -#include #include #include #include @@ -3647,8 +3646,6 @@ void Context::setDefaultProfiles(const Poco::Util::AbstractConfiguration & confi setCurrentProfile(shared->system_profile_name); applySettingsQuirks(settings, &Poco::Logger::get("SettingsQuirks")); - if (shared->application_type == ApplicationType::LOCAL) - applySettingsOverridesForLocal(settings); shared->buffer_profile_name = config.getString("buffer_profile", shared->system_profile_name); buffer_context = Context::createCopy(shared_from_this());