vector: Fix building vector index on DeltaVS may lead to delta compact failure (#10311) by ti-chi-bot · Pull Request #10353 · pingcap/tiflash

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

This is an automated cherry-pick of #10311

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: ti-chi-bot <ti-community-prow-bot@tidb.io>

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

@JaySon-Huang This PR has conflicts, I have hold it.
Please resolve them or ask others to resolve them, then comment /unhold to remove the hold label.

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

@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide.

Details

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.

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

JaySon-Huang · 2025-08-29T10:47:24Z

/unhold
conflict are resolved

ti-chi-bot · 2025-09-01T02:31:47Z

[LGTM Timeline notifier]

Timeline:

2025-08-29 10:47:43.766644701 +0000 UTC m=+523555.644475655: ☑️ agreed by JaySon-Huang.
2025-09-01 02:31:47.217030744 +0000 UTC m=+752999.094861708: ☑️ agreed by Lloyd-Pottiger.

ti-chi-bot · 2025-09-01T02:47:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, 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~~ [JaySon-Huang,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

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

Merged

12 tasks

ti-chi-bot bot added the do-not-merge/cherry-pick-not-approved label Aug 20, 2025

ti-chi-bot assigned JaySon-Huang Aug 20, 2025

ti-chi-bot bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Aug 20, 2025

Resolve conflict

dd7880d

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

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 29, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2025

JaySon-Huang requested review from JinheLin and Lloyd-Pottiger August 29, 2025 10:47

JaySon-Huang approved these changes Aug 29, 2025

View reviewed changes

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

Lloyd-Pottiger approved these changes Sep 1, 2025

View reviewed changes

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

JinheLin approved these changes Sep 1, 2025

View reviewed changes

ti-chi-bot bot merged commit 1bcc11e into pingcap:release-8.5 Sep 1, 2025
2 of 4 checks passed

ti-chi-bot bot deleted the cherry-pick-10311-to-release-8.5 branch September 1, 2025 04:15

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)#10353

vector: Fix building vector index on DeltaVS may lead to delta compact failure (#10311)#10353
ti-chi-bot[bot] merged 2 commits intopingcap:release-8.5from
ti-chi-bot:cherry-pick-10311-to-release-8.5

ti-chi-bot commented Aug 20, 2025

Uh oh!

ti-chi-bot commented Aug 20, 2025

Uh oh!

ti-chi-bot bot commented Aug 20, 2025

Uh oh!

JaySon-Huang commented Aug 29, 2025

Uh oh!

ti-chi-bot bot commented Sep 1, 2025

Uh oh!

ti-chi-bot bot commented Sep 1, 2025

Uh oh!

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

ti-chi-bot commented Aug 20, 2025

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Aug 20, 2025

Uh oh!

ti-chi-bot bot commented Aug 20, 2025

Uh oh!

JaySon-Huang commented Aug 29, 2025

Uh oh!

ti-chi-bot bot commented Sep 1, 2025

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Sep 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants