Skip to content

Commit 7c9c366

Browse files
authored
[fix](cloud) Normalize SC rowset graph before delete bitmap capture (#63960)
## Proposed changes This PR fixes the remaining MOW schema-change delete-bitmap path after #62256. #62256, whose master commit is `dd59f479af5a855401e3f862c751e8416070a1e2`, fixed the final schema-change commit path by deleting local rowsets in `[2, alter_version]` before adding the schema-change output rowsets to the real new tablet. That keeps the committed tablet rowset graph aligned with the Meta Service result. However, the delete-bitmap recompute path still builds and uses a temporary tablet in `CloudSchemaChangeJob::_process_delete_bitmap()`. That temporary tablet is initialized with the schema-change output rowsets, but after each `sync_tablet_rowsets(tmp_tablet)` it can again contain non-schema-change local rowsets in `[2, alter_version]`, such as double-write rowsets or compaction output rowsets. If the temporary tablet graph contains both: - schema-change output rowsets, for example `[2]`, `[3]`, ... - a wider local/compaction rowset, for example `[2-3]` then `capture_consistent_rowsets()` can choose the wider non-schema-change rowset from the temporary graph instead of the schema-change output rowsets. The delete bitmap is then recomputed against a rowset path that is not the one finally committed for the schema-changed tablet. A later MOW compaction may observe delete-bitmap coverage inconsistent with the visible rowset graph and fail row-count/delete-bitmap correctness checks. The fix is to normalize the temporary tablet rowset graph immediately after every `sync_tablet_rowsets(tmp_tablet)` and before capturing rowsets for delete-bitmap recomputation. Concretely this PR: - extracts `CloudTablet::replace_rowsets_with_schema_change_output()`; - removes non-schema-change local rowsets in `[2, alter_version]` from both `_rs_version_map` and the version graph before adding schema-change output rowsets; - reuses the helper in the real schema-change commit path; - calls the same helper after both tmp-tablet syncs in `_process_delete_bitmap()`; - keeps cache/delete-bitmap cleanup only for the real tablet, while the temporary tablet only normalizes its local graph; - adds a unit test that simulates a polluted tmp graph with `[2]`, `[3]`, and a stale compaction rowset `[2-3]`. ## Root cause #62256 fixed the final commit graph but not the earlier delete-bitmap recompute graph. The final tablet graph and the temporary delete-bitmap tablet graph must use the same schema-change output rowset path for historical versions. Otherwise delete bitmap recomputation may be based on a different rowset path from the one that becomes visible after schema change. This is why the issue can surface in a compaction after schema change has finished: the compaction output itself does not need to contain duplicate rows. The failure comes from delete bitmap state being recomputed from a polluted temporary rowset graph and later being applied to the committed schema-change graph. ## Testing ``` ./run-be-ut.sh --run --filter=CloudTabletDeleteRowsetsForSchemaChangeTest.* -j100 ```
1 parent d073c95 commit 7c9c366

4 files changed

Lines changed: 111 additions & 35 deletions

File tree

be/src/cloud/cloud_schema_change_job.cpp

Lines changed: 12 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -550,29 +550,8 @@ Status CloudSchemaChangeJob::_convert_historical_rowsets(const SchemaChangeParam
550550
// during double write phase by `CloudMetaMgr::sync_tablet_rowsets` in another thread
551551
std::unique_lock lock {_new_tablet->get_sync_meta_lock()};
552552
std::unique_lock wlock(_new_tablet->get_header_lock());
553-
// Mirror MS behavior: delete rowsets in [2, alter_version] before adding
554-
// SC output rowsets to avoid stale compaction rowsets remaining visible.
555-
{
556-
int64_t alter_ver = sc_job->alter_version();
557-
std::vector<RowsetSharedPtr> to_delete;
558-
for (auto& [v, rs] : _new_tablet->rowset_map()) {
559-
if (v.first >= 2 && v.second <= alter_ver) {
560-
to_delete.push_back(rs);
561-
}
562-
}
563-
if (!to_delete.empty()) {
564-
LOG_INFO(
565-
"schema change: delete {} local rowsets in [2, {}] before adding SC "
566-
"output, tablet_id={}, versions=[{}]",
567-
to_delete.size(), alter_ver, _new_tablet->tablet_id(),
568-
fmt::join(to_delete | std::views::transform([](const auto& rs) {
569-
return rs->version().to_string();
570-
}),
571-
", "));
572-
_new_tablet->delete_rowsets_for_schema_change(to_delete, wlock);
573-
}
574-
}
575-
_new_tablet->add_rowsets(std::move(_output_rowsets), true, wlock, false);
553+
_new_tablet->replace_rowsets_with_schema_change_output(
554+
_output_rowsets, sc_job->alter_version(), wlock, "commit", true);
576555
// Ensure the real new tablet has a continuous local version graph before it becomes
577556
// visible. Later RUNNING-tablet delete bitmap sync depends on capturing all old versions.
578557
RETURN_IF_ERROR(_cloud_storage_engine.meta_mgr().fill_version_holes(
@@ -619,6 +598,11 @@ Status CloudSchemaChangeJob::_process_delete_bitmap(int64_t alter_version,
619598

620599
// step 1, process incremental rowset without delete bitmap update lock
621600
RETURN_IF_ERROR(_cloud_storage_engine.meta_mgr().sync_tablet_rowsets(tmp_tablet.get()));
601+
{
602+
std::unique_lock wlock(tmp_tablet->get_header_lock());
603+
tmp_tablet->replace_rowsets_with_schema_change_output(_output_rowsets, alter_version, wlock,
604+
"delete_bitmap_without_lock", false);
605+
}
622606
int64_t max_version = tmp_tablet->max_version().second;
623607
LOG(INFO) << "alter table for mow table, calculate delete bitmap of "
624608
<< "incremental rowsets without lock, version: " << start_calc_delete_bitmap_version
@@ -628,10 +612,6 @@ Status CloudSchemaChangeJob::_process_delete_bitmap(int64_t alter_version,
628612
{start_calc_delete_bitmap_version, max_version}, CaptureRowsetOps {}));
629613
DBUG_EXECUTE_IF("CloudSchemaChangeJob::_process_delete_bitmap.after.capture_without_lock",
630614
DBUG_BLOCK);
631-
{
632-
std::unique_lock wlock(tmp_tablet->get_header_lock());
633-
tmp_tablet->add_rowsets(_output_rowsets, true, wlock, false);
634-
}
635615
for (auto rowset : ret.rowsets) {
636616
RETURN_IF_ERROR(CloudTablet::update_delete_bitmap_without_lock(tmp_tablet, rowset));
637617
}
@@ -644,17 +624,18 @@ Status CloudSchemaChangeJob::_process_delete_bitmap(int64_t alter_version,
644624
RETURN_IF_ERROR(_cloud_storage_engine.meta_mgr().get_delete_bitmap_update_lock(
645625
*_new_tablet, SCHEMA_CHANGE_DELETE_BITMAP_LOCK_ID, initiator));
646626
RETURN_IF_ERROR(_cloud_storage_engine.meta_mgr().sync_tablet_rowsets(tmp_tablet.get()));
627+
{
628+
std::unique_lock wlock(tmp_tablet->get_header_lock());
629+
tmp_tablet->replace_rowsets_with_schema_change_output(_output_rowsets, alter_version, wlock,
630+
"delete_bitmap_with_lock", false);
631+
}
647632
int64_t new_max_version = tmp_tablet->max_version().second;
648633
LOG(INFO) << "alter table for mow table, calculate delete bitmap of "
649634
<< "incremental rowsets with lock, version: " << max_version + 1 << "-"
650635
<< new_max_version << " new_tablet_id: " << _new_tablet->tablet_id();
651636
if (new_max_version > max_version) {
652637
auto ret = DORIS_TRY(tmp_tablet->capture_consistent_rowsets_unlocked(
653638
{max_version + 1, new_max_version}, CaptureRowsetOps {}));
654-
{
655-
std::unique_lock wlock(tmp_tablet->get_header_lock());
656-
tmp_tablet->add_rowsets(_output_rowsets, true, wlock, false);
657-
}
658639
for (auto rowset : ret.rowsets) {
659640
RETURN_IF_ERROR(CloudTablet::update_delete_bitmap_without_lock(tmp_tablet, rowset));
660641
}

be/src/cloud/cloud_tablet.cpp

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,17 @@ bvar::Window<bvar::Adder<int64_t>> g_capture_with_freshness_tolerance_fallback_c
9393

9494
static constexpr int LOAD_INITIATOR_ID = -1;
9595

96+
namespace {
97+
98+
bool is_schema_change_output_rowset(const RowsetSharedPtr& rowset,
99+
const std::vector<RowsetSharedPtr>& output_rowsets) {
100+
return std::ranges::any_of(output_rowsets, [&rowset](const RowsetSharedPtr& output_rowset) {
101+
return output_rowset->rowset_id() == rowset->rowset_id();
102+
});
103+
}
104+
105+
} // namespace
106+
96107
bvar::Adder<uint64_t> g_file_cache_cloud_tablet_submitted_segment_size(
97108
"file_cache_cloud_tablet_submitted_segment_size");
98109
bvar::Adder<uint64_t> g_file_cache_cloud_tablet_submitted_segment_num(
@@ -491,7 +502,8 @@ void CloudTablet::delete_rowsets(const std::vector<RowsetSharedPtr>& to_delete,
491502
}
492503

493504
void CloudTablet::delete_rowsets_for_schema_change(const std::vector<RowsetSharedPtr>& to_delete,
494-
std::unique_lock<std::shared_mutex>&) {
505+
std::unique_lock<std::shared_mutex>&,
506+
bool recycle_deleted_rowsets) {
495507
if (to_delete.empty()) {
496508
return;
497509
}
@@ -514,8 +526,35 @@ void CloudTablet::delete_rowsets_for_schema_change(const std::vector<RowsetShare
514526
// the other hits a DCHECK(false) in delete_expired_stale_rowsets().
515527
_tablet_meta->modify_rs_metas({}, rs_metas, true);
516528

517-
// Schedule for direct cache cleanup. MS has already recycled these rowsets.
518-
add_unused_rowsets(to_delete);
529+
if (recycle_deleted_rowsets) {
530+
// Schedule for direct cache cleanup. MS has already recycled these rowsets.
531+
add_unused_rowsets(to_delete);
532+
}
533+
}
534+
535+
void CloudTablet::replace_rowsets_with_schema_change_output(
536+
const std::vector<RowsetSharedPtr>& output_rowsets, int64_t alter_version,
537+
std::unique_lock<std::shared_mutex>& meta_lock, const char* stage,
538+
bool recycle_deleted_rowsets) {
539+
std::vector<RowsetSharedPtr> to_delete;
540+
for (auto& [v, rs] : _rs_version_map) {
541+
if (v.first >= 2 && v.second <= alter_version &&
542+
!is_schema_change_output_rowset(rs, output_rowsets)) {
543+
to_delete.push_back(rs);
544+
}
545+
}
546+
if (!to_delete.empty()) {
547+
LOG_INFO(
548+
"schema change: delete {} local rowsets in [2, {}] before adding SC output, "
549+
"tablet_id={}, stage={}, versions=[{}]",
550+
to_delete.size(), alter_version, tablet_id(), stage,
551+
fmt::join(to_delete | std::views::transform([](const auto& rs) {
552+
return rs->version().to_string();
553+
}),
554+
", "));
555+
delete_rowsets_for_schema_change(to_delete, meta_lock, recycle_deleted_rowsets);
556+
}
557+
add_rowsets(output_rowsets, true, meta_lock, false);
519558
}
520559

521560
uint64_t CloudTablet::delete_expired_stale_rowsets() {

be/src/cloud/cloud_tablet.h

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,19 @@ class CloudTablet final : public BaseTablet {
164164
// preferring stale compaction rowsets over individual SC output rowsets.
165165
// MUST hold EXCLUSIVE `_meta_lock`.
166166
void delete_rowsets_for_schema_change(const std::vector<RowsetSharedPtr>& to_delete,
167-
std::unique_lock<std::shared_mutex>& meta_lock);
167+
std::unique_lock<std::shared_mutex>& meta_lock,
168+
bool recycle_deleted_rowsets = true);
169+
170+
// Replace local rowsets in [2, alter_version] with schema change output rowsets.
171+
// Existing SC output rowsets are kept; other local/double-write/compaction rowsets
172+
// in this version range are removed from both _rs_version_map and version graph.
173+
// recycle_deleted_rowsets should only be true for the real tablet; temporary
174+
// schema-change delete-bitmap tablets only need to normalize their local graph.
175+
// MUST hold EXCLUSIVE `_meta_lock`.
176+
void replace_rowsets_with_schema_change_output(
177+
const std::vector<RowsetSharedPtr>& output_rowsets, int64_t alter_version,
178+
std::unique_lock<std::shared_mutex>& meta_lock, const char* stage,
179+
bool recycle_deleted_rowsets);
168180

169181
// When the tablet is dropped, we need to recycle cached data:
170182
// 1. The data in file cache

be/test/cloud/cloud_tablet_test.cpp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1469,6 +1469,50 @@ TEST_F(CloudTabletDeleteRowsetsForSchemaChangeTest, TestSchemaChangeDeletesCompa
14691469
}
14701470
}
14711471

1472+
TEST_F(CloudTabletDeleteRowsetsForSchemaChangeTest,
1473+
TestReplaceSchemaChangeOutputCleansPollutedTmpGraph) {
1474+
auto rs_placeholder = create_rowset(Version(0, 1));
1475+
auto rs_sc_2 = create_rowset(Version(2, 2));
1476+
auto rs_sc_3 = create_rowset(Version(3, 3));
1477+
auto rs_compacted = create_rowset(Version(2, 3));
1478+
auto rs_post_alter = create_rowset(Version(4, 4));
1479+
ASSERT_NE(rs_placeholder, nullptr);
1480+
ASSERT_NE(rs_sc_2, nullptr);
1481+
ASSERT_NE(rs_sc_3, nullptr);
1482+
ASSERT_NE(rs_compacted, nullptr);
1483+
ASSERT_NE(rs_post_alter, nullptr);
1484+
1485+
{
1486+
std::unique_lock wlock(_tablet->get_header_lock());
1487+
_tablet->add_rowsets({rs_placeholder, rs_sc_2, rs_sc_3, rs_compacted, rs_post_alter}, false,
1488+
wlock, false);
1489+
}
1490+
ASSERT_TRUE(_tablet->rowset_map().count(Version(2, 2)));
1491+
ASSERT_TRUE(_tablet->rowset_map().count(Version(3, 3)));
1492+
ASSERT_TRUE(_tablet->rowset_map().count(Version(2, 3)));
1493+
1494+
{
1495+
std::unique_lock wlock(_tablet->get_header_lock());
1496+
_tablet->replace_rowsets_with_schema_change_output({rs_sc_2, rs_sc_3}, 3, wlock, "test",
1497+
false);
1498+
}
1499+
1500+
ASSERT_TRUE(_tablet->rowset_map().count(Version(2, 2)));
1501+
ASSERT_TRUE(_tablet->rowset_map().count(Version(3, 3)));
1502+
ASSERT_FALSE(_tablet->rowset_map().count(Version(2, 3)));
1503+
ASSERT_TRUE(_tablet->rowset_map().count(Version(4, 4)));
1504+
ASSERT_FALSE(_tablet->need_remove_unused_rowsets());
1505+
1506+
auto versions_result = _tablet->capture_consistent_versions_unlocked(Version(0, 4), {});
1507+
ASSERT_TRUE(versions_result.has_value()) << versions_result.error();
1508+
auto& versions = versions_result.value();
1509+
ASSERT_EQ(versions.size(), 4);
1510+
ASSERT_EQ(versions[0], Version(0, 1));
1511+
ASSERT_EQ(versions[1], Version(2, 2));
1512+
ASSERT_EQ(versions[2], Version(3, 3));
1513+
ASSERT_EQ(versions[3], Version(4, 4));
1514+
}
1515+
14721516
// Test that delete_rowsets_for_schema_change with empty input is a no-op
14731517
TEST_F(CloudTabletDeleteRowsetsForSchemaChangeTest, TestEmptyDeleteIsNoop) {
14741518
auto rs = create_rowset(Version(0, 1));

0 commit comments

Comments
 (0)