Skip to content

unlink METADATA_VERSION_FILE_NAME before rewrite it, fix no such key thrown #87838

Merged
CheSema merged 2 commits intomasterfrom
chesema-no-such-key
Sep 30, 2025
Merged

unlink METADATA_VERSION_FILE_NAME before rewrite it, fix no such key thrown #87838
CheSema merged 2 commits intomasterfrom
chesema-no-such-key

Conversation

@CheSema
Copy link
Copy Markdown
Member

@CheSema CheSema commented Sep 29, 2025

In private repo I see that the race is possible between attaching parts and removing condemned parts.
Removing process reads files, those files are supposed to be immutable. But attach process changes content in METADATA_VERSION_FILE_NAME file in a way that hard linked file in condemned directory also is changed.

https://d1k2gkhrlfqv31.cloudfront.net/clickhouse-test-reports-private/json.html?REF=master&sha=a71ec07a0761d5e1a8eb4bf2ec3d6e40d20708fc&name_0=MasterCI&name_1=Stateless%20tests%20%28amd_msan%2C%20meta%20storage%20in%20keeper%2C%20s3%20storage%2C%20parallel%2C%202%2F2%29&name_1=Stateless%20tests%20%28amd_msan%2C%20meta%20storage%20in%20keeper%2C%20s3%20storage%2C%20parallel%2C%202%2F2%29

This change is not related to public but the function is shared between repos and this code would work correctly for both.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

...

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Sep 29, 2025

Workflow [PR], commit [4825f59]

Summary:

job_name test_name status info comment
Integration tests (arm_binary, distributed plan, 3/4) failure
test_keeper_memory_soft_limit/test.py::test_soft_limit_create FAIL
Integration tests (arm_binary, distributed plan, 4/4) failure
test_trace_log_memory_context/test.py::test_memory_context_in_trace_log FAIL

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Sep 29, 2025
@Michicosun Michicosun self-assigned this Sep 29, 2025
@CheSema CheSema force-pushed the chesema-no-such-key branch from 76e8fe3 to aca3ee8 Compare September 29, 2025 15:37
@CheSema CheSema changed the title unlink METADATA_VERSION_FILE_NAME before rewrite it unlink METADATA_VERSION_FILE_NAME before rewrite it, fix no such key thrown Sep 29, 2025
Comment thread src/Storages/MergeTree/IMergeTreeDataPart.cpp Outdated
Co-authored-by: Mikhail Artemenko <34075916+Michicosun@users.noreply.github.com>
@CheSema CheSema added this pull request to the merge queue Sep 30, 2025
Merged via the queue into master with commit 73ad3cc Sep 30, 2025
120 of 123 checks passed
@CheSema CheSema deleted the chesema-no-such-key branch September 30, 2025 11:49
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants