Skip to content

Fix plain object storage move directory operation undo#88198

Merged
Michicosun merged 4 commits intomasterfrom
fix_plain_undo
Oct 15, 2025
Merged

Fix plain object storage move directory operation undo#88198
Michicosun merged 4 commits intomasterfrom
fix_plain_undo

Conversation

@Michicosun
Copy link
Copy Markdown
Member

@Michicosun Michicosun commented Oct 7, 2025

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Do proper undo of the move directory operation in case of error. We need to rewrite all prefix.path objects changed during the execution, not only the root one.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 7, 2025

Workflow [PR], commit [d891f81]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Oct 7, 2025
@tuanpach
Copy link
Copy Markdown
Member

tuanpach commented Oct 8, 2025

Could you give some explanation about the bug and the fix of this change?

@antonio2368 antonio2368 self-assigned this Oct 8, 2025
@antonio2368
Copy link
Copy Markdown
Member

This is a bugfix so it needs changelog.

@Michicosun
Copy link
Copy Markdown
Member Author

Could you give some explanation about the bug and the fix of this change?

For example, if you have this tree

├── __meta
│   ├── jgtebynqegaurjaqegmluoztzyhnuxfr
│   │   └── prefix.path 
│   │       └── xl.meta A/B/C/G/
│   ├── qfyudsrpexcaikpgnpvfppjvyskkbhqg
│   │   └── prefix.path
│   │       └── xl.meta A/B/
│   ├── vajvgtqqpluxgbstacyeuprihfvxpbzi
│   │   └── prefix.path
│   │       └── xl.meta A/B/D/
│   ├── vqmufevphfeqbetdniedzqimqbwpraey
│   │   └── prefix.path
│   │       └── xl.meta A/
│   ├── wnygkhaxgcruzpmxtmzoaknnmaeorvds
│   │   └── prefix.path
│   │       └── xl.meta A/B/E/
│   └── yaillkbvcggwapggvstxwjbvgiqfzruh
│       └── prefix.path
│           └── xl.meta A/B/C/

And you execute moveDirectory('A', 'MOVED'); moveFile('non-existing', 'other-place'), then the tx will fail, because the non-existing file does not exist, but during the undo of the first operation, only the root folder (in this case 'A') will be moved back, but A/B will not.

Though here is another problem, move directory renames only the first level folder, which means in the example ^ only A/ and A/B/ will be changed, but not the whole subtree, but I will fix that in another PR.

@Michicosun
Copy link
Copy Markdown
Member Author

Waiting for #88240 to be able to extend with tests and change the changelog entry to bugfix

@clickhouse-gh clickhouse-gh bot added pr-bugfix Pull request with bugfix, not backported by default and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Oct 14, 2025
@Michicosun Michicosun added this pull request to the merge queue Oct 15, 2025
Merged via the queue into master with commit 0f1ff9c Oct 15, 2025
118 of 123 checks passed
@Michicosun Michicosun deleted the fix_plain_undo branch October 15, 2025 12:15
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default 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.

4 participants