vector: Fix building vector index on DeltaVS may lead to delta compact failure by JaySon-Huang · Pull Request #10311 · pingcap/tiflash

JaySon-Huang · 2025-07-14T12:40:08Z

What problem does this PR solve?

Issue Number: close #10310, ref #9600

Problem Summary: Introduced by #9546 in the vector update workload

In DeltaMergeStore::segmentEnsureDeltaLocalIndex, the function will update the persisted ColumnFiles in DeltaVS.

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

Lines 1126 to 1136 in b153a8f

    
           // Update the column files in the delta with the new column files. 
        
           for (auto & column_file : delta_persisted_column_files) 
        
           { 
        
               const auto * tiny_file = column_file->tryToTinyFile(); 
        
               if (!tiny_file) 
        
                   continue; 
        
               if (auto iter = new_column_files_map.find(tiny_file->getDataPageId()); iter != new_column_files_map.end()) 
        
                   column_file = iter->second; 
        
           } 
        
           delta_persisted_file_set->updatePersistedColumnFilesAfterAddingIndex(delta_persisted_column_files, wbs);

In ColumnFilePersistedSet::installCompactionResults it will check whether the persisted ColumnFiles are changed during the minor compaction. If the persisted ColumnFiles not matched, it will throw an exception as the issue describe.

tiflash/dbms/src/Storages/DeltaMerge/Delta/ColumnFilePersistedSet.cpp

Lines 326 to 352 in b153a8f

    
           auto old_persisted_files_iter = persisted_files.begin(); 
        
           for (const auto & task : compaction->getTasks()) 
        
           { 
        
               for (const auto & file : task.to_compact) 
        
               { 
        
                   if (unlikely( 
        
                           old_persisted_files_iter == persisted_files.end() 
        
                           || (file->getId() != (*old_persisted_files_iter)->getId()) 
        
                           || (file->getRows() != (*old_persisted_files_iter)->getRows()))) 
        
                   { 
        
                       throw Exception( 
        
                           ErrorCodes::LOGICAL_ERROR, 
        
                           "Compaction algorithm broken, " 
        
                           "compaction={{{}}} persisted_files={} " 
        
                           "old_persisted_files_iter.is_end={} " 
        
                           "file->getId={} old_persist_files->getId={} file->getRows={} old_persist_files->getRows={}", 
        
                           compaction->info(), 
        
                           detailInfo(), 
        
                           old_persisted_files_iter == persisted_files.end(), 
        
                           file->getId(), 
        
                           old_persisted_files_iter == persisted_files.end() ? -1 : (*old_persisted_files_iter)->getId(), 
        
                           file->getRows(), 
        
                           old_persisted_files_iter == persisted_files.end() ? -1 : (*old_persisted_files_iter)->getRows()); 
        
                   } 
        
                   old_persisted_files_iter++; 
        
               } 
        
           }

So the issue is cause by local index on the DeltaVS changed the persisted ColumnFiles between Minor Compaction.

What is changed and how it works?

* In `DeltaMergeStore::segmentEnsureDeltaLocalIndexAsync` and `DeltaMergeStore::segmentWaitDeltaLocalIndexReady` if we detect that `segment->getDelta()->isUpdating()`, it means the segment is running an update task (SegmentMergeDelta/SegmentMerge/SegmentSplit), just skip the local index build job
* `ColumnFilePersistedSet::installCompactionResults` if we detect the column file set is changed during minor compaction, then log an error and rollback the minor compaction
* `WriteBatches::rollbackWrittenLogAndData` will also remove the pending `removed_log` and `removed_data`

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

Release note

None

Signed-off-by: JaySon-Huang <tshent@qq.com>

ti-chi-bot · 2025-08-18T13:40:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JinheLin, Lloyd-Pottiger

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-08-18T13:40:48Z

[LGTM Timeline notifier]

Timeline:

2025-08-18 02:24:05.722709873 +0000 UTC m=+233853.665885379: ☑️ agreed by Lloyd-Pottiger.
2025-08-18 13:40:47.527121486 +0000 UTC m=+274455.470297012: ☑️ agreed by JinheLin.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-08-20T08:04:39Z

In response to a cherrypick label: new pull request created to branch release-8.5: #10353.
But this PR has conflicts, please resolve them!

