ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

JaySon-Huang · 2024-07-12T08:41:44Z

What problem does this PR solve?

Issue Number: close #9227

Problem Summary: #9227 (comment)
In SchemaBuilder::syncAllSchema, each table that does not exists in tikv but still in the tiflash instance. TiFlash will execute applyDropPhysicalTable for each table, wich will update the "tombstone_ts" of the table in its ".sql" file.
Even if the table is already "tombstone", the function will update the "tombstone_ts" by a newer tso from PD. This make the IStorage instance still exist even after the gc_safepoint is exceed than the drop timepoint + gc_lifetime.

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 1437 to 1451 in 28a2314

    
           /// Drop all unmapped tables. 
        
           auto storage_map = tmt_context.getStorages().getAllStorage(); 
        
           for (auto it = storage_map.begin(); it != storage_map.end(); it++) 
        
           { 
        
               auto table_info = it->second->getTableInfo(); 
        
               if (table_info.keyspace_id != keyspace_id) 
        
               { 
        
                   continue; 
        
               } 
        
               if (table_set.count(table_info.id) == 0) 
        
               { 
        
                   applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id); 
        
                   LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info)); 
        
               } 
        
           }

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 1240 to 1268 in 28a2314

    
           template <typename Getter, typename NameMapper> 
        
           void SchemaBuilder<Getter, NameMapper>::applyDropPhysicalTable(const String & db_name, TableID table_id) 
        
           { 
        
               auto & tmt_context = context.getTMTContext(); 
        
               auto storage = tmt_context.getStorages().get(keyspace_id, table_id); 
        
               if (storage == nullptr) 
        
               { 
        
                   LOG_DEBUG(log, "table {} does not exist.", table_id); 
        
                   return; 
        
               } 
        
               GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); 
        
               LOG_INFO(log, "Tombstoning table {}.{}", db_name, name_mapper.debugTableName(storage->getTableInfo())); 
        
               const UInt64 tombstone_ts = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime); 
        
               AlterCommands commands; 
        
               { 
        
                   AlterCommand command; 
        
                   command.type = AlterCommand::TOMBSTONE; 
        
                   // We don't try to get a precise time that TiDB drops this table. 
        
                   // We use a more relaxing GC strategy: 
        
                   // 1. Use current timestamp, which is after TiDB's drop time, to be the tombstone of this table; 
        
                   // 2. Use the same GC safe point as TiDB. 
        
                   // In such way our table will be GC-ed later than TiDB, which is safe and correct. 
        
                   command.tombstone = tombstone_ts; 
        
                   commands.emplace_back(std::move(command)); 
        
               } 
        
               auto alter_lock = storage->lockForAlter(getThreadNameAndID()); 
        
               storage->alterFromTiDB(alter_lock, commands, db_name, storage->getTableInfo(), name_mapper, context); 
        
               LOG_INFO(log, "Tombstoned table {}.{}, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); 
        
           }

What is changed and how it works?

Skip updating the tombstone_ts if the table is already tombstone. So even if the "syncAllSchema" is called repeatedly, the dropped IStorage instance can be physically removed from TiFlash instances correctly. And the time spend on "syncAllSchema" won't become longer and longer.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
Run the following test scripts. It will make tiflash run into sync all schema frequently.

>  cat test.sh
set -x

for (( i=0 ; i<1000 ; i++ )); do
echo "running with i=${i}"
mycli -h 10.2.12.81 -P 8042 -u root -D test -e 'CREATE TABLE if not exists test_range_1 (id INT,name VARCHAR(20)) PARTITION BY RANGE (id) (PARTITION p0 VALUES LESS THAN (10),PARTITION p1 VALUES LESS THAN (20),PARTITION p2 VALUES LESS THAN (30),PARTITION p3 VALUES LESS THAN (40),PARTITION p4 VALUES LESS THAN (50));'

mycli -h 10.2.12.81 -P 8042 -u root -D test -e 'CREATE TABLE if not exists test_swap_1 (id INT,name VARCHAR(20));'
mycli -h 10.2.12.81 -P 8042 -u root -D test -e 'ALTER TABLE test_range_1 EXCHANGE PARTITION p0 WITH TABLE test_swap_1;'
mycli -h 10.2.12.81 -P 8042 -u root -D test -e 'DROP TABLE test_swap_1;'
done

In the v6.5.10 version, after that script ran,

