Skip to content

Fix EmbeddedRocksDB upgrade#87392

Merged
Algunenano merged 1 commit intoClickHouse:masterfrom
Algunenano:rocksdb_upgrade
Sep 22, 2025
Merged

Fix EmbeddedRocksDB upgrade#87392
Algunenano merged 1 commit intoClickHouse:masterfrom
Algunenano:rocksdb_upgrade

Conversation

@Algunenano
Copy link
Copy Markdown
Member

@Algunenano Algunenano commented Sep 22, 2025

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

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

Fix EmbeddedRocksDB upgrade

Before #87109 we used to a) allow any path for the EmbeddedRocksDB table, and b) create it under databases if it wasn't provided. #87109 changed it to only allow using user_files, but it breaks upgrades for tables created without path. This addressed this by allowing attaching tables from the old default path

Follow up to #87109. Must be backported with it
Closes #87331

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 22, 2025 •
edited
Loading

Workflow [PR], commit [c5aab7d]

Summary: ⏳

job_name test_name status info comment
Integration tests (amd_asan, old analyzer, 3/6) failure
test_restore_replica/test.py::test_restore_replica_sequential FAIL
test_restore_replica/test.py::test_restore_replica_parallel FAIL

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels Sep 22, 2025
@bharatnc bharatnc self-assigned this Sep 22, 2025
@Algunenano Algunenano added v25.8-must-backport v25.9-must-backport and removed pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels Sep 22, 2025
@Algunenano Algunenano added this pull request to the merge queue Sep 22, 2025
Merged via the queue into ClickHouse:master with commit 8b8a710 Sep 22, 2025
117 of 122 checks passed
@Algunenano Algunenano deleted the rocksdb_upgrade branch September 22, 2025 14:47
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Sep 22, 2025
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 mentioned this pull request Sep 22, 2025
Merged
robot-clickhouse-ci-2 added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse-ci-2
Merge pull request #87408 from ClickHouse/cherrypick/25.8/87392
f0c8f7f 
Cherry pick #87392 to 25.8: Fix EmbeddedRocksDB upgrade
robot-clickhouse added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse
Backport #87392 to 25.8: Fix EmbeddedRocksDB upgrade
d924592
This was referenced Sep 22, 2025
Merged
Merged
robot-clickhouse-ci-2 added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse-ci-2
Merge pull request #87410 from ClickHouse/cherrypick/25.9/87392
416e681 
Cherry pick #87392 to 25.9: Fix EmbeddedRocksDB upgrade
robot-clickhouse added a commit that referenced this pull request Sep 22, 2025
@robot-clickhouse
Backport #87392 to 25.9: Fix EmbeddedRocksDB upgrade
6c69d5e
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 mentioned this pull request Sep 22, 2025
Merged
@janeklb
Copy link
Copy Markdown

janeklb commented Sep 22, 2025

@Algunenano this PR (as well as #87109) break existing EmbeddedRocksDB engine tables that have specified a rocksdb_path that is outside of user_files

https://clickhousedb.slack.com/archives/CU478UEQZ/p1758050072176469

@Algunenano
Copy link
Copy Markdown
Member Author

Algunenano commented Sep 22, 2025

@Algunenano this PR (as well as #87109) break existing EmbeddedRocksDB engine tables that have specified a rocksdb_path that is outside of user_files

That's intended. It should not be possible to create files outside of user_files. It was a security vulnerability that should not have been possible.

@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 22, 2025
@janeklb
Copy link
Copy Markdown

janeklb commented Sep 22, 2025

Ok, but then this should at least be listed as a breaking change in the changelog.

We have ~40 EmbeddedRocksDB tables with a custom path (outside of user files). Is there an easy way to update those or do they need to be recreated

@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created-cloud deprecated label, NOOP label Sep 22, 2025
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Sep 22, 2025
clickhouse-gh bot added a commit that referenced this pull request Sep 22, 2025
@clickhouse-gh
Merge pull request #87411 from ClickHouse/backport/25.9/87392
7da3f45 
Backport #87392 to 25.9: Fix EmbeddedRocksDB upgrade
@janeklb
Copy link
Copy Markdown

janeklb commented Sep 22, 2025 •
edited
Loading

That's intended. It should not be possible to create files outside of user_files. It was a security vulnerability that should not have been possible.

On further thought, this reasoning seems to suggest you should only ever mount disks (eg. for use with storage_policy) in the user_files path.. but that doesn't seem right at all since it contradicts official docs (eg. https://clickhouse.com/docs/engines/table-engines/mergetree-family/mergetree#table_engine-mergetree-multiple-volumes has no mention of user_files)

Can you please help me understand what I'm missing? Why do EmbeddedRocksDB tables need to live within user_files? And if that's a security vulnerability, why is it ok to use storage_policy that's configured on disks outside of user_files?

Algunenano added a commit that referenced this pull request Sep 23, 2025
@Algunenano
Merge pull request #87409 from ClickHouse/backport/25.8/87392
c295b3a 
Backport #87392 to 25.8: Fix EmbeddedRocksDB upgrade
@Algunenano
Copy link
Copy Markdown
Member Author

Algunenano commented Sep 23, 2025

Tables must live in a directory controlled by ClickHouse. The path option in the engine allowed them to live anywhere in the system.

@Algunenano Algunenano mentioned this pull request Sep 23, 2025
Merged
1 task
azat added a commit that referenced this pull request Sep 23, 2025
@azat
Merge remote-tracking branch 'u/backport/25.9/87449' into cherrypick/…
cda044d 
…25.9/87449

* u/backport/25.9/87449:
  Backport #87426 to 25.9: Ignore only not found errors for s3_plain_rewritable (and some other S3 code)
  Backport #87231 to 25.9: Fix "Too large size passed to allocator" UB in JOIN due to mixed const and non-const blocks
  Backport #87198 to 25.9: Disable the setting by default
  Backport #87392 to 25.9: Fix EmbeddedRocksDB upgrade
  Backport #87140 to 25.9: fix: fix server level max temporary size limit
  Backport #87178 to 25.9: PR: fix LEFT/INNER ... RIGHT ... JOINS chain
  Update autogenerated version to 25.10.1.1 and contributors
@telperions telperions mentioned this pull request Sep 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bharatnc bharatnc bharatnc approved these changes

Assignees

@bharatnc bharatnc

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-critical-bugfix pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v25.8-must-backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

25.8.4 breakes existing RocksDB tables

5 participants

@Algunenano @janeklb @bharatnc @robot-ch-test-poll @robot-clickhouse-ci-2