Replace the export partition local manifests lock by a multiversion container by arthurpassos · Pull Request #1912 · Altinity/ClickHouse

arthurpassos · 2026-06-16T13:14:33Z

Changelog category (leave one):

Performance Improvement

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

Yet another attempt to reduce lock contention observed while slow writes / commits to iceberg backed by ice-rest-catalog. Uses a MultiVersion container instead of locking. Follow up of #1813. Closes #1920

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

Regression jobs to run:

github-actions · 2026-06-16T13:17:09Z

Workflow [PR], commit [653f722]

arthurpassos · 2026-06-24T00:08:42Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef56768b4f

ℹ️ 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".

chatgpt-codex-connector · 2026-06-24T00:16:27Z


-    std::vector<ReplicatedPartitionExportInfo> infos;
-    infos.reserve(snapshots.size());
+    std::vector<ReplicatedPartitionExportInfo> infos(model->size());


Reserve the export info vector instead of pre-sizing it

When the read model contains N exports, this constructs N default ReplicatedPartitionExportInfo rows and the loop then appends the N real rows with emplace_back. As soon as any export exists, system.replicated_partition_exports will expose extra blank rows and report twice as many entries; this should reserve capacity rather than resize the vector.

Useful? React with 👍 / 👎.

arthurpassos · 2026-06-24T00:29:52Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46f5b11775

ℹ️ 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".

chatgpt-codex-connector · 2026-06-24T00:35:10Z


-    std::vector<ReplicatedPartitionExportInfo> infos;
-    infos.reserve(snapshots.size());
+    std::vector<ReplicatedPartitionExportInfo> infos(model->size());


Avoid pre-sizing infos before appending rows

When there is at least one published export entry, this constructs model->size() default ReplicatedPartitionExportInfo objects and then the loop appends another row for each real entry with emplace_back. As a result, system.replicated_partition_exports returns one blank/default row per real export in addition to the correct rows, with fields like destination/table/status empty and some numeric fields default/uninitialized. This should reserve capacity instead of changing the vector size.

Useful? React with 👍 / 👎.

This seems to be a forgotten thing to be fixed

Oh wow, i did fix it yesterday before pinging you. But I think I forgot to push it.

mkmkme

Requesting changes since the comment found by Codex looks like one worth fixing. The rest of my comments is open for discussions and not necessarily an urge for action.

mkmkme · 2026-06-24T09:03:41Z

+    bool local_entry_found = false;
+    bool local_entry_pending = false;
+    std::string local_composite_key;
+    {


Just a style nitpick: instead of this

{ const auto model = export_read_model.get(); if (model) { /// do stuff with model... } }

you could do this

if (const auto model = export_read_model.get()) { /// do stuff with model... }

mkmkme · 2026-06-24T09:32:15Z

    {
-        const auto & key = it->getCompositeKey();
+        const auto & key = entry.getCompositeKey();
        if (zk_children.contains(key))


Since this loop got split into two separate loops, this body looks quite simple. It can be even simpler if done as

for (const auto & entry : entries_by_key) { const auto & key = entry.getCompositeKey(); if (!zk_children.contains(key)) stale_keys.emplace_back(key, entry.manifest.transaction_id); }

IMHO this looks a bit more straightforward and to the point of what we actually expect from the loop.

But the bigger question is: was there any justification for splitting the loop in the first place? I agree it does look simpler now, but now we have two iterations instead of one (and introduces another entries_by_key.find() operation that wasn't needed in the previous version). And IIUC both loops are under one larger lock (IOW there's no reduce in lock scope). So I'm a bit puzzled here

mkmkme · 2026-06-24T09:43:05Z

                local_status_changes.pop();
                continue;
            }
+            const std::string transaction_id = it->manifest.transaction_id;


This could be moved closer to line 711 where it's actually needed. Also this could be a const-ref.

And since it's used only once, maybe we don't need it as a local var at all and can continue calling killExportPart with it->manifest.transaction_id directly

mkmkme · 2026-06-24T10:13:58Z

 #include <Common/EventNotifier.h>
+#include <Common/MultiVersion.h>
 #include <Common/ProfileEventsScope.h>
+#include <Common/SharedMutex.h>


Is this include needed here? Don't see any code changes in the header that would require it

small improvements

376d0d0

arthurpassos added port-antalya PRs to be ported to all new Antalya releases antalya antalya-26.3 labels Jun 16, 2026

arthurpassos added 4 commits June 17, 2026 19:15

refactor export updating task to make lock free network requests

22e5746

my own modifications

accaf96

add missing continue

ccee3a4

add best-effort comment

3b9da6d

svb-alt requested a review from mkmkme June 19, 2026 13:36

arthurpassos added 7 commits June 19, 2026 10:38

remove unnecessary comments

18aa8d6

Merge branch 'antalya-26.3' into rw_lock_for_partition_export

9db06b9

Merge branch 'antalya-26.3' into rw_lock_for_partition_export

4323e7f

faster, but unsure about ub and code complexity

15b140f

multiversion as opposed to locking

415a80a

simplify a thing or two

041439e

unrelated fix

ef56768

arthurpassos changed the title ~~RW lock for the export partition feature~~ Replace the export partition local manifests lock by a multiversion container Jun 24, 2026

Merge branch 'antalya-26.3' into rw_lock_for_partition_export

46f5b11

chatgpt-codex-connector Bot reviewed Jun 24, 2026

View reviewed changes

mkmkme requested changes Jun 24, 2026

View reviewed changes

opsy

653f722

Conversation

arthurpassos commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Documentation entry for user-facing changes

CI/CD Options

Exclude tests:

Regression jobs to run:

Uh oh!

github-actions Bot commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arthurpassos commented Jun 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

arthurpassos commented Jun 24, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurpassos Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme left a comment

Choose a reason for hiding this comment

Uh oh!

mkmkme Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

mkmkme Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arthurpassos commented Jun 16, 2026 •

edited

Loading

github-actions Bot commented Jun 16, 2026 •

edited

Loading

mkmkme Jun 24, 2026 •

edited

Loading