Drop flushed WAL data from the secondary's active memtable - #15066
Drop flushed WAL data from the secondary's active memtable#15066andpred wants to merge 7 commits into
Conversation
## Summary Fix facebook#15051: a secondary serves stale reads because entries replayed from a WAL the primary has since flushed are never evicted from the active memtable. During TryCatchUpWithPrimary(), seal the active memtable once the installed Version provably covers everything it holds, so the following RemoveOldMemTables() call drops it. Applies to the follower as well. ## Why and How A secondary replays the primary's WALs into its active memtable and stops replaying a WAL once the primary flushes it. Entries already replayed stayed behind, and since reads consult the memtable first, they shadowed the newer flushed values indefinitely. Iterators go stale too once bottommost compaction rewrites the flushed entry's sequence number to 0; until then Get() and an iterator disagree. Seal the active memtable during catch-up, gated on the installed Version: each point-in-time Version now carries the column family's log number it reflects, exposed via the new ReactiveVersionSet::GetInstalledVersionLogNumber(). Sealing requires cfd_to_current_log_[cfd] < that number, which guarantees the primary flushed everything the memtable holds (unflushed WAL data is kept) and that the flushed data is readable through the installed Version. The gate asks the Version directly because proxies fail: the log number alone advances even when no Version was installed, and "no missing files" misses atomic_flush groups parked by a sibling's missing file and corrupt-but-present files with verify_sst_unique_id_in_manifest=false. In those cases the memtable holds the only readable copy, so dropping it would turn a stale read into a vanished key; the gate fails closed and keeps it. Reconcile every initialized column family, not just cfds_changed: a round that failed after advancing the log number leaves a stale memtable that later rounds see no new MANIFEST record for. It can also leave a collectible immutable memtable, so the loop consults the new O(1) MemTableList::HasOldMemTablesToRemove() before skipping a column family. The predicate keeps the skip precise: reconciling any column family with a non-empty immutable list would be correct but wasteful, installing a fresh SuperVersion every catch-up call for memtables legitimately waiting on a primary flush and costing readers a mutex acquisition each time. With it, no extra super versions are installed in the steady state. The seal sequence, shared with RecoverLogFiles() and the follower's TryCatchUpWithLeader(), is extracted into DBImplSecondary::SealActiveMemtable(). ## Testing New tests in db_secondary_test cover: the stale read via both Get() and an iterator, before and after bottommost compaction; unflushed WAL-only data surviving sealing, including two column families sharing a WAL; no data loss with a missing, atomic_flush-sibling-missing, or corrupt flushed file; and reconciliation after a failed round (injected via FaultInjectionTestFS at a sync point). Each mechanism was individually neutralized and confirmed to fail its test. Verified with build_tools/rocksptest.sh over db_secondary_test, db_follower_test, memtable_list_test, version_set_test and version_edit_test (all pass, also under ASSERT_STATUS_CHECKED=1), plus db_basic_test, corruption_test, repair_test and db_flush_test; the secondary tests also ran 15x under COERCE_CONTEXT_SWITCH=1 with no flakes. make format-auto and make check-sources are clean.
|
| Check | Count |
|---|---|
cppcoreguidelines-special-member-functions |
1 |
| Total | 1 |
Details
db/db_secondary_test.cc (1 warning(s))
db/db_secondary_test.cc:1100:7: warning: class 'DBSecondaryCatchUpFaultTest' defines a non-default destructor but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit f3731c1 SummaryThis is a well-designed, carefully gated fix for a real correctness bug (stale reads on secondary instances). The two-level gate (cheap log-number check + authoritative installed-Version check) is the right approach, and the test coverage is thorough. The High-severity findings (0): No high-severity findings. Full review (click to expand)Findings🔴 HIGHNone. 🟡 MEDIUMM1. Follower path not updated to use
|
| Context | Does code execute? | Assumptions hold? | Action needed? |
|---|---|---|---|
| DBImplFollower | YES (SealActiveMemtable only) | YES | None (no WAL replay) |
| Atomic flush | YES | YES (tested) | None |
| Missing files | YES | YES (tested, gate fails closed) | None |
| Corrupt files | YES | YES (tested, gate fails closed) | None |
| Multiple CFs sharing WAL | YES | YES (tested) | None |
| CF dropped during catch-up | YES | Skipped (IsDropped() check) |
None |
| Failed round then recovery | YES | YES (tested with FaultInjectionTestFS) | None |
| WritePreparedTxnDB | NO (secondary doesn't support txns) | N/A | N/A |
| User-defined timestamps | Possible | Change doesn't affect key comparison | Safe |
Assumption stress-test results:
-
"installed Version covers everything the memtable holds" --
installed_log_numberis set atAppendVersiontime from the log number captured at Version build time. Cannot be ahead of what's actually installed. Holds. -
"dropping it would turn a stale read into a vanished key" -- When the installed Version doesn't cover the flush, the memtable holds the only readable copy (WAL is obsolete, SST not in installed Version). Holds.
-
assert(installed_log_number <= cfd->GetLogNumber())-- The installed log number is set from a past value ofGetLogNumber(), which only increases monotonically. Safe.
Positive Observations
- Two-level gate design is elegant: the cheap check filters most CFs in O(1), the authoritative check only runs for the rare flush case.
- Fail-closed design: When in doubt, the gate keeps the memtable -- correct default for data safety.
- Thorough test coverage: Missing file, corrupt file, atomic flush sibling missing, failed round -- each individually tested with both Get() and iterator verification.
- Clean refactoring:
SealActiveMemtablededuplicates code from three call sites. HasOldMemTablesToRemoveO(1) optimization avoids unnecessary SuperVersion installations.- Release note is appropriately concise.
ℹ️ 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 has imported this pull request. If you are a Meta employee, you can view this in D115317523. |
|
Fix looks good. Thanks for the contribution. We would love to hear about how you use this in production. |
| // switch can also leave a collectible immutable memtable behind, so | ||
| // check for one instead of relying on this round having sealed. | ||
| if (!sealed && | ||
| !cfd->imm()->HasOldMemTablesToRemove(cfd->GetLogNumber()) && |
There was a problem hiding this comment.
We want to use GetInstalledVersionLogNumber() for the cut off, otherwise Immutable memtables can be removed using manifest-advanced cfd->GetLogNumber(), even when the installed Version watermark remains lower.
Here's am example scenario
WAL 100:
X = 1
Y = 1
WAL 101:
X = 2
1. Secondary replays WAL 100:
active M100: {X=1, Y=1}
2. Secondary starts replaying WAL 101. It seals M100 and creates M101:
immutable M100: {X=1, Y=1}, next_log=101
active M101: {X=2}
3. Primary flushes WAL 100 into SST S1 and advances the MANIFEST log number to 101. S1 is unavailable before the secondary opens it (due to a file system issue, or being compacted away by another compaction, etc.):
cfd->GetLogNumber() = 101
installed Version log number = 100
4. A catch-up failure skips cleanup. On the next retry, the new reconciliation code:
- Correctly keeps active M101.
- Incorrectly removes immutable M100 using cfd->GetLogNumber() == 101.
The resulting secondary view is:
X = 2 // still available from M101
Y = missing // M100 was its only readable copy
There was a problem hiding this comment.
Good catch, fixed. HasOldMemTablesToRemove() and RemoveOldMemTables() now take GetInstalledVersionLogNumber() like the seal gate does, and the sealed memtable is stamped with cf_id_to_current_log_[cfd->GetID()] + 1 so it is still collected in the same round. KeepsImmutableMemtableWhenFlushedFileIsMissing builds your scenario and fails with NotFound when the cutoff is reverted.
Chasing it turned up a second way that gate could free a memtable the installed Version does not cover, so the branch is now five commits:
5c115fb4dthe fix above and its test. This addresses your comment.5a614575bthe gate readscf_id_to_current_log_, which was keyed by aColumnFamilyData*that can dangle and be recycled, and was recorded only after a successfulInsertInto(), so a batch failing partway left it naming an older WAL than the memtable held. Now keyed by id and recorded before the insert.KeepsMemtableAfterPartialReplayFailurecovers it.0660ee78dstale comments, plus three tests that leaked a moved-aside SST when an earlier assertion failed.a52f0a9b4warns while the watermark lags and uncollectible memtables accumulate. Droppable if you would rather not carry it.6f4dd0365merge with main. Only conflict was two tests appended at the same line ofmemtable_list_test.cc; kept both.
Review feedback on facebook#15066: TryCatchUpWithPrimary() gated sealing the active memtable on the installed Version's log number, but still freed immutable memtables against cfd->GetLogNumber(), through HasOldMemTablesToRemove() and RemoveOldMemTables(). That number advances as soon as the primary's flush record is read, even when the flushed files cannot be opened, so a memtable holding the only readable copy of what was flushed can be freed, turning a stale read into a vanished key. A memtable sealed on a WAL switch is stamped with the new WAL's number, so the cutoff reaches it immediately. Compute GetInstalledVersionLogNumber() once per column family and use it for the seal gate, the removal predicate and the removal cutoff, and seal with one past the WAL recorded for the column family, the log number following the memtable's contents. The installed number differs from cfd->GetLogNumber() only when a Version fails to install, so a secondary whose files are all readable behaves exactly as before. Keeping a memtable costs memory that nothing on a secondary reclaims until the primary's files become readable or the instance is reopened, so document that with TryCatchUpWithPrimary(). Correct the comments the fix falsifies or leaves imprecise: RemoveOldMemTables() collects a prefix, stopping at the first memtable that does not qualify, and the cutoff is no longer cfd->GetLogNumber(). Extend the release note, since the vanished key predates facebook#15066. New test KeepsImmutableMemtableWhenFlushedFileIsMissing covers the reported scenario and fails when the cutoff is reverted.
MaybeSealFullyFlushedActiveMemtable() now decides from the map of WALs replayed per column family whether a memtable can be freed, so all of its weaknesses matter. Key it by column family id rather than by ColumnFamilyData*, and rename it to cf_id_to_current_log_ to say so. A dropped column family's ColumnFamilyData is destroyed while its entry remains, and the allocator can hand the same address to a new column family, which would then find the old entry and a log number that has nothing to do with it. Ids come from ++max_column_family_ and are never reused. Erase the entry when the reconciliation loop sees the column family dropped, which is the only removal path it has ever had; that leaves behind an entry for a column family destroyed before the loop next runs, which with id keys wastes a few bytes and cannot be misread. Record the WAL before inserting from it, not after. The gate needs the recorded value to be no older than the newest WAL the active memtable holds, and a batch stops at its first failure with earlier entries already inserted, so recording afterwards left the map behind the memtable. Recording first can only overstate, which fails the gate for longer and merely retains the memtable. The header called this unreachable because the failure poisons LogReaderContainer::status_, but FindNewLogNumbers() discards that reader once the WAL goes obsolete, so a later round can reach the gate with the map still stale. Record in the loop that already walks the batch's column families rather than in a second one: a column family's entry only feeds its own gate, so recording just before the seal keeps the ordering the gate needs while dropping a repeated column family lookup per batch. New test KeepsMemtableAfterPartialReplayFailure drives the partial failure by rejecting memtable writes to a second column family on the secondary only, and fails with NotFound when the update is moved back after the insert.
Correct comments that overstated an invariant, named the wrong thing or leaked a caller's rationale: l0_files.back() is the oldest L0 file; ExtractInfoFromVersionEdit(), not ApplyVersionEdit(), applies an edit's log number; AtomicUpdateVersionsPut() and AtomicUpdateVersionsApply() no longer describe `Version*` updates; HasOldMemTablesToRemove() no longer describes its caller's bookkeeping; and VersionEditHandlerPointInTime::GetInstalledVersionLogNumber() says what the installed Version covers without naming a primary, because that handler also serves best-effort recovery of an ordinary DB, while the ReactiveVersionSet declaration a secondary calls keeps that framing. Document that it is a follower's catch-up path, rather than the class, that makes cfd->GetLogNumber() a safe watermark there: DBImplFollower inherits DBImplSecondary::TryCatchUpWithPrimary(), which does replay WALs. Restore the moved-aside table files with a Defer instead of a trailing rename that a failing assertion skips, which would leave a *.sst.aside file behind in dbname_ for every later test in the fixture. Take the newest table file with a strict comparison, assert the retained memtable in KeepsMemtableWhenAtomicFlushSiblingFileIsMissing, and rename an Options for consistency.
While the installed Version's log number lags behind the column family's, every WAL switch adds an immutable memtable that RemoveOldMemTables() cannot collect, and nothing on a secondary flushes it, so their number grows until the primary's flushed files become readable or the instance is reopened. Report it, naming both log numbers and the retained count. ColumnFamilyData::RecalculateWriteStallConditions() already counts this as a memtable limit stop, but attributes it to a flush that is not coming and to a max_write_buffer_number that no writer of this instance's own can hit, so the warning names the actual cause instead. Warn only when what would be reported has changed. The condition lasts until the primary's files become readable and TryCatchUpWithPrimary() is called as often as the application chooses, so warning per round would bury everything else in the log; both reported values move only as the condition worsens, making this one line per newly retained memtable.
Conflict in db/memtable_list_test.cc: upstream added FlushRequestPersistsAfterPartialPick and this branch added RemoveOldMemTablesTest at the same place. Kept both.
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit 6f4dd03 SummaryWell-designed fix for a real data-correctness bug (stale reads on secondary instances). The approach of gating memtable sealing on the installed Version's log number rather than High-severity findings (0): No high-severity findings. Full review (click to expand)Findings🔴 HIGHNone. 🟡 MEDIUMM1. L0 file ordering comment may mislead future readers --
|
| Context | Affected? | Safe? |
|---|---|---|
| DBImplFollower | YES - refactored | YES - same behavior, uses cfd->GetLogNumber() which is safe because follower never replays WALs |
| WritePreparedTxnDB | NO - secondary doesn't support transactions | N/A |
| ReadOnly DB | NO - no TryCatchUpWithPrimary |
N/A |
| Atomic flush | YES - tested | YES - incomplete group keeps memtables |
| CompactionService | MAYBE - uses OpenAsSecondaryImpl |
YES - sets recover_wal=false, no WAL replay |
| User-defined timestamps | NO - no interaction | N/A |
Assumption Stress Test
Claim: "The installed Version covers everything the memtable holds"
- Precondition:
cf_id_to_current_log_[id] < installed_log_number - Counterexample: WAL replayed into memtable but not recorded in
cf_id_to_current_log_→ impossible, recording happens before insertion - Counterexample:
installed_log_numberadvances past the WAL without a Version being installed → impossible by construction,installed_version_log_numbers_is only set whenAppendVersion()succeeds - Conclusion: Claim holds.
Claim: "Recording WAL before InsertInto is the safe direction"
- Precondition: On partial failure, recorded WAL >= newest WAL in memtable
- Counterexample: Batch inserts entries from WAL N, fails partway, recorded WAL is N → correct, gate keeps memtable until Version covers WAL N
- Conclusion: Claim holds.
Positive Observations
-
Fail-closed design: Every uncertain case keeps the memtable rather than dropping it. This is exactly right for a data-integrity fix.
-
Thorough testing: 10 new test cases covering the main bug, safety properties (missing/corrupt files, atomic flush, partial replay), and the unflushed-WAL preservation. Each test documents which mechanism it exercises.
-
Good separation of concerns:
SealActiveMemtable()extracts a reusable operation.MaybeSealFullyFlushedActiveMemtable()encapsulates the gate logic.MaybeWarnAboutRetainedMemtables()handles monitoring with good deduplication. -
Key change to
uint32_tmap key: Moving fromColumnFamilyData*to CF ID is correct -- ColumnFamilyData pointers can be recycled, but IDs are never reused. -
Recording WAL before InsertInto: On partial failure, the recorded WAL is at least as new as what the memtable holds, which is the safe direction for the seal gate.
-
HasOldMemTablesToRemove()O(1) predicate: Avoids unnecessary super version installations in the common case, keeping the cost of iterating all CFs low. -
Exceptional documentation: The PR description and code comments explain not just what the code does but why each decision was made and what would go wrong with alternatives.
ℹ️ About this response
Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.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 has imported this pull request. If you are a Meta employee, you can view this in D115317523. |
| // merely retains it. Recording before the seal below is equivalent, | ||
| // because a column family's entry only feeds its own gate. | ||
| const auto [log_iter, inserted] = | ||
| cf_id_to_current_log_.insert({id, log_number}); |
There was a problem hiding this comment.
This cf to wal tracking only considers cf ids physically present in the current record. With write-after-commit 2PC, a commit-only record has no ids, but
Line 3099 in 58d835d
WAL 10: Prepare transaction T. T contains Put(X, 2), but X is not inserted yet.
WAL 20: Put(Y, 1) - Primary flushes Y and installs a Version with log watermark 30.
WAL 40: Commit transaction T. This record contains only Commit(T), not Put(X, 2).
On the secondary:
1. Replaying WAL 10 records tracked_log = 10, but only buffers Put(X, 2) as a prepared transaction.
2. WAL 20 may advance tracked_log to 20. Since Y was flushed, the secondary installs the SST Version with: installed_log = 30
3. Replaying WAL 40 calls MarkCommit(T). That internally inserts X=2 into the active memtable.
4. However, the commit record contains no column-family IDs, so tracked_log does not update:
tracked_log remains 20 // should become 40
5. Reconciliation evaluates: tracked_log < installed_log because 20 < 30
It concludes the active memtable is covered by the installed Version and removes it. But X=2 was committed in WAL 40 and has not been flushed, so X disappears from the secondary.
This is just a theory and haven't verified this. Could you add a 2PC regression test to see if the theory here is valid?
There was a problem hiding this comment.
Thanks, you were right. Was able to reproduce it (KeepsMemtableAfterTwoPhaseCommitReplay), the latest commit has the fix.
A 2PC commit record names no column family, yet MarkCommit() replays its prepared batch into the memtables, so cf_id_to_current_log_ stays behind the committed data and the seal gate drops what no file covers yet. Have InsertInto() report the column families it selected for writing, collected in MemTableInserter::SeekToColumnFamily(), and advance the map and cfds_changed from that. New test KeepsMemtableAfterTwoPhaseCommitReplay fails with NotFound without it.
3 participants
Summary
Fix #15051: a secondary serves stale reads because entries replayed from a WAL the primary has since flushed are never evicted from the active memtable. During TryCatchUpWithPrimary(), seal the active memtable once the installed Version provably covers everything it holds, so the following RemoveOldMemTables() call drops it. Every memtable-dropping decision takes that same watermark, so a file set the secondary cannot read never turns the stale read into a vanished key.
Why and How
A secondary replays the primary's WALs into its active memtable and stops replaying a WAL once the primary flushes it. Entries already replayed stayed behind, and since reads consult the memtable first, they shadowed the newer flushed values indefinitely. Iterators go stale too once bottommost compaction rewrites the flushed entry's sequence number to 0; until then Get() and an iterator disagree.
Seal the active memtable during catch-up, gated on the installed Version: each point-in-time Version now carries the column family's log number it reflects, exposed via the new
ReactiveVersionSet::GetInstalledVersionLogNumber(). Sealing requires cf_id_to_current_log_[cfd->GetID()] < that number, which guarantees the primary flushed everything the memtable holds (unflushed WAL data is kept) and that the flushed data is readable through the installed Version. The gate asks the Version directly because proxies fail: the log number alone advances even when no Version was installed, and "no missing files" misses atomic_flush groups parked by a sibling's missing file and corrupt-but-present files with
verify_sst_unique_id_in_manifest=false. In those cases the memtable holds the only readable copy, so dropping it would turn a stale read into a vanished key; the gate fails closed and keeps it.
Reconcile every initialized column family, not just cfds_changed: a round that failed after advancing the log number leaves a stale memtable that later rounds see no new MANIFEST record for. It can also leave a collectible immutable memtable, so the loop consults the new O(1) MemTableList::HasOldMemTablesToRemove() before skipping a column family. The predicate keeps the skip precise: reconciling any column family with a non-empty immutable list would be correct but wasteful, installing a fresh SuperVersion every catch-up call for memtables legitimately waiting on a primary flush and costing readers a mutex acquisition each time. With it, no extra super versions are installed in the steady state. That predicate and the RemoveOldMemTables() cutoff take the installed Version's log number as well, not cfd->GetLogNumber(): that advances as soon as the flush record is read, so an immutable memtable sealed on a WAL switch could otherwise be freed while the flushed file superseding it is unreadable. A sealed memtable is stamped with cf_id_to_current_log_[cfd->GetID()] + 1, the log number following its contents.
cf_id_to_current_log_ is keyed by column family id rather than ColumnFamilyData*, because ids are never reused while a dropped column family's pointer can be recycled, and it is recorded before WriteBatchInternal::InsertInto() rather than after: a batch stops at its first failure with earlier entries already inserted, so recording afterwards could leave the map naming an older WAL than the memtable holds. While the watermark lags and uncollectible memtables accumulate, the secondary warns, since it has no write path to enforce max_write_buffer_number.
The seal sequence, shared with RecoverLogFiles() and the follower's TryCatchUpWithLeader(), is extracted into
DBImplSecondary::SealActiveMemtable(). The follower is refactored onto it with no behaviour change: it keeps cfd->GetLogNumber() as its cutoff because that catch-up path never replays WALs, so its memtables hold nothing that only a not-yet-installed Version could make readable.
Testing
New tests in db_secondary_test cover: the stale read via both Get() and an iterator, before and after bottommost compaction; unflushed WAL-only data surviving sealing, including two column families sharing a WAL; no data loss with a missing, atomic_flush-sibling-missing, or corrupt flushed file; an immutable memtable sealed on a WAL switch surviving an unreadable flushed file; a batch rejected partway through replay leaving the recorded WAL no older than the memtable's contents; and reconciliation after a failed round (injected via FaultInjectionTestFS at a sync point). Each mechanism was individually neutralized and confirmed to fail its test.
Verified with build_tools/rocksptest.sh over db_secondary_test, db_follower_test, memtable_list_test, version_set_test and version_edit_test (all pass, also under ASSERT_STATUS_CHECKED=1), plus db_basic_test, corruption_test, repair_test and db_flush_test; the secondary tests also ran 20x under COERCE_CONTEXT_SWITCH=1 with no flakes. make format-auto and make check-sources are clean.