Skip to content

Take the trn repo lock only for logical commits - #6293

Open
emelialei88 wants to merge 3 commits into
bloomberg:mainfrom
emelialei88:perf/commit-repo-lock
Open

emelialei88 wants to merge 3 commits into
bloomberg:mainfrom
emelialei88:perf/commit-repo-lock

Conversation

@emelialei88

@emelialei88 emelialei88 commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

🎯 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.
  • The lock keeps logical commits in log order for the live schema change redo list and serializable shadows; plain commits touch 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. 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.
  • Commit records can now reach replicas out of order (the log lock is dropped before the send). Replicas park early records until the gap fills, and ask for a resend more often: ~3% of commits per replica vs ~0.3% before. The numbers above include that cost; forcing in-order sends made throughput worse (32 clients: 13,703), so it isn't worth fixing.

@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: 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]

@emelialei88
emelialei88 force-pushed the perf/commit-repo-lock branch from c72eec1 to 97fc12a Compare October 5, 2026 20:18

@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_truncate [db unavailable at finish]
timepart_retro
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:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

@emelialei88
emelialei88 force-pushed the perf/commit-repo-lock branch 2 times, most recently from fcbd9a5 to 52a8ca1 Compare October 7, 2026 18:52
Signed-off-by: Emelia Lei <wlei29@bloomberg.net>
Signed-off-by: Emelia Lei <wlei29@bloomberg.net>
Signed-off-by: Emelia Lei <wlei29@bloomberg.net>
@emelialei88
emelialei88 force-pushed the perf/commit-repo-lock branch from 52a8ca1 to 9f6bd05 Compare October 7, 2026 18:55
@emelialei88
emelialei88 marked this pull request as ready for review October 7, 2026 20:12

@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:
consumer_non_atomic_default_consumer_generated **quarantined**

This branch has not been deployed

No deployments
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