Skip to content

Fix commit map remove notfound#6074

Open
markhannum wants to merge 2 commits into
bloomberg:mainfrom
markhannum:fix-commit-map-remove-notfound
Open

Fix commit map remove notfound#6074
markhannum wants to merge 2 commits into
bloomberg:mainfrom
markhannum:fix-commit-map-remove-notfound

Conversation

@markhannum

Copy link
Copy Markdown
Contributor

Tolerate commit-LSN map entries missing during recovery undo

Fixes DRQS 185164072.

Problem

A physical replicant repeatedly failed to truncate its transaction log, aborting recovery's backward pass with:

__txn_commit_map_remove_nolock: Could not find transaction 9526228882 in the map
__txn_regop_gen_recover: Failed to remove 9526228882 from the commit map
Recovery function for LSN 539110 19439111 failed on backward pass

Root cause

The commit-LSN map is a windowed, in-memory cache, not a complete index of every commit: it is pruned as log files are deleted, is rebuilt only from the last checkpoint forward on restart, and (on a physical replicant) need not span a log range being rewound/truncated. So a committed transaction that recovery's backward pass walks over may legitimately be absent from the map.

The undo path (__txn_dist_commit_recover, __txn_regop_gen_recover, __txn_regop_recover, __txn_regop_rowlocks_recover) called __txn_commit_map_remove and treated "not found" as a fatal error, which aborted the whole backward pass.

This became reachable when snapshot isolation was made the default. The commit-map machinery used to be gated by get_commit_lsn_map_switch_value() — which folded in the now-removed gbl_snapisol/gbl_modsnap flags and evaluated to 0 on a physrep without enable_snapshot_isolation. It is now gated only by gbl_utxnid_log (on by default), so the removal path runs where it previously did not.

Fix

  • __txn_commit_map_remove_nolock returns DB_NOTFOUND (was 1) on a miss and bumps gbl_commit_map_remove_miss so the condition is still observable — surfaced in bdb clminfo as "Remove misses: N". The per-miss WARN is rate-limited (once/sec), since a large rewind can miss many entries in a row.
  • The four recovery undo sites swallow DB_NOTFOUND to 0 and fall through to record the undo, so the backward pass continues. Genuine errors remain fatal. The swallow must happen in the recover function: returning DB_NOTFOUND up to the backward-pass loop would itself be treated as a failure.
  • __txn_child_recover is deliberately left strict — it only removes after a guarding __txn_commit_map_get, so a miss there would indicate a real inconsistency.

Test

Adds test_recover_to_lsn_missing_map_entries to the commit_lsn_map_delete_generated variant. It reproduces the condition deterministically without a physrep: run committed traffic, capture an early recovery LSN, generate more traffic spanning higher logfiles, use clm_delete_logfile to drop a logfile's entries from the in-memory map while leaving the log records intact, then truncate_log back across them. It asserts the node stays healthy, pre-recovery state survives (do_verify tmiss), and that the miss path was actually exercised (the clminfo counter increased). Fails on the pre-fix binary.

markhannum and others added 2 commits July 16, 2026 10:40
The commit-LSN map is a windowed, in-memory cache: it is pruned as log
files are deleted, is rebuilt only from the last checkpoint forward on
restart, and (on physical replicants) need not span a log range that is
being rewound / truncated. It therefore does not contain every committed
transaction that a recovery backward pass may walk over.

Recovery's undo path (records newer than trunc_lsn / past a recovery
timestamp) calls __txn_commit_map_remove and, until now, treated a
"not found" result as a fatal error. That aborted the entire backward
pass -- e.g. a physical replicant truncating its txn log would fail with
"Could not find transaction N in the map" and be unable to make progress.

This became reachable because snapshot isolation was made the default:
the commit-map machinery used to be gated by get_commit_lsn_map_switch_value()
(which folded in the now-removed gbl_snapisol/gbl_modsnap flags and was 0
on a physrep without enable_snapshot_isolation), and is now gated only by
gbl_utxnid_log (on by default). The recovery add path is guarded by
opcode == TXN_COMMIT, so the map only ever holds commits; the removal path
had no matching tolerance for entries that were legitimately never present.

Make a missing entry benign:
 - __txn_commit_map_remove_nolock now returns DB_NOTFOUND (was 1) on a
   miss, logs at WARN (was ERROR), and bumps gbl_commit_map_remove_miss
   so a real map-population regression is still observable. The counter is
   surfaced in 'bdb clminfo' as "Remove misses: N".
 - The four recovery undo sites swallow DB_NOTFOUND to 0 and fall through
   to record the undo, so the backward pass continues. Genuine errors are
   still fatal. (The swallow must happen here: returning DB_NOTFOUND up to
   the backward-pass loop would still be treated as a failure.)

__txn_child_recover is left strict: it only removes after a guarding
__txn_commit_map_get, so a miss there would indicate a real inconsistency.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Mark Hannum <mhannum@bloomberg.net>
Add a regression test for the commit-LSN map "not found on remove" fix.
It reproduces the exact failing condition deterministically, without
needing a physical replicant:

 - run committed traffic and capture an early recovery lsn (post-flush),
 - generate more committed traffic spanning higher logfiles (the range a
   recover-to-lsn will undo),
 - use clm_delete_logfile to drop a logfile's entries from the in-memory
   map while leaving the actual log records intact -- so the backward pass
   still walks those committed txns but they are absent from the map,
 - truncate_log back to the early lsn.

Before the fix the backward pass aborted ("Could not find transaction N
in the map"); now it must complete. The test asserts the node stays
healthy, the pre-recovery-lsn state survives (undone traffic is gone), and
that the miss path was actually exercised (via the "Remove misses" counter
reported by 'bdb clminfo').

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Mark Hannum <mhannum@bloomberg.net>

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
ssl_san
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**
sql_logfill_autodisable [timeout]

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
reco-ddlk-sql **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
sc_resume [timeout]

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
truncatesc_offline_generated **quarantined**
sc_resume_logicalsc_generated **quarantined**
noresetgen
reco-ddlk-sql **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cbuild submission: Success ✓.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
truncatesc_offline_generated [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants