export part - skip or throw on pending mutations and patch parts #1294

arthurpassos · 2026-01-13T12:35:00Z

Changelog category (leave one):

Improvement

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

Add export_merge_tree_part_throw_on_pending_mutations and export_merge_tree_part_throw_on_pending_patch_parts to control the behavior of pending mutations / patch parts on the export part feature. By default it will throw. It is important to note that when exporting an outdated it will never throw.

Also add export_merge_tree_part_allow_outdated_parts that allows outdated parts to be exported. Defaults to false.

Documentation entry for user-facing changes

Add export_merge_tree_part_throw_on_pending_mutations and export_merge_tree_part_throw_on_pending_patch_parts to control the behavior of pending mutations / patch parts on the export part feature. By default it will throw. It is important to note that when exporting an outdated it will never throw.

Also add export_merge_tree_part_allow_outdated_parts that allows outdated parts to be exported. Defaults to false.

CI/CD Options

Exclude tests:

Regression jobs to run:

arthurpassos · 2026-01-13T12:35:14Z

One sec and I will adapt it for export partition

github-actions · 2026-01-13T12:35:56Z

Workflow [PR], commit [3ee9ce3]

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 28e124685d

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

src/Storages/MergeTree/MergeTreeData.cpp

arthurpassos · 2026-01-14T16:02:09Z

src/Storages/StorageReplicatedMergeTree.cpp


-    const auto parts = getDataPartsVectorInPartitionForInternalUsage(MergeTreeDataPartState::Active, partition_id, &data_parts_lock);
+    {
+        auto data_parts_lock = lockParts();


Either getMutationsSnapshot or getAlterConversionsForPart grabs a lock. Putting this in a scope to avoid dead locks

arthurpassos · 2026-01-14T17:29:39Z

QA team has requested a report of why I think this is ready to be reviewed / merged.

I have written several tests and performed some manual tests that cover the following cases for both part and partition export:

Mutations and patch parts created before the export. Skip and throw should behave as expected.
Mutations and patch parts created after the export has been issued, but not processed yet. Tests were implemented with iptable rules to simulate s3 outage. Should not throw or apply patches, should simply skip.

I have also tested that IN PARTITION works for replicated engines, they don't for plain merge tree tho.

I have also manually tested that outdated parts can be exported if the setting is set to true. It does not work for partition export, and it is expected. -- Honestly, maybe this setting should not even exist.

arthurpassos · 2026-01-15T12:01:40Z

Messed up with the tests, need to fix it

Fixed

arthurpassos · 2026-01-15T18:56:31Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1d1f865b6

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

docs/en/engines/table-engines/mergetree-family/part_export.md

docs/en/engines/table-engines/mergetree-family/partition_export.md

ianton-ru · 2026-01-16T10:28:20Z

src/Storages/MergeTree/MergeTreeData.cpp

+    const auto alter_conversions = getAlterConversionsForPart(part, mutations_snapshot, query_context);
+
+    /// re-check `throw_on_pending_mutations` because `pending_mutations` might have been filled due to `throw_on_pending_patch_parts`
+    if (alter_conversions->hasMutations() && throw_on_pending_mutations)


NIT: suggestion to check boolean flag at left and call method at right from &&. hasMutations is a fast method, just a check if list empty or not, but current order calls question "is hasMutations fast or not".

ianton-ru · 2026-01-16T12:02:42Z

src/Storages/MergeTree/MergeTreeData.cpp

+    {
+        .metadata_version = source_metadata_ptr->getMetadataVersion(),
+        .min_part_metadata_version = part->getMetadataVersion(),
+        .need_data_mutations = throw_on_pending_mutations,


Does ClickHouse throw some exception when this flag is set? made only fast check, but looks like only some parts are skipped.

Does ClickHouse throw some exception when this flag is set

It should not, at least in this context. The code you are reviewing is the code that process the export query request, not the code that actually exports the data. The mutations_snapshot_params is used to create mutations_snapshot and alter_conversions. None is used for anything but to check if there are pending mutations within this function.

but looks like only some parts are skipped.

I did not understand this, can you elaborate?

Unclean. why flags "need_something" are filled with values from "throw_on_some_condition".
When I see "throw_on_some_condition", I expect that setting causes exception, but here it changes other behavior.

TLDR: To throw, I need to know if it exists. To know if it exists, I need to set it to true.

Ah, I see. The need_something flags dictacte if the mutation snapshot contains the mutations / patch parts and etc. If I hard code it to false, the snapshot will never include information about pending mutations / patch parts. In this case, I will not be able to check if they exist and will not be able to throw.

I expect that setting causes exception, but here it changes other behavior.

It does throw, look at https://github.com/Altinity/ClickHouse/pull/1294/changes#diff-b7d33a7abda5f3365d5e4fb58ca98b756f6401372b2e2d5eb449a2bd3444fb5eR6286 and https://github.com/Altinity/ClickHouse/pull/1294/changes#diff-b7d33a7abda5f3365d5e4fb58ca98b756f6401372b2e2d5eb449a2bd3444fb5eR6293

I see that it throw exception. I mean, setting does something more than only throw exception.

What else does it do in this case? I don't think so.

I say about naming, not about code logic :)

Setting export_merge_tree_part_throw_on_pending_mutations change value of need_data_mutations field in mutations_snapshot_params. need_data_mutations changes behavior. So setting name is confusing, something like "export_merge_tree_part_do_not_export_on_pending_mutations" (I'm not good in naming) is more clean.

need_data_mutations changes behavior.

Not in this case. Like I said here, in this code snippet the only effect of setting need_data_mutations is that the snapshot and alter conversions object will contain information about mutations.

And if it does, we throw. Because the only reason for that object to have mutations is: 1. there are pending mutations; 2. the user set throw = true

ianton-ru · 2026-01-16T13:45:23Z

src/Storages/MergeTree/MergeTreeData.cpp

+            part_name);
+    }
+
+    if (alter_conversions->hasPatches())


