Refactor alter addition

dbms/src/Interpreters/InterpreterAlterQuery.cpp

@@ -106,6 +106,7 @@ BlockIO InterpreterAlterQuery::execute()
         StorageInMemoryMetadata metadata = table->getInMemoryMetadata();
         alter_commands.validate(metadata, context);
         alter_commands.prepare(metadata, context);
+        table->checkAlterIsPossible(alter_commands, context.getSettingsRef());
         table->alter(alter_commands, context, table_lock_holder);
     }
 

dbms/src/Storages/AlterCommands.cpp

@@ -392,8 +392,9 @@ void AlterCommand::apply(StorageInMemoryMetadata & metadata) const
         for (const auto & change : settings_changes)
         {
             auto finder = [&change](const SettingChange & c) { return c.name == change.name; };
-            if (auto it = std::find_if(settings_from_storage.begin(), settings_from_storage.end(), finder);
-                it != settings_from_storage.end())
+            auto it = std::find_if(settings_from_storage.begin(), settings_from_storage.end(), finder);
+
+            if (it != settings_from_storage.end())
                 it->value = change.value;
             else
                 settings_from_storage.push_back(change);
@@ -644,11 +645,6 @@ void AlterCommands::prepare(const StorageInMemoryMetadata & metadata, const Cont
 
 void AlterCommands::validate(const StorageInMemoryMetadata & metadata, const Context & context) const
 {
-    /// We will save ALTER ADD/MODIFY command indices (only the last for each column) for possible modification
-    /// (we might need to add deduced types or modify default expressions).
-    /// Saving indices because we can add new commands later and thus cause vector resize.
-    std::unordered_map<String, size_t> column_to_command_idx;
-
     for (size_t i = 0; i < size(); ++i)
     {
         auto & command = (*this)[i];

dbms/src/Storages/AlterCommands.h

@@ -96,32 +96,45 @@ struct AlterCommand
     /// in each part on disk (it's not lightweight alter).
     bool isModifyingData() const;
 
-    /// checks that only settings changed by alter
+    /// Checks that only settings changed by alter
     bool isSettingsAlter() const;
 
+    /// Checks that only comment changed by alter
     bool isCommentAlter() const;
 };
 
+/// Return string representation of AlterCommand::Type
 String alterTypeToString(const AlterCommand::Type type);
 
 class Context;
 
+/// Vector of AlterCommand with several additional functions
 class AlterCommands : public std::vector<AlterCommand>
 {
 private:
     bool prepared = false;
 public:
-    void apply(StorageInMemoryMetadata & metadata) const;
-
+    /// Validate that commands can be applied to metadata.
+    /// Checks that all columns exist and dependecies between them.
+    /// This check is lightweight and base only on metadata.
+    /// More accurate check have to be performed with storage->checkAlterIsPossible.
+    void validate(const StorageInMemoryMetadata & metadata, const Context & context) const;
 
+    /// Prepare alter commands. Set ignore flag to some of them
+    /// and additional commands for dependent columns.
     void prepare(const StorageInMemoryMetadata & metadata, const Context & context);
 
-    void validate(const StorageInMemoryMetadata & metadata, const Context & context) const;
+    /// Apply all alter command in sequential order to storage metadata.
+    /// Commands have to be prepared before apply.
+    void apply(StorageInMemoryMetadata & metadata) const;
 
+    /// At least one command modify data on disk.
     bool isModifyingData() const;
 
+    /// At least one command modify settings.
     bool isSettingsAlter() const;
 
+    /// At least one command modify comments.
     bool isCommentAlter() const;
 };
 

dbms/src/Storages/IStorage.cpp

@@ -381,12 +381,13 @@ StorageInMemoryMetadata IStorage::getInMemoryMetadata() const
 void IStorage::alter(
     const AlterCommands & params,
     const Context & context,
-    TableStructureWriteLockHolder & /*table_lock_holder*/)
+    TableStructureWriteLockHolder & table_lock_holder)
 {
-    checkAlterIsPossible(params, context.getSettingsRef());
-
     const String database_name = getDatabaseName();
     const String table_name = getTableName();
+
+    lockStructureExclusively(table_lock_holder, context.getCurrentQueryId());
+
     StorageInMemoryMetadata metadata = getInMemoryMetadata();
     params.apply(metadata);
     context.getDatabase(database_name)->alterTable(context, table_name, metadata);

dbms/src/Storages/StorageBuffer.cpp

@@ -716,8 +716,6 @@ void StorageBuffer::alter(const AlterCommands & params, const Context & context,
 {
     lockStructureExclusively(table_lock_holder, context.getCurrentQueryId());
 
-    checkAlterIsPossible(params, context.getSettingsRef());
-
     const String database_name_ = getDatabaseName();
     const String table_name_ = getTableName();
 

dbms/src/Storages/StorageDistributed.cpp

@@ -411,8 +411,6 @@ void StorageDistributed::alter(const AlterCommands & params, const Context & con
 {
     lockStructureExclusively(table_lock_holder, context.getCurrentQueryId());
 
-    checkAlterIsPossible(params, context.getSettingsRef());
-
     const String current_database_name = getDatabaseName();
     const String current_table_name = getTableName();
 

dbms/src/Storages/StorageMergeTree.cpp

@@ -252,8 +252,6 @@ void StorageMergeTree::alter(
 
     lockNewDataStructureExclusively(table_lock_holder, context.getCurrentQueryId());
 
-    checkAlterIsPossible(params, context.getSettingsRef());
-
     StorageInMemoryMetadata metadata = getInMemoryMetadata();
 
     params.apply(metadata);

dbms/src/Storages/StorageReplicatedMergeTree.cpp

@@ -3198,9 +3198,6 @@ void StorageReplicatedMergeTree::alter(
     const String current_database_name = getDatabaseName();
     const String current_table_name = getTableName();
 
-
-    checkAlterIsPossible(params, query_context.getSettingsRef());
-
     /// We cannot check this alter commands with method isModifyingData()
     /// because ReplicatedMergeTree stores both columns and metadata for
     /// each replica. So we have to wait AlterThread even with lightweight