Disable backward scan in stress test when separate_key_value_in_data_block is enabled

[anand1976]

Summary: When separate_key_value_in_data_block=true, the data block iterator stores keys and values in separate sections. Under certain conditions during backward iteration (Prev/SeekForPrev), DBIter::FindValueForCurrentKey encounters entries whose values cannot be pinned, causing a NotSupported error: "Backward iteration not supported if underlying iterator's value cannot be pinned." This manifests in the crash test as TestIterateAgainstExpected failures when the random flag combination includes separate_key_value_in_data_block=1 (added in 47cfeb656ff3) with test_backward_scan=1 (the default). The fix automatically disables test_backward_scan when separate_key_value_in_data_block is enabled, matching the pattern used for other option incompatibilities in the stress tool.

Differential Revision: D109837793

[meta-codesync]

@anand1976 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109837793.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 18.2s.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit ab91fee

---

❌ 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 ab91fee

---

Summary

This is a correct, minimal fix for a known crash in the stress test. The change disables backward scanning when separate_key_value_in_data_block is enabled, matching an existing pattern for incompatible flag overrides. The root cause (value pinning not available during backward iteration with separated KV blocks) is well-understood and the guard is appropriately placed.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Silent override without log message — db_stress_tool.cc:491

• Issue: The flag override is completely silent. When separate_key_value_in_data_block=true and test_backward_scan=true are both set (which happens frequently since separate_key_value_in_data_block is randomly enabled 2/3 of the time in db_crashtest.py:313 and test_backward_scan defaults to true), backward scan is silently disabled with no indication to the user.
• Root cause: The existing pattern at line 347 (FLAGS_atomic_flush = true) is also silent, so this is consistent. However, most other incompatibility checks in this function print a message (via fprintf(stderr, ...)) before either exiting or adjusting.
• Suggested fix: Consider adding fprintf(stdout, "WARNING: ...backward scan disabled...\n") before the override, similar to the warning at line 377. This is a minor suggestion — the silent override is acceptable given precedent.

🟢 LOW / NIT

L1. Comment accuracy is good but could note the pinning mechanism — db_stress_tool.cc:493-496

• Issue: The comment accurately explains the high-level reason. For completeness, the mechanism is: BlockBasedTableIterator::IsValuePinned() (block_based_table_iterator.h:186-193) returns false when pinned_iters_mgr_ is not set or pinning is not enabled, and DBIter::FindValueForCurrentKey() (db_iter.cc:1244-1256) requires pinned values during backward iteration. With separated KV, the value slice points into the values section of the block, and this is only pinned when block contents are cached or from an immortal source.
• Suggested fix: No change needed — the existing comment is sufficient for maintainability.

L2. No dedicated test for the flag interaction — db_stress_tool.cc:491

• Issue: There is no unit test verifying that the flag override happens correctly. However, similar flag overrides (e.g., FLAGS_atomic_flush at line 347) also lack dedicated tests. This is a stress test configuration guard, not production logic, so the risk is low.
• Suggested fix: No test needed for this trivial guard.

Cross-Component Analysis

Context	Affected?	Assessment
Production code (db engine)	NO	Change is stress-test-only
Public API	NO	No API changes
db_crashtest.py	Indirectly	Python does not set test_backward_scan; relies on C++ guard
Other table formats (plain, cuckoo)	NO	Only block-based tables use separate_key_value_in_data_block
inplace_update_support	Already handled	Has its own backward scan incompatibility check elsewhere

Assumption stress-test:

• Claim: "the block-based table iterator cannot pin values" when separate_key_value_in_data_block=true.
• Verification: BlockIter::IsValuePinned() (block.h:441) returns block_contents_pinned_, which CAN be true when blocks are cached. However, BlockBasedTableIterator::IsValuePinned() (block_based_table_iterator.h:186-193) additionally requires pinned_iters_mgr_ to be set and enabled. During normal iteration in the stress test, pinned_iters_mgr_ may not always be configured for backward iteration, causing the NotSupported error. The guard is correct — the incompatibility exists even though it may not trigger 100% of the time.

Positive Observations

• The fix follows the established pattern for flag incompatibility handling in the stress tool (line 347).
• The comment clearly explains the technical reason for the incompatibility.
• The placement is correct — after the compaction filter check and before multi-ops txn setup, consistent with the ordering of other flag checks.
• The guard is minimal and low-risk — it only affects stress test behavior, not the database engine.

---

ℹ️ 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

[meta-codesync]

This pull request has been merged in ffb7788.