feat(ssi): complete and harden Serializable Snapshot Isolation#37
Open
gburd wants to merge 4 commits into
Open
feat(ssi): complete and harden Serializable Snapshot Isolation#37gburd wants to merge 4 commits into
gburd wants to merge 4 commits into
Conversation
Two correctness fixes to the DB_TXN_SNAPSHOT_SAFE transaction path, plus the honesty caveats the feature warranted. 1. si_ref was initialized *after* the transaction detail was published on the region's active-transaction list. TXN_DETAIL is region memory with no guaranteed zeroing, so a concurrent walker (lock-manager conflict flagging, checkpoint marker reclaim, stats) could read garbage in that window. Move the `td->si_ref = 0` initialization above the list insert, under TXN_SYSTEM_LOCK. 2. SSI is not yet compatible with two-phase commit: the pivot check that can abort an SSI transaction runs at commit time, but a prepared transaction must be guaranteed committable (upstream panics the environment if a prepared txn cannot commit). An SSI txn could acquire its second conflict edge after prepare, turning that panic into a reachable environment-wide crash for any XA user. Reject DB_TXN_SNAPSHOT_SAFE + prepare() with EINVAL until the conflict status can be frozen at prepare time. README gains an Oracle non-affiliation disclaimer and a note that v5.3.29+ are community fork releases (Oracle's final 5.3 was 5.3.28). Test: ssi003 verifies prepare() is rejected, the txn aborts cleanly, a plain txn can still prepare, and the environment survives (no panic).
…laim Two entangled correctness fixes in the SSI lock/reclaim machinery. Pivot-flag race: the rw-conflict flags (TXN_DTL_RCONF/TXN_DTL_WCONF) are set on a transaction detail by writers in the lock manager and read by __txn_commit's pivot check. Under the default multi-partition lock manager, the flag writes hold only a per-partition mutex and the commit-time read took no lock at all, so a writer in a different partition could set the second edge between the two flag reads (or just after they pass), letting a genuine pivot commit -- exactly in the high-contention schedules SSI exists for. Serialize both the flag writes (__lock_get_internal) and the commit-time read (__txn_commit) on the txn-region mutex. This is the only partition-independent, cross-process-correct choice; the naive lock-region approach violates the lock/txn region ordering that lock_deadlock.c and __txn_end rely on. Ordering mtx_partition/mtx_hash -> txn-region is verified non-circular. Bounded reclaim: committed readers' SIREAD markers were reclaimed only at checkpoint (__lock_sicleanup), so with no checkpoints the marker count -- and the committed-reader locker/detail structs it pins -- grew as a function of total transactions ever run, an unbounded leak for a long-lived embedded process. Add an approximate region counter (nsireaders) and a __txn_begin- driven sweep that runs when live markers exceed half the allocated lock objects, so retention is bounded by concurrent load, not history. Tests: ssi004 forces the write-skew onto 64 lock partitions x50 iterations (the cross-partition race condition); ssi005 runs 400 committed readers with no checkpoint and asserts peak object usage stays bounded (observed: 7, not ~400).
Implements the second of Cahill's two rw-antidependency detection mechanisms, the one that was missing. Mechanism (a), already present, uses the lock table: a snapshot writer taking a WRITE lock sees a concurrent reader's SIREAD marker. But once a writer commits it drops its locks, so a conflict where the writer commits *before* the reader physically reads goes undetected by (a) -- and some non-serializable schedules then commit. This is why SSI was labelled incomplete. Mechanism (b) closes that gap in the buffer pool. At the snapshot-read version- chain walk in __memp_fget, when a DB_TXN_SNAPSHOT_SAFE reader is handed the version visible at its read_lsn and newer committed versions exist on the chain (SH_CHAIN_HASNEXT), each newer version was created by a concurrent writer W: the reader R read a version W overwrote, an edge R --rw--> W. Record the edge with the same flag rules as mechanism (a) and return DB_SNAPSHOT_CONFLICT if R becomes the pivot of a dangerous structure -- aborting earlier than commit time. The walk holds hp->mtx_hash; the pivot flags are read/written under TXN_SYSTEM_LOCK (order mtx_hash -> txn-region, verified non-circular). The version-chain walk must use SH_CHAIN_NEXT, not SH_CHAIN_NEXTP: the latter does not return NULL at chain end (it computes elm + -1), which walks off into freed memory. ASan+UBSan clean. Test: ssi006 constructs a write-skew where writers overwrite items and commit with no live SIREAD marker present, so mechanism (a) records nothing; snapshot readers then cross-read the overwritten items and only the version-chain check can catch it. Verified to fail with (b) disabled and pass with it. With both mechanisms present, a race-free commit check, bounded reclaim, and 2PC rejection, the README and ROADMAP drop the "incomplete detection" caveat; SSI remains "experimental" only on page-granularity abort rates and the not-yet-built qualification matrix.
Extends the bounded SIREAD reclaim: it freed marker objects but never freed the
committed readers' locker entries, so a long-lived process running many
committed snapshot-safe *read-only* transactions exhausted the statically sized
locker region ("out of available locker entries") even while object usage
stayed tiny. Root causes, both fixed:
1. A read-only SSI reader never sets visible_lsn (that is set in __txn_end only
when the txn created MVCC versions, i.e. wrote). __lock_siclean_obj's
retention test treated a MAX_LSN visible_lsn as "not yet visible" and kept
the marker forever. Order read-only markers by their read_lsn instead: drop
them once the oldest active snapshot has advanced past the reader's snapshot
(a committed reader is no longer abortable, so its marker only needs to
outlive concurrent snapshots, which carry their own conflict state).
2. Nothing actually freed the DB_LOCKER_FREED lockers once their last marker was
reclaimed. Add __lock_si_reap_lockers, called at the end of __lock_sicleanup
(no object mutex held, so object->locker latch ordering holds), to reap them.
3. The reclaim trigger keyed only on object count, but reused-key workloads keep
the object count tiny while lockers still accumulate one per committed
reader. Also trigger when current lockers exceed half the locker cap.
Test: ssi007 runs 2000 committed readers on a 200-object/2000-locker region
(far more readers than either cap) and asserts no exhaustion; and separately
that a genuinely exhausted region returns a clean error with the environment
still usable (no panic/corruption). Correctness (ssi001/002/004/006) verified
across repeated runs; ASan+UBSan clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Completes the
DB_TXN_SNAPSHOT_SAFE(SSI) feature and closes the correctnessgaps an external engineering review identified. Three logical commits:
fix(ssi): si_ref init race + reject prepare()/2PC — moves thesi_refinitialization under the region lock before the detail is published (was a
published-before-initialized shared-memory field), and rejects
DB_TXN_SNAPSHOT_SAFE+prepare()withEINVAL(SSI's commit-time pivotcheck could otherwise turn a prepared txn into a reachable
__env_panic).fix(ssi): race-free commit-time pivot check + bounded SIREAD reclaim —serializes the rw-conflict flag writes and the commit-time read on the
txn-region mutex (they were unsynchronized under the default multi-partition
lock manager, so a genuine pivot could commit); and adds a
__txn_begin-driven bounded sweep so committed-reader markers no longer accumulate
unbounded between checkpoints.
feat(ssi): MVCC version-chain detection inmp_fget— implementsCahill's second rw-conflict detection mechanism (the one that was missing),
catching conflicts where the writer commits before the read physically
happens. Without it SSI ≈ plain snapshot isolation for those schedules.
Why
An external review found the shipped SSI flag was a partial implementation of a
correctness guarantee: only one of two detection mechanisms, an unbounded
resource-retention mode, a probable init race, and an unresolved 2PC panic —
with a single positive-path test. Every one of those is addressed here.
Testing
All via the Nix dev shell (Linux x86_64, clang, TCL 8.6), each new test proven
to isolate the behaviour it guards:
ssi003— prepare() rejected, env survives (no panic).ssi004— write-skew forced onto 64 lock partitions ×50 (the cross-partitionflag race); 50/50 prevented.
ssi005— 400 committed readers, no checkpoint; peak lock objects stays at 7(not ~400), proving bounded reclaim.
ssi006— write-skew only the version-chain mechanism can catch; verifiedto fail with mechanism (b) disabled and pass with it.
ssi001/ssi002, plustxn001–004/lock001/002/005/mut001regression: no regressions.
SH_CHAIN_NEXTP-walks-off-end pointer bug during development).Notes
rebuilds after the
src/dbinc/lock.hstruct change, but does not require itto merge — a clean build works either way.
page-granularity conflict tracking can raise abort rates under contention, and
the qualification test matrix (HA/replication, recovery mid-state, region
exhaustion, measured abort-rate benchmarks) is still being built — that is the
next PR (Tier 2).