[FLASH-796]: support rename pk column (#379)

* try to print some log * fix * fix pk prob * output stacktrace * add some logs * fix view * address comments * address comments * address comment Co-authored-by: lidezhu <[email protected]>
pingcap · Jan 11, 2020 · cd7bf30 · cd7bf30
1 parent 8892fd1
commit cd7bf30
Show file tree

Hide file tree

Showing 7 changed files with 58 additions and 26 deletions.
diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp
@@ -938,8 +938,11 @@ void MergeTreeData::checkAlter(const AlterCommands & commands)
                     continue;
             }
 
+            if (command.type == AlterCommand::RENAME_COLUMN || command.type == AlterCommand::MODIFY_COLUMN)
+                    continue;
+
             throw Exception(
-                    "ALTER of key column " + command.column_name + " must be metadata-only",
+                    "ALTER of key column " + command.column_name + " must be metadata-only, and command tyoe is " + std::to_string(command.type),
                     ErrorCodes::ILLEGAL_COLUMN);
         }
     }

diff --git a/dbms/src/Storages/StorageMergeTree.cpp b/dbms/src/Storages/StorageMergeTree.cpp
@@ -365,9 +365,15 @@ void StorageMergeTree::alterInternal(
         else if (param.type == AlterCommand::RENAME_COLUMN)
         {
             rename_column = true;
+            if (param.primary_key != nullptr)
+            {
+                LOG_INFO(log, "old pk: " << *new_primary_key_ast);
+                new_primary_key_ast = param.primary_key;
+                LOG_INFO(log, "change to new pk: " << *new_primary_key_ast);
+            }
             if (params.size() != 1)
             {
-                throw Exception("There is an internal error for rename columns", ErrorCodes::LOGICAL_ERROR);
+                throw Exception("There is an internal error for rename columns, params size is " + std::to_string(params.size()) + ", but should be 1", ErrorCodes::LOGICAL_ERROR);
             }
         }
     }
@@ -418,7 +424,7 @@ void StorageMergeTree::alterInternal(
     if (table_info)
         setTableInfo(table_info->get());
 
-    if (primary_key_is_modified)
+    if (new_primary_key_ast != nullptr)
     {
         data.primary_expr_ast = new_primary_key_ast;
     }

diff --git a/dbms/src/Storages/Transaction/SchemaBuilder.cpp b/dbms/src/Storages/Transaction/SchemaBuilder.cpp
@@ -45,6 +45,25 @@ inline void setAlterCommandColumn(Logger * log, AlterCommand & command, const Co
     }
 }
 