…t failure (#10311) (#10353) ref #9600, close #10310 * In `DeltaMergeStore::segmentEnsureDeltaLocalIndexAsync` and `DeltaMergeStore::segmentWaitDeltaLocalIndexReady` if we detect that `segment->getDelta()->isUpdating()`, it means the segment is running an update task (SegmentMergeDelta/SegmentMerge/SegmentSplit), just skip the local index build job * `ColumnFilePersistedSet::installCompactionResults` if we detect the column file set is changed during minor compaction, then log an error and rollback the minor compaction * `WriteBatches::rollbackWrittenLogAndData` will also remove the pending `removed_log` and `removed_data` Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: JaySon-Huang <tshent@qq.com> Co-authored-by: JaySon <tshent@qq.com> Co-authored-by: JaySon-Huang <tshent@qq.com>

Add test case for delta compact failure

8c2d879

Signed-off-by: JaySon-Huang <tshent@qq.com>

Accept rollback the delta-compact

2686c8a

Signed-off-by: JaySon-Huang <tshent@qq.com>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-triage-completed labels Jul 14, 2025

Do not generate local index tasks if delta is being updating

13f464b

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the vec_delta_compact_fail branch from e1b6111 to 13f464b Compare July 15, 2025 05:32

JaySon-Huang added 2 commits July 15, 2025 14:59

Add ut for WriteBatches

f73ee93

Signed-off-by: JaySon-Huang <tshent@qq.com>

Avoid keeping segment as a shared_ptr in LocalIndexScheduler task

9e48dbd

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang changed the title ~~[WIP] Add test case for delta compact failure~~ vector: Add test case for delta compact failure Jul 15, 2025

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2025

Update logging

2b5ee16

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang requested review from JinheLin and Lloyd-Pottiger August 16, 2025 14:49

JaySon-Huang changed the title ~~vector: Add test case for delta compact failure~~ vector: Fix building vector index on DeltaVS may lead to delta compact failure Aug 16, 2025

JaySon-Huang requested a review from CalvinNeo August 16, 2025 14:52

Lloyd-Pottiger approved these changes Aug 18, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Aug 18, 2025

JinheLin approved these changes Aug 18, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 18, 2025

ti-chi-bot bot merged commit 76a4ec4 into pingcap:master Aug 18, 2025
7 checks passed

JaySon-Huang deleted the vec_delta_compact_fail branch August 18, 2025 14:44

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Aug 20, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Aug 20, 2025

This is an automated cherry-pick of pingcap#10311

87d999a

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Aug 20, 2025

vector: Fix building vector index on DeltaVS may lead to delta compact failure (#10311) #10353

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vector: Fix building vector index on DeltaVS may lead to delta compact failure#10311

vector: Fix building vector index on DeltaVS may lead to delta compact failure#10311
ti-chi-bot[bot] merged 6 commits intopingcap:masterfrom
JaySon-Huang:vec_delta_compact_fail

JaySon-Huang commented Jul 14, 2025 •

edited

Loading

Uh oh!

ti-chi-bot bot commented Aug 18, 2025

Uh oh!

ti-chi-bot bot commented Aug 18, 2025

Uh oh!

Uh oh!

ti-chi-bot commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	// Update the column files in the delta with the new column files.
	for (auto & column_file : delta_persisted_column_files)
	{
	const auto * tiny_file = column_file->tryToTinyFile();
	if (!tiny_file)
	continue;
	if (auto iter = new_column_files_map.find(tiny_file->getDataPageId()); iter != new_column_files_map.end())
	column_file = iter->second;
	}

	delta_persisted_file_set->updatePersistedColumnFilesAfterAddingIndex(delta_persisted_column_files, wbs);

	auto old_persisted_files_iter = persisted_files.begin();
	for (const auto & task : compaction->getTasks())
	{
	for (const auto & file : task.to_compact)
	{
	if (unlikely(
	old_persisted_files_iter == persisted_files.end()
	\|\| (file->getId() != (*old_persisted_files_iter)->getId())
	\|\| (file->getRows() != (*old_persisted_files_iter)->getRows())))
	{
	throw Exception(
	ErrorCodes::LOGICAL_ERROR,
	"Compaction algorithm broken, "
	"compaction={{{}}} persisted_files={} "
	"old_persisted_files_iter.is_end={} "
	"file->getId={} old_persist_files->getId={} file->getRows={} old_persist_files->getRows={}",
	compaction->info(),
	detailInfo(),
	old_persisted_files_iter == persisted_files.end(),
	file->getId(),
	old_persisted_files_iter == persisted_files.end() ? -1 : (*old_persisted_files_iter)->getId(),
	file->getRows(),
	old_persisted_files_iter == persisted_files.end() ? -1 : (*old_persisted_files_iter)->getRows());
	}
	old_persisted_files_iter++;
	}
	}

Conversation

JaySon-Huang commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Aug 18, 2025

Uh oh!

ti-chi-bot bot commented Aug 18, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

ti-chi-bot commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JaySon-Huang commented Jul 14, 2025 •

edited

Loading