[C API] add rocksdb_set_db_options for dynamic DB option updates

[mjc2-stripe]

Context

The C API lacks a SetDBOptions wrapper, so callers using the C bindings cannot dynamically reconfigure DB-level options at runtime without dropping down to the C++ API.

Changes

• Add rocksdb_set_db_options to the C API as a wrapper around DB::SetDBOptions
• Adds new tests in c_test.c covering both valid and invalid option updates

[meta-codesync]

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D101010593.

[github-actions]

⚠️ clang-tidy: 25 warning(s) on changed lines

Completed in 6805.2s.

Summary by check

Check	Count
bugprone-argument-comment	2
cert-err58-cpp	2
clang-analyzer-optin.core.EnumCastOutOfRange	4
concurrency-mt-unsafe	9
cppcoreguidelines-avoid-non-const-global-variables	5
cppcoreguidelines-pro-type-member-init	2
cppcoreguidelines-special-member-functions	1
Total	25

Details

db/blob/db_blob_direct_write_test.cc (1 warning(s))

db/blob/db_blob_direct_write_test.cc:127:7: warning: class 'ActiveBlobVisibilityWritableFile' defines a non-default destructor but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

db/db_wal_test.cc (1 warning(s))

db/db_wal_test.cc:117:5: warning: uninitialized record type: 'fs_stat' [cppcoreguidelines-pro-type-member-init]

db_stress_tool/db_stress_common.cc (4 warning(s))

db_stress_tool/db_stress_common.cc:22:25: warning: variable 'raw_env' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:22:25: warning: variable 'raw_env' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:25:56: warning: variable 'wbm' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_common.cc:26:49: warning: variable 'rate_limiter' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

db_stress_tool/db_stress_test_base.cc (6 warning(s))

db_stress_tool/db_stress_test_base.cc:380:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4152:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4693:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4727:11: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:4766:11: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_test_base.cc:5500:7: warning: function is not thread safe [concurrency-mt-unsafe]

db_stress_tool/db_stress_tool.cc (4 warning(s))

db_stress_tool/db_stress_tool.cc:41:5: warning: variable 'fault_fs_for_crash_report' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
db_stress_tool/db_stress_tool.cc:402:7: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_tool.cc:416:9: warning: function is not thread safe [concurrency-mt-unsafe]
db_stress_tool/db_stress_tool.cc:448:9: warning: function is not thread safe [concurrency-mt-unsafe]

file/random_access_file_reader_test.cc (1 warning(s))

file/random_access_file_reader_test.cc:114:21: warning: argument name 'direct_io_buffer' in comment does not match parameter name 'direct_io_buffer_context' [bugprone-argument-comment]

include/rocksdb/file_system.h (4 warning(s))

include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]

table/block_fetcher.cc (1 warning(s))

table/block_fetcher.cc:359:23: warning: argument name 'direct_io_buffer' in comment does not match parameter name 'direct_io_buffer_context' [bugprone-argument-comment]

util/coding_test.cc (3 warning(s))

util/coding_test.cc:220:8: warning: constructor does not initialize these fields: length [cppcoreguidelines-pro-type-member-init]
util/coding_test.cc:371:38: warning: initialization of 'kPrefixVarint32TestCases' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
util/coding_test.cc:386:38: warning: initialization of 'kPrefixVarint64TestCases' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]

[mjc2-stripe]

Hey @jaykorean! Could you please take a look at this when you get the chance? Thanks!

[hemal-stripe]

why not have an iteration on the length of keys?

[mjc2-stripe]

The arrays passed in are just pointers, so there's no quick way to get the length of the array.

[hemal-stripe]

potentially index out of bounds exception here - assertions required to check the length of both keys and values are same.

[mjc2-stripe]

Same issue here, where there's no quick way to get the length of the array

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 5321f5f

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 5321f5f

---

Summary

Clean, well-structured C API addition that correctly wraps DB::SetDBOptions following the established pattern of rocksdb_set_options. One naming inconsistency to address.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Parameter naming inconsistency — c.cc / c.h