## At the beginning, sync all schema taks about 38ms
[2024/09/02 14:53:02.454 +08:00] [INFO] [TiDBSchemaSyncer.h:124] ["Start to sync schemas. current version is: 589 and try to sync schema version to: 600"] [thread_id=85]
[2024/09/02 14:53:02.455 +08:00] [DEBUG] [TiDBSchemaSyncer.h:181] ["Try load schema diffs."] [thread_id=85]
[2024/09/02 14:53:02.457 +08:00] [DEBUG] [TiDBSchemaSyncer.h:192] ["End load schema diffs with total 11 entries."] [thread_id=85]
[2024/09/02 14:53:02.460 +08:00] [WARN] [TiDBSchemaSyncer.h:236] ["apply diff meets exception : DB::TiFlashException: miss partition table in TiKV, may have been dropped, physical_table_id=446 \n stack is \n       0x1779921\tDB::TiFlashException::TiFlashException(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, DB::TiFlashError const&) [tiflash+24615201]\n                \tdbms/src/Common/TiFlashException.h:250\n       0x6cac991\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyExchangeTablePartition(DB::SchemaDiff const&) [tiflash+113953169]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:908\n       0x6ca9123\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyDiff(DB::SchemaDiff const&) [tiflash+113938723]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:551\n       0x6bf165b\tDB::TiDBSchemaSyncer<false, false>::syncSchemas(DB::Context&) [tiflash+113186395]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:139\n       0x6cf1ff4\tstd::__1::__function::__func<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0, std::__1::allocator<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0>, bool ()>::operator()() [tiflash+114237428]\n                \t/usr/local/bin/../include/c++/v1/__functional/function.h:345\n       0x68e7fbc\tvoid* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, DB::BackgroundProcessingPool::BackgroundProcessingPool(int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)::$_1> >(void*) [tiflash+110002108]\n                \t/usr/local/bin/../include/c++/v1/thread:291\n  0x7f415ae7d802\tstart_thread [libc.so.6+653314]\n  0x7f415ae1d450\tclone3 [libc.so.6+259152]"] [thread_id=85]
[2024/09/02 14:53:02.492 +08:00] [INFO] [TiDBSchemaSyncer.h:146] ["End sync schema, version has been updated to 600"] [thread_id=85]

## the last sync all schema takes about 300ms (to update about 429 tables)
238043:[2024/09/02 15:29:05.265 +08:00] [INFO] [TiDBSchemaSyncer.h:124] ["Start to sync schemas. current version is: 6576 and try to sync schema version to: 6606"] [thread_id=84]
238044:[2024/09/02 15:29:05.265 +08:00] [DEBUG] [TiDBSchemaSyncer.h:181] ["Try load schema diffs."] [thread_id=84]
238045:[2024/09/02 15:29:05.270 +08:00] [DEBUG] [TiDBSchemaSyncer.h:192] ["End load schema diffs with total 30 entries."] [thread_id=84]
238065:[2024/09/02 15:29:05.274 +08:00] [WARN] [TiDBSchemaSyncer.h:236] ["apply diff meets exception : DB::TiFlashException: miss table in TiFlash, npt_table_id=4444 : test(2).test_range_1(83) \n stack is \n       0x1779921\tDB::TiFlashException::TiFlashException(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, DB::TiFlashError const&) [tiflash+24615201]\n                \tdbms/src/Common/TiFlashException.h:250\n       0x6cac822\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyExchangeTablePartition(DB::SchemaDiff const&) [tiflash+113952802]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:880\n       0x6ca9123\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyDiff(DB::SchemaDiff const&) [tiflash+113938723]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:551\n       0x6bf165b\tDB::TiDBSchemaSyncer<false, false>::syncSchemas(DB::Context&) [tiflash+113186395]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:139\n       0x6cf1ff4\tstd::__1::__function::__func<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0, std::__1::allocator<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0>, bool ()>::operator()() [tiflash+114237428]\n                \t/usr/local/bin/../include/c++/v1/__functional/function.h:345\n       0x68e7fbc\tvoid* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, DB::BackgroundProcessingPool::BackgroundProcessingPool(int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)::$_1> >(void*) [tiflash+110002108]\n                \t/usr/local/bin/../include/c++/v1/thread:291\n  0x7f415ae7d802\tstart_thread [libc.so.6+653314]\n  0x7f415ae1d450\tclone3 [libc.so.6+259152]"] [thread_id=84]
239987:[2024/09/02 15:29:05.565 +08:00] [INFO] [TiDBSchemaSyncer.h:146] ["End sync schema, version has been updated to 6606"] [thread_id=84]

