Storages: small refine of MergedTask.

[JinheLin]

What problem does this PR solve?

Issue Number: ref #6834

What is changed and how it works?

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

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

None

[JinheLin]

/run-all-tests

[JinheLin]

/run-integration-test

[JaySon-Huang]

Will this cause performance regression when there are multiple MergedUnits? Previously we only compare two size_t, now we need to iterate all units to get the result.

tiflash/dbms/src/Storages/DeltaMerge/ReadThread/SegmentReader.cpp

Lines 90 to 111 in 939532a

	int read_count = 0;
	while (!merged_task->allStreamsFinished() && !isStop())
	{
	auto c = merged_task->readBlock();
	read_count += c;
	if (c <= 0)
	{
	break;
	}
	}
	if (read_count <= 0)
	{
	LOG_DEBUG(log, "All finished, merged_task=<{}> read_count={}", merged_task->toString(), read_count);
	}
	// If `merged_task` is pushed back to `MergedTaskPool`, it can be accessed by another read thread if it is scheduled.
	// So do not push back to `MergedTaskPool` when exception happened since current read thread can still access to this `merged_task` object and set exception message to it.
	// If exception happens, `merged_task` will be released by `shared_ptr` automatically.
	if (!merged_task->allStreamsFinished())
	{
	SegmentReadTaskScheduler::instance().pushMergedTask(merged_task);
	}
	}

[JinheLin]

Normally, size of units is small, such as 1, 2, or 3... In such cases, no performance hurted.

In extreme cases, its size is 100.

Currently, 100 is the limitation of the total number of merged segments, not for each MergedTask.

I think we should limit the size of each MergedTask too, such 5, because segments of a MergedTask is read sequentially. (This will be considered in another PR.)

tiflash/dbms/src/Storages/DeltaMerge/ReadThread/SegmentReadTaskScheduler.cpp

Lines 189 to 202 in 939532a

	if (MergedTask::getPassiveMergedSegments() < 100 || target->second.size() == 1)
	{
	result = *target;
	merging_segments.erase(target);
	}
	else
	{
	result = std::pair{target->first, std::vector<uint64_t>(1, pool->pool_id)};
	auto itr = std::find(target->second.begin(), target->second.end(), pool->pool_id);
	*itr = target->second
	.back(); // SegmentReadTaskPool::scheduleSegment ensures `pool->poolId` must exists in `target`.
	target->second.resize(target->second.size() - 1);
	}
	}

[JinheLin]

However, keeping finished_count could be more effective in all scenarios. So I have added finished_count back.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JaySon-Huang,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]

[LGTM Timeline notifier]

Timeline:

• 2023-12-13 07:23:29.699874883 +0000 UTC m=+427300.737101809: ☑️ agreed by Lloyd-Pottiger.
• 2023-12-14 05:34:07.255632585 +0000 UTC m=+507138.292859497: ☑️ agreed by JaySon-Huang.

[JaySon-Huang]

/run-all-tests