Need to check export_merge_tree_part_throw_on_pending_patch_parts here?

No because of #1294 (comment)

I can add the check if you think it is more clear, but it is not needed at all

export part - skip or throw on pending mutations and patch parts

28e1246

chatgpt-codex-connector bot reviewed Jan 13, 2026

View reviewed changes

src/Storages/MergeTree/MergeTreeData.cpp Outdated Show resolved Hide resolved

export partition as well

dd10dda

svb-alt added the antalya label Jan 14, 2026

arthurpassos added 2 commits January 14, 2026 09:32

add more tests and remove unneeded change

506a8ab

add tests and add settings history

8254d0b

arthurpassos commented Jan 14, 2026

View reviewed changes

arthurpassos added 2 commits January 14, 2026 13:04

simplify things

87bf725

add docs for new settings

d4f1609

remove unsupported setting from partition export docs

4c59e82

fix test glitch

d1d1f86

arthurpassos added the port-antalya PRs to be ported to all new Antalya releases label Jan 15, 2026

chatgpt-codex-connector bot reviewed Jan 15, 2026

View reviewed changes

docs/en/engines/table-engines/mergetree-family/part_export.md Outdated Show resolved Hide resolved

docs/en/engines/table-engines/mergetree-family/partition_export.md Outdated Show resolved Hide resolved

fix docs

8eaadcf

svb-alt added antalya-25.8 antalya-25.8.14.20001 antalya-25.8.14 labels Jan 15, 2026

ianton-ru reviewed Jan 16, 2026

View reviewed changes

reorder boolean expression

3ee9ce3

ianton-ru reviewed Jan 16, 2026

View reviewed changes

export part - skip or throw on pending mutations and patch parts #1294

Are you sure you want to change the base?

export part - skip or throw on pending mutations and patch parts #1294

Uh oh!

Conversation

arthurpassos commented Jan 13, 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!

arthurpassos commented Jan 13, 2026

Uh oh!

github-actions bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arthurpassos commented Jan 14, 2026

Uh oh!

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

Uh oh!

Uh oh!

arthurpassos commented Jan 15, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arthurpassos commented Jan 13, 2026 •

edited

Loading

github-actions bot commented Jan 13, 2026 •

edited

Loading

arthurpassos commented Jan 15, 2026 •

edited

Loading