This is an automated comment for commit d0bc728 with description of existing statuses. It's updated for the latest CI running

✅ Click here to open a full report in a separate page

We have done this locally (but using v9.2.2 instead), I'm about to submit upstream PR then see this PR.
There's two backward incompatibility rocksdb option when upgrading rocksdb from current version to any version > 8.6.

• rocksdb::BlockBasedTableOptions::checksum: 3 -> 4
• rocksdb::BlockBasedTableOptions::format_version: 5 -> 6

Therefore we introduce two new settings for rocksdb table:

/* Advanced build-in rocksdb options */ \
M(UInt32, rocksdb_buildin_block_based_table_options_checksum, 3, "Checksum type for block based table options, see rocksdb::BlockBasedTableOptions::checksum", 0) \
M(UInt32, rocksdb_buildin_block_based_table_options_format_version, 5, "Version number for block based table options, see rocksdb::BlockBasedTableOptions::format_version", 0) \

To be set by default to same as current version for backward compatibility, when new version of rocksdb is stable then we will change these settings to new values.

It has been running for few weeks and I think the new rocksdb version (v9.2.2) is quite stable.

@canhld94 As I am really a noob when it comes to RocksDB, I'd appreciate help 😄

Side note: An upgrade from 6.x to 7.10.2 is here and I'll try to get it merged this week.

What about this: Once the other PR is merged, we can remove the underlying two commits

... and then make this an upgrade to the latest RocksDB version you are using in your production (+ new settings if needed)?

@canhld94 As I am really a noob when it comes to RocksDB, I'd appreciate help 😄

Side note: An upgrade from 6.x to 7.10.2 is here and I'll try to get it merged this week.

What about this: Once the other PR is merged, we can remove the underlying two commits

... and then make this an upgrade to the latest RocksDB version you are using in your production (+ new settings if needed)?

@rschu1ze thanks, yes I'm willing to submit PR for rocksdb 9.2.2 upgrade.
Note that there is also backward incompatibility rocksdb option (rocksdb::BlockBasedTableOptions::checksum from 3->4) when upgrade to 7.10.2.

What you are saying is that #66475 is backward-incompatible without special settings?

In that case, please go ahead and kindly push your v6.x --> v9.2.2 upgrade. No need then to update rocksdb in small increments.

What you are saying is that #66475 is backward-incompatible without special settings

Yes, in version 6.27.0 new hash type kXXH3 was added https://github.com/facebook/rocksdb/blob/main/HISTORY.md#new-features-30. Then in v7.8.0 kXXH3 was made default https://github.com/facebook/rocksdb/blob/main/HISTORY.md#public-api-changes-19. Current ClickHouse rocksdb version is 6.26, which doesn't support kXXH3.

What you are saying is that #66475 is backward-incompatible without special settings

Yes, in version 6.27.0 new hash type kXXH3 was added https://github.com/facebook/rocksdb/blob/main/HISTORY.md#new-features-30. Then in v7.8.0 kXXH3 was made default https://github.com/facebook/rocksdb/blob/main/HISTORY.md#public-api-changes-19. Current ClickHouse rocksdb version is 6.26, which doesn't support kXXH3.

Well may be saying backward-incompatible is incorrect. I mean it's a breaking change, once we upgrade then we cannot rollback.

@canhld94 We had some internal discussion based on above compatibility concerns. Turns out that there are two places in ClickHouse which use RocksDB: The EmbeddedRocksDB table engine, and RocksDB as a experimental backend of Keeper. The latter is disabled by default (docs, experimental_use_rocksdb). The former is disabled in ClickHouse Cloud but of course enabled in ClickHouse OSS.

I am really not against aforementioned compatibility settings which would enable a rollback but from our POV, such settings are not a priority. I'll continue upgrading RocksDB (to 8.9.1 in this PR, to the latest and greatest 9.5 in a follow-up PR). Feel free to submit compatibility settings separately on top (basically steal the corresponding bits from your PR) if that is important for you or you think others might benefit - I'll happily review and merge.

@canhld94 We had some internal discussion based on above compatibility concerns. Turns out that there are two places in ClickHouse which use RocksDB: The EmbeddedRocksDB table engine, and RocksDB as a experimental backend of Keeper. The latter is disabled by default (docs, experimental_use_rocksdb). The former is disabled in ClickHouse Cloud but of course enabled in ClickHouse OSS.

I am really not against aforementioned compatibility settings which would enable a rollback but from our POV, such settings are not a priority. I'll continue upgrading RocksDB (to 8.9.1 in this PR, to the latest and greatest 9.5 in a follow-up PR). Feel free to submit compatibility settings separately on top (basically steal the corresponding bits from your PR) if that is important for you or you think others might benefit - I'll happily review and merge.

@rschu1ze for us it's not a big problem anymore, as we've verified the new version is good and certainly won't rollback (Btw in #67274 I just hardcode it, not exposing any setting).
Also you can take a look at the CMakelist file in #67274, besides refactoring, it also adds new feat (enables io_uring, using jemalloc...). Or may be I can submit it in a separate PR after you done upgrading?

@canhld94 Alright, thanks for the info. Yes, I'll definitely look at your CMakeLists for sure and most likely steal some pieces 😉. (So far, already stole your changes in StorageEmbeddedRocksDB.cpp about the new signature of GetBlockBasedTableOptionsFromMap.)

Let me proceed with the upgrade to latest RocksDB for now. Once done, we can enable the newer features you mentioned.