Skip to content

Readers yield to replication on a replicant - #6407

Draft
WalidNejmi wants to merge 1 commit into
bloomberg:mainfrom
WalidNejmi:rep-yield-readers
Draft

WalidNejmi wants to merge 1 commit into
bloomberg:mainfrom
WalidNejmi:rep-yield-readers

Conversation

@WalidNejmi

@WalidNejmi WalidNejmi commented Oct 24, 2026 •

Copy link
Copy Markdown
Member

Readers yield to replication on a replicant

Summary

On a replicant (a non-master node), the replication thread can get stuck behind SQL readers. With enough readers on one node, a single replicated transaction loses hundreds or thousands of deadlocks in a row, the node falls more than 10 s behind, and the master marks it incoherent. Clients reading from that node then get stale data. This change makes readers step aside briefly once replication has clearly lost too many deadlocks on one transaction.

Headline numbers: this commit (e8edab6, on main c026cb7) with the yield off (rep_yield_retries 0) and on (the default, 10). Same binary and test client for both arms, reco-ddlk-sql-pinned (all 16 readers on one replicant), 3-node cluster, debug build, 4 alternating rounds (setup 1 under Measurements):

yield off yield on change
failed runs 0 of 4 0 of 4 –
most deadlocks one replicated transaction lost (worst run) 3097 19 −99%
slowest write statement (worst run) 9.46 s 1.12 s −88%
longest replicant apply stall (worst run) 9 s under 2 s (none logged) at least −78%
write statements done (mean of 4) 420 518 +23%

On older measurements against main itself (historical, previous revision of this PR, before the rebase and the review fixes), main failed 1 of 4 and 2 of 4 pinned runs with the replicant marked incoherent, and the yield failed none (see "Historical results").

Type: bug fix (replication livelock on replicants). Not affected: the master, the deadlock-victim policy, snapshot (modsnap) and serializable readers, and the order in which replication takes its locks.

Root cause

Who takes which locks

  • Readers take read locks on pages. In reco-ddlk-sql they run a 3-table join. For each row they hold a leaf page of a covering ("datacopy") index, then fetch that row's data and blob pages.
  • Replication applies a transaction from the master. Before applying it, it takes write locks on every page the transaction changed. They come from the transaction's lock list in the commit record, taken one by one in that list's order (__lock_get_list). That order puts data and blob pages before index pages.
  • The deadlock detector on a replicant runs with DB_LOCK_MINWRITE_NOREAD: a locker that holds no write locks (a reader) is never chosen as the victim. So replication loses every deadlock against readers. It drops all its locks and retries the whole transaction from the first lock (the retry loop in __rep_apply, berkdb/rep/rep_record.c).

The cycle

Readers and replication take the same pages in opposite orders, so two-locker cycles form:

sequenceDiagram
    participant R as Reader (sql thread)
    participant L as Lock manager
    participant P as Replication thread
    R->>L: read-lock index leaf I (granted)
    P->>L: write-lock blob page B (granted)
    P->>L: write-lock index leaf I (waits for R)
    R->>L: read-lock blob page B (waits for P)
    Note over L: cycle: R waits for P, P waits for R
    L-->>P: DB_LOCK_DEADLOCK (MINWRITE_NOREAD never picks a reader)
    P->>L: release every lock, re-read txn from the log
    P->>L: retry from the first lock ... meets the next reader
Loading

With many readers on one node, replication meets a new cycle almost as soon as it retries. In the RoboRivers failure that started this, one replicant took 27-39 s per transaction, each losing 669-3530 deadlocks. The master's limit is 10 s (REPTIMEOUT_MINMS), after which it marks the node incoherent.

Why the existing release points don't help