+AlterCommand newRenameColCommand(const String & old_col, const String & new_col, const TableInfo & orig_table_info)
+{
+    AlterCommand command;
+    command.type = AlterCommand::RENAME_COLUMN;
+    command.column_name = old_col;
+    command.new_column_name = new_col;
+    if (auto pk = orig_table_info.getPKHandleColumn())
+    {
+        if (pk->get().name == old_col)
+        {
+            auto list = std::make_shared<ASTExpressionList>();
+            auto new_pk = std::make_shared<ASTIdentifier>(new_col);
+            list->children.push_back(new_pk);
+            command.primary_key = list;
+        }
+    }
+    return command;
+}
+
 inline std::vector<AlterCommands> detectSchemaChanges(Logger * log, const TableInfo & table_info, const TableInfo & orig_table_info)
 {
     std::vector<AlterCommands> result;
@@ -94,11 +113,7 @@ inline std::vector<AlterCommands> detectSchemaChanges(Logger * log, const TableI
         for (const auto & rename_pair : rename_result)
         {
             AlterCommands rename_commands;
-            AlterCommand command;
-            command.type = AlterCommand::RENAME_COLUMN;
-            command.column_name = rename_pair.first;
-            command.new_column_name = rename_pair.second;
-            rename_commands.push_back(command);
+            rename_commands.push_back(newRenameColCommand(rename_pair.first, rename_pair.second, orig_table_info));
             result.push_back(rename_commands);
         }
     }
@@ -247,44 +262,37 @@ void SchemaBuilder<Getter>::applyDiff(const SchemaDiff & diff)
     switch (diff.type)
     {
         case SchemaActionCreateTable:
-        case SchemaActionRecoverTable:
-        {
+        case SchemaActionRecoverTable: {
             newTableID = diff.table_id;
             break;
         }
         case SchemaActionDropTable:
-        case SchemaActionDropView:
-        {
+        case SchemaActionDropView: {
             oldTableID = diff.table_id;
             break;
         }
-        case SchemaActionTruncateTable:
-        {
+        case SchemaActionTruncateTable: {
             newTableID = diff.table_id;
             oldTableID = diff.old_table_id;
             break;
         }
         case SchemaActionAddColumn:
         case SchemaActionDropColumn:
         case SchemaActionModifyColumn:
-        case SchemaActionSetDefaultValue:
-        {
+        case SchemaActionSetDefaultValue: {
             applyAlterTable(di, diff.table_id);
             break;
         }
-        case SchemaActionRenameTable:
-        {
+        case SchemaActionRenameTable: {
             applyRenameTable(di, diff.old_schema_id, diff.table_id);
             break;
         }
         case SchemaActionAddTablePartition:
-        case SchemaActionDropTablePartition:
-        {
+        case SchemaActionDropTablePartition: {
             applyAlterPartition(di, diff.table_id);
             break;
         }
-        default:
-        {
+        default: {
             LOG_INFO(log, "ignore change type: " << int(diff.type));
             break;
         }
@@ -499,12 +507,14 @@ void SchemaBuilder<Getter>::applyDropSchemaImpl(const String & database_name)
     drop_interpreter.execute();
 }
 
-String createTableStmt(const DBInfo & db_info, const TableInfo & table_info)
+String createTableStmt(const DBInfo & db_info, const TableInfo & table_info, Logger * log)
 {
+    LOG_DEBUG(log, "Analyzing table info :" << table_info.serialize());
     NamesAndTypes columns;
     std::vector<String> pks;
     for (const auto & column : table_info.columns)
     {
+        LOG_DEBUG(log, "Analyzing column :" + column.name + " type " + std::to_string((int)column.tp));
         DataTypePtr type = getDataTypeByColumnInfo(column);
         columns.emplace_back(NameAndTypePair(column.name, type));
 
@@ -553,9 +563,15 @@ String createTableStmt(const DBInfo & db_info, const TableInfo & table_info)
 template <typename Getter>
 void SchemaBuilder<Getter>::applyCreatePhysicalTableImpl(const TiDB::DBInfo & db_info, TiDB::TableInfo & table_info)
 {
+    if (table_info.is_view)
+    {
+        LOG_INFO(log, "Table " << table_info.name << " is a view table, ignore it.");
+        return;
+    }
+
     table_info.schema_version = target_version;
 
-    String stmt = createTableStmt(db_info, table_info);
+    String stmt = createTableStmt(db_info, table_info, log);
 
     LOG_INFO(log, "try to create table with stmt: " << stmt);
 

diff --git a/dbms/src/Storages/Transaction/SchemaSyncService.cpp b/dbms/src/Storages/Transaction/SchemaSyncService.cpp
@@ -17,7 +17,7 @@ SchemaSyncService::SchemaSyncService(DB::Context & context_)
             }
             catch (const Exception & e)
             {
-                LOG_WARNING(log, "Schema sync failed by " << e.message());
+                LOG_WARNING(log, "Schema sync failed by " << e.displayText() << " \n stack : " << e.getStackTrace().toString());
             }
             return false;
         },

diff --git a/dbms/src/Storages/Transaction/TiDB.cpp b/dbms/src/Storages/Transaction/TiDB.cpp
@@ -399,6 +399,10 @@ void TableInfo::deserialize(const String & json_str) try
     {
         schema_version = obj->getValue<Int64>("schema_version");
     }
+    if (obj->has("view") && !obj->getObject("view").isNull())
+    {
+        is_view = true;
+    }
 }
 catch (const Poco::Exception & e)
 {

diff --git a/dbms/src/Storages/Transaction/TiDB.h b/dbms/src/Storages/Transaction/TiDB.h
@@ -272,6 +272,8 @@ struct TableInfo
     bool is_partition_table = false;
     TableID belonging_table_id = -1;
     PartitionInfo partition;
+    // If the table is view, we should ignore it.
+    bool is_view = false;
     Int64 schema_version = -1;
 
     ColumnID getColumnID(const String & name) const;

diff --git a/dbms/src/Storages/Transaction/TiDBSchemaSyncer.h b/dbms/src/Storages/Transaction/TiDBSchemaSyncer.h
@@ -78,6 +78,7 @@ struct TiDBSchemaSyncer : public SchemaSyncer
             loadAllSchema(getter, version, context);
         }
         cur_version = version;
+        LOG_INFO(log, "end sync schema, version has been updated to " + std::to_string(cur_version));
         return true;
     }
 
@@ -109,7 +110,7 @@ struct TiDBSchemaSyncer : public SchemaSyncer
         }
         catch (Exception & e)
         {
-            LOG_ERROR(log, "apply diff meets exception : " + e.displayText());
+            LOG_ERROR(log, "apply diff meets exception : " << e.displayText() << " \n stack is " << e.getStackTrace().toString());
             return false;
         }
         return true;