## And the same table_id is being tombstone again and again
>  rg 'Tombstoned table db_2.t_94\(94\)' clusters/tiflash-5040/log/tiflash.log                                                                                                                                                                                                                                                                        2024-09-02 16:51:01
1309:[2024/09/02 14:46:29.519 +08:00] [INFO] [SchemaBuilder.cpp:1298] ["Tombstoned table db_2.t_94(94), tombstone=452266449829888006"] [thread_id=91]
1570:[2024/09/02 14:46:40.143 +08:00] [INFO] [SchemaBuilder.cpp:1298] ["Tombstoned table db_2.t_94(94), tombstone=452266452608614402"] [thread_id=92]
...
236355:[2024/09/02 15:28:54.154 +08:00] [INFO] [SchemaBuilder.cpp:1298] ["Tombstoned table db_2.t_94(94), tombstone=452267116894617606"] [thread_id=81]
238316:[2024/09/02 15:29:05.296 +08:00] [INFO] [SchemaBuilder.cpp:1298] ["Tombstoned table db_2.t_94(94), tombstone=452267119804416006"] [thread_id=84]

## At the beginning, sync all schema taks about 32ms
[2024/09/02 16:47:01.052 +08:00] [INFO] [TiDBSchemaSyncer.h:124] ["Start to sync schemas. current version is: 6626 and try to sync schema version to: 6658"] [thread_id=83]
[2024/09/02 16:47:01.052 +08:00] [DEBUG] [TiDBSchemaSyncer.h:181] ["Try load schema diffs."] [thread_id=83]
[2024/09/02 16:47:01.058 +08:00] [DEBUG] [TiDBSchemaSyncer.h:192] ["End load schema diffs with total 32 entries."] [thread_id=83]
[2024/09/02 16:47:01.061 +08:00] [WARN] [TiDBSchemaSyncer.h:236] ["apply diff meets exception : DB::TiFlashException: miss partition table in TiKV, may have been dropped, physical_table_id=4472 \n stack is \n       0x1752003\tStackTrace::StackTrace() [tiflash+24453123]\n                \tdbms/src/Common/StackTrace.cpp:23\n       0x177a67b\tDB::TiFlashException::TiFlashException(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, DB::TiFlashError const&) [tiflash+24618619]\n                \tdbms/src/Common/TiFlashException.h:250\n       0x6c7aaa1\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyExchangeTablePartition(DB::SchemaDiff const&) [tiflash+113748641]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:923\n       0x6c7790f\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyDiff(DB::SchemaDiff const&) [tiflash+113735951]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:551\n       0x6bb98fb\tDB::TiDBSchemaSyncer<false, false>::tryLoadSchemaDiffs(DB::SchemaGetter&, long, DB::Context&) [tiflash+112957691]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:227\n       0x6bb8a85\tDB::TiDBSchemaSyncer<false, false>::syncSchemas(DB::Context&) [tiflash+112953989]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:139\n       0x6cb1a3e\tstd::__1::__function::__func<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0, std::__1::allocator<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0>, bool ()>::operator()() [tiflash+113973822]\n                \t/DATA/disk1/ra_common/tiflash-env-13/sysroot/bin/../include/c++/v1/__functional/function.h:345\n       0x6895d5f\tDB::BackgroundProcessingPool::threadFunction(unsigned long) [tiflash+109665631]\n                \tdbms/src/Storages/BackgroundProcessingPool.cpp:226\n       0x6896655\tvoid* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, DB::BackgroundProcessingPool::BackgroundProcessingPool(int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)::$_1> >(void*) [tiflash+109667925]\n                \t/DATA/disk1/ra_common/tiflash-env-13/sysroot/bin/../include/c++/v1/thread:291\n  0x7f3b92a52802\tstart_thread [libc.so.6+653314]\n  0x7f3b929f2450\tclone3 [libc.so.6+259152]"] [thread_id=83]
[2024/09/02 16:47:01.084 +08:00] [INFO] [TiDBSchemaSyncer.h:146] ["End sync schema, version has been updated to 6658"] [thread_id=83]

