From 68dbbb9850e8669cad7a2cbbf8382e7bc08da45d Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Mon, 25 Apr 2022 12:22:50 +0800 Subject: [PATCH] Fix create table error (#4630) (#4650) close pingcap/tiflash#4596 --- .gitmodules | 6 +- dbms/src/Databases/test/gtest_database.cpp | 120 ++++++++++++++++++++- dbms/src/Storages/Transaction/TiDB.cpp | 33 +++--- dbms/src/Storages/Transaction/TiDB.h | 24 ++--- 4 files changed, 149 insertions(+), 34 deletions(-) diff --git a/.gitmodules b/.gitmodules index fdfc7b5d3cb..e9f0c9f8aa2 100644 --- a/.gitmodules +++ b/.gitmodules @@ -31,13 +31,13 @@ branch = master [submodule "contrib/client-c"] path = contrib/client-c - url = git@github.com:tikv/client-c.git + url = https://github.com/tikv/client-c.git [submodule "contrib/tiflash-proxy"] path = contrib/tiflash-proxy - url = git@github.com:pingcap/tidb-engine-ext.git + url = https://github.com/pingcap/tidb-engine-ext.git [submodule "contrib/prometheus-cpp"] path = contrib/prometheus-cpp - url = git@github.com:jupp0r/prometheus-cpp.git + url = https://github.com/jupp0r/prometheus-cpp.git [submodule "contrib/junction"] path = contrib/junction url = https://github.com/preshing/junction.git diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index 85ca759372b..56c28359b6d 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -80,7 +80,7 @@ class DatabaseTiFlashTest : public ::testing::Test } } - void recreateMetadataPath() const + static void recreateMetadataPath() { String path = TiFlashTestEnv::getContext().getPath(); @@ -638,6 +638,118 @@ try } CATCH +TEST_F(DatabaseTiFlashTest, ISSUE4596) +try +{ + const String db_name = "db_1"; + auto ctx = TiFlashTestEnv::getContext(); + + { + // Create database + const String statement = "CREATE DATABASE " + db_name + " ENGINE=TiFlash"; + ASTPtr ast = parseCreateStatement(statement); + InterpreterCreateQuery interpreter(ast, ctx); + interpreter.setInternal(true); + interpreter.setForceRestoreData(false); + interpreter.execute(); + } + + auto db = ctx.getDatabase(db_name); + + const String tbl_name = "t_111"; + { + /// Create table + ParserCreateQuery parser; + const String stmt = fmt::format("CREATE TABLE `{}`.`{}` ", db_name, tbl_name) + + R"stmt( + (`id` Int32,`b` String) Engine = DeltaMerge((`id`), + '{ + "cols":[{ + "comment":"", + "default":null, + "default_bit":null, + "id":1, + "name":{ + "L":"id", + "O":"id" + }, + "offset":0, + "origin_default":null, + "state":5, + "type":{ + "Charset":"binary", + "Collate":"binary", + "Decimal":0, + "Elems":null, + "Flag":515, + "Flen":16, + "Tp":3 + } + }, + { + "comment":"", + "default":"", + "default_bit":null, + "id":15, + "name":{ + "L":"b", + "O":"b" + }, + "offset":12, + "origin_default":"", + "state":5, + "type":{ + "Charset":"binary", + "Collate":"binary", + "Decimal":0, + "Elems":null, + "Flag":4225, + "Flen":-1, + "Tp":251 + } + }], + "comment":"", + "id":330, + "index_info":[], + "is_common_handle":false, + "name":{ + "L":"test", + "O":"test" + }, + "partition":null, + "pk_is_handle":true, + "schema_version":465, + "state":5, + "update_timestamp":99999 + }' + ) + )stmt"; + ASTPtr ast = parseQuery(parser, stmt, 0); + + InterpreterCreateQuery interpreter(ast, ctx); + interpreter.setInternal(true); + interpreter.setForceRestoreData(false); + interpreter.execute(); + } + + EXPECT_FALSE(db->empty(ctx)); + EXPECT_TRUE(db->isTableExist(ctx, tbl_name)); + + { + // Get storage from database + auto storage = db->tryGetTable(ctx, tbl_name); + ASSERT_NE(storage, nullptr); + + EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); + EXPECT_EQ(storage->getTableName(), tbl_name); + + auto managed_storage = std::dynamic_pointer_cast(storage); + EXPECT_EQ(managed_storage->getDatabaseName(), db_name); + EXPECT_EQ(managed_storage->getTableInfo().name, "test"); + } +} +CATCH + TEST_F(DatabaseTiFlashTest, ISSUE1055) try { @@ -674,7 +786,7 @@ try DatabaseLoading::loadTable(ctx, *db, meta_path, db_name, db_data_path, "TiFlash", "t_45.sql", false); // Get storage from database - const auto tbl_name = "t_45"; + const auto * tbl_name = "t_45"; auto storage = db->tryGetTable(ctx, tbl_name); ASSERT_NE(storage, nullptr); EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); @@ -762,7 +874,7 @@ try auto db = ctx.getDatabase(name_mapper.mapDatabaseName(*db_info)); ASSERT_NE(db, nullptr); EXPECT_EQ(db->getEngineName(), "TiFlash"); - auto flash_db = typeid_cast(db.get()); + auto * flash_db = typeid_cast(db.get()); auto & db_info_get = flash_db->getDatabaseInfo(); ASSERT_EQ(db_info_get.name, expect_name); } @@ -827,7 +939,7 @@ try )", }; - for (auto & statement : statements) + for (const auto & statement : statements) { { // Cleanup: Drop database if exists diff --git a/dbms/src/Storages/Transaction/TiDB.cpp b/dbms/src/Storages/Transaction/TiDB.cpp index 31d62ebed6f..eb5cfa34218 100644 --- a/dbms/src/Storages/Transaction/TiDB.cpp +++ b/dbms/src/Storages/Transaction/TiDB.cpp @@ -87,7 +87,7 @@ ColumnInfo::ColumnInfo(Poco::JSON::Object::Ptr json) Field ColumnInfo::defaultValueToField() const { - auto & value = origin_default_value; + const auto & value = origin_default_value; if (value.isEmpty()) { if (hasNotNullFlag()) @@ -109,7 +109,7 @@ Field ColumnInfo::defaultValueToField() const // TODO: We shall use something like `orig_default_bit`, which will never change once created, // rather than `default_bit`, which could be altered. // See https://github.com/pingcap/tidb/issues/17641 and https://github.com/pingcap/tidb/issues/17642 - auto & bit_value = default_bit_value; + const auto & bit_value = default_bit_value; // TODO: There might be cases that `orig_default` is not null but `default_bit` is null, // i.e. bit column added with an default value but later modified to another. // For these cases, neither `orig_default` (may get corrupted) nor `default_bit` (modified) is correct. @@ -141,10 +141,13 @@ Field ColumnInfo::defaultValueToField() const auto v = value.convert(); if (hasBinaryFlag()) { - // For binary column, we have to pad trailing zeros according to the specified type length. + // For some binary column(like varchar(20)), we have to pad trailing zeros according to the specified type length. // User may define default value `0x1234` for a `BINARY(4)` column, TiDB stores it in a string "\u12\u34" (sized 2). // But it actually means `0x12340000`. - v.append(flen - v.length(), '\0'); + // And for some binary column(like longblob), we do not need to pad trailing zeros. + // And the `Flen` is set to -1, therefore we need to check `Flen >= 0` here. + if (Int32 vlen = v.length(); flen >= 0 && vlen < flen) + v.append(flen - vlen, '\0'); } return v; } @@ -251,9 +254,9 @@ UInt64 ColumnInfo::getSetValue(const String & set_str) const throw DB::Exception(std::string(__PRETTY_FUNCTION__) + ": can't parse set type value."); } -Int64 ColumnInfo::getTimeValue(const String & time_str) const +Int64 ColumnInfo::getTimeValue(const String & time_str) const // NOLINT { - const static long fractional_seconds_multiplier[] = {1000000000, 100000000, 10000000, 1000000, 100000, 10000, 1000, 100, 10, 1}; + const static Int64 fractional_seconds_multiplier[] = {1000000000, 100000000, 10000000, 1000000, 100000, 10000, 1000, 100, 10, 1}; bool negative = time_str[0] == '-'; Poco::StringTokenizer second_and_fsp(time_str, "."); Poco::StringTokenizer string_tokens(second_and_fsp[0], ":"); @@ -271,7 +274,7 @@ Int64 ColumnInfo::getTimeValue(const String & time_str) const return negative ? -ret : ret; } -Int64 ColumnInfo::getYearValue(const String & val) const +Int64 ColumnInfo::getYearValue(const String & val) const // NOLINT { // do not check validation of the val because TiDB will do it Int64 year = std::stol(val); @@ -284,7 +287,7 @@ Int64 ColumnInfo::getYearValue(const String & val) const return year; } -UInt64 ColumnInfo::getBitValue(const String & val) const +UInt64 ColumnInfo::getBitValue(const String & val) const // NOLINT { // The `default_bit` is a base64 encoded, big endian byte array. Poco::MemoryInputStream istr(val.data(), val.size()); @@ -323,7 +326,7 @@ try if (!elems.empty()) { Poco::JSON::Array::Ptr elem_arr = new Poco::JSON::Array(); - for (auto & elem : elems) + for (const auto & elem : elems) elem_arr->add(elem.first); tp_json->set("Elems", elem_arr); } @@ -459,7 +462,7 @@ try Poco::JSON::Array::Ptr def_arr = new Poco::JSON::Array(); - for (auto & part_def : definitions) + for (const auto & part_def : definitions) { def_arr->add(part_def.getJSONObject()); } @@ -668,7 +671,7 @@ try json->set("tbl_name", tbl_name_json); Poco::JSON::Array::Ptr cols_array = new Poco::JSON::Array(); - for (auto & col : idx_cols) + for (const auto & col : idx_cols) { auto col_obj = col.getJSONObject(); cols_array->add(col_obj); @@ -752,7 +755,7 @@ try json->set("name", name_json); Poco::JSON::Array::Ptr cols_arr = new Poco::JSON::Array(); - for (auto & col_info : columns) + for (const auto & col_info : columns) { auto col_obj = col_info.getJSONObject(); cols_arr->add(col_obj); @@ -760,7 +763,7 @@ try json->set("cols", cols_arr); Poco::JSON::Array::Ptr index_arr = new Poco::JSON::Array(); - for (auto & index_info : index_infos) + for (const auto & index_info : index_infos) { auto index_info_obj = index_info.getJSONObject(); index_arr->add(index_info_obj); @@ -956,7 +959,7 @@ ColumnID TableInfo::getColumnID(const String & name) const String TableInfo::getColumnName(const ColumnID id) const { - for (auto & col : columns) + for (const auto & col : columns) { if (id == col.id) { @@ -989,7 +992,7 @@ std::optional> TableInfo::getPKHandleCo if (!pk_is_handle) return std::nullopt; - for (auto & col : columns) + for (const auto & col : columns) { if (col.hasPriKeyFlag()) return std::optional>(col); diff --git a/dbms/src/Storages/Transaction/TiDB.h b/dbms/src/Storages/Transaction/TiDB.h index 09a7d69c75f..232019c1f0d 100644 --- a/dbms/src/Storages/Transaction/TiDB.h +++ b/dbms/src/Storages/Transaction/TiDB.h @@ -77,7 +77,7 @@ enum TP #ifdef M #error "Please undefine macro M first." #endif -#define M(tt, v, cf, ct, w) Type##tt = v, +#define M(tt, v, cf, ct, w) Type##tt = (v), COLUMN_TYPES(M) #undef M }; @@ -109,7 +109,7 @@ enum ColumnFlag #ifdef M #error "Please undefine macro M first." #endif -#define M(cf, v) ColumnFlag##cf = v, +#define M(cf, v) ColumnFlag##cf = (v), COLUMN_FLAGS(M) #undef M }; @@ -138,7 +138,7 @@ enum CodecFlag #ifdef M #error "Please undefine macro M first." #endif -#define M(cf, v) CodecFlag##cf = v, +#define M(cf, v) CodecFlag##cf = (v), CODEC_FLAGS(M) #undef M }; @@ -183,10 +183,10 @@ struct ColumnInfo #ifdef M #error "Please undefine macro M first." #endif -#define M(f, v) \ - inline bool has##f##Flag() const { return (flag & v) != 0; } \ - inline void set##f##Flag() { flag |= v; } \ - inline void clear##f##Flag() { flag &= (~v); } +#define M(f, v) \ + inline bool has##f##Flag() const { return (flag & (v)) != 0; } \ + inline void set##f##Flag() { flag |= (v); } \ + inline void clear##f##Flag() { flag &= (~(v)); } COLUMN_FLAGS(M) #undef M @@ -211,7 +211,7 @@ struct PartitionDefinition { PartitionDefinition() = default; - PartitionDefinition(Poco::JSON::Object::Ptr json); + explicit PartitionDefinition(Poco::JSON::Object::Ptr json); Poco::JSON::Object::Ptr getJSONObject() const; @@ -227,7 +227,7 @@ struct PartitionInfo { PartitionInfo() = default; - PartitionInfo(Poco::JSON::Object::Ptr json); + explicit PartitionInfo(Poco::JSON::Object::Ptr json); Poco::JSON::Object::Ptr getJSONObject() const; @@ -250,7 +250,7 @@ struct DBInfo SchemaState state; DBInfo() = default; - DBInfo(const String & json) { deserialize(json); } + explicit DBInfo(const String & json) { deserialize(json); } String serialize() const; @@ -357,9 +357,9 @@ struct TableInfo ::TiDB::StorageEngine engine_type = ::TiDB::StorageEngine::UNSPECIFIED; ColumnID getColumnID(const String & name) const; - String getColumnName(const ColumnID id) const; + String getColumnName(ColumnID id) const; - const ColumnInfo & getColumnInfo(const ColumnID id) const; + const ColumnInfo & getColumnInfo(ColumnID id) const; std::optional> getPKHandleColumn() const;