WBM policy changes (#15047)

[rban1]

Summary: Pull Request resolved: #15047

Differential Revision: D112396106

[meta-codesync]

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

[github-actions]

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

Completed in 319.0s.

Summary by check

Check	Count
modernize-make-shared	3
Total	3

Details

db/db_write_buffer_manager_test.cc (3 warning(s))

db/db_write_buffer_manager_test.cc:862:32: warning: use std::make_shared instead [modernize-make-shared]
db/db_write_buffer_manager_test.cc:958:32: warning: use std::make_shared instead [modernize-make-shared]
db/db_write_buffer_manager_test.cc:1005:32: warning: use std::make_shared instead [modernize-make-shared]

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit bce48da

---

Summary

Well-structured PR adding two new WriteBufferManager flush policies. Lock ordering, shutdown sequencing, and background job lifecycle are handled correctly. The core design of a FlushInitiator registry for cross-DB coordination is sound.

High-severity findings (2):

• [write_buffer_manager.cc:209] InitiateFlushOnLargestDB can select a DB with 0 flushable memory when all other DBs are unflushable, causing the caller to defer its own flush while no useful flush occurs, delaying memory relief.
• [db_impl_write.cc:2142] When kFlushLargestAcrossDBs defers to another DB, the writing DB skips its local flush entirely, but ShouldFlush() remains true; repeated deferrals to a 0-memory DB could prevent memory from being reclaimed.

Full review (click to expand)

Findings

🔴 HIGH

H1. InitiateFlushOnLargestDB can defer to a DB with zero flushable memory -- write_buffer_manager.cc:209

• Issue: The selection logic picks the first non-self DB even if best_mem == 0. When all other registered DBs have no flushable memtables (e.g., read-only, immutables in flight, or empty mutable memtables), the function returns true, causing the calling DB to skip its own flush. Meanwhile the target DB's BackgroundCallWBMFlush finds nothing to flush, accomplishing nothing.
• Root cause: The best_mem > 0 check is missing. The initial best = nullptr condition causes the first initiator in the list to be selected regardless of its memory.
• Suggested fix: Add a check before returning true:

  if (best == nullptr || best == self || best_mem == 0) {
    return false;
  }

H2. Repeated deferral without progress when no other DB has flushable memory -- db_impl_write.cc:2142

• Issue: Downstream consequence of H1. When InitiateFlushOnLargestDB returns true but no useful flush occurs, the calling DB's PreprocessWrite skips its local flush. ShouldFlush() remains true, so the next writer defers again, and memory keeps growing.
• Suggested fix: Fixing H1 resolves this.

🟡 MEDIUM

M1. WBM destructor does not assert flush_initiators_ is empty -- write_buffer_manager.cc:41

• Issue: The destructor asserts queue_.empty() but not flush_initiators_.empty(). If a DB is destroyed without calling DeregisterFlushInitiator, dangling pointers remain.
• Suggested fix: Add assert(flush_initiators_.empty()) under #ifndef NDEBUG.

M2. FlushInitiator exposed in public header -- write_buffer_manager.h:58

• Issue: FlushInitiator is "internal use only" but in the public header, adding API maintenance burden. Same for RegisterFlushInitiator, DeregisterFlushInitiator, InitiateFlushOnLargestDB.
• Suggested fix: Consider an internal header or explicit "unstable" documentation.

M3. SetFlushPolicy allows runtime change without coordination -- write_buffer_manager.h:152

• Issue: Changing to kFlushLargestAcrossDBs at runtime when DBs may not have registered their FlushInitiator could result in InitiateFlushOnLargestDB seeing an empty registry and always returning false.
• Suggested fix: Document the constraint or add a registry-empty check.

M4. InitiateFlushOnLargestDB contention with many DBs -- write_buffer_manager.cc:199

• Issue: Holds flush_initiators_mu_ while acquiring each DB's mutex_ in turn. O(N) mutex acquisitions blocking all registration/deregistration.
• Suggested fix: Acceptable for small N; document the scaling limitation.

M5. Error from FlushMemTable silently discarded -- db_impl_compaction_flush.cc:4049

• Issue: s.PermitUncheckedError() without logging makes debugging harder.
• Suggested fix: Log the error before permitting.

M6. Test coverage gaps

• Issue: No tests for: self-is-largest fallback, shutdown race, SetFlushPolicy() runtime change, atomic_flush exclusion, 0-memory deferral, stress test integration.

🟢 LOW / NIT

L1. virtual ~FlushInitiator() {} should be = default

L2. FlushedDBRecorder could use std::unordered_set<DB*>

L3. Empty else branch comment could be clearer about intentional no-op

L4. Registration happens unconditionally regardless of policy (harmless)

Cross-Component Analysis

Context	Assumptions hold?	Action needed?
ReadOnly DB	YES -- returns 0 from GetFlushableMemUsage	Safe
atomic_flush	YES -- returns 0, ScheduleWBMFlush bails out	Safe
WritePreparedTxnDB	YES -- CF selection orthogonal to txn	Safe
MemPurge	YES -- FlushMemTable handles internally	Safe
FIFO/Universal	YES -- compaction style irrelevant	Safe
User-defined timestamps	YES -- orthogonal	Safe
allow_concurrent_memtable_write	YES -- reads under mutex_	Safe

Positive Observations

• Lock ordering is well-designed: flush_initiators_mu_ -> mutex_ consistently followed. No deadlock risk.
• Coalescing is effective: One outstanding WBM flush per DB prevents thundering herd.
• Shutdown sequence is correct: Deregistration before teardown, wait loop includes bg_wbm_flush_scheduled_.
• UnSchedule tag compatibility: this tag means existing CloseHelper UnSchedule covers WBM tasks.
• CFD lifetime management: Correct Ref/Unref across mutex release in BackgroundCallWBMFlush.

---

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