## the last sync all schema takes about 38ms
[2024/09/02 17:22:10.555 +08:00] [INFO] [TiDBSchemaSyncer.h:124] ["Start to sync schemas. current version is: 12585 and try to sync schema version to: 12606"] [thread_id=95]
[2024/09/02 17:22:10.555 +08:00] [DEBUG] [TiDBSchemaSyncer.h:181] ["Try load schema diffs."] [thread_id=95]
[2024/09/02 17:22:10.560 +08:00] [DEBUG] [TiDBSchemaSyncer.h:192] ["End load schema diffs with total 21 entries."] [thread_id=95]
[2024/09/02 17:22:10.566 +08:00] [WARN] [TiDBSchemaSyncer.h:236] ["apply diff meets exception : DB::TiFlashException: miss table in TiFlash, npt_table_id=8452 : test(2).test_range_1(83) \n stack is \n       0x1752003\tStackTrace::StackTrace() [tiflash+24453123]\n                \tdbms/src/Common/StackTrace.cpp:23\n       0x177a67b\tDB::TiFlashException::TiFlashException(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, DB::TiFlashError const&) [tiflash+24618619]\n                \tdbms/src/Common/TiFlashException.h:250\n       0x6c7a9a3\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyExchangeTablePartition(DB::SchemaDiff const&) [tiflash+113748387]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:895\n       0x6c7790f\tDB::SchemaBuilder<DB::SchemaGetter, DB::SchemaNameMapper>::applyDiff(DB::SchemaDiff const&) [tiflash+113735951]\n                \tdbms/src/TiDB/Schema/SchemaBuilder.cpp:551\n       0x6bb98fb\tDB::TiDBSchemaSyncer<false, false>::tryLoadSchemaDiffs(DB::SchemaGetter&, long, DB::Context&) [tiflash+112957691]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:227\n       0x6bb8a85\tDB::TiDBSchemaSyncer<false, false>::syncSchemas(DB::Context&) [tiflash+112953989]\n                \tdbms/src/TiDB/Schema/TiDBSchemaSyncer.h:139\n       0x6cb1a3e\tstd::__1::__function::__func<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0, std::__1::allocator<DB::SchemaSyncService::SchemaSyncService(DB::Context&)::$_0>, bool ()>::operator()() [tiflash+113973822]\n                \t/DATA/disk1/ra_common/tiflash-env-13/sysroot/bin/../include/c++/v1/__functional/function.h:345\n       0x6895d5f\tDB::BackgroundProcessingPool::threadFunction(unsigned long) [tiflash+109665631]\n                \tdbms/src/Storages/BackgroundProcessingPool.cpp:226\n       0x6896655\tvoid* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, DB::BackgroundProcessingPool::BackgroundProcessingPool(int, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >)::$_1> >(void*) [tiflash+109667925]\n                \t/DATA/disk1/ra_common/tiflash-env-13/sysroot/bin/../include/c++/v1/thread:291\n  0x7f3b92a52802\tstart_thread [libc.so.6+653314]\n  0x7f3b929f2450\tclone3 [libc.so.6+259152]"] [thread_id=95]
[2024/09/02 17:22:10.593 +08:00] [INFO] [TiDBSchemaSyncer.h:146] ["End sync schema, version has been updated to 12606"] [thread_id=95]

## The same table_id only be dropped once
[2024/09/02 16:46:50.264 +08:00] [INFO] [SchemaBuilder.cpp:1343] ["Tombstone table db_2.t_4460(4460) begin, action=SyncAllSchema"] [thread_id=86]
[2024/09/02 16:46:50.264 +08:00] [INFO] [StorageDeltaMerge.cpp:115] ["updateTableColumnInfo: TableName t_4460 ordinary columns format version: 1\n3 columns:\n`id` Nullable(Int32)\n`name` Nullable(String)\n`_tidb_rowid` Int64\n materialized columns format version: 1\n2 columns:\n`_INTERNAL_VERSION` UInt64\n`_INTERNAL_DELMARK` UInt8\n"] [source=db_2.t_4460] [thr
ead_id=86]
[2024/09/02 16:46:50.265 +08:00] [INFO] [SchemaBuilder.cpp:1359] ["Tombstone table db_2.t_4460(4460) end, action=SyncAllSchema tombstone=452268342706176001"] [thread_id=86]
[2024/09/02 16:46:50.265 +08:00] [INFO] [SchemaBuilder.cpp:1542] ["Table db_2.t_4460(4460) dropped during sync all schemas"] [thread_id=86]
[2024/09/02 16:47:01.084 +08:00] [INFO] [SchemaBuilder.cpp:1337] ["Tombstone table db_2.t_4460(4460) has been done before, action=SyncAllSchema tombstone=452268342706176001"] [thread_id=83]
[2024/09/02 16:47:01.084 +08:00] [INFO] [SchemaBuilder.cpp:1542] ["Table db_2.t_4460(4460) dropped during sync all schemas"] [thread_id=83]
[2024/09/02 16:47:12.040 +08:00] [INFO] [SchemaBuilder.cpp:1337] ["Tombstone table db_2.t_4460(4460) has been done before, action=SyncAllSchema tombstone=452268342706176001"] [thread_id=84]
...

