From 1b236236c9bbedc911cfe9d2a814112668b11b30 Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Tue, 7 Jan 2025 12:50:28 +0800 Subject: [PATCH] KVStore: Fix unreleased Region instance (#9763) close pingcap/tiflash#9722 Signed-off-by: Calvin Neo Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> --- dbms/src/Common/TiFlashMetrics.h | 5 ++ dbms/src/Storages/KVStore/KVStore.cpp | 3 +- dbms/src/Storages/KVStore/KVStore.h | 3 - .../KVStore/MultiRaft/RaftCommandsKVS.cpp | 4 +- .../Storages/KVStore/MultiRaft/RegionData.cpp | 10 ++- .../Storages/KVStore/MultiRaft/RegionData.h | 1 + .../KVStore/MultiRaft/RegionRangeKeys.cpp | 71 +++++++++++++++++++ .../KVStore/MultiRaft/RegionRangeKeys.h | 2 + .../KVStore/MultiRaft/RegionSerde.cpp | 3 +- .../KVStore/MultiRaft/RegionState.cpp | 49 ------------- .../Storages/KVStore/MultiRaft/RegionState.h | 1 - dbms/src/Storages/KVStore/Region.cpp | 6 +- .../TiKVHelpers/DecodedLockCFValue.cpp | 7 ++ .../KVStore/TiKVHelpers/DecodedLockCFValue.h | 1 + .../Storages/KVStore/tests/gtest_kvstore.cpp | 2 +- .../KVStore/tests/gtest_new_kvstore.cpp | 5 +- 16 files changed, 109 insertions(+), 64 deletions(-) create mode 100644 dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.cpp diff --git a/dbms/src/Common/TiFlashMetrics.h b/dbms/src/Common/TiFlashMetrics.h index f1c6c664068..69205fe4261 100644 --- a/dbms/src/Common/TiFlashMetrics.h +++ b/dbms/src/Common/TiFlashMetrics.h @@ -576,6 +576,11 @@ static_assert(RAFT_REGION_BIG_WRITE_THRES * 4 < RAFT_REGION_BIG_WRITE_MAX, "Inva F(type_tikv_server_issue, {{"type", "tikv_server_issue"}}), \ F(type_tikv_lock, {{"type", "tikv_lock"}}), \ F(type_other, {{"type", "other"}})) \ + M(tiflash_raft_classes_count, \ + "Raft classes counter", \ + Gauge, \ + F(type_region, {{"type", "region"}}), \ + F(type_fully_decoded_lockcf, {{"type", "fully_decoded_lockcf"}})) \ /* required by DBaaS */ \ M(tiflash_server_info, \ "Indicate the tiflash server info, and the value is the start timestamp (s).", \ diff --git a/dbms/src/Storages/KVStore/KVStore.cpp b/dbms/src/Storages/KVStore/KVStore.cpp index 0ae98d6e362..4344bec5044 100644 --- a/dbms/src/Storages/KVStore/KVStore.cpp +++ b/dbms/src/Storages/KVStore/KVStore.cpp @@ -57,7 +57,6 @@ KVStore::KVStore(Context & context) : region_persister( context.getSharedContextDisagg()->isDisaggregatedComputeMode() ? nullptr : std::make_unique(context)) - , raft_cmd_res(std::make_unique()) , log(Logger::get()) , region_compact_log_min_rows(40 * 1024) , region_compact_log_min_bytes(32 * 1024 * 1024) @@ -363,7 +362,7 @@ void KVStore::handleDestroy(UInt64 region_id, TMTContext & tmt, const KVStoreTas LOG_INFO(log, "region_id={} not found, might be removed already", region_id); return; } - LOG_INFO(log, "Handle destroy {}", region->toString()); + LOG_INFO(log, "Handle destroy {}, refCount {}", region->toString(), region.use_count()); region->setPendingRemove(); removeRegion( region_id, diff --git a/dbms/src/Storages/KVStore/KVStore.h b/dbms/src/Storages/KVStore/KVStore.h index d18d2848613..953b95047b6 100644 --- a/dbms/src/Storages/KVStore/KVStore.h +++ b/dbms/src/Storages/KVStore/KVStore.h @@ -379,9 +379,6 @@ class KVStore final : private boost::noncopyable mutable std::mutex task_mutex; - // raft_cmd_res stores the result of applying raft cmd. It must be protected by task_mutex. - std::unique_ptr raft_cmd_res; - LoggerPtr log; std::atomic region_compact_log_min_rows; diff --git a/dbms/src/Storages/KVStore/MultiRaft/RaftCommandsKVS.cpp b/dbms/src/Storages/KVStore/MultiRaft/RaftCommandsKVS.cpp index ae9598cc271..9e1031c7e63 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RaftCommandsKVS.cpp +++ b/dbms/src/Storages/KVStore/MultiRaft/RaftCommandsKVS.cpp @@ -216,9 +216,9 @@ EngineStoreApplyRes KVStore::handleAdminRaftCmd( curr_region.orphanKeysInfo().advanceAppliedIndex(index); } + RaftCommandResult result; curr_region.makeRaftCommandDelegate(task_lock) - .handleAdminRaftCmd(request, response, index, term, *this, region_table, *raft_cmd_res); - RaftCommandResult & result = *raft_cmd_res; + .handleAdminRaftCmd(request, response, index, term, *this, region_table, result); // After region split / merge, try to flush it const auto try_to_flush_region = [&tmt](const RegionPtr & region) { diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionData.cpp b/dbms/src/Storages/KVStore/MultiRaft/RegionData.cpp index 7831dee1ab0..96ad9fa68ad 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionData.cpp +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionData.cpp @@ -310,7 +310,8 @@ void RegionData::deserialize(ReadBuffer & buf, RegionData & region_data) total_size += RegionWriteCFData::deserialize(buf, region_data.write_cf); total_size += RegionLockCFData::deserialize(buf, region_data.lock_cf); - region_data.cf_data_size += total_size; + region_data.cf_data_size = total_size; + reportAlloc(total_size); } RegionWriteCFData & RegionData::writeCF() @@ -348,6 +349,13 @@ RegionData::RegionData(RegionData && data) , cf_data_size(data.cf_data_size.load()) {} + +RegionData::~RegionData() +{ + reportDealloc(cf_data_size); + cf_data_size = 0; +} + RegionData & RegionData::operator=(RegionData && rhs) { write_cf = std::move(rhs.write_cf); diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionData.h b/dbms/src/Storages/KVStore/MultiRaft/RegionData.h index a066e149851..ec68df3f82b 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionData.h +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionData.h @@ -81,6 +81,7 @@ class RegionData const RegionLockCFData & lockCF() const; RegionData() = default; + ~RegionData(); RegionData(RegionData && data); RegionData & operator=(RegionData &&); diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.cpp b/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.cpp new file mode 100644 index 00000000000..357b7312dfa --- /dev/null +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.cpp @@ -0,0 +1,71 @@ +// Copyright 2025 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +namespace DB +{ +RegionRangeKeys::RegionRangeKeys(TiKVKey && start_key, TiKVKey && end_key) + : ori(RegionRangeKeys::makeComparableKeys(std::move(start_key), std::move(end_key))) + , raw(std::make_shared( + ori.first.key.empty() ? DecodedTiKVKey() : RecordKVFormat::decodeTiKVKey(ori.first.key)), + std::make_shared( + ori.second.key.empty() ? DecodedTiKVKey() : RecordKVFormat::decodeTiKVKey(ori.second.key))) +{ + keyspace_id = raw.first->getKeyspaceID(); + if (!computeMappedTableID(*raw.first, mapped_table_id) || ori.first.compare(ori.second) >= 0) + { + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Illegal region range, should not happen, start_key={} end_key={}", + ori.first.key.toDebugString(), + ori.second.key.toDebugString()); + } +} + +TableID RegionRangeKeys::getMappedTableID() const +{ + return mapped_table_id; +} + +KeyspaceID RegionRangeKeys::getKeyspaceID() const +{ + return keyspace_id; +} + +const std::pair & RegionRangeKeys::rawKeys() const +{ + return raw; +} + +const RegionRangeKeys::RegionRange & RegionRangeKeys::comparableKeys() const +{ + return ori; +} + +RegionRangeKeys::RegionRange RegionRangeKeys::makeComparableKeys(TiKVKey && start_key, TiKVKey && end_key) +{ + return std::make_pair( + TiKVRangeKey::makeTiKVRangeKey(std::move(start_key)), + TiKVRangeKey::makeTiKVRangeKey(std::move(end_key))); +} + +RegionRangeKeys::RegionRange RegionRangeKeys::cloneRange(const RegionRange & from) +{ + return std::make_pair(from.first.copy(), from.second.copy()); +} + + +} // namespace DB diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.h b/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.h index 31e3a0f95c9..8c8beac1f33 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.h +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionRangeKeys.h @@ -88,4 +88,6 @@ class RegionRangeKeys : boost::noncopyable KeyspaceID keyspace_id = NullspaceID; }; +bool computeMappedTableID(const DecodedTiKVKey & key, TableID & table_id); + } // namespace DB diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionSerde.cpp b/dbms/src/Storages/KVStore/MultiRaft/RegionSerde.cpp index d0ca2587e62..9c19ee1d65a 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionSerde.cpp +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionSerde.cpp @@ -158,7 +158,6 @@ RegionPtr Region::deserializeImpl( // deserialize data RegionData::deserialize(buf, region->data); - region->data.reportAlloc(region->data.cf_data_size); // restore other var according to meta region->last_restart_log_applied = region->appliedIndex(); @@ -166,4 +165,4 @@ RegionPtr Region::deserializeImpl( return region; } -} // namespace DB \ No newline at end of file +} // namespace DB diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionState.cpp b/dbms/src/Storages/KVStore/MultiRaft/RegionState.cpp index 4f7cb4619d0..2e99cee93a4 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionState.cpp +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionState.cpp @@ -136,38 +136,6 @@ bool computeMappedTableID(const DecodedTiKVKey & key, TableID & table_id) return false; } -RegionRangeKeys::RegionRangeKeys(TiKVKey && start_key, TiKVKey && end_key) - : ori(RegionRangeKeys::makeComparableKeys(std::move(start_key), std::move(end_key))) - , raw(std::make_shared( - ori.first.key.empty() ? DecodedTiKVKey() : RecordKVFormat::decodeTiKVKey(ori.first.key)), - std::make_shared( - ori.second.key.empty() ? DecodedTiKVKey() : RecordKVFormat::decodeTiKVKey(ori.second.key))) -{ - keyspace_id = raw.first->getKeyspaceID(); - if (!computeMappedTableID(*raw.first, mapped_table_id) || ori.first.compare(ori.second) >= 0) - { - throw Exception( - ErrorCodes::LOGICAL_ERROR, - "Illegal region range, should not happen, start_key={} end_key={}", - ori.first.key.toDebugString(), - ori.second.key.toDebugString()); - } -} - -TableID RegionRangeKeys::getMappedTableID() const -{ - return mapped_table_id; -} - -KeyspaceID RegionRangeKeys::getKeyspaceID() const -{ - return keyspace_id; -} - -const std::pair & RegionRangeKeys::rawKeys() const -{ - return raw; -} HandleRange getHandleRangeByTable( const std::pair & rawKeys, @@ -176,11 +144,6 @@ HandleRange getHandleRangeByTable( return TiKVRange::getHandleRangeByTable(*rawKeys.first, *rawKeys.second, table_id); } -const RegionRangeKeys::RegionRange & RegionRangeKeys::comparableKeys() const -{ - return ori; -} - template TiKVRangeKey TiKVRangeKey::makeTiKVRangeKey(TiKVKey && key) { @@ -191,18 +154,6 @@ TiKVRangeKey TiKVRangeKey::makeTiKVRangeKey(TiKVKey && key) template TiKVRangeKey TiKVRangeKey::makeTiKVRangeKey(TiKVKey &&); template TiKVRangeKey TiKVRangeKey::makeTiKVRangeKey(TiKVKey &&); -RegionRangeKeys::RegionRange RegionRangeKeys::makeComparableKeys(TiKVKey && start_key, TiKVKey && end_key) -{ - return std::make_pair( - TiKVRangeKey::makeTiKVRangeKey(std::move(start_key)), - TiKVRangeKey::makeTiKVRangeKey(std::move(end_key))); -} - -RegionRangeKeys::RegionRange RegionRangeKeys::cloneRange(const RegionRange & from) -{ - return std::make_pair(from.first.copy(), from.second.copy()); -} - std::string TiKVRangeKey::toDebugString() const { if (this->state == MAX) diff --git a/dbms/src/Storages/KVStore/MultiRaft/RegionState.h b/dbms/src/Storages/KVStore/MultiRaft/RegionState.h index 9fde120ef86..1e6c06c9909 100644 --- a/dbms/src/Storages/KVStore/MultiRaft/RegionState.h +++ b/dbms/src/Storages/KVStore/MultiRaft/RegionState.h @@ -66,6 +66,5 @@ class RegionState : private boost::noncopyable HandleRange getHandleRangeByTable( const std::pair & rawKeys, TableID table_id); -bool computeMappedTableID(const DecodedTiKVKey & key, TableID & table_id); } // namespace DB diff --git a/dbms/src/Storages/KVStore/Region.cpp b/dbms/src/Storages/KVStore/Region.cpp index 42d6d885885..0d27195f2a1 100644 --- a/dbms/src/Storages/KVStore/Region.cpp +++ b/dbms/src/Storages/KVStore/Region.cpp @@ -340,11 +340,13 @@ Region::Region(DB::RegionMeta && meta_, const TiFlashRaftProxyHelper * proxy_hel , keyspace_id(meta.getRange()->getKeyspaceID()) , mapped_table_id(meta.getRange()->getMappedTableID()) , proxy_helper(proxy_helper_) -{} +{ + GET_METRIC(tiflash_raft_classes_count, type_region).Increment(1); +} Region::~Region() { - data.reportDealloc(data.cf_data_size); + GET_METRIC(tiflash_raft_classes_count, type_region).Decrement(); } TableID Region::getMappedTableID() const diff --git a/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.cpp b/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.cpp index fdb10662b65..abb4da11afb 100644 --- a/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.cpp +++ b/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.cpp @@ -164,10 +164,17 @@ DecodedLockCFValue::DecodedLockCFValue(std::shared_ptr key_, std: if (parsed->generation == 0) { // It is not a large txn, we cache the parsed lock. + GET_METRIC(tiflash_raft_classes_count, type_fully_decoded_lockcf).Increment(1); inner = std::move(parsed); } } +DecodedLockCFValue::~DecodedLockCFValue() +{ + if (inner != nullptr) + GET_METRIC(tiflash_raft_classes_count, type_fully_decoded_lockcf).Decrement(1); +} + void DecodedLockCFValue::withInner(std::function && f) const { if likely (inner != nullptr) diff --git a/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.h b/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.h index 61991bd8613..508f7a45081 100644 --- a/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.h +++ b/dbms/src/Storages/KVStore/TiKVHelpers/DecodedLockCFValue.h @@ -45,6 +45,7 @@ struct DecodedLockCFValue : boost::noncopyable void intoLockInfo(const std::shared_ptr & key, kvrpcpb::LockInfo &) const; }; DecodedLockCFValue(std::shared_ptr key_, std::shared_ptr val_); + ~DecodedLockCFValue(); std::unique_ptr intoLockInfo() const; void intoLockInfo(kvrpcpb::LockInfo &) const; bool isLargeTxn() const; diff --git a/dbms/src/Storages/KVStore/tests/gtest_kvstore.cpp b/dbms/src/Storages/KVStore/tests/gtest_kvstore.cpp index 53d6e7a0e46..0b8ebddc627 100644 --- a/dbms/src/Storages/KVStore/tests/gtest_kvstore.cpp +++ b/dbms/src/Storages/KVStore/tests/gtest_kvstore.cpp @@ -356,7 +356,7 @@ static void testRaftSplit(KVStore & kvs, TMTContext & tmt, std::unique_ptrgetMeta().getMetaRegion().region_epoch(); - const auto & ori_source_range = source_region->getRange()->comparableKeys(); + auto ori_source_range = RegionRangeKeys::cloneRange(source_region->getRange()->comparableKeys()); RegionRangeKeys::RegionRange new_source_range = RegionRangeKeys::makeComparableKeys( // RecordKVFormat::genKey(table_id, 5), RecordKVFormat::genKey(table_id, 10)); diff --git a/dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp b/dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp index e9b126df194..b9ac4e1eba5 100644 --- a/dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp +++ b/dbms/src/Storages/KVStore/tests/gtest_new_kvstore.cpp @@ -123,8 +123,11 @@ try RegionPtr region = tests::makeRegion(702, start, end, proxy_helper.get()); region->insert("default", TiKVKey::copyFrom(str_key), TiKVValue::copyFrom(str_val_default)); ASSERT_EQ(root_of_kvstore_mem_trackers->get(), str_key.dataSize() + str_val_default.size()); + // 702 is not registed, so we persist as 1. tryPersistRegion(kvs, 1); - reloadKVSFromDisk(); + root_of_kvstore_mem_trackers->reset(); + ASSERT_EQ(root_of_kvstore_mem_trackers->get(), 0); + reloadKVSFromDisk(false); ASSERT_EQ(root_of_kvstore_mem_trackers->get(), str_key.dataSize() + str_val_default.size()); } ASSERT_EQ(root_of_kvstore_mem_trackers->get(), 0);