[emelialei88]

🎯 The "Why" (Intent)

Every master commit takes the trn repo mutex, so all commits run one at a time. Since 048d67f snapshot no longer uses the repo (modsnap keeps its own list); only serializable sessions and logical commits need it. Of ~51k prod databases in c2cfgdb, 49 enable serializable.

🛠️ The "What" (Critical Changes)

• Decide whether the commit writes a logical commit record before locking; take trn_repo_mtx only for logical commits or when serializable is enabled.

• What the lock guards (the log itself is serialized by berkdb's log region lock either way):

  What it guards	Who needs it
  The list of open serializable sessions, so a commit's changes reach every session's shadow copy and no session registers mid-commit	Only when serializable is enabled
  Logical commit record → sc redo list → commit as one unit, so the redo list stays in log order	Only logical commits

  A plain commit needs neither.

• Child (nested) commits never write a logical commit record, so they no longer take it either. The block processor commits a child and then its parent for every request, so the old code took the lock twice per request.

before:  commit ─▶ lock ─▶ [logical record?] ─▶ write commit ─▶ unlock
                   ▲ every commit queues here, one at a time

after:   commit ─▶ logical, or serializable enabled?
                   ├─ yes ─▶ lock ─▶ logical record ─▶ write commit ─▶ unlock   (unchanged)
                   └─ no  ───────────────────────────▶ write commit             (no queue)

🔧 Races this exposes (fixed in their own commits)

Two places relied on the lock making commits run one at a time.

1. The master's election LSN (rep->committed_lsn) could move backwards. It's an in-memory cache of the last commit; once set, an election votes with it and doesn't re-read the log, so a stale value is what gets sent. A commit writes its record (gets an LSN), then saves that LSN for elections under a different lock:

each commit, in __txn_commit_int (berkdb/txn/txn.c):
  ┌─ rep_mutexp ─┐   ┌─── log region lock ────┐              ┌────── rep_mutexp ─────┐
  │ 1. read gen  │ → │ 2. write commit record │ ─ no lock ─▶ │ 3. save that LSN in   │
  └──────────────┘   │    → gets its LSN      │              │    rep->committed_lsn │
                     └────────────────────────┘              └───────────────────────┘
  log region lock: one writer at a time, so it decides log order
  rep_mutexp:      guards the rep->* fields, including committed_lsn
  nothing is held between 2 and 3, so saves can land in a different order than writes

before (repo lock held around the whole commit):
  A: [1 → 2 @100 → 3 save 100]
  B:                           [1 → 2 @200 → 3 save 200]    saved = 200 ✅

after (plain commits skip the repo lock):
  A: 1 → 2 @100 ───────── slow ─────────── 3 save 100
  B:       1 → 2 @200 → 3 save 200                          saved = 100 ❌

fix: step 3 saves only if newer (gen first, then LSN), so A's late save 100 is ignored
                                                            saved = 200 ✅

2. Logical live SC redo could save a restart LSN past a write it hadn't replayed. With the redo list empty, the redo thread treats the latest commit LSN as "caught up". A plain commit on another table can now move that LSN past a write to the SC table that is logged but not yet queued. Fix: read the LSN under the repo lock, which the writer holds until it is queued:

logical commit (write to SC table), holding the repo lock throughout:
  [lock] ① write logical record → ② join redo list → ③ commit [unlock]

redo thread (fix):
  [lock] read latest commit position [unlock]
     │
     └─ can't get the lock while any writer is between ① and ②,
        so when it reads, every write logged so far is already queued

Each fix has a test (committed_lsn_order, sc_redo_start_lsn) that fails without it and passes with it and on the old locking.

📊 Results

8-node cluster, single-row autocommit inserts (one commit in flight per client), 8 writer threads, Release build (RelWithDebInfo). Both columns use the same binary, with a benchmark-only switch forcing the old locking.

clients (3 × 120 s)	lock on every commit	lock only when needed
32	11,868	14,181 (+19%)
64	11,310	13,069 (+16%)

• Logical logging on (LLOG, so every parent commit still takes the lock), 32 clients: 11,363 → 12,299 (+8%), from child commits no longer locking.
• Replicants acking 1–2 ms late (benchmark-only delay): no change (1 ms: 5,924 → 5,915; 2 ms: 3,427 → 3,445). Each commit then waits on the ack, so the lock is no longer the bottleneck.
• Records can now reach replicas out of order more often. The master drops the log lock before sending; before, the repo lock kept commit records in order, now they race like other records. Replicas park early records until the gap fills (~2× more than before) and ask for a resend more often (~3% of commits per replica vs ~0.3%). The resent copies are almost all duplicates of records that were only late. The numbers above include that cost; forcing commit records to be sent in order made throughput worse (32 clients: 13,703).

[roborivers]

Cbuild submission: Success ✓.
Regression testing: 2/727 tests failed ⚠.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
remotecreate_twopc_generated
sc_downgrade [timeout] **quarantined**
truncatesc_offline_generated [timeout] **quarantined**
sql_logfill_autodisable [timeout]

[roborivers]

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

The first 10 failing tests are:
sc_truncate [db unavailable at finish]
timepart_retro
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

[roborivers]

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

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

[roborivers]

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

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**

[roborivers]

Cbuild submission: Success ✓.
Regression testing: 1/733 tests failed ⚠.

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

[roborivers]

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

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**