No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Fix an issue that rapidly executes `DROP TABLE` after other DDL operations on the same table for a long time, could potentially lead to a significant slowdown in TiFlash schema synchronization.

JaySon-Huang · 2024-07-15T05:16:12Z

/run-all-tests

Signed-off-by: JaySon-Huang <[email protected]>

JaySon-Huang · 2024-07-15T06:30:36Z

/run-all-tests

JaySon-Huang · 2024-08-28T14:11:22Z

/run-all-tests

JaySon-Huang · 2024-08-30T10:03:55Z

/run-all-tests

JaySon-Huang · 2024-09-02T10:06:18Z

/run-all-tests

Lloyd-Pottiger · 2024-09-02T12:27:55Z

dbms/src/TiDB/Schema/SchemaBuilder.cpp

+void SchemaBuilder<Getter, NameMapper>::applyRenameTable(const DBInfoPtr & new_db_info, TableID table_id, std::string_view action)
 {


Maybe use enum instead of string_view?

No. I think string_view is better than enum. Because something I attach more info than just a enum name. For example, attaching the table_id

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 682 to 683 in 5b9d758

/// Apply changes to physical tables.

auto reason = fmt::format("ApplyPartitionDiff-logical_table_id={}", orig_table_info.id);

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 1241 to 1245 in 5b9d758

// If "CREATE DATABASE" is executed in TiFlash after user has executed "DROP DATABASE"

// in TiDB, then TiFlash may not create the IDatabase instance. Make sure we can access

// to the IDatabase when creating IStorage.

const auto database_mapped_name = name_mapper.mapDatabaseName(*db_info);

ensureLocalDatabaseExist(db_info->id, database_mapped_name, fmt::format("CreatePhysicalTable-table_id={}", table_info->id));

CalvinNeo

lgtm

ti-chi-bot · 2024-09-03T03:12:56Z

[LGTM Timeline notifier]

Timeline:

2024-09-03 02:55:45.884038192 +0000 UTC m=+326670.402091117: ☑️ agreed by CalvinNeo.
2024-09-03 03:12:56.134052212 +0000 UTC m=+327700.652105137: ☑️ agreed by Lloyd-Pottiger.

JaySon-Huang · 2024-09-03T03:14:12Z

/run-all-tests

CalvinNeo

lgtm

ti-chi-bot · 2024-09-03T03:15:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, JinheLin, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CalvinNeo,JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/cherry-pick-not-approved labels Jul 12, 2024

JaySon-Huang marked this pull request as draft July 12, 2024 08:41

JaySon-Huang changed the title ~~ddl: Skip updating the tombstone_ts if the table is already tombstone~~ ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) Jul 12, 2024

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2024

JaySon-Huang force-pushed the tombstone_repeat_65 branch 2 times, most recently from c4b74a9 to a0fc643 Compare July 14, 2024 09:50

JaySon-Huang added 4 commits July 15, 2024 14:17

Add ut

17d0130

Skip updating the tombstone_ts if the table is already tombstone

6d21ee1

Signed-off-by: JaySon-Huang <[email protected]>

Refine logging

2ab5131

Signed-off-by: JaySon-Huang <[email protected]>

Drop table schema diff should update the tombstone

6ce0236

JaySon-Huang force-pushed the tombstone_repeat_65 branch from 9bca9b1 to 6ce0236 Compare July 15, 2024 06:18

JaySon-Huang marked this pull request as ready for review July 15, 2024 06:40

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2024

ti-chi-bot bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Aug 26, 2024

Merge branch 'release-6.5' into tombstone_repeat_65

5ba24d1

JaySon-Huang added 2 commits August 30, 2024 17:08

Resolve conflict

759cbfd

Add comments

5212e81

JaySon-Huang requested review from CalvinNeo and JinheLin September 2, 2024 10:06

JaySon-Huang requested a review from Lloyd-Pottiger September 2, 2024 10:06

Merge branch 'release-6.5' into tombstone_repeat_65

2f6a6f2

JaySon-Huang self-assigned this Sep 2, 2024

