From 0607964dad35a80d5aa87fddce3a781e1f01140b Mon Sep 17 00:00:00 2001 From: writemorecode Date: Sat, 5 Sep 2026 21:51:47 +0200 Subject: [PATCH 1/2] Enable concurrent transaction execution Make the database, storage runtime, page cache, and transaction paths safe for shared multi-threaded access. Add table-level lock admission, eager WAL records, rollback coordination, and coverage for concurrent sessions and recovery. --- Cargo.lock | 351 +------- Cargo.toml | 9 +- src/core/database.rs | 192 +++-- src/core/error.rs | 4 +- src/core/error/internal.rs | 2 + src/core/lock_manager.rs | 122 ++- src/core/mod.rs | 3 + src/core/test_utils.rs | 82 ++ src/core/transaction.rs | 62 +- src/executor/expression.rs | 20 +- src/executor/mod.rs | 182 +--- src/executor/tests.rs | 425 +++++++--- src/lib.rs | 4 + src/loom_support.rs | 32 + src/planner/tests.rs | 35 +- src/relational/catalog_manager.rs | 87 +- src/relational/cursor.rs | 17 +- src/relational/index_manager.rs | 25 +- src/relational/record_manager.rs | 62 +- src/server.rs | 28 +- src/session.rs | 106 ++- src/storage/btree.rs | 46 +- src/storage/btree/mutation.rs | 23 +- src/storage/btree/payload.rs | 7 +- src/storage/btree/rebalance.rs | 10 +- src/storage/btree/rebalance_repair.rs | 4 +- src/storage/btree/root.rs | 9 +- src/storage/btree/search.rs | 27 +- src/storage/btree/split.rs | 26 +- src/storage/btree/tests.rs | 22 +- src/storage/engine.rs | 85 +- src/storage/error.rs | 6 +- src/storage/overflow.rs | 14 +- src/storage/page/core.rs | 2 +- src/storage/page_cache.rs | 779 +++++++++++++----- src/storage/recovery.rs | 53 ++ src/storage/storage_runtime.rs | 207 +++-- src/storage/transaction_manager.rs | 604 +++++--------- .../transaction_manager/fault_injection.rs | 7 +- src/sync.rs | 16 + 40 files changed, 2190 insertions(+), 1607 deletions(-) create mode 100644 src/core/test_utils.rs create mode 100644 src/loom_support.rs create mode 100644 src/sync.rs diff --git a/Cargo.lock b/Cargo.lock index d1207f2..6bcb746 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,12 +11,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "anyhow" -version = "1.0.102" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" - [[package]] name = "autocfg" version = "1.5.1" @@ -40,9 +34,9 @@ checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" [[package]] name = "bitflags" -version = "2.13.0" +version = "2.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b4388bee8683e3d04af747c73422af53102d2bd24d9eadb6cbc100baef4b43f8" +checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" [[package]] name = "cc" @@ -87,12 +81,6 @@ dependencies = [ "thiserror", ] -[[package]] -name = "equivalent" -version = "1.0.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" - [[package]] name = "errno" version = "0.3.14" @@ -105,9 +93,9 @@ dependencies = [ [[package]] name = "fastrand" -version = "2.4.1" +version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9f1f227452a390804cdb637b74a86990f2a7d7ba4b7d5693aac9b4dd6defd8d6" +checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" [[package]] name = "find-msvc-tools" @@ -121,12 +109,6 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" -[[package]] -name = "foldhash" -version = "0.1.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d9c4f5dac5e15c24eb999c26181a6ca40b39fe946cbe4c263c7209467bc83af2" - [[package]] name = "generator" version = "0.8.9" @@ -156,79 +138,26 @@ dependencies = [ [[package]] name = "getrandom" -version = "0.4.2" +version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0de51e6874e94e7bf76d726fc5d13ba782deca734ff60d5bb2fb2607c7406555" +checksum = "300e883d756b2e4ec94e02791f39b04b522276138852cfc41d9fb7e904106099" dependencies = [ "cfg-if", "libc", "r-efi 6.0.0", - "wasip2", - "wasip3", -] - -[[package]] -name = "hashbrown" -version = "0.15.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9229cfe53dfd69f0609a49f65461bd93001ea1ef889cd5529dd176593f5338a1" -dependencies = [ - "foldhash", -] - -[[package]] -name = "hashbrown" -version = "0.17.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" - -[[package]] -name = "heck" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" - -[[package]] -name = "id-arena" -version = "2.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d3067d79b975e8844ca9eb072e16b31c3c1c36928edf9c6789548c524d0d954" - -[[package]] -name = "indexmap" -version = "2.14.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d466e9454f08e4a911e14806c24e16fba1b4c121d1ea474396f396069cf949d9" -dependencies = [ - "equivalent", - "hashbrown 0.17.1", - "serde", - "serde_core", ] -[[package]] -name = "itoa" -version = "1.0.18" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" - [[package]] name = "lazy_static" version = "1.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" -[[package]] -name = "leb128fmt" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09edd9e8b54e49e587e4f6295a7d29c3ea94d469cb40ab8ca70b288248a81db2" - [[package]] name = "libc" -version = "0.2.186" +version = "0.2.189" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" [[package]] name = "linux-raw-sys" @@ -238,9 +167,9 @@ checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" [[package]] name = "log" -version = "0.4.32" +version = "0.4.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "953f07c43838f8e6f9758cab68bf5bed85465e7587ebe0b823f1bcd81978ad3a" +checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6" [[package]] name = "loom" @@ -266,9 +195,9 @@ dependencies = [ [[package]] name = "memchr" -version = "2.8.1" +version = "2.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6b947ae49db0d222b1dbc6b113ce7248a3fc3a6ca21b696717bfc000ba4484d8" +checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" [[package]] name = "nu-ansi-term" @@ -309,21 +238,11 @@ dependencies = [ "zerocopy", ] -[[package]] -name = "prettyplease" -version = "0.2.37" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "479ca8adacdd7ce8f1fb39ce9ecccbfe93a3f1344b3d0d97f20bc0196208f62b" -dependencies = [ - "proc-macro2", - "syn", -] - [[package]] name = "proc-macro2" -version = "1.0.106" +version = "1.0.107" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8fd00f0bb2e90d81d1044c2b32617f68fcb9fa3bb7640c23e9c748e53fb30934" +checksum = "985e7ec9bb745e6ce6535b544d84d6cd6f7ad8bd711c398938ae983b91a766d9" dependencies = [ "unicode-ident", ] @@ -355,9 +274,9 @@ checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" [[package]] name = "quote" -version = "1.0.45" +version = "1.0.47" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41f2619966050689382d2b44f664f4bc593e129785a36d6ee376ddf37259b924" +checksum = "1fbf4db142a473a8d80c26bbf18454ed458bf8d26c8219c331daecfdbd079001" dependencies = [ "proc-macro2", ] @@ -466,54 +385,6 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e1cf6437eb19a8f4a6cc0f7dca544973b0b78843adbfeb3683d1a94a0024a294" -[[package]] -name = "semver" -version = "1.0.28" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a7852d02fc848982e0c167ef163aaff9cd91dc640ba85e263cb1ce46fae51cd" - -[[package]] -name = "serde" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" -dependencies = [ - "serde_core", -] - -[[package]] -name = "serde_core" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" -dependencies = [ - "serde_derive", -] - -[[package]] -name = "serde_derive" -version = "1.0.228" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - -[[package]] -name = "serde_json" -version = "1.0.150" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e8014e44b4736ed0538adeecded0fce2a272f22dc9578a7eb6b2d9993c74cfb9" -dependencies = [ - "itoa", - "memchr", - "serde", - "serde_core", - "zmij", -] - [[package]] name = "sharded-slab" version = "0.1.7" @@ -531,15 +402,26 @@ checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" [[package]] name = "smallvec" -version = "1.15.2" +version = "1.16.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ed6a63f02c8539c91a8685a86f4099661ba3da017932f6ebbea6de3f0fa7c90" +checksum = "b9be42f50aa861c555654aa3a37f52f4b1074bacf4e48fe0ef7fa584e80f1f0f" [[package]] name = "syn" -version = "2.0.117" +version = "2.0.119" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e665b8803e7b1d2a727f4023456bbbbe74da67099c585258af0ad9c5013b9b99" +checksum = "872831b642d1a07999a962a351ed35b955ea2cfc8f3862091e2a240a84f17297" +dependencies = [ + "proc-macro2", + "quote", + "unicode-ident", +] + +[[package]] +name = "syn" +version = "3.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6275cddf4610d1775e6d1fe9469b2e77d0f39fd98fb7450901b821e0c53649f" dependencies = [ "proc-macro2", "quote", @@ -553,7 +435,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.2", + "getrandom 0.4.3", "once_cell", "rustix", "windows-sys", @@ -561,22 +443,22 @@ dependencies = [ [[package]] name = "thiserror" -version = "2.0.18" +version = "2.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4288b5bcbc7920c07a1149a35cf9590a2aa808e0bc1eafaade0b80947865fbc4" +checksum = "ec86235f5fcc2a73650310756d2ac5b138a5780bbbdfae3eeccec992c435ba4f" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "2.0.18" +version = "2.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ebc4ee7f67670e9b64d05fa4253e753e016c6c95ff35b89b7941d6b856dec1d5" +checksum = "bc04cd3e1236dd4a98afca4569f2deb3f120e5422a4023be2cb683f8486292af" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 3.0.4", ] [[package]] @@ -649,12 +531,6 @@ version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" -[[package]] -name = "unicode-xid" -version = "0.2.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ebc1c04c71510c7f702b52b7c350734c9ff1295c464a03335b00bb84fc54f853" - [[package]] name = "valuable" version = "0.1.1" @@ -672,54 +548,11 @@ dependencies = [ [[package]] name = "wasip2" -version = "1.0.3+wasi-0.2.9" +version = "1.0.4+wasi-0.2.12" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "20064672db26d7cdc89c7798c48a0fdfac8213434a1186e5ef29fd560ae223d6" +checksum = "b67efb37e106e55ce722a510d6b5f9c17f083e5fc79afc2badeb12cc313d9487" dependencies = [ - "wit-bindgen 0.57.1", -] - -[[package]] -name = "wasip3" -version = "0.4.0+wasi-0.3.0-rc-2026-01-06" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5428f8bf88ea5ddc08faddef2ac4a67e390b88186c703ce6dbd955e1c145aca5" -dependencies = [ - "wit-bindgen 0.51.0", -] - -[[package]] -name = "wasm-encoder" -version = "0.244.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "990065f2fe63003fe337b932cfb5e3b80e0b4d0f5ff650e6985b1048f62c8319" -dependencies = [ - "leb128fmt", - "wasmparser", -] - -[[package]] -name = "wasm-metadata" -version = "0.244.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bb0e353e6a2fbdc176932bbaab493762eb1255a7900fe0fea1a2f96c296cc909" -dependencies = [ - "anyhow", - "indexmap", - "wasm-encoder", - "wasmparser", -] - -[[package]] -name = "wasmparser" -version = "0.244.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "47b807c72e1bac69382b3a6fb3dbe8ea4c0ed87ff5629b8685ae6b9a611028fe" -dependencies = [ - "bitflags", - "hashbrown 0.15.5", - "indexmap", - "semver", + "wit-bindgen", ] [[package]] @@ -746,122 +579,28 @@ dependencies = [ "windows-link", ] -[[package]] -name = "wit-bindgen" -version = "0.51.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d7249219f66ced02969388cf2bb044a09756a083d0fab1e566056b04d9fbcaa5" -dependencies = [ - "wit-bindgen-rust-macro", -] - [[package]] name = "wit-bindgen" version = "0.57.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1ebf944e87a7c253233ad6766e082e3cd714b5d03812acc24c318f549614536e" -[[package]] -name = "wit-bindgen-core" -version = "0.51.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ea61de684c3ea68cb082b7a88508a8b27fcc8b797d738bfc99a82facf1d752dc" -dependencies = [ - "anyhow", - "heck", - "wit-parser", -] - -[[package]] -name = "wit-bindgen-rust" -version = "0.51.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b7c566e0f4b284dd6561c786d9cb0142da491f46a9fbed79ea69cdad5db17f21" -dependencies = [ - "anyhow", - "heck", - "indexmap", - "prettyplease", - "syn", - "wasm-metadata", - "wit-bindgen-core", - "wit-component", -] - -[[package]] -name = "wit-bindgen-rust-macro" -version = "0.51.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0c0f9bfd77e6a48eccf51359e3ae77140a7f50b1e2ebfe62422d8afdaffab17a" -dependencies = [ - "anyhow", - "prettyplease", - "proc-macro2", - "quote", - "syn", - "wit-bindgen-core", - "wit-bindgen-rust", -] - -[[package]] -name = "wit-component" -version = "0.244.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d66ea20e9553b30172b5e831994e35fbde2d165325bec84fc43dbf6f4eb9cb2" -dependencies = [ - "anyhow", - "bitflags", - "indexmap", - "log", - "serde", - "serde_derive", - "serde_json", - "wasm-encoder", - "wasm-metadata", - "wasmparser", - "wit-parser", -] - -[[package]] -name = "wit-parser" -version = "0.244.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ecc8ac4bc1dc3381b7f59c34f00b67e18f910c2c0f50015669dde7def656a736" -dependencies = [ - "anyhow", - "id-arena", - "indexmap", - "log", - "semver", - "serde", - "serde_derive", - "serde_json", - "unicode-xid", - "wasmparser", -] - [[package]] name = "zerocopy" -version = "0.8.52" +version = "0.8.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce1022995ff5ff5d841ad7d994facc23098cd40152f2c1d11cd607c6f530653f" +checksum = "556764e583adb45a9f8d413c2a147fa7e8d821e48e12b14fd560b607998b75eb" dependencies = [ "zerocopy-derive", ] [[package]] name = "zerocopy-derive" -version = "0.8.52" +version = "0.8.56" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ae7f38b72ec2a254e2b87ef277cf2cd4fb97cbebf944faa6f33354da0867930" +checksum = "f2ab42fc20575779bd240faa45f94a74256f755c0fa9e89f0ede20d91d0cdfc1" dependencies = [ "proc-macro2", "quote", - "syn", + "syn 2.0.119", ] - -[[package]] -name = "zmij" -version = "1.0.21" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" diff --git a/Cargo.toml b/Cargo.toml index 58596b6..ffcb75c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,9 +5,6 @@ authors = ['writemorecode'] edition = '2024' repository = 'https://github.com/writemorecode/databas' -[features] -loom = [] - [dependencies] crc = "3.4.0" thiserror = "2.0.18" @@ -18,6 +15,12 @@ loom = "0.7.2" proptest = "1.9.0" tempfile = "3.25.0" +[profile.release] +panic = "abort" + +[lints.rust] +unexpected_cfgs = { level = "warn", check-cfg = ["cfg(loom)"] } + [lints.clippy] # Don't panic expect_used = "deny" diff --git a/src/core/database.rs b/src/core/database.rs index 477351f..126e7c7 100644 --- a/src/core/database.rs +++ b/src/core/database.rs @@ -1,16 +1,14 @@ use std::path::Path; use crate::core::{ - IndexKeyRange, IndexSchema, OwnedTableRecord, TableKeyRange, TableSchema, TupleSchema, Value, - access::CatalogRead, error::StorageResult, + IndexSchema, TableId, TableSchema, + access::CatalogRead, + error::StorageResult, + lock_manager::{LockManager, TableLease}, }; +use crate::relational::catalog_manager::CatalogManager; #[cfg(test)] use crate::relational::cursor::{IndexCursor, TableCursor}; -use crate::relational::{ - catalog_manager::CatalogManager, - index_manager, - record_manager::{self, IndexScan, TableScan}, -}; use crate::storage::{ engine::Storage, log_manager::TxnId, transaction_manager::TransactionSavepoint, }; @@ -19,6 +17,13 @@ use crate::storage::{ pub struct Database { catalog: CatalogManager, storage: Storage, + locks: LockManager, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum StatementTransactionMode { + Ordinary, + Ddl, } impl Database { @@ -57,7 +62,7 @@ impl Database { fn from_storage(storage: Storage) -> StorageResult { let catalog = CatalogManager::from_storage(storage.clone())?; - Ok(Self { catalog, storage }) + Ok(Self { catalog, storage, locks: LockManager::default() }) } /// Returns the database-file path associated with this database. @@ -81,16 +86,56 @@ impl Database { } pub(crate) fn begin_transaction(&self) -> StorageResult { - self.storage.begin_transaction() + self.begin_statement_transaction(StatementTransactionMode::Ordinary) + } + + pub(crate) fn acquire_ddl_gate(&self, txn_id: TxnId) -> StorageResult<()> { + self.locks.acquire_ddl_gate(txn_id).map_err(Into::into) + } + + pub(crate) fn begin_statement_transaction( + &self, + mode: StatementTransactionMode, + ) -> StorageResult { + let txn_id = self.storage.begin_transaction()?; + let admission = match mode { + StatementTransactionMode::Ordinary => self.locks.begin_transaction(txn_id), + StatementTransactionMode::Ddl => self.locks.begin_ddl_transaction(txn_id), + }; + if let Err(error) = admission { + self.storage.rollback_transaction(txn_id)?; + return Err(error.into()); + } + Ok(txn_id) + } + + pub(crate) fn acquire_table_leases( + &self, + txn_id: TxnId, + table_ids: &[TableId], + ) -> StorageResult> { + table_ids + .iter() + .map(|table_id| self.locks.acquire(txn_id, *table_id).map_err(Into::into)) + .collect() } /// Returns the concrete relational gateway for an active transaction. - pub(crate) fn transaction(&self, txn_id: TxnId) -> crate::core::transaction::Transaction<'_> { - crate::core::transaction::Transaction::new(self, txn_id) + pub(crate) fn transaction( + &self, + txn_id: TxnId, + leases: Vec, + ) -> crate::core::transaction::Transaction<'_> { + crate::core::transaction::Transaction::new(self, txn_id, leases) } pub(crate) fn commit_transaction(&self, txn_id: TxnId) -> StorageResult<()> { - self.storage.commit_transaction(txn_id) + let result = self.storage.commit_transaction(txn_id); + if !self.storage.transaction_is_active(txn_id)? { + self.locks.begin_commit(txn_id)?; + self.locks.finish_transaction(txn_id)?; + } + result } pub(crate) fn statement_savepoint(&self, txn_id: TxnId) -> StorageResult { @@ -105,11 +150,14 @@ impl Database { } pub(crate) fn rollback_transaction(&self, txn_id: TxnId) -> StorageResult<()> { - self.storage.rollback_transaction(txn_id) + self.locks.begin_rollback(txn_id)?; + self.storage.rollback_transaction(txn_id)?; + self.locks.finish_transaction(txn_id)?; + Ok(()) } - pub(crate) fn active_transaction_id(&self) -> Option { - self.storage.active_transaction_id() + pub(crate) fn transaction_is_active(&self, txn_id: TxnId) -> StorageResult { + self.storage.transaction_is_active(txn_id) } pub(crate) fn transaction_is_poisoned(&self, txn_id: TxnId) -> StorageResult { @@ -118,17 +166,26 @@ impl Database { #[cfg(test)] pub(crate) fn force_next_lsn_exhausted_for_test(&self) { - self.storage.force_next_lsn_exhausted_for_test(); + self.storage.force_next_lsn_exhausted_for_test().unwrap(); } #[cfg(test)] pub(crate) fn fail_next_savepoint_rollback_for_test(&self) { - self.storage.fail_next_savepoint_rollback_for_test(); + self.storage.fail_next_savepoint_rollback_for_test().unwrap(); } #[cfg(test)] pub(crate) fn fail_next_wal_flush_for_test(&self) { - self.storage.fail_next_wal_flush_for_test(); + self.storage.fail_next_wal_flush_for_test().unwrap(); + } + + #[cfg(test)] + pub(crate) fn transaction_is_waiting_for_test( + &self, + txn_id: TxnId, + table_id: TableId, + ) -> StorageResult { + Ok(self.locks.transaction_is_waiting_for(txn_id, table_id)?) } #[cfg(test)] @@ -140,6 +197,10 @@ impl Database { pub(crate) fn index_cursor_by_name(&self, name: &str) -> StorageResult { self.catalog.index_cursor_by_name(name) } + + pub(super) fn catalog(&self) -> &CatalogManager { + &self.catalog + } } impl CatalogRead for Database { @@ -152,75 +213,21 @@ impl CatalogRead for Database { } } -impl Database { - pub(crate) fn create_table(&self, name: &str, row: TupleSchema) -> StorageResult { - self.catalog.create_table(name, row) - } - - pub(crate) fn create_index( - &self, - name: &str, - table_name: &str, - columns: &[&str], - ) -> StorageResult { - index_manager::create_index(&self.catalog, name, table_name, columns) - } -} - -impl Database { - pub(crate) fn scan_table(&self, table: &TableSchema) -> StorageResult { - record_manager::scan_table(&self.catalog, table) - } - - pub(crate) fn scan_table_range( - &self, - table: &TableSchema, - range: TableKeyRange, - ) -> StorageResult { - record_manager::scan_table_range(&self.catalog, table, range) - } - - pub(crate) fn scan_index( - &self, - table: &TableSchema, - index: &IndexSchema, - key_range: IndexKeyRange, - ) -> StorageResult { - record_manager::scan_index(&self.catalog, table, index, key_range) - } - - pub(crate) fn insert_table_row( - &self, - table: &TableSchema, - values: Vec, - ) -> StorageResult { - record_manager::insert_table_row(&self.catalog, table, values) - } - - pub(crate) fn delete_table_row( - &self, - table: &TableSchema, - record: &OwnedTableRecord, - ) -> StorageResult<()> { - record_manager::delete_table_row(&self.catalog, table, record) - } - - pub(crate) fn update_table_row( - &self, - table: &TableSchema, - record: &OwnedTableRecord, - values: Vec, - ) -> StorageResult { - record_manager::update_table_row(&self.catalog, table, record, values) - } -} - #[cfg(test)] mod tests { use tempfile::{NamedTempFile, tempdir}; use super::*; - use crate::core::error::{CorruptionError, CorruptionKind, StorageError}; + use crate::core::{ + LockError, + error::{CorruptionError, CorruptionKind, StorageError}, + }; + + #[test] + fn database_handle_is_send_and_sync() { + fn assert_send_sync() {} + assert_send_sync::(); + } #[test] fn create_initializes_database_that_can_be_opened() { @@ -252,6 +259,27 @@ mod tests { assert!(Database::create(file.path()).is_err()); } + #[test] + fn failed_lock_admission_rolls_back_the_new_storage_transaction() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + let ddl_txn = database.begin_statement_transaction(StatementTransactionMode::Ddl).unwrap(); + let rejected_txn = ddl_txn + 1; + + assert!(matches!( + database.begin_statement_transaction(StatementTransactionMode::Ordinary), + Err(StorageError::Lock(LockError::DdlBusy { txn_id })) + if txn_id == rejected_txn + )); + assert!(!database.storage.transaction_is_active(rejected_txn).unwrap()); + assert_eq!( + database.locks.transaction_phase(rejected_txn), + Err(LockError::TransactionNotActive { txn_id: rejected_txn }) + ); + + database.rollback_transaction(ddl_txn).unwrap(); + } + #[test] fn open_rejects_empty_file_without_header() { let file = NamedTempFile::new().unwrap(); diff --git a/src/core/error.rs b/src/core/error.rs index 7e75da1..53c1473 100644 --- a/src/core/error.rs +++ b/src/core/error.rs @@ -3,7 +3,7 @@ use std::collections::TryReserveError; use thiserror::Error; -use crate::core::PageId; +use crate::core::{PageId, lock_manager::LockError}; mod corruption; mod internal; @@ -24,6 +24,8 @@ pub enum StorageError { InvalidArgument(#[source] InvalidArgumentError), #[error("limit exceeded: {0}")] LimitExceeded(#[source] LimitExceededError), + #[error("lock error: {0}")] + Lock(#[from] LockError), #[error("internal error: {0}")] Internal(#[source] InternalError), } diff --git a/src/core/error/internal.rs b/src/core/error/internal.rs index f9faec7..099baf9 100644 --- a/src/core/error/internal.rs +++ b/src/core/error/internal.rs @@ -11,6 +11,8 @@ pub enum InternalError { InvariantViolation(#[source] InvariantViolation), #[error("allocation failed: {0}")] AllocationFailed(#[source] TryReserveError), + #[error("synchronization lock poisoned: {lock}")] + SynchronizationPoisoned { lock: &'static str }, } /// Internal state that should be unreachable through a valid operation. diff --git a/src/core/lock_manager.rs b/src/core/lock_manager.rs index b8ea026..c8cf243 100644 --- a/src/core/lock_manager.rs +++ b/src/core/lock_manager.rs @@ -30,25 +30,15 @@ //! # Loom testing //! //! The synchronization primitives are substituted with Loom's modeled types -//! in test builds using the `loom` feature. Run the focused models with -//! `cargo test --features loom core::lock_manager::loom_tests`. +//! in test builds using the `loom` configuration flag. Run the focused models +//! with `RUSTFLAGS="--cfg loom" cargo test --lib core::lock_manager::loom_tests`. use std::{ collections::{BTreeSet, HashMap, HashSet, VecDeque}, fmt, }; -#[cfg(all(test, feature = "loom"))] -mod sync { - pub(super) use loom::sync::{Arc, Condvar, Mutex, MutexGuard}; -} - -#[cfg(not(all(test, feature = "loom")))] -mod sync { - pub(super) use std::sync::{Arc, Condvar, Mutex, MutexGuard}; -} - -use sync::{Arc, Condvar, Mutex, MutexGuard}; +use crate::sync::{Arc, Condvar, Mutex, MutexGuard}; use thiserror::Error; use crate::core::{CatalogId, TxnId}; @@ -311,13 +301,13 @@ impl WaitForGraph { } /// Returns whether `txn_id` has an outgoing edge. - #[cfg(all(test, not(feature = "loom")))] + #[cfg(all(test, not(loom)))] fn contains(&self, txn_id: TxnId) -> bool { self.edges.contains_key(&txn_id) } /// Returns whether the graph has no edges. - #[cfg(all(test, feature = "loom"))] + #[cfg(all(test, loom))] fn is_empty(&self) -> bool { self.edges.is_empty() } @@ -363,7 +353,7 @@ struct LockManagerInner { /// Transaction registry, wait-for edges, and DDL admission state. graph: Mutex, /// Test-only synchronization proving that a modeled waiter has enqueued. - #[cfg(all(test, feature = "loom"))] + #[cfg(all(test, loom))] enqueue_signal: Mutex>>, } @@ -443,6 +433,27 @@ impl LockManager { Ok(()) } + /// Reserves the database-wide DDL gate for an active ordinary transaction. + /// + /// # Errors + /// + /// Returns [`LockError::TransactionNotActive`] if `txn_id` is unknown, + /// or [`LockError::DdlBusy`] unless it is the sole active transaction. + pub fn acquire_ddl_gate(&self, txn_id: TxnId) -> Result<(), LockError> { + let mut graph = self.lock_graph()?; + if !graph.transactions.contains_key(&txn_id) { + return Err(LockError::TransactionNotActive { txn_id }); + } + if graph.ddl_owner == Some(txn_id) { + return Ok(()); + } + if graph.transactions.len() != 1 || graph.ddl_owner.is_some() { + return Err(LockError::DdlBusy { txn_id }); + } + graph.ddl_owner = Some(txn_id); + Ok(()) + } + /// Returns the lock-acquisition phase of an active transaction. /// /// # Errors @@ -458,6 +469,19 @@ impl LockManager { .ok_or(LockError::TransactionNotActive { txn_id }) } + #[cfg(test)] + pub(crate) fn transaction_is_waiting_for( + &self, + txn_id: TxnId, + table_id: TableId, + ) -> Result { + let graph = self.lock_graph()?; + Ok(graph + .transactions + .get(&txn_id) + .is_some_and(|transaction| transaction.waiting_for == Some(table_id))) + } + /// Acquires an exclusive table lock, waiting in FIFO order when contended. /// /// Acquisition is idempotent: an existing owner receives another @@ -549,7 +573,7 @@ impl LockManager { return Ok(AcquireDecision::Deadlock); } - #[cfg(all(test, feature = "loom"))] + #[cfg(all(test, loom))] self.signal_waiter_enqueued()?; Ok(AcquireDecision::Wait) } @@ -609,7 +633,7 @@ impl LockManager { } /// Sends the test-only notification that a Loom waiter is fully queued. - #[cfg(all(test, feature = "loom"))] + #[cfg(all(test, loom))] fn signal_waiter_enqueued(&self) -> Result<(), LockError> { if let Some(signal) = lock(&self.inner.enqueue_signal, "Loom enqueue signal")?.take() { signal @@ -841,7 +865,7 @@ impl LockManager { } /// Arms a Loom-only notification for the next successfully queued waiter. - #[cfg(all(test, feature = "loom"))] + #[cfg(all(test, loom))] fn signal_next_enqueue(&self) -> Result, LockError> { let (send, receive) = loom::sync::mpsc::channel(); let mut signal = lock(&self.inner.enqueue_signal, "Loom enqueue signal")?; @@ -855,7 +879,8 @@ impl LockManager { /// /// Callers use this only after worker threads join because its diagnostic /// multi-queue traversal does not follow the production latch protocol. - #[cfg(all(test, not(feature = "loom")))] + #[cfg(all(test, not(loom)))] + #[allow(clippy::unwrap_used)] fn assert_invariants(&self) { let queues = self.inner.queues.lock().unwrap(); let graph = self.inner.graph.lock().unwrap(); @@ -987,7 +1012,8 @@ fn invariant(message: impl Into) -> LockError { LockError::Invariant { message: message.into() } } -#[cfg(all(test, not(feature = "loom")))] +#[cfg(all(test, not(loom)))] +#[allow(clippy::panic, clippy::unwrap_used)] mod tests { use std::{ sync::{Arc, Barrier, mpsc}, @@ -1069,6 +1095,17 @@ mod tests { manager.begin_transaction(3).unwrap(); } + #[test] + fn ordinary_transaction_can_acquire_ddl_gate_only_when_solo() { + let manager = manager_with_transactions(&[1, 2]); + assert_eq!(manager.acquire_ddl_gate(1), Err(LockError::DdlBusy { txn_id: 1 })); + rollback(&manager, 2); + + manager.acquire_ddl_gate(1).unwrap(); + manager.acquire_ddl_gate(1).unwrap(); + assert_eq!(manager.begin_transaction(3), Err(LockError::DdlBusy { txn_id: 3 })); + } + #[test] fn same_table_requests_are_exclusive_and_fifo_without_barging() { let manager = manager_with_transactions(&[1, 2, 3, 4, 5]); @@ -1326,32 +1363,37 @@ mod tests { ); rollback(&manager, 1); } + + #[test] + #[allow(clippy::panic)] + fn poisoned_graph_lock_is_reported() { + let manager = Arc::new(LockManager::new()); + let poisoned_manager = Arc::clone(&manager); + let panicked = thread::spawn(move || { + let _graph = poisoned_manager.inner.graph.lock().unwrap(); + panic!("poison wait-for graph"); + }) + .join(); + assert!(panicked.is_err()); + + assert_eq!( + manager.begin_transaction(1), + Err(LockError::Poisoned { mutex: "wait-for graph" }) + ); + } } -#[cfg(all(test, feature = "loom"))] +#[cfg(all(test, loom))] #[allow(clippy::panic, clippy::unwrap_used)] mod loom_tests { - use loom::{ - model::Builder, - sync::{ - Arc, - atomic::{AtomicUsize, Ordering}, - mpsc, - }, - thread, + use loom::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + mpsc, }; use super::*; - - fn check_model(model: impl Fn() + Send + Sync + 'static) { - let mut builder = Builder::new(); - // These protocols are small, but condition-variable wakeups can create - // many equivalent schedules. Two preemptions cover the races targeted - // by these tests while keeping the suite suitable for routine use. - builder.preemption_bound = Some(2); - builder.max_branches = 200; - builder.check(model); - } + use crate::loom_support::{check_model, thread}; fn rollback(manager: &LockManager, txn_id: TxnId) { manager.begin_rollback(txn_id).unwrap(); diff --git a/src/core/mod.rs b/src/core/mod.rs index 9ffbff9..b7651cd 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -11,6 +11,8 @@ pub(crate) mod access; pub(crate) mod database; pub mod error; pub mod lock_manager; +#[cfg(test)] +pub(crate) mod test_utils; pub(crate) mod transaction; mod types; @@ -26,6 +28,7 @@ pub use error::{ ConstraintError, CorruptionComponent, CorruptionError, CorruptionKind, InternalError, InvalidArgumentError, LimitExceededError, StorageError, StorageResult, }; +pub use lock_manager::{LockError, TableId}; pub(crate) use transaction::Transaction; pub use types::{ CatalogId, IndexKeyBound, IndexKeyRange, PageId, TableKey, TableKeyBound, TableKeyRange, TxnId, diff --git a/src/core/test_utils.rs b/src/core/test_utils.rs new file mode 100644 index 0000000..de42cb3 --- /dev/null +++ b/src/core/test_utils.rs @@ -0,0 +1,82 @@ +use std::ops::Deref; + +use crate::core::{ + Database, IndexSchema, StorageResult, TableId, TableSchema, Transaction, TupleSchema, + database::StatementTransactionMode, +}; + +pub(crate) fn create_table( + database: &Database, + name: &str, + row: TupleSchema, +) -> StorageResult { + with_ddl_transaction(database, |transaction| transaction.create_table(name, row)) +} + +pub(crate) fn create_index( + database: &Database, + name: &str, + table_name: &str, + columns: &[&str], +) -> StorageResult { + with_ddl_transaction(database, |transaction| { + transaction.create_index(name, table_name, columns) + }) +} + +fn with_ddl_transaction( + database: &Database, + operation: impl FnOnce(&Transaction<'_>) -> StorageResult, +) -> StorageResult { + let txn_id = database.begin_statement_transaction(StatementTransactionMode::Ddl)?; + let transaction = database.transaction(txn_id, Vec::new()); + let result = operation(&transaction); + drop(transaction); + + match result { + Ok(value) => { + database.commit_transaction(txn_id)?; + Ok(value) + } + Err(error) => { + database.rollback_transaction(txn_id)?; + Err(error) + } + } +} + +pub(crate) struct TestTransaction<'db> { + database: &'db Database, + transaction: Transaction<'db>, +} + +impl<'db> TestTransaction<'db> { + pub(crate) fn begin(database: &'db Database, table_ids: &[TableId]) -> StorageResult { + let txn_id = database.begin_transaction()?; + let leases = match database.acquire_table_leases(txn_id, table_ids) { + Ok(leases) => leases, + Err(error) => { + database.rollback_transaction(txn_id)?; + return Err(error); + } + }; + let transaction = database.transaction(txn_id, leases); + Ok(Self { database, transaction }) + } +} + +impl<'db> Deref for TestTransaction<'db> { + type Target = Transaction<'db>; + + fn deref(&self) -> &Self::Target { + &self.transaction + } +} + +impl Drop for TestTransaction<'_> { + fn drop(&mut self) { + if self.database.transaction_is_active(self.transaction.id()).unwrap_or(false) { + self.database.rollback_transaction(self.transaction.id()).unwrap(); + } + } +} diff --git a/src/core/transaction.rs b/src/core/transaction.rs index a277e8a..b40517a 100644 --- a/src/core/transaction.rs +++ b/src/core/transaction.rs @@ -8,9 +8,14 @@ use crate::{ core::{ IndexKeyRange, IndexSchema, OwnedTableRecord, TableKeyRange, TableSchema, TupleSchema, - Value, error::StorageResult, + Value, + error::{StorageError, StorageResult}, + lock_manager::{LockError, TableId, TableLease}, + }, + relational::{ + index_manager, + record_manager::{self, IndexScan, TableScan}, }, - relational::record_manager::{IndexScan, TableScan}, storage::{log_manager::TxnId, transaction_manager::TransactionSavepoint}, }; @@ -20,11 +25,12 @@ use super::Database; pub(crate) struct Transaction<'db> { database: &'db Database, txn_id: TxnId, + leases: Vec, } impl<'db> Transaction<'db> { - pub(super) fn new(database: &'db Database, txn_id: TxnId) -> Self { - Self { database, txn_id } + pub(super) fn new(database: &'db Database, txn_id: TxnId, leases: Vec) -> Self { + Self { database, txn_id, leases } } /// Returns the storage transaction identity associated with this gateway. @@ -39,11 +45,20 @@ impl<'db> Transaction<'db> { pub(crate) fn is_poisoned(&self) -> StorageResult { self.database.transaction_is_poisoned(self.txn_id) } + + fn require_table(&self, table: &TableSchema) -> StorageResult<()> { + let table_id = TableId::from(table.table_id); + if self.leases.iter().any(|lease| lease.authorize(self.txn_id, table_id).is_ok()) { + Ok(()) + } else { + Err(StorageError::Lock(LockError::LeaseMismatch { txn_id: self.txn_id, table_id })) + } + } } impl Transaction<'_> { pub(crate) fn create_table(&self, name: &str, row: TupleSchema) -> StorageResult { - self.database.create_table(name, row) + self.database.catalog().for_transaction(self.txn_id).create_table(name, row) } pub(crate) fn create_index( @@ -52,13 +67,19 @@ impl Transaction<'_> { table_name: &str, columns: &[&str], ) -> StorageResult { - self.database.create_index(name, table_name, columns) + index_manager::create_index( + &self.database.catalog().for_transaction(self.txn_id), + name, + table_name, + columns, + ) } } impl Transaction<'_> { pub(crate) fn scan_table(&self, table: &TableSchema) -> StorageResult { - self.database.scan_table(table) + self.require_table(table)?; + record_manager::scan_table(self.database.catalog(), Some(self.txn_id), table) } pub(crate) fn scan_table_range( @@ -66,7 +87,8 @@ impl Transaction<'_> { table: &TableSchema, range: TableKeyRange, ) -> StorageResult { - self.database.scan_table_range(table, range) + self.require_table(table)?; + record_manager::scan_table_range(self.database.catalog(), Some(self.txn_id), table, range) } pub(crate) fn scan_index( @@ -75,7 +97,14 @@ impl Transaction<'_> { index: &IndexSchema, key_range: IndexKeyRange, ) -> StorageResult { - self.database.scan_index(table, index, key_range) + self.require_table(table)?; + record_manager::scan_index( + self.database.catalog(), + Some(self.txn_id), + table, + index, + key_range, + ) } pub(crate) fn insert_table_row( @@ -83,7 +112,8 @@ impl Transaction<'_> { table: &TableSchema, values: Vec, ) -> StorageResult { - self.database.insert_table_row(table, values) + self.require_table(table)?; + record_manager::insert_table_row(self.database.catalog(), Some(self.txn_id), table, values) } pub(crate) fn delete_table_row( @@ -91,7 +121,8 @@ impl Transaction<'_> { table: &TableSchema, record: &OwnedTableRecord, ) -> StorageResult<()> { - self.database.delete_table_row(table, record) + self.require_table(table)?; + record_manager::delete_table_row(self.database.catalog(), Some(self.txn_id), table, record) } pub(crate) fn update_table_row( @@ -100,6 +131,13 @@ impl Transaction<'_> { record: &OwnedTableRecord, values: Vec, ) -> StorageResult { - self.database.update_table_row(table, record, values) + self.require_table(table)?; + record_manager::update_table_row( + self.database.catalog(), + Some(self.txn_id), + table, + record, + values, + ) } } diff --git a/src/executor/expression.rs b/src/executor/expression.rs index 86e452d..387cfd6 100644 --- a/src/executor/expression.rs +++ b/src/executor/expression.rs @@ -1,12 +1,10 @@ use crate::{ - core::{OwnedTableRecord, TableKey, TableSchema, Tuple, TupleView, Value}, + core::{OwnedTableRecord, TableKey, TableSchema, Transaction, Tuple, TupleView, Value}, planner::{BoundColumn, PlannedExpression, UpdateAssignment}, sql_parser::parser::op::Op, }; -use super::{ - ExecutionDatabase, ExecutionOutput, ExecutorError, ExecutorResult, ExecutorRow, RowStream, -}; +use super::{ExecutionOutput, ExecutorError, ExecutorResult, ExecutorRow, RowStream, collect_rows}; /// Evaluates one planned scalar expression against a record. /// @@ -31,7 +29,7 @@ pub(super) fn execute_values(rows: Vec>) -> ExecutorResul let input = empty_record(table_key)?; evaluate_expressions(&expressions, &input) }); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } /// Skips rows from a child stream while still surfacing skipped-row errors. @@ -60,7 +58,7 @@ pub(super) fn offset_rows(mut rows: RowStream, mut remaining: usize) -> RowStrea /// Each value row is evaluated, expanded into the target table layout, and then /// handed to storage for validation and insertion. pub(super) fn execute_insert_values( - records: &ExecutionDatabase<'_>, + transaction: &Transaction<'_>, table: TableSchema, columns: Vec, values: Vec>, @@ -92,7 +90,7 @@ pub(super) fn execute_insert_values( } Ok(row_values) })?; - records.insert_table_row(&table, row_values)?; + transaction.insert_table_row(&table, row_values)?; affected += 1; } @@ -101,7 +99,7 @@ pub(super) fn execute_insert_values( /// Executes an `UPDATE` plan by consuming and mutating one target row at a time. pub(super) fn execute_update( - records: &ExecutionDatabase<'_>, + transaction: &Transaction<'_>, table: TableSchema, assignments: Vec, target_rows: RowStream, @@ -128,7 +126,7 @@ pub(super) fn execute_update( *slot = value; } - records.update_table_row(&table, &owned_row, values)?; + transaction.update_table_row(&table, &owned_row, values)?; affected += 1; } @@ -137,7 +135,7 @@ pub(super) fn execute_update( /// Executes a `DELETE` plan by consuming and deleting one target row at a time. pub(super) fn execute_delete( - records: &ExecutionDatabase<'_>, + transaction: &Transaction<'_>, table: TableSchema, target_rows: RowStream, ) -> ExecutorResult { @@ -146,7 +144,7 @@ pub(super) fn execute_delete( for row in target_rows { let owned_row = row?.into_owned_record()?; - records.delete_table_row(&table, &owned_row)?; + transaction.delete_table_row(&table, &owned_row)?; affected += 1; } diff --git a/src/executor/mod.rs b/src/executor/mod.rs index a215964..c90e759 100644 --- a/src/executor/mod.rs +++ b/src/executor/mod.rs @@ -13,8 +13,7 @@ use crate::{ core::{ - Database, IndexKeyRange, IndexSchema, OwnedTableRecord, TableKey, TableKeyRange, - TableRecord as BorrowedTableRecord, TableSchema, Transaction, Tuple, TupleSchema, Value, + OwnedTableRecord, TableKey, TableRecord as BorrowedTableRecord, Transaction, Tuple, Value, error::{StorageError, StorageResult}, }, planner::PhysicalPlan, @@ -211,21 +210,28 @@ impl std::fmt::Display for ExecutorRow { } } -/// Lazy stream of records produced by a row-returning plan. -/// -/// Individual rows can fail while the stream is being consumed, for example if -/// a later scanned page cannot be read or a downstream expression fails for a -/// specific record. -pub type RowStream = Box>>; +/// Internal iterator used while an execution operator is being drained. +pub(crate) type RowStream = Box>>; + +fn collect_rows( + rows: impl Iterator>, +) -> Vec> { + rows.map(|row| { + row.and_then(|row| { + row.into_owned_record().map(ExecutorRow::Owned).map_err(ExecutorError::from) + }) + }) + .collect() +} /// Result of executing one physical plan. pub enum ExecutionOutput { /// Textual physical plan produced by `EXPLAIN`. Explain(String), - /// A lazy stream of result rows. + /// Fully materialized result rows owned by the completed execution. Rows { - /// Result rows yielded on demand. - rows: RowStream, + /// Rows drained before the statement transaction is finalized. + rows: Vec>, }, /// Number of table rows changed by a data-modification statement. RowsAffected(u64), @@ -241,10 +247,10 @@ impl ExecutionOutput { /// Row operators call this when they expect their child plan to produce /// rows. Non-row outputs become [`ExecutorError::ExpectedRows`] tagged with /// the requesting operator name. - pub fn into_rows(self, operator: &'static str) -> ExecutorResult { + pub(crate) fn into_rows(self, operator: &'static str) -> ExecutorResult { match self { Self::Explain(_) => Err(ExecutorError::ExpectedRows { operator }), - Self::Rows { rows } => Ok(rows), + Self::Rows { rows } => Ok(Box::new(rows.into_iter())), Self::RowsAffected(_) | Self::SchemaAffected | Self::CommandOk => { Err(ExecutorError::ExpectedRows { operator }) } @@ -278,118 +284,18 @@ impl std::fmt::Display for ExecutionOutput { } } -/// Executes physical query plans through a relational access gateway. +/// Executes physical query plans through a transaction-scoped relational gateway. /// -/// The executor owns no transaction state; the caller supplies either a -/// transaction-scoped gateway or, temporarily during migration, a database -/// handle for non-transactional reads. -pub struct Executor<'db> { - database: ExecutionDatabase<'db>, -} - -#[derive(Clone, Copy)] -pub(super) enum ExecutionDatabase<'db> { - Unscoped(&'db Database), - Transaction(&'db Transaction<'db>), -} - -impl ExecutionDatabase<'_> { - fn create_table(&self, name: &str, row: TupleSchema) -> StorageResult { - match self { - Self::Unscoped(database) => database.create_table(name, row), - Self::Transaction(transaction) => transaction.create_table(name, row), - } - } - - fn create_index( - &self, - name: &str, - table: &str, - columns: &[&str], - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.create_index(name, table, columns), - Self::Transaction(transaction) => transaction.create_index(name, table, columns), - } - } - - fn scan_table( - &self, - table: &TableSchema, - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.scan_table(table), - Self::Transaction(transaction) => transaction.scan_table(table), - } - } - - fn scan_table_range( - &self, - table: &TableSchema, - range: TableKeyRange, - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.scan_table_range(table, range), - Self::Transaction(transaction) => transaction.scan_table_range(table, range), - } - } - - fn scan_index( - &self, - table: &TableSchema, - index: &IndexSchema, - range: IndexKeyRange, - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.scan_index(table, index, range), - Self::Transaction(transaction) => transaction.scan_index(table, index, range), - } - } - - fn insert_table_row( - &self, - table: &TableSchema, - values: Vec, - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.insert_table_row(table, values), - Self::Transaction(transaction) => transaction.insert_table_row(table, values), - } - } - - fn delete_table_row( - &self, - table: &TableSchema, - record: &OwnedTableRecord, - ) -> StorageResult<()> { - match self { - Self::Unscoped(database) => database.delete_table_row(table, record), - Self::Transaction(transaction) => transaction.delete_table_row(table, record), - } - } - - fn update_table_row( - &self, - table: &TableSchema, - record: &OwnedTableRecord, - values: Vec, - ) -> StorageResult { - match self { - Self::Unscoped(database) => database.update_table_row(table, record, values), - Self::Transaction(transaction) => transaction.update_table_row(table, record, values), - } - } +/// The executor owns no transaction state; the caller supplies an active +/// transaction with the leases required by the plan. +pub struct Executor<'txn, 'db> { + transaction: &'txn Transaction<'db>, } -impl<'db> Executor<'db> { - /// Creates an executor for legacy unscoped reads and focused executor tests. - pub(crate) fn new(database: &'db Database) -> Self { - Self { database: ExecutionDatabase::Unscoped(database) } - } - +impl<'txn, 'db> Executor<'txn, 'db> { /// Creates an executor scoped to an active transaction. - pub(crate) fn in_transaction(transaction: &'db Transaction<'db>) -> Self { - Self { database: ExecutionDatabase::Transaction(transaction) } + pub(crate) fn in_transaction(transaction: &'txn Transaction<'db>) -> Self { + Self { transaction } } /// Executes a physical plan and returns its output. @@ -402,22 +308,22 @@ impl<'db> Executor<'db> { match plan { PhysicalPlan::Explain { input } => Ok(ExecutionOutput::Explain(input.to_string())), PhysicalPlan::CreateTable { name, schema } => { - self.database.create_table(&name, schema)?; + self.transaction.create_table(&name, schema)?; Ok(ExecutionOutput::SchemaAffected) } PhysicalPlan::CreateIndex { name, table, columns } => { let column_names: Vec<&str> = columns.iter().map(|col| col.name.as_str()).collect(); - self.database.create_index(&name, &table.name, &column_names)?; + self.transaction.create_index(&name, &table.name, &column_names)?; Ok(ExecutionOutput::SchemaAffected) } PhysicalPlan::Values { rows } => execute_values(rows), PhysicalPlan::InsertValues { table, columns, values } => { - execute_insert_values(&self.database, table, columns, values) + execute_insert_values(self.transaction, table, columns, values) } PhysicalPlan::Update { table, assignments, input } => { let output_inner = self.execute(*input)?; execute_update( - &self.database, + self.transaction, table, assignments, output_inner.into_rows("UPDATE")?, @@ -425,31 +331,31 @@ impl<'db> Executor<'db> { } PhysicalPlan::Delete { table, input } => { let output_inner = self.execute(*input)?; - execute_delete(&self.database, table, output_inner.into_rows("DELETE")?) + execute_delete(self.transaction, table, output_inner.into_rows("DELETE")?) } PhysicalPlan::OneRow => Ok(ExecutionOutput::Rows { - rows: Box::new(std::iter::once_with(|| empty_record(0))), + rows: collect_rows(std::iter::once_with(|| empty_record(0))), }), PhysicalPlan::FullTableScan { table } => { let rows = self - .database + .transaction .scan_table(&table)? .map(|record| record.map(ExecutorRow::Borrowed).map_err(Into::into)); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::PrimaryKeyRangeScan { table, range } => { let rows = self - .database + .transaction .scan_table_range(&table, range)? .map(|record| record.map(ExecutorRow::Borrowed).map_err(Into::into)); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::SecondaryIndexScan { scan } => { let rows = self - .database + .transaction .scan_index(&scan.table, &scan.index, scan.key_range)? .map(|record| record.map(ExecutorRow::Borrowed).map_err(Into::into)); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::Filter { input, predicate } => { let output_inner = self.execute(*input)?; @@ -467,7 +373,7 @@ impl<'db> Executor<'db> { } Err(error) => Some(Err(error)), }); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::Sort { input: _, terms: _ } => { // TODO: Change tuple serialization format to allow value comparison from raw byte slices @@ -478,7 +384,7 @@ impl<'db> Executor<'db> { let rows = output_inner .into_rows("PROJECT")? .map(move |row| row.and_then(|row| evaluate_expressions(&expressions, &row))); - Ok(ExecutionOutput::Rows { rows: Box::new(rows) }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::Offset { input, offset } => { @@ -486,14 +392,14 @@ impl<'db> Executor<'db> { // TODO: Make `offset` a usize value. let offset = offset as usize; let rows = offset_rows(output_inner.into_rows("OFFSET")?, offset); - Ok(ExecutionOutput::Rows { rows }) + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } PhysicalPlan::Limit { input, limit } => { let output_inner = self.execute(*input)?; // TODO: Make `limit` a usize value. let limit = limit as usize; - let rows = Box::new(output_inner.into_rows("LIMIT")?.take(limit)); - Ok(ExecutionOutput::Rows { rows }) + let rows = output_inner.into_rows("LIMIT")?.take(limit); + Ok(ExecutionOutput::Rows { rows: collect_rows(rows) }) } } } diff --git a/src/executor/tests.rs b/src/executor/tests.rs index cca95da..60455e2 100644 --- a/src/executor/tests.rs +++ b/src/executor/tests.rs @@ -1,16 +1,18 @@ -use std::fmt::Write as _; +use std::{fmt::Write as _, thread, time::Duration}; use tempfile::tempdir; use super::*; use crate::{ core::{ - ColumnSchema, DataType, OwnedTableRecord, PAGE_SIZE, TableKey, Tuple, TupleSchema, + ColumnSchema, DataType, Database, LockError, OwnedTableRecord, PAGE_SIZE, TableId, + TableKey, Tuple, TupleSchema, access::CatalogRead, error::{ConstraintError, InternalError, InvariantViolation, StorageError}, + test_utils::{TestTransaction, create_index, create_table}, }, error::DatabaseError, - planner::{BoundColumn, PlannedExpression, Planner}, + planner::{BoundColumn, PlannedExpression, Planner, PlannerError}, relational::cursor::encode_index_entry_key, session::{Session, SessionError}, sql_parser::parser::Parser, @@ -70,6 +72,27 @@ fn execute_sql_with_session<'a>( session.execute_sql(sql) } +fn wait_until_transaction_waits_for(database: &Database, txn_id: u64, table_id: TableId) { + for _ in 0..10_000 { + if database.transaction_is_waiting_for_test(txn_id, table_id).unwrap() { + return; + } + thread::sleep(Duration::from_micros(10)); + } + panic!("transaction {txn_id} did not wait for table {table_id:?}"); +} + +fn integer_rows(output: ExecutionOutput) -> Vec { + collect_rows(output) + .unwrap() + .into_iter() + .map(|row| match values(&row).as_slice() { + [Value::Integer(value)] => i64::from(*value), + values => panic!("expected one integer column, got {values:?}"), + }) + .collect() +} + fn execute_script(database: &Database, sql: &str) { let items = Parser::new(sql).collect::, _>>().unwrap(); let mut session = Session::new(database); @@ -79,6 +102,17 @@ fn execute_script(database: &Database, sql: &str) { } } +fn executor_transaction<'db>( + database: &'db Database, + table_names: &[&str], +) -> TestTransaction<'db> { + let table_ids = table_names + .iter() + .map(|name| database.table_schema_by_name(name).unwrap().table_id.into()) + .collect::>(); + TestTransaction::begin(database, &table_ids).unwrap() +} + fn is_null_value_error(error: ExecutorError, expected_column: &str) -> bool { matches!( error, @@ -264,7 +298,8 @@ fn single_column_expression_reads_bound_ordinal() { fn project_evaluates_multiple_expressions_in_order() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Project { input: Box::new(PhysicalPlan::Values { rows: vec![vec![ @@ -293,7 +328,8 @@ fn project_evaluates_multiple_expressions_in_order() { fn filter_keeps_only_rows_with_true_predicate() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Values { rows: vec![ @@ -317,22 +353,23 @@ fn filter_keeps_only_rows_with_true_predicate() { } #[test] -fn full_table_scan_yields_borrowed_rows_that_can_be_materialized() { +fn full_table_scan_materializes_owned_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", ) .unwrap(); let table = database.table_schema_by_name("users").unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let rows = collect_rows(executor.execute(PhysicalPlan::FullTableScan { table }).unwrap()).unwrap(); - assert!(matches!(rows[0], ExecutorRow::Borrowed(_))); + assert!(matches!(rows[0], ExecutorRow::Owned(_))); assert_eq!( values(&rows[0]), vec![Value::Integer(1), Value::String("Ada".to_owned()), Value::Boolean(true)] @@ -343,17 +380,18 @@ fn full_table_scan_yields_borrowed_rows_that_can_be_materialized() { } #[test] -fn filter_over_table_scan_preserves_borrowed_rows() { +fn filter_over_table_scan_materializes_owned_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", ) .unwrap(); let table = database.table_schema_by_name("users").unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Filter { input: Box::new(PhysicalPlan::FullTableScan { table }), predicate: PlannedExpression::Column(bound("active", 2, DataType::Boolean)), @@ -362,7 +400,7 @@ fn filter_over_table_scan_preserves_borrowed_rows() { let rows = collect_rows(executor.execute(plan).unwrap()).unwrap(); assert_eq!(rows.len(), 1); - assert!(matches!(rows[0], ExecutorRow::Borrowed(_))); + assert!(matches!(rows[0], ExecutorRow::Owned(_))); assert_eq!(rows[0].table_key(), 1); } @@ -370,11 +408,12 @@ fn filter_over_table_scan_preserves_borrowed_rows() { fn project_over_table_scan_returns_owned_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); let table = database.table_schema_by_name("users").unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Project { input: Box::new(PhysicalPlan::FullTableScan { table }), expressions: vec![PlannedExpression::Column(bound("name", 1, DataType::Text))], @@ -391,7 +430,8 @@ fn project_over_table_scan_returns_owned_rows() { fn filter_rejects_non_boolean_predicate() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Values { rows: vec![vec![PlannedExpression::Literal(Value::Integer(1))]], @@ -409,7 +449,8 @@ fn filter_rejects_non_boolean_predicate() { fn limit_does_not_evaluate_rows_beyond_limit() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Limit { input: Box::new(PhysicalPlan::Values { rows: vec![ @@ -434,7 +475,8 @@ fn limit_does_not_evaluate_rows_beyond_limit() { fn limit_larger_than_child_rows_returns_all_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Limit { input: Box::new(PhysicalPlan::Values { rows: vec![ @@ -456,7 +498,8 @@ fn limit_larger_than_child_rows_returns_all_rows() { fn limit_rejects_non_row_child() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Limit { input: Box::new(PhysicalPlan::CreateTable { name: "users".to_owned(), @@ -475,7 +518,8 @@ fn limit_rejects_non_row_child() { fn offset_reports_errors_from_skipped_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Offset { input: Box::new(PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Values { @@ -499,7 +543,8 @@ fn offset_reports_errors_from_skipped_rows() { fn offset_larger_than_child_rows_returns_no_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Offset { input: Box::new(PhysicalPlan::Values { rows: vec![ @@ -519,7 +564,8 @@ fn offset_larger_than_child_rows_returns_no_rows() { fn offset_rejects_non_row_child() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Offset { input: Box::new(PhysicalPlan::CreateTable { name: "users".to_owned(), @@ -538,7 +584,8 @@ fn offset_rejects_non_row_child() { fn filter_propagates_child_row_errors() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Values { @@ -559,7 +606,8 @@ fn filter_propagates_child_row_errors() { fn project_propagates_child_row_errors() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Project { input: Box::new(PhysicalPlan::Filter { input: Box::new(PhysicalPlan::Values { @@ -580,7 +628,8 @@ fn project_propagates_child_row_errors() { fn row_operator_rejects_non_row_child() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Project { input: Box::new(PhysicalPlan::CreateTable { name: "users".to_owned(), @@ -599,7 +648,8 @@ fn row_operator_rejects_non_row_child() { fn sort_returns_unsupported_error_instead_of_panicking() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let plan = PhysicalPlan::Sort { input: Box::new(PhysicalPlan::Values { rows: Vec::new() }), terms: Vec::new(), @@ -689,7 +739,7 @@ fn invalid_type_combinations_return_executor_errors() { fn select_with_projection_and_filter_executes_end_to_end() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let mut users = database.table_cursor_by_name("users").unwrap(); users .insert( @@ -718,7 +768,8 @@ fn select_with_projection_and_filter_executes_end_to_end() { let statement = Parser::new("SELECT name FROM users WHERE id == 1;").stmt().unwrap(); let plan = Planner::new(&database).plan_statement(&statement).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let rows = collect_rows(executor.execute(plan.physical).unwrap()).unwrap(); @@ -731,7 +782,7 @@ fn select_with_projection_and_filter_executes_end_to_end() { fn select_with_primary_key_range_returns_only_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, &insert_many_users_sql(25)).unwrap(); let output = @@ -749,7 +800,7 @@ fn select_with_primary_key_range_returns_only_matching_rows() { fn select_with_secondary_index_equality_returns_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -773,7 +824,7 @@ fn select_with_secondary_index_equality_returns_matching_rows() { fn select_with_secondary_index_equality_applies_residual_filter() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -797,7 +848,7 @@ fn select_with_secondary_index_equality_applies_residual_filter() { fn select_with_text_index_range_returns_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -826,7 +877,7 @@ fn select_with_text_index_range_returns_matching_rows() { fn select_with_text_index_exclusive_range_excludes_boundary_values() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -848,7 +899,7 @@ fn select_with_text_index_exclusive_range_excludes_boundary_values() { fn select_with_text_index_range_does_not_skip_short_matching_values() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -871,7 +922,7 @@ fn select_with_text_index_range_does_not_skip_short_matching_values() { fn explain_select_returns_explain_output() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); let output = @@ -884,7 +935,7 @@ fn explain_select_returns_explain_output() { fn insert_values_uses_primary_keys_and_persists_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let statement = Parser::new( "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", @@ -892,7 +943,8 @@ fn insert_values_uses_primary_keys_and_persists_rows() { .stmt() .unwrap(); let plan = Planner::new(&database).plan_statement(&statement).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let output = executor.execute(plan.physical).unwrap(); @@ -915,14 +967,15 @@ fn insert_values_uses_primary_keys_and_persists_rows() { fn insert_values_rejects_omitted_non_nullable_columns() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let plan = insert_values_plan( &database, vec![bound("id", 0, DataType::Integer), bound("name", 1, DataType::Text)], vec![vec![Value::Integer(1), Value::String("Ada".to_owned())]], ); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); assert!(executor.execute(plan).is_err_and(|error| is_null_value_error(error, "active"))); } @@ -931,14 +984,15 @@ fn insert_values_rejects_omitted_non_nullable_columns() { fn insert_values_rejects_values_with_wrong_type() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let statement = Parser::new("INSERT INTO users (id, name, active) VALUES ('one', 'Ada', TRUE);") .stmt() .unwrap(); let plan = Planner::new(&database).plan_statement(&statement).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); assert!(executor.execute(plan.physical).is_err_and(|error| is_type_mismatch_error( error, @@ -952,7 +1006,7 @@ fn insert_values_rejects_values_with_wrong_type() { fn insert_values_rejects_null_for_non_nullable_columns() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let plan = insert_values_plan( &database, @@ -963,7 +1017,8 @@ fn insert_values_rejects_null_for_non_nullable_columns() { ], vec![vec![Value::Integer(1), Value::Null, Value::Boolean(true)]], ); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); assert!(executor.execute(plan).is_err_and(|error| is_null_value_error(error, "name"))); } @@ -972,8 +1027,9 @@ fn insert_values_rejects_null_for_non_nullable_columns() { fn failed_insert_does_not_write_partial_row() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); - let mut executor = Executor::new(&database); + create_table(&database, "users", users_schema()).unwrap(); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); let valid = insert_values_plan( &database, @@ -1012,7 +1068,7 @@ fn failed_insert_does_not_write_partial_row() { fn failed_multi_row_insert_rolls_back_rows_already_inserted_in_statement() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let result = execute_sql( &database, @@ -1033,8 +1089,8 @@ fn failed_multi_row_insert_in_explicit_transaction_rolls_back_statement_before_c let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); let mut session = Session::new(&database); @@ -1077,7 +1133,7 @@ fn failed_multi_row_insert_in_explicit_transaction_rolls_back_statement_before_c fn savepoint_rollback_error_takes_precedence_over_executor_error() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let mut session = Session::new(&database); execute_sql_with_session(&mut session, "BEGIN;").unwrap(); @@ -1099,7 +1155,7 @@ fn savepoint_rollback_error_takes_precedence_over_executor_error() { fn wal_logging_failure_during_explicit_statement_is_reported_immediately() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let mut session = Session::new(&database); execute_sql_with_session(&mut session, "BEGIN;").unwrap(); @@ -1112,7 +1168,7 @@ fn wal_logging_failure_during_explicit_statement_is_reported_immediately() { assert!(matches!( result, Err(DatabaseError::Storage(StorageError::Internal(InternalError::InvariantViolation( - InvariantViolation::TransactionPoisoned { txn_id: 1 } + InvariantViolation::TransactionPoisoned { txn_id: 2 } )))) )); } @@ -1121,14 +1177,15 @@ fn wal_logging_failure_during_explicit_statement_is_reported_immediately() { fn create_index_backfills_existing_table_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let insert = Parser::new( "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", ) .stmt() .unwrap(); let insert_plan = Planner::new(&database).plan_statement(&insert).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); executor.execute(insert_plan.physical).unwrap(); let create_index = Parser::new("CREATE INDEX idx_users_name ON users (name);").stmt().unwrap(); @@ -1146,10 +1203,11 @@ fn create_index_backfills_existing_table_rows() { fn insert_values_updates_existing_secondary_indexes() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); let create_index = Parser::new("CREATE INDEX idx_users_name ON users (name);").stmt().unwrap(); let create_index_plan = Planner::new(&database).plan_statement(&create_index).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &["users"]); + let mut executor = Executor::in_transaction(&transaction); executor.execute(create_index_plan.physical).unwrap(); let insert = Parser::new("INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") @@ -1170,7 +1228,7 @@ fn secondary_indexes_allow_duplicate_values_and_persist_entries() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( @@ -1191,7 +1249,7 @@ fn secondary_indexes_allow_duplicate_values_and_persist_entries() { fn delete_all_rows_returns_count_and_empties_table() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", @@ -1209,7 +1267,7 @@ fn delete_all_rows_returns_count_and_empties_table() { fn delete_streams_across_leaf_merges_without_skipping_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, &insert_many_large_users_sql(40, 300)).unwrap(); let output = execute_sql(&database, "DELETE FROM users;").unwrap(); @@ -1225,7 +1283,7 @@ fn delete_streams_across_leaf_merges_without_skipping_rows() { fn delete_where_removes_only_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", @@ -1243,7 +1301,7 @@ fn delete_where_removes_only_matching_rows() { fn delete_where_primary_key_range_removes_only_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, &insert_many_users_sql(25)).unwrap(); let output = execute_sql(&database, "DELETE FROM users WHERE 10 <= id AND id < 20;").unwrap(); @@ -1259,7 +1317,7 @@ fn delete_where_primary_key_range_removes_only_matching_rows() { fn delete_without_matches_returns_zero_rows_affected() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1273,7 +1331,7 @@ fn delete_without_matches_returns_zero_rows_affected() { fn delete_removes_secondary_index_entries() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -1294,7 +1352,7 @@ fn delete_removes_secondary_index_entries() { fn delete_where_indexed_predicate_removes_all_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -1317,7 +1375,7 @@ fn delete_where_indexed_predicate_removes_all_matching_rows() { fn delete_with_non_boolean_where_does_not_delete_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1336,7 +1394,7 @@ fn delete_with_non_boolean_where_does_not_delete_rows() { fn update_all_rows_returns_count_and_replaces_values() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", @@ -1355,7 +1413,7 @@ fn update_all_rows_returns_count_and_replaces_values() { fn update_streams_across_leaf_splits_without_revisiting_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, &insert_many_large_users_sql(40, 40)).unwrap(); let updated_name = "y".repeat(500); @@ -1372,7 +1430,7 @@ fn update_streams_across_leaf_splits_without_revisiting_rows() { fn update_where_replaces_only_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', FALSE);", @@ -1396,7 +1454,7 @@ fn update_where_replaces_only_matching_rows() { fn update_where_primary_key_range_replaces_only_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, &insert_many_users_sql(25)).unwrap(); let output = @@ -1414,7 +1472,7 @@ fn update_where_primary_key_range_replaces_only_matching_rows() { fn update_without_matches_returns_zero_rows_affected() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1428,7 +1486,7 @@ fn update_without_matches_returns_zero_rows_affected() { fn update_assignment_expression_reads_original_row_values() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1446,7 +1504,7 @@ fn update_assignment_expression_reads_original_row_values() { fn update_rejects_primary_key_assignment() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1462,7 +1520,7 @@ fn update_rejects_primary_key_assignment() { fn update_rejects_wrong_type_without_changing_row() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1478,7 +1536,7 @@ fn update_rejects_wrong_type_without_changing_row() { fn update_with_non_boolean_where_does_not_change_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1497,7 +1555,7 @@ fn update_with_non_boolean_where_does_not_change_rows() { fn update_refreshes_secondary_index_entries() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -1518,7 +1576,7 @@ fn update_refreshes_secondary_index_entries() { fn update_where_indexed_predicate_updates_all_matching_rows() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -1557,27 +1615,27 @@ fn update_where_indexed_predicate_updates_all_matching_rows() { fn update_indexed_range_changes_each_matching_row_once() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database - .create_table( - "scores", - TupleSchema { - columns: vec![ - ColumnSchema { - name: "id".to_owned(), - data_type: DataType::Integer, - nullable: false, - primary_key: true, - }, - ColumnSchema { - name: "score".to_owned(), - data_type: DataType::Integer, - nullable: false, - primary_key: false, - }, - ], - }, - ) - .unwrap(); + create_table( + &database, + "scores", + TupleSchema { + columns: vec![ + ColumnSchema { + name: "id".to_owned(), + data_type: DataType::Integer, + nullable: false, + primary_key: true, + }, + ColumnSchema { + name: "score".to_owned(), + data_type: DataType::Integer, + nullable: false, + primary_key: false, + }, + ], + }, + ) + .unwrap(); execute_sql(&database, "CREATE INDEX idx_scores_score ON scores (score);").unwrap(); execute_sql( &database, @@ -1611,7 +1669,7 @@ fn update_indexed_range_changes_each_matching_row_once() { fn failed_multi_row_update_rolls_back_rows_already_updated_in_statement() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql( &database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE), (2, 'Grace', TRUE);", @@ -1632,7 +1690,7 @@ fn failed_multi_row_update_rolls_back_rows_already_updated_in_statement() { fn explicit_transaction_rollback_restores_updated_rows_and_indexes() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -1654,6 +1712,132 @@ fn explicit_transaction_rollback_restores_updated_rows_and_indexes() { assert_name_index_absent(&database, "Linus"); } +#[test] +fn concurrent_explicit_transactions_are_admitted() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + let mut first = Session::new(&database); + let mut second = Session::new(&database); + + execute_sql_with_session(&mut first, "BEGIN;").unwrap(); + execute_sql_with_session(&mut second, "BEGIN;").unwrap(); + execute_sql_with_session(&mut second, "ROLLBACK;").unwrap(); + execute_sql_with_session(&mut first, "ROLLBACK;").unwrap(); +} + +#[test] +fn same_table_waiter_after_commit_observes_committed_row() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + execute_sql(&database, "CREATE TABLE users (id INT PRIMARY KEY);").unwrap(); + let table_id = TableId::from(database.table_schema_by_name("users").unwrap().table_id); + let mut writer = Session::new(&database); + + execute_sql_with_session(&mut writer, "BEGIN;").unwrap(); + let writer_txn = writer.active_transaction_id_for_test().unwrap(); + execute_sql_with_session(&mut writer, "INSERT INTO users (id) VALUES (1);").unwrap(); + + let observed = thread::scope(|scope| { + let reader = scope.spawn(|| { + execute_sql(&database, "SELECT id FROM users;") + .map(integer_rows) + .map_err(|error| error.to_string()) + }); + wait_until_transaction_waits_for(&database, writer_txn + 1, table_id); + + execute_sql_with_session(&mut writer, "COMMIT;").unwrap(); + reader.join().unwrap() + }); + + assert_eq!(observed.unwrap(), vec![1]); +} + +#[test] +fn same_table_waiter_observes_rollback_restoration() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + execute_sql(&database, "CREATE TABLE users (id INT PRIMARY KEY);").unwrap(); + let table_id = TableId::from(database.table_schema_by_name("users").unwrap().table_id); + let mut writer = Session::new(&database); + + execute_sql_with_session(&mut writer, "BEGIN;").unwrap(); + let writer_txn = writer.active_transaction_id_for_test().unwrap(); + execute_sql_with_session(&mut writer, "INSERT INTO users (id) VALUES (1);").unwrap(); + + let observed = thread::scope(|scope| { + let reader = scope.spawn(|| { + let output = execute_sql(&database, "SELECT id FROM users;").unwrap(); + integer_rows(output) + }); + wait_until_transaction_waits_for(&database, writer_txn + 1, table_id); + + execute_sql_with_session(&mut writer, "ROLLBACK;").unwrap(); + reader.join().unwrap() + }); + + assert!(observed.is_empty()); +} + +#[test] +fn session_deadlock_victim_rolls_back_and_releases_earlier_locks() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + execute_sql(&database, "CREATE TABLE alpha (id INT PRIMARY KEY);").unwrap(); + execute_sql(&database, "CREATE TABLE beta (id INT PRIMARY KEY);").unwrap(); + let beta_id = TableId::from(database.table_schema_by_name("beta").unwrap().table_id); + let mut first = Session::new(&database); + let mut second = Session::new(&database); + + execute_sql_with_session(&mut first, "BEGIN;").unwrap(); + execute_sql_with_session(&mut second, "BEGIN;").unwrap(); + let first_txn = first.active_transaction_id_for_test().unwrap(); + execute_sql_with_session(&mut first, "INSERT INTO alpha (id) VALUES (1);").unwrap(); + execute_sql_with_session(&mut second, "INSERT INTO beta (id) VALUES (2);").unwrap(); + + thread::scope(|scope| { + let winner = scope.spawn(move || { + execute_sql_with_session(&mut first, "INSERT INTO beta (id) VALUES (3);") + .map_err(|error| error.to_string())?; + execute_sql_with_session(&mut first, "ROLLBACK;").map_err(|error| error.to_string())?; + Ok::<(), String>(()) + }); + wait_until_transaction_waits_for(&database, first_txn, beta_id); + + assert!(matches!( + execute_sql_with_session(&mut second, "INSERT INTO alpha (id) VALUES (4);"), + Err(DatabaseError::Storage(StorageError::Lock(LockError::Deadlock { .. }))) + )); + assert_eq!(second.active_transaction_id_for_test(), None); + winner.join().unwrap().unwrap(); + }); + + assert!(integer_rows(execute_sql(&database, "SELECT id FROM alpha;").unwrap()).is_empty()); + assert!(integer_rows(execute_sql(&database, "SELECT id FROM beta;").unwrap()).is_empty()); +} + +#[test] +fn planner_observes_uncommitted_ddl_before_admission_rejects_execution() { + let dir = tempdir().unwrap(); + let database = Database::create(dir.path().join("test.db")).unwrap(); + let mut ddl = Session::new(&database); + + execute_sql_with_session(&mut ddl, "BEGIN;").unwrap(); + execute_sql_with_session(&mut ddl, "CREATE TABLE staged (id INT PRIMARY KEY);").unwrap(); + + // Reaching lock admission rather than returning TableNotFound proves that + // planning observed the uncommitted catalog row. + assert!(matches!( + execute_sql(&database, "SELECT id FROM staged;"), + Err(DatabaseError::Storage(StorageError::Lock(LockError::DdlBusy { .. }))) + )); + + execute_sql_with_session(&mut ddl, "ROLLBACK;").unwrap(); + assert!(matches!( + execute_sql(&database, "SELECT id FROM staged;"), + Err(DatabaseError::Planner(PlannerError::TableNotFound { .. })) + )); +} + #[test] fn explicit_transaction_commit_persists_schema_rows_and_indexes_after_reopen() { let dir = tempdir().unwrap(); @@ -1714,7 +1898,7 @@ ROLLBACK; fn explicit_transaction_rollback_restores_deleted_rows_and_indexes() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); execute_sql(&database, "CREATE INDEX idx_users_name ON users (name);").unwrap(); execute_sql( &database, @@ -1887,7 +2071,7 @@ fn nested_begin_errors_without_ending_outer_transaction() { fn dropping_session_with_active_transaction_rolls_back_and_releases_database_handle() { let dir = tempdir().unwrap(); let database = Database::create(dir.path().join("test.db")).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); { let mut session = Session::new(&database); @@ -1938,7 +2122,7 @@ fn committed_create_index_backfill_recovers_from_wal_after_crash_without_databas let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); database.flush().unwrap(); execute_sql( @@ -1962,7 +2146,7 @@ fn committed_large_insert_with_btree_splits_recovers_from_wal_after_crash() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); database.flush().unwrap(); execute_sql(&database, &insert_many_users_sql(500)).unwrap(); @@ -1981,7 +2165,7 @@ fn committed_overflow_insert_recovers_from_wal_after_crash() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); database.flush().unwrap(); let large_name = "x".repeat(PAGE_SIZE * 3); let insert = format!("INSERT INTO users (id, name, active) VALUES (1, '{large_name}', TRUE);"); @@ -2005,8 +2189,8 @@ fn failed_indexed_multi_row_insert_rolls_back_after_reopen() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); let result = execute_sql( @@ -2034,16 +2218,16 @@ fn uncommitted_flushed_insert_is_undone_during_recovery() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); let txn_id = database.begin_transaction().unwrap(); let table = database.table_schema_by_name("users").unwrap(); - let transaction = database.transaction(txn_id); - let access = ExecutionDatabase::Transaction(&transaction); + let leases = database.acquire_table_leases(txn_id, &[table.table_id.into()]).unwrap(); + let transaction = database.transaction(txn_id, leases); execute_insert_values( - &access, + &transaction, table, vec![ bound("id", 0, DataType::Integer), @@ -2087,7 +2271,7 @@ fn committed_insert_recovers_from_wal_after_crash_without_database_flush() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); database.flush().unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -2109,8 +2293,8 @@ fn committed_delete_recovers_from_wal_after_crash() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); execute_sql( &database, @@ -2134,8 +2318,8 @@ fn committed_update_recovers_from_wal_after_crash() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -2161,8 +2345,8 @@ fn uncommitted_flushed_delete_is_undone_during_recovery() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -2187,8 +2371,8 @@ fn uncommitted_flushed_update_is_undone_during_recovery() { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); database.flush().unwrap(); execute_sql(&database, "INSERT INTO users (id, name, active) VALUES (1, 'Ada', TRUE);") .unwrap(); @@ -2216,7 +2400,8 @@ fn select_without_from_executes_through_one_row_and_project() { let database = Database::create(dir.path().join("test.db")).unwrap(); let statement = Parser::new("SELECT 1 + 2;").stmt().unwrap(); let plan = Planner::new(&database).plan_statement(&statement).unwrap(); - let mut executor = Executor::new(&database); + let transaction = executor_transaction(&database, &[]); + let mut executor = Executor::in_transaction(&transaction); let rows = collect_rows(executor.execute(plan.physical).unwrap()).unwrap(); diff --git a/src/lib.rs b/src/lib.rs index 57342e5..703c57b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,6 +2,9 @@ pub mod client; pub mod core; pub mod error; pub mod executor; +#[cfg(all(test, loom))] +#[allow(clippy::unwrap_used)] +pub(crate) mod loom_support; pub mod planner; pub mod protocol; pub(crate) mod relational; @@ -9,3 +12,4 @@ pub mod server; pub mod session; pub mod sql_parser; pub(crate) mod storage; +pub(crate) mod sync; diff --git a/src/loom_support.rs b/src/loom_support.rs new file mode 100644 index 0000000..70a03df --- /dev/null +++ b/src/loom_support.rs @@ -0,0 +1,32 @@ +//! Shared configuration for bounded Loom models. + +use std::sync::Arc as StdArc; + +use loom::model::Builder; + +pub(crate) mod thread { + pub(crate) use loom::thread::yield_now; + + const STACK_SIZE: usize = 256 * 1024; + + pub(crate) fn spawn( + function: impl FnOnce() -> T + Send + 'static, + ) -> loom::thread::JoinHandle + where + T: Send + 'static, + { + loom::thread::Builder::new().stack_size(STACK_SIZE).spawn(function).unwrap() + } +} + +/// Explores a focused two-thread race with bounded scheduling choices. +pub(crate) fn check_model(model: impl Fn() + Send + Sync + 'static) { + let model = StdArc::new(model); + let mut builder = Builder::new(); + builder.preemption_bound = Some(2); + builder.max_branches = 200; + builder.check(move || { + let model = StdArc::clone(&model); + thread::spawn(move || model()).join().unwrap(); + }); +} diff --git a/src/planner/tests.rs b/src/planner/tests.rs index b21e872..380497a 100644 --- a/src/planner/tests.rs +++ b/src/planner/tests.rs @@ -2,7 +2,10 @@ use tempfile::tempdir; use super::*; use crate::{ - core::{ColumnSchema, DataType}, + core::{ + ColumnSchema, DataType, + test_utils::{create_index, create_table}, + }, sql_parser::parser::Parser, }; @@ -39,7 +42,7 @@ fn database_with_users() -> (tempfile::TempDir, Database) { let dir = tempdir().unwrap(); let path = dir.path().join("test.db"); let database = Database::create(&path).unwrap(); - database.create_table("users", users_schema()).unwrap(); + create_table(&database, "users", users_schema()).unwrap(); (dir, database) } @@ -252,7 +255,7 @@ fn update_where_binds_filter_and_assignment_column_refs() { #[test] fn update_where_secondary_index_predicate_uses_full_table_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_age", "users", &["age"]).unwrap(); + create_index(&database, "idx_users_age", "users", &["age"]).unwrap(); let planner = Planner::new(&database); let statement = parse("UPDATE users SET age = age + 1 WHERE age < 3;"); @@ -346,7 +349,7 @@ fn delete_where_binds_column_refs_in_filter() { #[test] fn delete_where_secondary_index_predicate_uses_full_table_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("DELETE FROM users WHERE name == 'Ada';"); @@ -550,7 +553,7 @@ fn select_non_leading_primary_key_range_stays_full_table_scan() { #[test] fn select_secondary_index_integer_column_range_uses_index_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_age", "users", &["age"]).unwrap(); + create_index(&database, "idx_users_age", "users", &["age"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE 18 <= age AND age < 65;"); @@ -578,7 +581,7 @@ fn select_secondary_index_integer_column_range_uses_index_scan() { #[test] fn select_secondary_index_equality_uses_index_scan_with_residual_filter() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE name == 'Ada';"); @@ -616,7 +619,7 @@ fn select_secondary_index_equality_uses_index_scan_with_residual_filter() { #[test] fn primary_key_scan_wins_over_secondary_index_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE id == 1 AND name == 'Ada';"); @@ -643,7 +646,7 @@ fn primary_key_scan_wins_over_secondary_index_scan() { #[test] fn select_text_secondary_index_range_stays_full_table_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE name >= 'Amy' AND name <= 'Charlotte';"); @@ -663,7 +666,7 @@ fn select_text_secondary_index_range_stays_full_table_scan() { #[test] fn select_reversed_text_secondary_index_range_stays_full_table_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE 'Amy' < name AND 'Charlotte' >= name;"); @@ -683,7 +686,7 @@ fn select_reversed_text_secondary_index_range_stays_full_table_scan() { #[test] fn secondary_index_selection_skips_text_range_conjuncts() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE age == 7 AND name >= 'Amy';"); @@ -703,8 +706,8 @@ fn secondary_index_selection_skips_text_range_conjuncts() { #[test] fn leftmost_usable_secondary_index_predicate_wins() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); - database.create_index("idx_users_age", "users", &["age"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_age", "users", &["age"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE age == 7 AND name == 'Ada';"); @@ -733,8 +736,8 @@ fn leftmost_usable_secondary_index_predicate_wins() { #[test] fn earliest_created_exact_secondary_index_wins_for_same_column() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name_first", "users", &["name"]).unwrap(); - database.create_index("idx_users_name_second", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name_first", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name_second", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE name == 'Ada';"); @@ -755,7 +758,7 @@ fn earliest_created_exact_secondary_index_wins_for_same_column() { #[test] fn unindexed_equality_stays_full_table_scan() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("SELECT name FROM users WHERE age == 7;"); @@ -849,7 +852,7 @@ fn explain_update_wraps_planned_update() { #[test] fn explain_delete_wraps_planned_delete() { let (_dir, database) = database_with_users(); - database.create_index("idx_users_name", "users", &["name"]).unwrap(); + create_index(&database, "idx_users_name", "users", &["name"]).unwrap(); let planner = Planner::new(&database); let statement = parse("EXPLAIN DELETE FROM users WHERE name == 'Ada';"); diff --git a/src/relational/catalog_manager.rs b/src/relational/catalog_manager.rs index 6d03b9e..adcb11e 100644 --- a/src/relational/catalog_manager.rs +++ b/src/relational/catalog_manager.rs @@ -1,7 +1,7 @@ use std::path::Path; use crate::core::{ - CatalogId, IndexSchema, PageId, TableRecord, TableSchema, Tuple, TupleSchema, + CatalogId, IndexSchema, PageId, TableRecord, TableSchema, Tuple, TupleSchema, TxnId, error::{ ConstraintError, CorruptionComponent, CorruptionError, CorruptionKind, InternalError, InvalidArgumentError, InvariantViolation, StorageError, StorageResult, @@ -24,16 +24,21 @@ use crate::storage::engine::Storage; #[derive(Clone)] pub struct CatalogManager { storage: Storage, + txn_id: Option, } impl CatalogManager { pub(crate) fn from_storage(storage: Storage) -> StorageResult { - let manager = Self { storage }; + let manager = Self { storage, txn_id: None }; manager.initialize_or_validate_system_catalog()?; manager.validate_page_formats()?; Ok(manager) } + pub(crate) fn for_transaction(&self, txn_id: TxnId) -> Self { + Self { storage: self.storage.clone(), txn_id: Some(txn_id) } + } + /// Returns the database-file path associated with this manager. pub fn path(&self) -> &Path { self.storage.path() @@ -54,7 +59,11 @@ impl CatalogManager { } let table_id = self.next_object_id()?; - let root_page_id = self.storage.create_tree()?.root_page_id(); + let root_page_id = match self.txn_id { + Some(txn_id) => self.storage.transaction_create_tree(txn_id)?, + None => self.storage.create_tree()?, + } + .root_page_id(); let schema = TableSchema { table_id, name: name.to_owned(), root_page_id, row }; self.insert_table_catalog_row(&schema.catalog_row())?; @@ -131,7 +140,11 @@ impl CatalogManager { }); } - let root_page_id = self.storage.create_tree()?.root_page_id(); + let root_page_id = match self.txn_id { + Some(txn_id) => self.storage.transaction_create_tree(txn_id)?, + None => self.storage.create_tree()?, + } + .root_page_id(); let schema = IndexSchema { index_id, name: name.to_owned(), @@ -150,13 +163,31 @@ impl CatalogManager { /// Returns a typed cursor for the cataloged table named `name`. pub fn table_cursor_by_name(&self, name: &str) -> StorageResult { let schema = self.table_schema_by_name(name)?; - Ok(self.table_cursor(schema.root_page_id)) + self.table_cursor(schema.root_page_id) + } + + pub(crate) fn transaction_table_cursor_by_name( + &self, + txn_id: crate::core::TxnId, + name: &str, + ) -> StorageResult { + let schema = self.table_schema_by_name(name)?; + Ok(TableCursor::new(self.storage.transaction_tree_cursor(txn_id, schema.root_page_id)?)) } /// Returns a typed cursor for the cataloged index named `name`. pub fn index_cursor_by_name(&self, name: &str) -> StorageResult { let schema = self.index_schema_by_name(name)?; - Ok(self.index_cursor(schema.root_page_id)) + self.index_cursor(schema.root_page_id) + } + + pub(crate) fn transaction_index_cursor_by_name( + &self, + txn_id: crate::core::TxnId, + name: &str, + ) -> StorageResult { + let schema = self.index_schema_by_name(name)?; + Ok(IndexCursor::new(self.storage.transaction_tree_cursor(txn_id, schema.root_page_id)?)) } fn initialize_or_validate_system_catalog(&self) -> StorageResult<()> { @@ -169,7 +200,11 @@ impl CatalogManager { } fn initialize_system_root(&self, expected_page_id: PageId) -> StorageResult<()> { - let actual_page_id = self.storage.create_tree()?.root_page_id(); + let actual_page_id = match self.txn_id { + Some(txn_id) => self.storage.transaction_create_tree(txn_id)?, + None => self.storage.create_tree()?, + } + .root_page_id(); if actual_page_id == expected_page_id { Ok(()) } else { @@ -185,8 +220,8 @@ impl CatalogManager { } fn seed_system_catalog(&self) -> StorageResult<()> { - let mut tables = self.table_cursor(SYS_TABLES_ROOT_PAGE_ID); - let mut columns = self.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID); + let mut tables = self.table_cursor(SYS_TABLES_ROOT_PAGE_ID)?; + let mut columns = self.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID)?; for schema in system_table_schemas() { let bytes = schema.catalog_row().to_bytes()?; @@ -201,12 +236,20 @@ impl CatalogManager { Ok(()) } - fn table_cursor(&self, root_page_id: PageId) -> TableCursor { - TableCursor::new(self.storage.tree_cursor(root_page_id)) + fn table_cursor(&self, root_page_id: PageId) -> StorageResult { + let cursor = match self.txn_id { + Some(txn_id) => self.storage.transaction_tree_cursor(txn_id, root_page_id)?, + None => self.storage.tree_cursor(root_page_id)?, + }; + Ok(TableCursor::new(cursor)) } - fn index_cursor(&self, root_page_id: PageId) -> IndexCursor { - IndexCursor::new(self.storage.tree_cursor(root_page_id)) + fn index_cursor(&self, root_page_id: PageId) -> StorageResult { + let cursor = match self.txn_id { + Some(txn_id) => self.storage.transaction_tree_cursor(txn_id, root_page_id)?, + None => self.storage.tree_cursor(root_page_id)?, + }; + Ok(IndexCursor::new(cursor)) } fn validate_page_formats(&self) -> StorageResult<()> { @@ -335,7 +378,7 @@ impl CatalogManager { catalog_table_name: &'static str, decode: impl Fn(&Tuple) -> Result, ) -> StorageResult> { - let mut cursor = self.table_cursor(root_page_id); + let mut cursor = self.table_cursor(root_page_id)?; let mut rows = Vec::new(); if !cursor.seek_to_first()? { return Ok(rows); @@ -376,7 +419,7 @@ impl CatalogManager { table_key: CatalogId, tuple: &Tuple, ) -> StorageResult<()> { - let mut cursor = self.table_cursor(root_page_id); + let mut cursor = self.table_cursor(root_page_id)?; let bytes = tuple.to_bytes()?; cursor.insert(table_key, &bytes) } @@ -508,7 +551,7 @@ mod tests { assert_eq!(manager.storage.create_tree().unwrap().root_page_id(), 4); - let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID); + let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID).unwrap(); assert_table_catalog_row( &mut tables, SYS_TABLES_TABLE_ID, @@ -528,7 +571,7 @@ mod tests { SYS_COLUMNS_ROOT_PAGE_ID, ); - let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID); + let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID).unwrap(); for row in system_column_rows() { let record = columns.get(row.column_id).unwrap().expect("system column row should exist"); @@ -556,7 +599,7 @@ mod tests { } let manager = open(file.path()).unwrap(); - let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID); + let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID).unwrap(); assert_table_catalog_row( &mut tables, SYS_TABLES_TABLE_ID, @@ -606,11 +649,11 @@ mod tests { table.root_page_id ); - let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID); + let mut tables = manager.table_cursor(SYS_TABLES_ROOT_PAGE_ID).unwrap(); assert_table_catalog_row(&mut tables, table.table_id, "users", table.root_page_id); let first_user_column_id = CatalogId::try_from(system_column_rows().len()).unwrap() + 1; - let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID); + let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID).unwrap(); assert_column_catalog_row( &mut columns, ColumnCatalogRow { @@ -695,7 +738,7 @@ mod tests { index.root_page_id ); - let mut indexes = manager.table_cursor(SYS_INDEXES_ROOT_PAGE_ID); + let mut indexes = manager.table_cursor(SYS_INDEXES_ROOT_PAGE_ID).unwrap(); assert_index_catalog_row( &mut indexes, IndexCatalogRow { @@ -710,7 +753,7 @@ mod tests { let index_column_id = CatalogId::try_from(system_column_rows().len()).unwrap() + CatalogId::try_from(table.row.columns.len()).unwrap() + 1; - let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID); + let mut columns = manager.table_cursor(SYS_COLUMNS_ROOT_PAGE_ID).unwrap(); assert_column_catalog_row( &mut columns, ColumnCatalogRow { diff --git a/src/relational/cursor.rs b/src/relational/cursor.rs index b8069a1..47fd446 100644 --- a/src/relational/cursor.rs +++ b/src/relational/cursor.rs @@ -292,12 +292,14 @@ impl IndexCursor { /// Looks up an index entry by key without eagerly copying page-resident bytes. #[cfg(test)] + #[cfg_attr(all(test, loom), allow(dead_code))] pub fn get_entry(&mut self, key: &[u8]) -> StorageResult> { self.inner.get(key)?.map(IndexEntry::try_from).transpose() } /// Replaces the table key stored for an existing index `key`. #[cfg(test)] + #[cfg_attr(all(test, loom), allow(dead_code))] pub fn update(&mut self, key: &[u8], table_key: TableKey) -> StorageResult<()> { self.inner.update(key, &encode_index_table_key(table_key)) } @@ -402,9 +404,10 @@ impl TryFrom for IndexEntry { } } -#[cfg(test)] +#[cfg(all(test, not(loom)))] +#[allow(clippy::unwrap_used)] mod tests { - use std::rc::Rc; + use std::sync::Arc; use tempfile::NamedTempFile; @@ -422,20 +425,20 @@ mod tests { let file = NamedTempFile::new().unwrap(); let disk_manager = DiskManager::new(file.path()).unwrap(); let runtime = - Rc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); + Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); PageCache::new(runtime, cache_frames).unwrap() } fn temp_table_cursor(cache_frames: usize) -> TableCursor { let page_cache = temp_page_cache(cache_frames); - let root_page_id = initialize_empty_root(&page_cache).unwrap(); - TableCursor::new(TreeCursor::new(page_cache, root_page_id)) + let root_page_id = initialize_empty_root(&page_cache, None).unwrap(); + TableCursor::new(TreeCursor::new(page_cache, root_page_id).unwrap()) } fn temp_index_cursor(cache_frames: usize) -> IndexCursor { let page_cache = temp_page_cache(cache_frames); - let root_page_id = initialize_empty_root(&page_cache).unwrap(); - IndexCursor::new(TreeCursor::new(page_cache, root_page_id)) + let root_page_id = initialize_empty_root(&page_cache, None).unwrap(); + IndexCursor::new(TreeCursor::new(page_cache, root_page_id).unwrap()) } #[test] diff --git a/src/relational/index_manager.rs b/src/relational/index_manager.rs index b58b2e7..50c3140 100644 --- a/src/relational/index_manager.rs +++ b/src/relational/index_manager.rs @@ -1,5 +1,6 @@ use crate::core::{ - IndexSchema, OwnedTableRecord, TableKey, TableRecord, TableSchema, Tuple, TupleView, Value, + IndexSchema, OwnedTableRecord, TableKey, TableRecord, TableSchema, Tuple, TupleView, TxnId, + Value, error::{CorruptionComponent, CorruptionError, CorruptionKind, StorageError, StorageResult}, }; use crate::relational::{catalog_manager::CatalogManager, cursor::encode_index_entry_key}; @@ -13,19 +14,20 @@ pub(crate) fn create_index( ) -> StorageResult { let table = catalog.table_schema_by_name(table_name)?; let index = catalog.create_index(name, table_name, columns)?; - backfill_index(catalog, &table, &index)?; + backfill_index(catalog, None, &table, &index)?; Ok(index) } pub(crate) fn insert_index_entries( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, record: &OwnedTableRecord, ) -> StorageResult<()> { for index in catalog.index_schemas_for_table(table)? { let key = index_key_from_record(table, &index, record)?; let key = encode_index_entry_key(&key, record.table_key); - let mut index_cursor = catalog.index_cursor_by_name(&index.name)?; + let mut index_cursor = index_cursor(catalog, txn_id, &index.name)?; index_cursor.insert(&key, record.table_key)?; } Ok(()) @@ -33,13 +35,14 @@ pub(crate) fn insert_index_entries( pub(crate) fn delete_index_entries( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, record: &OwnedTableRecord, ) -> StorageResult<()> { for index in catalog.index_schemas_for_table(table)? { let key = index_key_from_record(table, &index, record)?; let key = encode_index_entry_key(&key, record.table_key); - let mut index_cursor = catalog.index_cursor_by_name(&index.name)?; + let mut index_cursor = index_cursor(catalog, txn_id, &index.name)?; index_cursor.delete(&key)?; } Ok(()) @@ -47,11 +50,12 @@ pub(crate) fn delete_index_entries( fn backfill_index( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, index: &IndexSchema, ) -> StorageResult<()> { let mut table_cursor = catalog.table_cursor_by_name(&table.name)?; - let mut index_cursor = catalog.index_cursor_by_name(&index.name)?; + let mut index_cursor = index_cursor(catalog, txn_id, &index.name)?; while let Some(record) = table_cursor.next_record()? { let key = index_key_from_table_record(table, index, &record)?; let table_key = record.table_key(); @@ -61,6 +65,17 @@ fn backfill_index( Ok(()) } +fn index_cursor( + catalog: &CatalogManager, + txn_id: Option, + name: &str, +) -> StorageResult { + match txn_id { + Some(txn_id) => catalog.transaction_index_cursor_by_name(txn_id, name), + None => catalog.index_cursor_by_name(name), + } +} + /// Compatibility wrapper retained only for focused module tests during the /// manager-to-operation migration. #[cfg(test)] diff --git a/src/relational/record_manager.rs b/src/relational/record_manager.rs index 8f78b14..aea9270 100644 --- a/src/relational/record_manager.rs +++ b/src/relational/record_manager.rs @@ -1,7 +1,7 @@ use crate::core::{ CorruptionComponent, CorruptionError, CorruptionKind, DataType, IndexEntry, IndexKeyRange, IndexSchema, OwnedTableRecord, TableKey, TableKeyBound, TableKeyRange, TableRecord, - TableSchema, Tuple, Value, + TableSchema, Tuple, TxnId, Value, error::{ConstraintError, InvalidArgumentError, StorageError, StorageResult}, }; use crate::relational::{ @@ -39,17 +39,19 @@ pub(crate) struct IndexScan { pub(crate) fn scan_table( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, ) -> StorageResult { - scan_table_range(catalog, table, TableKeyRange::unbounded()) + scan_table_range(catalog, txn_id, table, TableKeyRange::unbounded()) } pub(crate) fn scan_table_range( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, range: TableKeyRange, ) -> StorageResult { - let cursor = catalog.table_cursor_by_name(&table.name)?; + let cursor = table_cursor(catalog, txn_id, &table.name)?; let observed_mutation_epoch = cursor.mutation_epoch(); Ok(TableScan { cursor, @@ -63,14 +65,15 @@ pub(crate) fn scan_table_range( pub(crate) fn scan_index( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, index: &IndexSchema, key_range: IndexKeyRange, ) -> StorageResult { Ok(IndexScan { table: table.clone(), - table_cursor: catalog.table_cursor_by_name(&table.name)?, - index_cursor: catalog.index_cursor_by_name(&index.name)?, + table_cursor: table_cursor(catalog, txn_id, &table.name)?, + index_cursor: index_cursor(catalog, txn_id, &index.name)?, key_range, initialized: false, done: false, @@ -79,6 +82,7 @@ pub(crate) fn scan_index( pub(crate) fn insert_table_row( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, values: Vec, ) -> StorageResult { @@ -86,26 +90,28 @@ pub(crate) fn insert_table_row( let table_key = table_key_from_values(table, &values)?; let record = Tuple::new(values).to_bytes()?; - let mut table_cursor = catalog.table_cursor_by_name(&table.name)?; + let mut table_cursor = table_cursor(catalog, txn_id, &table.name)?; table_cursor.insert(table_key, &record)?; let record = OwnedTableRecord { table_key, record: record.into_boxed_slice() }; - index_manager::insert_index_entries(catalog, table, &record)?; + index_manager::insert_index_entries(catalog, txn_id, table, &record)?; Ok(record) } pub(crate) fn delete_table_row( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, record: &OwnedTableRecord, ) -> StorageResult<()> { - index_manager::delete_index_entries(catalog, table, record)?; - let mut table_cursor = catalog.table_cursor_by_name(&table.name)?; + index_manager::delete_index_entries(catalog, txn_id, table, record)?; + let mut table_cursor = table_cursor(catalog, txn_id, &table.name)?; table_cursor.delete(record.table_key) } pub(crate) fn update_table_row( catalog: &CatalogManager, + txn_id: Option, table: &TableSchema, record: &OwnedTableRecord, values: Vec, @@ -123,42 +129,64 @@ pub(crate) fn update_table_row( let updated = OwnedTableRecord { table_key: record.table_key, record: updated.into_boxed_slice() }; - index_manager::delete_index_entries(catalog, table, record)?; - let mut table_cursor = catalog.table_cursor_by_name(&table.name)?; + index_manager::delete_index_entries(catalog, txn_id, table, record)?; + let mut table_cursor = table_cursor(catalog, txn_id, &table.name)?; table_cursor.update(record.table_key, &updated.record)?; - index_manager::insert_index_entries(catalog, table, &updated)?; + index_manager::insert_index_entries(catalog, txn_id, table, &updated)?; Ok(updated) } +fn table_cursor( + catalog: &CatalogManager, + txn_id: Option, + name: &str, +) -> StorageResult { + match txn_id { + Some(txn_id) => catalog.transaction_table_cursor_by_name(txn_id, name), + None => catalog.table_cursor_by_name(name), + } +} + +fn index_cursor( + catalog: &CatalogManager, + txn_id: Option, + name: &str, +) -> StorageResult { + match txn_id { + Some(txn_id) => catalog.transaction_index_cursor_by_name(txn_id, name), + None => catalog.index_cursor_by_name(name), + } +} + #[cfg(test)] impl RecordManager { pub(crate) fn new(catalog: CatalogManager) -> Self { Self { catalog } } pub(crate) fn scan_table(&self, table: &TableSchema) -> StorageResult { - scan_table(&self.catalog, table) + scan_table(&self.catalog, None, table) } pub(crate) fn scan_table_range( &self, table: &TableSchema, range: TableKeyRange, ) -> StorageResult { - scan_table_range(&self.catalog, table, range) + scan_table_range(&self.catalog, None, table, range) } pub(crate) fn insert_table_row( &self, table: &TableSchema, values: Vec, ) -> StorageResult { - insert_table_row(&self.catalog, table, values) + insert_table_row(&self.catalog, None, table, values) } pub(crate) fn delete_table_row( &self, table: &TableSchema, record: &OwnedTableRecord, ) -> StorageResult<()> { - delete_table_row(&self.catalog, table, record) + delete_table_row(&self.catalog, None, table, record) } pub(crate) fn update_table_row( &self, @@ -166,7 +194,7 @@ impl RecordManager { record: &OwnedTableRecord, values: Vec, ) -> StorageResult { - update_table_row(&self.catalog, table, record, values) + update_table_row(&self.catalog, None, table, record, values) } } diff --git a/src/server.rs b/src/server.rs index 5f3311c..6ab09bc 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1,12 +1,13 @@ -//! Strictly sequential Databas TCP server. +//! Concurrent Databas TCP server. //! -//! The server accepts one connection, serves all requests on that connection -//! synchronously, and only then accepts the next connection. It does not spawn -//! worker threads and does not pipeline queries. +//! Each accepted connection owns a session on a worker thread. Transactions +//! coordinate through shared storage and table leases. use std::{ io, net::{TcpListener, TcpStream}, + sync::Arc, + thread, }; use thiserror::Error; @@ -41,7 +42,7 @@ pub enum ServerError { /// A single-database, single-connection-at-a-time TCP server. pub struct Server { listener: TcpListener, - database: Database, + database: Arc, database_name: String, } @@ -62,24 +63,30 @@ impl Server { ) -> Result { let database_name = database_name.into(); validate_database_name(&database_name)?; - Ok(Self { listener, database, database_name }) + Ok(Self { listener, database: Arc::new(database), database_name }) } - /// Accepts and serves connections forever, one at a time. + /// Accepts connections forever and serves each on a worker thread. /// /// Connection-level I/O and protocol errors close only that connection. - /// The next connection is not accepted until the current connection has - /// closed and the database has been flushed. /// /// # Errors /// /// Returns if accepting a connection or flushing the database fails. pub fn serve(self) -> Result<(), ServerError> { loop { - self.serve_one()?; + let (mut stream, _) = self.listener.accept()?; + let database = Arc::clone(&self.database); + let database_name = self.database_name.clone(); + thread::spawn(move || { + let _ = stream.set_nodelay(true); + let _ = handle_connection(&mut stream, &database, &database_name); + let _ = database.flush(); + }); } } + #[cfg(test)] fn serve_one(&self) -> Result<(), ServerError> { let (mut stream, _) = self.listener.accept()?; let _ = stream.set_nodelay(true); @@ -279,6 +286,7 @@ fn storage_error_code(error: &StorageError) -> ErrorCode { StorageError::Constraint(_) => ErrorCode::ConstraintViolation, StorageError::InvalidArgument(_) => ErrorCode::InvalidArgument, StorageError::LimitExceeded(_) => ErrorCode::LimitExceeded, + StorageError::Lock(_) => ErrorCode::ExecutionError, StorageError::Internal(_) => ErrorCode::InternalError, } } diff --git a/src/session.rs b/src/session.rs index 9d2d9b8..8bfce87 100644 --- a/src/session.rs +++ b/src/session.rs @@ -10,7 +10,9 @@ use crate::storage::transaction_manager::TransactionSavepoint; use crate::{ core::{ Database, + database::StatementTransactionMode, error::{InternalError, InvariantViolation, StorageError}, + lock_manager::TableLease, }, error::DatabaseError, executor::{ExecutionOutput, Executor}, @@ -42,6 +44,11 @@ impl<'db> Session<'db> { Self { database, active_txn: None } } + #[cfg(test)] + pub(crate) fn active_transaction_id_for_test(&self) -> Option { + self.active_txn + } + /// Parses and executes one top-level SQL item. pub fn execute_sql<'sql>( &mut self, @@ -66,17 +73,25 @@ impl<'db> Session<'db> { &mut self, statement: Statement<'sql>, ) -> Result> { - let mutating = statement_is_mutating(&statement); + let transaction_mode = statement_transaction_mode(&statement); let plan = Planner::new(self.database).plan_physical_statement(&statement)?; - - if !mutating { - return self.execute_plan(plan); - } + let table_ids = plan_table_ids(&plan); if let Some(txn_id) = self.active_txn { - self.execute_explicit_transaction_statement(txn_id, plan) + if transaction_mode == StatementTransactionMode::Ddl { + self.database.acquire_ddl_gate(txn_id)?; + } + let leases = match self.database.acquire_table_leases(txn_id, &table_ids) { + Ok(leases) => leases, + Err(error) => { + self.database.rollback_transaction(txn_id)?; + self.active_txn = None; + return Err(error.into()); + } + }; + self.execute_explicit_transaction_statement(txn_id, leases, plan) } else { - self.execute_implicit_transaction(plan) + self.execute_implicit_transaction(plan, table_ids, transaction_mode) } } @@ -109,7 +124,7 @@ impl<'db> Session<'db> { Ok(ExecutionOutput::CommandOk) } Err(error) => { - self.sync_active_transaction(txn_id); + self.sync_active_transaction(txn_id)?; Err(error.into()) } } @@ -123,25 +138,19 @@ impl<'db> Session<'db> { Ok(ExecutionOutput::CommandOk) } Err(error) => { - self.sync_active_transaction(txn_id); + self.sync_active_transaction(txn_id)?; Err(error.into()) } } } - fn execute_plan<'sql>( - &self, - plan: PhysicalPlan, - ) -> Result> { - Ok(Executor::new(self.database).execute(plan)?) - } - fn execute_explicit_transaction_statement<'sql>( &mut self, txn_id: u64, + leases: Vec, plan: PhysicalPlan, ) -> Result> { - let transaction = self.database.transaction(txn_id); + let transaction = self.database.transaction(txn_id, leases); debug_assert_eq!(transaction.id(), txn_id); let savepoint = transaction.statement_savepoint()?; match Executor::in_transaction(&transaction).execute(plan) { @@ -174,14 +183,24 @@ impl<'db> Session<'db> { fn execute_implicit_transaction<'sql>( &self, plan: PhysicalPlan, + table_ids: Vec, + mode: StatementTransactionMode, ) -> Result> { - let txn_id = self.database.begin_transaction()?; - let transaction = self.database.transaction(txn_id); + let txn_id = self.database.begin_statement_transaction(mode)?; + let leases = match self.database.acquire_table_leases(txn_id, &table_ids) { + Ok(leases) => leases, + Err(error) => { + self.database.rollback_transaction(txn_id)?; + return Err(error.into()); + } + }; + let transaction = self.database.transaction(txn_id, leases); match Executor::in_transaction(&transaction).execute(plan) { Ok(output) => match self.database.commit_transaction(txn_id) { Ok(()) => Ok(output), Err(commit_error) => { - if let Err(rollback_error) = self.database.rollback_transaction(txn_id) + if self.database.transaction_is_active(txn_id)? + && let Err(rollback_error) = self.database.rollback_transaction(txn_id) && !is_no_active_transaction(&rollback_error) { return Err(rollback_error.into()); @@ -198,10 +217,11 @@ impl<'db> Session<'db> { } } - fn sync_active_transaction(&mut self, txn_id: u64) { - if self.database.active_transaction_id() != Some(txn_id) { + fn sync_active_transaction(&mut self, txn_id: u64) -> Result<(), StorageError> { + if !self.database.transaction_is_active(txn_id)? { self.active_txn = None; } + Ok(()) } } @@ -213,14 +233,42 @@ impl Drop for Session<'_> { } } -fn statement_is_mutating(statement: &Statement<'_>) -> bool { +fn statement_transaction_mode(statement: &Statement<'_>) -> StatementTransactionMode { match statement { - Statement::CreateTable(_) - | Statement::CreateIndex(_) - | Statement::Insert(_) - | Statement::Update(_) - | Statement::Delete(_) => true, - Statement::Select(_) | Statement::Explain(_) => false, + Statement::CreateTable(_) | Statement::CreateIndex(_) => StatementTransactionMode::Ddl, + _ => StatementTransactionMode::Ordinary, + } +} + +fn plan_table_ids(plan: &PhysicalPlan) -> Vec { + let mut table_ids = Vec::new(); + collect_plan_table_ids(plan, &mut table_ids); + table_ids.sort_unstable(); + table_ids.dedup(); + table_ids +} + +fn collect_plan_table_ids(plan: &PhysicalPlan, table_ids: &mut Vec) { + match plan { + PhysicalPlan::CreateIndex { table, .. } + | PhysicalPlan::InsertValues { table, .. } + | PhysicalPlan::Update { table, .. } + | PhysicalPlan::Delete { table, .. } + | PhysicalPlan::FullTableScan { table } + | PhysicalPlan::PrimaryKeyRangeScan { table, .. } => table_ids.push(table.table_id.into()), + PhysicalPlan::SecondaryIndexScan { scan } => table_ids.push(scan.table.table_id.into()), + _ => {} + } + match plan { + PhysicalPlan::Explain { input } + | PhysicalPlan::Update { input, .. } + | PhysicalPlan::Delete { input, .. } + | PhysicalPlan::Filter { input, .. } + | PhysicalPlan::Sort { input, .. } + | PhysicalPlan::Project { input, .. } + | PhysicalPlan::Offset { input, .. } + | PhysicalPlan::Limit { input, .. } => collect_plan_table_ids(input, table_ids), + _ => {} } } diff --git a/src/storage/btree.rs b/src/storage/btree.rs index c1bfb3e..356bb97 100644 --- a/src/storage/btree.rs +++ b/src/storage/btree.rs @@ -4,10 +4,12 @@ //! only with raw byte keys and raw byte values stored in `RawLeaf` pages //! and separator byte keys stored in `RawInterior` pages. -use std::{borrow::Cow, cell::Cell, cmp::Ordering, rc::Rc}; +use std::{borrow::Cow, cmp::Ordering}; + +use crate::sync::{Arc, AtomicU64, Ordering as AtomicOrdering}; use crate::core::{ - PAGE_SIZE, PageId, + PAGE_SIZE, PageId, TxnId, error::{CorruptionComponent, CorruptionError, CorruptionKind, StorageError, StorageResult}, }; use crate::storage::{ @@ -34,17 +36,44 @@ mod root; mod search; mod split; -#[cfg(test)] +#[cfg(all(test, not(loom)))] +#[allow(clippy::expect_used, clippy::unwrap_used)] mod tests; -#[cfg(test)] +#[cfg(all(test, loom))] +#[allow(clippy::unwrap_used)] +mod loom_tests { + use super::search::advance_mutation_epoch; + use crate::{ + loom_support::{check_model, thread}, + sync::{Arc, AtomicU64, Ordering}, + }; + + #[test] + fn concurrent_mutations_each_advance_the_shared_epoch() { + check_model(|| { + let epoch = Arc::new(AtomicU64::new(0)); + + let first_epoch = Arc::clone(&epoch); + let first = thread::spawn(move || advance_mutation_epoch(&first_epoch)); + let second_epoch = Arc::clone(&epoch); + let second = thread::spawn(move || advance_mutation_epoch(&second_epoch)); + + first.join().unwrap(); + second.join().unwrap(); + assert_eq!(epoch.load(Ordering::Acquire), 2); + }); + } +} + +#[cfg(all(test, not(loom)))] pub use record::OwnedRecord; pub use record::Record; pub(crate) use root::{initialize_empty_root, validate_tree_page_formats}; -#[cfg(test)] +#[cfg(all(test, not(loom)))] use record::RecordStorage; -#[cfg(test)] +#[cfg(all(test, not(loom)))] use root::read_page_kind; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -69,8 +98,9 @@ pub enum CursorState { #[derive(Clone)] pub struct TreeCursor { page_cache: PageCache, - root_page_id: Rc>, - mutation_epoch: Rc>, + root_page_id: Arc, + mutation_epoch: Arc, + txn_id: Option, state: CursorState, } diff --git a/src/storage/btree/mutation.rs b/src/storage/btree/mutation.rs index 8c660a7..07b8e77 100644 --- a/src/storage/btree/mutation.rs +++ b/src/storage/btree/mutation.rs @@ -36,6 +36,7 @@ impl TreeCursor { Some( write_overflow_chain_from_slices( &self.page_cache, + self.txn_id, &key[MAX_INLINE_OVERFLOW_PAYLOAD_BYTES..], value, )? @@ -49,13 +50,14 @@ impl TreeCursor { inline_payload[key.len()..MAX_INLINE_OVERFLOW_PAYLOAD_BYTES] .copy_from_slice(&value[..value_prefix_len]); Some( - overflow::write_chain(&self.page_cache, &value[value_prefix_len..])? - .ok_or_else(|| { - cell_corruption( - self.root_page_id(), - CorruptionKind::CellLengthOutOfBounds, - ) - })?, + overflow::write_chain( + &self.page_cache, + self.txn_id, + &value[value_prefix_len..], + )? + .ok_or_else(|| { + cell_corruption(self.root_page_id(), CorruptionKind::CellLengthOutOfBounds) + })?, ) } } else { @@ -116,6 +118,7 @@ impl TreeCursor { Some( overflow::write_chain( &self.page_cache, + self.txn_id, &key[MAX_INLINE_OVERFLOW_PAYLOAD_BYTES..], )? .ok_or_else(|| { @@ -168,7 +171,7 @@ impl TreeCursor { if has_capacity { let inserted_new_leaf_max; { - let mut leaf_guard = leaf_pin_guard.write()?; + let mut leaf_guard = leaf_pin_guard.write(self.txn_id)?; let mut page = leaf_guard.open_mut::()?; let slot_index = self.insert_leaf_payload_at(&mut page, slot_index, key, value)?; self.mark_tree_mutated(); @@ -207,7 +210,7 @@ impl TreeCursor { if has_capacity { { - let mut leaf_guard = leaf_pin_guard.write()?; + let mut leaf_guard = leaf_pin_guard.write(self.txn_id)?; let mut page = leaf_guard.open_mut::()?; let slot_index = self.update_leaf_payload_at(&mut page, slot_index, key, value)?; self.mark_tree_mutated(); @@ -230,7 +233,7 @@ impl TreeCursor { let (leaf_page_id, tree_path) = self.leaf_page_path_for_key(key)?; { let leaf_pin_guard = self.page_cache.fetch_page(leaf_page_id)?; - let mut leaf_guard = leaf_pin_guard.write()?; + let mut leaf_guard = leaf_pin_guard.write(self.txn_id)?; let mut page = leaf_guard.open_mut::()?; page.delete(key)?; self.mark_tree_mutated(); diff --git a/src/storage/btree/payload.rs b/src/storage/btree/payload.rs index af14dbc..cfa5264 100644 --- a/src/storage/btree/payload.rs +++ b/src/storage/btree/payload.rs @@ -24,6 +24,7 @@ fn read_overflow_next_page_id(page: &[u8; PAGE_SIZE]) -> Option { pub(super) fn write_overflow_chain_from_slices( page_cache: &PageCache, + txn_id: Option, mut first: &[u8], mut second: &[u8], ) -> StorageResult> { @@ -34,9 +35,9 @@ pub(super) fn write_overflow_chain_from_slices( let mut first_page_id = None; let mut previous_page_id = None; while !first.is_empty() || !second.is_empty() { - let (page_id, pin) = page_cache.new_page()?; + let (page_id, pin) = page_cache.new_page(txn_id)?; { - let mut page = pin.write()?; + let mut page = pin.write(txn_id)?; page.page_mut().fill(0); page::format::write_optional_u64(page.page_mut(), 0, None); @@ -60,7 +61,7 @@ pub(super) fn write_overflow_chain_from_slices( } if let Some(previous_page_id) = previous_page_id { let previous_pin = page_cache.fetch_page(previous_page_id)?; - let mut previous_page = previous_pin.write()?; + let mut previous_page = previous_pin.write(txn_id)?; page::format::write_optional_u64(previous_page.page_mut(), 0, Some(page_id)); } previous_page_id = Some(page_id); diff --git a/src/storage/btree/rebalance.rs b/src/storage/btree/rebalance.rs index 8118eb3..1537bd4 100644 --- a/src/storage/btree/rebalance.rs +++ b/src/storage/btree/rebalance.rs @@ -317,7 +317,7 @@ impl TreeCursor { next_page_id: Option, ) -> StorageResult<()> { let pin = self.page_cache.fetch_page(page_id)?; - let mut guard = pin.write()?; + let mut guard = pin.write(self.txn_id)?; let mut leaf = RawLeaf::>::initialize(guard.page_mut()); leaf.set_prev_page_id(prev_page_id); leaf.set_next_page_id(next_page_id); @@ -379,7 +379,7 @@ impl TreeCursor { } let pin = self.page_cache.fetch_page(page_id)?; - let mut guard = pin.write()?; + let mut guard = pin.write(self.txn_id)?; *guard.page_mut() = page_image; Ok(()) } @@ -442,7 +442,7 @@ impl TreeCursor { prev_page_id: Option, ) -> StorageResult<()> { let pin = self.page_cache.fetch_page(page_id)?; - let mut guard = pin.write()?; + let mut guard = pin.write(self.txn_id)?; let mut leaf = guard.open_mut::()?; leaf.set_prev_page_id(prev_page_id); Ok(()) @@ -455,7 +455,7 @@ impl TreeCursor { prev_page_id: Option, ) -> StorageResult<()> { let pin = self.page_cache.fetch_page(page_id)?; - let mut guard = pin.write()?; + let mut guard = pin.write(self.txn_id)?; let mut interior = guard.open_mut::()?; interior.set_prev_page_id(prev_page_id); Ok(()) @@ -662,7 +662,7 @@ impl TreeCursor { children: &[ChildEntry], ) -> StorageResult { let (prev_page_id, next_page_id) = self.read_interior_page_links(page_id)?; - let (right_page_id, right_page_guard) = self.page_cache.new_page()?; + let (right_page_id, right_page_guard) = self.page_cache.new_page(self.txn_id)?; drop(right_page_guard); let split_index = Self::choose_interior_fitting_split(children) diff --git a/src/storage/btree/rebalance_repair.rs b/src/storage/btree/rebalance_repair.rs index 882fbd6..58a6c12 100644 --- a/src/storage/btree/rebalance_repair.rs +++ b/src/storage/btree/rebalance_repair.rs @@ -28,7 +28,7 @@ impl TreeCursor { *child_page.page() }; - let mut root_guard = pin.write()?; + let mut root_guard = pin.write(self.txn_id)?; *root_guard.page_mut() = child_snapshot; drop(root_guard); @@ -39,7 +39,7 @@ impl TreeCursor { fn clear_root_sibling_links(&self, root_page_id: PageId) -> StorageResult<()> { let pin = self.page_cache.fetch_page(root_page_id)?; - let mut guard = pin.write()?; + let mut guard = pin.write(self.txn_id)?; match read_page_kind(guard.page(), root_page_id)? { PageKind::RawLeaf => { let mut leaf = guard.open_mut::()?; diff --git a/src/storage/btree/root.rs b/src/storage/btree/root.rs index fc8725b..ebc8da6 100644 --- a/src/storage/btree/root.rs +++ b/src/storage/btree/root.rs @@ -1,9 +1,12 @@ use super::*; /// Allocates and initializes a brand-new empty raw root leaf page. -pub(crate) fn initialize_empty_root(page_cache: &PageCache) -> StorageResult { - let (page_id, pin) = page_cache.new_page()?; - let mut page = pin.write()?; +pub(crate) fn initialize_empty_root( + page_cache: &PageCache, + txn_id: Option, +) -> StorageResult { + let (page_id, pin) = page_cache.new_page(txn_id)?; + let mut page = pin.write(txn_id)?; let _ = RawLeaf::>::initialize(page.page_mut()); Ok(page_id) } diff --git a/src/storage/btree/search.rs b/src/storage/btree/search.rs index bf52b0a..6f1bf93 100644 --- a/src/storage/btree/search.rs +++ b/src/storage/btree/search.rs @@ -2,6 +2,10 @@ use super::payload::{compare_key_prefix, compare_overflow_key}; use super::root::{expect_page_kind, read_page_kind}; use super::*; +pub(super) fn advance_mutation_epoch(epoch: &AtomicU64) { + epoch.fetch_add(1, AtomicOrdering::Release); +} + /// Outcome of trying to position a scan within or beyond one leaf page. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum LeafSeek { @@ -15,29 +19,36 @@ enum LeafSeek { impl TreeCursor { /// Creates a cursor anchored at `root_page_id` in page-level state. - pub(crate) fn new(page_cache: PageCache, root_page_id: PageId) -> Self { - let mutation_epoch = page_cache.tree_mutation_epoch(root_page_id); - Self { + pub(crate) fn new(page_cache: PageCache, root_page_id: PageId) -> StorageResult { + let mutation_epoch = page_cache.tree_mutation_epoch(root_page_id)?; + Ok(Self { page_cache, - root_page_id: Rc::new(Cell::new(root_page_id)), + root_page_id: Arc::new(AtomicU64::new(root_page_id)), mutation_epoch, + txn_id: None, state: CursorState::Page { page_id: root_page_id }, - } + }) + } + + /// Associates subsequent mutations with an explicit transaction. + pub(crate) fn for_transaction(mut self, txn_id: TxnId) -> Self { + self.txn_id = Some(txn_id); + self } /// Returns the root page id that anchors this tree. pub fn root_page_id(&self) -> PageId { - self.root_page_id.get() + self.root_page_id.load(AtomicOrdering::Acquire) } /// Returns the current mutation epoch shared by cursors over this tree. pub(crate) fn mutation_epoch(&self) -> u64 { - self.mutation_epoch.get() + self.mutation_epoch.load(AtomicOrdering::Acquire) } /// Invalidates physical positions held by other cursors over this tree. pub(super) fn mark_tree_mutated(&self) { - self.mutation_epoch.set(self.mutation_epoch.get().wrapping_add(1)); + advance_mutation_epoch(&self.mutation_epoch); } /// Returns the cursor's current logical state. diff --git a/src/storage/btree/split.rs b/src/storage/btree/split.rs index 034326a..0948d62 100644 --- a/src/storage/btree/split.rs +++ b/src/storage/btree/split.rs @@ -64,7 +64,7 @@ impl TreeCursor { }; if has_capacity { - let mut interior_guard = interior_page_guard.write()?; + let mut interior_guard = interior_page_guard.write(self.txn_id)?; let mut interior_page = interior_guard.open_mut::()?; let inserted_slot_index = self.insert_interior_payload_at( &mut interior_page, @@ -237,12 +237,12 @@ impl TreeCursor { Ok(cells) } - #[cfg(test)] + #[cfg(all(test, not(loom)))] pub(super) fn leaf_cell_storage_is_borrowed_for_test(cell: &LeafSplitCell<'_>) -> (bool, bool) { (matches!(cell.key, Cow::Borrowed(_)), matches!(cell.value, Cow::Borrowed(_))) } - #[cfg(test)] + #[cfg(all(test, not(loom)))] pub(super) fn leaf_cell_storage_is_owned_for_test(cell: &LeafSplitCell<'_>) -> (bool, bool) { (matches!(cell.key, Cow::Owned(_)), matches!(cell.value, Cow::Owned(_))) } @@ -260,7 +260,7 @@ impl TreeCursor { let split_index = Self::choose_leaf_split_index(cells)?; let (left_cells, right_cells) = cells.split_at(split_index); - let (right_page_id, right_page_guard) = self.page_cache.new_page()?; + let (right_page_id, right_page_guard) = self.page_cache.new_page(self.txn_id)?; drop(right_page_guard); let mut left_page_image = [0; PAGE_SIZE]; @@ -286,8 +286,8 @@ impl TreeCursor { { let right_page_guard = self.page_cache.fetch_page(right_page_id)?; - let mut leaf_guard = leaf_pin_guard.write()?; - let mut right_guard = right_page_guard.write()?; + let mut leaf_guard = leaf_pin_guard.write(self.txn_id)?; + let mut right_guard = right_page_guard.write(self.txn_id)?; *leaf_guard.page_mut() = left_page_image; *right_guard.page_mut() = right_page_image; } @@ -295,7 +295,7 @@ impl TreeCursor { if let Some(next_page_id) = next_page_id { let next_page_guard = self.page_cache.fetch_page(next_page_id)?; - let mut next_guard = next_page_guard.write()?; + let mut next_guard = next_page_guard.write(self.txn_id)?; let mut next_page = next_guard.open_mut::()?; next_page.set_prev_page_id(Some(right_page_id)); } @@ -410,7 +410,7 @@ impl TreeCursor { ChildEntry { page_id: incoming_right_page_id, max_key: original_max_key }, ); - let (right_page_id, right_page_guard) = self.page_cache.new_page()?; + let (right_page_id, right_page_guard) = self.page_cache.new_page(self.txn_id)?; drop(right_page_guard); let split_index = Self::choose_interior_fitting_split(&children) @@ -437,7 +437,7 @@ impl TreeCursor { if let Some(next_page_id) = next_page_id { let next_page_guard = self.page_cache.fetch_page(next_page_id)?; - let mut next_guard = next_page_guard.write()?; + let mut next_guard = next_page_guard.write(self.txn_id)?; let mut next_page = next_guard.open_mut::()?; next_page.set_prev_page_id(Some(right_page_id)); } @@ -463,14 +463,14 @@ impl TreeCursor { *root_guard.page() }; - let (left_page_id, left_page_pin) = self.page_cache.new_page()?; + let (left_page_id, left_page_pin) = self.page_cache.new_page(self.txn_id)?; { - let mut left_guard = left_page_pin.write()?; + let mut left_guard = left_page_pin.write(self.txn_id)?; *left_guard.page_mut() = root_snapshot; } self.relink_copied_root_left_child(left_page_id, pending.right_page_id)?; - let mut root_guard = root_pin.write()?; + let mut root_guard = root_pin.write(self.txn_id)?; let mut root_page = RawInterior::>::initialize_with_rightmost( root_guard.page_mut(), pending.right_page_id, @@ -486,7 +486,7 @@ impl TreeCursor { right_page_id: PageId, ) -> StorageResult<()> { let left_pin = self.page_cache.fetch_page(left_page_id)?; - let mut left_guard = left_pin.write()?; + let mut left_guard = left_pin.write(self.txn_id)?; match read_page_kind(left_guard.page(), left_page_id)? { PageKind::RawLeaf => { { diff --git a/src/storage/btree/tests.rs b/src/storage/btree/tests.rs index 2c0b0f7..0ee263b 100644 --- a/src/storage/btree/tests.rs +++ b/src/storage/btree/tests.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, rc::Rc}; +use std::{collections::BTreeMap, sync::Arc}; use fastrand::Rng; use tempfile::NamedTempFile; @@ -52,14 +52,14 @@ fn assert_supported_cell(key: &[u8], value: &[u8]) { fn temp_page_cache(cache_frames: usize) -> PageCache { let file = NamedTempFile::new().unwrap(); let disk_manager = DiskManager::new(file.path()).unwrap(); - let runtime = Rc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); + let runtime = Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); PageCache::new(runtime, cache_frames).unwrap() } fn temp_tree_cursor(cache_frames: usize) -> TreeCursor { let page_cache = temp_page_cache(cache_frames); - let root_page_id = initialize_empty_root(&page_cache).unwrap(); - TreeCursor::new(page_cache, root_page_id) + let root_page_id = initialize_empty_root(&page_cache, None).unwrap(); + TreeCursor::new(page_cache, root_page_id).unwrap() } fn tree_height(cursor: &TreeCursor) -> StorageResult { @@ -117,7 +117,7 @@ fn rejected_mutations_do_not_advance_tree_epoch() { #[test] fn successful_mutations_advance_shared_tree_epoch_once() { let mut cursor = temp_tree_cursor(32); - let observer = TreeCursor::new(cursor.page_cache.clone(), cursor.root_page_id()); + let observer = TreeCursor::new(cursor.page_cache.clone(), cursor.root_page_id()).unwrap(); let before_insert = observer.mutation_epoch(); cursor.insert(b"key", b"value").unwrap(); @@ -464,15 +464,15 @@ fn insert_get_supports_oversized_keys_promoted_to_interior_pages() { #[test] fn failed_interior_rewrite_leaves_page_unchanged() { let page_cache = temp_page_cache(16); - let (page_id, pin) = page_cache.new_page().unwrap(); + let (page_id, pin) = page_cache.new_page(None).unwrap(); { - let mut guard = pin.write().unwrap(); + let mut guard = pin.write(None).unwrap(); let mut interior = RawInterior::>::initialize_with_rightmost(guard.page_mut(), 2); interior.insert_payload_at(0, 0, b"stable".len(), None, b"stable").unwrap(); } drop(pin); - let cursor = TreeCursor::new(page_cache.clone(), page_id); + let cursor = TreeCursor::new(page_cache.clone(), page_id).unwrap(); let original_page = { let pin = page_cache.fetch_page(page_id).unwrap(); let page = pin.read().unwrap(); @@ -503,10 +503,10 @@ fn failed_interior_rewrite_leaves_page_unchanged() { fn unchanged_path_separator_refresh_does_not_grow_file() { let file = NamedTempFile::new().unwrap(); let disk_manager = DiskManager::new(file.path()).unwrap(); - let runtime = Rc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); + let runtime = Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk_manager).unwrap()); let page_cache = PageCache::new(runtime, 256).unwrap(); - let root_page_id = initialize_empty_root(&page_cache).unwrap(); - let mut cursor = TreeCursor::new(page_cache, root_page_id); + let root_page_id = initialize_empty_root(&page_cache, None).unwrap(); + let mut cursor = TreeCursor::new(page_cache, root_page_id).unwrap(); let mut expected = BTreeMap::new(); for index in 0..96 { diff --git a/src/storage/engine.rs b/src/storage/engine.rs index 3987077..404975d 100644 --- a/src/storage/engine.rs +++ b/src/storage/engine.rs @@ -1,8 +1,5 @@ -use std::{path::Path, rc::Rc}; +use std::path::Path; -use crate::core::{PageId, error::StorageResult}; -#[cfg(test)] -use crate::storage::disk_manager::DiskManagerError; use crate::storage::{ btree::{TreeCursor, initialize_empty_root, validate_tree_page_formats}, database_header::{DATABASE_HEADER_PAGE_ID, DatabaseHeader, missing_header}, @@ -12,6 +9,10 @@ use crate::storage::{ storage_runtime::StorageRuntime, transaction_manager::TransactionSavepoint, }; +use crate::{ + core::{PageId, error::StorageResult}, + sync::Arc, +}; const DEFAULT_PAGE_CACHE_SIZE: usize = 16384; @@ -34,7 +35,7 @@ impl Default for StorageOptions { /// only for producing raw B+-tree cursors rooted at specific page ids. #[derive(Clone)] pub(crate) struct Storage { - runtime: Rc, + runtime: Arc, page_cache: PageCache, opened_page_count: u64, } @@ -101,8 +102,8 @@ impl Storage { options: StorageOptions, ) -> StorageResult { let opened_page_count = disk_manager.page_count(); - let runtime = Rc::new(StorageRuntime::new(path, disk_manager)?); - let page_cache = PageCache::new(Rc::clone(&runtime), options.cache_frames)?; + let runtime = Arc::new(StorageRuntime::new(path, disk_manager)?); + let page_cache = PageCache::new(Arc::clone(&runtime), options.cache_frames)?; Ok(Self { runtime, page_cache, opened_page_count }) } @@ -124,7 +125,7 @@ impl Storage { } #[cfg(test)] - pub(crate) fn unlock_for_crash_for_test(&self) -> Result<(), DiskManagerError> { + pub(crate) fn unlock_for_crash_for_test(&self) -> StorageResult<()> { self.runtime.unlock_for_crash_for_test() } @@ -136,8 +137,8 @@ impl Storage { self.runtime.commit_transaction(txn_id) } - pub(crate) fn active_transaction_id(&self) -> Option { - self.runtime.active_transaction_id() + pub(crate) fn transaction_is_active(&self, txn_id: TxnId) -> StorageResult { + self.runtime.transaction_is_active(txn_id) } pub(crate) fn transaction_is_poisoned(&self, txn_id: TxnId) -> StorageResult { @@ -154,7 +155,7 @@ impl Storage { ) -> StorageResult<()> { let undo_pages = self.runtime.rollback_to_savepoint(savepoint)?; if let Err(err) = self.page_cache.restore_rollback_pages(undo_pages) { - self.runtime.record_transaction_failure(); + self.runtime.record_transaction_failure(savepoint.txn_id)?; return Err(err.into()); } self.runtime.complete_savepoint_rollback(savepoint) @@ -162,39 +163,58 @@ impl Storage { pub(crate) fn rollback_transaction(&self, txn_id: TxnId) -> StorageResult<()> { let rollback = self.runtime.prepare_rollback_pages(txn_id)?; + let mut restored_page_ids = + rollback.pages.iter().map(|restore| restore.page_id).collect::>(); + restored_page_ids.sort_unstable(); + restored_page_ids.dedup(); self.page_cache.restore_rollback_pages(rollback.pages)?; - self.page_cache.flush_all()?; + for page_id in restored_page_ids { + self.page_cache.flush_page(page_id)?; + } self.runtime.sync_database_file()?; self.runtime.finish_rollback(txn_id)?; Ok(()) } #[cfg(test)] - pub(crate) fn force_next_lsn_exhausted_for_test(&self) { - self.runtime.force_next_lsn_exhausted_for_test(); + pub(crate) fn force_next_lsn_exhausted_for_test(&self) -> StorageResult<()> { + self.runtime.force_next_lsn_exhausted_for_test() } #[cfg(test)] - pub(crate) fn fail_next_savepoint_rollback_for_test(&self) { - self.runtime.fail_next_savepoint_rollback_for_test(); + pub(crate) fn fail_next_savepoint_rollback_for_test(&self) -> StorageResult<()> { + self.runtime.fail_next_savepoint_rollback_for_test() } #[cfg(test)] - pub(crate) fn fail_next_wal_flush_for_test(&self) { - self.runtime.fail_next_wal_flush_for_test(); + pub(crate) fn fail_next_wal_flush_for_test(&self) -> StorageResult<()> { + self.runtime.fail_next_wal_flush_for_test() } /// Creates a new empty raw tree and returns a cursor rooted at it. pub(crate) fn create_tree(&self) -> StorageResult { - let root_page_id = initialize_empty_root(&self.page_cache)?; - Ok(TreeCursor::new(self.page_cache.clone(), root_page_id)) + let root_page_id = initialize_empty_root(&self.page_cache, None)?; + TreeCursor::new(self.page_cache.clone(), root_page_id) + } + + pub(crate) fn transaction_create_tree(&self, txn_id: TxnId) -> StorageResult { + let root_page_id = initialize_empty_root(&self.page_cache, Some(txn_id))?; + Ok(TreeCursor::new(self.page_cache.clone(), root_page_id)?.for_transaction(txn_id)) } /// Returns a raw cursor rooted at an existing tree. - pub(crate) fn tree_cursor(&self, root_page_id: PageId) -> TreeCursor { + pub(crate) fn tree_cursor(&self, root_page_id: PageId) -> StorageResult { TreeCursor::new(self.page_cache.clone(), root_page_id) } + pub(crate) fn transaction_tree_cursor( + &self, + txn_id: TxnId, + root_page_id: PageId, + ) -> StorageResult { + Ok(self.tree_cursor(root_page_id)?.for_transaction(txn_id)) + } + /// Validates every B+-tree page reachable from `root_page_id`. pub(crate) fn validate_tree_page_formats(&self, root_page_id: PageId) -> StorageResult<()> { validate_tree_page_formats(&self.page_cache, root_page_id) @@ -238,7 +258,26 @@ mod tests { let storage = Storage::open(file.path()).unwrap(); assert_eq!(storage.opened_page_count(), 3); - assert_eq!(storage.tree_cursor(1).root_page_id(), 1); - assert_eq!(storage.tree_cursor(2).root_page_id(), 2); + assert_eq!(storage.tree_cursor(1).unwrap().root_page_id(), 1); + assert_eq!(storage.tree_cursor(2).unwrap().root_page_id(), 2); + } + + #[test] + fn rollback_does_not_flush_pages_pinned_by_other_work() { + let file = NamedTempFile::new().unwrap(); + let storage = Storage::open_or_create(file.path()).unwrap(); + let first_root = storage.create_tree().unwrap().root_page_id(); + let second_root = storage.create_tree().unwrap().root_page_id(); + storage.flush().unwrap(); + + let txn_id = storage.begin_transaction().unwrap(); + storage + .transaction_tree_cursor(txn_id, first_root) + .unwrap() + .insert(b"key", b"value") + .unwrap(); + let _unrelated_pin = storage.page_cache.fetch_page(second_root).unwrap(); + + storage.rollback_transaction(txn_id).unwrap(); } } diff --git a/src/storage/error.rs b/src/storage/error.rs index b195ed2..1f64fbe 100644 --- a/src/storage/error.rs +++ b/src/storage/error.rs @@ -33,8 +33,7 @@ impl From for StorageError { impl From for StorageError { fn from(error: PageCacheError) -> Self { match error { - PageCacheError::Disk(error) => error.into(), - PageCacheError::Transaction(error) => *error, + PageCacheError::Storage(error) => *error, PageCacheError::NoEvictableFrame => { Self::LimitExceeded(LimitExceededError::CacheCapacityExhausted) } @@ -61,6 +60,9 @@ impl From for StorageError { PageCacheError::PinCountOverflow { page_id } => { invariant(InvariantViolation::PagePinCountOverflow { page_id }) } + PageCacheError::Poisoned { lock } => { + Self::Internal(InternalError::SynchronizationPoisoned { lock }) + } } } } diff --git a/src/storage/overflow.rs b/src/storage/overflow.rs index ccef703..2159a2d 100644 --- a/src/storage/overflow.rs +++ b/src/storage/overflow.rs @@ -1,6 +1,6 @@ //! Overflow-page chain helpers for large B+-tree cell payloads. -use crate::core::{PAGE_SIZE, PageId, error::StorageResult}; +use crate::core::{PAGE_SIZE, PageId, TxnId, error::StorageResult}; use crate::storage::{ page::format::{self, OVERFLOW_NEXT_PAGE_ID_SIZE}, page_cache::PageCache, @@ -13,7 +13,11 @@ fn write_next_page_id(page: &mut [u8; PAGE_SIZE], next_page_id: Option) } /// Writes `payload` into a newly allocated overflow chain. -pub(crate) fn write_chain(page_cache: &PageCache, payload: &[u8]) -> StorageResult> { +pub(crate) fn write_chain( + page_cache: &PageCache, + txn_id: Option, + payload: &[u8], +) -> StorageResult> { if payload.is_empty() { return Ok(None); } @@ -22,9 +26,9 @@ pub(crate) fn write_chain(page_cache: &PageCache, payload: &[u8]) -> StorageResu let mut previous_page_id = None; for chunk in payload.chunks(OVERFLOW_PAYLOAD_SIZE) { - let (page_id, pin) = page_cache.new_page()?; + let (page_id, pin) = page_cache.new_page(txn_id)?; { - let mut page = pin.write()?; + let mut page = pin.write(txn_id)?; page.page_mut().fill(0); write_next_page_id(page.page_mut(), None); page.page_mut()[OVERFLOW_NEXT_PAGE_ID_SIZE..OVERFLOW_NEXT_PAGE_ID_SIZE + chunk.len()] @@ -38,7 +42,7 @@ pub(crate) fn write_chain(page_cache: &PageCache, payload: &[u8]) -> StorageResu if let Some(previous_page_id) = previous_page_id { let previous_pin = page_cache.fetch_page(previous_page_id)?; - let mut previous_page = previous_pin.write()?; + let mut previous_page = previous_pin.write(txn_id)?; write_next_page_id(previous_page.page_mut(), Some(page_id)); } diff --git a/src/storage/page/core.rs b/src/storage/page/core.rs index 618e180..67df1c9 100644 --- a/src/storage/page/core.rs +++ b/src/storage/page/core.rs @@ -201,7 +201,7 @@ where } /// Returns the statically known encoded kind of this page. - #[cfg(test)] + #[cfg(all(test, not(loom)))] pub(crate) fn kind(&self) -> format::PageKind { page_kind::() } diff --git a/src/storage/page_cache.rs b/src/storage/page_cache.rs index 73ba880..ab17da6 100644 --- a/src/storage/page_cache.rs +++ b/src/storage/page_cache.rs @@ -1,7 +1,7 @@ -//! Single-threaded page cache with explicit pin and page-access guards. +//! Shared page cache with explicit pin and page-access guards. //! //! [`PageCache`] is a cheap-to-clone handle that shares cache state through -//! [`Rc`]. The cache is intentionally single-threaded today and uses interior +//! [`Arc`]. The cache uses synchronized interior //! mutability to allow multiple concurrent pins without requiring a mutable //! borrow of the cache handle itself. //! @@ -17,17 +17,20 @@ //! explicit flushes or eviction. use std::{ - cell::{Cell, Ref, RefCell, RefMut}, collections::{HashMap, TryReserveError}, - rc::Rc, + sync::TryLockError, +}; + +use crate::sync::{ + Arc, AtomicBool, AtomicU32, AtomicU64, Mutex, MutexGuard, Ordering, RwLock, RwLockReadGuard, + RwLockWriteGuard, }; use thiserror::Error; -use crate::core::{PAGE_SIZE, PageId, error::StorageError}; +use crate::core::{PAGE_SIZE, PageId, TxnId, error::StorageError}; use crate::storage::{ - disk_manager::DiskManagerError, - log_manager::{Lsn, ZERO_LSN}, + log_manager::ZERO_LSN, page::{NodeMarker, Page, PageResult, Read, Write}, page_replacement::ClockPolicy, storage_runtime::StorageRuntime, @@ -36,10 +39,8 @@ use crate::storage::{ #[derive(Debug, Error)] pub(crate) enum PageCacheError { - #[error("disk manager error: {0}")] - Disk(#[from] DiskManagerError), - #[error("transaction error: {0}")] - Transaction(Box), + #[error("storage runtime error: {0}")] + Storage(Box), #[error("no evictable frame available")] NoEvictableFrame, #[error("page {page_id} is pinned")] @@ -58,30 +59,75 @@ pub(crate) enum PageCacheError { CorruptPageTableEntry { page_id: PageId, frame_id: usize, frame_count: usize }, #[error("page {page_id} pin count overflowed")] PinCountOverflow { page_id: PageId }, + #[error("synchronization lock poisoned: {lock}")] + Poisoned { lock: &'static str }, } pub(crate) type PageCacheResult = Result; +fn runtime_error(error: StorageError) -> PageCacheError { + PageCacheError::Storage(Box::new(error)) +} + +fn try_read_page_data( + data: &RwLock<[u8; PAGE_SIZE]>, + page_id: PageId, +) -> PageCacheResult> { + match data.try_read() { + Ok(page) => Ok(page), + Err(TryLockError::WouldBlock) => { + Err(PageCacheError::PageImmutableBorrowConflict { page_id }) + } + Err(TryLockError::Poisoned(_poisoned)) => { + Err(PageCacheError::Poisoned { lock: "page data" }) + } + } +} + +fn try_write_page_data( + data: &RwLock<[u8; PAGE_SIZE]>, + page_id: PageId, +) -> PageCacheResult> { + match data.try_write() { + Ok(page) => Ok(page), + Err(TryLockError::WouldBlock) => Err(PageCacheError::PageMutableBorrowConflict { page_id }), + Err(TryLockError::Poisoned(_poisoned)) => { + Err(PageCacheError::Poisoned { lock: "page data" }) + } + } +} + pub(crate) type FrameId = usize; #[derive(Debug)] struct Frame { - page_id: Cell>, - data: RefCell<[u8; PAGE_SIZE]>, - dirty: Cell, - lsn: Cell, - pin_count: Cell, + page_id: AtomicU64, + data: RwLock<[u8; PAGE_SIZE]>, + dirty: AtomicBool, + lsn: AtomicU64, + pin_count: AtomicU32, } impl Frame { + fn page_id(&self) -> Option { + match self.page_id.load(Ordering::Acquire) { + u64::MAX => None, + page_id => Some(page_id), + } + } + + fn set_page_id(&self, page_id: Option) { + self.page_id.store(page_id.unwrap_or(u64::MAX), Ordering::Release); + } + /// Creates an empty frame with zeroed page data and cleared metadata bits. fn empty() -> Self { Self { - page_id: Cell::new(None), - data: RefCell::new([0u8; PAGE_SIZE]), - dirty: Cell::new(false), - lsn: Cell::new(ZERO_LSN), - pin_count: Cell::new(0), + page_id: AtomicU64::new(u64::MAX), + data: RwLock::new([0u8; PAGE_SIZE]), + dirty: AtomicBool::new(false), + lsn: AtomicU64::new(ZERO_LSN), + pin_count: AtomicU32::new(0), } } } @@ -89,29 +135,37 @@ impl Frame { struct CacheMeta { page_table: HashMap, replacement: ClockPolicy, - tree_mutation_epochs: HashMap>>, + tree_mutation_epochs: HashMap>, } struct PageCacheInner { - runtime: Rc, - meta: RefCell, + runtime: Arc, + meta: Mutex, frames: Vec, } -/// Shared handle to the single-threaded page cache. +impl PageCacheInner { + fn lock_meta(&self) -> PageCacheResult> { + self.meta + .lock() + .map_err(|_poisoned| PageCacheError::Poisoned { lock: "page cache metadata" }) + } +} + +/// Thread-safe shared handle to the page cache. /// -/// Cloning the handle shares the same cache state through [`Rc`]. The handle +/// Cloning the handle shares the same cache state through [`Arc`]. The handle /// itself does not represent a pin or a page borrow; it only provides access to /// cache operations. Use [`PinGuard`] to keep pages resident and use /// [`PageReadGuard`] or [`PageWriteGuard`] for temporary access to the page /// bytes. pub(crate) struct PageCache { - inner: Rc, + inner: Arc, } impl Clone for PageCache { fn clone(&self) -> Self { - Self { inner: Rc::clone(&self.inner) } + Self { inner: Arc::clone(&self.inner) } } } @@ -119,7 +173,7 @@ impl PageCache { /// Creates a new page cache with a fixed number of preallocated frames. /// /// Returns an error when `frame_count` is zero. - pub(crate) fn new(runtime: Rc, frame_count: usize) -> PageCacheResult { + pub(crate) fn new(runtime: Arc, frame_count: usize) -> PageCacheResult { if frame_count == 0 { return Err(PageCacheError::InvalidFrameCount { frame_count }); } @@ -131,9 +185,9 @@ impl PageCache { frames.extend((0..frame_count).map(|_| Frame::empty())); Ok(Self { - inner: Rc::new(PageCacheInner { + inner: Arc::new(PageCacheInner { runtime, - meta: RefCell::new(CacheMeta { + meta: Mutex::new(CacheMeta { page_table: HashMap::new(), replacement: ClockPolicy::new(frame_count), tree_mutation_epochs: HashMap::new(), @@ -144,11 +198,16 @@ impl PageCache { } /// Returns the mutation epoch shared by cursors over one B+-tree root. - pub(crate) fn tree_mutation_epoch(&self, root_page_id: PageId) -> Rc> { - let mut meta = self.inner.meta.borrow_mut(); - Rc::clone( - meta.tree_mutation_epochs.entry(root_page_id).or_insert_with(|| Rc::new(Cell::new(0))), - ) + pub(crate) fn tree_mutation_epoch( + &self, + root_page_id: PageId, + ) -> PageCacheResult> { + let mut meta = self.inner.lock_meta()?; + Ok(Arc::clone( + meta.tree_mutation_epochs + .entry(root_page_id) + .or_insert_with(|| Arc::new(AtomicU64::new(0))), + )) } /// Fetches an existing page into the cache and returns a pin guard. @@ -156,48 +215,52 @@ impl PageCache { /// Cache hits update replacement state and increment pin count. /// Cache misses use CLOCK replacement and may evict a dirty page. pub(crate) fn fetch_page(&self, page_id: PageId) -> PageCacheResult { - if let Some(frame_id) = self.resident_frame_id(page_id)? { + let mut meta = self.inner.lock_meta()?; + if let Some(frame_id) = self.resident_frame_id(&meta, page_id)? { let frame = &self.inner.frames[frame_id]; let pin_count = frame .pin_count - .get() + .load(Ordering::Acquire) .checked_add(1) .ok_or(PageCacheError::PinCountOverflow { page_id })?; - frame.pin_count.set(pin_count); - self.inner.meta.borrow_mut().replacement.record_access(frame_id); - return Ok(PinGuard::new(Rc::clone(&self.inner), frame_id, page_id)); + frame.pin_count.store(pin_count, Ordering::Release); + meta.replacement.record_access(frame_id); + return Ok(PinGuard::new(Arc::clone(&self.inner), frame_id, page_id)); } - let frame_id = self.select_victim_frame().ok_or(PageCacheError::NoEvictableFrame)?; - self.replace_frame(frame_id, page_id)?; - Ok(PinGuard::new(Rc::clone(&self.inner), frame_id, page_id)) + let frame_id = + self.select_victim_frame(&mut meta).ok_or(PageCacheError::NoEvictableFrame)?; + self.replace_frame(&mut meta, frame_id, page_id)?; + Ok(PinGuard::new(Arc::clone(&self.inner), frame_id, page_id)) } /// Allocates a new on-disk page and returns it pinned in the cache. /// /// A victim frame is selected before allocation so a full pinned cache /// returns `NoEvictableFrame` without growing the file. - pub(crate) fn new_page(&self) -> PageCacheResult<(PageId, PinGuard)> { - let frame_id = self.select_victim_frame().ok_or(PageCacheError::NoEvictableFrame)?; - let page_id = self.inner.runtime.new_page()?; - if let Err(err) = self.inner.runtime.record_page_alloc(page_id) { - return Err(PageCacheError::Transaction(Box::new(err))); + pub(crate) fn new_page(&self, txn_id: Option) -> PageCacheResult<(PageId, PinGuard)> { + let mut meta = self.inner.lock_meta()?; + let frame_id = + self.select_victim_frame(&mut meta).ok_or(PageCacheError::NoEvictableFrame)?; + let page_id = self.inner.runtime.new_page().map_err(runtime_error)?; + if let Err(err) = self.inner.runtime.record_page_alloc(txn_id, page_id) { + return Err(PageCacheError::Storage(Box::new(err))); } - self.replace_frame(frame_id, page_id)?; - Ok((page_id, PinGuard::new(Rc::clone(&self.inner), frame_id, page_id))) + self.replace_frame(&mut meta, frame_id, page_id)?; + Ok((page_id, PinGuard::new(Arc::clone(&self.inner), frame_id, page_id))) } /// Flushes one resident page if dirty. /// /// Non-resident pages are a no-op. Pinned pages return `PinnedPage`. - #[cfg(test)] pub(crate) fn flush_page(&self, page_id: PageId) -> PageCacheResult<()> { - let Some(frame_id) = self.resident_frame_id(page_id)? else { + let meta = self.inner.lock_meta()?; + let Some(frame_id) = self.resident_frame_id(&meta, page_id)? else { return Ok(()); }; let frame = &self.inner.frames[frame_id]; - if frame.pin_count.get() > 0 { + if frame.pin_count.load(Ordering::Acquire) > 0 { return Err(PageCacheError::PinnedPage { page_id }); } @@ -208,10 +271,11 @@ impl PageCache { /// /// Returns `PinnedPage` if a dirty page is pinned. pub(crate) fn flush_all(&self) -> PageCacheResult<()> { + let _meta = self.inner.lock_meta()?; for (frame_id, frame) in self.inner.frames.iter().enumerate() { - let page_id = frame.page_id.get(); - let pin_count = frame.pin_count.get(); - let dirty = frame.dirty.get(); + let page_id = frame.page_id(); + let pin_count = frame.pin_count.load(Ordering::Acquire); + let dirty = frame.dirty.load(Ordering::Acquire); if !dirty { continue; @@ -231,8 +295,11 @@ impl PageCache { Ok(()) } - fn resident_frame_id(&self, page_id: PageId) -> PageCacheResult> { - let meta = self.inner.meta.borrow(); + fn resident_frame_id( + &self, + meta: &CacheMeta, + page_id: PageId, + ) -> PageCacheResult> { let Some(&frame_id) = meta.page_table.get(&page_id) else { return Ok(None); }; @@ -251,40 +318,38 @@ impl PageCache { Ok(()) } - fn select_victim_frame(&self) -> Option { + fn select_victim_frame(&self, meta: &mut CacheMeta) -> Option { let frames = &self.inner.frames; - self.inner - .meta - .borrow_mut() - .replacement - .select_victim(|frame_id| frames[frame_id].pin_count.get() > 0) + meta.replacement + .select_victim(|frame_id| frames[frame_id].pin_count.load(Ordering::Acquire) > 0) } /// Replaces frame contents with `new_page_id`, flushing old dirty data first. - fn replace_frame(&self, frame_id: FrameId, new_page_id: PageId) -> PageCacheResult<()> { + fn replace_frame( + &self, + meta: &mut CacheMeta, + frame_id: FrameId, + new_page_id: PageId, + ) -> PageCacheResult<()> { self.flush_frame_if_dirty(frame_id)?; let frame = &self.inner.frames[frame_id]; - let old_page_id = frame.page_id.get(); + let old_page_id = frame.page_id(); let mut data = [0u8; PAGE_SIZE]; - self.inner.runtime.read_page(new_page_id, &mut data)?; + self.inner.runtime.read_page(new_page_id, &mut data).map_err(runtime_error)?; { - let mut frame_data = frame.data.try_borrow_mut().map_err(|_borrow_conflict| { - PageCacheError::PageMutableBorrowConflict { - page_id: old_page_id.unwrap_or(new_page_id), - } - })?; + let mut frame_data = + try_write_page_data(&frame.data, old_page_id.unwrap_or(new_page_id))?; *frame_data = data; } - frame.page_id.set(Some(new_page_id)); - frame.dirty.set(false); - frame.lsn.set(ZERO_LSN); - frame.pin_count.set(1); + frame.set_page_id(Some(new_page_id)); + frame.dirty.store(false, Ordering::Release); + frame.lsn.store(ZERO_LSN, Ordering::Release); + frame.pin_count.store(1, Ordering::Release); - let mut meta = self.inner.meta.borrow_mut(); if let Some(old_page_id) = old_page_id { meta.page_table.remove(&old_page_id); } @@ -296,24 +361,21 @@ impl PageCache { /// Writes a dirty resident frame to disk and clears its dirty bit. fn flush_frame_if_dirty(&self, frame_id: FrameId) -> PageCacheResult<()> { let frame = &self.inner.frames[frame_id]; - if !frame.dirty.get() { + if !frame.dirty.load(Ordering::Acquire) { return Ok(()); } - let Some(page_id) = frame.page_id.get() else { + let Some(page_id) = frame.page_id() else { return Ok(()); }; - let page = frame - .data - .try_borrow() - .map_err(|_borrow_conflict| PageCacheError::PageImmutableBorrowConflict { page_id })?; + let page = try_read_page_data(&frame.data, page_id)?; self.inner .runtime - .flush_wal_through(frame.lsn.get()) - .map_err(|err| PageCacheError::Transaction(Box::new(err)))?; - self.inner.runtime.write_page(page_id, &page)?; - frame.dirty.set(false); + .flush_wal_through(frame.lsn.load(Ordering::Acquire)) + .map_err(|err| PageCacheError::Storage(Box::new(err)))?; + self.inner.runtime.write_page(page_id, &page).map_err(runtime_error)?; + frame.dirty.store(false, Ordering::Release); Ok(()) } @@ -325,13 +387,11 @@ impl PageCache { let pin = self.fetch_page(restore.page_id)?; let frame = &self.inner.frames[pin.frame_id]; { - let mut data = frame.data.try_borrow_mut().map_err(|_borrow_conflict| { - PageCacheError::PageMutableBorrowConflict { page_id: restore.page_id } - })?; + let mut data = try_write_page_data(&frame.data, restore.page_id)?; *data = restore.image; } - frame.dirty.set(true); - frame.lsn.set(restore.wal_flush_lsn); + frame.dirty.store(true, Ordering::Release); + frame.lsn.store(restore.wal_flush_lsn, Ordering::Release); } Ok(()) } @@ -346,19 +406,19 @@ impl PageCache { /// /// Dropping the guard decrements the frame pin count. pub(crate) struct PinGuard { - page_cache: Rc, + page_cache: Arc, frame_id: FrameId, page_id: PageId, } impl PinGuard { /// Creates a new pin guard for a specific frame. - fn new(page_cache: Rc, frame_id: FrameId, page_id: PageId) -> Self { + fn new(page_cache: Arc, frame_id: FrameId, page_id: PageId) -> Self { Self { page_cache, frame_id, page_id } } /// Returns the page ID associated with this pin. - #[cfg(test)] + #[cfg(all(test, not(loom)))] pub(crate) fn page_id(&self) -> PageId { self.page_id } @@ -369,9 +429,7 @@ impl PinGuard { /// fails while a write guard is active. pub(crate) fn read(&self) -> PageCacheResult> { let frame = &self.page_cache.frames[self.frame_id]; - let page = frame.data.try_borrow().map_err(|_borrow_conflict| { - PageCacheError::PageImmutableBorrowConflict { page_id: self.page_id } - })?; + let page = try_read_page_data(&frame.data, self.page_id)?; Ok(PageReadGuard { page }) } @@ -380,21 +438,20 @@ impl PinGuard { /// Mutable access fails while any read or write guard is active for the /// same frame. Acquiring a write guard marks the frame dirty even if the /// caller later decides not to mutate the page bytes. - pub(crate) fn write(&self) -> PageCacheResult> { + pub(crate) fn write(&self, txn_id: Option) -> PageCacheResult> { let frame = &self.page_cache.frames[self.frame_id]; - let page = frame.data.try_borrow_mut().map_err(|_borrow_conflict| { - PageCacheError::PageMutableBorrowConflict { page_id: self.page_id } - })?; + let page = try_write_page_data(&frame.data, self.page_id)?; let before = *page; - let was_dirty = frame.dirty.get(); - frame.dirty.set(true); + let was_dirty = frame.dirty.load(Ordering::Acquire); + frame.dirty.store(true, Ordering::Release); Ok(PageWriteGuard { page, before, was_dirty, - runtime: Rc::clone(&self.page_cache.runtime), + runtime: Arc::clone(&self.page_cache.runtime), frame, page_id: self.page_id, + txn_id, }) } } @@ -402,10 +459,16 @@ impl PinGuard { impl Drop for PinGuard { /// Decrements the frame pin count when the guard leaves scope. fn drop(&mut self) { + // A poisoned cache is permanently fail-closed. Retaining this pin is + // safer than mutating cache state whose invariants may no longer hold. + let Ok(_meta) = self.page_cache.lock_meta() else { + return; + }; let frame = &self.page_cache.frames[self.frame_id]; - debug_assert!(frame.pin_count.get() > 0, "pin count underflow"); - if frame.pin_count.get() > 0 { - frame.pin_count.set(frame.pin_count.get() - 1); + let pin_count = frame.pin_count.load(Ordering::Acquire); + debug_assert!(pin_count > 0, "pin count underflow"); + if pin_count > 0 { + frame.pin_count.store(pin_count - 1, Ordering::Release); } } } @@ -417,7 +480,7 @@ impl Drop for PinGuard { /// for the page to stay resident. Use this guard for raw byte inspection or to /// construct typed read-only page views. pub(crate) struct PageReadGuard<'a> { - page: Ref<'a, [u8; PAGE_SIZE]>, + page: RwLockReadGuard<'a, [u8; PAGE_SIZE]>, } impl PageReadGuard<'_> { @@ -441,12 +504,13 @@ impl PageReadGuard<'_> { /// write guard may exist for a frame at a time, and no read guards may coexist /// with it. Creating a write guard marks the frame dirty immediately. pub(crate) struct PageWriteGuard<'a> { - page: RefMut<'a, [u8; PAGE_SIZE]>, + page: RwLockWriteGuard<'a, [u8; PAGE_SIZE]>, before: [u8; PAGE_SIZE], was_dirty: bool, - runtime: Rc, + runtime: Arc, frame: &'a Frame, page_id: PageId, + txn_id: Option, } impl PageWriteGuard<'_> { @@ -472,36 +536,39 @@ impl PageWriteGuard<'_> { impl Drop for PageWriteGuard<'_> { fn drop(&mut self) { if *self.page == self.before { - self.frame.dirty.set(self.was_dirty); + self.frame.dirty.store(self.was_dirty, Ordering::Release); return; } - match self.runtime.record_page_update(self.page_id, &self.before, &self.page) { + match self.runtime.record_page_update(self.txn_id, self.page_id, &self.before, &self.page) { Ok(Some(update)) => { *self.page = update.redo; - self.frame.lsn.set(update.lsn); + self.frame.lsn.store(update.lsn, Ordering::Release); } Ok(None) => { - self.frame.lsn.set(ZERO_LSN); + self.frame.lsn.store(ZERO_LSN, Ordering::Release); } Err(_) => { *self.page = self.before; - self.frame.dirty.set(self.was_dirty); - self.runtime.record_transaction_failure(); + self.frame.dirty.store(self.was_dirty, Ordering::Release); + if let Some(txn_id) = self.txn_id { + let _ = self.runtime.record_transaction_failure(txn_id); + } } } } } -#[cfg(test)] +#[cfg(all(test, not(loom)))] +#[allow(clippy::unwrap_used)] mod tests { - use std::{path::Path, rc::Rc}; + use std::{path::Path, sync::Arc, thread}; use tempfile::NamedTempFile; use super::*; - use crate::storage::disk_manager::DiskManager; - use crate::storage::log_manager::{OwnedLogRecordKind, read_log_record_kinds_for_test}; + use crate::storage::disk_manager::{DiskManager, DiskManagerError}; + use crate::storage::log_manager::{Lsn, OwnedLogRecordKind, read_log_record_kinds_for_test}; use crate::storage::page; use crate::storage::page::format::PageKind; use crate::storage::page::{Leaf, Page, Write}; @@ -526,17 +593,17 @@ mod tests { } /// Creates a temporary database file and writes the provided pages to it. - fn runtime_for_disk(path: &Path, disk_manager: DiskManager) -> Rc { - Rc::new(StorageRuntime::new(path.to_path_buf(), disk_manager).unwrap()) + fn runtime_for_disk(path: &Path, disk_manager: DiskManager) -> Arc { + Arc::new(StorageRuntime::new(path.to_path_buf(), disk_manager).unwrap()) } - fn runtime_for_path(path: &Path) -> Rc { + fn runtime_for_path(path: &Path) -> Arc { let disk_manager = DiskManager::new(path).unwrap(); runtime_for_disk(path, disk_manager) } /// Creates a temporary database file and writes the provided pages to it. - fn create_disk_with_pages(pages: &[[u8; PAGE_SIZE]]) -> (NamedTempFile, Rc) { + fn create_disk_with_pages(pages: &[[u8; PAGE_SIZE]]) -> (NamedTempFile, Arc) { let file = NamedTempFile::new().unwrap(); let mut disk_manager = DiskManager::new(file.path()).unwrap(); for page in pages { @@ -571,10 +638,10 @@ mod tests { assert_eq!(cache.inner.frames.len(), 3); for frame in &cache.inner.frames { - assert_eq!(frame.page_id.get(), None); - assert!(!frame.dirty.get()); - assert_eq!(frame.pin_count.get(), 0); - assert_eq!(*frame.data.borrow(), [0u8; PAGE_SIZE]); + assert_eq!(frame.page_id(), None); + assert!(!frame.dirty.load(Ordering::Acquire)); + assert_eq!(frame.pin_count.load(Ordering::Acquire), 0); + assert_eq!(*frame.data.read().unwrap(), [0u8; PAGE_SIZE]); } } @@ -589,8 +656,8 @@ mod tests { assert_eq!(guard.read().unwrap().page(), &page); drop(guard); - assert_eq!(cache.inner.frames[0].page_id.get(), Some(0)); - assert_eq!(cache.inner.frames[0].pin_count.get(), 0); + assert_eq!(cache.inner.frames[0].page_id(), Some(0)); + assert_eq!(cache.inner.frames[0].pin_count.load(Ordering::Acquire), 0); } #[test] @@ -604,7 +671,7 @@ mod tests { let _guard = cache.fetch_page(0).unwrap(); } - assert_eq!(cache.inner.frames[0].pin_count.get(), 0); + assert_eq!(cache.inner.frames[0].pin_count.load(Ordering::Acquire), 0); } #[test] @@ -618,8 +685,8 @@ mod tests { assert_eq!(left.page_id(), 0); assert_eq!(right.page_id(), 1); - assert_eq!(cache.inner.frames[0].pin_count.get(), 1); - assert_eq!(cache.inner.frames[1].pin_count.get(), 1); + assert_eq!(cache.inner.frames[0].pin_count.load(Ordering::Acquire), 1); + assert_eq!(cache.inner.frames[1].pin_count.load(Ordering::Acquire), 1); } #[test] @@ -666,8 +733,8 @@ mod tests { let guard0 = cache.fetch_page(0).unwrap(); let guard1 = cache.fetch_page(1).unwrap(); - let mut write0 = guard0.write().unwrap(); - let mut write1 = guard1.write().unwrap(); + let mut write0 = guard0.write(None).unwrap(); + let mut write1 = guard1.write(None).unwrap(); write0.page_mut()[0] = 42; write1.page_mut()[0] = 84; @@ -682,10 +749,10 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - let (_page_id, guard) = cache.new_page().unwrap(); + let (_page_id, guard) = cache.new_page(None).unwrap(); { - let mut write = guard.write().unwrap(); + let mut write = guard.write(None).unwrap(); let _ = Page::, Leaf>::init(write.page_mut()); assert_eq!( @@ -710,15 +777,15 @@ mod tests { let guard = cache.fetch_page(0).unwrap(); assert_eq!(guard.read().unwrap().page()[0], page[0]); } - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); { let guard = cache.fetch_page(0).unwrap(); - let mut page = guard.write().unwrap(); + let mut page = guard.write(None).unwrap(); page.page_mut()[0] = 99; } - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } #[test] @@ -730,19 +797,19 @@ mod tests { let guard = cache.fetch_page(0).unwrap(); { - let _write = guard.write().unwrap(); - assert!(cache.inner.frames[0].dirty.get()); + let _write = guard.write(None).unwrap(); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); - cache.inner.frames[0].dirty.set(true); + cache.inner.frames[0].dirty.store(true, Ordering::Release); { - let _write = guard.write().unwrap(); - assert!(cache.inner.frames[0].dirty.get()); + let _write = guard.write(None).unwrap(); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } #[test] @@ -753,7 +820,7 @@ mod tests { let cache = PageCache::new(disk_manager, 1).unwrap(); let guard = cache.fetch_page(0).unwrap(); - let _write = guard.write().unwrap(); + let _write = guard.write(None).unwrap(); let result = guard.read(); assert!(matches!(result, Err(PageCacheError::PageImmutableBorrowConflict { page_id: 0 }))); @@ -769,7 +836,7 @@ mod tests { let guard = cache.fetch_page(0).unwrap(); let _read = guard.read().unwrap(); - let result = guard.write(); + let result = guard.write(None); assert!(matches!(result, Err(PageCacheError::PageMutableBorrowConflict { page_id: 0 }))); } @@ -781,12 +848,71 @@ mod tests { let cache = PageCache::new(disk_manager, 1).unwrap(); let guard = cache.fetch_page(0).unwrap(); - let _first_write = guard.write().unwrap(); + let _first_write = guard.write(None).unwrap(); - let result = guard.write(); + let result = guard.write(None); assert!(matches!(result, Err(PageCacheError::PageMutableBorrowConflict { page_id: 0 }))); } + #[test] + #[allow(clippy::panic)] + fn poisoned_page_data_is_not_reported_as_a_borrow_conflict() { + let page = page_with_pattern(17); + let (_file, runtime) = create_disk_with_pages(&[page]); + let cache = PageCache::new(runtime, 1).unwrap(); + let guard = cache.fetch_page(0).unwrap(); + + let inner = Arc::clone(&cache.inner); + let panicked = thread::spawn(move || { + let _page = inner.frames[0].data.write().unwrap(); + panic!("poison page data"); + }) + .join(); + assert!(panicked.is_err()); + + assert!(matches!(guard.read(), Err(PageCacheError::Poisoned { lock: "page data" }))); + } + + #[test] + #[allow(clippy::panic)] + fn poisoned_metadata_prevents_further_cache_operations() { + let page = page_with_pattern(18); + let (_file, runtime) = create_disk_with_pages(&[page]); + let cache = PageCache::new(runtime, 1).unwrap(); + + let inner = Arc::clone(&cache.inner); + let panicked = thread::spawn(move || { + let _meta = inner.meta.lock().unwrap(); + panic!("poison page cache metadata"); + }) + .join(); + assert!(panicked.is_err()); + + assert!(matches!( + cache.fetch_page(0), + Err(PageCacheError::Poisoned { lock: "page cache metadata" }) + )); + } + + #[test] + fn rollback_restoration_installs_its_wal_dependency() { + let page = page_with_pattern(21); + let (_file, runtime) = create_disk_with_pages(&[page]); + let cache = PageCache::new(runtime, 1).unwrap(); + + cache + .restore_rollback_pages(vec![PageRestore { + page_id: 0, + image: page_with_pattern(22), + wal_flush_lsn: 9, + }]) + .unwrap(); + + let frame = &cache.inner.frames[0]; + assert_eq!(frame.lsn.load(Ordering::Acquire), 9); + assert!(frame.dirty.load(Ordering::Acquire)); + } + #[test] fn dirty_page_is_written_during_eviction() { let page0 = page_with_pattern(1); @@ -797,7 +923,7 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 222; + guard.write(None).unwrap().page_mut()[0] = 222; } { @@ -824,7 +950,7 @@ mod tests { let _guard = cache.fetch_page(2).unwrap(); } - let page_table = &cache.inner.meta.borrow().page_table; + let page_table = &cache.inner.lock_meta().unwrap().page_table; assert!(!page_table.contains_key(&0)); assert!(page_table.contains_key(&1)); assert!(page_table.contains_key(&2)); @@ -846,8 +972,8 @@ mod tests { } assert_eq!(pinned.page_id(), 0); - assert_eq!(cache.inner.frames[0].page_id.get(), Some(0)); - let page_table = &cache.inner.meta.borrow().page_table; + assert_eq!(cache.inner.frames[0].page_id(), Some(0)); + let page_table = &cache.inner.lock_meta().unwrap().page_table; assert!(page_table.contains_key(&0)); assert!(!page_table.contains_key(&1)); assert!(page_table.contains_key(&2)); @@ -875,13 +1001,13 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 177; + guard.write(None).unwrap().page_mut()[0] = 177; } - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); cache.flush_page(0).unwrap(); - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); let flushed_page = read_disk_page(file.path(), 0); assert_eq!(flushed_page[0], 177); } @@ -894,14 +1020,14 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(None).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } cache.flush_page(0).unwrap(); let flushed_page = read_disk_page(file.path(), 0); assert_eq!(flushed_page[PAGE_SIZE - 1], 177); - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); } #[test] @@ -913,7 +1039,7 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 222; + guard.write(None).unwrap().page_mut()[PAGE_SIZE - 1] = 222; } { @@ -933,11 +1059,11 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 10; + guard.write(None).unwrap().page_mut()[PAGE_SIZE - 1] = 10; } { let guard = cache.fetch_page(1).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 20; + guard.write(None).unwrap().page_mut()[PAGE_SIZE - 1] = 20; } cache.flush_all().unwrap(); @@ -947,28 +1073,28 @@ mod tests { assert_eq!(flushed_page0[PAGE_SIZE - 1], 10); assert_eq!(flushed_page1[PAGE_SIZE - 1], 20); for frame in &cache.inner.frames { - assert!(!frame.dirty.get()); + assert!(!frame.dirty.load(Ordering::Acquire)); } } #[test] - fn transactional_page_flush_appends_and_flushes_pending_wal_before_write() { + fn transactional_page_flush_makes_eager_wal_durable_before_write() { let page = formatted_page_with_lsn(15, ZERO_LSN); let (file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } cache.flush_page(0).unwrap(); let flushed_page = read_disk_page(file.path(), 0); assert_eq!(flushed_page[PAGE_SIZE - 1], 177); - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); assert_eq!( read_log_record_kinds_for_test(file.path()), [ @@ -979,32 +1105,33 @@ mod tests { } #[test] - fn transactional_page_flush_after_repeated_update_appends_one_latest_page_update() { + fn transactional_page_flush_preserves_each_eager_page_update() { let page = formatted_page_with_lsn(15, ZERO_LSN); let (file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 222; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 222; } cache.flush_page(0).unwrap(); let flushed_page = read_disk_page(file.path(), 0); assert_eq!(flushed_page[PAGE_SIZE - 1], 222); - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); assert_eq!( read_log_record_kinds_for_test(file.path()), [ (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 0 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 0 }), ] ); } @@ -1013,33 +1140,33 @@ mod tests { fn wal_flush_failure_prevents_transactional_page_write_and_leaves_frame_dirty() { let page = formatted_page_with_lsn(15, ZERO_LSN); let (file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); - runtime.begin_transaction().unwrap(); + let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } - runtime.fail_next_wal_flush_for_test(); + runtime.fail_next_wal_flush_for_test().unwrap(); let result = cache.flush_page(0); - assert!(matches!(result, Err(PageCacheError::Transaction(_)))); + assert!(matches!(result, Err(PageCacheError::Storage(_)))); let page_on_disk = read_disk_page(file.path(), 0); assert_eq!(page_on_disk[PAGE_SIZE - 1], page[PAGE_SIZE - 1]); - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } #[test] fn rollback_without_forced_wal_flush_restores_page_and_logs_transaction_outcome() { let page = formatted_page_with_lsn(15, ZERO_LSN); let (file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } let rollback = runtime.prepare_rollback_pages(txn_id).unwrap(); @@ -1051,7 +1178,11 @@ mod tests { assert_eq!(read_disk_page(file.path(), 0), page); assert_eq!( read_log_record_kinds_for_test(file.path()), - [(txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::Rollback),] + [ + (txn_id, OwnedLogRecordKind::Begin), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 0 }), + (txn_id, OwnedLogRecordKind::Rollback), + ] ); } @@ -1059,12 +1190,12 @@ mod tests { fn rollback_after_steal_flush_restores_page_and_writes_rollback_record() { let page = formatted_page_with_lsn(15, ZERO_LSN); let (file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 177; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 177; } cache.flush_page(0).unwrap(); @@ -1086,23 +1217,23 @@ mod tests { } #[test] - fn prepared_rollback_keeps_pending_wal_available_for_cache_eviction() { + fn prepared_rollback_keeps_eager_wal_available_for_cache_eviction() { let pages = [ formatted_page_with_lsn(10, ZERO_LSN), formatted_page_with_lsn(20, ZERO_LSN), formatted_page_with_lsn(30, ZERO_LSN), ]; let (file, runtime) = create_disk_with_pages(&pages); - let cache = PageCache::new(Rc::clone(&runtime), 2).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 2).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 100; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 100; } { let guard = cache.fetch_page(1).unwrap(); - guard.write().unwrap().page_mut()[PAGE_SIZE - 1] = 110; + guard.write(Some(txn_id)).unwrap().page_mut()[PAGE_SIZE - 1] = 110; } let rollback = runtime.prepare_rollback_pages(txn_id).unwrap(); @@ -1121,6 +1252,7 @@ mod tests { [ (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 0 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 1 }), (txn_id, OwnedLogRecordKind::Rollback), ] ); @@ -1130,19 +1262,19 @@ mod tests { fn wal_logging_failure_restores_page_bytes_and_dirty_state() { let page = formatted_page_with_lsn(17, ZERO_LSN); let (_file, runtime) = create_disk_with_pages(&[page]); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); - runtime.force_next_lsn_exhausted_for_test(); + runtime.force_next_lsn_exhausted_for_test().unwrap(); let guard = cache.fetch_page(0).unwrap(); { - let mut write = guard.write().unwrap(); + let mut write = guard.write(Some(txn_id)).unwrap(); write.page_mut()[PAGE_SIZE - 1] = 88; } assert_eq!(guard.read().unwrap().page(), &page); - assert!(!cache.inner.frames[0].dirty.get()); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); assert!(runtime.commit_transaction(txn_id).is_err()); } @@ -1152,19 +1284,25 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - cache.inner.frames[0].page_id.set(Some(99)); - *cache.inner.frames[0].data.borrow_mut() = page_with_pattern(15); - cache.inner.frames[0].dirty.set(true); - cache.inner.frames[0].pin_count.set(0); - cache.inner.meta.borrow_mut().page_table.insert(99, 0); + cache.inner.frames[0].set_page_id(Some(99)); + *cache.inner.frames[0].data.write().unwrap() = page_with_pattern(15); + cache.inner.frames[0].dirty.store(true, Ordering::Release); + cache.inner.frames[0].pin_count.store(0, Ordering::Release); + cache.inner.lock_meta().unwrap().page_table.insert(99, 0); let result = cache.flush_page(99); assert!(matches!( result, - Err(PageCacheError::Disk(DiskManagerError::InvalidPageId { page_id: 99 })) + Err(PageCacheError::Storage(error)) + if matches!( + *error, + StorageError::InvalidArgument( + crate::core::error::InvalidArgumentError::InvalidPageId { page_id: 99 } + ) + ) )); - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } #[test] @@ -1174,7 +1312,7 @@ mod tests { let cache = PageCache::new(disk_manager, 1).unwrap(); let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 99; + guard.write(None).unwrap().page_mut()[0] = 99; let result = cache.flush_page(0); assert!(matches!(result, Err(PageCacheError::PinnedPage { page_id: 0 }))); @@ -1201,17 +1339,17 @@ mod tests { { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 10; + guard.write(None).unwrap().page_mut()[0] = 10; } { let guard = cache.fetch_page(1).unwrap(); - guard.write().unwrap().page_mut()[0] = 20; + guard.write(None).unwrap().page_mut()[0] = 20; } cache.flush_all().unwrap(); for frame in &cache.inner.frames { - assert!(!frame.dirty.get()); + assert!(!frame.dirty.load(Ordering::Acquire)); } let page0 = read_disk_page(file.path(), 0); @@ -1227,7 +1365,7 @@ mod tests { let cache = PageCache::new(disk_manager, 1).unwrap(); let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 99; + guard.write(None).unwrap().page_mut()[0] = 99; let result = cache.flush_all(); assert!(matches!(result, Err(PageCacheError::PinnedPage { page_id: 0 }))); @@ -1243,9 +1381,9 @@ mod tests { let cache = PageCache::new(disk_manager, 1).unwrap(); { let guard = cache.fetch_page(0).unwrap(); - guard.write().unwrap().page_mut()[0] = 144; + guard.write(None).unwrap().page_mut()[0] = 144; } - assert!(cache.inner.frames[0].dirty.get()); + assert!(cache.inner.frames[0].dirty.load(Ordering::Acquire)); } let page_on_disk = read_disk_page(file.path(), 0); @@ -1258,7 +1396,7 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - let (page_id, guard) = cache.new_page().unwrap(); + let (page_id, guard) = cache.new_page(None).unwrap(); assert_eq!(page_id, 0); assert_eq!(guard.read().unwrap().page(), &[0u8; PAGE_SIZE]); } @@ -1269,11 +1407,11 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - let (first_page_id, first_guard) = cache.new_page().unwrap(); + let (first_page_id, first_guard) = cache.new_page(None).unwrap(); assert_eq!(first_page_id, 0); drop(first_guard); - let (second_page_id, second_guard) = cache.new_page().unwrap(); + let (second_page_id, second_guard) = cache.new_page(None).unwrap(); assert_eq!(second_page_id, 1); drop(second_guard); } @@ -1284,7 +1422,7 @@ mod tests { let runtime = runtime_for_path(file.path()); let cache = PageCache::new(runtime, 1).unwrap(); - let (page_id, guard) = cache.new_page().unwrap(); + let (page_id, guard) = cache.new_page(None).unwrap(); drop(guard); assert_eq!(page_id, 0); @@ -1295,10 +1433,10 @@ mod tests { fn new_page_with_active_transaction_writes_page_alloc_wal_record() { let file = NamedTempFile::new().unwrap(); let runtime = runtime_for_path(file.path()); - let cache = PageCache::new(Rc::clone(&runtime), 1).unwrap(); + let cache = PageCache::new(Arc::clone(&runtime), 1).unwrap(); let txn_id = runtime.begin_transaction().unwrap(); - let (page_id, guard) = cache.new_page().unwrap(); + let (page_id, guard) = cache.new_page(Some(txn_id)).unwrap(); drop(guard); runtime.commit_transaction(txn_id).unwrap(); @@ -1319,9 +1457,9 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - cache.inner.frames[0].pin_count.set(1); + cache.inner.frames[0].pin_count.store(1, Ordering::Release); - let result = cache.new_page(); + let result = cache.new_page(None); assert!(matches!(result, Err(PageCacheError::NoEvictableFrame))); let mut disk_manager = DiskManager::new(file.path()).unwrap(); @@ -1337,8 +1475,8 @@ mod tests { let page_id = { let cache = PageCache::new(disk_manager, 1).unwrap(); - let (page_id, guard) = cache.new_page().unwrap(); - let mut page = guard.write().unwrap(); + let (page_id, guard) = cache.new_page(None).unwrap(); + let mut page = guard.write(None).unwrap(); page.page_mut()[0] = 61; page.page_mut()[PAGE_SIZE - 1] = 142; drop(page); @@ -1361,7 +1499,7 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - cache.inner.meta.borrow_mut().page_table.insert(7, 99); + cache.inner.lock_meta().unwrap().page_table.insert(7, 99); let result = cache.fetch_page(7); assert!(matches!( @@ -1376,7 +1514,7 @@ mod tests { let disk_manager = runtime_for_path(file.path()); let cache = PageCache::new(disk_manager, 1).unwrap(); - cache.inner.meta.borrow_mut().page_table.insert(8, 100); + cache.inner.lock_meta().unwrap().page_table.insert(8, 100); let result = cache.flush_page(8); assert!(matches!( @@ -1389,3 +1527,204 @@ mod tests { )); } } + +#[cfg(all(test, loom))] +#[allow(clippy::panic, clippy::unwrap_used)] +mod loom_tests { + use loom::sync::{ + Arc, + atomic::{AtomicUsize, Ordering as LoomOrdering}, + }; + use tempfile::NamedTempFile; + + use super::*; + use crate::{ + loom_support::{check_model, thread}, + storage::{disk_manager::DiskManager, storage_runtime::StorageRuntime}, + }; + + fn cache_with_pages(patterns: &[u8], frame_count: usize) -> (NamedTempFile, PageCache) { + let file = NamedTempFile::new().unwrap(); + let mut disk = DiskManager::new(file.path()).unwrap(); + for (page_id, pattern) in patterns.iter().copied().enumerate() { + assert_eq!(disk.new_page().unwrap(), page_id as PageId); + disk.write_page(page_id as PageId, &[pattern; PAGE_SIZE]).unwrap(); + } + let runtime = Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk).unwrap()); + (file, PageCache::new(runtime, frame_count).unwrap()) + } + + fn assert_guard_matches_frame(guard: &PinGuard, pattern: u8) { + let _meta = guard.page_cache.lock_meta().unwrap(); + let frame = &guard.page_cache.frames[guard.frame_id]; + assert_eq!(frame.page_id(), Some(guard.page_id)); + assert!(frame.pin_count.load(Ordering::Acquire) > 0); + assert_eq!(guard.read().unwrap().page()[0], pattern); + } + + fn assert_cache_consistent(cache: &PageCache) { + let meta = cache.inner.lock_meta().unwrap(); + for (page_id, frame_id) in &meta.page_table { + assert!(*frame_id < cache.inner.frames.len()); + assert_eq!(cache.inner.frames[*frame_id].page_id(), Some(*page_id)); + } + for (frame_id, frame) in cache.inner.frames.iter().enumerate() { + assert_eq!(frame.pin_count.load(Ordering::Acquire), 0); + if let Some(page_id) = frame.page_id() { + assert_eq!(meta.page_table.get(&page_id), Some(&frame_id)); + } + } + } + + #[test] + fn concurrent_hits_keep_each_live_pin_counted() { + check_model(|| { + let (_file, cache) = cache_with_pages(&[17], 1); + drop(cache.fetch_page(0).unwrap()); + + let first_cache = cache.clone(); + let first = thread::spawn(move || { + let guard = first_cache.fetch_page(0).unwrap(); + thread::yield_now(); + assert_guard_matches_frame(&guard, 17); + drop(guard); + }); + let second_cache = cache.clone(); + let second = thread::spawn(move || { + let guard = second_cache.fetch_page(0).unwrap(); + thread::yield_now(); + assert_guard_matches_frame(&guard, 17); + drop(guard); + }); + + first.join().unwrap(); + second.join().unwrap(); + assert_cache_consistent(&cache); + }); + } + + #[test] + fn concurrent_misses_for_one_page_install_one_resident_frame() { + check_model(|| { + let (_file, cache) = cache_with_pages(&[29], 2); + + let first_cache = cache.clone(); + let first = thread::spawn(move || { + let guard = first_cache.fetch_page(0).unwrap(); + thread::yield_now(); + assert_guard_matches_frame(&guard, 29); + }); + let second_cache = cache.clone(); + let second = thread::spawn(move || { + let guard = second_cache.fetch_page(0).unwrap(); + thread::yield_now(); + assert_guard_matches_frame(&guard, 29); + }); + + first.join().unwrap(); + second.join().unwrap(); + assert_eq!( + cache.inner.frames.iter().filter(|frame| frame.page_id() == Some(0)).count(), + 1 + ); + assert_cache_consistent(&cache); + }); + } + + #[test] + fn cache_hit_racing_with_eviction_never_retargets_a_live_guard() { + check_model(|| { + let (_file, cache) = cache_with_pages(&[41, 73], 1); + drop(cache.fetch_page(0).unwrap()); + let successes = Arc::new(AtomicUsize::new(0)); + + let first_cache = cache.clone(); + let first_successes = Arc::clone(&successes); + let first = thread::spawn(move || { + if let Ok(guard) = first_cache.fetch_page(0) { + first_successes.fetch_add(1, LoomOrdering::Relaxed); + thread::yield_now(); + assert_guard_matches_frame(&guard, 41); + } + }); + let second_cache = cache.clone(); + let second_successes = Arc::clone(&successes); + let second = thread::spawn(move || { + if let Ok(guard) = second_cache.fetch_page(1) { + second_successes.fetch_add(1, LoomOrdering::Relaxed); + thread::yield_now(); + assert_guard_matches_frame(&guard, 73); + } + }); + + first.join().unwrap(); + second.join().unwrap(); + assert!(successes.load(LoomOrdering::Relaxed) >= 1); + assert_cache_consistent(&cache); + }); + } + + #[test] + fn flush_racing_with_write_does_not_lose_the_dirty_generation() { + check_model(|| { + let (file, cache) = cache_with_pages(&[113], 1); + drop(cache.fetch_page(0).unwrap()); + + let writer_cache = cache.clone(); + let writer = thread::spawn(move || { + let guard = writer_cache.fetch_page(0).unwrap(); + let mut page = guard.write(None).unwrap(); + page.page_mut()[0] = 127; + thread::yield_now(); + }); + let flush_cache = cache.clone(); + let flush = thread::spawn(move || flush_cache.flush_all()); + + writer.join().unwrap(); + let flush_result = flush.join().unwrap(); + assert!( + flush_result.is_ok() + || matches!(flush_result, Err(PageCacheError::PinnedPage { page_id: 0 })) + ); + + cache.flush_all().unwrap(); + let mut disk = DiskManager::new(file.path()).unwrap(); + let mut page = [0; PAGE_SIZE]; + disk.read_page(0, &mut page).unwrap(); + assert_eq!(page[0], 127); + assert!(!cache.inner.frames[0].dirty.load(Ordering::Acquire)); + }); + } + + #[test] + fn concurrent_misses_do_not_share_one_victim_frame() { + check_model(|| { + let (_file, cache) = cache_with_pages(&[89, 101], 1); + let successes = Arc::new(AtomicUsize::new(0)); + + let first_cache = cache.clone(); + let first_successes = Arc::clone(&successes); + let first = thread::spawn(move || { + if let Ok(guard) = first_cache.fetch_page(0) { + first_successes.fetch_add(1, LoomOrdering::Relaxed); + thread::yield_now(); + assert_guard_matches_frame(&guard, 89); + } + }); + let second_cache = cache.clone(); + let second_successes = Arc::clone(&successes); + let second = thread::spawn(move || { + if let Ok(guard) = second_cache.fetch_page(1) { + second_successes.fetch_add(1, LoomOrdering::Relaxed); + thread::yield_now(); + assert_guard_matches_frame(&guard, 101); + } + }); + + first.join().unwrap(); + second.join().unwrap(); + assert!(successes.load(LoomOrdering::Relaxed) >= 1); + assert_cache_consistent(&cache); + }); + } +} diff --git a/src/storage/recovery.rs b/src/storage/recovery.rs index cd849e1..e666004 100644 --- a/src/storage/recovery.rs +++ b/src/storage/recovery.rs @@ -203,6 +203,59 @@ mod tests { assert_eq!(read_disk_page(file.path(), 0), after); } + #[test] + fn recovery_tracks_interleaved_frames_by_transaction_id() { + let file = NamedTempFile::new().unwrap(); + let first_before = formatted_page(1, ZERO_LSN); + let first_after = formatted_page(2, 2); + let second_before = formatted_page(3, ZERO_LSN); + let second_after = formatted_page(4, 4); + { + let mut disk = DiskManager::new(file.path()).unwrap(); + disk.ensure_page_exists(1).unwrap(); + disk.write_page(0, &first_after).unwrap(); + disk.write_page(1, &second_after).unwrap(); + } + + append_transaction( + file.path(), + 1, + &[ + LogRecord { txn_id: 1, kind: LogRecordKind::Begin }, + LogRecord { + txn_id: 1, + kind: LogRecordKind::PageUpdate { + page_id: 0, + redo_data: &first_after, + undo_data: &first_before, + }, + }, + ], + ); + append_transaction( + file.path(), + 2, + &[ + LogRecord { txn_id: 2, kind: LogRecordKind::Begin }, + LogRecord { + txn_id: 2, + kind: LogRecordKind::PageUpdate { + page_id: 1, + redo_data: &second_after, + undo_data: &second_before, + }, + }, + ], + ); + append_transaction(file.path(), 1, &[LogRecord { txn_id: 1, kind: LogRecordKind::Commit }]); + + let mut disk = DiskManager::new(file.path()).unwrap(); + recover_from_wal(file.path(), &mut disk).unwrap(); + + assert_eq!(read_disk_page(file.path(), 0), first_after); + assert_eq!(read_disk_page(file.path(), 1), second_before); + } + #[test] fn recovery_replays_committed_update_when_page_lsn_is_current() { let file = NamedTempFile::new().unwrap(); diff --git a/src/storage/storage_runtime.rs b/src/storage/storage_runtime.rs index 78f3533..32366f0 100644 --- a/src/storage/storage_runtime.rs +++ b/src/storage/storage_runtime.rs @@ -1,13 +1,9 @@ -use std::{cell::RefCell, path::PathBuf}; +use std::path::PathBuf; -use crate::core::{ - PAGE_SIZE, PageId, - error::{InternalError, InvariantViolation, StorageError, StorageResult}, -}; #[cfg(test)] use crate::storage::transaction_manager::FaultInjectingTransactionManager; use crate::storage::{ - disk_manager::{DiskManager, DiskManagerError}, + disk_manager::DiskManager, log_manager::{LogManager, Lsn, TxnId}, recovery::recover_from_wal, transaction_manager::{ @@ -15,6 +11,13 @@ use crate::storage::{ TransactionSavepoint, }, }; +use crate::{ + core::{ + PAGE_SIZE, PageId, + error::{InternalError, StorageError, StorageResult}, + }, + sync::{Mutex, MutexGuard}, +}; #[cfg(not(test))] type ActiveTransactionManager = TransactionManager; @@ -36,11 +39,13 @@ fn make_transaction_manager(max_txn_id: TxnId) -> ActiveTransactionManager { /// The runtime keeps raw database-file I/O and WAL I/O adjacent without making /// either manager own the other. Page cache code uses it for WAL-protected page /// writes, and future transaction code can share the same log manager. +/// Operations that need both transaction and log state always lock transactions +/// before the log. pub(crate) struct StorageRuntime { path: PathBuf, - disk: RefCell, - log: RefCell, - transactions: RefCell, + disk: Mutex, + log: Mutex, + transactions: Mutex, } impl StorageRuntime { @@ -51,9 +56,15 @@ impl StorageRuntime { let max_txn_id = recovery.max_txn_id.max(log.highest_txn_id()); Ok(Self { path, - disk: RefCell::new(disk), - log: RefCell::new(log), - transactions: RefCell::new(make_transaction_manager(max_txn_id)), + disk: Mutex::new(disk), + log: Mutex::new(log), + transactions: Mutex::new(make_transaction_manager(max_txn_id)), + }) + } + + fn lock<'a, T>(mutex: &'a Mutex, lock: &'static str) -> StorageResult> { + mutex.lock().map_err(|_poisoned| { + StorageError::Internal(InternalError::SynchronizationPoisoned { lock }) }) } @@ -61,138 +72,204 @@ impl StorageRuntime { &self.path } - pub(crate) fn new_page(&self) -> Result { - self.disk.borrow_mut().new_page() + pub(crate) fn new_page(&self) -> StorageResult { + Ok(Self::lock(&self.disk, "disk manager")?.new_page()?) } - pub(crate) fn record_page_alloc(&self, page_id: PageId) -> StorageResult> { - let Some(txn_id) = self.active_transaction_id() else { + pub(crate) fn record_page_alloc( + &self, + txn_id: Option, + page_id: PageId, + ) -> StorageResult> { + let Some(txn_id) = txn_id else { return Ok(None); }; - self.transactions.borrow_mut().record_page_alloc(txn_id, page_id) + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + transactions.record_page_alloc(&mut log, txn_id, page_id) } pub(crate) fn read_page( &self, page_id: PageId, buf: &mut [u8; PAGE_SIZE], - ) -> Result<(), DiskManagerError> { - self.disk.borrow_mut().read_page(page_id, buf) + ) -> StorageResult<()> { + Ok(Self::lock(&self.disk, "disk manager")?.read_page(page_id, buf)?) } - pub(crate) fn write_page( - &self, - page_id: PageId, - buf: &[u8; PAGE_SIZE], - ) -> Result<(), DiskManagerError> { - self.disk.borrow_mut().write_page(page_id, buf) + pub(crate) fn write_page(&self, page_id: PageId, buf: &[u8; PAGE_SIZE]) -> StorageResult<()> { + Ok(Self::lock(&self.disk, "disk manager")?.write_page(page_id, buf)?) } - pub(crate) fn sync_database_file(&self) -> Result<(), DiskManagerError> { - self.disk.borrow().sync() + pub(crate) fn sync_database_file(&self) -> StorageResult<()> { + Ok(Self::lock(&self.disk, "disk manager")?.sync()?) } #[cfg(test)] - pub(crate) fn unlock_for_crash_for_test(&self) -> Result<(), DiskManagerError> { - self.disk.borrow_mut().unlock_for_crash_for_test() + pub(crate) fn unlock_for_crash_for_test(&self) -> StorageResult<()> { + Ok(Self::lock(&self.disk, "disk manager")?.unlock_for_crash_for_test()?) } pub(crate) fn flush_wal_through(&self, lsn: Lsn) -> StorageResult<()> { - let mut log = self.log.borrow_mut(); - if let Some(txn_id) = self.active_transaction_id() { - self.transactions.borrow_mut().append_pending_through(txn_id, &mut log, lsn)?; - } - log.flush_through(lsn)?; + Self::lock(&self.log, "log manager")?.flush_through(lsn)?; Ok(()) } #[cfg(test)] - pub(crate) fn force_next_lsn_exhausted_for_test(&self) { - if !self.transactions.borrow_mut().force_next_lsn_exhausted_for_test() { - self.log.borrow_mut().force_next_lsn_exhausted_for_test(); - } + pub(crate) fn force_next_lsn_exhausted_for_test(&self) -> StorageResult<()> { + Self::lock(&self.log, "log manager")?.force_next_lsn_exhausted_for_test(); + Ok(()) } #[cfg(test)] - pub(crate) fn fail_next_savepoint_rollback_for_test(&self) { - self.transactions.borrow_mut().fail_next_savepoint_rollback(); + pub(crate) fn fail_next_savepoint_rollback_for_test(&self) -> StorageResult<()> { + Self::lock(&self.transactions, "transaction manager")?.fail_next_savepoint_rollback(); + Ok(()) } #[cfg(test)] - pub(crate) fn fail_next_wal_flush_for_test(&self) { - self.log.borrow_mut().fail_next_flush_for_test(); + pub(crate) fn fail_next_wal_flush_for_test(&self) -> StorageResult<()> { + Self::lock(&self.log, "log manager")?.fail_next_flush_for_test(); + Ok(()) } pub(crate) fn begin_transaction(&self) -> StorageResult { - if let Some(txn_id) = self.active_transaction_id() { - return Err(StorageError::Internal(InternalError::InvariantViolation( - InvariantViolation::ActiveTransaction { txn_id }, - ))); - } - self.transactions.borrow_mut().begin(&mut self.log.borrow_mut()) + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + transactions.begin(&mut log) } pub(crate) fn record_page_update( &self, + txn_id: Option, page_id: PageId, before: &[u8; PAGE_SIZE], after: &[u8; PAGE_SIZE], ) -> StorageResult> { - let Some(txn_id) = self.active_transaction_id() else { + let Some(txn_id) = txn_id else { return Ok(None); }; - let result = - self.transactions.borrow_mut().record_page_update(txn_id, page_id, before, after); + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + let result = transactions.record_page_update(&mut log, txn_id, page_id, before, after); if result.is_err() { - self.transactions.borrow_mut().record_failure(txn_id); + transactions.record_failure(txn_id); } result } - pub(crate) fn record_transaction_failure(&self) { - if let Some(txn_id) = self.active_transaction_id() { - self.transactions.borrow_mut().record_failure(txn_id); - } + pub(crate) fn record_transaction_failure(&self, txn_id: TxnId) -> StorageResult<()> { + Self::lock(&self.transactions, "transaction manager")?.record_failure(txn_id); + Ok(()) } - pub(crate) fn active_transaction_id(&self) -> Option { - self.transactions.borrow().active_transaction_id() + pub(crate) fn transaction_is_active(&self, txn_id: TxnId) -> StorageResult { + Ok(Self::lock(&self.transactions, "transaction manager")?.transaction_is_active(txn_id)) } pub(crate) fn transaction_is_poisoned(&self, txn_id: TxnId) -> StorageResult { - self.transactions.borrow().transaction_is_poisoned(txn_id) + Self::lock(&self.transactions, "transaction manager")?.transaction_is_poisoned(txn_id) } pub(crate) fn commit_transaction(&self, txn_id: TxnId) -> StorageResult<()> { - self.transactions.borrow_mut().commit(&mut self.log.borrow_mut(), txn_id) + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + transactions.commit(&mut log, txn_id) } pub(crate) fn statement_savepoint(&self, txn_id: TxnId) -> StorageResult { - self.transactions.borrow().statement_savepoint(txn_id) + Self::lock(&self.transactions, "transaction manager")?.statement_savepoint(txn_id) } pub(crate) fn rollback_to_savepoint( &self, savepoint: TransactionSavepoint, ) -> StorageResult> { - self.transactions.borrow_mut().rollback_to_savepoint(savepoint) + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + transactions.rollback_to_savepoint(&mut log, savepoint) } pub(crate) fn complete_savepoint_rollback( &self, savepoint: TransactionSavepoint, ) -> StorageResult<()> { - self.transactions.borrow_mut().complete_savepoint_rollback(savepoint) + Self::lock(&self.transactions, "transaction manager")? + .complete_savepoint_rollback(savepoint) } pub(crate) fn prepare_rollback_pages( &self, txn_id: TxnId, ) -> StorageResult { - self.transactions.borrow_mut().prepare_rollback_pages(txn_id) + Self::lock(&self.transactions, "transaction manager")?.prepare_rollback_pages(txn_id) } pub(crate) fn finish_rollback(&self, txn_id: TxnId) -> StorageResult<()> { - self.transactions.borrow_mut().finish_rollback(&mut self.log.borrow_mut(), txn_id) + let mut transactions = Self::lock(&self.transactions, "transaction manager")?; + let mut log = Self::lock(&self.log, "log manager")?; + transactions.finish_rollback(&mut log, txn_id) + } +} + +#[cfg(all(test, not(loom)))] +#[allow(clippy::panic, clippy::unwrap_used)] +mod tests { + use std::{sync::Arc, thread}; + + use tempfile::NamedTempFile; + + use super::*; + + #[test] + fn poisoned_manager_lock_is_reported_as_an_internal_error() { + let file = NamedTempFile::new().unwrap(); + let disk = DiskManager::new(file.path()).unwrap(); + let runtime = Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk).unwrap()); + + let poisoned_runtime = Arc::clone(&runtime); + let panicked = thread::spawn(move || { + let _transactions = poisoned_runtime.transactions.lock().unwrap(); + panic!("poison transaction manager"); + }) + .join(); + assert!(panicked.is_err()); + + assert!(matches!( + runtime.transaction_is_active(1), + Err(StorageError::Internal(InternalError::SynchronizationPoisoned { + lock: "transaction manager" + })) + )); + } +} + +#[cfg(all(test, loom))] +#[allow(clippy::panic, clippy::unwrap_used)] +mod loom_tests { + use loom::sync::Arc; + use tempfile::NamedTempFile; + + use super::*; + use crate::{ + loom_support::{check_model, thread}, + storage::log_manager::ZERO_LSN, + }; + + #[test] + fn wal_flush_and_transaction_begin_share_log_safely() { + check_model(|| { + let file = NamedTempFile::new().unwrap(); + let disk = DiskManager::new(file.path()).unwrap(); + let runtime = Arc::new(StorageRuntime::new(file.path().to_path_buf(), disk).unwrap()); + + let flush_runtime = Arc::clone(&runtime); + let flush = thread::spawn(move || flush_runtime.flush_wal_through(ZERO_LSN)); + let begin_runtime = Arc::clone(&runtime); + let begin = thread::spawn(move || begin_runtime.begin_transaction()); + + flush.join().unwrap().unwrap(); + assert!(begin.join().unwrap().is_ok()); + }); } } diff --git a/src/storage/transaction_manager.rs b/src/storage/transaction_manager.rs index ec105c1..6d79d47 100644 --- a/src/storage/transaction_manager.rs +++ b/src/storage/transaction_manager.rs @@ -1,10 +1,10 @@ //! Transaction coordinator for WAL-backed page changes. //! //! While a transaction is active, page allocations and full-page updates are -//! assigned LSNs immediately, but WAL bytes are only appended when the -//! write-ahead rule requires them or when the transaction commits. Rollback -//! uses the in-memory undo images accumulated here; crash recovery uses the -//! durable WAL records written by [`LogManager`]. +//! appended to the WAL immediately, while durability is deferred until the +//! write-ahead rule or transaction outcome requires a flush. Rollback uses the +//! in-memory undo images accumulated here; crash recovery uses the durable WAL +//! records written by [`LogManager`]. use std::collections::HashMap; @@ -13,7 +13,7 @@ use crate::core::{ error::{InternalError, InvariantViolation, StorageError, StorageResult}, }; use crate::storage::{ - log_manager::{LogManager, LogManagerError, LogRecord, LogRecordKind, Lsn, TxnId, ZERO_LSN}, + log_manager::{LogManager, LogRecordKind, Lsn, TxnId}, page, }; @@ -33,8 +33,6 @@ struct PageUndo { after: [u8; PAGE_SIZE], /// LSN assigned to the update that this image undoes. lsn: Lsn, - /// Index of the matching pending WAL record. - pending_record_index: usize, } /// Page image to install while rolling back in memory. @@ -59,7 +57,7 @@ pub(crate) struct TransactionRollback { #[derive(Debug, Clone, Copy)] pub(crate) struct TransactionSavepoint { /// Transaction that created this savepoint, used to reject stale handles. - txn_id: TxnId, + pub(crate) txn_id: TxnId, /// Undo-log boundary before the statement made any page changes. undo_len: usize, } @@ -79,10 +77,9 @@ pub(crate) struct LoggedPageUpdate { /// Tracks active transactions and their rollback state. /// /// `TransactionManager` is deliberately small: it assigns monotonically -/// increasing transaction ids, buffers or appends transaction-control records, -/// remembers in-memory undo images for explicit rollback, and marks a -/// transaction as poisoned after an error that may have left its effects only -/// partially logged. +/// increasing transaction ids, remembers in-memory undo images for explicit +/// rollback, and marks a transaction as poisoned after an error that may have +/// left its effects only partially logged. #[derive(Debug)] pub(crate) struct TransactionManager { /// Greatest transaction ID issued by this manager or observed during open. @@ -92,46 +89,19 @@ pub(crate) struct TransactionManager { } /// In-memory state for one transaction owned by the storage runtime. -/// -/// WAL records can be assigned logical LSNs before they are physically appended. -/// Consequently, `last_lsn` and the LSNs in `pending_records` describe the -/// transaction's intended WAL order, while each record's `appended` flag records -/// how much of that order has actually reached the WAL writer. #[derive(Debug)] struct ActiveTransaction { - /// ID assigned when the transaction began. - txn_id: TxnId, - /// Greatest logical LSN reserved for a buffered record in this transaction. - /// - /// This can be ahead of the log manager's highest appended LSN. Repeated - /// updates coalesced into one pending page record reuse that record's LSN and - /// therefore do not advance this value. - last_lsn: Lsn, - /// Transaction records in reserved LSN order. - /// - /// Appended entries remain in the vector because [`PageUndo`] values refer - /// to records by stable index when deciding whether rollback needs a WAL - /// flush before writing a before-image. - pending_records: Vec, - /// Unappended page-update record currently eligible for redo coalescing. - /// - /// Each value indexes `pending_records`. The entry is removed when its WAL - /// record is appended or when savepoint compensation starts for the page, - /// after which another update must reserve a new record and LSN. - pending_page_updates: HashMap, /// One before-image per logical page mutation, in mutation order. /// - /// Unlike `pending_records`, this log is not coalesced: explicit rollback - /// walks every entry in reverse so repeated writes restore intermediate - /// images before finally restoring the transaction's original image. + /// Explicit rollback walks every entry in reverse so repeated writes restore + /// intermediate images before finally restoring the transaction's original + /// image. undo_pages: Vec, /// LSN of an appended rollback outcome awaiting or having completed flush. /// /// `Some` means physical page restoration has finished and rollback /// finalization has begun. If flushing fails, retrying uses this same LSN - /// instead of appending a duplicate outcome. Pending transaction records - /// must not be appended once this is set because the rollback record already - /// occupies the next physical WAL position. + /// instead of appending a duplicate outcome. rollback_lsn: Option, /// Whether commit is unsafe for this transaction. /// @@ -142,41 +112,6 @@ struct ActiveTransaction { poisoned: bool, } -/// WAL record reserved by the active transaction. -#[derive(Debug)] -struct PendingLogRecord { - /// Logical LSN this record must receive if it is appended. - lsn: Lsn, - /// Payload retained until commit, rollback, or write-ahead flushing decides its fate. - kind: PendingLogRecordKind, - /// Whether a complete frame containing this record was appended. - /// - /// This does not imply durability; [`LogManager::flush_through`] establishes - /// that separately. - appended: bool, -} - -/// Owned payload for a WAL record buffered by the transaction manager. -#[derive(Debug)] -enum PendingLogRecordKind { - /// Start marker reserved when the transaction is created. - Begin, - /// Full-page physical update used for both redo and undo during recovery. - PageUpdate { - /// Page changed by this record. - page_id: PageId, - /// Latest page image to install when redoing the transaction. - redo_data: Box<[u8; PAGE_SIZE]>, - /// Earliest page image covered by this record, restored when undoing it. - undo_data: Box<[u8; PAGE_SIZE]>, - }, - /// Page whose allocation becomes visible if the transaction commits. - PageAlloc { - /// Allocated database page. - page_id: PageId, - }, -} - impl TransactionManager { /// Creates a manager whose next transaction id will be greater than `max_txn_id`. /// @@ -187,7 +122,7 @@ impl TransactionManager { Self { max_txn_id, transactions: HashMap::new() } } - /// Begins a transaction and buffers its `Begin` WAL record. + /// Begins a transaction and appends its `Begin` WAL record. /// /// Returns an invariant violation if the transaction-id counter is exhausted. pub(crate) fn begin(&mut self, log: &mut LogManager) -> StorageResult { @@ -195,36 +130,15 @@ impl TransactionManager { .max_txn_id .checked_add(1) .ok_or_else(|| invariant(InvariantViolation::TransactionIdExhausted))?; - let lsn = self.next_lsn(log)?; + log.append_record(txn_id, LogRecordKind::Begin)?; self.max_txn_id = txn_id; self.transactions.insert( txn_id, - ActiveTransaction { - txn_id, - last_lsn: lsn, - pending_records: vec![PendingLogRecord { - lsn, - kind: PendingLogRecordKind::Begin, - appended: false, - }], - pending_page_updates: HashMap::new(), - undo_pages: Vec::new(), - rollback_lsn: None, - poisoned: false, - }, + ActiveTransaction { undo_pages: Vec::new(), rollback_lsn: None, poisoned: false }, ); Ok(txn_id) } - /// Reserves an LSN after both the log and all active transactions. - fn next_lsn(&self, log: &LogManager) -> StorageResult { - self.transactions - .values() - .map(|active| active.last_lsn) - .max() - .map_or_else(|| log.next_lsn().map_err(Into::into), next_lsn) - } - /// Records a page allocation for a transaction, if it exists. /// /// Page allocations outside a transaction are allowed and do not write WAL. @@ -233,181 +147,77 @@ impl TransactionManager { /// before replaying their updates. pub(crate) fn record_page_alloc( &mut self, + log: &mut LogManager, txn_id: TxnId, page_id: PageId, ) -> StorageResult> { // Allocated page ids are not reclaimed on rollback until a freelist exists. - let Some(active) = self.transactions.get_mut(&txn_id) else { + if !self.transactions.contains_key(&txn_id) { return Ok(None); - }; - let lsn = match next_lsn(active.last_lsn) { - Ok(lsn) => lsn, + } + match log.append_record(txn_id, LogRecordKind::PageAlloc { page_id }) { + Ok(lsn) => Ok(Some(lsn)), Err(err) => { - active.poisoned = true; - return Err(err); + self.record_failure(txn_id); + Err(err.into()) } - }; - active.pending_records.push(PendingLogRecord { - lsn, - kind: PendingLogRecordKind::PageAlloc { page_id }, - appended: false, - }); - active.last_lsn = lsn; - Ok(Some(lsn)) + } } - /// Buffers a full-page update record for a transaction, if any. + /// Appends a full-page update record for a transaction, if any. /// /// When the transaction is not active, the update is not logged and `Ok(None)` - /// is returned. With an active transaction, this method reserves the next - /// LSN, stamps it into the redo image for current B+-tree pages, buffers a - /// `PageUpdate` WAL record containing both redo and undo full-page images, - /// and remembers the undo image for explicit rollback. + /// is returned. With an active transaction, this method obtains the next WAL + /// position, stamps it into the redo image for current B+-tree pages, appends + /// the update containing both redo and undo full-page images, and remembers + /// the undo image for explicit rollback. /// - /// If LSN reservation or WAL append later fails, the active transaction is marked - /// poisoned. A poisoned transaction cannot commit because the caller can no - /// longer prove that all page effects were logged. + /// If WAL insertion fails, the active transaction is marked poisoned. A + /// poisoned transaction cannot commit because the caller can no longer prove + /// that all page effects were logged. pub(crate) fn record_page_update( &mut self, + log: &mut LogManager, txn_id: TxnId, page_id: PageId, before: &[u8; PAGE_SIZE], after: &[u8; PAGE_SIZE], ) -> StorageResult> { - let Some(active) = self.transactions.get_mut(&txn_id) else { + if !self.transactions.contains_key(&txn_id) { return Ok(None); - }; - - if let Some(&pending_record_index) = active.pending_page_updates.get(&page_id) { - let record = &mut active.pending_records[pending_record_index]; - if let PendingLogRecordKind::PageUpdate { redo_data, .. } = &mut record.kind { - let mut redo = *after; - stamp_page_lsn(&mut redo, record.lsn); - **redo_data = redo; - active.undo_pages.push(PageUndo { - page_id, - before: *before, - after: redo, - lsn: record.lsn, - pending_record_index, - }); - return Ok(Some(LoggedPageUpdate { lsn: record.lsn, redo })); - } - - active.poisoned = true; - return Err(invariant(InvariantViolation::WalLog { - message: format!( - "pending page-update index {pending_record_index} for page {page_id} did not point to a PageUpdate record" - ), - })); } - let lsn = match next_lsn(active.last_lsn) { + let expected_lsn = match log.next_lsn() { Ok(lsn) => lsn, Err(err) => { - active.poisoned = true; - return Err(err); + self.record_failure(txn_id); + return Err(err.into()); } }; let mut redo = *after; - stamp_page_lsn(&mut redo, lsn); - - let pending_record_index = active.pending_records.len(); - active.pending_records.push(PendingLogRecord { - lsn, - kind: PendingLogRecordKind::PageUpdate { - page_id, - redo_data: Box::new(redo), - undo_data: Box::new(*before), - }, - appended: false, - }); - active.last_lsn = lsn; - active.undo_pages.push(PageUndo { - page_id, - before: *before, - after: redo, - lsn, - pending_record_index, - }); - active.pending_page_updates.insert(page_id, pending_record_index); - Ok(Some(LoggedPageUpdate { lsn, redo })) - } - - /// Appends a transaction's buffered records up to `requested_lsn`, preserving record order. - /// - /// This is a no-op after a rollback outcome has been appended. At that point - /// the outcome occupies the next physical WAL position and only its durability - /// flush may be retried; appending older reserved records would assign them - /// different LSNs from those stamped into their page images. - pub(crate) fn append_pending_through( - &mut self, - txn_id: TxnId, - log: &mut LogManager, - requested_lsn: Lsn, - ) -> StorageResult<()> { - if requested_lsn == ZERO_LSN { - return Ok(()); - } - - let Some(active) = self.transactions.get_mut(&txn_id) else { - return Ok(()); - }; - if active.rollback_lsn.is_some() { - return Ok(()); - } - - let Some(start) = active.pending_records.iter().position(|record| !record.appended) else { - return Ok(()); - }; - if active.pending_records[start].lsn > requested_lsn { - return Ok(()); - } - - let mut end = start; - while end < active.pending_records.len() - && !active.pending_records[end].appended - && active.pending_records[end].lsn <= requested_lsn - { - end += 1; - } - - let records = active.pending_records[start..end] - .iter() - .map(|record| pending_log_record(active.txn_id, record)) - .collect::>(); - let expected_lsn = active.pending_records[end - 1].lsn; - let appended_lsn = match log.append_transaction(active.txn_id, &records) { + stamp_page_lsn(&mut redo, expected_lsn); + let lsn = match log.append_record( + txn_id, + LogRecordKind::PageUpdate { page_id, redo_data: &redo, undo_data: before }, + ) { Ok(lsn) => lsn, Err(err) => { - active.poisoned = true; + self.record_failure(txn_id); return Err(err.into()); } }; - if appended_lsn != expected_lsn { - active.poisoned = true; + if lsn != expected_lsn { + self.record_failure(txn_id); return Err(invariant(InvariantViolation::WalLog { message: format!( - "pending WAL append assigned LSN {appended_lsn}, expected {expected_lsn}" + "page-update WAL append assigned LSN {lsn}, expected {expected_lsn}" ), })); } - for record in &mut active.pending_records[start..end] { - record.appended = true; - } - for pending_record_index in start..end { - if let PendingLogRecordKind::PageUpdate { page_id, .. } = - &active.pending_records[pending_record_index].kind - && active - .pending_page_updates - .get(page_id) - .is_some_and(|index| *index == pending_record_index) - { - active.pending_page_updates.remove(page_id); - } - } - Ok(()) + let active = self.transactions.get_mut(&txn_id).ok_or_else(no_active_transaction)?; + active.undo_pages.push(PageUndo { page_id, before: *before, after: redo, lsn }); + Ok(Some(LoggedPageUpdate { lsn, redo })) } /// Marks a transaction as unsafe to commit. @@ -420,10 +230,11 @@ impl TransactionManager { } } - /// Returns one active transaction id, if a transaction is open. - /// - /// The storage runtime is currently strictly sequential, so callers only - /// use this when there is at most one transaction making page changes. + pub(crate) fn transaction_is_active(&self, txn_id: TxnId) -> bool { + self.transactions.contains_key(&txn_id) + } + + #[cfg(test)] pub(crate) fn active_transaction_id(&self) -> Option { self.transactions.keys().next().copied() } @@ -440,47 +251,17 @@ impl TransactionManager { /// the transaction is no longer available for explicit rollback; recovery /// will decide the outcome from the WAL contents on the next open. pub(crate) fn commit(&mut self, log: &mut LogManager, txn_id: TxnId) -> StorageResult<()> { - let active = self.transaction(txn_id)?; - if active.poisoned { + if self.transaction(txn_id)?.poisoned { return Err(invariant(InvariantViolation::TransactionPoisoned { txn_id })); } - let commit_lsn = match next_lsn(active.last_lsn) { + let commit_lsn = match log.append_record(txn_id, LogRecordKind::Commit) { Ok(lsn) => lsn, Err(err) => { - if let Some(active) = self.transactions.get_mut(&txn_id) { - active.poisoned = true; - } - return Err(err); - } - }; - let mut records = active - .pending_records - .iter() - .filter(|record| !record.appended) - .map(|record| pending_log_record(txn_id, record)) - .collect::>(); - records.push(LogRecord { txn_id, kind: LogRecordKind::Commit }); - let appended_lsn = match log.append_transaction(txn_id, &records) { - Ok(lsn) => lsn, - Err(err) => { - if let Some(active) = self.transactions.get_mut(&txn_id) { - active.poisoned = true; - } + self.record_failure(txn_id); return Err(err.into()); } }; - if appended_lsn != commit_lsn { - if let Some(active) = self.transactions.get_mut(&txn_id) { - active.poisoned = true; - } - return Err(invariant(InvariantViolation::WalLog { - message: format!( - "commit WAL append assigned LSN {appended_lsn}, expected {commit_lsn}" - ), - })); - } - self.transactions.remove(&txn_id); log.flush_through(commit_lsn)?; Ok(()) @@ -506,10 +287,10 @@ impl TransactionManager { /// those entries remain available to a full transaction rollback. pub(crate) fn rollback_to_savepoint( &mut self, + log: &mut LogManager, savepoint: TransactionSavepoint, ) -> StorageResult> { - let active = - self.transactions.get_mut(&savepoint.txn_id).ok_or_else(no_active_transaction)?; + let active = self.transaction(savepoint.txn_id)?; if savepoint.undo_len > active.undo_pages.len() { return Err(invariant(InvariantViolation::InvalidTransactionSavepoint { txn_id: savepoint.txn_id, @@ -521,26 +302,37 @@ impl TransactionManager { let rollback_pages = active.undo_pages[savepoint.undo_len..].to_vec(); let mut restore_pages = Vec::with_capacity(rollback_pages.len()); for undo in rollback_pages.into_iter().rev() { - let lsn = match next_lsn(active.last_lsn) { + let expected_lsn = match log.next_lsn() { Ok(lsn) => lsn, Err(err) => { - active.poisoned = true; - return Err(err); + self.record_failure(savepoint.txn_id); + return Err(err.into()); } }; let mut redo = undo.before; - stamp_page_lsn(&mut redo, lsn); - active.pending_page_updates.remove(&undo.page_id); - active.pending_records.push(PendingLogRecord { - lsn, - kind: PendingLogRecordKind::PageUpdate { + stamp_page_lsn(&mut redo, expected_lsn); + let lsn = match log.append_record( + savepoint.txn_id, + LogRecordKind::PageUpdate { page_id: undo.page_id, - redo_data: Box::new(redo), - undo_data: Box::new(undo.after), + redo_data: &redo, + undo_data: &undo.after, }, - appended: false, - }); - active.last_lsn = lsn; + ) { + Ok(lsn) => lsn, + Err(err) => { + self.record_failure(savepoint.txn_id); + return Err(err.into()); + } + }; + if lsn != expected_lsn { + self.record_failure(savepoint.txn_id); + return Err(invariant(InvariantViolation::WalLog { + message: format!( + "compensation WAL append assigned LSN {lsn}, expected {expected_lsn}" + ), + })); + } restore_pages.push(PageRestore { page_id: undo.page_id, image: redo, @@ -578,9 +370,8 @@ impl TransactionManager { /// Returns a transaction's undo images for rollback. /// /// The returned vector is ordered from newest update to oldest update. The - /// active transaction stays available while callers restore pages because - /// cache eviction during restore may still need to append buffered WAL - /// records for dirty transaction pages. + /// active transaction stays available while callers restore pages so a + /// failed restoration can still be retried. pub(crate) fn prepare_rollback_pages( &mut self, txn_id: TxnId, @@ -591,16 +382,10 @@ impl TransactionManager { .undo_pages .iter() .rev() - .map(|undo| { - let appended = active - .pending_records - .get(undo.pending_record_index) - .is_some_and(|record| record.appended); - PageRestore { - page_id: undo.page_id, - image: undo.before, - wal_flush_lsn: if appended { undo.lsn } else { ZERO_LSN }, - } + .map(|undo| PageRestore { + page_id: undo.page_id, + image: undo.before, + wal_flush_lsn: undo.lsn, }) .collect::>(); pages.shrink_to_fit(); @@ -610,9 +395,7 @@ impl TransactionManager { /// Writes and flushes the `Rollback` record after undo pages reach disk. /// /// Callers perform the physical page restoration first, then use this - /// method to make the completed rollback durable in the WAL. When no prior - /// record was appended, a compact `Begin`/`Rollback` frame is written so the - /// assigned transaction ID remains observable after restart. + /// method to append and make the completed rollback durable in the WAL. /// /// If the outcome is appended but flushing fails, the active transaction is /// retained with `rollback_lsn` set. Calling this method again flushes through @@ -623,26 +406,16 @@ impl TransactionManager { txn_id: TxnId, ) -> StorageResult<()> { let active = self.transactions.get_mut(&txn_id).ok_or_else(no_active_transaction)?; - active.poisoned = true; - let rollback_lsn = match active.rollback_lsn { - Some(lsn) => lsn, - None => { - let lsn = if active.pending_records.iter().any(|record| record.appended) { - log.append_record(txn_id, LogRecordKind::Rollback)? - } else { - log.append_transaction( - txn_id, - &[ - LogRecord { txn_id, kind: LogRecordKind::Begin }, - LogRecord { txn_id, kind: LogRecordKind::Rollback }, - ], - )? - }; - active.rollback_lsn = Some(lsn); - lsn - } - }; + if let Some(rollback_lsn) = active.rollback_lsn { + log.flush_through(rollback_lsn)?; + self.transactions.remove(&txn_id); + return Ok(()); + } + + let rollback_lsn = log.append_record(txn_id, LogRecordKind::Rollback)?; + self.transactions.get_mut(&txn_id).ok_or_else(no_active_transaction)?.rollback_lsn = + Some(rollback_lsn); log.flush_through(rollback_lsn)?; self.transactions.remove(&txn_id); Ok(()) @@ -659,38 +432,6 @@ impl TransactionManager { } Err(no_active_transaction()) } - - #[cfg(test)] - pub(crate) fn force_next_lsn_exhausted_for_test(&mut self) -> bool { - let Some(active) = self.transactions.values_mut().next() else { - return false; - }; - active.last_lsn = Lsn::MAX; - true - } -} - -fn next_lsn(current_lsn: Lsn) -> StorageResult { - current_lsn.checked_add(1).ok_or_else(|| LogManagerError::LsnExhausted.into()) -} - -fn pending_log_record(txn_id: TxnId, record: &PendingLogRecord) -> LogRecord<'_> { - LogRecord { - txn_id, - kind: match &record.kind { - PendingLogRecordKind::Begin => LogRecordKind::Begin, - PendingLogRecordKind::PageUpdate { page_id, redo_data, undo_data } => { - LogRecordKind::PageUpdate { - page_id: *page_id, - redo_data: redo_data.as_ref(), - undo_data: undo_data.as_ref(), - } - } - PendingLogRecordKind::PageAlloc { page_id } => { - LogRecordKind::PageAlloc { page_id: *page_id } - } - }, - } } /// Stamps the assigned page LSN into page formats that carry one. @@ -733,25 +474,26 @@ mod tests { #[test] fn page_alloc_without_active_transaction_does_not_write_wal() { let file = NamedTempFile::new().unwrap(); - let _log = LogManager::new(file.path()).unwrap(); + let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); - let lsn = transactions.record_page_alloc(0, 7).unwrap(); + let lsn = transactions.record_page_alloc(&mut log, 0, 7).unwrap(); assert_eq!(lsn, None); assert_eq!(read_log_record_kinds_for_test(file.path()), []); } #[test] - fn page_alloc_with_active_transaction_buffers_wal_until_commit() { + fn page_alloc_with_active_transaction_appends_wal_immediately() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); let txn_id = transactions.begin(&mut log).unwrap(); - let alloc_lsn = transactions.record_page_alloc(txn_id, 7).unwrap(); + let alloc_lsn = transactions.record_page_alloc(&mut log, txn_id, 7).unwrap(); - assert_eq!(read_log_record_kinds_for_test(file.path()), []); + assert_eq!(log.highest_appended_lsn(), Some(2)); + assert_eq!(log.highest_durable_lsn(), None); transactions.commit(&mut log, txn_id).unwrap(); assert_eq!(txn_id, 1); @@ -769,7 +511,7 @@ mod tests { } #[test] - fn repeated_page_updates_commit_as_one_page_update_record() { + fn repeated_page_updates_append_separate_wal_records() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -779,25 +521,27 @@ mod tests { let txn_id = transactions.begin(&mut log).unwrap(); let first_update = - transactions.record_page_update(txn_id, 7, &before, &after_first).unwrap(); - let second_update = - transactions.record_page_update(txn_id, 7, &after_first, &after_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after_first).unwrap(); + let second_update = transactions + .record_page_update(&mut log, txn_id, 7, &after_first, &after_second) + .unwrap(); transactions.commit(&mut log, txn_id).unwrap(); assert_eq!(first_update.as_ref().map(|update| update.lsn), Some(2)); - assert_eq!(second_update.as_ref().map(|update| update.lsn), Some(2)); + assert_eq!(second_update.as_ref().map(|update| update.lsn), Some(3)); assert_eq!( read_log_record_kinds_for_test(file.path()), [ (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), (txn_id, OwnedLogRecordKind::Commit), ] ); } #[test] - fn coalesced_page_update_keeps_first_undo_and_latest_redo_image() { + fn eager_page_updates_keep_each_undo_and_redo_image() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -806,23 +550,36 @@ mod tests { let after_second = [2; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after_first).unwrap(); - transactions.record_page_update(txn_id, 7, &after_first, &after_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after_first).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &after_first, &after_second).unwrap(); transactions.commit(&mut log, txn_id).unwrap(); let scan = read_recovery_log(file.path()).unwrap(); - match &scan.records[1].kind { - RecoveryLogRecordKind::PageUpdate { page_id, redo_data, undo_data } => { - assert_eq!(*page_id, 7); - assert_eq!(undo_data.as_ref(), &before); - assert_eq!(redo_data.as_ref(), &after_second); + match (&scan.records[1].kind, &scan.records[2].kind) { + ( + RecoveryLogRecordKind::PageUpdate { + page_id: first_page_id, + redo_data: first_redo, + undo_data: first_undo, + }, + RecoveryLogRecordKind::PageUpdate { + page_id: second_page_id, + redo_data: second_redo, + undo_data: second_undo, + }, + ) => { + assert_eq!((*first_page_id, *second_page_id), (7, 7)); + assert_eq!(first_undo.as_ref(), &before); + assert_eq!(first_redo.as_ref(), &after_first); + assert_eq!(second_undo.as_ref(), &after_first); + assert_eq!(second_redo.as_ref(), &after_second); } - kind => panic!("unexpected record kind: {kind:?}"), + kinds => panic!("unexpected record kinds: {kinds:?}"), } } #[test] - fn mixed_page_updates_coalesce_per_page_without_reordering() { + fn mixed_page_updates_append_in_mutation_order() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -833,9 +590,11 @@ mod tests { let after_a_second = [2; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before_a, &after_a_first).unwrap(); - transactions.record_page_update(txn_id, 8, &before_b, &after_b).unwrap(); - transactions.record_page_update(txn_id, 7, &after_a_first, &after_a_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before_a, &after_a_first).unwrap(); + transactions.record_page_update(&mut log, txn_id, 8, &before_b, &after_b).unwrap(); + transactions + .record_page_update(&mut log, txn_id, 7, &after_a_first, &after_a_second) + .unwrap(); transactions.commit(&mut log, txn_id).unwrap(); assert_eq!( @@ -844,13 +603,14 @@ mod tests { (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 8 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), (txn_id, OwnedLogRecordKind::Commit), ] ); } #[test] - fn page_update_after_append_creates_new_record_for_same_page() { + fn page_updates_advance_highest_appended_lsn_before_commit() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -859,9 +619,11 @@ mod tests { let after_second = [2; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after_first).unwrap(); - transactions.append_pending_through(txn_id, &mut log, 2).unwrap(); - transactions.record_page_update(txn_id, 7, &after_first, &after_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after_first).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &after_first, &after_second).unwrap(); + + assert_eq!(log.highest_appended_lsn(), Some(3)); + assert_eq!(log.highest_durable_lsn(), None); transactions.commit(&mut log, txn_id).unwrap(); assert_eq!( @@ -876,7 +638,7 @@ mod tests { } #[test] - fn savepoint_rollback_buffers_compensation_record_until_commit() { + fn savepoint_rollback_appends_compensation_record_immediately() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -885,17 +647,17 @@ mod tests { let after_second = [2; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after_first).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after_first).unwrap(); let savepoint = transactions.statement_savepoint(txn_id).unwrap(); - transactions.record_page_update(txn_id, 7, &after_first, &after_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &after_first, &after_second).unwrap(); - let restore_pages = transactions.rollback_to_savepoint(savepoint).unwrap(); + let restore_pages = transactions.rollback_to_savepoint(&mut log, savepoint).unwrap(); transactions.complete_savepoint_rollback(savepoint).unwrap(); assert_eq!(restore_pages.len(), 1); assert_eq!(restore_pages[0].page_id, 7); - assert_eq!(restore_pages[0].wal_flush_lsn, 3); - assert_eq!(read_log_record_kinds_for_test(file.path()), []); + assert_eq!(restore_pages[0].wal_flush_lsn, 4); + assert_eq!(log.highest_appended_lsn(), Some(4)); transactions.commit(&mut log, txn_id).unwrap(); @@ -905,6 +667,7 @@ mod tests { (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), (txn_id, OwnedLogRecordKind::Commit), ] ); @@ -918,7 +681,7 @@ mod tests { let txn_id = transactions.begin(&mut log).unwrap(); let savepoint = TransactionSavepoint { txn_id, undo_len: 1 }; - let result = transactions.rollback_to_savepoint(savepoint); + let result = transactions.rollback_to_savepoint(&mut log, savepoint); assert!(matches!( result, @@ -944,8 +707,7 @@ mod tests { let after = [1; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after).unwrap(); - transactions.append_pending_through(txn_id, &mut log, 2).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after).unwrap(); log.fail_next_flush_for_test(); assert!(transactions.finish_rollback(&mut log, txn_id).is_err()); @@ -963,11 +725,9 @@ mod tests { ); } - // Issue: Retaining a transaction after its rollback flush failed left lower-LSN - // pending records appendable after the rollback marker. A later page flush could - // append one at a new LSN and poison the transaction with an LSN mismatch. + // After an outcome append succeeds, a flush retry must not append it again. #[test] - fn pending_records_are_not_appended_while_rollback_flush_is_pending() { + fn rollback_flush_retry_does_not_append_another_outcome() { let file = NamedTempFile::new().unwrap(); let mut log = LogManager::new(file.path()).unwrap(); let mut transactions = TransactionManager::new(0); @@ -976,13 +736,11 @@ mod tests { let after_second = [2; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after_first).unwrap(); - transactions.append_pending_through(txn_id, &mut log, 2).unwrap(); - transactions.record_page_update(txn_id, 8, &before, &after_second).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after_first).unwrap(); + transactions.record_page_update(&mut log, txn_id, 8, &before, &after_second).unwrap(); log.fail_next_flush_for_test(); assert!(transactions.finish_rollback(&mut log, txn_id).is_err()); - transactions.append_pending_through(txn_id, &mut log, 3).unwrap(); transactions.finish_rollback(&mut log, txn_id).unwrap(); assert_eq!( @@ -990,14 +748,48 @@ mod tests { [ (txn_id, OwnedLogRecordKind::Begin), (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 7 }), + (txn_id, OwnedLogRecordKind::PageUpdate { page_id: 8 }), (txn_id, OwnedLogRecordKind::Rollback), ] ); } - // Issue: A transaction that rolled back before any WAL record was appended left - // no durable trace of its assigned ID. Reopening seeded the manager from the WAL, - // so the next transaction reused that ID instead of remaining monotonic. + #[test] + fn interleaved_transactions_receive_lsns_in_append_order() { + let file = NamedTempFile::new().unwrap(); + let mut log = LogManager::new(file.path()).unwrap(); + let mut transactions = TransactionManager::new(0); + let before = [0; PAGE_SIZE]; + let after_a = [1; PAGE_SIZE]; + let after_b = [2; PAGE_SIZE]; + + let txn_a = transactions.begin(&mut log).unwrap(); + let txn_b = transactions.begin(&mut log).unwrap(); + let update_a = + transactions.record_page_update(&mut log, txn_a, 7, &before, &after_a).unwrap(); + let update_b = + transactions.record_page_update(&mut log, txn_b, 8, &before, &after_b).unwrap(); + + transactions.commit(&mut log, txn_b).unwrap(); + transactions.commit(&mut log, txn_a).unwrap(); + + assert_eq!(update_a.map(|update| update.lsn), Some(3)); + assert_eq!(update_b.map(|update| update.lsn), Some(4)); + assert_eq!( + read_log_record_kinds_for_test(file.path()), + [ + (txn_a, OwnedLogRecordKind::Begin), + (txn_b, OwnedLogRecordKind::Begin), + (txn_a, OwnedLogRecordKind::PageUpdate { page_id: 7 }), + (txn_b, OwnedLogRecordKind::PageUpdate { page_id: 8 }), + (txn_b, OwnedLogRecordKind::Commit), + (txn_a, OwnedLogRecordKind::Commit), + ] + ); + } + + // A rolled-back transaction remains observable in the WAL so reopening can + // keep transaction IDs monotonic. #[test] fn clean_rollback_preserves_transaction_id_across_reopen() { let file = NamedTempFile::new().unwrap(); @@ -1028,7 +820,7 @@ mod tests { let after = [1; PAGE_SIZE]; let txn_id = transactions.begin(&mut log).unwrap(); - transactions.record_page_update(txn_id, 7, &before, &after).unwrap(); + transactions.record_page_update(&mut log, txn_id, 7, &before, &after).unwrap(); log.fail_next_flush_for_test(); let result = transactions.commit(&mut log, txn_id); diff --git a/src/storage/transaction_manager/fault_injection.rs b/src/storage/transaction_manager/fault_injection.rs index 69fe9f8..d707163 100644 --- a/src/storage/transaction_manager/fault_injection.rs +++ b/src/storage/transaction_manager/fault_injection.rs @@ -2,7 +2,7 @@ use std::ops::{Deref, DerefMut}; use crate::core::error::StorageResult; -use super::{PageRestore, TransactionManager, TransactionSavepoint}; +use super::{LogManager, PageRestore, TransactionManager, TransactionSavepoint}; /// Transaction-manager decorator that injects one-shot rollback failures. #[derive(Debug)] @@ -22,12 +22,13 @@ impl FaultInjectingTransactionManager { pub(crate) fn rollback_to_savepoint( &mut self, + log: &mut LogManager, savepoint: TransactionSavepoint, ) -> StorageResult> { if std::mem::take(&mut self.fail_next_savepoint_rollback) { - self.inner.force_next_lsn_exhausted_for_test(); + log.force_next_lsn_exhausted_for_test(); } - self.inner.rollback_to_savepoint(savepoint) + self.inner.rollback_to_savepoint(log, savepoint) } } diff --git a/src/sync.rs b/src/sync.rs new file mode 100644 index 0000000..df5a082 --- /dev/null +++ b/src/sync.rs @@ -0,0 +1,16 @@ +//! Synchronization primitives shared by production code and Loom models. +//! +//! Loom mirrors the standard library synchronization API, so keeping the +//! substitution here lets the concurrent implementation use one code path. + +#[cfg(all(test, loom))] +pub(crate) use loom::sync::{ + Arc, Condvar, Mutex, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard, + atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}, +}; + +#[cfg(not(all(test, loom)))] +pub(crate) use std::sync::{ + Arc, Condvar, Mutex, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard, + atomic::{AtomicBool, AtomicU32, AtomicU64, Ordering}, +}; From 9818ec9684a517ad3c10192d78cad61401187fb3 Mon Sep 17 00:00:00 2001 From: writemorecode Date: Sat, 5 Sep 2026 22:55:41 +0200 Subject: [PATCH 2/2] Exercise Loom models in CI --- .github/workflows/rust.yml | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 2e912e0..6840562 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -35,6 +35,9 @@ jobs: - name: Clippy run: cargo clippy --no-deps --all-targets --all-features + - name: Clippy (Loom configuration) + run: RUSTFLAGS="--cfg loom" cargo clippy --no-deps --lib --tests + - name: Documentation run: cargo doc --no-deps --all-features @@ -61,7 +64,10 @@ jobs: uses: taiki-e/install-action@nextest - name: Test - run: cargo nextest run --all-features + run: cargo nextest run + + - name: Loom tests + run: RUSTFLAGS="--cfg loom" cargo test --lib loom_tests -- --test-threads=1 - name: Documentation tests - run: cargo test --doc --all-features + run: cargo test --doc