Better comment

This commit is contained in:
alesapin 2019-08-06 19:29:31 +03:00
parent ca29343f54
commit e62101b8e8
6 changed files with 20 additions and 12 deletions

View File

@ -587,24 +587,28 @@ public:
return found_changes;
}
/// Applies changes to the settings. Doesn't check settings mutability.
/// Applies change to the settings. Doesn't check settings mutability.
void loadFromChange(const SettingChange & change)
{
set(change.name, change.value);
}
/// Applies changes to the settings. Should be used in initial settings loading.
/// (on table creation or loading from config)
void loadFromChanges(const SettingsChanges & changes)
{
for (const SettingChange & change : changes)
loadFromChange(change);
}
/// Applies changes to the settings, checks settings mutability.
/// Applies change to the settings, checks settings mutability.
void updateFromChange(const SettingChange & change)
{
update(change.name, change.value);
}
/// Applies changes to the settings. Should be used for settigns update.
/// (ALTER MODIFY SETTINGS)
void updateFromChanges(const SettingsChanges & changes)
{
for (const SettingChange & change : changes)

View File

@ -12,7 +12,6 @@
#include <Common/ActionLock.h>
#include <Common/Exception.h>
#include <Common/RWLock.h>
#include <Common/SettingsChanges.h>
#include <optional>
#include <shared_mutex>
@ -130,7 +129,7 @@ public: /// thread-unsafe part. lockStructure must be acquired
/// If |need_all| is set, then checks that all the columns of the table are in the block.
void check(const Block & block, bool need_all = false) const;
/// Check storage has setting
/// Check storage has setting. Exception will be thrown if it doesn't support settings at all.
virtual bool hasSetting(const String & setting_name) const;
protected: /// still thread-unsafe part.

View File

@ -1269,7 +1269,6 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c
{
if (!hasSetting(setting.name))
throw Exception{"Storage '" + getName() + "' doesn't have setting '" + setting.name + "'", ErrorCodes::UNKNOWN_SETTING};
}
/// Check that type conversions are possible.
@ -1588,7 +1587,8 @@ void MergeTreeData::alterSettings(
const SettingsChanges & new_changes,
const String & current_database_name,
const String & current_table_name,
const Context & context)
const Context & context,
TableStructureWriteLockHolder & /* table_lock_holder */)
{
settings.updateFromChanges(new_changes);
IDatabase::ASTModifier storage_modifier = [&] (IAST & ast)

View File

@ -509,13 +509,16 @@ public:
bool skip_sanity_checks,
AlterDataPartTransactionPtr& transaction);
/// Performs ALTER of table settings
/// Performs ALTER of table settings (MergeTreeSettings). Lightweight operation, affects metadata only.
/// Not atomic, have to be done with alter intention lock.
void alterSettings(
const SettingsChanges & new_changes,
const String & current_database_name,
const String & current_table_name,
const Context & context);
const Context & context,
TableStructureWriteLockHolder & table_lock_holder);
/// All MergeTreeData children have settings.
bool hasSetting(const String & setting_name) const override;
/// Remove columns, that have been markedd as empty after zeroing values with expired ttl

View File

@ -245,15 +245,16 @@ void StorageMergeTree::alter(
{
if (!params.isMutable())
{
lockStructureExclusively(table_lock_holder, context.getCurrentQueryId());
SettingsChanges new_changes;
/// We don't need to lock table structure exclusively to ALTER settings.
if (params.isSettingsAlter())
{
params.applyForSettingsOnly(new_changes);
alterSettings(new_changes, current_database_name, current_table_name, context);
alterSettings(new_changes, current_database_name, current_table_name, context, table_lock_holder);
return;
}
lockStructureExclusively(table_lock_holder, context.getCurrentQueryId());
auto new_columns = getColumns();
auto new_indices = getIndices();
ASTPtr new_order_by_ast = order_by_ast;

View File

@ -3081,11 +3081,12 @@ void StorageReplicatedMergeTree::alter(
if (params.isSettingsAlter())
{
/// We don't replicate settings ALTER. It's local operation.
/// Also we don't upgrade alter lock to table structure lock.
LOG_DEBUG(log, "ALTER settings only");
lockStructureExclusively(table_lock_holder, query_context.getCurrentQueryId());
SettingsChanges new_changes;
params.applyForSettingsOnly(new_changes);
alterSettings(new_changes, current_database_name, current_table_name, query_context);
alterSettings(new_changes, current_database_name, current_table_name, query_context, table_lock_holder);
return;
}