Lloyd-Pottiger reviewed Sep 2, 2024

View reviewed changes

CalvinNeo approved these changes Sep 3, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 3, 2024

Align with master

5b9d758

Lloyd-Pottiger approved these changes Sep 3, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 3, 2024

CalvinNeo approved these changes Sep 3, 2024

View reviewed changes

JinheLin approved these changes Sep 3, 2024

View reviewed changes

ti-chi-bot bot merged commit a80b6c7 into pingcap:release-6.5 Sep 3, 2024
5 checks passed

JaySon-Huang deleted the tombstone_repeat_65 branch September 3, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

JaySon-Huang commented Jul 12, 2024 •

edited

Loading

JaySon-Huang commented Jul 15, 2024

JaySon-Huang commented Jul 15, 2024

JaySon-Huang commented Aug 28, 2024

JaySon-Huang commented Aug 30, 2024

JaySon-Huang commented Sep 2, 2024

Lloyd-Pottiger Sep 2, 2024

JaySon-Huang Sep 3, 2024

CalvinNeo left a comment

ti-chi-bot bot commented Sep 3, 2024

JaySon-Huang commented Sep 3, 2024

CalvinNeo left a comment

ti-chi-bot bot commented Sep 3, 2024

	/// Drop all unmapped tables.
	auto storage_map = tmt_context.getStorages().getAllStorage();
	for (auto it = storage_map.begin(); it != storage_map.end(); it++)
	{
	auto table_info = it->second->getTableInfo();
	if (table_info.keyspace_id != keyspace_id)
	{
	continue;
	}
	if (table_set.count(table_info.id) == 0)
	{
	applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id);
	LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info));
	}
	}

	template <typename Getter, typename NameMapper>
	void SchemaBuilder<Getter, NameMapper>::applyDropPhysicalTable(const String & db_name, TableID table_id)
	{
	auto & tmt_context = context.getTMTContext();
	auto storage = tmt_context.getStorages().get(keyspace_id, table_id);
	if (storage == nullptr)
	{
	LOG_DEBUG(log, "table {} does not exist.", table_id);
	return;
	}
	GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment();
	LOG_INFO(log, "Tombstoning table {}.{}", db_name, name_mapper.debugTableName(storage->getTableInfo()));
	const UInt64 tombstone_ts = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime);
	AlterCommands commands;
	{
	AlterCommand command;
	command.type = AlterCommand::TOMBSTONE;
	// We don't try to get a precise time that TiDB drops this table.
	// We use a more relaxing GC strategy:
	// 1. Use current timestamp, which is after TiDB's drop time, to be the tombstone of this table;
	// 2. Use the same GC safe point as TiDB.
	// In such way our table will be GC-ed later than TiDB, which is safe and correct.
	command.tombstone = tombstone_ts;
	commands.emplace_back(std::move(command));
	}
	auto alter_lock = storage->lockForAlter(getThreadNameAndID());
	storage->alterFromTiDB(alter_lock, commands, db_name, storage->getTableInfo(), name_mapper, context);
	LOG_INFO(log, "Tombstoned table {}.{}, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts);
	}

	/// Apply changes to physical tables.
	auto reason = fmt::format("ApplyPartitionDiff-logical_table_id={}", orig_table_info.id);

	// If "CREATE DATABASE" is executed in TiFlash after user has executed "DROP DATABASE"
	// in TiDB, then TiFlash may not create the IDatabase instance. Make sure we can access
	// to the IDatabase when creating IStorage.
	const auto database_mapped_name = name_mapper.mapDatabaseName(*db_info);
	ensureLocalDatabaseExist(db_info->id, database_mapped_name, fmt::format("CreatePhysicalTable-table_id={}", table_info->id));

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

Conversation

JaySon-Huang commented Jul 12, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

JaySon-Huang commented Jul 15, 2024

JaySon-Huang commented Jul 15, 2024

JaySon-Huang commented Aug 28, 2024

JaySon-Huang commented Aug 30, 2024

JaySon-Huang commented Sep 2, 2024

Lloyd-Pottiger Sep 2, 2024

Choose a reason for hiding this comment

JaySon-Huang Sep 3, 2024

Choose a reason for hiding this comment

CalvinNeo left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 3, 2024

[LGTM Timeline notifier]

JaySon-Huang commented Sep 3, 2024

CalvinNeo left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 3, 2024

JaySon-Huang commented Jul 12, 2024 •

edited

Loading