From 32bb1d97d38f508afd0abb7d8ac979f9c535529b Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Sun, 6 Sep 2026 21:56:22 -0400 Subject: [PATCH 1/2] fix(txn,lock,mp): publish the SSI pivot-check-passed state (#136) A write skew committed under DB_TXN_SNAPSHOT when the second transaction's write landed while the first was inside DB_TXN->commit. __txn_commit's pivot check read both flags under TXN_SYSTEM_LOCK, but td->status stays TXN_RUNNING until __txn_end publishes TXN_COMMITTED -- far later, past cursor close, lease checks and the log write. Throughout that span T1 looked like a running transaction with its pivot check still ahead of it. The writer-side "the reader will abort itself" optimization trusted exactly that: seeing TXN_RUNNING plus TXN_DTL_WCONF, T2 deferred the conflict to a check that had already happened, skipped the branch that would have rejected T2 for its own TXN_DTL_RCONF, and only added TXN_DTL_RCONF to T1. T1 ended up holding both pivot flags with nobody left to look at them; both transactions committed and the stored state had no serial order. Close the window where it is created rather than re-checking later: a passing pivot check now publishes TXN_DTL_SICHECKED on the detail in the same TXN_SYSTEM_LOCK critical section as the check itself. The two writer-side fate tests (__lock_get_internal, __memp_si_rwconflict) ask "can the peer still resolve this edge?" via the new TXN_SI_PAST_CHECK predicate instead of "status == TXN_COMMITTED", so a committing-but-not- yet-committed peer is treated like a committed one and the writer aborts itself with DB_SNAPSHOT_UNSAFE. Re-checking after the point of no return is not an option: by then the commit record is written and aborting would be wrong. An aborted transaction is deliberately not "past check" -- its reads never committed, so an edge into it is not a conflict. In the narrow window where a commit published the flag and then failed, a writer may abort itself needlessly: a spurious DB_SNAPSHOT_UNSAFE, never a missed one, on a path where the peer is already failing. TXN_DTL_SICHECKED is a spare bit (0x80) in TXN_DETAIL's existing flags word: sizeof(TXN_DETAIL) stays 344 and every field offset is unchanged, so there is no region-layout, on-disk, log-format or ABI change. Lock ordering is unchanged -- both sides already serialized on TXN_SYSTEM_LOCK, and the commit critical section grows by one F_SET. test/isolation's write_skew_trigger and write_skew_samebtree_trigger no longer violate, so their expect_fail markers are cleared and the tier becomes a true regression gate. The reporter's control (DB_SNAPSHOT_ CONFLICT) and late (DB_SNAPSHOT_UNSAFE) timings are unchanged. Fixes #136 --- docs_src/api/c/txnbegin.md | 2 +- ...003-ssi-serializable-snapshot-isolation.md | 22 ++++++++------ src/dbinc/txn.h | 30 +++++++++++++++++++ src/lock/lock.c | 22 ++++++++++---- src/mp/mp_fget.c | 14 +++++---- src/txn/txn.c | 27 +++++++++++++---- test/isolation/README.md | 12 ++++---- test/isolation/test_iso_anomaly.c | 4 +-- 8 files changed, 100 insertions(+), 33 deletions(-) diff --git a/docs_src/api/c/txnbegin.md b/docs_src/api/c/txnbegin.md index 648b7feb7..55cde85a0 100644 --- a/docs_src/api/c/txnbegin.md +++ b/docs_src/api/c/txnbegin.md @@ -83,7 +83,7 @@ The **flags** parameter must be set to 0 or by bitwise inclusively **OR**'ing to > **Note:** In this fork, `DB_TXN_SNAPSHOT` provides *serializable* snapshot isolation. In stock Oracle Berkeley DB, `DB_TXN_SNAPSHOT` provided only plain (non-serializable) snapshot isolation, and the earlier `DB_TXN_SNAPSHOT_SAFE` flag has been removed — there is no separate non-serializable snapshot mode. - > **Known limitation (as of 5.3.34).** Outside review found that the serializable guarantee is **not yet absolute**. A conflicting write that lands while another transaction is *inside* DB_TXN->commit() can escape detection, allowing a write skew to commit; and two records on different pages of one B-tree may not be detected as conflicting. Separately, long-lived environments running many snapshot transactions can exhaust the mutex region (`ENOMEM`) because reader bookkeeping is not fully reclaimed. Applications that depend on serializability for a correctness invariant should not yet rely on it alone. Tracking: issues #136, #137, #138 (and #140 for a related replication-path defect). + > **Known limitation (as of 5.3.34).** Long-lived environments running many snapshot transactions can exhaust the mutex region (`ENOMEM`) because reader bookkeeping is not fully reclaimed. Tracking: issues #137, #138 (and #140 for a related replication-path defect). - `DB_TXN_SYNC` diff --git a/rfc/0003-ssi-serializable-snapshot-isolation.md b/rfc/0003-ssi-serializable-snapshot-isolation.md index 44ef3652f..f62fd9685 100644 --- a/rfc/0003-ssi-serializable-snapshot-isolation.md +++ b/rfc/0003-ssi-serializable-snapshot-isolation.md @@ -60,19 +60,23 @@ Both of Cahill's rw-conflict detection paths are implemented: SIREAD markers are reclaimed incrementally (not only at checkpoint). -> **Known limitations (2026-09, from external reports #136–#140).** The claims in +The commit-time pivot check is atomic with respect to conflict recording. Both +pivot flags are read under `TXN_SYSTEM_LOCK` — the mutex every recorder +(`__lock_get_internal`, `__memp_si_rwconflict`) takes around its flag +read-modify-write — and, in the same critical section, a passing check publishes +`TXN_DTL_SICHECKED` on the detail. That flag is what makes the window between +the check and `__txn_end`'s `TXN_COMMITTED` store safe: a recorder that arrives +during it sees that the committing transaction will not re-examine its flags and +resolves the edge itself (`DB_SNAPSHOT_UNSAFE`) instead of deferring to a check +that has already happened. Deferring on `status == TXN_RUNNING` alone was +issue #136 — a write skew where both transactions committed; `test/isolation` +gates it. + +> **Known limitations (2026-09, from external reports #137–#140).** The claims in > this RFC describe the *intended* design; the delivered behavior is weaker in > ways confirmed by outside review. Until the fixes land with regression tests, > treat the serializability guarantee as **best-effort, not absolute**: > -> - **#136 — write skew can commit.** The commit-time pivot check is **not** -> atomic with respect to the `TXN_RUNNING` → `TXN_COMMITTED` transition. A -> conflicting write that lands while the first transaction is *inside* -> `DB_TXN->commit` can leave both transactions committing, producing a state -> with no serial order. A separate observation from the same report: two -> records on *different pages of one B-tree* may detect no conflict at all. -> (An earlier working note in this directory claimed this race was resolved; -> that claim was wrong and is retracted.) > - **#137 / #138 — marker reclamation is not fully bounded.** SIREAD cleanup > does not reclaim the deferred committed-reader locker, and > `__txn_reap_si_details` frees a transaction detail without releasing its MVCC diff --git a/src/dbinc/txn.h b/src/dbinc/txn.h index ffec7da3c..5fa030cdb 100644 --- a/src/dbinc/txn.h +++ b/src/dbinc/txn.h @@ -114,6 +114,7 @@ typedef struct __txn_detail { #define TXN_DTL_NOWAIT 0x10 /* Don't block on locks. */ #define TXN_DTL_WCONF 0x20 /* SSI: write end of an rw-conflict. */ #define TXN_DTL_RCONF 0x40 /* SSI: read end of an rw-conflict. */ +#define TXN_DTL_SICHECKED 0x80 /* SSI: past its commit pivot check. */ u_int32_t flags; SH_TAILQ_ENTRY links; /* active/free/snapshot list */ @@ -129,6 +130,35 @@ typedef struct __txn_detail { roff_t slots[TXN_NSLOTS]; /* Initial DB slot allocation. */ } TXN_DETAIL; +/* + * TXN_SI_PAST_CHECK -- + * SSI: TRUE when this transaction will not consult its own pivot flags + * again, so a writer forming an rw-conflict edge into it now cannot defer + * the conflict to it and must resolve the edge itself. That is the case + * once the transaction has committed and, crucially, also while it is + * inside DB_TXN->commit past its one and only pivot check: __txn_commit + * publishes TXN_DTL_SICHECKED under TXN_SYSTEM_LOCK, atomically with that + * check. + * + * Without the flag the whole span from the pivot check to __txn_end's + * status store still reads TXN_RUNNING, so a writer deferred to a check + * that had already happened and both transactions committed a write skew + * (issue #136). An aborted transaction is deliberately NOT "past check": + * its reads never committed, so an edge into it is not a conflict at all + * -- hence the status test rather than testing TXN_DTL_SICHECKED alone, + * which survives on the detail if the commit fails after publishing it. + * In that narrow error window (commit published the flag, then failed and + * his on its way to TXN_ABORTED) a writer may abort itself needlessly. + * That errs safe -- a spurious DB_SNAPSHOT_UNSAFE, never a missed one -- + * and only on a path where the peer is already failing. + * + * Callers hold TXN_SYSTEM_LOCK, which is where every writer of the pivot + * flags and of TXN_DTL_SICHECKED serializes. + */ +#define TXN_SI_PAST_CHECK(td) \ + ((td)->status == TXN_COMMITTED || \ + ((td)->status == TXN_RUNNING && F_ISSET(td, TXN_DTL_SICHECKED))) + /* * DB_TXNMGR -- * The transaction manager encapsulates the transaction system. diff --git a/src/lock/lock.c b/src/lock/lock.c index 72353ee69..cba5df65c 100644 --- a/src/lock/lock.c +++ b/src/lock/lock.c @@ -1102,22 +1102,34 @@ again: if (obj == NULL) { * Record R --rw--> W under the txn-region mutex so * the two-flag reads and writes are atomic against * a concurrent recorder and the commit-time check. + * + * Both fate tests below ask "can R still resolve + * this edge itself?", which is FALSE not only once R + * has committed but also while R is inside + * DB_TXN->commit past its pivot check -- + * TXN_SI_PAST_CHECK covers both, because + * __txn_commit publishes TXN_DTL_SICHECKED under + * this same mutex. Testing R's status alone read + * TXN_RUNNING for the whole span between R's check + * and __txn_end, so we deferred to a check that had + * already happened and both transactions committed a + * write skew (#136). */ if (TXN_ON(env)) TXN_SYSTEM_LOCK(env); if (F_ISSET(LOCK_OWNER(env, sireadlp), TXN_DTL_WCONF) && - LOCK_OWNER(env, sireadlp)->status == - TXN_COMMITTED) + TXN_SI_PAST_CHECK(LOCK_OWNER(env, sireadlp))) ret = DB_SNAPSHOT_UNSAFE; else { rwconf = 1; /* * Set our incoming-conflict flag unless - * the reader will itself abort. + * the reader will itself abort -- which it + * only will if it has a pivot check left. */ - if (LOCK_OWNER(env, sireadlp)->status == - TXN_COMMITTED || + if (TXN_SI_PAST_CHECK(LOCK_OWNER(env, + sireadlp)) || !F_ISSET(LOCK_OWNER(env, sireadlp), TXN_DTL_WCONF)) { if (F_ISSET(LOCKER_TD(env, diff --git a/src/mp/mp_fget.c b/src/mp/mp_fget.c index 9bcb53fc6..33f684f99 100644 --- a/src/mp/mp_fget.c +++ b/src/mp/mp_fget.c @@ -156,12 +156,16 @@ __memp_si_rwconflict(env, txn, visible_bhp) break; } /* - * Set W's write-end flag unless W already committed with the - * read end set (W was itself a pivot that slipped through); - * in that case the incoming edge still makes R the pivot. + * Set W's write-end flag unless W already reached the point + * where it will not re-examine its own pivot flags -- it has + * committed, or it is inside DB_TXN->commit past its pivot + * check (TXN_DTL_SICHECKED, published under this same mutex). + * If W is there and already carries the read end, W is a pivot + * that nobody will stop, so the incoming edge makes R abort + * instead. Testing W's status alone missed the committing case + * and let a write skew commit (issue #136). */ - if (F_ISSET(wtd, TXN_DTL_RCONF) && - wtd->status == TXN_COMMITTED) { + if (F_ISSET(wtd, TXN_DTL_RCONF) && TXN_SI_PAST_CHECK(wtd)) { ret = DB_SNAPSHOT_CONFLICT; break; } diff --git a/src/txn/txn.c b/src/txn/txn.c index 2524e08c2..8c17d022b 100644 --- a/src/txn/txn.c +++ b/src/txn/txn.c @@ -746,12 +746,25 @@ __txn_commit(txn, flags) * could produce a non-serializable schedule. * * The pivot flags are set on td by concurrent writers in the lock - * manager, which serialize their flag writes on the txn-region mutex - * (see __lock_get_internal). Read both flags under the same mutex so - * the two-flag test and this commit decision are atomic with respect - * to a writer recording our second conflict edge -- otherwise an edge - * set between the two reads, or just after they pass, would let a real - * pivot commit. + * manager and in mpool, which serialize their flag writes on the + * txn-region mutex (see __lock_get_internal, __memp_si_rwconflict). + * Read both flags under that mutex so the two-flag test is atomic with + * respect to a writer recording our second conflict edge. + * + * This is our ONLY pivot check: everything below (cursor close, lease + * check, log write) is past the point where aborting on a late edge + * would be correct, and __txn_end does not publish TXN_COMMITTED until + * much later. So, in the same critical section, publish + * TXN_DTL_SICHECKED: it tells a writer that arrives during that whole + * span that this transaction will not look at its pivot flags again, so + * the writer must resolve the edge itself (abort with + * DB_SNAPSHOT_UNSAFE) rather than defer to a check that has already + * happened. Without it a writer treats us as "still running, already + * flagged, it will abort at its check" and both transactions commit a + * write skew (issue #136). + * + * Only a snapshot-safe txn is examined by those writers, so nothing is + * published for the ordinary path. */ if (F_ISSET(txn, TXN_SNAPSHOT_SAFE)) { int is_pivot; @@ -759,6 +772,8 @@ __txn_commit(txn, flags) TXN_SYSTEM_LOCK(env); is_pivot = F_ISSET(td, TXN_DTL_WCONF) && F_ISSET(td, TXN_DTL_RCONF); + if (!is_pivot) + F_SET(td, TXN_DTL_SICHECKED); if (TXN_ON(env)) TXN_SYSTEM_UNLOCK(env); if (is_pivot) { diff --git a/test/isolation/README.md b/test/isolation/README.md index 2efea5c77..d403669a0 100644 --- a/test/isolation/README.md +++ b/test/isolation/README.md @@ -38,11 +38,11 @@ The state vector has two kinds of slot: | Scenario | Shape | Expectation on master | |---|---|---| -| `write_skew_trigger` | two one-page DBs; T2's write lands while T1 is inside `commit` | **XFAIL — reproduces #136** | +| `write_skew_trigger` | two one-page DBs; T2's write lands while T1 is inside `commit` | PASS (`DB_SNAPSHOT_UNSAFE` to T2) — regression gate for #136 | | `write_skew_control` | two one-page DBs; T2 writes and commits before T1 commits | PASS (`DB_SNAPSHOT_CONFLICT` to T1) | | `write_skew_late` | two one-page DBs; T2 writes after T1's commit returned | PASS (`DB_SNAPSHOT_UNSAFE` to T2) | | `write_skew_samebtree_control` | two records on **different pages of one** B-tree; control timing | PASS | -| `write_skew_samebtree_trigger` | same, trigger timing | **XFAIL — reproduces #136** | +| `write_skew_samebtree_trigger` | same, trigger timing | PASS — regression gate for #136 | | `g2_antidep` | G2-item: both txns scan for markers, both insert one | PASS | | `read_only_anomaly` | Fekete's 3-txn pattern; the read-only txn's observation is checked | PASS | | `lost_update` | both txns read the counter and write read+1 | PASS | @@ -83,7 +83,8 @@ purpose: **one** violation in any attempt is a reproduction, while a pass requires **every** attempt to be clean. A serializability violation is a real counterexample; a single clean run of a racy schedule proves nothing. -In practice both #136 shapes violate on the first attempt. +In practice both #136 shapes violated on the first attempt before the fix; both +now report `DB_SNAPSHOT_UNSAFE` to T2 in every attempt. ## Running it @@ -113,5 +114,6 @@ Environment: `CC`, `LIBDB_BUILD` (default `../../build_unix`), `ISO_TIMEOUT` `test_iso_anomaly.c`. The message says which. - `2` — harness error. -When #136 lands, clear `expect_fail` on `write_skew_trigger` and -`write_skew_samebtree_trigger`; the tier then gates the fix against regression. +#136 is fixed (`TXN_DTL_SICHECKED`, see `rfc/0003`), so `expect_fail` is clear +on every scenario and this tier is a plain regression gate: any violation is a +new bug. diff --git a/test/isolation/test_iso_anomaly.c b/test/isolation/test_iso_anomaly.c index 9c953eab0..ac271af3d 100644 --- a/test/isolation/test_iso_anomaly.c +++ b/test/isolation/test_iso_anomaly.c @@ -1019,7 +1019,7 @@ read_your_writes(iso_scenario *sc, iso_state *st, iso_txn *t) static iso_scenario scenarios[] = { { "write_skew_trigger", "two one-page DBs; T2's write lands while T1 is inside commit", - 2, 2, 2, 1, "#136", 40, sk_trigger }, + 2, 2, 2, 0, NULL, 40, sk_trigger }, { "write_skew_control", "two one-page DBs; T2 writes and commits before T1 commits", 2, 2, 2, 0, NULL, 5, sk_control }, @@ -1031,7 +1031,7 @@ static iso_scenario scenarios[] = { 2, 2, 2, 0, NULL, 5, sk_samebtree_control }, { "write_skew_samebtree_trigger", "two records on DIFFERENT pages of ONE btree; trigger timing", - 2, 2, 2, 1, "#136", 40, sk_samebtree_trigger }, + 2, 2, 2, 0, NULL, 40, sk_samebtree_trigger }, { "g2_antidep", "G2-item: both txns scan for markers, both insert one", 1, 1, 2, 0, NULL, 1, g2_antidep }, From e577baa391509e55b2959891519d363b19f9c713 Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Sun, 6 Sep 2026 22:03:56 -0400 Subject: [PATCH 2/2] build: regenerate int_def.in for __os_csprng The PUBLIC prototype for __os_csprng (added with the CSPRNG IV seeding) never had dist/s_include re-run, so src/dbinc_auto/int_def.in lacked its name-mangling #define. The header-regen drift gate only runs on pull requests, so master pushes never surfaced it; it fails on any PR branched from current master. Pure regeneration output ('cd dist && sh s_include'), no hand edits. --- src/dbinc_auto/int_def.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/dbinc_auto/int_def.in b/src/dbinc_auto/int_def.in index fe0097178..a483f4b2e 100644 --- a/src/dbinc_auto/int_def.in +++ b/src/dbinc_auto/int_def.in @@ -1846,6 +1846,7 @@ #define __os_support_db_register __os_support_db_register@DB_VERSION_UNIQUE_NAME@ #define __os_support_replication __os_support_replication@DB_VERSION_UNIQUE_NAME@ #define __os_cpu_count __os_cpu_count@DB_VERSION_UNIQUE_NAME@ +#define __os_csprng __os_csprng@DB_VERSION_UNIQUE_NAME@ #define __os_ctime __os_ctime@DB_VERSION_UNIQUE_NAME@ #define __os_dirlist __os_dirlist@DB_VERSION_UNIQUE_NAME@ #define __os_dirfree __os_dirfree@DB_VERSION_UNIQUE_NAME@