Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make sanity check of settings worse #63119

Merged
merged 5 commits into from
Apr 30, 2024
Merged

Make sanity check of settings worse #63119

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97c781e
Sanity check: Clamp values instead of throwing
Algunenano Apr 29, 2024
30d10bf
Style
Algunenano Apr 29, 2024
987558e
Improvements based on review
Algunenano Apr 29, 2024
1672709
Fix client crash
Algunenano Apr 29, 2024
5355dec
Give the server a chance
Algunenano Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/Core/SettingsQuirks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@ bool queryProfilerWorks() { return false; }
namespace DB
{

namespace ErrorCodes
{
extern const int INVALID_SETTING_VALUE;
}

/// Update some settings defaults to avoid some known issues.
void applySettingsQuirks(Settings & settings, LoggerPtr log)
{
Expand Down Expand Up @@ -95,7 +90,7 @@ void applySettingsQuirks(Settings & settings, LoggerPtr log)
}
}

void doSettingsSanityCheck(const Settings & current_settings)
void doSettingsSanityCheckClamp(Settings & current_settings)
{
auto getCurrentValue = [&current_settings](const std::string_view name) -> Field
{
Expand All @@ -106,8 +101,9 @@ void doSettingsSanityCheck(const Settings & current_settings)
};

UInt64 max_threads = getCurrentValue("max_threads").get<UInt64>();
if (max_threads > getNumberOfPhysicalCPUCores() * 65536)
throw Exception(ErrorCodes::INVALID_SETTING_VALUE, "Sanity check: Too many threads requested ({})", max_threads);
UInt64 max_threads_max_value = 256 * getNumberOfPhysicalCPUCores();
if (max_threads > max_threads_max_value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe log the fact that clamping happened, so when somebody gets surprised, we can check the logs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs can rotate quite quickly. I would love to see this information in system.errors as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add some logs. Adding to system.errors might come in the future since we need extra code to support this (right now we only log exceptions) and I'd rather not backport that part.

current_settings.set("max_threads", max_threads_max_value);

constexpr UInt64 max_sane_block_rows_size = 4294967296; // 2^32
std::unordered_set<String> block_rows_settings{
Expand All @@ -122,7 +118,7 @@ void doSettingsSanityCheck(const Settings & current_settings)
{
auto block_size = getCurrentValue(setting).get<UInt64>();
if (block_size > max_sane_block_rows_size)
throw Exception(ErrorCodes::INVALID_SETTING_VALUE, "Sanity check: '{}' value is too high ({})", setting, block_size);
current_settings.set(setting, max_sane_block_rows_size);
}
}
}
4 changes: 2 additions & 2 deletions src/Core/SettingsQuirks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ struct Settings;
/// Update some settings defaults to avoid some known issues.
void applySettingsQuirks(Settings & settings, LoggerPtr log = nullptr);

/// Verify that some settings have sane values. Throws if not
void doSettingsSanityCheck(const Settings & settings);
/// Verify that some settings have sane values. Alters the value to a reasonable one if not
void doSettingsSanityCheckClamp(Settings & settings);
}
52 changes: 26 additions & 26 deletions src/Interpreters/Context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1374,18 +1374,18 @@ std::shared_ptr<const EnabledRolesInfo> Context::getRolesInfo() const
namespace
{
ALWAYS_INLINE inline void
contextSanityCheckWithLock(const Context & context, const Settings & settings, const std::lock_guard<ContextSharedMutex> &)
contextSanityClampSettingsWithLock(const Context & context, Settings & settings, const std::lock_guard<ContextSharedMutex> &)
{
const auto type = context.getApplicationType();
if (type == Context::ApplicationType::LOCAL || type == Context::ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

ALWAYS_INLINE inline void contextSanityCheck(const Context & context, const Settings & settings)
ALWAYS_INLINE inline void contextSanityClampSettings(const Context & context, Settings & settings)
{
const auto type = context.getApplicationType();
if (type == Context::ApplicationType::LOCAL || type == Context::ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}
}

Expand Down Expand Up @@ -1498,7 +1498,7 @@ void Context::setCurrentProfilesWithLock(const SettingsProfilesInfo & profiles_i
checkSettingsConstraintsWithLock(profiles_info.settings, SettingSource::PROFILE);
applySettingsChangesWithLock(profiles_info.settings, lock);
settings_constraints_and_current_profiles = profiles_info.getConstraintsAndProfileIDs(settings_constraints_and_current_profiles);
contextSanityCheckWithLock(*this, settings, lock);
contextSanityClampSettingsWithLock(*this, settings, lock);
}

void Context::setCurrentProfile(const String & profile_name, bool check_constraints)
Expand Down Expand Up @@ -2101,7 +2101,7 @@ void Context::setSettings(const Settings & settings_)
std::lock_guard lock(mutex);
settings = settings_;
need_recalculate_access = true;
contextSanityCheck(*this, settings);
contextSanityClampSettings(*this, settings);
}

void Context::setSettingWithLock(std::string_view name, const String & value, const std::lock_guard<ContextSharedMutex> & lock)
Expand All @@ -2114,7 +2114,7 @@ void Context::setSettingWithLock(std::string_view name, const String & value, co
settings.set(name, value);
if (ContextAccessParams::dependsOnSettingName(name))
need_recalculate_access = true;
contextSanityCheckWithLock(*this, settings, lock);
contextSanityClampSettingsWithLock(*this, settings, lock);
}

void Context::setSettingWithLock(std::string_view name, const Field & value, const std::lock_guard<ContextSharedMutex> & lock)
Expand All @@ -2134,7 +2134,7 @@ void Context::applySettingChangeWithLock(const SettingChange & change, const std
try
{
setSettingWithLock(change.name, change.value, lock);
contextSanityCheckWithLock(*this, settings, lock);
contextSanityClampSettingsWithLock(*this, settings, lock);
}
catch (Exception & e)
{
Expand Down Expand Up @@ -2162,7 +2162,7 @@ void Context::setSetting(std::string_view name, const Field & value)
{
std::lock_guard lock(mutex);
setSettingWithLock(name, value, lock);
contextSanityCheckWithLock(*this, settings, lock);
contextSanityClampSettingsWithLock(*this, settings, lock);
}

void Context::applySettingChange(const SettingChange & change)
Expand All @@ -2187,72 +2187,72 @@ void Context::applySettingsChanges(const SettingsChanges & changes)
applySettingsChangesWithLock(changes, lock);
}

void Context::checkSettingsConstraintsWithLock(const SettingsProfileElements & profile_elements, SettingSource source) const
void Context::checkSettingsConstraintsWithLock(const SettingsProfileElements & profile_elements, SettingSource source)
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(settings, profile_elements, source);
if (getApplicationType() == ApplicationType::LOCAL || getApplicationType() == ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::checkSettingsConstraintsWithLock(const SettingChange & change, SettingSource source) const
void Context::checkSettingsConstraintsWithLock(const SettingChange & change, SettingSource source)
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(settings, change, source);
if (getApplicationType() == ApplicationType::LOCAL || getApplicationType() == ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::checkSettingsConstraintsWithLock(const SettingsChanges & changes, SettingSource source) const
void Context::checkSettingsConstraintsWithLock(const SettingsChanges & changes, SettingSource source)
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(settings, changes, source);
if (getApplicationType() == ApplicationType::LOCAL || getApplicationType() == ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::checkSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source) const
void Context::checkSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source)
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(settings, changes, source);
if (getApplicationType() == ApplicationType::LOCAL || getApplicationType() == ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::clampToSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source) const
void Context::clampToSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source)
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.clamp(settings, changes, source);
if (getApplicationType() == ApplicationType::LOCAL || getApplicationType() == ApplicationType::SERVER)
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::checkMergeTreeSettingsConstraintsWithLock(const MergeTreeSettings & merge_tree_settings, const SettingsChanges & changes) const
{
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(merge_tree_settings, changes);
}

void Context::checkSettingsConstraints(const SettingsProfileElements & profile_elements, SettingSource source) const
void Context::checkSettingsConstraints(const SettingsProfileElements & profile_elements, SettingSource source)
{
SharedLockGuard lock(mutex);
checkSettingsConstraintsWithLock(profile_elements, source);
}

void Context::checkSettingsConstraints(const SettingChange & change, SettingSource source) const
void Context::checkSettingsConstraints(const SettingChange & change, SettingSource source)
{
SharedLockGuard lock(mutex);
checkSettingsConstraintsWithLock(change, source);
}

void Context::checkSettingsConstraints(const SettingsChanges & changes, SettingSource source) const
void Context::checkSettingsConstraints(const SettingsChanges & changes, SettingSource source)
{
SharedLockGuard lock(mutex);
getSettingsConstraintsAndCurrentProfilesWithLock()->constraints.check(settings, changes, source);
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);
}

void Context::checkSettingsConstraints(SettingsChanges & changes, SettingSource source) const
void Context::checkSettingsConstraints(SettingsChanges & changes, SettingSource source)
{
SharedLockGuard lock(mutex);
checkSettingsConstraintsWithLock(changes, source);
}

void Context::clampToSettingsConstraints(SettingsChanges & changes, SettingSource source) const
void Context::clampToSettingsConstraints(SettingsChanges & changes, SettingSource source)
{
SharedLockGuard lock(mutex);
clampToSettingsConstraintsWithLock(changes, source);
Expand Down Expand Up @@ -4484,7 +4484,7 @@ void Context::setDefaultProfiles(const Poco::Util::AbstractConfiguration & confi
setCurrentProfile(shared->system_profile_name);

applySettingsQuirks(settings, getLogger("SettingsQuirks"));
doSettingsSanityCheck(settings);
doSettingsSanityCheckClamp(settings);

shared->buffer_profile_name = config.getString("buffer_profile", shared->system_profile_name);
buffer_context = Context::createCopy(shared_from_this());
Expand Down
20 changes: 10 additions & 10 deletions src/Interpreters/Context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,11 +783,11 @@ class Context: public ContextData, public std::enable_shared_from_this<Context>
void applySettingsChanges(const SettingsChanges & changes);

/// Checks the constraints.
void checkSettingsConstraints(const SettingsProfileElements & profile_elements, SettingSource source) const;
void checkSettingsConstraints(const SettingChange & change, SettingSource source) const;
void checkSettingsConstraints(const SettingsChanges & changes, SettingSource source) const;
void checkSettingsConstraints(SettingsChanges & changes, SettingSource source) const;
void clampToSettingsConstraints(SettingsChanges & changes, SettingSource source) const;
void checkSettingsConstraints(const SettingsProfileElements & profile_elements, SettingSource source);
void checkSettingsConstraints(const SettingChange & change, SettingSource source);
void checkSettingsConstraints(const SettingsChanges & changes, SettingSource source);
void checkSettingsConstraints(SettingsChanges & changes, SettingSource source);
void clampToSettingsConstraints(SettingsChanges & changes, SettingSource source);
void checkMergeTreeSettingsConstraints(const MergeTreeSettings & merge_tree_settings, const SettingsChanges & changes) const;

/// Reset settings to default value
Expand Down Expand Up @@ -1293,15 +1293,15 @@ class Context: public ContextData, public std::enable_shared_from_this<Context>

void setCurrentDatabaseWithLock(const String & name, const std::lock_guard<ContextSharedMutex> & lock);

void checkSettingsConstraintsWithLock(const SettingsProfileElements & profile_elements, SettingSource source) const;
void checkSettingsConstraintsWithLock(const SettingsProfileElements & profile_elements, SettingSource source);

void checkSettingsConstraintsWithLock(const SettingChange & change, SettingSource source) const;
void checkSettingsConstraintsWithLock(const SettingChange & change, SettingSource source);

void checkSettingsConstraintsWithLock(const SettingsChanges & changes, SettingSource source) const;
void checkSettingsConstraintsWithLock(const SettingsChanges & changes, SettingSource source);

void checkSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source) const;
void checkSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source);

void clampToSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source) const;
void clampToSettingsConstraintsWithLock(SettingsChanges & changes, SettingSource source);

void checkMergeTreeSettingsConstraintsWithLock(const MergeTreeSettings & merge_tree_settings, const SettingsChanges & changes) const;

Expand Down
18 changes: 18 additions & 0 deletions tests/queries/0_stateless/02994_sanity_check_settings.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
0 0
0 0
1
1
1
1
1
1
1
1
1
1
(Expression)
ExpressionTransform
(Limit)
Limit
(ReadFromStorage)
Zeros 0 → 1
10 changes: 5 additions & 5 deletions tests/queries/0_stateless/02994_sanity_check_settings.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ FROM numbers(1);

SELECT * APPLY max
FROM data_02052_1_wide0__fuzz_48
GROUP BY toFixedString(toFixedString(toFixedString(toFixedString(toFixedString(toLowCardinality('UInt256'), toFixedString(toNullable(toNullable(2)), toFixedString(toFixedString(7), 7)), 7), 7), materialize(toNullable(7))), 7), materialize(7))
GROUP BY key
WITH CUBE
SETTINGS max_read_buffer_size = 7, max_threads = 9223372036854775807; -- { serverError INVALID_SETTING_VALUE }
SETTINGS max_read_buffer_size = 7, max_threads = 9223372036854775807;

SELECT zero + 1 AS x
FROM system.zeros
SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break'; -- { serverError INVALID_SETTING_VALUE }
FROM system.zeros LIMIT 10
SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break';

EXPLAIN PIPELINE SELECT zero + 1 AS x FROM system.zeros SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break'; -- { serverError INVALID_SETTING_VALUE }
EXPLAIN PIPELINE SELECT zero + 1 AS x FROM system.zeros LIMIT 10 SETTINGS max_block_size = 9223372036854775806, max_rows_to_read = 20, read_overflow_mode = 'break';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do something like:

SET max_block_size =9223372036854775806;
SELECT value FROM system.settings WHERE name = 'max_block_size';