[FLASH-1026] Synchronization between drop table and remove region

[JaySon-Huang]

While running tests/fullstack-test/fault-inject/drop-table.test, tiflash may crash because of the synchronization between drop table and remove region.

Problem

After truncate table test.t ;, SchemaBuilder will create a InterpreterDropQuery to drop table. It call shutdown without lock.
https://github.com/pingcap/tics/blob/master/dbms/src/Interpreters/InterpreterDropQuery.cpp#L126-L129

At the same time, since the table is dropped, the region of that table is removed. RegionTable::removeRegion get a lock on DT table, and try to write a deleteRange on that table.
https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTable.cpp#L247-L273

When storage->deleteRange() call background_task_handle->wake(), which has been reset by shutdown() in InterpreterDropQuery, TiFlash will crash.

Solution

In InterpreterDropQuery, I move storage->shutdown() after it get lock on storage. shutdown is supposed to be "wait till finish or cancel all background task of storage".

[zanmato1984]

I think the fundamental difference, that causes this problem, between TMT and DT, is that DT has to remove corresponding storage data (by writing a delete range(?)) when removing a region whereas TMT does not (TMT removes corresponding data during compaction/merge).

Am I understanding it right? If so, could you explain why DT has to remove storage data instantly?

[JaySon-Huang]

I think the fundamental difference, that causes this problem, between TMT and DT, is that DT has to remove corresponding storage data (by writing a delete range(?)) when removing a region whereas TMT does not (TMT removes corresponding data during compaction/merge).

Am I understanding it right? If so, could you explain why DT has to remove storage data instantly?

Actually DT don't remove storage data instantly, deleteRange is a simple delete mark for the key range. deleteRange indicate that we don't need that data any more, and that range of data can be removed on its background compact(delta-merge).

For TMT, it will do final optimize on storage if all its regions are removed, or background thread to calculate its parts satisfied threshold or not.

• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTable.cpp#L277-L283
• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTableCheckOptimize.cpp#L14-L93

cc @zanmato1984 @flowbehappy

[zanmato1984]

I think the fundamental difference, that causes this problem, between TMT and DT, is that DT has to remove corresponding storage data (by writing a delete range(?)) when removing a region whereas TMT does not (TMT removes corresponding data during compaction/merge).
Am I understanding it right? If so, could you explain why DT has to remove storage data instantly?

Actually DT don't remove storage data instantly, deleteRange is a simple delete mark for the key range. deleteRange indicate that we don't need that data any more, and that range of data can be removed on its background compact(delta-merge).

For TMT, it will do final optimize on storage if all its regions are removed, or background thread to calculate its parts satisfied threshold or not.

• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTable.cpp#L277-L283
• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTableCheckOptimize.cpp#L14-L93

cc @zanmato1984 @flowbehappy

My actual question was, why does DT have to "mark" this range for reclaim, rather than doing it the "tmt" way, which is leaving it to compaction to detect that "this region has been removed, we don't need it anymore"?

[flowbehappy]

I think the fundamental difference, that causes this problem, between TMT and DT, is that DT has to remove corresponding storage data (by writing a delete range(?)) when removing a region whereas TMT does not (TMT removes corresponding data during compaction/merge).
Am I understanding it right? If so, could you explain why DT has to remove storage data instantly?

Actually DT don't remove storage data instantly, deleteRange is a simple delete mark for the key range. deleteRange indicate that we don't need that data any more, and that range of data can be removed on its background compact(delta-merge).
For TMT, it will do final optimize on storage if all its regions are removed, or background thread to calculate its parts satisfied threshold or not.

• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTable.cpp#L277-L283
• https://github.com/pingcap/tics/blob/master/dbms/src/Storages/Transaction/RegionTableCheckOptimize.cpp#L14-L93

cc @zanmato1984 @flowbehappy

My actual question was, why does DT have to "mark" this range for reclaim, rather than doing it the "tmt" way, which is leaving it to compaction to detect that "this region has been removed, we don't need it anymore"?

@zanmato1984
One design goal of DT is try to decouple the logic of storage engine and raft layer. And by providing a delete range api, we can easily avoid the necessary of getting the available ranges of regions from raft layer. BTW, as far as I know, TiVK do the same way.

[zanmato1984]

LGTM

[JaySon-Huang]

/run-integration-tests

[JaySon-Huang]

/run-integration-tests

[JaySon-Huang]

/run-integration-tests

[JaySon-Huang]

/merge

[sre-bot]

/run-all-tests

[sre-bot]

@JaySon-Huang merge failed.

[sre-bot]

/run-all-tests

[sre-bot]

@JaySon-Huang merge failed.

[JaySon-Huang]

/merge

[sre-bot]

/run-all-tests

[sre-bot]

@JaySon-Huang merge failed.

[JaySon-Huang]

/merge

[sre-bot]

/run-all-tests