Readers already give up locks voluntarily in two places, but both fire only when someone is waiting on the reader's own locks:

  • Row send: write_response() -> recover_deadlock_evbuffer() checks bdb_curtran_has_waiters() and does a full release. "Full" means page locks and the curtran (the reader's cursor transaction, which holds its table locks). The check is meant to run every sql_recover_time (10 ms), but the way write_response() computes the time since the last check (tv_sec * 1000 - tv_usec / 1000) makes any interval under a second come out zero or negative, so in practice it runs about once a second.
  • Cursor move: cursor_move_postop() (from cursor mini-snapshot take 3 #6256) releases page locks only, paced by pagelock_release_interval_ms. It runs only with locks_check_waiters on (the default).

A reader that is inside a cycle is blocked in a lock wait. It never sends a row or moves its cursor, so it never reaches either place.

The existing escape hatch, deadlock_rep_retry_max (default INT_MAX, read-only), turns MINWRITE_NOREAD off after N losses. Readers can then be killed. That does end the stall, but in our runs a reader killed inside a blob fetch failed the query with rc 203, because blob fetches have no deadlock recovery.

The fix

Replication publishes how many deadlocks it has lost on the transaction it is applying. Once that reaches rep_yield_retries (10), readers on that node yield at their next cursor move or row send, whether or not anyone waits on their own locks:

  1. release page locks only (keep the curtran and the table locks);
  2. sleep up to rep_yield_sleep_ms (50), waking early when the transaction is applied;
  3. continue. Cursors re-position on their next move, as after any page-lock release.

Readers that are not in a cycle clear out, replication gets all its locks in one pass, and the readers stuck in the cycle are released when it commits.

flowchart LR
    subgraph before[Before]
        A1[rep loses deadlock] --> A2[drop all locks, re-read txn]
        A2 --> A3[retry: next reader blocks it]
        A3 --> A1
    end
    subgraph after[After]
        B1[rep loses deadlock] --> B2{lost 10 or more on this txn?}
        B2 -- no --> B3[retry as before]
        B3 --> B1
        B2 -- yes --> B4[readers on this node release page locks, sleep up to 50 ms]
        B4 --> B5[rep takes all locks, applies, count reset to 0]
        B5 --> B6[readers continue]
    end
Loading

Replication publishes its loss count (berkdb/rep/rep_record.c)

The count is written with the existing comdb2_atomic.h helpers; readers load it the same way.

+#include "comdb2_atomic.h"
 ...
+int gbl_rep_txn_deadlock_retries = 0;
+int gbl_rep_max_txn_deadlocks = 0;
+int64_t gbl_rep_yield_txns = 0;
 ...
 					num_retries++;
+					XCHANGE32(gbl_rep_txn_deadlock_retries, num_retries);
+					if (num_retries == gbl_rep_yield_retries)
+						gbl_rep_yield_txns++;
 ...
+					if (num_retries > gbl_rep_max_txn_deadlocks)
+						gbl_rep_max_txn_deadlocks = num_retries;
 ...
 					if (ret == DB_LOCK_DEADLOCK_CUSTOM) {
+						XCHANGE32(gbl_rep_txn_deadlock_retries, 0);
 						ret = DB_LOCK_DEADLOCK;
 						goto err;
 					}
 ...
 			} while (ret == DB_LOCK_DEADLOCK);
+			XCHANGE32(gbl_rep_txn_deadlock_retries, 0);

Readers yield (db/sqlinterfaces.c, new rep_yield_to_replication())

+int rep_yield_to_replication(struct sqlclntstate *clnt, const char *func, int line, int *rc)
+{
+    extern int gbl_rep_txn_deadlock_retries;
+    static int last_print;
+
+    int retries = ATOMIC_LOAD32(gbl_rep_txn_deadlock_retries);
+    if (gbl_rep_yield_retries <= 0 || retries < gbl_rep_yield_retries)
+        return 0;
+    if (thedb->master == gbl_myhostname)
+        return 0;
+    struct sql_thread *thd = pthread_getspecific(query_info_key);
+    if (!thd || thd->clnt != clnt || !clnt->dbtran.cursor_tran)
+        return 0;
+    if (clnt->dbtran.mode == TRANLEVEL_SNAPISOL || clnt->dbtran.mode == TRANLEVEL_SERIAL ||
+        !clnt->recover_deadlock_sync_dta)
+        return 0;
+    if (bdb_lock_desired(thedb->bdb_env))
+        return 0;
+    int now = comdb2_time_epochms();
+    if (clnt->rep_yield_last_ms && (now - clnt->rep_yield_last_ms) < gbl_pagelock_release_interval_ms)
+        return 0;
+
+    *rc = release_locks_int(RLOCKS_REASON_REP_YIELD, func, line, clnt, RECOVER_DEADLOCK_PAGELOCKS_ONLY);
+    if (*rc == 0) {
+        for (int slept = 0; slept < gbl_rep_yield_sleep_ms; slept += 5) {
+            if (ATOMIC_LOAD32(gbl_rep_txn_deadlock_retries) < gbl_rep_yield_retries || bdb_lock_desired(thedb->bdb_env))
+                break;
+            int step = gbl_rep_yield_sleep_ms - slept;
+            poll(NULL, 0, step < 5 ? step : 5);
+        }
+    }
+    clnt->rep_yield_last_ms = comdb2_time_epochms();
+    int64_t n = ATOMIC_ADD64(gbl_rep_yields, 1);
+    int sec = now / 1000, last = ATOMIC_LOAD32(last_print);
+    if (sec != last && CAS32(last_print, last, sec)) {
+        logmsg(LOGMSG_USER, "%s: readers yielded to replication ...", ...);
+    }
+    return 1;
+}

It is called from the two existing safe points. In cursor_move_postop() the call comes before the locks_check_waiters test, which only gates the waiter-driven releases below it; rep_yield_to_replication() does its own mode, recover_deadlock_sync_dta and lock-desired checks, the same ones that used to come before the call:

 static int cursor_move_postop(BtCursor *pCur)
 ...
     if (gbl_debug_sleep_in_cursor_move)
         poll(NULL, 0, gbl_debug_sleep_in_cursor_move);
 
+    if (rep_yield_to_replication(clnt, __func__, __LINE__, &rc))
+        return rc;
+
     if (!gbl_locks_check_waiters)
         return 0;
 int recover_deadlock_evbuffer(struct sqlclntstate *clnt)
 {
+    int rc;
+    if (rep_yield_to_replication(clnt, __func__, __LINE__, &rc)) {
+        ATOMIC_ADD64(gbl_rep_yields_emit_row, 1);
+        return rc;
+    }

Because the row-send check runs only about once a second (see Root cause), almost all yields happen at cursor moves: 0-2 per run at row send against 74-484 at cursor moves in the runs below.

Tunables and metrics

tunable default meaning
rep_yield_retries (new) 10 losses on one transaction before readers yield; 0 disables the yield
rep_yield_sleep_ms (new) 50 longest a yielding reader stays without page locks
pagelock_release_interval_ms (unchanged) 100 pacing for the cursor-move release; now also the shortest gap between two yields of one request

All of them can be changed at runtime. tests/tunables.test is updated.

New comdb2_metrics entries: rep_max_txn_deadlocks (highest since start), rep_yield_txns, rep_yields_cursor_move, rep_yields_emit_row, emit_row_lock_releases (recover_deadlock_evbuffer() full releases).

Test changes

  • tests/tools/reco-ddlk-sql.c:
    • RECO_PIN_READERS puts all 16 readers on one replicant. If it can't find a replicant, the test fails at once ("failed to identify a replicant for the pinned readers") instead of spreading the readers over the cluster. Without RECO_PIN_READERS nothing changes.
    • Looping readers stop once the writer is done, instead of reading the rest of a 1M-row join. Reader 99 still does the full final read.
    • The coherency query retries on connect errors.
    • The test prints a === reco-ddlk-sql results === block before its pass/fail line (details in Testing).
  • New test reco-ddlk-sql-pinned runs the client with RECO_PIN_READERS=1. It needs a cluster; on a single node it prints "Test requires a cluster" and exits, as other cluster-only tests do.

Why it is safe

  • Normal path. When replication isn't losing, the count is below 10 and rep_yield_to_replication() returns after one atomic load (a plain load on x86) and a compare. In the reco-ddlk-sql runs (readers spread out), 21 to 40 transactions per replicant reached 10 losses over a whole run of about 100 s. With healthy replication (setup 4) no transaction reached 10 and a release build matched main within noise.
  • Deadlock policy. DB_LOCK_MINWRITE_NOREAD is unchanged. Readers are still never deadlock victims and get no new errors. The yield is voluntary.
  • Table locks kept. The yield uses the existing page-lock-only release (RECOVER_DEADLOCK_PAGELOCKS_ONLY, as in cursor_move_postop()). The curtran stays, so no schema change can run while the reader has no page locks.
  • Counter always reset. The retry loop has two exits, DB_LOCK_DEADLOCK_CUSTOM (goto err) and the end of the do ... while, and both reset the count. The same loop serves the serial and the concurrent apply paths.
  • Atomic count, no lock. Only the replication thread writes the count (XCHANGE32); readers read it with ATOMIC_LOAD32. A reader that reads it just before it changes costs at most one extra or one missed yield. The once-a-second log line's timestamp (last_print) is updated with CAS32, so one thread prints it per second.
  • Independent of locks_check_waiters. Turning waiter checks off no longer turns the cursor-move yield off. With waiter checks on (the default) nothing changes: the checks that used to come before the call are the ones rep_yield_to_replication() makes itself, and the waiter-driven releases after it are untouched.
  • Bounded cost per reader. At most one yield per pagelock_release_interval_ms (100 ms) per request, and each sleep is at most 50 ms, polled in 5 ms steps. The sleep ends early when the count drops (transaction applied) or the bdb write lock is wanted.
  • Master. Skipped explicitly. Only a replicant's replication thread sets the count anyway.
  • Wrong thread. recover_deadlock_evbuffer() can run from the sqlwriter flush callback on another thread. The yield only runs on the sql thread that owns clnt.
  • Modes not covered. Snapshot (modsnap) and serializable readers, and readers with recover_deadlock_sync_dta off, don't yield, because the page-lock-only release doesn't support them. They behave as before.
  • bdb write lock wanted. The yield steps back, and the callers' existing full-release path handles it.
  • Error path. If the release fails, its rc is returned through the same path as the existing releases: cursor_move_postop() returns it, and write_response() fails the request.
  • Between requests. rep_yield_last_ms is cleared in clnt_reset_release_state() with the other release state.
  • Existing releases unchanged. The row-send and cursor-move releases, their pacing and their log lines are as on main.

Measurements

Method: 3-node docker cluster (m1-m3, 4 CPUs each) plus a driver container, one test at a time on a shared 8-CPU host. Setups 1-3: debug build (-O0, -DCOMDB2_TEST=1) of this commit, and one binary for both arms: the control arm sets rep_yield_retries 0 (or locks_check_waiters 0) through an lrl copy of the test that is not committed, so the tunable is the only difference. Arms alternate, control first. Setup 4: RelWithDebInfo builds of this commit and of main c026cb7. Host load average 1.7-13.9 during these runs. Single node: not measured, because a single node has no replication and doesn't take this path.

Terms used in the tables:

  • Apply stall: the longest LONG rep_process_message line on the readers' replicant (any replicant in setup 2). It is printed only when one message takes more than rep_longreq (1 s, compared in whole seconds), so "none logged" means under about 2 s.
  • Most deadlocks on one transaction: the rep_max_txn_deadlocks metric on the readers' replicant.
  • Write statements done: write statements the test ran (its update phase repeats 30-statement iterations until 60 s have passed, so more means the writer got further in the same time). Counted from the test's per-statement lines; the master's coherency checks are this plus 3.
  • Row-send full releases: emit_row_lock_releases.
  • Readers' node: the first non-master node by host name.

Setup 1: yield off vs. on, pinned readers

reco-ddlk-sql-pinned, 4 rounds of off → on.

yield off yield on
result pass / pass / pass / pass pass / pass / pass / pass
readers' node m1 / m1 / m1 / m2 m1 / m1 / m2 / m1
checks with a node incoherent 0 / 0 / 0 / 0 0 / 0 / 0 / 0
master "replication timeout to node" 0 / 0 / 0 / 0 0 / 0 / 0 / 0
longest apply stall none / 2 / 4 / 9 s none logged (all 4)
most deadlocks on one transaction 262 / 314 / 347 / 3097 11 / 11 / 19 / 13
transactions that reached 10 losses – 38 / 10 / 15 / 14
yields: cursor move + row send – 370+0 / 83+0 / 121+0 / 113+2
row-send full releases 373 / 445 / 438 / 487 311 / 346 / 363 / 380
slowest write statement 1.12 / 2.13 / 4.49 / 9.46 s 0.40 / 1.02 / 1.11 / 1.12 s
write statements done 450 / 480 / 450 / 300 540 / 510 / 510 / 510
test time 102 / 106 / 108 / 106 s 98 / 102 / 103 / 101 s
host load (start-end) 1.7-2.5 / 8.0-6.2 / 5.3-9.4 / 8.9-11.8 2.5-8.0 / 6.2-5.3 / 9.4-8.9 / 11.8-7.0

How to read it:

  • Without the yield, replication hit the livelock in every run: 262 to 3097 lost deadlocks on one transaction, stalls up to 9 s and writes up to 9.46 s. No run crossed the master's 10 s limit this time; earlier runs on main did (see "Historical results").
  • With the yield, no transaction lost more than 19 deadlocks, no stall was logged, and no write took more than 1.12 s. Mean write statements done went from 420 to 518 per run (+23%).
  • A transaction can lose a few more than rep_yield_retries (10): readers yield at their next safe point, at most once per 100 ms each, so cycles can still form for a short while after the count reaches 10.

Setup 2: yield off vs. on, readers spread over the cluster

reco-ddlk-sql (readers on both replicants), 4 rounds of off → on. Values per replicant (by host name) where they differ.

yield off yield on
result pass / pass / pass / pass pass / pass / pass / pass
checks with a node incoherent 0 / 0 / 0 / 0 0 / 0 / 0 / 0
master "replication timeout to node" 0 / 0 / 0 / 0 0 / 0 / 0 / 0
longest apply stall none / 2 s and 5 s / none / none none logged (all 4)
most deadlocks on one transaction 21, 27 / 262, 423 / 396, 222 / 25, 39 11, 11 / 11, 11 / 15, 12 / 14, 15
transactions that reached 10 losses – 35+21 / 27+27 / 30+40 / 23+35
yields: cursor move + row send – 230+1 and 74+0 / 129+2 and 136+0 / 142+0 and 200+0 / 91+0 and 213+0
row-send full releases 202+105 / 205+196 / 214+107 / 87+244 198+110 / 137+135 / 104+151 / 83+196
slowest write statement 1.17 / 5.52 / 1.24 / 1.52 s 0.42 / 0.39 / 0.42 / 0.51 s
write statements done 480 / 450 / 420 / 510 570 / 540 / 570 / 540
test time 103 / 111 / 103 / 103 s 103 / 102 / 100 / 101 s

The livelock also forms with readers spread out, just milder: in run 2 without the yield one transaction lost 423 deadlocks and both replicants logged stalls (2 and 5 s). Mean write statements done went from 465 to 555 (+19%).

Setup 3: locks_check_waiters 0, pinned readers

Same pinned test with locks_check_waiters 0 on every node, yield off then on, 2 rounds.

waiter checks off, yield off waiter checks off, yield on
result FAIL / FAIL FAIL / FAIL
checks with a node incoherent 150 of 153 / 161 of 166 56 of 213 / 179 of 284
master "replication timeout to node" 40 / 41 2 / 8
longest apply stall 128 / 124 s 15 / 15 s
most deadlocks on one transaction 1486 / 119 15 / 18
transactions that reached 10 losses – 42 / 42
yields: cursor move + row send – 484+1 / 448+0
row-send full releases 0 / 0 0 / 0
slowest write statement 10.67 / 10.73 s 10.57 / 10.58 s
write statements done 60 / 60 210 / 210
test time 326 / 333 s 122 / 184 s

How to read it:

  • The yield now works with waiter checks off. Readers yielded 448-484 times at cursor moves, and no transaction lost more than 18 deadlocks (119-1486 without the yield). Before this revision the cursor-move yield sat behind the locks_check_waiters test and would not have run here.
  • Both arms still fail. With waiter checks off nothing releases the page locks replication waits on outside a cycle (the waiter-driven releases are off, and emit_row_lock_releases stays 0). The replicant log shows it waiting, not deadlocking: for example "26 lock waits took 14248 ms" for a transaction that lost 13 deadlocks with the yield, and single waits of 114 and 125 s without it. The yield is triggered by lost deadlocks only, so it doesn't cover those waits. This PR does not change that (see "Not changed").

Setup 4: overhead with healthy replication, release build

RelWithDebInfo builds of main c026cb7 and of this commit (-DCOMDB2_TEST=1 for both). A small cluster test, not committed, that uses only cdb2sql: 100k rows; 8 readers on one replicant each run 1500 index range reads (count/sum/max over 1001 rows); one writer on the master keeps running 50-row update transactions until the readers finish. 6 rounds each: main → PR for rounds 1-3, PR → main for rounds 4-6.

round main reads/s PR reads/s main writes/s PR writes/s main replicant CPU s PR replicant CPU s
1 365.0 360.6 187.8 191.0 121.0 122.9
2 360.6 360.0 189.2 180.7 120.8 117.7
3 365.4 360.2 188.8 196.6 119.7 125.0
4 363.3 352.5 191.1 203.1 123.3 127.3
5 360.4 362.4 183.7 185.2 116.0 117.8
6 362.3 363.4 177.1 186.2 120.4 118.5
mean 362.8 359.9 186.3 190.5 120.2 121.5
  • PR runs: rep_max_txn_deadlocks 1-2, rep_yield_txns 0, no yields at either safe point. Replication was healthy, so the yield never ran.
  • Reads/s are 0.8% lower and writes/s 2.3% higher on the PR, both within the run-to-run spread of either arm (main reads 360.4-365.4, PR 352.5-363.4; the readers and the writer share the host's CPUs, so the PR's run with the fewest reads had the most writes). Replicant CPU (utime+stime over the read phase) is within 1.1%, while the PR replicant applied 2.3% more writes. Read as no visible difference.
  • Latency percentiles are not measured: cdb2sql reports none. Each reader's wall time was 31.9-34.0 s in every run on both builds.

Baseline before the review fixes

Before these fixes, the same experiment ran on the rebased previous revision (c1268b4), debug, 3 rounds on → off pinned plus one spread run with the yield on: all passed; yield on lost at most 10 / 15 / 13 deadlocks per transaction with slowest writes of 1.11 / 1.22 / 0.56 s and 540 / 540 / 570 write statements; yield off lost 386 / 259 / 242 with slowest writes of 1.82 / 1.98 / 1.11 s and 450 write statements each. Spread, yield on: 16 and 16, 0.45 s, 510. These fixes leave the yield's effect where it was.

Historical results (previous revision, against main)

These were measured on the previous revision of this PR (before the rebase onto c026cb7 and before the review fixes; the yield logic was the same apart from the atomics and the locks_check_waiters ordering), against its parent main 8ff152d, with the same test client for both. "Write statements done" here is the master's coherency check count (statements + 3).

reco-ddlk-sql-pinned, 4 rounds main → PR:

main PR (previous revision)
result pass / pass / FAIL / pass pass / pass / pass / pass
checks with a node incoherent 0 / 0 / 12 of 423 / 0 0 / 0 / 0 / 0
master "replication timeout to node" 0 / 0 / 1 / 0 0 / 0 / 0 / 0
longest apply stall 3 / 10 / 19 / 3 s none logged (all 4)
most deadlocks on one transaction (main: from rep-locks took lines, a lower bound) 79 / 208 / 989 / 46 15 / 14 / 13 / 12
yields: cursor move + row send – 409+1 / 305+0 / 123+2 / 94+2
slowest write statement 3.95 / 9.91 / 10.72 / 3.15 s 0.55 / 0.87 / 1.23 / 1.12 s
write statements done 513 / 363 / 423 / 483 513 / 423 / 483 / 483
test time 113 / 121 / 128 / 104 s 102 / 117 / 104 / 107 s
  • reco-ddlk-sql (spread), 2 rounds: both arms passed; main lost up to 62 deadlocks on one transaction with 3 s stalls and slowest writes of 2.98 and 1.21 s, the PR at most 12, no stall logged, 0.82 and 0.52 s.
  • With an older test client (readers reading their joins to the end), the first version of the yield vs. main, 4 runs each: main failed 2 of 4 (5 and 13 incoherent at the end, 1 and 3 replication timeouts, stalls of 12 and 16 s), the yield passed 4 of 4.
  • An earlier revision also changed write_response() so the row-send check ran every 10 ms, paused a reader pagelock_release_interval_ms between row-send full releases and lowered that interval to 25 ms. Measured with the yield off, those changes on their own were worse than main (3 of 4 passed, 822-3261 lost deadlocks, 1188-2000 row-send releases against 374-424), so they were dropped.

Testing

This commit (e8edab6), 3-node docker cluster, debug build unless noted:

test result test duration
reco-ddlk-sql-pinned 4 / 4 passed with the yield on (setup 1); 4 / 4 also passed with it off 98-108 s each
reco-ddlk-sql 4 / 4 passed with the yield on (setup 2); 4 / 4 also passed with it off 100-111 s each
recovdlock_emptyblob passed (no replicant reached 10 losses in this run, so no yields) 153 s
recovdlock passed 134 s
blob2 passed 36 s
blob_sanity passed 53 s
tunables (single node) passed 17 s
pinned guard (single node) RECO_PIN_READERS=1 make reco-ddlk-sql fails at once with "failed to identify a replicant for the pinned readers"; make reco-ddlk-sql-pinned prints "Test requires a cluster" and passes –
reco-ddlk-sql-pinned with locks_check_waiters 0 fails with the yield on or off (setup 3) 122-333 s
  • Failures: only with locks_check_waiters 0 (setup 3), in both arms. Classified as replication waiting on readers' page locks outside a deadlock cycle with the waiter-driven releases turned off, not the livelock this PR fixes: lost deadlocks stay at 15-18 per transaction with the yield. The yield-off arm, which is main's behaviour, fails much worse (124-128 s stalls, 40-41 replication timeouts).

  • RoboRivers: an earlier run on the first version timed out in reco-ddlk-sql-pinned. Its log shows no incoherent node and no replication timeout. The 10 minutes went to readers reading 1M-row joins to the end after the writer had finished (214-274 s per reader), which the client change here fixes. This commit has not had a RoboRivers result yet.

  • Quarantine: reco-ddlk-sql is quarantined in tests/quarantine.csv. This PR leaves that entry alone. The new reco-ddlk-sql-pinned is not quarantined.

  • New test: reco-ddlk-sql-pinned. Spreading readers over the cluster rarely puts enough of them on one node to form a long livelock; pinning reproduces it reliably (262 or more lost deadlocks per transaction in every yield-off run of setup 1).

  • Results block: both reco tests print it before their pass/fail line:

    === reco-ddlk-sql results ===
    master m2: coherency checks:543  checks with num-incoherent > 0:0  max num-incoherent:0
    m1 (replicant, readers here):  rep txn max deadlocks:11  rep txns reaching rep_yield_retries:38  yields at cursor-move:370  yields at emit-row:0  emit-row releases:311
    ...
    slowest write: 0.397sec  ...
    total time: 98sec
    === end of reco-ddlk-sql results ===
    

    On a server without the new metrics the per-node values print n/a. The block is not printed if the test is killed by its timeout.

Not changed

  • The deadlock-victim policy (DB_LOCK_MINWRITE_NOREAD) and deadlock_rep_retry_max.
  • The order of the lock list in the commit record. Taking replication's locks in reader order (index, data, blob) cut lost deadlocks about 20x in a separate prototype, but it can't remove cycles between ASC and DESC readers on the same index. It is left for a possible follow-up.
  • What triggers the yield: lost deadlocks only. Replication waiting on readers' locks without a cycle is still left to the waiter-driven releases, so with locks_check_waiters 0 it can still stall (setup 3).
  • The row-send waiter check's interval computation in write_response() (so it still runs about once a second), the row-send release and pagelock_release_interval_ms.
  • The 60 s fallbacks (rep_wait_release_ms, pagelock_release_max_wait_ms) and how replication lock time is measured.
  • Snapshot (modsnap) and serializable readers: no yield.
  • The sorter: it is not a yield point.
  • Deadlock recovery inside blob fetches.
  • The reco-ddlk-sql quarantine entry.

@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-pinned [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 ✓.

@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 ✓.

@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: 0/759 tests failed ⚠.

@WalidNejmi
WalidNejmi marked this pull request as ready for review October 28, 2026 00:53
@WalidNejmi
WalidNejmi marked this pull request as draft October 28, 2026 01:06
On a replicant the replication thread takes a transaction's page locks
in the order of the master's commit record: data and blob pages first,
then the index.  Readers go the other way: they hold an index leaf (a
datacopy index here) and then fetch that row's data and blob pages.  A
reader that waits for a page replication already holds, while holding a
page replication wants, closes a two-locker cycle.  The replicant
deadlock detector uses DB_LOCK_MINWRITE_NOREAD, which never picks a
reader, so replication loses every time: it drops all its locks,
re-collects the transaction from the log and starts again from the
first lock.  With many readers on one node new cycles form faster than
it gets through, and a single transaction can lose thousands of
deadlocks and take tens of seconds, long enough for the master to mark
the node incoherent.  The existing release points do not help: they act
on readers that have a waiter, and a reader inside a cycle is itself
blocked in a lock wait, so it never reaches them.

Publish how many deadlocks the transaction being applied has lost
(gbl_rep_txn_deadlock_retries, set in the retry loop of __rep_apply and
cleared on every way out of it; the loop serves both the concurrent and
the serial apply paths).  It is written and read only through the
comdb2_atomic.h helpers (XCHANGE32, ATOMIC_LOAD32).  Once it reaches
rep_yield_retries, readers on that replicant step aside at their next
safe point, whether or not anyone waits on them: at a cursor move
(cursor_move_postop) and when emitting a row (recover_deadlock_evbuffer,
which also covers the sqlwriter flush path).  The cursor-move yield is
checked before the locks_check_waiters test, which only gates the
waiter-driven releases, so turning waiter checks off does not turn the
yield off.  A yield syncs the deferred data cursors, releases page locks
only, keeping the curtran and so the table locks, so a schema change can
never run under the query, then sleeps up to rep_yield_sleep_ms, waking
early once the transaction is applied or the bdb write lock is wanted.
Cursors re-position lazily, as after any page-lock release.  Each
request yields at most once per pagelock_release_interval_ms (100 ms by
default) so readers keep making progress.  Yields happen only on the sql
thread that owns the request, never on the master, and only for the
transaction levels the page-lock release already supports (not modsnap
or serializable).

The row-send waiter check in write_response() is left as it is; it
runs about once a second rather than every sql_recover_time ms, so in
practice almost all yields happen at cursor moves.

New tunables: rep_yield_retries (default 10, 0 disables) and
rep_yield_sleep_ms (default 50).

New comdb2_metrics, so a test (or an operator) can see what happened
on each node: rep_max_txn_deadlocks (most deadlocks one replicated
transaction lost), rep_yield_txns (transactions that reached
rep_yield_retries), rep_yields_cursor_move and rep_yields_emit_row
(reader yields at each safe point) and emit_row_lock_releases
(recover_deadlock_evbuffer releases).

Test changes in reco-ddlk-sql:
- RECO_PIN_READERS puts all 16 readers on one replicant, and fails the
  test if it cannot find one instead of spreading the readers over the
  cluster.  A new test, reco-ddlk-sql-pinned, uses it: that is the
  layout that reproduces the livelock.  It needs a cluster and says so
  on a single node.
- The looping readers stop once the writer is done instead of draining
  the rest of a 1M row join, which took minutes on a busy host and
  pushed the test past its timeout; reader 99 still does the final full
  read.
- The master coherency query retries on connect errors, since a slow
  connection setup under load failed the whole test.
- At the end it prints a "=== reco-ddlk-sql results ===" block: the
  master's coherency checks (how many saw an incoherent node, and the
  most at once), the new metrics for each node ("n/a" if a server does
  not have them), the slowest write statement and the total time.

Measured on a 3-node docker cluster, debug build, 4 CPUs per node, one
test at a time, one binary and test client for both arms: yield off
(rep_yield_retries 0) and on (the default 10), run alternately.
reco-ddlk-sql-pinned, 4 runs each ("-" means no apply stall over 2 s
was logged):

                        yield off                yield on
  result                pass/pass/pass/pass      pass/pass/pass/pass
  incoherent checks     0/0/0/0                  0/0/0/0
  max deadlocks, 1 txn  262/314/347/3097         11/11/19/13
  longest apply stall   -/2/4/9 s                -/-/-/-
  slowest write         1.12/2.13/4.49/9.46 s    0.40/1.02/1.11/1.12 s
  write statements      450/480/450/300          540/510/510/510

reco-ddlk-sql (readers on both replicants), 4 runs each, all passed:
with the yield off one transaction lost up to 27, 423, 396 and 39
deadlocks, one run logged stalls of 2 and 5 s, and the slowest writes
took 1.17-5.52 s; with the yield on at most 11-15 deadlocks, no stall
logged and slowest writes of 0.39-0.51 s.

With locks_check_waiters 0 the yield still runs (484 and 448
cursor-move yields, at most 15 and 18 deadlocks per transaction against
1486 and 119 with the yield off), but both arms fail the pinned test:
with waiter checks off, nothing releases the page locks replication
waits on outside a cycle (10-15 s of lock waits per transaction with
the yield, single waits of up to 125 s without it), and the yield does
not cover those waits.

A RelWithDebInfo build with healthy replication (8 readers on one
replicant running index range reads, one writer on the master, no
yields) matched main within noise over 6 alternating runs each:
359.9 vs 362.8 reads/s, 190.5 vs 186.3 writes/s.
recovdlock, recovdlock_emptyblob, blob2, blob_sanity and tunables pass.

Signed-off-by: Walid Nejmi <wnejmi@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 ✓.

@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 ✓.

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