This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new 3649de590a5 branch-4.2: [fix](cloud) Avoid lock inversion in delete
bitmap checks #69892 (#69947)
3649de590a5 is described below
commit 3649de590a5bcb129e35fe7916fcbb1ad3d5f03e
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Tue Nov 3 09:59:07 2026 +0800
branch-4.2: [fix](cloud) Avoid lock inversion in delete bitmap checks
#69892 (#69947)
Cherry-picked from #69892
Co-authored-by: bobhan1 <[email protected]>
---
be/src/cloud/cloud_tablet_mgr.cpp | 6 +-
be/src/storage/tablet/base_tablet.cpp | 9 ++-
be/test/cloud/cloud_tablet_mgr_test.cpp | 48 ++++++++++++++
be/test/cloud/cloud_tablet_test.cpp | 110 ++++++++++++++++++++++++++++++++
4 files changed, 170 insertions(+), 3 deletions(-)
diff --git a/be/src/cloud/cloud_tablet_mgr.cpp
b/be/src/cloud/cloud_tablet_mgr.cpp
index 04de0d373c0..3e5c7d0b764 100644
--- a/be/src/cloud/cloud_tablet_mgr.cpp
+++ b/be/src/cloud/cloud_tablet_mgr.cpp
@@ -353,7 +353,9 @@ void CloudTabletMgr::vacuum_stale_rowsets(const
CountDownLatch& stop_latch) {
int64_t max_useless_rowset_version_count = 0;
int64_t tablet_id_with_max_useless_rowset_version_count = 0;
OlapStopWatch watch;
- _tablet_map->traverse([&](auto&& tablet) {
+ // Reuse the snapshot outside TabletMap's mutex: a check waiting
on one tablet's
+ // header lock must not block loading or enumerating unrelated
tablets.
+ for (const auto& tablet : tablets_to_remove_unused_rowsets) {
int64_t useless_rowset_count = 0;
int64_t useless_rowset_version_count = 0;
tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowset_count,
@@ -366,7 +368,7 @@ void CloudTabletMgr::vacuum_stale_rowsets(const
CountDownLatch& stop_latch) {
max_useless_rowset_version_count =
useless_rowset_version_count;
tablet_id_with_max_useless_rowset_version_count =
tablet->tablet_id();
}
- });
+ }
g_max_rowsets_with_useless_delete_bitmap.set_value(max_useless_rowset_count);
g_max_rowsets_with_useless_delete_bitmap_version.set_value(
max_useless_rowset_version_count);
diff --git a/be/src/storage/tablet/base_tablet.cpp
b/be/src/storage/tablet/base_tablet.cpp
index fad76e86986..fd54b902374 100644
--- a/be/src/storage/tablet/base_tablet.cpp
+++ b/be/src/storage/tablet/base_tablet.cpp
@@ -2151,6 +2151,13 @@ void
BaseTablet::check_agg_delete_bitmap_for_stale_rowsets(int64_t& useless_rows
},
true);
+ // Compaction holds the header lock while merging the delete bitmap. Read
the schema
+ // before taking the bitmap lock so the traversal cannot acquire these
locks in reverse.
+ const bool has_sequence_col = tablet_schema()->has_sequence_col();
+ TEST_SYNC_POINT_CALLBACK(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ this);
+
std::set<RowsetId> useless_rowsets;
std::map<RowsetId, std::vector<int64_t>> useless_rowset_versions;
{
@@ -2167,7 +2174,7 @@ void
BaseTablet::check_agg_delete_bitmap_for_stale_rowsets(int64_t& useless_rows
return -1;
}
if (end_versions.find(version) == end_versions.end()) {
- if (tablet_schema()->has_sequence_col()) {
+ if (has_sequence_col) {
auto rowset_version = rowset_it->second;
if (version >= rowset_version.first &&
version <= rowset_version.second) {
diff --git a/be/test/cloud/cloud_tablet_mgr_test.cpp
b/be/test/cloud/cloud_tablet_mgr_test.cpp
index dafabfe04ec..2521cbb9e74 100644
--- a/be/test/cloud/cloud_tablet_mgr_test.cpp
+++ b/be/test/cloud/cloud_tablet_mgr_test.cpp
@@ -22,7 +22,9 @@
#include <gtest/gtest.h>
#include <atomic>
+#include <chrono>
#include <condition_variable>
+#include <future>
#include <memory>
#include <mutex>
#include <thread>
@@ -92,6 +94,52 @@ TEST_F(CloudTabletMgrTest,
VacuumReclaimsWarmupStatesWithoutStaleOrUnusedRowsets
EXPECT_EQ(tablet->get_rowset_warmup_state(rowset.rowset_id()).progress,
WarmUpProgress::NONE);
}
+TEST_F(CloudTabletMgrTest, DeleteBitmapCheckDoesNotHoldTabletMapLock) {
+ const bool old_check =
config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap;
+ auto sp = SyncPoint::get_instance();
+ sp->clear_all_call_backs();
+ Defer cleanup([&] {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap =
old_check;
+ });
+ config::enable_check_agg_and_remove_pre_rowsets_delete_bitmap = true;
+
+ CloudTabletMgr mgr(_engine);
+ auto tablet = std::make_shared<CloudTablet>(_engine, _tablet_meta);
+ mgr.put_tablet_for_UT(tablet);
+ TabletMetaSharedPtr other_meta(new TabletMeta(
+ 1, 2, _tablet_meta->tablet_id() + 1, 15674, 4, 5, TTabletSchema(),
6, {{7, 8}},
+ UniqueId(9, 10), TTabletType::TABLET_TYPE_DISK,
TCompressionType::LZ4F));
+ auto other_tablet = std::make_shared<CloudTablet>(_engine, other_meta);
+
+ CountDownLatch check_entered(1);
+ CountDownLatch resume_check(1);
+ sp->set_call_back(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ [&](auto&&) {
+ check_entered.count_down();
+ resume_check.wait();
+ });
+ sp->enable_processing();
+
+ CountDownLatch stop_latch(1);
+ auto vacuum = std::async(std::launch::async, [&] {
mgr.vacuum_stale_rowsets(stop_latch); });
+ Defer unblock_check([&] { resume_check.count_down(); });
+ ASSERT_TRUE(check_entered.wait_for(std::chrono::seconds(10)));
+
+ auto map_access = std::async(std::launch::async, [&] {
+ mgr.put_tablet_for_UT(other_tablet);
+ return mgr.get_weak_tablets().size();
+ });
+ const auto map_status = map_access.wait_for(std::chrono::seconds(10));
+ // Release the check before joining, so the old locking behavior fails
without hanging UT.
+ resume_check.count_down();
+ vacuum.get();
+ EXPECT_EQ(map_status, std::future_status::ready);
+ EXPECT_EQ(map_access.get(), 2);
+}
+
// Test concurrent get_tablet calls for the same tablet_id.
// Reproduces bug where tablet ends up in cache but not in _tablet_map.
//
diff --git a/be/test/cloud/cloud_tablet_test.cpp
b/be/test/cloud/cloud_tablet_test.cpp
index 84ba9fc2c72..a31c8c92879 100644
--- a/be/test/cloud/cloud_tablet_test.cpp
+++ b/be/test/cloud/cloud_tablet_test.cpp
@@ -40,6 +40,7 @@
#include "storage/rowset/rowset_meta.h"
#include "storage/rowset/unique_rowset_id_generator.h"
#include "storage/tablet/tablet_meta.h"
+#include "util/countdown_latch.h"
#include "util/defer_op.h"
#include "util/uid_util.h"
@@ -339,6 +340,115 @@ TEST_F(CloudTabletDeleteBitmapTest,
AggDeleteBitmapForCompactionReturnsPreRowset
EXPECT_EQ(rowset_versions_without_stats, pre_rowset_to_versions);
}
+class CloudTabletDeleteBitmapCheckTest : public CloudTabletWarmUpStateTest,
+ public
testing::WithParamInterface<bool> {
+public:
+ void SetUp() override {
+ CloudTabletWarmUpStateTest::SetUp();
+ TColumn key;
+ key.__set_column_name("k");
+ key.__set_is_key(true);
+ key.column_type.type = TPrimitiveType::BIGINT;
+ TColumn value;
+ value.__set_column_name(GetParam() ? SEQUENCE_COL : "v");
+ value.__set_is_key(false);
+ value.__set_aggregation_type(TAggregationType::REPLACE);
+ value.column_type.type = TPrimitiveType::BIGINT;
+ TTabletSchema schema;
+ schema.__set_short_key_column_count(1);
+ schema.__set_schema_hash(4);
+ schema.__set_keys_type(TKeysType::UNIQUE_KEYS);
+ schema.__set_columns({key, value});
+ schema.__set_sequence_col_idx(GetParam() ? 1 : -1);
+ _tablet_meta.reset(new TabletMeta(1, 2, 15673, 15674, 4, 5, schema, 2,
{{0, 0}, {1, 1}},
+ UniqueId(9, 10),
TTabletType::TABLET_TYPE_DISK,
+ TCompressionType::LZ4F, 0, true));
+ _tablet = std::make_shared<CloudTablet>(_engine, _tablet_meta);
+ }
+};
+
+TEST_P(CloudTabletDeleteBitmapCheckTest,
DoesNotAcquireHeaderLockDuringBitmapTraversal) {
+ auto rowset = create_rowset(Version(2, 4));
+ ASSERT_NE(rowset, nullptr);
+ _tablet->_rs_version_map.emplace(rowset->version(), rowset);
+ auto& delete_bitmap = _tablet->tablet_meta()->delete_bitmap();
+ roaring::Roaring bitmap;
+ bitmap.add(1);
+ // Version 3 is not a rowset end version, so both schema variants exercise
the sequence check.
+ delete_bitmap.set({rowset->rowset_id(), 0, 3}, bitmap);
+
+ auto sp = SyncPoint::get_instance();
+ sp->clear_all_call_backs();
+ Defer cleanup([sp] {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ });
+ CountDownLatch snapshot_ready(1);
+ CountDownLatch resume_check(1);
+ sp->set_call_back(
+
"BaseTablet::check_agg_delete_bitmap_for_stale_rowsets:before_traverse_delete_bitmap",
+ [&](auto&&) {
+ snapshot_ready.count_down();
+ resume_check.wait();
+ });
+ sp->enable_processing();
+
+ int64_t useless_rowsets = 0;
+ int64_t useless_versions = 0;
+ auto check = std::async(std::launch::async, [&] {
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ });
+ Defer unblock_check([&] { resume_check.count_down(); });
+ ASSERT_TRUE(snapshot_ready.wait_for(seconds(10)));
+
+ // Model compaction's exclusive header ownership after the check has
captured its rowsets.
+ std::unique_lock header_lock(_tablet->get_header_lock());
+ resume_check.count_down();
+ const auto check_status = check.wait_for(seconds(10));
+ if (check_status == std::future_status::ready) {
+ // Even an empty compaction bitmap takes the DBM write lock while
holding the header lock.
+ DeleteBitmap empty_bitmap(_tablet->tablet_id());
+ delete_bitmap.merge(empty_bitmap);
+ }
+ // Let the old callback finish after a timeout instead of leaving a
deadlocked test thread.
+ header_lock.unlock();
+ check.get();
+ EXPECT_EQ(check_status, std::future_status::ready);
+ EXPECT_EQ(useless_rowsets, 0);
+ EXPECT_EQ(useless_versions, GetParam() ? 0 : 1);
+}
+
+TEST_P(CloudTabletDeleteBitmapCheckTest,
PreservesActiveAndStaleRowsetVersionChecks) {
+ auto active = create_rowset(Version(2, 4));
+ auto stale = create_rowset(Version(5, 7));
+ auto orphan = create_rowset(Version(8, 8));
+ ASSERT_TRUE(active && stale && orphan);
+ _tablet->_rs_version_map.emplace(active->version(), active);
+ _tablet->_stale_rs_version_map.emplace(stale->version(), stale);
+ auto& delete_bitmap = _tablet->tablet_meta()->delete_bitmap();
+ roaring::Roaring bitmap;
+ bitmap.add(1);
+ delete_bitmap.set({active->rowset_id(), 0, 3}, bitmap);
+ delete_bitmap.set({active->rowset_id(), 0, 4}, bitmap);
+ delete_bitmap.set({stale->rowset_id(), 0, 6}, bitmap);
+ delete_bitmap.set({stale->rowset_id(), 0, 7}, bitmap);
+ delete_bitmap.set({orphan->rowset_id(), 0, 8}, bitmap);
+
+ int64_t useless_rowsets = 0;
+ int64_t useless_versions = 0;
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ EXPECT_EQ(useless_rowsets, 1);
+ EXPECT_EQ(useless_versions, GetParam() ? 0 : 2);
+
+ // A version outside the rowset's range is still useless for a sequence
table.
+ delete_bitmap.set({active->rowset_id(), 0, 1}, bitmap);
+ _tablet->check_agg_delete_bitmap_for_stale_rowsets(useless_rowsets,
useless_versions);
+ EXPECT_EQ(useless_rowsets, 1);
+ EXPECT_EQ(useless_versions, GetParam() ? 1 : 2);
+}
+
+INSTANTIATE_TEST_SUITE_P(SequenceModes, CloudTabletDeleteBitmapCheckTest,
testing::Bool());
+
class CloudTabletDeleteBitmapPrefillTest
: public CloudTabletWarmUpStateTest,
public testing::WithParamInterface<std::tuple<bool, bool>> {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
