Improvement locking Iceberg iterator

[ianton-ru]

Changelog category (leave one):

• Performance Improvement
  Improvement locking Iceberg iterator

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

What is going on:
In IcebergCluster request each replica creates pull of threads, each thread try to get next task. Each thread calls callback

std::lock_guard lock(callback_mutex);
sendReadTaskRequest();
auto res = receiveClusterFunctionReadTaskResponse(*query_state);

All threads share the same socket, only one send request, all others wait on mutex.

Requests from each replica are sending on initiator. Initiator processes them in multiple threads, but all threads calls StorageObjectStorageStableTaskDistributor::getNextTask. This method calls three other methods with lock mutex inside each

    auto file = getPreQueuedFile(number_of_current_replica);
    if (!file)
        file = getMatchingFileFromIterator(number_of_current_replica);
    if (!file)
        file = getAnyUnprocessedFile(number_of_current_replica);

getPreQueuedFile tries to find path for replica in already extracted paths.
getMatchingFileFromIterator calls IcebergIterator::next to extract next path, calculate replica for path, if it is a replica which send request, send this path as response, if this path is for other replica, puts it in list for getPreQueuedFile
getAnyUnprocessedFile executed only when all paths are extracted and more nothing for current replica.

When getMatchingFileFromIterator calls IcebergIterator::next.

IcebergIterator::next is built over ConcurrentBoundedQueue, next waits until something is pushed into queue or queue is finished. So when queue is empty, all threads are waiting until something is happened.

In separate thread IcebergIterator calls SingleThreadIcebergKeysIterator::next to extract next path from Iceberg metadata. It can take a long time, because of reading metadata from S3, pruning files, etc.

Possible scenario:

• Replica 1 sends request. Initiator executes StorageObjectStorageStableTaskDistributor::getNextTask, finds path for replica 2, put it into list, finds file for replica 1, return it.
• Replica 1 sends request again. Initiator waits for a next object for a long time, all other objects pruned inside SingleThreadIcebergKeysIterator::next, and it takes a dozens of seconds.
• Replica 2 send request. StorageObjectStorageStableTaskDistributor has an object for replica 2, but StorageObjectStorageStableTaskDistributor locked until SingleThreadIcebergKeysIterator not finished work.

As result, replica 2 gets path only after processing all metadata.

Solution:

IcebergIterator is built over ConcurrentBoundedQueue, so next method is thread-safe. It can be called without lock StorageObjectStorageStableTaskDistributor, and replica 2 can get prequeued path without waiting SingleThreadIcebergKeysIterator::next.

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All with Aarch64
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• Tiered Storage (2h)

[ianton-ru]

@codex review

[github-actions]

Workflow [PR], commit [51762a7]

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[zvonand]

Why there is an antalya-26.1 label? this is a PR to 25.8

[ianton-ru]

Change make sense for 26.1 too, but good to approve that it fixed the issue before porting.

[zvonand]

We're trying to abandon 25.8 :) We release it as is (hopefully, today/tomorrow), idk if we will do further releases

So I guess it is not a priority now (to verify something against 25.8, having in mind it will have to be re-verified later for 26.1)

[arthurpassos]

LGTM, left a few nit comments.

Did you benchmark it to see if there is any practical difference?

[arthurpassos]

Please add docs, it is not clear to the reader of this API what has_concurrent_next means. Plus, shouldn't it be hasConcurrentNext?

[arthurpassos]

Perhaps something more descriptive like hasLockingMechanismInNextOperation

[arthurpassos]

I would add a comment similar to the below:

In case IObjectIterator::hasConcurrentNext is true, a locking mechanism is already in place in IObjectIterator::next. Therefore, there is no need to guard it with StorageObjectStorageStableTaskDistributor::mutex

[zvonand]

Likely this was fixed in the branch. Let's try updating upon the latest changes