PageStorage: Fix unexpected dmfile removed after shutdown (cherry-pick #6558)

[Lloyd-Pottiger]

What problem does this PR solve?

Issue Number: close #6486

Problem Summary:
From the log file, we can see that (almost all) dmfile are removed after "Release DeltaMerge Store end". This should not happen because we will check the path_pool_weak_ref is valid or not in the remover callback

tiflash/dbms/src/Storages/DeltaMerge/DeltaMergeStore_InternalBg.cpp

Lines 70 to 77 in 5bd0e89

	callbacks.remover = [path_pool_weak_ref = std::weak_ptr<StoragePathPool>(path_pool), //
	file_provider = global_context.getFileProvider(),
	logger = log](const ExternalPageCallbacks::PathAndIdsVec & path_and_ids_vec, const std::set<PageId> & valid_ids) {
	// If the StoragePathPool is invalid, meaning we call `remover` after dropping the table,
	// simply skip is OK.
	auto path_pool = path_pool_weak_ref.lock();
	if (!path_pool)
	return;

However, it do happen :(
Maybe there are some background tasks that is being handled and keep a shared_ptr of path_pool or delta-merge-store so the detect is false positive passed.

I can reproduce the similar problem by a unit test

tiflash/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp

Lines 136 to 149 in eac71b9

	const size_t num_before_shutdown = get_num_stable_files();
	ASSERT_GT(num_before_shutdown, 0);
	// shutdown the table in the middle of page storage gc, but not dropped
	store->shutdown();
	const size_t num_after_shutdown = get_num_stable_files();
	ASSERT_EQ(num_before_shutdown, num_after_shutdown);
	sp_gc.next(); // continue the page storage gc
	th_gc.get();
	const size_t num_after_gc = get_num_stable_files();
	ASSERT_EQ(num_before_shutdown, num_after_gc);

[2022/12/13 21:38:02.332 +08:00] [INFO] [BaseDaemon.cpp:1300] ["Received termination signal (Terminated)"] [source=Application] [thread_id=86]
[2022/12/13 21:38:02.332 +08:00] [INFO] [Server.cpp:1342] ["Set store context status Stopping"] [thread_id=1]
[2022/12/13 21:38:02.336 +08:00] [INFO] [Server.cpp:1351] ["Set store context status Terminated"] [thread_id=1]
[2022/12/13 21:38:03.501 +08:00] [DEBUG] [Server.cpp:742] ["Received termination signal."] [thread_id=1]
...
[2022/12/13 21:38:07.398 +08:00] [INFO] [Server.cpp:1235] ["Shutting down storages."] [thread_id=1]
...
[2022/12/13 21:38:07.417 +08:00] [INFO] [RegionTable.cpp:200] ["remove table 93 in RegionTable success"] [thread_id=1]
[2022/12/13 21:38:07.417 +08:00] [INFO] [RegionTable.cpp:200] ["remove table 95 in RegionTable success"] [thread_id=1]
[2022/12/13 21:38:07.417 +08:00] [INFO] [DeltaMergeStore.cpp:289] ["Release DeltaMerge Store start"] [source="table_id=95"] [thread_id=1]
[2022/12/13 21:38:07.417 +08:00] [INFO] [DeltaMergeStore.cpp:293] ["Release DeltaMerge Store end"] [source="table_id=95"] [thread_id=1]
[2022/12/13 21:38:07.422 +08:00] [INFO] [DeltaMergeStore.cpp:289] ["Release DeltaMerge Store start"] [source="table_id=93"] [thread_id=1]
[2022/12/13 21:38:07.422 +08:00] [INFO] [DeltaMergeStore.cpp:293] ["Release DeltaMerge Store end"] [source="table_id=93"] [thread_id=1]
[2022/12/13 21:38:07.659 +08:00] [INFO] [RateLimiter.cpp:715] ["limiter 0 write 0 read 0 NOT need to tune."] [source=IOLimitTuner] [thread_id=48]
[2022/12/13 21:38:08.756 +08:00] [DEBUG] [GCManager.cpp:46] ["Start GC with table id: 4"] [source=GCManager] [thread_id=60]
[2022/12/13 21:38:08.756 +08:00] [DEBUG] [GCManager.cpp:101] ["End GC and next gc will start with table id: 1064300"] [source=GCManager] [thread_id=60]
[2022/12/13 21:38:09.314 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_51058"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:09.329 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_51172"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:09.342 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_51197"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:09.357 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_51385"] [source="table_id=93"] [thread_id=66]
...
[2022/12/13 21:38:23.547 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62619"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:23.564 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62629"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:23.577 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62634"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:23.586 +08:00] [DEBUG] [Server.cpp:1241] ["Shutted down storages."] [thread_id=1]
[2022/12/13 21:38:23.589 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62635"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:23.601 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62636"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:38:23.614 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_62637"] [source="table_id=93"] [thread_id=66]
...
[2022/12/13 21:39:32.363 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_95897"] [source="table_id=93"] [thread_id=66]
[2022/12/13 21:39:32.375 +08:00] [INFO] [DeltaMergeStore_InternalBg.cpp:126] ["GC removed useless DM file, dmfile=/data2/hongyunyan/tiflash-41350/data/data/t_93/stable/.del.dmf_95901"] [source="table_id=93"] [thread_id=66]
# next restart
[2022/12/13 21:39:36.453 +08:00] [INFO] [<unknown>] ["Welcome to TiFlash"] [thread_id=1]
[2022/12/13 21:39:36.453 +08:00] [INFO] [<unknown>] ["Starting daemon with revision 54381"] [thread_id=1]
[2022/12/13 21:39:36.453 +08:00] [INFO] [<unknown>] ["TiFlash build info: TiFlash\nRelease Version: v6.5.0-alpha-67-gcaef4c4-dirty\nEdition:         Community\nGit Commit Hash: caef4c48e918d02315f9d342754d5862bea0bb62\nGit Branch:      master\nUTC Build Time:  2022-12-12 09:58:30\nEnable Features: jemalloc sm4(GmSSL) avx2 avx512 unwind\nProfile:         RELEASE\n"] [thread_id=1]

What is changed and how it works?

• Shutdown the global_storage_pool explicitly inside ContextShared::shutdown
• Add an atomic flag shutdown_called in StoragePathPool
• Call StoragePathPool::shutdown before removing the callbacks from global page storage
• Check whether the StoragePathPool is shutdown or not inside the callbacks to ensure the safety of removing dmfiles
• Use functor class LocalDMFileGcScanner/LocalDMFileGcRemover instead of lambda functions to make sure the callbacks won't contain unexpected shared_ptrs
• Make sure the remover callback will NOT be called if the ns_id is invalid in PageDirectory::getAliveExternalIds
  • Caller still need to make sure scanner is safe to be called

Some refactor:

• Merge StoragePool::dataRegisterExternalPagesCallbacks, enableGC into StoragePool::startup
• Merge StoragePool::dataUnregisterExternalPagesCallbacks into StoragePool::shutdown to ensure the orders inside shutdown

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

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

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Fix a bug that may cause data lost after restart

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• JaySon-Huang
• flowbehappy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[Lloyd-Pottiger]

/run-all-tests

[JaySon-Huang]

lgtm

[JaySon-Huang]

/run-all-tests

[JaySon-Huang]

/run-integration-test tidb=release-6.5

[JaySon-Huang]

/merge

[ti-chi-bot]

@JaySon-Huang: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: c463b50

[JaySon-Huang]

/run-integration-test tidb=release-6.5

[JaySon-Huang]

/check-required-labels

[JaySon-Huang]

/check-issue-triage-complete

[JaySon-Huang]

/check-issue-triage-complete

[JaySon-Huang]

/check-issue-triage-complete

[wuhuizuo]

/label skip-issue-check

[ti-chi-bot]

@wuhuizuo: The label(s) skip-issue-check cannot be applied. These labels are supported: tide/merge-method-rebase, tide/merge-method-squash, tide/merge-method-merge, duplicate, first-time-contributor, good first issue, help wanted, invalid, question, wontfix, make-local-great-again, needs-cherry-pick-release-3.1, needs-cherry-pick-release-4.0, needs-cherry-pick-release-5.0-rc, needs-cherry-pick-release-5.0, needs-cherry-pick-release-5.1, needs-cherry-pick-release-5.2, needs-cherry-pick-release-5.3, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.0, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.2, needs-cherry-pick-release-6.3, needs-cherry-pick-release-6.4, needs-cherry-pick-release-6.5, needs-cherry-pick-release-6.6, needs-rebase.

In response to this:

/label skip-issue-check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[wuhuizuo]

/skip-issue-check