• Issue: The new function uses opt_keys[] and opt_values[] as parameter names, while the sibling functions rocksdb_set_options and rocksdb_set_options_cf use keys[] and values[]. This inconsistency appears in both the header declaration (c.h) and the implementation (c.cc).
• Root cause: Likely unintentional naming divergence.
• Suggested fix: Rename opt_keys to keys and opt_values to values to match the existing convention:

  // c.h
  extern ROCKSDB_LIBRARY_API void rocksdb_set_db_options(
      rocksdb_t* db, int count, const char* const keys[],
      const char* const values[], char** errptr);
  
  // c.cc
  void rocksdb_set_db_options(rocksdb_t* db, int count,
                              const char* const keys[],
                              const char* const values[], char** errptr) {

🟢 LOW / NIT

L1. Test does not verify option took effect — c_test.c

• Issue: The test confirms rocksdb_set_db_options returns success for a valid option and error for an invalid one, but does not verify the option was actually applied. Other C API tests typically follow a set-then-get pattern.
• Suggested fix: After setting stats_dump_period_sec, query the option via rocksdb_options_get_stats_dump_period_sec or rocksdb_property_value to confirm it took effect. This is a nice-to-have, not a blocker — the existing rocksdb_set_options test also lacks this verification.

L2. Test could cover multiple options in a single call — c_test.c

• Issue: The test only exercises count=1. Testing with count > 1 would exercise the loop and map-building logic more thoroughly.
• Suggested fix: Add a test case setting two valid DB options simultaneously.

Cross-Component Analysis

Context	Supported?	Notes
Standard DB	Yes	Full support via DBImpl::SetDBOptions
StackableDB	Yes	Forwards to underlying DB
Secondary DB	Returns NotSupported	Expected behavior
Read-Only DB	Inherits base impl	May succeed or fail depending on option

The function is purely additive (no breaking changes) and follows the exact implementation pattern of rocksdb_set_options. No symbol export changes needed beyond the standard ROCKSDB_LIBRARY_API macro already present. The Java API already has an equivalent setDBOptions binding, confirming the approach is well-established.

Positive Observations

• Correct use of SaveError for error propagation
• Proper placement in c.h between related functions (rocksdb_set_options and rocksdb_set_options_cf)
• Test covers both success and error paths
• Implementation is a clean, minimal wrapper with no unnecessary complexity

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[xingbowang]

Could you fix the naming inconsistency?

M1. Parameter naming inconsistency — c.cc / c.h
Issue: The new function uses opt_keys[] and opt_values[] as parameter names, while the sibling functions rocksdb_set_options and rocksdb_set_options_cf use keys[] and values[]. This inconsistency appears in both the header declaration (c.h) and the implementation (c.cc).
Root cause: Likely unintentional naming divergence.
Suggested fix: Rename opt_keys to keys and opt_values to values to match the existing convention:
// c.h
extern ROCKSDB_LIBRARY_API void rocksdb_set_db_options(
rocksdb_t* db, int count, const char* const keys[],
const char* const values[], char** errptr);

// c.cc
void rocksdb_set_db_options(rocksdb_t* db, int count,
const char* const keys[],
const char* const values[], char** errptr) {

[xingbowang]

Please fix format

[eringao-stripe]

@xingbowang could you ptal when you get a chance? thanks in advance :-)

[meta-codesync]

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D101010593.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 9a95475

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 9a95475

---

Summary

Clean, low-risk PR that adds a missing C API wrapper (rocksdb_set_db_options) for an existing C++ API (DB::SetDBOptions). The implementation exactly mirrors the established rocksdb_set_options pattern. Test covers both success and error paths.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Missing unreleased_history entry — unreleased_history/public_api_changes/

• Issue: New public C API functions should have an entry in unreleased_history/public_api_changes/ per RocksDB conventions. Recent C API additions (e.g., memtable_batch_lookup_optimization, optimize_multiget_for_io) all have corresponding entries that end up in HISTORY.md at release time.
• Suggested fix: Add a file like unreleased_history/public_api_changes/set_db_options_c_api.md with a one-line description, e.g.: Added rocksdb_set_db_options() to the C API, wrapping DB::SetDBOptions() for dynamic DB-level option updates at runtime.

🟢 LOW / NIT

L1. No documentation comment on the C header declaration — include/rocksdb/c.h

• Issue: The new declaration has no comment explaining its purpose or how it differs from rocksdb_set_options (which sets CF-level options). The sibling rocksdb_set_options also lacks a comment, so this is consistent, but a brief note distinguishing DB-level vs CF-level options would help C API users.
• Suggested fix: Consider adding a one-line comment above the declaration: /* Dynamically update DB-level options (as opposed to CF-level via rocksdb_set_options). */

L2. Test placement — db/c_test.c

• Issue: The set_db_options test phase is placed between the put phase and backup_and_restore phase. This works but grouping it near the existing set_options test (if one exists) would be more logical. Minor nit — current placement is functional.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (via StackableDB)	YES	safe
ReadOnly DB	YES (returns error)	YES	safe
SecondaryInstance	YES (returns NotSupported)	YES	safe
User-defined timestamps	N/A (config API)	YES	safe

The function is a pure pass-through wrapper. All validation, locking, and state mutation are handled by the existing DB::SetDBOptions() implementation. No new cross-component interactions are introduced.

Positive Observations

• Implementation is an exact structural match of the established rocksdb_set_options() pattern — zero deviation
• Test covers both success path (valid option stats_dump_period_sec) and error path (invalid option not_a_db_option)
• Header declaration is correctly placed between rocksdb_set_options and rocksdb_set_options_cf, maintaining logical grouping
• Function signature matches the established convention exactly

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase