diff --git a/Cargo.toml b/Cargo.toml index 59b8c5f4..7b274bd9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,10 @@ categories = ["database-implementations", "data-structures", "caching"] default = ["wti-predictable-search"] perf_measurements = ["dep:performance_measurement", "dep:performance_measurement_codegen"] s3-support = ["dep:rusty-s3", "dep:url", "dep:reqwest", "dep:walkdir", "worktable_codegen/s3-support"] +# Moves unique WorkTablesIndex structural CDC work out of the table mutation +# path and into the background persistence worker. The persisted page format +# is unchanged, so stores remain readable with or without this feature. +logical-index-persistence = ["worktable_codegen/logical-index-persistence"] wti-hybrid-search = ["indexset/wt-slice-binary-search"] wti-predictable-search = ["indexset/custom-binary-search"] wti-std-search = ["indexset/std-binary-search"] @@ -54,6 +58,7 @@ prettytable-rs = "^0.10" psc-nanoid = { version = "3.1.1", features = ["rkyv", "packed"] } rkyv = { version = "0.8.17", features = ["uuid-1"] } reqwest = { version = "0.12", optional = true, default-features = false, features = ["rustls-tls-webpki-roots", "charset", "http2"] } +rustc-hash = "2.1.1" rusty-s3 = { version = "0.10.2", optional = true } smart-default = "0.7.1" tokio = { version = "1", features = ["full"] } diff --git a/README.md b/README.md index b70f5c67..da2cf0a8 100644 --- a/README.md +++ b/README.md @@ -40,8 +40,10 @@ cargo add worktable@1.0.0-beta.4 > not mean the change is on stable storage. `wait_for_ops()` and `close()` flush the > persistence pipeline, but the current disk format has no transaction journal and > does not `fsync` every batch. Process or power loss can therefore lose acknowledged -> changes. A torn store is refused with `PersistenceLoadError` rather than opened as -> plausible-but-invented rows. See the [durability and recovery contract](docs/persistence-durability.md). +> changes. Normal loads refuse a torn store with `PersistenceLoadError` rather than +> opening plausible-but-invented rows. A deliberately explicit recovery mode can read +> individually validated rows from a private scratch copy through a surviving index; +> see the [durability and recovery contract](docs/persistence-durability.md). Persistence is implemented, not planned. `PersistedWorkTable` and `PersistenceConfig` are exported from the crate root; the prelude carries `DiskPersistenceEngine`, @@ -96,7 +98,10 @@ need an external snapshot/rebuild strategy. A graceful persistence error is terminal and surfaced consistently, but abrupt termination can currently leave a partial multi-file batch. Loading audits archived rows plus primary and secondary index consistency before exposing the table; torn state is refused as -`PersistenceLoadError` and must be restored or rebuilt as documented above. +`PersistenceLoadError`. Offline recovery tools may opt into `LoadMode::Recovery` to +copy individually validated rows through a surviving index into a clean table, which +must then pass a normal strict load before publication. This mode is not an in-place +repair and must never serve live traffic. Generated persisted tables store row-schema, primary-key, and secondary-index metadata in `SpaceInfo`. Existing legacy files whose schema metadata is diff --git a/codegen/Cargo.toml b/codegen/Cargo.toml index c7a55629..3871b643 100644 --- a/codegen/Cargo.toml +++ b/codegen/Cargo.toml @@ -8,6 +8,7 @@ repository = "https://github.com/pathscale/WorkTable" [features] s3-support = [] +logical-index-persistence = [] # Compatibility no-op retained for downstream manifests. versioned-row-publication = [] diff --git a/codegen/src/generators/in_memory/queries/select.rs b/codegen/src/generators/in_memory/queries/select.rs index 489758e3..42139128 100644 --- a/codegen/src/generators/in_memory/queries/select.rs +++ b/codegen/src/generators/in_memory/queries/select.rs @@ -35,10 +35,29 @@ impl InMemoryGenerator { let iter = std::iter::once_with(move || { let read_guard = self.0.data.read_guard(); self.0.primary_index.pk_map - .iter_links() - .filter_map(move |link| { + .iter_values() + .filter_map(move |(primary_key, link)| { let _read_guard = &read_guard; - self.0.data.select_non_ghosted(link.0).ok() + let mut current_link = link.0; + for _ in 0..64 { + if let Ok(row) = self.0.data.select_non_ghosted(current_link) { + return Some(row); + } + + // A reinsert publishes the replacement link + // before retiring the captured one. Follow that + // replacement instead of silently omitting the + // row from a concurrent full-table scan. + let replacement: Link = self.0.primary_index.pk_map + .lookup_for_select(&primary_key) + .map(Into::into)?; + if replacement == current_link { + return None; + } + current_link = replacement; + std::hint::spin_loop(); + } + None }) }).flatten(); diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 911b7147..949f9dc7 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -55,29 +55,93 @@ impl InMemoryGenerator { let persist_call = self.gen_persist_call(); let persist_op = self.gen_persist_op(); let full_row_lock = self.gen_full_lock_for_update(); - let size_check = if self.columns.is_sized { - quote! {} - } else { + // A full-row `update(row)` replaces EVERY column, so it inherently + // rewrites every secondary index. The in-place fast path only applies + // when no updated field is indexed (it emits no index diff), so a + // full-row update on a table with any secondary index must reinsert. + // Only a table with NO secondary indexes and an unsized column can use + // the in-place same-size path here. (The custom single-column updates — + // gen_unique_update / gen_pk_update — get the fast path via + // gen_size_check; gen_non_unique_update updates a non-unique-indexed + // column and therefore always reinserts, correctly.) + let const_name = name_generator.get_page_inner_size_const_ident(); + let full_row_in_place_eligible = !self.columns.is_sized && self.columns.indexes.is_empty(); + let update_body = if self.columns.is_sized { quote! { - if true { - drop(_guard); - let op_lock = { #full_row_lock }; - let _guard = LockGuard::new_with_mutation( - op_lock, - self.0.lock_manager.clone(), - pk.clone(), - ); - let row_old = self.0.data.select_non_ghosted(link)?; - if let Err(e) = self.reinsert(row_old, row).await { - self.0.update_state.remove(&pk); + let mut bytes = rkyv::to_bytes::(&row) + .map_err(|_| WorkTableError::SerializeError)?; + let mut archived_row = unsafe { + rkyv::access_unchecked_mut::<<#row_ident as rkyv::Archive>::Archived>(&mut bytes[..]) + .unseal_unchecked() + }; - return Err(e); - } + let op_id = OperationId::Single(uuid::Uuid::now_v7()); + #diff_process_insert + #persist_op - self.0.update_state.remove(&pk); + unsafe { + self.0 + .data + .with_mut_ref(link, move |archived| { + #(#row_updates)* + }) + .map_err(WorkTableError::PagesError)? + }; + + #diff_process_remove + self.0.update_state.remove(&pk); + + #persist_call + + core::result::Result::Ok(()) + } + } else if full_row_in_place_eligible { + quote! { + // No secondary indexes: same-size unsized full-row update may go + // in place at the current slot. `update_in_place` re-validates + // the serialized length and we fall back to reinsert otherwise. + let in_place_ok = unsafe { + self.0.data.update_in_place::<{ #const_name }>(row.clone(), link).is_ok() + }; + if in_place_ok { + self.0.update_state.remove(&pk); return core::result::Result::Ok(()); } + drop(_guard); + let op_lock = { #full_row_lock }; + let _guard = LockGuard::new_with_mutation( + op_lock, + self.0.lock_manager.clone(), + pk.clone(), + ); + let row_old = self.0.data.select_non_ghosted(link)?; + if let Err(e) = self.reinsert(row_old, row).await { + self.0.update_state.remove(&pk); + return Err(e); + } + self.0.update_state.remove(&pk); + core::result::Result::Ok(()) + } + } else { + quote! { + drop(_guard); + let op_lock = { #full_row_lock }; + let _guard = LockGuard::new_with_mutation( + op_lock, + self.0.lock_manager.clone(), + pk.clone(), + ); + let row_old = self.0.data.select_non_ghosted(link)?; + if let Err(e) = self.reinsert(row_old, row).await { + self.0.update_state.remove(&pk); + + return Err(e); + } + + self.0.update_state.remove(&pk); + + core::result::Result::Ok(()) } }; @@ -112,26 +176,7 @@ impl InMemoryGenerator { let row_old = self.0.data.select_non_ghosted(link)?; self.0.update_state.insert(pk.clone(), row_old); - let mut bytes = rkyv::to_bytes::(&row).map_err(|_| WorkTableError::SerializeError)?; - #size_check - - let mut archived_row = unsafe { rkyv::access_unchecked_mut::<<#row_ident as rkyv::Archive>::Archived>(&mut bytes[..]).unseal_unchecked() }; - - let op_id = OperationId::Single(uuid::Uuid::now_v7()); - #diff_process_insert - #persist_op - - unsafe { self.0.data.with_mut_ref(link, move |archived| { - #(#row_updates)* - }).map_err(WorkTableError::PagesError)? }; - - #diff_process_remove - - self.0.update_state.remove(&pk); - - #persist_call - - core::result::Result::Ok(()) + #update_body } } } @@ -220,8 +265,20 @@ impl InMemoryGenerator { } } - fn gen_size_check(&self, unsized_fields: Option>, idents: &[Ident]) -> TokenStream { - if let Some(f) = unsized_fields { + fn gen_size_check( + &self, + unsized_fields: Option>, + idents: &[Ident], + idx_idents: Option<&Vec>, + ) -> TokenStream { + // The in-place fast path re-serializes the row directly into its slot and + // republishes it, bypassing the generated secondary-index diff. That is + // only safe when NONE of the updated columns are indexed; an update that + // touches an indexed column must keep the index-maintaining reinsert + // path (and its unique-constraint check). Fall back to always-reinsert in + // that case. + let touches_index = idx_idents.map(|v| !v.is_empty()).unwrap_or(false); + if let (Some(f), false) = (unsized_fields, touches_index) { let fields_check: Vec<_> = f .iter() .map(|f| { @@ -235,16 +292,30 @@ impl InMemoryGenerator { .iter() .map(|i| { quote! { - row_new.#i = row.#i; + row_new.#i = row.#i.clone(); } }) .collect::>(); let full_row_lock = self.gen_full_lock_for_update(); + let name_generator = WorktableNameGenerator::from_table_name(self.name.to_string()); + let const_name = name_generator.get_page_inner_size_const_ident(); quote! { - let mut need_to_reinsert = true; + // Reinsert ONLY when an unsized field's serialized size CHANGED, + // so the row no longer fits its slot region. A same-size update + // (the common case) is written in place at the SAME slot below. + // `need_to_reinsert` starts false; the old `true` initializer + // forced every unsized update through a full delete-and-reinsert. + let mut need_to_reinsert = false; #(#fields_check)* - if need_to_reinsert { + + { + // Serialize the whole read-modify-write against other + // updates of this key by holding the full-row lock (the + // reinsert path does the same). The original query lock only + // covers this query's columns, so two different-column + // updates could otherwise each rebuild the row from a stale + // snapshot and lose each other's write. drop(_guard); let op_lock = { #full_row_lock }; let _guard = LockGuard::new_with_mutation( @@ -253,20 +324,102 @@ impl InMemoryGenerator { pk.clone(), ); + // Re-read the current row UNDER the full-row lock so the + // rebuilt row reflects any committed concurrent update. let row_old = self.0.select(pk.clone()).expect("should not be deleted by other thread"); let mut row_new = row_old.clone(); #(#row_updates)* + + if need_to_reinsert { + if let Err(e) = self.reinsert(row_old, row_new).await { + self.0.update_state.remove(&pk); + + return Err(e); + } + + self.0.update_state.remove(&pk); + return core::result::Result::Ok(()); + } + + // Same-size in-place write at the CURRENT slot. Re-serialize + // the full rebuilt row (only the changed fields differ) and + // overwrite the slot's bytes so any out-of-line `String` + // field's archived pointer resolves within the slot (NOT a + // `mem::swap`, which would dangle the pointer), then republish + // the row as live. + // Re-resolve the link under the held full-row lock. The + // earlier size check (#fields_check) read field sizes from the + // link captured before the lock, which a concurrent reinsert + // could have moved. CORRECTNESS DOES NOT rely on that size + // decision being current: `update_in_place` re-validates that + // the serialized row is EXACTLY the slot length and returns + // Err on mismatch, and we fall back to a full reinsert below. + // Do not remove that length re-check or this fallback. + let current_link: Link = self.0 + .primary_index + .pk_map + .get_value(&pk) + .map(Into::into) + .ok_or(WorkTableError::NotFound)?; + // Equal field sizes do not guarantee an equal TOTAL serialized + // length (alignment), so a same-slot write may not fit — fall + // back to a full reinsert rather than fail. Correctness-first. + // The clone is paid only so `row_new` survives for that rare + // reinsert fallback; the common in-place path drops it. + let in_place_ok = unsafe { + self.0.data.update_in_place::<{ #const_name }>(row_new.clone(), current_link).is_ok() + }; + if in_place_ok { + self.0.update_state.remove(&pk); + return core::result::Result::Ok(()); + } + + // `update_in_place` checks serialization and exact slot + // length before touching page bytes, so its error path + // leaves this locked snapshot authoritative for fallback. if let Err(e) = self.reinsert(row_old, row_new).await { self.0.update_state.remove(&pk); + return Err(e); + } + self.0.update_state.remove(&pk); + return core::result::Result::Ok(()); + } + } + } else if self.columns.is_sized { + // Sized rows never change length; the caller's field swap is safe and + // no size check / reinsert is needed. + quote! {} + } else { + // Unsized rows where an updated column IS indexed: keep the original + // always-reinsert path so secondary-index maintenance and unique + // checks run through reinsert. + let row_updates = idents + .iter() + .map(|i| quote! { row_new.#i = row.#i.clone(); }) + .collect::>(); + let full_row_lock = self.gen_full_lock_for_update(); + quote! { + { + drop(_guard); + let op_lock = { #full_row_lock }; + let _guard = LockGuard::new_with_mutation( + op_lock, + self.0.lock_manager.clone(), + pk.clone(), + ); + let row_old = self.0.select(pk.clone()).expect("should not be deleted by other thread"); + let mut row_new = row_old.clone(); + #(#row_updates)* + if let Err(e) = self.reinsert(row_old, row_new).await { + self.0.update_state.remove(&pk); return Err(e); } + self.0.update_state.remove(&pk); return core::result::Result::Ok(()); } } - } else { - quote! {} } } @@ -452,13 +605,32 @@ impl InMemoryGenerator { }) .collect::>(); - let size_check = self.gen_size_check(unsized_fields, idents); + let size_check = self.gen_size_check(unsized_fields, idents, idx_idents); let diff_process_insert = self.gen_process_diffs_insert_on_index(idents, idx_idents); let diff_process_remove = self.gen_process_diffs_remove_on_index(idx_idents); let persist_call = self.gen_persist_call(); let persist_op = self.gen_persist_op(); let custom_lock = self.gen_custom_lock_for_update(lock_ident); + let finish_update = if self.columns.is_sized { + quote! { + #diff_process_insert + #persist_op + + unsafe { self.0.data.with_mut_ref(link, |archived| { + #(#row_updates)* + }).map_err(WorkTableError::PagesError)? }; + + #diff_process_remove + + #persist_call + + core::result::Result::Ok(()) + } + } else { + quote! {} + }; + quote! { pub async fn #method_ident(&self, row: #query_ident, pk: Pk) -> core::result::Result<(), WorkTableError> where #pk_ident: From @@ -483,18 +655,7 @@ impl InMemoryGenerator { let op_id = OperationId::Single(uuid::Uuid::now_v7()); #size_check - #diff_process_insert - #persist_op - - unsafe { self.0.data.with_mut_ref(link, |archived| { - #(#row_updates)* - }).map_err(WorkTableError::PagesError)? }; - - #diff_process_remove - - #persist_call - - core::result::Result::Ok(()) + #finish_update } } } @@ -692,7 +853,7 @@ impl InMemoryGenerator { } }) .collect::>(); - let size_check = self.gen_size_check(unsized_fields, idents); + let size_check = self.gen_size_check(unsized_fields, idents, idx_idents); let diff_process_insert = self.gen_process_diffs_insert_on_index(idents, idx_idents); let diff_process_remove = self.gen_process_diffs_remove_on_index(idx_idents); let persist_call = self.gen_persist_call(); @@ -708,6 +869,27 @@ impl InMemoryGenerator { }; let custom_lock = self.gen_custom_lock_for_update(lock_ident); + let finish_update = if self.columns.is_sized { + quote! { + #diff_process_insert + #persist_op + + unsafe { + self.0.data.with_mut_ref(link, |archived| { + #(#row_updates)* + }).map_err(WorkTableError::PagesError)?; + } + + #diff_process_remove + + #persist_call + + core::result::Result::Ok(()) + } + } else { + quote! {} + }; + quote! { pub async fn #method_ident(&self, row: #query_ident, by: #by_ident) -> core::result::Result<(), WorkTableError> { let mut bytes = rkyv::to_bytes::(&row) @@ -751,20 +933,7 @@ impl InMemoryGenerator { let op_id = OperationId::Single(uuid::Uuid::now_v7()); #size_check - #diff_process_insert - #persist_op - - unsafe { - self.0.data.with_mut_ref(link, |archived| { - #(#row_updates)* - }).map_err(WorkTableError::PagesError)?; - } - - #diff_process_remove - - #persist_call - - core::result::Result::Ok(()) + #finish_update } } } diff --git a/codegen/src/generators/index_backend.rs b/codegen/src/generators/index_backend.rs index ca923da6..defcaf68 100644 --- a/codegen/src/generators/index_backend.rs +++ b/codegen/src/generators/index_backend.rs @@ -39,7 +39,16 @@ pub(crate) fn persistent_unique_index_type( value: &TokenStream, worktables_node: Option, ) -> syn::Result { + // This intentionally evaluates the proc-macro crate's feature. WorkTable's + // public feature forwards to worktable_codegen in Cargo.toml, so the + // runtime types and emitted types are selected together. Emitting a cfg in + // the expansion would instead test the consuming package's unrelated + // feature namespace, which may rename or omit the dependency feature. match backend { + IndexBackend::WorktablesIndex if cfg!(feature = "logical-index-persistence") => Ok(match worktables_node { + Some(node) => quote! { PersistentWtiIndex<#key, #value, #node> }, + None => quote! { PersistentWtiIndex<#key, #value> }, + }), IndexBackend::Congee => Ok(quote! { PersistentCongeeIndex<#key, #value> }), IndexBackend::Arctic => Ok(quote! { PersistentArcticIndex<#key, #value> }), _ => unique_index_type(backend, key, value, worktables_node), diff --git a/codegen/src/generators/persist/index/mod.rs b/codegen/src/generators/persist/index/mod.rs index b2068c62..476fe724 100644 --- a/codegen/src/generators/persist/index/mod.rs +++ b/codegen/src/generators/persist/index/mod.rs @@ -109,11 +109,16 @@ impl PersistGenerator { let res = if idx.is_unique { match idx.backend { crate::common::model::IndexBackend::WorktablesIndex => { + let map = if cfg!(feature = "logical-index-persistence") { + quote! { PersistentWtiIndex } + } else { + quote! { IndexMap } + }; if is_unsized(&t.to_string()) { - quote! { #i: IndexMap::with_maximum_node_size(#const_name), } + quote! { #i: #map::with_maximum_node_size(#const_name), } } else { quote! { - #i: IndexMap::with_maximum_node_size( + #i: #map::with_maximum_node_size( get_index_page_size_from_data_length::<#t>(#const_name) ), } diff --git a/codegen/src/generators/persist/queries/select.rs b/codegen/src/generators/persist/queries/select.rs index 64247950..7627a6fb 100644 --- a/codegen/src/generators/persist/queries/select.rs +++ b/codegen/src/generators/persist/queries/select.rs @@ -35,10 +35,29 @@ impl PersistGenerator { let iter = std::iter::once_with(move || { let read_guard = self.0.data.read_guard(); self.0.primary_index.pk_map - .iter_links() - .filter_map(move |link| { + .iter_values() + .filter_map(move |(primary_key, link)| { let _read_guard = &read_guard; - self.0.data.select_non_ghosted(link.0).ok() + let mut current_link = link.0; + for _ in 0..64 { + if let Ok(row) = self.0.data.select_non_ghosted(current_link) { + return Some(row); + } + + // A reinsert publishes the replacement link + // before retiring the captured one. Follow that + // replacement instead of silently omitting the + // row from a concurrent full-table scan. + let replacement: Link = self.0.primary_index.pk_map + .lookup_for_select(&primary_key) + .map(Into::into)?; + if replacement == current_link { + return None; + } + current_link = replacement; + std::hint::spin_loop(); + } + None }) }).flatten(); diff --git a/codegen/src/generators/persist/table/impls.rs b/codegen/src/generators/persist/table/impls.rs index 50658e70..02f8f823 100644 --- a/codegen/src/generators/persist/table/impls.rs +++ b/codegen/src/generators/persist/table/impls.rs @@ -53,7 +53,11 @@ impl PersistGenerator { fn gen_validate_loaded_secondary_state_fn(&self) -> TokenStream { if self.columns.indexes.is_empty() { return quote! { - fn validate_loaded_secondary_state(&self, _path: &str) -> Result<(), PersistenceLoadError> { + fn validate_loaded_secondary_state( + &self, + _path: &str, + _mode: LoadMode, + ) -> Result<(), PersistenceLoadError> { Ok(()) } }; @@ -120,19 +124,89 @@ impl PersistGenerator { } }) .collect::>(); + let recovery_entries = self + .columns + .indexes + .iter() + .map(|(column, index)| { + let index_field = &index.name; + let row_field = &index.field; + let index_name = Literal::string(&index_field.to_string()); + let field_type = self + .columns + .columns_map + .get(column) + .expect("indexed column should exist") + .to_string(); + let expected_key = if is_float(&field_type) { + quote! { OrderedFloat(row.#row_field) } + } else { + quote! { row.#row_field.clone() } + }; + + if index.is_unique { + quote! { + for (indexed_key, offset_link) in self.0.indexes.#index_field.iter_values() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("secondary index {} references an invalid row: {error}", #index_name), + ) + })?; + let expected_key = #expected_key; + if indexed_key != expected_key { + return Err(PersistenceLoadError::corrupt( + path, + format!("secondary index {} key does not match its referenced row", #index_name), + )); + } + } + } + } else { + quote! { + for (indexed_key, offset_link) in self.0.indexes.#index_field.iter() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("secondary index {} references an invalid row: {error}", #index_name), + ) + })?; + let expected_key = #expected_key; + if indexed_key != &expected_key { + return Err(PersistenceLoadError::corrupt( + path, + format!("secondary index {} key does not match its referenced row", #index_name), + )); + } + } + } + } + }) + .collect::>(); quote! { - fn validate_loaded_secondary_state(&self, path: &str) -> Result<(), PersistenceLoadError> { - let primary_count = self.0.primary_index.pk_map.len(); - #(#entry_counts)* - for (primary_key, offset_link) in self.0.primary_index.pk_map.iter_values() { - let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { - PersistenceLoadError::corrupt( - path, - format!("primary key {primary_key:?} references an invalid row: {error}"), - ) - })?; - #(#expected_entries)* + fn validate_loaded_secondary_state( + &self, + path: &str, + mode: LoadMode, + ) -> Result<(), PersistenceLoadError> { + match mode { + LoadMode::Strict => { + let primary_count = self.0.primary_index.pk_map.len(); + #(#entry_counts)* + for (primary_key, offset_link) in self.0.primary_index.pk_map.iter_values() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("primary key {primary_key:?} references an invalid row: {error}"), + ) + })?; + #(#expected_entries)* + } + } + LoadMode::Recovery => { + #(#recovery_entries)* + } } Ok(()) } @@ -163,10 +237,15 @@ impl PersistGenerator { }) .collect::>(); let pk_types_unsized = is_unsized_vec(pk_types); + let wti_map = if cfg!(feature = "logical-index-persistence") { + quote! { PersistentWtiIndex } + } else { + quote! { IndexMap } + }; let index_setup = if pk_types_unsized { quote! { inner.primary_index = std::sync::Arc::new(PrimaryIndex { - pk_map: IndexMap::<#pk_type, OffsetEqLink<#const_name>, UnsizedNode<_>>::with_maximum_node_size(#const_name), + pk_map: #wti_map::<#pk_type, OffsetEqLink<#const_name>, UnsizedNode<_>>::with_maximum_node_size(#const_name), reverse_pk_map: IndexMap::new(), }); } @@ -175,7 +254,7 @@ impl PersistGenerator { crate::common::model::IndexBackend::WorktablesIndex => quote! { let size = get_index_page_size_from_data_length::<#pk_type>(#const_name); inner.primary_index = std::sync::Arc::new(PrimaryIndex { - pk_map: IndexMap::<_, OffsetEqLink<#const_name>>::with_maximum_node_size(size), + pk_map: #wti_map::<_, OffsetEqLink<#const_name>>::with_maximum_node_size(size), reverse_pk_map: IndexMap::new(), }); }, @@ -232,7 +311,11 @@ impl PersistGenerator { )) } - async fn load(mut engine: E) -> eyre::Result { + async fn load(engine: E) -> eyre::Result { + Self::load_with(engine, LoadMode::Strict).await + } + + async fn load_with(mut engine: E, mode: LoadMode) -> eyre::Result { let schema = Self::space_info_default().inner; engine .validate_schema( @@ -247,7 +330,7 @@ impl PersistGenerator { }; let table = load_persisted_state(&table_path, async { let space = #space_ident::parse_file(&table_path).await?; - Ok::<_, eyre::Report>(space.into_worktable(engine, &table_path).await?) + Ok::<_, eyre::Report>(space.into_worktable_with_mode(engine, &table_path, mode).await?) }).await?; Ok(table) } diff --git a/codegen/src/generators/persist/table/mod.rs b/codegen/src/generators/persist/table/mod.rs index 272fcb1c..1d339ed3 100644 --- a/codegen/src/generators/persist/table/mod.rs +++ b/codegen/src/generators/persist/table/mod.rs @@ -89,6 +89,14 @@ impl PersistGenerator { #[derive(Debug, PersistTable)] #[table(pk_unsized, pk_upstream)] }, + (true, crate::common::model::IndexBackend::WorktablesIndex) + if cfg!(feature = "logical-index-persistence") => + { + quote! { + #[derive(Debug, PersistTable)] + #[table(pk_unsized, pk_wti_logical)] + } + } (true, _) => quote! { #[derive(Debug, PersistTable)] #[table(pk_unsized)] @@ -105,6 +113,14 @@ impl PersistGenerator { #[derive(Debug, PersistTable)] #[table(pk_congee)] }, + (false, crate::common::model::IndexBackend::WorktablesIndex) + if cfg!(feature = "logical-index-persistence") => + { + quote! { + #[derive(Debug, PersistTable)] + #[table(pk_wti_logical)] + } + } (false, crate::common::model::IndexBackend::WorktablesIndex) => quote! { #[derive(Debug, PersistTable)] }, diff --git a/codegen/src/generators/read_only/queries/select.rs b/codegen/src/generators/read_only/queries/select.rs index 11dd8309..0adcbe70 100644 --- a/codegen/src/generators/read_only/queries/select.rs +++ b/codegen/src/generators/read_only/queries/select.rs @@ -35,10 +35,29 @@ impl ReadOnlyGenerator { let iter = std::iter::once_with(move || { let read_guard = self.0.data.read_guard(); self.0.primary_index.pk_map - .iter_links() - .filter_map(move |link| { + .iter_values() + .filter_map(move |(primary_key, link)| { let _read_guard = &read_guard; - self.0.data.select_non_ghosted(link.0).ok() + let mut current_link = link.0; + for _ in 0..64 { + if let Ok(row) = self.0.data.select_non_ghosted(current_link) { + return Some(row); + } + + // A reinsert publishes the replacement link + // before retiring the captured one. Follow that + // replacement instead of silently omitting the + // row from a concurrent full-table scan. + let replacement: Link = self.0.primary_index.pk_map + .lookup_for_select(&primary_key) + .map(Into::into)?; + if replacement == current_link { + return None; + } + current_link = replacement; + std::hint::spin_loop(); + } + None }) }).flatten(); diff --git a/codegen/src/generators/read_only/table/impls.rs b/codegen/src/generators/read_only/table/impls.rs index 2542cff7..84330982 100644 --- a/codegen/src/generators/read_only/table/impls.rs +++ b/codegen/src/generators/read_only/table/impls.rs @@ -52,7 +52,11 @@ impl ReadOnlyGenerator { fn gen_validate_loaded_secondary_state_fn(&self) -> TokenStream { if self.columns.indexes.is_empty() { return quote! { - fn validate_loaded_secondary_state(&self, _path: &str) -> Result<(), PersistenceLoadError> { + fn validate_loaded_secondary_state( + &self, + _path: &str, + _mode: LoadMode, + ) -> Result<(), PersistenceLoadError> { Ok(()) } }; @@ -119,19 +123,89 @@ impl ReadOnlyGenerator { } }) .collect::>(); + let recovery_entries = self + .columns + .indexes + .iter() + .map(|(column, index)| { + let index_field = &index.name; + let row_field = &index.field; + let index_name = Literal::string(&index_field.to_string()); + let field_type = self + .columns + .columns_map + .get(column) + .expect("indexed column should exist") + .to_string(); + let expected_key = if is_float(&field_type) { + quote! { OrderedFloat(row.#row_field) } + } else { + quote! { row.#row_field.clone() } + }; + + if index.is_unique { + quote! { + for (indexed_key, offset_link) in self.0.indexes.#index_field.iter_values() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("secondary index {} references an invalid row: {error}", #index_name), + ) + })?; + let expected_key = #expected_key; + if indexed_key != expected_key { + return Err(PersistenceLoadError::corrupt( + path, + format!("secondary index {} key does not match its referenced row", #index_name), + )); + } + } + } + } else { + quote! { + for (indexed_key, offset_link) in self.0.indexes.#index_field.iter() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("secondary index {} references an invalid row: {error}", #index_name), + ) + })?; + let expected_key = #expected_key; + if indexed_key != &expected_key { + return Err(PersistenceLoadError::corrupt( + path, + format!("secondary index {} key does not match its referenced row", #index_name), + )); + } + } + } + } + }) + .collect::>(); quote! { - fn validate_loaded_secondary_state(&self, path: &str) -> Result<(), PersistenceLoadError> { - let primary_count = self.0.primary_index.pk_map.len(); - #(#entry_counts)* - for (primary_key, offset_link) in self.0.primary_index.pk_map.iter_values() { - let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { - PersistenceLoadError::corrupt( - path, - format!("primary key {primary_key:?} references an invalid row: {error}"), - ) - })?; - #(#expected_entries)* + fn validate_loaded_secondary_state( + &self, + path: &str, + mode: LoadMode, + ) -> Result<(), PersistenceLoadError> { + match mode { + LoadMode::Strict => { + let primary_count = self.0.primary_index.pk_map.len(); + #(#entry_counts)* + for (primary_key, offset_link) in self.0.primary_index.pk_map.iter_values() { + let row = self.0.data.select_non_ghosted_checked(offset_link.0).map_err(|error| { + PersistenceLoadError::corrupt( + path, + format!("primary key {primary_key:?} references an invalid row: {error}"), + ) + })?; + #(#expected_entries)* + } + } + LoadMode::Recovery => { + #(#recovery_entries)* + } } Ok(()) } @@ -204,13 +278,17 @@ impl ReadOnlyGenerator { } async fn load(engine: E) -> eyre::Result { + Self::load_with(engine, LoadMode::Strict).await + } + + async fn load_with(engine: E, mode: LoadMode) -> eyre::Result { let table_path = engine.config().table_path().to_owned(); if !std::path::Path::new(&table_path).exists() { return Self::new(engine).await; }; let table = load_persisted_state(&table_path, async { let space = #space_ident::parse_file(&table_path).await?; - Ok::<_, eyre::Report>(space.into_worktable(&table_path)?) + Ok::<_, eyre::Report>(space.into_worktable_with_mode(&table_path, mode)?) }).await?; Ok(table) } diff --git a/codegen/src/persist_index/generator.rs b/codegen/src/persist_index/generator.rs index 9af18581..9c536283 100644 --- a/codegen/src/persist_index/generator.rs +++ b/codegen/src/persist_index/generator.rs @@ -24,6 +24,7 @@ pub(super) struct IndexLayout { is_unique: bool, uses_upstream: bool, pub(super) art_backend: Option, + pub(super) logical_wti: bool, } #[derive(Copy, Clone, Debug, Eq, PartialEq)] @@ -46,12 +47,13 @@ pub(super) fn index_layout(field: &Field) -> syn::Result { .ok_or_else(|| syn::Error::new_spanned(&field.ty, "index type path cannot be empty"))? .ident .clone(); - let (is_unique, uses_upstream, art_backend) = match type_ident.to_string().as_str() { - "IndexMap" | "TreeIndex" => (true, false, None), - "UpstreamIndexMap" => (true, true, None), - "IndexMultiMap" | "TreeMultiIndex" => (false, false, None), - "PersistentArcticIndex" => (true, false, Some(ArtBackend::Arctic)), - "PersistentCongeeIndex" => (true, false, Some(ArtBackend::Congee)), + let (is_unique, uses_upstream, art_backend, logical_wti) = match type_ident.to_string().as_str() { + "IndexMap" | "TreeIndex" => (true, false, None, false), + "PersistentWtiIndex" => (true, false, None, true), + "UpstreamIndexMap" => (true, true, None, false), + "IndexMultiMap" | "TreeMultiIndex" => (false, false, None, false), + "PersistentArcticIndex" => (true, false, Some(ArtBackend::Arctic), false), + "PersistentCongeeIndex" => (true, false, Some(ArtBackend::Congee), false), _ => { return Err(syn::Error::new_spanned( &field.ty, @@ -64,6 +66,7 @@ pub(super) fn index_layout(field: &Field) -> syn::Result { is_unique, uses_upstream, art_backend, + logical_wti, }) } diff --git a/codegen/src/persist_index/space/index.rs b/codegen/src/persist_index/space/index.rs index d891d500..eb308ee3 100644 --- a/codegen/src/persist_index/space/index.rs +++ b/codegen/src/persist_index/space/index.rs @@ -25,6 +25,12 @@ impl Generator { Some(ArtBackend::Congee) => quote! { #i: SpaceCongeeIndex<#t, { #inner_const_name as u32}>, }, + None if layout.logical_wti && is_unsized(&t.to_string()) => quote! { + #i: SpaceLogicalIndexUnsized<#t, { #inner_const_name as u32}>, + }, + None if layout.logical_wti => quote! { + #i: SpaceLogicalIndex<#t, { #inner_const_name as u32}>, + }, None if is_unsized(&t.to_string()) => quote! { #i: SpaceIndexUnsized<#t, { #inner_const_name as u32}>, }, @@ -79,6 +85,12 @@ impl Generator { Some(ArtBackend::Congee) => quote! { #i: SpaceCongeeIndex::secondary_from_table_files_path(path, #literal_name, version).await?, }, + None if layout.logical_wti && is_unsized(&t.to_string()) => quote! { + #i: SpaceLogicalIndexUnsized::secondary_from_table_files_path(path, #literal_name, version).await?, + }, + None if layout.logical_wti => quote! { + #i: SpaceLogicalIndex::secondary_from_table_files_path(path, #literal_name, version).await?, + }, None if is_unsized(&t.to_string()) => quote! { #i: SpaceIndexUnsized::secondary_from_table_files_path(path, #literal_name, version).await?, }, diff --git a/codegen/src/persist_table/generator/mod.rs b/codegen/src/persist_table/generator/mod.rs index 7baf87b6..c93e091e 100644 --- a/codegen/src/persist_table/generator/mod.rs +++ b/codegen/src/persist_table/generator/mod.rs @@ -14,6 +14,7 @@ pub struct PersistTableAttributes { pub pk_upstream: bool, pub pk_arctic: bool, pub pk_congee: bool, + pub pk_wti_logical: bool, pub row_schema: Vec<(String, String)>, pub primary_key_fields: Vec, pub secondary_index_types: Vec<(String, String)>, diff --git a/codegen/src/persist_table/generator/space.rs b/codegen/src/persist_table/generator/space.rs index 1f81f397..dc2d279d 100644 --- a/codegen/src/persist_table/generator/space.rs +++ b/codegen/src/persist_table/generator/space.rs @@ -36,10 +36,18 @@ impl Generator { let space_secondary_indexes = name_generator.get_space_secondary_index_ident(); let space_secondary_indexes_events = name_generator.get_space_secondary_index_events_ident(); let avt_index_ident = name_generator.get_available_indexes_ident(); - let space_index_type = if self.attributes.pk_unsized { + let space_index_type = if self.attributes.pk_unsized && self.attributes.pk_wti_logical { + quote! { + SpaceLogicalIndexUnsized<#primary_key_type, { #inner_const_name as u32 }>, + } + } else if self.attributes.pk_unsized { quote! { SpaceIndexUnsized<#primary_key_type, { #inner_const_name as u32 }>, } + } else if self.attributes.pk_wti_logical { + quote! { + SpaceLogicalIndex<#primary_key_type, { #inner_const_name as u32 }>, + } } else if self.attributes.pk_arctic { quote! { SpaceArcticIndex<#primary_key_type, { #inner_const_name as u32 }>, diff --git a/codegen/src/persist_table/generator/space_file/mod.rs b/codegen/src/persist_table/generator/space_file/mod.rs index 8120e6eb..cef7ba3d 100644 --- a/codegen/src/persist_table/generator/space_file/mod.rs +++ b/codegen/src/persist_table/generator/space_file/mod.rs @@ -143,8 +143,13 @@ impl Generator { let primary_index_init = if self.attributes.pk_unsized { let pk_ident = &self.pk_ident; + let map_type = if self.attributes.pk_wti_logical { + quote! { PersistentWtiIndex } + } else { + quote! { IndexMap } + }; quote! { - let pk_map = IndexMap::<#pk_ident, OffsetEqLink<#const_name>, UnsizedNode<_>>::with_maximum_node_size(#const_name); + let pk_map = #map_type::<#pk_ident, OffsetEqLink<#const_name>, UnsizedNode<_>>::with_maximum_node_size(#const_name); for page in self.primary_index.1 { let node = page .inner @@ -175,7 +180,9 @@ impl Generator { let primary_index = PrimaryIndex { pk_map, reverse_pk_map }; } } else { - let map_type = if self.pk_upstream { + let map_type = if self.attributes.pk_wti_logical { + quote! { PersistentWtiIndex } + } else if self.pk_upstream { quote! { UpstreamIndexMap } } else { quote! { IndexMap } @@ -212,6 +219,14 @@ impl Generator { if self.attributes.read_only { quote! { pub fn into_worktable(self, path: &str) -> Result<#wt_ident, PersistenceLoadError> { + self.into_worktable_with_mode(path, LoadMode::Strict) + } + + pub fn into_worktable_with_mode( + self, + path: &str, + mode: LoadMode, + ) -> Result<#wt_ident, PersistenceLoadError> { let mut page_id = 1; let data = self.data.into_iter().map(|p| { let mut data = Data::from_data_page(p); @@ -240,7 +255,7 @@ impl Generator { table.validate_persisted_state(path)?; let worktable = #wt_ident(table); - worktable.validate_loaded_secondary_state(path)?; + worktable.validate_loaded_secondary_state(path, mode)?; Ok(worktable) } } @@ -251,6 +266,26 @@ impl Generator { engine: E, path: &str, ) -> Result<#wt_ident, PersistenceLoadError> + where + E: PersistenceEngine< + <<#pk_type as TablePrimaryKey>::Generator as PrimaryKeyGeneratorState>::State, + #pk_type, + #secondary_index_events, + #avt_index_ident, + Config=C + > + Send + + 'static, + C: Clone + PersistenceConfig, + { + self.into_worktable_with_mode(engine, path, LoadMode::Strict).await + } + + pub async fn into_worktable_with_mode( + self, + engine: E, + path: &str, + mode: LoadMode, + ) -> Result<#wt_ident, PersistenceLoadError> where E: PersistenceEngine< <<#pk_type as TablePrimaryKey>::Generator as PrimaryKeyGeneratorState>::State, @@ -293,7 +328,7 @@ impl Generator { table, #task_ident::run_engine(engine) ); - worktable.validate_loaded_secondary_state(path)?; + worktable.validate_loaded_secondary_state(path, mode)?; Ok(worktable) } } diff --git a/codegen/src/persist_table/generator/space_file/worktable_impls.rs b/codegen/src/persist_table/generator/space_file/worktable_impls.rs index 0b69fe62..20745e63 100644 --- a/codegen/src/persist_table/generator/space_file/worktable_impls.rs +++ b/codegen/src/persist_table/generator/space_file/worktable_impls.rs @@ -30,7 +30,7 @@ impl Generator { } else { quote! { /// Returns the physical size of this table's `.wt.data` file. - /// Persisted vacuum currently compacts logical/in-memory pages + /// Persisted vacuum makes freed pages reusable across reloads, /// but does not truncate this file. pub async fn persisted_data_file_size_bytes(&self) -> std::io::Result { self.1.persisted_data_file_size_bytes().await diff --git a/codegen/src/persist_table/mod.rs b/codegen/src/persist_table/mod.rs index 91d598bf..e0d12cfb 100644 --- a/codegen/src/persist_table/mod.rs +++ b/codegen/src/persist_table/mod.rs @@ -74,6 +74,11 @@ mod tests { !output.contains("async fn into_worktable"), "read_only into_worktable should not be async" ); + assert!( + output.contains("fn into_worktable_with_mode"), + "read_only should generate explicit recovery-mode conversion" + ); + assert!(output.contains("LoadMode :: Strict")); } #[test] @@ -98,6 +103,11 @@ mod tests { output.contains("async fn into_worktable"), "normal into_worktable should be async" ); + assert!( + output.contains("async fn into_worktable_with_mode"), + "normal should generate explicit recovery-mode conversion" + ); + assert!(output.contains("LoadMode :: Strict")); } #[test] diff --git a/codegen/src/persist_table/parser.rs b/codegen/src/persist_table/parser.rs index 26773d95..5803beb8 100644 --- a/codegen/src/persist_table/parser.rs +++ b/codegen/src/persist_table/parser.rs @@ -32,6 +32,7 @@ impl Parser { pk_upstream: false, pk_arctic: false, pk_congee: false, + pk_wti_logical: false, row_schema: vec![], primary_key_fields: vec![], secondary_index_types: vec![], @@ -60,6 +61,10 @@ impl Parser { res.pk_congee = true; return Ok(()); } + if meta.path.is_ident("pk_wti_logical") { + res.pk_wti_logical = true; + return Ok(()); + } if meta.path.is_ident("row_schema") { meta.parse_nested_meta(|field| { let name = field diff --git a/codegen/src/worktable/mod.rs b/codegen/src/worktable/mod.rs index fe993415..e9334afd 100644 --- a/codegen/src/worktable/mod.rs +++ b/codegen/src/worktable/mod.rs @@ -206,9 +206,42 @@ mod tests { indexes: { value_idx: value unique, }, - }); + }) + .unwrap() + .to_string(); + + if cfg!(feature = "logical-index-persistence") { + assert!(output.contains("PersistentWtiIndex")); + } else { + assert!(output.contains("IndexMap")); + assert!(!output.contains("PersistentWtiIndex")); + } + } - assert!(output.is_ok()); + #[cfg(feature = "logical-index-persistence")] + #[test] + fn logical_persistence_wraps_only_default_wti_backends() { + let output = expand(quote! { + name: LogicalDefaultBackend, + persist: true, + columns: { + id: u64 primary_key autoincrement, + wti_value: u64, + congee_value: u64, + arctic_value: u64, + }, + indexes: { + wti_idx: wti_value unique, + congee_idx: congee_value unique using congee, + arctic_idx: arctic_value unique using arctic, + }, + }) + .unwrap() + .to_string(); + + assert!(output.contains("PersistentWtiIndex")); + assert!(output.contains("PersistentCongeeIndex")); + assert!(output.contains("PersistentArcticIndex")); } #[test] diff --git a/docs/crate.md b/docs/crate.md index dc382cca..f697626e 100644 --- a/docs/crate.md +++ b/docs/crate.md @@ -81,6 +81,15 @@ secondary-index types. Existing legacy stores whose metadata is completely empty remain readable but cannot be schema-validated, and loading them does not rewrite the file. A non-empty schema mismatch is rejected before row loading. +`PersistedWorkTable::load()` is always the strict production path: it validates +primary/data integrity and agreement with every secondary index before returning a +table. An offline recovery program may explicitly use +`load_with(engine, LoadMode::Recovery)` on a private scratch copy to read +individually validated rows through a surviving index and copy them into a new table. +Recovery mode is not an in-place repair, must not serve traffic, and the rebuilt +destination must pass a normal strict load before publication. See the repository's +`docs/persistence-durability.md` for the complete procedure. + Persisted vacuum compacts the live in-memory layout and keeps persisted indexes consistent with moved rows. It does not truncate `.wt.data`. Generated persisted tables expose `persisted_data_file_size_bytes()` so operators can diff --git a/docs/persistence-durability.md b/docs/persistence-durability.md index 9f2e3215..bc37ac23 100644 --- a/docs/persistence-durability.md +++ b/docs/persistence-durability.md @@ -25,8 +25,8 @@ enqueue new work after the task appears idle. ## Load validation -Loading an existing table performs an additional startup-only audit before the -persistence worker starts: +The default `PersistedWorkTable::load()` path uses `LoadMode::Strict` and performs +an additional startup-only audit before the persistence worker starts: - persisted archived rows referenced by the primary index must pass rkyv validation; - each physical link must be in the initialized part of its page; @@ -53,9 +53,46 @@ match MyWorkTable::load(engine).await { } ``` -The audit is proportional to the number of primary-index entries. It runs only during -`load()` and adds no branch, lock, or scan to steady-state insert, select, update, or -delete paths. +The strict audit is proportional to the number of primary-index entries. It runs only +during `load()` and adds no branch, lock, or scan to steady-state insert, select, +update, or delete paths. + +## Offline index recovery + +`PersistedWorkTable::load_with(engine, LoadMode::Recovery)` is a low-level +escape hatch for an offline recovery program. It exists for a specific case: a +private copy has an index that cannot be trusted, while another index and the data +pages may still contain valid rows that can be copied into a fresh table. + +Recovery mode relaxes only cross-index completeness and equality. It still: + +- parses the persisted files normally; +- validates every surviving primary-index entry, including its forward/reverse + mapping and decoded primary key; and +- validates every surviving secondary-index entry by checked row decoding and by + comparing the index key with the referenced row. + +For example, a recovery tool may preserve the rejected directory, copy it to a +scratch location, move a damaged primary-index file aside in that scratch copy, let +the engine create an empty primary index, and then read valid rows through a surviving +secondary index: + +```rust +let scratch = RecoveryWorkTable::load_with(engine, LoadMode::Recovery).await?; +for row in scratch.select_by_tenant(tenant).execute()? { + clean_table.insert(row)?; +} +scratch.close().await?; +clean_table.close().await?; + +// Reopen the rebuilt destination using the strict default before publishing it. +let clean_table = RecoveryWorkTable::load(clean_engine).await?; +``` + +Never point recovery mode at a live or only copy, serve traffic from the returned +table, or treat it as an in-place repair. Do not insert, update, or delete through the +recovery table. A malformed surviving entry is still rejected; recovery mode does not +turn arbitrary bytes into rows. ## Supported recovery procedure @@ -65,15 +102,17 @@ logical generation even though the format cannot commit them atomically. 1. Stop every process that can write the table. 2. Preserve the rejected table directory for diagnosis. -3. Restore the **entire** table directory from one application-managed snapshot; or +3. Restore the **entire** table directory from one application-managed snapshot; create a new empty table directory and replay rows from an external authoritative - source or event log. + source or event log; or use the offline recovery mode above to copy individually + validated rows from a scratch copy into a new table. 4. Open the restored/rebuilt directory and require `load()` to pass before serving it. -WorkTable does not currently provide an in-place salvage tool that can prove which -side of a torn multi-file batch is authoritative. Full-directory restore or clean -replay is the supported recovery path. If neither exists, acknowledged data may be -unrecoverable under this best-effort contract. +WorkTable does not provide automatic or in-place salvage and cannot prove which side +of a torn multi-file batch is authoritative. Full-directory restore, clean replay, or +explicit row-by-row rebuilding from a checked scratch copy are the supported recovery +paths. If none is possible, acknowledged data may be unrecoverable under this +best-effort contract. ## When stronger durability is required diff --git a/docs/wti-dirty-generation-persistence-plan.md b/docs/wti-dirty-generation-persistence-plan.md index c881cac0..dd81f510 100644 --- a/docs/wti-dirty-generation-persistence-plan.md +++ b/docs/wti-dirty-generation-persistence-plan.md @@ -1,6 +1,6 @@ # WTI dirty-generation persistence -**Status:** immediate follow-up design; not implemented. +**Status:** first stage implemented behind `logical-index-persistence`; dirty-generation checkpoints remain follow-up work. **Scope:** reduce caller-thread persistence overhead for WorkTablesIndex (WTI) without changing point-read behavior or weakening recovery. @@ -18,6 +18,22 @@ A focused local ARM probe measured: These are two interleaved ten-trial microprobe runs, not publication results. They exclude row work and disk I/O. They establish that caller-side CDC is large enough to optimize. +The first implementation was then measured with another ten-trial interleaved +ARM probe over 2,000,000 existing-key mutations and 8,000,000 point reads per +trial: + +| Path | Median | Delta | +|---|---:|---:| +| Structural CDC update | 80.916 ns/op | baseline | +| Logical foreground update | 71.440 ns/op | 11.71% faster | +| Raw WTI point read | 105.096 ns/op | baseline | +| Wrapped WTI point read | 103.068 ns/op | 1.93% faster (noise; no observed regression) | + +These repeat-run results were tight across the ten trials, but they remain local +engineering probes rather than paper numbers. Generated-table select +measurements were noisy enough to require a dedicated quiet-window run before +drawing a sub-percent conclusion. The feature therefore remains opt-in. + WorkTable already uses lifecycle flags (`GHOSTED`, `DELETED`, and `VACUUMED`) and optional immutable row versions to prevent readers from observing partially published rows. Those are visibility states, not persistence dirty states, but the staged-publication pattern is relevant. ## Why one dirty bit is insufficient @@ -32,7 +48,69 @@ A Boolean has a lost-update race: The persistence marker must carry a generation or an explicit redirty state. Clearing is conditional: the flusher may mark a node clean only if no writer advanced its generation after the snapshot began. -## Proposed architecture +## Implemented first stage: background structural translation + +The first implementation deliberately avoids a new WTI disk format. With the +`logical-index-persistence` Cargo feature enabled: + +1. each persisted primary or unique-secondary WTI is wrapped by `PersistentWtiIndex`; +2. point reads delegate directly to WTI, with no runtime feature branch or new + read-side lock; +3. each successful foreground mutation emits one logical Set/Remove event; +4. the existing persistence worker owns a private shadow WTI reconstructed from + the current `.wt.idx` pages; +5. that shadow translates logical events into native structural CDC; and +6. the existing `SpaceIndex`/`SpaceIndexUnsized` writer applies those events to + the unchanged DataBucket page format. + +Foreground stripes establish same-key mutation order only. They use +`DefaultHasher`, so stripe identity has no relationship to key or range order. +The queue analyzer sorts each per-index stream by its global event ID before +dispatch, and the logical shadow sorts again at its boundary as defense in +depth. Reversed-delivery regression coverage proves that a Set/Remove pair is +applied in event-ID order. + +Each logical WTI contains 64 inline `parking_lot::Mutex<()>` stripes. Their +target-dependent fixed footprint is paid once per persisted primary or unique +WTI; point reads never access them. This is a deliberate write-concurrency +tradeoff and should be included in schema-level memory measurements. + +This is an intentionally smaller step than the checkpoint/WAL design below. It +moves the measured structural-CDC work off the caller thread while retaining +format compatibility in both directions: a store written with the feature can +be reopened without it, and a pre-feature store can be opened with it. The +existing WorkTable persistence queue remains the recovery authority, including +its documented best-effort crash-durability boundary; this stage does not add a +new durable logical WAL. + +Shadow divergence is reported as `PersistenceIndexCorruption`, not a generic +worker error. It quarantines the table's whole persistence engine, rejects later +persistence submissions, and is surfaced by `wait_for_ops` and `close`. +Continuing row or sibling-index writes after one index diverges would knowingly +create an inconsistent store, so the current architecture cannot safely +quarantine only that index. + +The code generator selects this path using the forwarded +`worktable_codegen/logical-index-persistence` feature. That check intentionally +runs in the proc-macro crate: emitting a downstream `#[cfg]` would inspect the +consumer package's feature namespace rather than WorkTable's dependency +feature. Feature-off and feature-on expansion tests cover both selections. + +The DSL contract is unchanged: + +- omitting `using` selects WorkTablesIndex; +- `using congee` selects `congee-wt`; +- `using arctic` selects `arctic-wt`. + +Congee and Arctic already use their native topology checkpoint plus logical WAL +implementations. Their `export_topology`/`from_topology` APIs are sufficient, so +this first stage requires no source change in either fork. + +Non-unique WTI secondary indexes continue to emit structural multimap CDC in +this stage. Their logical record must identify both key and row link, and should +be added only with the same rollback, duplicate-ordering, and reload coverage. + +## Longer-term architecture Separate crash authority from physical checkpoint maintenance: @@ -155,11 +233,11 @@ It should not: ## Implementation stages -1. **Measure and instrument.** Keep the direct-versus-structural-CDC probe, add allocations/op and p50/p99, and measure full generated WTI table operations. -2. **Define recovery authority.** Make index rebuild from authoritative data pages an explicit, tested fallback and version the new WTI format. -3. **Add feature-gated logical WTI redo.** Preserve the existing structural-CDC path as default until validation is complete. -4. **Add whole-index background checkpointing.** Use atomic replacement and a checkpoint generation; prove redo truncation ordering with crash injection. -5. **Evaluate the result.** Continue only if the caller-thread savings survive end-to-end WorkTable benchmarks. +1. **Background structural translation (implemented, feature-gated).** Preserve the existing disk format and move structural CDC onto a worker-owned shadow WTI. +2. **Measure and instrument.** Keep the direct-versus-structural-CDC probe, add allocations/op and p50/p99, and measure full generated WTI table operations. +3. **Evaluate the result.** Continue only if the caller-thread savings survive end-to-end WorkTable benchmarks. +4. **Define stronger recovery authority.** Make index rebuild from authoritative data pages an explicit, tested fallback and version any new WTI format. +5. **Add durable logical WTI redo and whole-index checkpoints.** Use atomic replacement and a checkpoint generation; prove redo truncation ordering with crash injection. 6. **Optionally add incremental dirty nodes.** Introduce the generation state machine, pinning, structural groups, and generation manifest. 7. **Validate local disk and S3.** Test interrupted append, checkpoint, rename, upload, download, and writes after recovery. diff --git a/src/in_memory/data.rs b/src/in_memory/data.rs index 789bb83b..a440dd00 100644 --- a/src/in_memory/data.rs +++ b/src/in_memory/data.rs @@ -135,6 +135,10 @@ impl Data { if length != link.length { return Err(ExecutionError::InvalidLink); } + debug_assert_eq!( + length, link.length, + "slot length was checked before archived bytes are overwritten" + ); let inner_data = unsafe { &mut *self.inner_data.get() }; inner_data[link.offset as usize..][..link.length as usize].copy_from_slice(bytes.as_slice()); diff --git a/src/in_memory/pages.rs b/src/in_memory/pages.rs index 1aaa4e4f..156e5063 100644 --- a/src/in_memory/pages.rs +++ b/src/in_memory/pages.rs @@ -629,6 +629,70 @@ where Ok(result) } + /// In-place update of an already-live row at `link`: re-serialize the full + /// row into the SAME slot and republish it as LIVE (unghosted). Unlike + /// [`Self::update`], this does not stage the row as a new (ghosted) + /// publication — a live row that is edited must stay visible to readers. + /// The caller must guarantee the new row serializes to the same length as + /// the current slot (so it fits exactly). + /// + /// # Persistence + /// This path emits **no** persistence CDC. It is only sound for tables that + /// are not persisted (or on a persistence sink that reconstructs state from + /// the page image on reload). Do NOT route a persisted-table update through + /// this method: the row would change in memory and republish but no change + /// event would reach disk, silently losing durability until reload. The + /// generated persisted update path deliberately keeps the reinsert path for + /// this reason. + /// + /// Serialization and the exact-length check finish before any page byte is + /// changed. `page_access` excludes low-level archived-page readers during + /// the copy, while generated reads continue from the old immutable + /// publication until [`Self::publish_wrapped_row`] replaces the complete + /// owned row and flags together. + /// + /// # Safety + /// Same contract as [`Self::update`]: `link` must be valid and no other + /// mutable references to the row may exist during modification. + pub unsafe fn update_in_place(&self, row: Row, link: Link) -> Result<(), ExecutionError> + where + Row: Archive + Clone, + ::WrappedRow: + Archive + for<'a> Serialize, Share>, rkyv::rancor::Error>>, + <::WrappedRow as Archive>::Archived: Portable + ArchivedRowWrapper, + <::WrappedRow as Archive>::Archived: + Deserialize<::WrappedRow, HighDeserializer>, + { + let _page_access = self.page_access.write(); + let pages = self.pages.read(); + let page = pages + .get(page_id_mapper(link.page_id.into())) + .ok_or(ExecutionError::PageNotFound(link.page_id))?; + // Write the new bytes into the slot. `save_row_by_link` requires the + // serialized wrapped row to be EXACTLY the slot length; the caller only + // guaranteed equal *field* sizes, which need not imply equal total + // serialized length (alignment/padding). If it does not fit, report it + // so the caller can fall back to a reinsert instead of corrupting. + // `row` is consumed by the wrapper here (no clone): it is not used again. + let gen_row = ::WrappedRow::from_inner(row); + unsafe { + page.save_row_by_link(&gen_row, link) + .map_err(ExecutionError::DataPageError)?; + } + // Clear the ghost bit on the stored row and republish the LIVE image + // from the page, exactly like `with_mut_ref` — so the publication cache + // is not left ghosted (a fresh `from_inner` wrapper is ghosted). + unsafe { + page.get_mut_row_ref(link) + .map_err(ExecutionError::DataPageError)? + .unseal_unchecked() + .unghost(); + } + let wrapped = page.get_row(link).map_err(ExecutionError::DataPageError)?; + self.publish_wrapped_row(link, wrapped); + Ok(()) + } + pub fn delete(&self, link: Link) -> Result<(), ExecutionError> where Row: Archive + for<'a> Serialize, Share>, rkyv::rancor::Error>>, @@ -851,6 +915,7 @@ mod tests { use crate::in_memory::pages::{DataPages, ExecutionError}; use crate::in_memory::{DATA_INNER_LENGTH, PagesExecutionError, RowWrapper, StorableRow}; use crate::prelude::ArchivedRowWrapper; + use data_bucket::Link; #[derive(Archive, Copy, Clone, Deserialize, Debug, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] struct TestRow { @@ -1031,6 +1096,32 @@ mod tests { assert_eq!(pages.select_non_ghosted(link), Ok(TestRow { a: 1, b: 1 })); } + #[test] + fn failed_exact_length_update_preserves_page_bytes_and_publication() { + let pages = DataPages::::new(); + let old_row = TestRow { a: 10, b: 20 }; + let link = pages.insert(old_row).unwrap(); + unsafe { + pages.with_mut_ref(link, |row| row.unghost()).unwrap(); + } + let old_bytes = pages.select_raw(link).unwrap(); + let wrong_length = Link { + length: link.length - 1, + ..link + }; + + let result = unsafe { pages.update_in_place::(TestRow { a: 30, b: 40 }, wrong_length) }; + + assert!(matches!( + result, + Err(ExecutionError::DataPageError( + crate::in_memory::DataExecutionError::InvalidLink + )) + )); + assert_eq!(pages.select_raw(link).unwrap(), old_bytes); + assert_eq!(pages.select_non_ghosted(link), Ok(old_row)); + } + #[test] fn retired_version_survives_link_reuse_for_in_flight_reader() { let pages = DataPages::::new(); diff --git a/src/index/mod.rs b/src/index/mod.rs index b6c4351f..8ed7f279 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -3,6 +3,7 @@ mod available_index; mod congee; mod multipair; mod persistent_art; +mod persistent_wti; mod primary_index; mod table_index; mod table_secondary_index; @@ -16,6 +17,7 @@ pub use indexset::concurrent::map::BTreeMap as IndexMap; pub use indexset::concurrent::multimap::BTreeMultiMap as IndexMultiMap; pub use multipair::MultiPairRecreate; pub use persistent_art::{PersistentArcticIndex, PersistentArtIndex, PersistentCongeeIndex}; +pub use persistent_wti::PersistentWtiIndex; pub use primary_index::PrimaryIndex; pub use table_index::{TableIndex, TableIndexCdc, convert_change_events, convert_upstream_change_events}; pub use table_secondary_index::{ diff --git a/src/index/persistent_wti.rs b/src/index/persistent_wti.rs new file mode 100644 index 00000000..abbc0ad4 --- /dev/null +++ b/src/index/persistent_wti.rs @@ -0,0 +1,330 @@ +//! Logical change capture for persisted unique WorkTablesIndex instances. +//! +//! The normal persisted path asks the live index to produce structural CDC +//! events. With `logical-index-persistence`, this wrapper instead emits one +//! logical Set/Remove event. The background disk-side shadow index translates +//! it back into structural events, preserving the existing on-disk format +//! while removing structural CDC bookkeeping from foreground mutations. +//! `index = 0` and `max_value == value` are an intentionally synthetic marker, +//! not claims about the live WTI node position or maximum; the shadow validates +//! that marker and derives the real structural metadata itself. + +use std::array; +use std::fmt::{self, Debug}; +use std::hash::{Hash, Hasher}; +use std::ops::RangeBounds; +use std::sync::Arc; +use std::sync::atomic::{AtomicU64, Ordering}; + +use data_bucket::Link; +use indexset::cdc::change::{ChangeEvent, Id}; +use indexset::core::node::NodeLike; +use indexset::core::pair::Pair; +use parking_lot::{Mutex, MutexGuard}; +use rustc_hash::FxHasher; + +use crate::index::UniqueIndex; +use crate::util::OffsetEqLink; +use crate::{IndexMap, TableIndexCdc}; + +// A stripe provides per-key exclusion only: FxHasher has no relationship +// to key order, so callers must not infer range or cross-key ordering from the +// selected mutex. Reads never touch this fixed inline table. Logical batches +// are ordered independently by event id in the persistence worker. +const MUTATION_STRIPES: usize = 64; + +#[inline] +fn mutation_stripe_index(key: &Q) -> usize { + // Stripe selection is not a security boundary. FxHasher avoids SipHash's + // per-mutation cost and distributes the overwhelmingly common sequential + // integer keys across the power-of-two stripe table. + let mut hasher = FxHasher::default(); + key.hash(&mut hasher); + hasher.finish() as usize & (MUTATION_STRIPES - 1) +} + +/// A persisted WorkTablesIndex whose foreground mutations emit logical CDC. +/// +/// Point reads delegate directly to the native index. There is no runtime +/// feature check, read lock, or consistency branch on the select path. +pub struct PersistentWtiIndex>> +where + K: Send + Ord + Clone + 'static, + V: Send + Clone + 'static, + Node: NodeLike>, +{ + inner: IndexMap, + next_event_id: AtomicU64, + // Fixed inline allocation: 64 parking_lot mutexes per persisted WTI. The + // enclosing index's allocation size accounts for these; there is no + // per-mutation or per-key mutex allocation. + mutation_stripes: [Mutex<()>; MUTATION_STRIPES], +} + +impl Debug for PersistentWtiIndex +where + K: Send + Ord + Clone + 'static, + V: Send + Clone + 'static, + Node: NodeLike>, +{ + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("PersistentWtiIndex") + .field("next_event_id", &self.next_event_id.load(Ordering::Relaxed)) + .finish_non_exhaustive() + } +} + +impl Default for PersistentWtiIndex +where + K: Send + Ord + Clone + 'static, + V: Send + Clone + 'static, + Node: NodeLike>, + IndexMap: Default, +{ + fn default() -> Self { + Self::from_inner(IndexMap::default()) + } +} + +impl PersistentWtiIndex +where + K: Send + Ord + Clone + 'static, + V: Send + Clone + 'static, + Node: NodeLike>, +{ + pub fn from_inner(inner: IndexMap) -> Self { + Self { + inner, + next_event_id: AtomicU64::new(0), + mutation_stripes: array::from_fn(|_| Mutex::new(())), + } + } + + pub fn into_inner(self) -> IndexMap { + self.inner + } + + pub fn inner(&self) -> &IndexMap { + &self.inner + } + + #[inline] + fn mutation_stripe(&self, key: &Q) -> MutexGuard<'_, ()> { + self.mutation_stripes[mutation_stripe_index(key)].lock() + } + + fn next_event_id(&self) -> Id { + self.next_event_id.fetch_add(1, Ordering::AcqRel).into() + } +} + +impl PersistentWtiIndex +where + K: Debug + Send + Ord + Clone + 'static, + V: Debug + Send + Clone + 'static, + Node: NodeLike> + Send + 'static, +{ + pub fn with_maximum_node_size(node_capacity: usize) -> Self { + Self::from_inner(IndexMap::with_maximum_node_size(node_capacity)) + } + + pub fn attach_node(&self, node: Node) { + self.inner.attach_node(node); + } + + pub fn iter_nodes(&self) -> impl Iterator>> + '_ { + self.inner.iter_nodes() + } + + pub fn iter(&self) -> impl DoubleEndedIterator + '_ { + self.inner.iter() + } + + pub fn capacity(&self) -> usize { + self.inner.capacity() + } + + pub fn node_count(&self) -> usize { + self.inner.node_count() + } +} + +impl UniqueIndex for PersistentWtiIndex +where + K: Debug + Eq + Hash + Clone + Send + Ord + 'static, + V: Debug + Clone + Send + Ord + 'static, + Node: NodeLike> + Send + 'static, +{ + #[inline(always)] + fn get_value(&self, key: &K) -> Option { + self.inner.lookup_for_select(key) + } + + #[inline(always)] + fn lookup_for_select(&self, key: &K) -> Option { + self.inner.lookup_for_select(key) + } + + #[inline(always)] + fn with_value(&self, key: &K, read: impl FnOnce(&V) -> R) -> Option { + self.inner.lookup_for_select(key).as_ref().map(read) + } + + #[inline(always)] + fn contains_key(&self, key: &K) -> bool { + self.inner.contains_key(key) + } + + #[inline] + fn insert_value(&self, key: K, value: V) -> Option { + self.inner.insert(key, value) + } + + #[inline] + fn insert_value_checked(&self, key: K, value: V) -> Option<()> { + self.inner.checked_insert(key, value) + } + + #[inline] + fn remove_value(&self, key: &K) -> Option<(K, V)> { + self.inner.remove(key) + } + + #[inline] + fn len(&self) -> usize { + self.inner.len() + } + + fn iter_values(&self) -> impl DoubleEndedIterator + '_ { + self.inner.iter().map(|(key, value)| (key.clone(), value.clone())) + } + + fn iter_links(&self) -> impl DoubleEndedIterator + '_ { + self.inner.iter().map(|(_, value)| value.clone()) + } + + fn range_values<'a, R>(&'a self, range: R) -> impl DoubleEndedIterator + 'a + where + R: RangeBounds + 'a, + { + self.inner.range(range).map(|(key, value)| (key.clone(), value.clone())) + } + + fn range_links<'a, R>(&'a self, range: R) -> impl DoubleEndedIterator + 'a + where + R: RangeBounds + 'a, + { + self.inner.range(range).map(|(_, value)| value.clone()) + } +} + +impl TableIndexCdc for PersistentWtiIndex, Node> +where + T: Debug + Eq + Hash + Clone + Send + Ord + 'static, + Node: NodeLike>> + Send + 'static, +{ + fn insert_cdc(&self, value: T, link: Link) -> (Option, Vec>>) { + let _sequence_guard = self.mutation_stripe(&value); + let old = self + .inner + .insert(value.clone(), OffsetEqLink(link)) + .map(|value| value.0); + let pair = Pair { + key: value, + value: link, + }; + let event = ChangeEvent::InsertAt { + event_id: self.next_event_id(), + max_value: pair.clone(), + value: pair, + index: 0, + }; + (old, vec![event]) + } + + fn insert_checked_cdc(&self, value: T, link: Link) -> Option>>> { + let _sequence_guard = self.mutation_stripe(&value); + self.inner.checked_insert(value.clone(), OffsetEqLink(link))?; + let pair = Pair { + key: value, + value: link, + }; + Some(vec![ChangeEvent::InsertAt { + event_id: self.next_event_id(), + max_value: pair.clone(), + value: pair, + index: 0, + }]) + } + + fn remove_cdc(&self, value: T, _: Link) -> (Option<(T, Link)>, Vec>>) { + let _sequence_guard = self.mutation_stripe(&value); + let Some((key, old)) = self.inner.remove(&value) else { + return (None, Vec::new()); + }; + let pair = Pair { + key: key.clone(), + value: old.0, + }; + let event = ChangeEvent::RemoveAt { + event_id: self.next_event_id(), + max_value: pair.clone(), + value: pair, + index: 0, + }; + (Some((key, old.0)), vec![event]) + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use data_bucket::page::PageId; + + use super::*; + + fn link(offset: u32) -> Link { + Link { + page_id: PageId::from(1), + offset, + length: 8, + } + } + + #[test] + fn reads_delegate_and_mutations_emit_one_logical_event() { + let index = PersistentWtiIndex::>::default(); + let (old, events) = index.insert_cdc(7, link(7)); + assert_eq!(old, None); + assert_eq!(events.len(), 1); + assert_eq!(index.get_value(&7), Some(OffsetEqLink(link(7)))); + + let (removed, events) = index.remove_cdc(7, link(7)); + assert_eq!(removed, Some((7, link(7)))); + assert_eq!(events.len(), 1); + assert_eq!(index.get_value(&7), None); + } + + #[test] + fn rejected_checked_insert_does_not_consume_an_event_id() { + let index = PersistentWtiIndex::>::default(); + assert!(index.insert_checked_cdc(7, link(7)).is_some()); + assert_eq!(index.next_event_id.load(Ordering::Relaxed), 1); + + assert!(index.insert_checked_cdc(7, link(8)).is_none()); + assert_eq!(index.next_event_id.load(Ordering::Relaxed), 1); + + assert!(index.insert_checked_cdc(8, link(8)).is_some()); + assert_eq!(index.next_event_id.load(Ordering::Relaxed), 2); + } + + #[test] + fn sequential_integer_keys_use_every_mutation_stripe() { + let stripes = (0_u64..4_096) + .map(|key| mutation_stripe_index(&key)) + .collect::>(); + assert_eq!(stripes.len(), MUTATION_STRIPES); + } +} diff --git a/src/index/table_index/mod.rs b/src/index/table_index/mod.rs index 75c8b71b..4affdaeb 100644 --- a/src/index/table_index/mod.rs +++ b/src/index/table_index/mod.rs @@ -11,7 +11,7 @@ use vanilla_indexset::core::pair::Pair as VanillaPair; use crate::util::OffsetEqLink; use crate::{ ArcticIndex, ArcticKey, CongeeIndex, CongeeKey, IndexMap, IndexMultiMap, PersistentArcticIndex, - PersistentCongeeIndex, UniqueIndex, UpstreamIndexMap, + PersistentCongeeIndex, PersistentWtiIndex, UniqueIndex, UpstreamIndexMap, }; mod cdc; @@ -132,6 +132,24 @@ where } } +impl TableIndex for PersistentWtiIndex +where + T: Debug + Eq + Hash + Clone + Send + Ord + 'static, + Node: NodeLike> + Send + 'static, +{ + fn insert(&self, value: T, link: Link) -> Option { + unique_insert(self, value, link) + } + + fn insert_checked(&self, value: T, link: Link) -> Option<()> { + unique_insert_checked(self, value, link) + } + + fn remove(&self, value: &T, _: Link) -> Option<(T, Link)> { + unique_remove(self, value) + } +} + impl_unique_table_index!(CongeeIndex, [CongeeKey + Eq + Hash]); impl_unique_table_index!(ArcticIndex, [ ArcticKey + Eq + Hash diff --git a/src/lib.rs b/src/lib.rs index 21c25cbf..9e28c540 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -15,7 +15,7 @@ mod util; pub mod features; pub use index::*; -pub use persistence::{PersistedWorkTable, PersistenceConfig, PersistenceLoadError}; +pub use persistence::{LoadMode, PersistedWorkTable, PersistenceConfig, PersistenceLoadError}; pub use row::*; pub use table::*; @@ -31,16 +31,16 @@ pub mod prelude { pub use crate::in_memory::{ArchivedRowWrapper, Data, DataPages, Query, RowWrapper, StorableRow}; pub use crate::lock::FullRowLock; pub use crate::lock::{Lock, RowLock}; - pub use crate::lock::{LockGuard, LockMap}; + pub use crate::lock::{LockAcquirer, LockGuard, LockMap}; pub use crate::mem_stat::MemStat; pub use crate::persistence::{ AcknowledgeOperation, ArtPersistenceKey, DeleteOperation, DiskConfig, DiskPersistenceEngine, - IndexTableOfContents, InsertOperation, Operation, OperationId, PersistedWorkTable, PersistenceConfig, - PersistenceEngine, PersistenceError, PersistenceLoadError, PersistenceResult, PersistenceState, - PersistenceTask, ReadOnlyPersistenceEngine, SpaceArcticIndex, SpaceCongeeIndex, SpaceData, SpaceDataOps, - SpaceIndex, SpaceIndexOps, SpaceIndexUnsized, SpaceSecondaryIndexOps, UpdateOperation, load_persisted_state, - map_index_pages_to_toc_and_general, map_unsized_index_pages_to_toc_and_general, reconstruct_multi_index_nodes, - validate_events, + IndexTableOfContents, InsertOperation, LoadMode, Operation, OperationId, PersistedWorkTable, PersistenceConfig, + PersistenceEngine, PersistenceError, PersistenceIndexCorruption, PersistenceLoadError, PersistenceResult, + PersistenceState, PersistenceTask, ReadOnlyPersistenceEngine, SpaceArcticIndex, SpaceCongeeIndex, SpaceData, + SpaceDataOps, SpaceIndex, SpaceIndexOps, SpaceIndexUnsized, SpaceLogicalIndex, SpaceLogicalIndexUnsized, + SpaceSecondaryIndexOps, UpdateOperation, load_persisted_state, map_index_pages_to_toc_and_general, + map_unsized_index_pages_to_toc_and_general, reconstruct_multi_index_nodes, validate_events, }; pub use crate::primary_key::{PrimaryKeyGenerator, PrimaryKeyGeneratorState, TablePrimaryKey}; pub use crate::table::select::{Order, QueryParams, SelectQueryBuilder, SelectQueryExecutor}; @@ -49,10 +49,10 @@ pub mod prelude { pub use crate::{ ArcticIndex, ArcticKey, AvailableIndex, CongeeIndex, CongeeKey, Difference, IndexError, IndexMap, IndexMultiMap, MultiPairRecreate, PersistentArcticIndex, PersistentArtIndex, PersistentCongeeIndex, - PrimaryIndex, TableIndex, TableIndexCdc, TableRow, TableSecondaryIndex, TableSecondaryIndexCdc, - TableSecondaryIndexEventsOps, TableSecondaryIndexInfo, UniqueIndex, UnsizedNode, UpstreamIndexMap, - UpstreamIndexPair, WorkTable, WorkTableError, vacuum::EmptyDataVacuum, vacuum::VacuumPersistence, - vacuum::WorkTableVacuum, + PersistentWtiIndex, PrimaryIndex, TableIndex, TableIndexCdc, TableRow, TableSecondaryIndex, + TableSecondaryIndexCdc, TableSecondaryIndexEventsOps, TableSecondaryIndexInfo, UniqueIndex, UnsizedNode, + UpstreamIndexMap, UpstreamIndexPair, WorkTable, WorkTableError, vacuum::EmptyDataVacuum, + vacuum::VacuumPersistence, vacuum::WorkTableVacuum, }; pub use data_bucket::{ DATA_VERSION, DataPage, GENERAL_HEADER_SIZE, GeneralHeader, GeneralPage, INNER_PAGE_SIZE, IndexPage, Interval, diff --git a/src/lock/map.rs b/src/lock/map.rs index 2b563d55..088aedf8 100644 --- a/src/lock/map.rs +++ b/src/lock/map.rs @@ -2,8 +2,9 @@ use std::collections::HashMap; use std::collections::hash_map::DefaultHasher; use std::fmt::Debug; use std::hash::{Hash, Hasher}; +use std::ops::Deref; use std::sync::Arc; -use std::sync::atomic::{AtomicU16, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicU16, AtomicU64, AtomicUsize, Ordering}; use parking_lot::RwLock; @@ -28,15 +29,88 @@ pub struct MutationGuard { stripe: usize, } +#[derive(Debug)] +struct LockEntry { + lock: Arc>, + acquirers: Arc, +} + +/// A tracked reference to one row-lock entry while an operation registers. +/// +/// Dropping this handle, including through async task cancellation, retries +/// map cleanup after releasing its lock reference. Clones remain tracked so an +/// entry cannot be removed while any caller may still register against it. +#[derive(Debug)] +pub struct LockAcquirer +where + LockType: RowLock, + PrimaryKey: Hash + Eq + Debug + Clone, +{ + lock: Option>>, + acquirers: Arc, + lock_map: Arc>, + primary_key: PrimaryKey, +} + +impl Clone for LockAcquirer +where + LockType: RowLock, + PrimaryKey: Hash + Eq + Debug + Clone, +{ + fn clone(&self) -> Self { + self.acquirers.fetch_add(1, Ordering::AcqRel); + Self { + lock: self.lock.clone(), + acquirers: self.acquirers.clone(), + lock_map: self.lock_map.clone(), + primary_key: self.primary_key.clone(), + } + } +} + +impl Deref for LockAcquirer +where + LockType: RowLock, + PrimaryKey: Hash + Eq + Debug + Clone, +{ + type Target = tokio::sync::RwLock; + + fn deref(&self) -> &Self::Target { + self.lock.as_deref().expect("the acquisition lock exists until drop") + } +} + +impl Drop for LockAcquirer +where + LockType: RowLock, + PrimaryKey: Hash + Eq + Debug + Clone, +{ + fn drop(&mut self) { + self.acquirers.fetch_sub(1, Ordering::AcqRel); + drop(self.lock.take()); + self.lock_map.remove_with_lock_check(&self.primary_key); + } +} + impl Drop for MutationGuard { fn drop(&mut self) { self.stripes[self.stripe].serving.fetch_add(1, Ordering::Release); } } +/// Registry for per-row async locks and synchronous mutation stripes. +/// +/// # Sync/async lock boundary +/// +/// The `parking_lot` map guard is never returned and never crosses an +/// `.await`. Acquisition clones a tracked `Arc>` before +/// releasing the map guard. Cleanup may synchronously take the short-lived map +/// write guard, but only probes the per-row lock with `try_read`; it never waits +/// on a Tokio lock while holding the map. This one-way boundary prevents a +/// map-lock/per-row-lock cycle during cancellation and `Drop`. #[derive(Debug)] pub struct LockMap { - map: RwLock>>>, + map: RwLock>>, next_id: AtomicU16, mutation_stripes: Arc<[MutationStripe; MUTATION_STRIPE_COUNT]>, } @@ -55,16 +129,33 @@ impl LockMap where PrimaryKey: Hash + Eq + Debug + Clone, { + /// Inserts a raw lock entry. + /// + /// A returned or externally retained `Arc` pins cleanup through + /// `Arc::strong_count`. Generated operations should prefer + /// [`Self::get_or_insert_with`], whose [`LockAcquirer`] makes cancellation + /// tracking explicit. pub fn insert( &self, key: PrimaryKey, lock: Arc>, ) -> Option>> { - self.map.write().insert(key, lock) + self.map + .write() + .insert( + key, + LockEntry { + lock, + acquirers: Arc::new(AtomicUsize::new(0)), + }, + ) + .map(|entry| entry.lock) } + /// Returns an untracked raw lock clone, which keeps the map entry alive + /// until that clone is dropped. pub fn get(&self, key: &PrimaryKey) -> Option>> { - self.map.read().get(key).cloned() + self.map.read().get(key).map(|entry| entry.lock.clone()) } /// Returns the lock for `key`, inserting one built by `f` if absent. @@ -76,8 +167,9 @@ where /// can merge into it, but the *winner* already registered its operation on /// a lock that is no longer in the map, so it never waits for the loser and /// both proceed into the row at once. - pub fn get_or_insert_with(&self, key: PrimaryKey, f: F) -> Arc> + pub fn get_or_insert_with(self: &Arc, key: PrimaryKey, f: F) -> LockAcquirer where + LockType: RowLock, F: FnOnce() -> LockType, { // Fast path: the row is usually already locked by someone, and a read @@ -85,17 +177,28 @@ where // under the guard, so `remove_with_lock_check` (which needs the write // lock) either runs before we looked or sees our extra strong reference // and keeps the entry. - if let Some(lock) = self.map.read().get(&key) { - return lock.clone(); + if let Some(entry) = self.map.read().get(&key) { + entry.acquirers.fetch_add(1, Ordering::AcqRel); + return LockAcquirer { + lock: Some(entry.lock.clone()), + acquirers: entry.acquirers.clone(), + lock_map: self.clone(), + primary_key: key, + }; } let mut map = self.map.write(); // Re-check: another task can insert between the read and write guards. - if let Some(lock) = map.get(&key) { - return lock.clone(); + let entry = map.entry(key.clone()).or_insert_with(|| LockEntry { + lock: Arc::new(tokio::sync::RwLock::new(f())), + acquirers: Arc::new(AtomicUsize::new(0)), + }); + entry.acquirers.fetch_add(1, Ordering::AcqRel); + LockAcquirer { + lock: Some(entry.lock.clone()), + acquirers: entry.acquirers.clone(), + lock_map: self.clone(), + primary_key: key, } - let lock = Arc::new(tokio::sync::RwLock::new(f())); - map.insert(key, lock.clone()); - lock } pub fn remove(&mut self, key: &PrimaryKey) { @@ -107,24 +210,20 @@ where LockType: RowLock, { let mut set = self.map.write(); - if let Some(lock) = set.get(key).cloned() - && let Ok(guard) = lock.try_read() - && !guard.is_locked() - // Two strong references means this map entry and our own `lock` - // clone above, and nothing else. Any higher count is a task that - // has already taken this Arc out of `get_or_insert_with` and is - // about to register on it; removing the entry now would let the - // next caller build a *second* lock for the same row, and the two - // would not serialise against each other. - // - // Known trade-off: if that other task is cancelled between taking - // the Arc and registering its operation, nothing re-triggers this - // cleanup and the (unlocked, unused) entry stays in the map until - // the next operation on the same key drops its guard. That leaks at - // most one empty lock per abandoned key and never affects mutual - // exclusion. - && Arc::strong_count(&lock) == 2 - { + let should_remove = set.get(key).is_some_and(|entry| { + let Ok(guard) = entry.lock.try_read() else { + return false; + }; + !guard.is_locked() + // Every acquisition is counted before the map guard is released. + // A non-zero count means a caller may still register an operation; + // removing now would let a second lock be created for the same row. + && entry.acquirers.load(Ordering::Acquire) == 0 + // `insert` is public and accepts an Arc, so retain the old safety + // check for callers holding a raw clone outside tracked acquisition. + && Arc::strong_count(&entry.lock) == 1 + }); + if should_remove { set.remove(key); } } @@ -161,3 +260,60 @@ where } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::lock::FullRowLock; + + /// Regression for issue #33: cleanup can run while a task owns the value + /// returned by `get_or_insert_with`, then that task can be cancelled before + /// registering an operation. Dropping the acquisition must retry cleanup. + #[test] + fn cancelled_acquirer_removes_the_abandoned_entry() { + let lock_map: Arc> = Arc::new(LockMap::default()); + let acquirer = lock_map.get_or_insert_with(31, FullRowLock::new); + + lock_map.remove_with_lock_check(&31); + assert!(lock_map.map.read().contains_key(&31)); + + drop(acquirer); + assert!(!lock_map.map.read().contains_key(&31)); + } + + /// Cloning the acquisition handle represents two tasks between lookup and + /// registration. The first cancellation must retain the shared lock, and + /// only the last handle may remove it. + #[test] + fn cleanup_waits_for_every_acquirer_to_drop() { + let lock_map: Arc> = Arc::new(LockMap::default()); + let first = lock_map.get_or_insert_with(33, FullRowLock::new); + let second = first.clone(); + + drop(first); + assert!(lock_map.map.read().contains_key(&33)); + + drop(second); + assert!(!lock_map.map.read().contains_key(&33)); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn cancelling_async_waiter_releases_tracking_without_deadlock() { + let lock_map: Arc> = Arc::new(LockMap::default()); + let owner = lock_map.get_or_insert_with(41, FullRowLock::new); + let owner_guard = owner.write().await; + let waiter = lock_map.get_or_insert_with(41, FullRowLock::new); + let waiting_task = tokio::spawn(async move { + let _guard = waiter.write().await; + }); + tokio::task::yield_now().await; + + waiting_task.abort(); + assert!(waiting_task.await.unwrap_err().is_cancelled()); + assert!(lock_map.map.read().contains_key(&41)); + + drop(owner_guard); + drop(owner); + assert!(!lock_map.map.read().contains_key(&41)); + } +} diff --git a/src/lock/mod.rs b/src/lock/mod.rs index 81e194f8..75dea47b 100644 --- a/src/lock/mod.rs +++ b/src/lock/mod.rs @@ -14,7 +14,7 @@ use std::task::{Context, Poll}; use futures::task::AtomicWaker; use parking_lot::Mutex; -pub use map::{LockMap, MutationGuard}; +pub use map::{LockAcquirer, LockMap, MutationGuard}; pub use row_lock::{FullRowLock, RowLock}; /// Maximum number of spin iterations before falling back to async waiting. diff --git a/src/mem_stat/mod.rs b/src/mem_stat/mod.rs index a933b4d6..666d61d1 100644 --- a/src/mem_stat/mod.rs +++ b/src/mem_stat/mod.rs @@ -21,7 +21,8 @@ use crate::persistence::OperationType; use crate::prelude::OperationId; use crate::util::OffsetEqLink; use crate::{ - ArcticIndex, ArcticKey, CongeeIndex, CongeeKey, IndexMultiMap, PersistentArtIndex, UniqueIndex, UpstreamIndexMap, + ArcticIndex, ArcticKey, CongeeIndex, CongeeKey, IndexMultiMap, PersistentArtIndex, PersistentWtiIndex, UniqueIndex, + UpstreamIndexMap, }; use crate::{IndexMap, impl_memstat_zero}; @@ -141,6 +142,21 @@ impl MemStat for PersistentArtIndex { } } +impl MemStat for PersistentWtiIndex +where + K: Debug + Ord + Clone + 'static + MemStat + Send, + V: Debug + Clone + 'static + MemStat + Send, + Node: NodeLike> + Send + 'static, +{ + fn heap_size(&self) -> usize { + self.inner().heap_size() + } + + fn used_size(&self) -> usize { + self.inner().used_size() + } +} + impl MemStat for IndexMultiMap where K: Debug + Ord + Clone + 'static + MemStat + Send, diff --git a/src/persistence/engine.rs b/src/persistence/engine.rs index 6e929bc7..37254f6f 100644 --- a/src/persistence/engine.rs +++ b/src/persistence/engine.rs @@ -285,6 +285,10 @@ where Ok(()) } + async fn reclaim_data_pages(&mut self, page_ids: Vec) -> eyre::Result<()> { + self.data.reclaim_data_pages(page_ids).await + } + async fn ensure_schema( &mut self, row_schema: Vec<(String, String)>, diff --git a/src/persistence/error.rs b/src/persistence/error.rs index ed077fc2..fbfac49e 100644 --- a/src/persistence/error.rs +++ b/src/persistence/error.rs @@ -79,6 +79,50 @@ where } } +/// A persisted index no longer agrees with the logical mutation stream. +/// +/// The safe quarantine boundary is the table's entire persistence engine: a +/// worker must not continue writing row data or sibling indexes after one +/// index diverges, because that would knowingly create a store that cannot be +/// recovered consistently. The in-memory table remains inspectable, while +/// `wait_for_ops`, `close`, and all later persistence submissions return this +/// terminal error through [`PersistenceError::IndexCorruption`]. +#[derive(Debug)] +pub struct PersistenceIndexCorruption { + path: PathBuf, + reason: String, +} + +impl PersistenceIndexCorruption { + pub fn new(path: impl AsRef, reason: impl Display) -> Self { + Self { + path: path.as_ref().to_path_buf(), + reason: reason.to_string(), + } + } + + pub fn path(&self) -> &Path { + &self.path + } + + pub fn reason(&self) -> &str { + &self.reason + } +} + +impl Display for PersistenceIndexCorruption { + fn fmt(&self, formatter: &mut Formatter<'_>) -> std::fmt::Result { + write!( + formatter, + "persisted index at {} was quarantined: {}", + self.path.display(), + self.reason + ) + } +} + +impl Error for PersistenceIndexCorruption {} + /// Terminal and lifecycle errors reported by a persistence task. #[derive(Debug)] pub enum PersistenceError { @@ -86,6 +130,8 @@ pub enum PersistenceError { Closing, /// New work was submitted after graceful shutdown completed. Closed, + /// An index diverged from its validated logical mutation stream. + IndexCorruption(PersistenceIndexCorruption), /// The persistence engine or its queue analyzer failed permanently. Engine(eyre::Report), } @@ -95,6 +141,7 @@ impl Display for PersistenceError { match self { Self::Closing => formatter.write_str("persistence task is closing"), Self::Closed => formatter.write_str("persistence task is closed"), + Self::IndexCorruption(error) => Display::fmt(error, formatter), Self::Engine(error) => write!(formatter, "persistence engine failed: {error:#}"), } } diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index df98e797..121e27ae 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -1,10 +1,15 @@ use std::future::Future; +use data_bucket::page::PageId; + use crate::persistence::operation::BatchOperation; pub use engine::DiskConfig; pub use engine::DiskPersistenceEngine; -pub use error::{PersistenceError, PersistenceLoadError, PersistenceResult, PersistenceState, load_persisted_state}; +pub use error::{ + PersistenceError, PersistenceIndexCorruption, PersistenceLoadError, PersistenceResult, PersistenceState, + load_persisted_state, +}; pub use operation::{ AcknowledgeOperation, DeleteOperation, InsertOperation, Operation, OperationId, OperationType, UpdateOperation, validate_events, @@ -12,8 +17,8 @@ pub use operation::{ pub use readonly_engine::ReadOnlyPersistenceEngine; pub use space::{ ArtPersistenceKey, IndexTableOfContents, SpaceArcticIndex, SpaceCongeeIndex, SpaceData, SpaceDataOps, SpaceIndex, - SpaceIndexOps, SpaceIndexUnsized, SpaceSecondaryIndexOps, map_index_pages_to_toc_and_general, - map_unsized_index_pages_to_toc_and_general, reconstruct_multi_index_nodes, + SpaceIndexOps, SpaceIndexUnsized, SpaceLogicalIndex, SpaceLogicalIndexUnsized, SpaceSecondaryIndexOps, + map_index_pages_to_toc_and_general, map_unsized_index_pages_to_toc_and_general, reconstruct_multi_index_nodes, }; pub use task::PersistenceTask; @@ -31,6 +36,26 @@ pub trait PersistenceConfig { fn version(&self) -> u32; } +/// Controls the consistency checks applied while loading persisted state. +/// +/// Normal application opens must use [`LoadMode::Strict`], which is also the +/// default used by [`PersistedWorkTable::load`]. Recovery tools may use +/// [`LoadMode::Recovery`] on a private copy of a rejected store to read rows +/// through a surviving index and rebuild them into a fresh table. +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub enum LoadMode { + /// Reject disagreement between the primary index, secondary indexes, and + /// rows before exposing the table. + #[default] + Strict, + /// Permit indexes to contain different sets of otherwise valid rows. + /// + /// This mode does not disable file parsing, checked row decoding, or + /// per-entry key/link validation. It is only for offline recovery; never + /// use the returned table to serve live traffic. + Recovery, +} + pub trait PersistedWorkTable: Sized where E: Send, @@ -38,6 +63,16 @@ where fn new(engine: E) -> impl Future> + Send; fn load(engine: E) -> impl Future> + Send; + + /// Loads a table with an explicit consistency policy. + /// + /// The compatibility default forwards to [`Self::load`], so custom + /// persistence implementations remain strict unless they deliberately + /// implement recovery semantics. WorkTable's generated disk and read-only + /// implementations override this method. + fn load_with(engine: E, _mode: LoadMode) -> impl Future> + Send { + Self::load(engine) + } } pub trait PersistenceEngine { @@ -57,6 +92,15 @@ pub trait PersistenceEngine, ) -> impl Future> + Send; + /// Persists whole data pages made reusable by vacuum. + /// + /// The persistence task invokes this only after every row move queued + /// before the reclamation barrier has reached the engine. Custom engines + /// that do not manage data pages may keep the default no-op. + fn reclaim_data_pages(&mut self, _page_ids: Vec) -> impl Future> + Send { + async { Ok(()) } + } + /// Installs the generated table schema and rejects a non-empty schema that /// belongs to a different table shape. /// Custom engines may keep the default no-op when they do not expose diff --git a/src/persistence/space/data.rs b/src/persistence/space/data.rs index 1fbffeaa..b4df72dd 100644 --- a/src/persistence/space/data.rs +++ b/src/persistence/space/data.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::io::SeekFrom; use std::path::Path; @@ -19,6 +20,121 @@ use rkyv::{Archive, Deserialize, Serialize}; use tokio::fs::File; use tokio::io::{AsyncSeekExt, AsyncWriteExt}; +fn link_sort_key(link: &Link) -> (u32, u32) { + (link.page_id.into(), link.offset) +} + +fn link_end(link: &Link) -> u64 { + u64::from(link.offset) + u64::from(link.length) +} + +/// Sorts and coalesces ranges within each page. +fn normalize_ranges(mut ranges: Vec) -> (Vec, bool) { + let was_sorted = ranges + .windows(2) + .all(|pair| link_sort_key(&pair[0]) <= link_sort_key(&pair[1])); + ranges.sort_unstable_by_key(link_sort_key); + + let mut changed = !was_sorted; + let mut normalized: Vec = Vec::with_capacity(ranges.len()); + for range in ranges { + if range.length == 0 { + changed = true; + continue; + } + + if let Some(last) = normalized.last_mut() + && last.page_id == range.page_id + && u64::from(range.offset) <= link_end(last) + { + let end = link_end(last).max(link_end(&range)); + last.length = (end - u64::from(last.offset)) as u32; + changed = true; + continue; + } + + normalized.push(range); + } + + (normalized, changed) +} + +/// Subtracts sorted, coalesced used ranges from sorted, coalesced free ranges. +/// +/// Both cursors only move forward, so the subtraction is O(f log f + u log u) +/// for sorting and O(f + u) for the scan instead of rebuilding the full free +/// list once per used link. +fn subtract_used_ranges(free_ranges: Vec, used_ranges: impl IntoIterator) -> (Vec, bool) { + let (free_ranges, mut changed) = normalize_ranges(free_ranges); + let (used_ranges, _) = normalize_ranges(used_ranges.into_iter().collect()); + if used_ranges.is_empty() { + return (free_ranges, changed); + } + + let mut remaining = Vec::with_capacity(free_ranges.len() + used_ranges.len()); + let mut used_index = 0; + + for free in free_ranges { + let free_page: u32 = free.page_id.into(); + let free_start = u64::from(free.offset); + let free_end = link_end(&free); + + while let Some(used) = used_ranges.get(used_index) { + let used_page: u32 = used.page_id.into(); + if used_page < free_page || (used_page == free_page && link_end(used) <= free_start) { + used_index += 1; + } else { + break; + } + } + + let mut cursor = free_start; + let mut scan = used_index; + while let Some(used) = used_ranges.get(scan) { + let used_page: u32 = used.page_id.into(); + let used_start = u64::from(used.offset); + let used_end = link_end(used); + if used_page != free_page || used_start >= free_end { + break; + } + + if used_end > cursor { + if cursor < used_start { + let segment_end = used_start.min(free_end); + remaining.push(Link { + page_id: free.page_id, + offset: cursor as u32, + length: (segment_end - cursor) as u32, + }); + } + + let overlap_end = used_end.min(free_end); + if cursor.max(used_start) < overlap_end { + changed = true; + cursor = overlap_end; + } + } + + if used_end <= free_end { + scan += 1; + } else { + break; + } + } + used_index = scan; + + if cursor < free_end { + remaining.push(Link { + page_id: free.page_id, + offset: cursor as u32, + length: (free_end - cursor) as u32, + }); + } + } + + (remaining, changed) +} + #[derive(Debug)] pub struct SpaceData { pub info: GeneralPage>, @@ -37,6 +153,18 @@ impl SpaceData

) -> bool { + let free_ranges = std::mem::take(&mut self.info.inner.empty_links_list); + let (remaining, changed) = subtract_used_ranges(free_ranges, used_links); + self.info.inner.empty_links_list = remaining; + changed + } } impl SpaceDataOps @@ -100,6 +228,9 @@ where } async fn save_data(&mut self, link: Link, bytes: &[u8]) -> eyre::Result<()> { + if self.consume_reusable_ranges([link]) { + self.save_info().await?; + } if link.page_id > self.last_page_id.into() { let mut page = GeneralPage { header: GeneralHeader::new(link.page_id, PageType::Data, 0.into()), @@ -123,6 +254,11 @@ where } async fn save_batch_data(&mut self, batch_data: BatchData) -> eyre::Result<()> { + let used_links = batch_data.values().flat_map(|ops| ops.iter().map(|(link, _)| *link)); + if self.consume_reusable_ranges(used_links) { + self.save_info().await?; + } + let page_ids = batch_data.keys().map(|id| (*id).into()).collect::>(); let ids_to_create = page_ids .iter() @@ -182,6 +318,40 @@ where Ok(()) } + async fn reclaim_data_pages(&mut self, page_ids: Vec) -> eyre::Result<()> { + let page_ids = page_ids + .into_iter() + .filter(|page_id| { + let id: u32 = (*page_id).into(); + id != 0 && id <= self.last_page_id + }) + .collect::>(); + + if page_ids.is_empty() { + return Ok(()); + } + + self.info + .inner + .empty_links_list + .retain(|link| !page_ids.contains(&link.page_id)); + let mut page_ids = page_ids.into_iter().collect::>(); + page_ids.sort_unstable(); + self.info + .inner + .empty_links_list + .extend(page_ids.into_iter().map(|page_id| Link { + page_id, + offset: 0, + length: INNER_PAGE_SIZE as u32, + })); + self.info.inner.empty_links_list.sort_by_key(|link| { + let page_id: u32 = link.page_id.into(); + (page_id, link.offset) + }); + self.save_info().await + } + fn get_mut_info(&mut self) -> &mut GeneralPage> { &mut self.info } @@ -195,3 +365,82 @@ where Ok(()) } } + +#[cfg(test)] +mod tests { + use data_bucket::page::PageId; + + use super::subtract_used_ranges; + use crate::prelude::Link; + + fn link(page_id: u32, offset: u32, length: u32) -> Link { + Link { + page_id: PageId::from(page_id), + offset, + length, + } + } + + #[test] + fn reusable_ranges_are_subtracted_in_one_sorted_scan() { + let free = vec![link(2, 0, 50), link(1, 0, 100)]; + let used = vec![link(1, 40, 20), link(2, 0, 10), link(1, 10, 20), link(1, 25, 30)]; + + let (remaining, changed) = subtract_used_ranges(free, used); + + assert!(changed); + assert_eq!(remaining, vec![link(1, 0, 10), link(1, 60, 40), link(2, 10, 40)]); + } + + #[test] + fn non_overlapping_used_ranges_leave_free_ranges_unchanged() { + let free = vec![link(1, 0, 10), link(1, 20, 10), link(2, 0, 10)]; + let used = vec![link(1, 10, 10), link(3, 0, 10)]; + + let (remaining, changed) = subtract_used_ranges(free.clone(), used); + + assert!(!changed); + assert_eq!(remaining, free); + } + + #[test] + fn randomized_subtraction_matches_byte_level_coverage() { + const PAGES: usize = 4; + const BYTES: usize = 64; + let mut rng = fastrand::Rng::with_seed(0x51ce_5eed); + + for case in 0..1_000 { + let mut free = Vec::new(); + let mut used = Vec::new(); + let mut expected = [[false; BYTES]; PAGES]; + + for _ in 0..rng.usize(0..20) { + let page = rng.usize(0..PAGES); + let start = rng.usize(0..BYTES); + let end = rng.usize(start + 1..=BYTES); + free.push(link((page + 1) as u32, start as u32, (end - start) as u32)); + expected[page][start..end].fill(true); + } + for _ in 0..rng.usize(0..20) { + let page = rng.usize(0..PAGES); + let start = rng.usize(0..BYTES); + let end = rng.usize(start + 1..=BYTES); + used.push(link((page + 1) as u32, start as u32, (end - start) as u32)); + expected[page][start..end].fill(false); + } + + let (remaining, _) = subtract_used_ranges(free, used); + let mut actual = [[false; BYTES]; PAGES]; + for range in remaining { + let page: u32 = range.page_id.into(); + let page = page as usize - 1; + let start = range.offset as usize; + let end = start + range.length as usize; + assert!(actual[page][start..end].iter().all(|occupied| !occupied), "case {case}"); + actual[page][start..end].fill(true); + } + + assert_eq!(actual, expected, "case {case}"); + } + } +} diff --git a/src/persistence/space/logical_index.rs b/src/persistence/space/logical_index.rs new file mode 100644 index 00000000..7f8fd860 --- /dev/null +++ b/src/persistence/space/logical_index.rs @@ -0,0 +1,482 @@ +//! Background logical-to-structural CDC translation for WorkTablesIndex. +//! +//! The disk shadow is reconstructed from the existing DataBucket index pages. +//! Foreground tables can therefore emit compact logical Set/Remove events while +//! this persistence-worker-owned index derives the structural events required +//! by the unchanged WTI disk format. + +use std::fmt::Debug; +use std::hash::Hash; +use std::path::{Path, PathBuf}; + +use data_bucket::{Link, SizeMeasurable, SpaceId, VariableSizeMeasurable}; +use indexset::cdc::change::ChangeEvent; +use indexset::concurrent::map::BTreeMap; +use indexset::core::node::NodeLike; +use indexset::core::pair::Pair; +use rkyv::de::Pool; +use rkyv::rancor::Strategy; +use rkyv::ser::Serializer; +use rkyv::ser::allocator::ArenaHandle; +use rkyv::ser::sharing::Share; +use rkyv::util::AlignedVec; +use rkyv::{Archive, Deserialize, Serialize, rancor}; +use tokio::fs::File; + +use crate::UnsizedNode; +use crate::persistence::space::BatchChangeEvent; +use crate::persistence::{PersistenceIndexCorruption, SpaceIndex, SpaceIndexOps, SpaceIndexUnsized}; +use crate::prelude::WT_INDEX_EXTENSION; + +fn translate_logical_event( + index_path: &Path, + shadow: &BTreeMap, + event: ChangeEvent>, +) -> Result>>, PersistenceIndexCorruption> +where + T: Debug + Eq + Hash + Clone + Send + Ord + 'static, + Node: NodeLike> + Send + 'static, +{ + match event { + ChangeEvent::InsertAt { + max_value, + value, + index: 0, + .. + } if max_value.key == value.key && max_value.value == value.value => { + let (_, events) = shadow.insert_cdc(value.key, value.value); + Ok(events) + } + ChangeEvent::RemoveAt { + max_value, + value, + index: 0, + .. + } if max_value.key == value.key && max_value.value == value.value => { + let found = shadow.get(&value.key).map(|entry| entry.get().value); + if found != Some(value.value) { + return Err(PersistenceIndexCorruption::new( + index_path, + format!( + "logical WTI shadow diverged while removing key {:?}: expected {:?}, found {:?}", + value.key, value.value, found, + ), + )); + } + let (_, events) = shadow.remove_cdc(&value.key); + Ok(events) + } + _ => Err(PersistenceIndexCorruption::new( + index_path, + "logical WTI persistence received a structural or malformed event", + )), + } +} + +fn translate_logical_batch( + index_path: &Path, + shadow: &BTreeMap, + mut events: BatchChangeEvent, +) -> Result, PersistenceIndexCorruption> +where + T: Debug + Eq + Hash + Clone + Send + Ord + 'static, + Node: NodeLike> + Send + 'static, +{ + // BatchOperation already sorts every per-index stream by event id before + // dispatch. Sort again at this logical/structural boundary so direct + // SpaceIndexOps callers and future batching changes cannot reorder a + // same-key Set/Remove pair after the foreground stripe guard is released. + events.sort_by_key(ChangeEvent::id); + let mut structural = Vec::new(); + for event in events { + structural.extend(translate_logical_event(index_path, shadow, event)?); + } + Ok(structural) +} + +/// Sized-key WTI persistence with foreground logical CDC and a background +/// structural shadow. The wrapped `SpaceIndex` retains the existing file +/// layout byte-for-byte. +pub struct SpaceLogicalIndex +where + T: Send + Ord + Eq + Clone + 'static, +{ + index_path: PathBuf, + shadow: BTreeMap, + disk: SpaceIndex, +} + +impl Debug for SpaceLogicalIndex +where + T: Send + Ord + Eq + Clone + 'static, +{ + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.debug_struct("SpaceLogicalIndex").finish_non_exhaustive() + } +} + +impl SpaceLogicalIndex +where + T: Archive + + Ord + + Eq + + Hash + + Clone + + Default + + Debug + + SizeMeasurable + + for<'a> Serialize, Share>, rancor::Error>> + + Send + + Sync + + 'static, + ::Archived: Deserialize> + + Ord + + Eq + + Debug + + for<'a> rkyv::bytecheck::CheckBytes>, +{ + async fn new(path: String, version: u32) -> eyre::Result { + let index_path = PathBuf::from(&path); + let mut disk = SpaceIndex::new(path, SpaceId::from(0), version).await?; + let shadow = disk.parse_indexset().await?; + Ok(Self { + index_path, + shadow, + disk, + }) + } +} + +impl SpaceIndexOps for SpaceLogicalIndex +where + T: Archive + + Ord + + Eq + + Hash + + Clone + + Default + + Debug + + SizeMeasurable + + for<'a> Serialize, Share>, rancor::Error>> + + Send + + Sync + + 'static, + ::Archived: Deserialize> + + Ord + + Eq + + Debug + + for<'a> rkyv::bytecheck::CheckBytes>, +{ + async fn primary_from_table_files_path + Send>(path: S, version: u32) -> eyre::Result { + Self::new(format!("{}/primary{}", path.as_ref(), WT_INDEX_EXTENSION), version).await + } + + async fn secondary_from_table_files_path + Send, S2: AsRef + Send>( + path: S1, + name: S2, + version: u32, + ) -> eyre::Result { + Self::new( + format!("{}/{}{}", path.as_ref(), name.as_ref(), WT_INDEX_EXTENSION), + version, + ) + .await + } + + async fn bootstrap(file: &mut File, table_name: String, version: u32) -> eyre::Result<()> { + SpaceIndex::::bootstrap(file, table_name, version).await + } + + async fn process_change_event(&mut self, event: ChangeEvent>) -> eyre::Result<()> { + let events = translate_logical_event(&self.index_path, &self.shadow, event)?; + self.disk.process_change_event_batch(events).await + } + + async fn process_change_event_batch(&mut self, events: BatchChangeEvent) -> eyre::Result<()> { + let events = translate_logical_batch(&self.index_path, &self.shadow, events)?; + self.disk.process_change_event_batch(events).await + } +} + +/// Variable-sized-key counterpart to [`SpaceLogicalIndex`]. +pub struct SpaceLogicalIndexUnsized +where + T: Send + Ord + Eq + Clone + Default + Debug + SizeMeasurable + VariableSizeMeasurable + 'static, +{ + index_path: PathBuf, + shadow: BTreeMap>>, + disk: SpaceIndexUnsized, +} + +impl Debug for SpaceLogicalIndexUnsized +where + T: Send + Ord + Eq + Clone + Default + Debug + SizeMeasurable + VariableSizeMeasurable + 'static, +{ + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("SpaceLogicalIndexUnsized") + .finish_non_exhaustive() + } +} + +impl SpaceLogicalIndexUnsized +where + T: Archive + + Ord + + Eq + + Hash + + Clone + + Default + + Debug + + SizeMeasurable + + VariableSizeMeasurable + + for<'a> Serialize, Share>, rancor::Error>> + + Send + + Sync + + 'static, + ::Archived: Deserialize> + + Ord + + Eq + + Debug + + for<'a> rkyv::bytecheck::CheckBytes>, +{ + async fn new(path: String, version: u32) -> eyre::Result { + let index_path = PathBuf::from(&path); + let mut disk = SpaceIndexUnsized::new(path, SpaceId::from(0), version).await?; + let shadow = disk.parse_indexset().await?; + Ok(Self { + index_path, + shadow, + disk, + }) + } +} + +impl SpaceIndexOps for SpaceLogicalIndexUnsized +where + T: Archive + + Ord + + Eq + + Hash + + Clone + + Default + + Debug + + SizeMeasurable + + VariableSizeMeasurable + + for<'a> Serialize, Share>, rancor::Error>> + + Send + + Sync + + 'static, + ::Archived: Deserialize> + + Ord + + Eq + + Debug + + for<'a> rkyv::bytecheck::CheckBytes>, +{ + async fn primary_from_table_files_path + Send>(path: S, version: u32) -> eyre::Result { + Self::new(format!("{}/primary{}", path.as_ref(), WT_INDEX_EXTENSION), version).await + } + + async fn secondary_from_table_files_path + Send, S2: AsRef + Send>( + path: S1, + name: S2, + version: u32, + ) -> eyre::Result { + Self::new( + format!("{}/{}{}", path.as_ref(), name.as_ref(), WT_INDEX_EXTENSION), + version, + ) + .await + } + + async fn bootstrap(file: &mut File, table_name: String, version: u32) -> eyre::Result<()> { + SpaceIndexUnsized::::bootstrap(file, table_name, version).await + } + + async fn process_change_event(&mut self, event: ChangeEvent>) -> eyre::Result<()> { + let events = translate_logical_event(&self.index_path, &self.shadow, event)?; + self.disk.process_change_event_batch(events).await + } + + async fn process_change_event_batch(&mut self, events: BatchChangeEvent) -> eyre::Result<()> { + let events = translate_logical_batch(&self.index_path, &self.shadow, events)?; + self.disk.process_change_event_batch(events).await + } +} + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap as StdBTreeMap; + + use data_bucket::page::PageId; + + use super::*; + + fn link(offset: u32) -> Link { + Link { + page_id: PageId::from(1), + offset, + length: 8, + } + } + + #[test] + fn logical_events_rebuild_structural_events_on_the_shadow() { + let shadow = BTreeMap::::default(); + let pair = Pair { key: 7, value: link(7) }; + let events = translate_logical_event( + Path::new("test.wt.idx"), + &shadow, + ChangeEvent::InsertAt { + event_id: 0.into(), + max_value: pair.clone(), + value: pair, + index: 0, + }, + ) + .unwrap(); + assert!(!events.is_empty()); + assert_eq!(shadow.get(&7).map(|entry| entry.get().value), Some(link(7))); + } + + #[test] + fn logical_event_requires_an_exact_key_and_link_sentinel() { + let shadow = BTreeMap::::default(); + let result = translate_logical_event( + Path::new("test.wt.idx"), + &shadow, + ChangeEvent::InsertAt { + event_id: 0.into(), + max_value: Pair { key: 7, value: link(8) }, + value: Pair { key: 7, value: link(7) }, + index: 0, + }, + ); + + assert!(result.is_err()); + assert!(shadow.get(&7).is_none()); + } + + #[test] + fn logical_batch_restores_event_id_order_after_reversed_delivery() { + let shadow = BTreeMap::::default(); + let pair = Pair { key: 7, value: link(7) }; + let insert = ChangeEvent::InsertAt { + event_id: 0.into(), + max_value: pair.clone(), + value: pair.clone(), + index: 0, + }; + let remove = ChangeEvent::RemoveAt { + event_id: 1.into(), + max_value: pair.clone(), + value: pair, + index: 0, + }; + + let structural = translate_logical_batch(Path::new("test.wt.idx"), &shadow, vec![remove, insert]).unwrap(); + + assert!(!structural.is_empty()); + assert!(shadow.get(&7).is_none()); + } + + #[test] + fn divergence_is_typed_and_does_not_mutate_the_shadow() { + let shadow = BTreeMap::::default(); + shadow.insert(7, link(7)); + let pair = Pair { key: 7, value: link(8) }; + + let error = translate_logical_event( + Path::new("test.wt.idx"), + &shadow, + ChangeEvent::RemoveAt { + event_id: 1.into(), + max_value: pair.clone(), + value: pair, + index: 0, + }, + ) + .unwrap_err(); + + assert_eq!(error.path(), Path::new("test.wt.idx")); + assert!(error.reason().contains("shadow diverged")); + assert_eq!(shadow.get(&7).map(|entry| entry.get().value), Some(link(7))); + } + + #[test] + fn logical_set_replacement_derives_the_new_structural_link() { + let shadow = BTreeMap::::default(); + shadow.insert(7, link(7)); + let replacement = Pair { key: 7, value: link(8) }; + + let structural = translate_logical_event( + Path::new("test.wt.idx"), + &shadow, + ChangeEvent::InsertAt { + event_id: 1.into(), + max_value: replacement.clone(), + value: replacement, + index: 0, + }, + ) + .unwrap(); + + assert!(!structural.is_empty()); + assert_eq!(shadow.get(&7).map(|entry| entry.get().value), Some(link(8))); + } + + #[test] + fn shuffled_large_logical_batch_replays_in_event_id_order() { + let shadow = BTreeMap::::default(); + let mut expected = StdBTreeMap::::new(); + let mut events = Vec::new(); + + for event_id in 0_u64..2_000 { + let key = event_id.wrapping_mul(17) % 97; + let event = if event_id % 5 == 0 { + if let Some(old_link) = expected.remove(&key) { + let pair = Pair { key, value: old_link }; + ChangeEvent::RemoveAt { + event_id: event_id.into(), + max_value: pair.clone(), + value: pair, + index: 0, + } + } else { + let new_link = link(event_id as u32); + expected.insert(key, new_link); + let pair = Pair { key, value: new_link }; + ChangeEvent::InsertAt { + event_id: event_id.into(), + max_value: pair.clone(), + value: pair, + index: 0, + } + } + } else { + let new_link = link(event_id as u32); + expected.insert(key, new_link); + let pair = Pair { key, value: new_link }; + ChangeEvent::InsertAt { + event_id: event_id.into(), + max_value: pair.clone(), + value: pair, + index: 0, + } + }; + events.push(event); + } + + fastrand::Rng::with_seed(0x10_91ca1).shuffle(&mut events); + let structural = translate_logical_batch(Path::new("test.wt.idx"), &shadow, events).unwrap(); + + assert!(!structural.is_empty()); + for key in 0..97 { + assert_eq!( + shadow.get(&key).map(|entry| entry.get().value), + expected.get(&key).copied(), + "key {key}" + ); + } + } +} diff --git a/src/persistence/space/mod.rs b/src/persistence/space/mod.rs index dee296f3..843ca8b0 100644 --- a/src/persistence/space/mod.rs +++ b/src/persistence/space/mod.rs @@ -1,6 +1,7 @@ mod art_index; mod data; mod index; +mod logical_index; use std::collections::HashMap; use std::future::Future; @@ -18,6 +19,7 @@ pub use index::{ IndexTableOfContents, SpaceIndex, SpaceIndexUnsized, map_index_pages_to_toc_and_general, map_unsized_index_pages_to_toc_and_general, reconstruct_multi_index_nodes, }; +pub use logical_index::{SpaceLogicalIndex, SpaceLogicalIndexUnsized}; pub type BatchData = HashMap)>>; @@ -33,6 +35,7 @@ pub trait SpaceDataOps { fn bootstrap(file: &mut File, table_name: String, version: u32) -> impl Future> + Send; fn save_data(&mut self, link: Link, bytes: &[u8]) -> impl Future> + Send; fn save_batch_data(&mut self, batch_data: BatchData) -> impl Future> + Send; + fn reclaim_data_pages(&mut self, page_ids: Vec) -> impl Future> + Send; fn get_mut_info(&mut self) -> &mut GeneralPage>; fn save_info(&mut self) -> impl Future> + Send; } diff --git a/src/persistence/task.rs b/src/persistence/task.rs index 578fe75f..a80a3a78 100644 --- a/src/persistence/task.rs +++ b/src/persistence/task.rs @@ -13,7 +13,9 @@ use tokio::task::JoinHandle; use worktable_codegen::worktable; use crate::persistence::operation::{BatchInnerRow, BatchInnerWorkTable, BatchOperation, OperationId, PosByOpIdQuery}; -use crate::persistence::{PersistenceEngine, PersistenceError, PersistenceResult, PersistenceState}; +use crate::persistence::{ + PersistenceEngine, PersistenceError, PersistenceIndexCorruption, PersistenceResult, PersistenceState, +}; use crate::prelude::*; use crate::util::OptimizedVec; use crate::vacuum::VacuumPersistence; @@ -81,7 +83,10 @@ impl PersistenceLifecycle { let error = match &*state { PersistenceState::Failed(error) => error.clone(), _ => { - let error = Arc::new(PersistenceError::Engine(report)); + let error = Arc::new(match report.downcast::() { + Ok(corruption) => PersistenceError::IndexCorruption(corruption), + Err(report) => PersistenceError::Engine(report), + }); *state = PersistenceState::Failed(error.clone()); error } @@ -396,8 +401,16 @@ mod lifecycle_tests { struct TestEngine { batches: Arc, + events: Arc>>, config: TestConfig, - fail: bool, + failure: TestFailure, + } + + #[derive(Clone, Copy)] + enum TestFailure { + None, + Engine, + IndexCorruption, } impl PersistenceEngine<(), u64, TestEvents, TestIndex> for TestEngine { @@ -406,8 +419,9 @@ mod lifecycle_tests { async fn new(config: Self::Config) -> eyre::Result { Ok(Self { batches: Arc::new(AtomicUsize::new(0)), + events: Arc::new(ParkingMutex::new(Vec::new())), config, - fail: false, + failure: TestFailure::None, }) } @@ -419,10 +433,22 @@ mod lifecycle_tests { &mut self, _batch_op: BatchOperation<(), u64, TestEvents, TestIndex>, ) -> eyre::Result<()> { - if self.fail { - return Err(eyre::eyre!("injected batch failure")); + match self.failure { + TestFailure::None => {} + TestFailure::Engine => return Err(eyre::eyre!("injected batch failure")), + TestFailure::IndexCorruption => { + return Err( + PersistenceIndexCorruption::new("table/primary.wt.idx", "injected shadow divergence").into(), + ); + } } self.batches.fetch_add(1, Ordering::Relaxed); + self.events.lock().push("batch"); + Ok(()) + } + + async fn reclaim_data_pages(&mut self, _page_ids: Vec) -> eyre::Result<()> { + self.events.lock().push("reclaim"); Ok(()) } @@ -451,8 +477,9 @@ mod lifecycle_tests { let batches = Arc::new(AtomicUsize::new(0)); let task = PersistenceTask::run_engine(TestEngine { batches: batches.clone(), + events: Arc::new(ParkingMutex::new(Vec::new())), config: TestConfig, - fail: false, + failure: TestFailure::None, }); task.apply_operation(insert_operation(1)).unwrap(); @@ -465,8 +492,9 @@ mod lifecycle_tests { async fn engine_failure_is_terminal_and_reused_for_later_callers() { let task = PersistenceTask::run_engine(TestEngine { batches: Arc::new(AtomicUsize::new(0)), + events: Arc::new(ParkingMutex::new(Vec::new())), config: TestConfig, - fail: true, + failure: TestFailure::Engine, }); task.apply_operation(insert_operation(1)).unwrap(); @@ -479,6 +507,68 @@ mod lifecycle_tests { let close_error = task.close().await.unwrap_err(); assert!(Arc::ptr_eq(&wait_error, &close_error)); } + + #[tokio::test] + async fn vacuum_reclamation_waits_for_preceding_row_moves() { + let events = Arc::new(ParkingMutex::new(Vec::new())); + let task = PersistenceTask::run_engine(TestEngine { + batches: Arc::new(AtomicUsize::new(0)), + events: events.clone(), + config: TestConfig, + failure: TestFailure::None, + }); + + task.apply_operation(insert_operation(1)).unwrap(); + task.queue.reclaim_pages(vec![1.into()]).unwrap(); + task.apply_operation(insert_operation(2)).unwrap(); + task.wait_for_ops().await.unwrap(); + + assert_eq!(&*events.lock(), &["batch", "reclaim", "batch"]); + } + + #[tokio::test] + async fn index_corruption_from_engine_is_typed_and_terminal_for_callers() { + let task = PersistenceTask::run_engine(TestEngine { + batches: Arc::new(AtomicUsize::new(0)), + events: Arc::new(ParkingMutex::new(Vec::new())), + config: TestConfig, + failure: TestFailure::IndexCorruption, + }); + + task.apply_operation(insert_operation(1)).unwrap(); + let wait_error = task.wait_for_ops().await.unwrap_err(); + assert!(matches!(wait_error.as_ref(), PersistenceError::IndexCorruption(_))); + + let intake_error = task.apply_operation(insert_operation(2)).unwrap_err(); + assert!(Arc::ptr_eq(&wait_error, &intake_error)); + let close_error = task.close().await.unwrap_err(); + assert!(Arc::ptr_eq(&wait_error, &close_error)); + } + + #[test] + fn typed_index_corruption_quarantines_the_persistence_lifecycle() { + let lifecycle = PersistenceLifecycle::new(); + let error = lifecycle.fail( + PersistenceIndexCorruption::new("table/primary.wt.idx", "shadow diverged from logical stream").into(), + ); + + match error.as_ref() { + PersistenceError::IndexCorruption(corruption) => { + assert_eq!(corruption.path(), std::path::Path::new("table/primary.wt.idx")); + assert!(corruption.reason().contains("shadow diverged")); + } + other => panic!("expected typed index corruption, got {other:?}"), + } + + let intake_error = lifecycle.ensure_running().unwrap_err(); + assert!(Arc::ptr_eq(&error, &intake_error)); + } +} + +#[derive(Debug)] +enum PersistenceMessage { + Operation(Operation), + ReclaimPages(Vec), } #[derive(Debug)] @@ -487,7 +577,7 @@ pub struct Queue { // element type via `mem::uninitialized`, which aborts at runtime for // `Operation` layouts that reject uninit bytes. The queue has a single // consumer (the engine task), so a mutexed deque is uncontended here. - queue: ParkingMutex>>, + queue: ParkingMutex>>, notify: Notify, // usize, not u16: the queue is unbounded and a 16-bit counter wraps at // 65_536 queued operations, making the wait triggers see an "empty" @@ -507,6 +597,13 @@ impl Queue) -> PersistenceResult { + self.push_message(PersistenceMessage::Operation(value)) + } + + fn push_message( + &self, + value: PersistenceMessage, + ) -> PersistenceResult { let state = self.lifecycle.state.lock(); match &*state { PersistenceState::Running => {} @@ -525,10 +622,10 @@ impl Queue Option> { + ) -> Option> { loop { let notified = self.notify.notified(); // Drain values @@ -554,7 +651,7 @@ impl Queue Option> { + fn immediate_pop(&self) -> Option> { if let Some(v) = self.queue.lock().pop_front() { self.len.fetch_sub(1, Ordering::Release); Some(v) @@ -563,10 +660,6 @@ impl Queue impl Iterator> { - std::iter::from_fn(|| self.immediate_pop()) - } - pub fn len(&self) -> usize { self.len.load(Ordering::Acquire) } @@ -594,6 +687,10 @@ where link: new_link, })) } + + fn reclaim_pages(&self, page_ids: Vec) -> PersistenceResult { + self.push_message(PersistenceMessage::ReclaimPages(page_ids)) + } } #[derive(Debug)] @@ -665,9 +762,9 @@ impl /// Returns the current physical size of the table data file. /// - /// This is intentionally separate from `VacuumStats`: vacuum currently - /// compacts and reuses in-memory pages but does not truncate `.wt.data`. - /// Operators can sample this value to observe physical growth. + /// This is intentionally separate from `VacuumStats`: online vacuum makes + /// freed pages durably reusable, but does not truncate `.wt.data`. + /// Operators can sample this value to observe physical growth and reuse. pub async fn persisted_data_file_size_bytes(&self) -> std::io::Result { tokio::fs::metadata(format!( "{}/{}", @@ -709,10 +806,16 @@ impl let task_analyzer_in_progress = analyzer_in_progress.clone(); let task = async move { + let mut pending_reclaim: Option> = None; loop { - let op = if let Some(next_op) = engine_queue.immediate_pop() { - Some(next_op) - } else if analyzer.len() == 0 { + let message = if pending_reclaim.is_none() { + engine_queue.immediate_pop() + } else { + None + }; + let message = if message.is_some() { + message + } else if analyzer.len() == 0 && pending_reclaim.is_none() { task_analyzer_in_progress.store(false, Ordering::Release); engine_lifecycle.notify.notify_waiters(); if matches!(engine_lifecycle.state(), PersistenceState::Closing) { @@ -726,17 +829,35 @@ impl } else { None }; - if let Some(op) = op - && let Err(err) = analyzer.push(op.clone()) - { - engine_lifecycle.fail(err); - return; + + if let Some(message) = message { + match message { + PersistenceMessage::Operation(op) => { + if let Err(err) = analyzer.push(op) { + engine_lifecycle.fail(err); + return; + } + } + PersistenceMessage::ReclaimPages(page_ids) => pending_reclaim = Some(page_ids), + } } - let ops_available_iter = engine_queue.pop_iter(); - if let Err(err) = analyzer.extend_from_iter(ops_available_iter) { - engine_lifecycle.fail(err); - return; + + // Pull operations up to, but never past, a reclamation + // barrier. This gives the analyzer every CDC event required + // for a batch while preserving FIFO ordering for maintenance. + while pending_reclaim.is_none() { + match engine_queue.immediate_pop() { + Some(PersistenceMessage::Operation(op)) => { + if let Err(err) = analyzer.push(op) { + engine_lifecycle.fail(err); + return; + } + } + Some(PersistenceMessage::ReclaimPages(page_ids)) => pending_reclaim = Some(page_ids), + None => break, + } } + if let Some(op_id) = analyzer.get_first_op_id_available() { let batch_op = analyzer.collect_batch_from_op_id(op_id).await; if let Err(e) = batch_op { @@ -751,6 +872,23 @@ impl } else { tokio::time::sleep(Duration::from_millis(500)).await; } + } else if let Some(page_ids) = pending_reclaim.take() { + // `get_first_op_id_available() == None` is only sufficient + // when the analyzer itself is empty. If its operation-id + // index ever loses an entry, reclaiming here would make a + // source page reusable before its buffered row move became + // durable. Fail terminally instead of trusting that state. + let buffered_operations = analyzer.len(); + if buffered_operations != 0 { + engine_lifecycle.fail(eyre::eyre!( + "persistence reclamation barrier found {buffered_operations} buffered operations without an operation-id index entry" + )); + return; + } + if let Err(error) = engine.reclaim_data_pages(page_ids).await { + engine_lifecycle.fail(error); + return; + } } } }; diff --git a/src/table/vacuum/mod.rs b/src/table/vacuum/mod.rs index bcfb4148..727ceedd 100644 --- a/src/table/vacuum/mod.rs +++ b/src/table/vacuum/mod.rs @@ -1,6 +1,7 @@ use async_trait::async_trait; use data_bucket::Link; +use data_bucket::page::PageId; use indexset::cdc::change::ChangeEvent; use indexset::core::pair::Pair; @@ -38,6 +39,10 @@ pub trait VacuumPersistence: Send + Sync { primary_key_events: Vec>>, secondary_keys_events: SecondaryEvents, ) -> PersistenceResult; + + /// Queue a barrier that makes pages reusable only after all preceding row + /// moves have become durable. + fn reclaim_pages(&self, page_ids: Vec) -> PersistenceResult; } /// Trait for unifying different [`WorkTable`] related [`EmptyDataVacuum`]'s. diff --git a/src/table/vacuum/vacuum.rs b/src/table/vacuum/vacuum.rs index a2fced84..e555c0ae 100644 --- a/src/table/vacuum/vacuum.rs +++ b/src/table/vacuum/vacuum.rs @@ -202,13 +202,24 @@ where // expose the whole page for reuse. Otherwise a concurrent insert // could claim a stale fragment between retirement and cleanup. registry.remove_link_for_page(page_from); + if let Some(persistence) = &self.persistence { + // Queue the durable-free marker before publishing this page to + // in-memory allocators. Any concurrent reuse is then ordered + // after the marker and consumes the durable free range again. + persistence.reclaim_pages(vec![page_from])?; + } self.data_pages.mark_page_empty(page_from); pages_freed += 1; } pages_freed += free_pages.len(); + if let Some(persistence) = &self.persistence + && !free_pages.is_empty() + { + persistence.reclaim_pages(free_pages.iter().copied().collect())?; + } for id in free_pages { - self.data_pages.mark_page_empty(id) + self.data_pages.mark_page_empty(id); } for id in defragmented_pages { self.data_pages.mark_page_full(id) diff --git a/tests/persistence/mod.rs b/tests/persistence/mod.rs index 4425022b..7dc62be6 100644 --- a/tests/persistence/mod.rs +++ b/tests/persistence/mod.rs @@ -9,6 +9,7 @@ mod failure; mod index_page; mod loaded_index_growth; mod read; +mod recovery_load; mod schema; mod space_index; mod sync; diff --git a/tests/persistence/recovery_load.rs b/tests/persistence/recovery_load.rs new file mode 100644 index 00000000..d3e06423 --- /dev/null +++ b/tests/persistence/recovery_load.rs @@ -0,0 +1,136 @@ +use std::collections::BTreeSet; + +use crate::remove_dir_if_exists; +use worktable::prelude::*; +use worktable::worktable; + +worktable!( + name: RecoveryLoad, + persist: true, + columns: { + id: String primary_key, + project_id: String, + body: String, + }, + indexes: { + project_idx: project_id, + }, +); + +const DIR: &str = "tests/data/recovery_load/persisted"; +const CORRUPT_DIR: &str = "tests/data/recovery_load/corrupt_row"; + +fn config(dir: &str) -> DiskConfig { + DiskConfig::new_with_table_name( + dir, + RecoveryLoadWorkTable::name_snake_case(), + RecoveryLoadWorkTable::version(), + ) +} + +async fn engine(dir: &str) -> RecoveryLoadPersistenceEngine { + RecoveryLoadPersistenceEngine::new(config(dir)).await.unwrap() +} + +fn row(id: &str, project_id: &str) -> RecoveryLoadRow { + RecoveryLoadRow { + id: id.to_owned(), + project_id: project_id.to_owned(), + body: format!("body-{id}"), + } +} + +#[tokio::test] +async fn recovery_mode_reads_valid_rows_through_a_surviving_secondary_index() { + remove_dir_if_exists(DIR.to_owned()).await; + + let table = RecoveryLoadWorkTable::load(engine(DIR).await).await.unwrap(); + table.insert(row("row-1", "project-a")).unwrap(); + table.insert(row("row-2", "project-a")).unwrap(); + table.insert(row("row-3", "project-b")).unwrap(); + table.close().await.unwrap(); + + let table_dir = format!("{DIR}/{}", RecoveryLoadWorkTable::name_snake_case()); + let primary_path = format!("{table_dir}/primary{WT_INDEX_EXTENSION}"); + tokio::fs::rename(&primary_path, format!("{primary_path}.damaged")) + .await + .unwrap(); + + let error = RecoveryLoadWorkTable::load(engine(DIR).await).await.unwrap_err(); + let typed = error + .downcast_ref::() + .expect("strict load must return a typed corruption error"); + assert!( + typed.reason().contains("project_idx"), + "unexpected strict-load reason: {}", + typed.reason() + ); + + let table = RecoveryLoadWorkTable::load_with(engine(DIR).await, LoadMode::Recovery) + .await + .unwrap(); + assert!(table.select("row-1".to_owned()).is_none()); + + let project_a: BTreeSet<_> = table + .select_by_project_id("project-a".to_owned()) + .execute() + .unwrap() + .into_iter() + .map(|row| row.id) + .collect(); + assert_eq!(project_a, BTreeSet::from(["row-1".to_owned(), "row-2".to_owned()])); + + let project_b: BTreeSet<_> = table + .select_by_project_id("project-b".to_owned()) + .execute() + .unwrap() + .into_iter() + .map(|row| row.id) + .collect(); + assert_eq!(project_b, BTreeSet::from(["row-3".to_owned()])); + + table.close().await.unwrap(); + remove_dir_if_exists(DIR.to_owned()).await; +} + +#[tokio::test] +async fn recovery_mode_rejects_corrupt_rows_reached_through_a_secondary_index() { + remove_dir_if_exists(CORRUPT_DIR.to_owned()).await; + + let table = RecoveryLoadWorkTable::load(engine(CORRUPT_DIR).await).await.unwrap(); + let id = table.insert(row("row-corrupt", "project-a")).unwrap(); + let link = table.0.primary_index.pk_map.get_value(&id).unwrap().0; + table.close().await.unwrap(); + + let table_dir = format!("{CORRUPT_DIR}/{}", RecoveryLoadWorkTable::name_snake_case()); + let primary_path = format!("{table_dir}/primary{WT_INDEX_EXTENSION}"); + tokio::fs::rename(&primary_path, format!("{primary_path}.damaged")) + .await + .unwrap(); + + let data_path = format!("{table_dir}/{WT_DATA_EXTENSION}"); + let page_id: u32 = link.page_id.into(); + let byte_offset = u64::from(page_id) * PAGE_SIZE as u64 + GENERAL_HEADER_SIZE as u64 + u64::from(link.offset); + { + use std::io::{Seek, SeekFrom, Write}; + + let mut file = std::fs::OpenOptions::new().write(true).open(data_path).unwrap(); + file.seek(SeekFrom::Start(byte_offset)).unwrap(); + file.write_all(&vec![0; link.length as usize]).unwrap(); + file.sync_all().unwrap(); + } + + let error = RecoveryLoadWorkTable::load_with(engine(CORRUPT_DIR).await, LoadMode::Recovery) + .await + .unwrap_err(); + let typed = error + .downcast_ref::() + .expect("recovery must return a typed corruption error"); + assert!( + typed.reason().contains("project_idx") && typed.reason().contains("key does not match"), + "unexpected recovery-load reason: {}", + typed.reason() + ); + + remove_dir_if_exists(CORRUPT_DIR.to_owned()).await; +} diff --git a/tests/persistence/vacuum.rs b/tests/persistence/vacuum.rs index 9ea77bcd..3a9ba923 100644 --- a/tests/persistence/vacuum.rs +++ b/tests/persistence/vacuum.rs @@ -47,6 +47,7 @@ fn test_vacuum_on_persisted_table_survives_reload() { let mut rows = HashMap::new(); let deleted: Vec; + let reused_after_reload_id: u64; { let engine = VacuumPersistPersistenceEngine::new(config.clone()).await.unwrap(); let table = VacuumPersistWorkTable::load(engine).await.unwrap(); @@ -88,7 +89,7 @@ fn test_vacuum_on_persisted_table_survives_reload() { let physical_bytes_after = table.persisted_data_file_size_bytes().await.unwrap(); assert!( physical_bytes_after >= physical_bytes_before, - "persisted vacuum is logical compaction and must not report implicit file truncation" + "online vacuum may append a relocation page; durable reclamation is reuse, not truncation" ); // Insert after vacuum: these operations carry event ids issued @@ -131,6 +132,41 @@ fn test_vacuum_on_persisted_table_survives_reload() { let engine = VacuumPersistPersistenceEngine::new(config.clone()).await.unwrap(); let table = VacuumPersistWorkTable::load(engine).await.unwrap(); + let physical_bytes_before_reuse = table.persisted_data_file_size_bytes().await.unwrap(); + let durable_free_bytes: u64 = table + .0 + .data + .get_empty_links() + .iter() + .map(|link| u64::from(link.length)) + .sum(); + assert!( + durable_free_bytes > 0, + "vacuum-freed ranges must survive reload so later inserts can reuse them" + ); + + // Exercise reuse after reload. Without durable free-page metadata, + // this insert allocates a new page and grows `.wt.data` again. + let reused_row = VacuumPersistRow { + id: table.get_next_pk().into(), + test: 1_100, + another: 1_100, + exchange: "reused-after-reload".to_string(), + }; + let reused_id = reused_row.id; + reused_after_reload_id = reused_id; + table.insert(reused_row.clone()).unwrap(); + rows.insert(reused_id, reused_row); + timeout(Duration::from_secs(30), table.wait_for_ops()) + .await + .expect("persistence should catch up after durable page reuse") + .expect("persistence engine failed"); + assert_eq!( + table.persisted_data_file_size_bytes().await.unwrap(), + physical_bytes_before_reuse, + "an insert after reload must consume vacuum-freed space instead of extending the file" + ); + assert_eq!(table.select_all().execute().unwrap().len(), rows.len()); for (id, expected) in &rows { assert_eq!(table.select(*id).as_ref(), Some(expected)); @@ -145,5 +181,30 @@ fn test_vacuum_on_persisted_table_survives_reload() { assert_eq!(table.select(*id), None); } } + { + // Reload once more and allocate from the remaining durable range. + // The first reused slot must have been removed from the free + // metadata before its bytes were written, or this insert could + // overwrite it after reopening the table. + let engine = VacuumPersistPersistenceEngine::new(config.clone()).await.unwrap(); + let table = VacuumPersistWorkTable::load(engine).await.unwrap(); + let second_reused_row = VacuumPersistRow { + id: table.get_next_pk().into(), + test: 1_101, + another: 1_101, + exchange: "second-reuse-after-reload".to_string(), + }; + table.insert(second_reused_row).unwrap(); + timeout(Duration::from_secs(30), table.wait_for_ops()) + .await + .expect("persistence should catch up after a second durable page reuse") + .expect("persistence engine failed"); + + assert_eq!( + table.select(reused_after_reload_id).as_ref(), + rows.get(&reused_after_reload_id), + "consumed durable free ranges must not be offered again after another reload" + ); + } }) } diff --git a/tests/worktable/base.rs b/tests/worktable/base.rs index 2c8fdf42..5ca38862 100644 --- a/tests/worktable/base.rs +++ b/tests/worktable/base.rs @@ -1,3 +1,6 @@ +macro_rules! base_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; @@ -8,8 +11,9 @@ use worktable::worktable; worktable! ( name: Test, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, test: i64, another: u64, exchange: String @@ -185,7 +189,7 @@ async fn update_string() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let updated = TestRow { id: pk.clone().into(), test: 2, @@ -265,7 +269,7 @@ async fn delete() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); table.delete(pk.clone()).await.unwrap(); let selected_row = table.select(pk); assert!(selected_row.is_none()); @@ -281,7 +285,7 @@ async fn delete() { exchange: "test".to_string(), }; let pk = table.insert(updated.clone()).unwrap(); - let new_link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let new_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); assert_eq!(link, new_link) } @@ -366,7 +370,7 @@ async fn delete_and_insert_less() { exchange: "test1234567890".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); table.delete(pk.clone()).await.unwrap(); let selected_row = table.select(pk); assert!(selected_row.is_none()); @@ -378,7 +382,7 @@ async fn delete_and_insert_less() { exchange: "test1".to_string(), }; let pk = table.insert(updated.clone()).unwrap(); - let new_link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let new_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); assert_ne!(link.0, new_link.0) } @@ -400,7 +404,7 @@ async fn delete_and_replace() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); table.delete(pk.clone()).await.unwrap(); let selected_row = table.select(pk); assert!(selected_row.is_none()); @@ -412,7 +416,7 @@ async fn delete_and_replace() { exchange: "test".to_string(), }; let pk = table.insert(updated.clone()).unwrap(); - let new_link = table.0.primary_index.pk_map.get(&pk).map(|kv| kv.get().value).unwrap(); + let new_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); assert_eq!(link, new_link) } @@ -1199,3 +1203,11 @@ async fn _bench() { let _ = table.select(a).expect("TODO: panic message"); } } + + } + }; +} + +base_backend_suite!(wti, worktables_index); +base_backend_suite!(congee, congee); +base_backend_suite!(arctic, arctic); diff --git a/tests/worktable/count.rs b/tests/worktable/count.rs index 4b98542c..a6f366a1 100644 --- a/tests/worktable/count.rs +++ b/tests/worktable/count.rs @@ -1,11 +1,15 @@ +macro_rules! count_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use worktable::prelude::*; use worktable::worktable; // The test checks updates for 3 indecies at once worktable!( name: Test, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, val: i64, attr1: String, attr2: i16, @@ -69,3 +73,11 @@ async fn count() { // Count by WT assert_eq!(4, test_table.count()); } + + } + }; +} + +count_backend_suite!(wti, worktables_index); +count_backend_suite!(congee, congee); +count_backend_suite!(arctic, arctic); diff --git a/tests/worktable/float.rs b/tests/worktable/float.rs index a924d343..1b8af9f5 100644 --- a/tests/worktable/float.rs +++ b/tests/worktable/float.rs @@ -1,10 +1,14 @@ +macro_rules! float_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use worktable::prelude::*; use worktable::worktable; worktable! ( name: TestFloat, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, test: i64, another: f64, exchange: String @@ -18,8 +22,9 @@ worktable! ( worktable! ( name: TestUniqueFloat, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, value: f64, }, indexes: { @@ -45,8 +50,8 @@ fn unique_float_point_read_revalidates_the_returned_row() { .0 .primary_index .pk_map - .get(&TestUniqueFloatPrimaryKey(second.id)) - .map(|entry| entry.get().value.0) + .get_value(&TestUniqueFloatPrimaryKey(second.id)) + .map(|value| value.0) .unwrap(); TableIndex::insert(&table.0.indexes.value_idx, OrderedFloat(first.value), second_link); @@ -76,8 +81,8 @@ fn float_range_read_revalidates_each_resolved_row() { .0 .primary_index .pk_map - .get(&TestFloatPrimaryKey(outside.id)) - .map(|entry| entry.get().value.0) + .get_value(&TestFloatPrimaryKey(outside.id)) + .map(|value| value.0) .unwrap(); TableIndex::insert(&table.0.indexes.another_idx, OrderedFloat(15.0), outside_link); @@ -203,3 +208,11 @@ fn select_by_another_range_test() { assert_eq!(results.first().unwrap().another, 50.0); assert_eq!(results.last().unwrap().another, 20.0); } + + } + }; +} + +float_backend_suite!(wti, worktables_index); +float_backend_suite!(congee, congee); +float_backend_suite!(arctic, arctic); diff --git a/tests/worktable/in_place.rs b/tests/worktable/in_place.rs index 38ce26a9..6d82a8ac 100644 --- a/tests/worktable/in_place.rs +++ b/tests/worktable/in_place.rs @@ -1,3 +1,6 @@ +macro_rules! in_place_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; @@ -8,8 +11,9 @@ use worktable::worktable; worktable!( name: Test, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, val: i64, val1: u64, val2: i16, @@ -372,3 +376,11 @@ async fn test_update_in_place_and_update_unsized_multithread() -> eyre::Result<( assert_eq!(errors, 0); Ok(()) } + + } + }; +} + +in_place_backend_suite!(wti, worktables_index); +in_place_backend_suite!(congee, congee); +in_place_backend_suite!(arctic, arctic); diff --git a/tests/worktable/index_backends.rs b/tests/worktable/index_backends.rs index 0924722b..f789ae39 100644 --- a/tests/worktable/index_backends.rs +++ b/tests/worktable/index_backends.rs @@ -455,6 +455,71 @@ async fn native_art_backends_recover_concurrent_same_row_updates() { remove_dir_if_exists(ROOT.to_string()).await; } +#[cfg(feature = "logical-index-persistence")] +#[tokio::test] +async fn logical_wti_recovers_concurrent_same_row_updates() { + use std::sync::Arc; + + use provider_switch_wti as wti; + use tokio::sync::Barrier; + + const ROOT: &str = "tests/data/index_backend_logical_wti_concurrent"; + const WORKERS: u64 = 8; + const UPDATES_PER_WORKER: u64 = 100; + remove_dir_if_exists(ROOT.to_string()).await; + + let config = DiskConfig::new_with_table_name( + ROOT, + wti::ProviderSwitchWorkTable::name_snake_case(), + wti::ProviderSwitchWorkTable::version(), + ); + let engine = wti::ProviderSwitchPersistenceEngine::new(config.clone()).await.unwrap(); + let table = Arc::new(wti::ProviderSwitchWorkTable::load(engine).await.unwrap()); + let id: u64 = table.get_next_pk().into(); + table.insert(wti::ProviderSwitchRow { id, unique_key: 1 }).unwrap(); + + let barrier = Arc::new(Barrier::new(WORKERS as usize + 1)); + let mut workers = Vec::new(); + for worker in 0..WORKERS { + let table = Arc::clone(&table); + let barrier = Arc::clone(&barrier); + workers.push(tokio::spawn(async move { + barrier.wait().await; + for update in 0..UPDATES_PER_WORKER { + table + .update(wti::ProviderSwitchRow { + id, + unique_key: 10_000 + worker * UPDATES_PER_WORKER + update, + }) + .await + .unwrap(); + } + })); + } + barrier.wait().await; + for worker in workers { + worker.await.unwrap(); + } + + let expected = table.select(id).unwrap(); + table.wait_for_ops().await.unwrap(); + drop(table); + + let engine = wti::ProviderSwitchPersistenceEngine::new(config).await.unwrap(); + let table = wti::ProviderSwitchWorkTable::load(engine).await.unwrap(); + assert_eq!(table.select(id), Some(expected.clone())); + assert_eq!(table.select_by_unique_key(expected.unique_key), Some(expected.clone())); + for key in 10_000..10_000 + WORKERS * UPDATES_PER_WORKER { + if key != expected.unique_key { + assert!(table.select_by_unique_key(key).is_none()); + } + } + table.wait_for_ops().await.unwrap(); + drop(table); + + remove_dir_if_exists(ROOT.to_string()).await; +} + #[tokio::test] async fn persisted_tables_can_switch_between_wti_and_upstream_without_rebuild() { use provider_switch_upstream as upstream; diff --git a/tests/worktable/unsized_.rs b/tests/worktable/unsized_.rs index 32734e80..a1b8916e 100644 --- a/tests/worktable/unsized_.rs +++ b/tests/worktable/unsized_.rs @@ -1,3 +1,6 @@ +macro_rules! unsized_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use std::collections::{HashMap, HashSet}; use std::sync::Arc; use std::time::Duration; @@ -8,8 +11,9 @@ use worktable::worktable; worktable! ( name: Test, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, test: i64, another: u64, exchange: String, @@ -38,7 +42,7 @@ async fn test_update_string_full_row() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); table .update(TestRow { @@ -74,7 +78,7 @@ async fn test_update_string_by_unique() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeByTestQuery { exchange: "bigger test to test string update".to_string(), @@ -105,7 +109,7 @@ async fn test_update_string_by_pk() { exchange: "test".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeByIdQuery { exchange: "bigger test to test string update".to_string(), @@ -136,7 +140,7 @@ async fn test_update_string_by_non_unique() { exchange: "test".to_string(), }; let pk = table.insert(row1.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row2 = TestRow { id: table.get_next_pk().into(), test: 2, @@ -144,7 +148,7 @@ async fn test_update_string_by_non_unique() { exchange: "test".to_string(), }; let pk = table.insert(row2.clone()).unwrap(); - let second_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let second_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeByAbotherQuery { exchange: "bigger test to test string update".to_string(), @@ -291,8 +295,9 @@ async fn update_parallel() { worktable! ( name: TestMoreStrings, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, test: i64, another: u64, exchange: String, @@ -329,7 +334,7 @@ async fn test_update_many_strings_by_unique() { other_srting: "other".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeAndSomeByTestQuery { exchange: "bigger test to test string update".to_string(), @@ -365,7 +370,7 @@ async fn test_update_many_strings_by_pk() { other_srting: "other".to_string(), }; let pk = table.insert(row.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeAndSomeByIdQuery { exchange: "bigger test to test string update".to_string(), @@ -401,7 +406,7 @@ async fn test_update_many_strings_by_non_unique() { other_srting: "other".to_string(), }; let pk = table.insert(row1.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row2 = TestMoreStringsRow { id: table.get_next_pk().into(), test: 2, @@ -411,7 +416,7 @@ async fn test_update_many_strings_by_non_unique() { other_srting: "other".to_string(), }; let pk = table.insert(row2.clone()).unwrap(); - let second_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let second_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = ExchangeAndSomeByAnotherQuery { exchange: "bigger test to test string update".to_string(), @@ -466,7 +471,7 @@ async fn test_update_many_strings_by_string() { other_srting: "other er".to_string(), }; let pk = table.insert(row1.clone()).unwrap(); - let first_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let first_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row2 = TestMoreStringsRow { id: table.get_next_pk().into(), test: 2, @@ -476,7 +481,7 @@ async fn test_update_many_strings_by_string() { other_srting: "other".to_string(), }; let pk = table.insert(row2.clone()).unwrap(); - let second_link = table.0.primary_index.pk_map.get(&pk).unwrap().get().value; + let second_link = table.0.primary_index.pk_map.get_value(&pk).unwrap(); let row = SomeOtherByExchangeQuery { other_srting: "bigger test to test string update".to_string(), @@ -954,3 +959,11 @@ async fn upsert_parallel() { assert_eq!(&row.exchange, e) } } + + } + }; +} + +unsized_backend_suite!(wti, worktables_index); +unsized_backend_suite!(congee, congee); +unsized_backend_suite!(arctic, arctic); diff --git a/tests/worktable/update_in_place_unsized.rs b/tests/worktable/update_in_place_unsized.rs index d7e51136..9aba3a0a 100644 --- a/tests/worktable/update_in_place_unsized.rs +++ b/tests/worktable/update_in_place_unsized.rs @@ -1,131 +1,210 @@ -//! Regression (KNOWN BUG, currently #[ignore]): updating a variable-length -//! (unsized / `String`) row to a value of the SAME serialized length still does -//! a full delete-and-reinsert instead of an in-place mutation. +//! Regression coverage for the unsized (variable-length / `String`) in-place +//! update path, run across ALL THREE primary-index backends selectable through +//! the `using` keyword — WorkTablesIndex (the default), Congee, and Arctic. //! -//! ## Why this matters -//! Reinsert allocates a fresh page slot (the row's `Link` changes), re-serializes -//! the whole row, and rebuilds every secondary index. On a `String`-bearing -//! table this makes UPDATE ~9x slower than an in-place field write — WorkTable -//! leads insert and point_read in the KV benchmark but is dead last on overwrite -//! purely because of this path. +//! ## What this guards +//! A same-length update to an unsized field must mutate the row in place (its +//! physical `Link` is preserved) rather than delete-and-reinsert; a +//! length-changing update must still round-trip correctly (via reinsert); and a +//! reader racing same-length in-place updates must never observe a torn value. //! -//! ## Root cause -//! `codegen/src/generators/in_memory/queries/update.rs`, custom-update size -//! check (`gen_size_check`): `let mut need_to_reinsert = true;` then -//! `need_to_reinsert |= `. Initialized to `true`, the `|=` can -//! never clear it, so EVERY update reinserts regardless of whether any unsized -//! field changed size. -//! -//! ## Why the obvious fix is not enough (do not just flip the initializer) -//! Setting the initializer to `false` lets same-length updates skip reinsert — -//! but the in-place write then CORRUPTS long strings. The generated field write -//! is `mem::swap(&mut archived.inner., &mut archived_row.)`. +//! ## Why in-place is subtle (do not "just flip the initializer") +//! The naive fix — letting same-length updates skip reinsert — corrupts long +//! strings unless the out-of-line byte region is overwritten in place. The +//! generated field write is +//! `mem::swap(&mut archived.inner., &mut archived_row.)`. //! `ArchivedString` is a union: short strings (<= rkyv INLINE_CAPACITY) are //! inline, so the swap is self-contained; LONG strings are out-of-line — a //! relative pointer + length whose characters live in `archived_row`'s buffer. -//! Swapping only the pointer into the slot leaves it pointing at bytes that were -//! never written to the slot → reads come back as raw archived bytes (see -//! `worktable::unsized_::update_parallel_more_strings`, `update_many_times`, -//! `in_place::test_update_in_place_and_update_unsized_multithread`). -//! -//! A real fix must overwrite the existing out-of-line byte region in place -//! (e.g. `ArchivedStringRepr::as_bytes_seal`) when the new value fits, reinserting -//! only when it doesn't — preserving field-level semantics. Unsafe archived-memory -//! work; a subtle error is silent corruption. See docs/pr46-review-findings.md (F4). +//! Swapping only the pointer into the slot leaves it pointing at bytes never +//! written to the slot. See `codegen/.../queries/update.rs` (`gen_size_check`) +//! and `docs/pr46-review-findings.md` (F4). //! -//! The observable is the row's physical `Link`: an in-place update keeps it, a -//! reinsert changes it. Remove `#[ignore]` when the in-place path is fixed. - -use worktable::prelude::*; -use worktable::worktable; - -worktable!( - name: UnsizedUpdate, - columns: { - id: u64 primary_key, - payload: String, - }, - queries: { - update: { - Payload(payload) by id, - } - } -); - -/// Read the current physical link for a primary key. -fn link_of(table: &UnsizedUpdateWorkTable, pk: u64) -> Link { - table - .0 - .primary_index - .pk_map - .get_value(&UnsizedUpdatePrimaryKey::from(pk)) - .map(Into::into) - .expect("row must exist") -} +//! ## Why run it on every backend +//! The in-place fast path re-resolves the row's current `Link` and republishes +//! through the primary index; a backend whose link lookup or publication path +//! differs could keep the wrong slot or tear a read. Parametrizing over the +//! backends turns any such divergence into a test failure rather than a silent +//! corruption on one index type. -#[tokio::test] -#[ignore = "known bug: same-length unsized update reinserts; in-place write of a String field corrupts the row — needs a storage-path fix"] -async fn same_length_update_stays_in_place() { - let table = UnsizedUpdateWorkTable::default(); - table - .insert(UnsizedUpdateRow { - id: 1, - payload: "abcdefgh".to_string(), // 8 bytes - }) - .unwrap(); - - let before = link_of(&table, 1); - - table - .update_payload( - PayloadQuery { - payload: "12345678".to_string(), // 8 bytes — same length - }, - 1, - ) - .await - .unwrap(); - - let after = link_of(&table, 1); - - assert_eq!(table.select(1).unwrap().payload, "12345678"); - assert_eq!( - before, after, - "same-length unsized update must not reinsert (link changed: {before:?} -> {after:?})" - ); -} +/// Generates the full unsized in-place update suite for one primary-index +/// backend. Each backend gets its own module so the generated +/// `UnsizedUpdateRow` / `PayloadQuery` / `UnsizedUpdateWorkTable` idents do not +/// collide. `persist: false` is explicit because Congee and Arctic require a +/// persistence choice (WorkTablesIndex accepts it too). +macro_rules! unsized_in_place_suite { + ($module:ident, $using:ident) => { + mod $module { + use worktable::prelude::*; + use worktable::worktable; + + worktable!( + name: UnsizedUpdate, + persist: false, + columns: { + id: u64 primary_key using $using, + payload: String, + }, + queries: { + update: { + Payload(payload) by id, + } + } + ); + + /// Read the current physical link for a primary key. `get_value` is + /// a `TableIndex` trait method implemented by every backend's pk_map, + /// so this observability check is backend-agnostic. + fn link_of(table: &UnsizedUpdateWorkTable, pk: u64) -> Link { + table + .0 + .primary_index + .pk_map + .get_value(&UnsizedUpdatePrimaryKey::from(pk)) + .map(Into::into) + .expect("row must exist") + } + + #[tokio::test] + async fn same_length_update_stays_in_place() { + let table = UnsizedUpdateWorkTable::default(); + table + .insert(UnsizedUpdateRow { + id: 1, + payload: "abcdefgh".to_string(), // 8 bytes + }) + .unwrap(); + + let before = link_of(&table, 1); + + table + .update_payload( + PayloadQuery { + payload: "12345678".to_string(), // 8 bytes — same length + }, + 1, + ) + .await + .unwrap(); + + let after = link_of(&table, 1); -/// This one already holds on master and must keep holding through any fix: -/// a length change round-trips correctly (via reinsert). -#[tokio::test] -async fn different_length_update_is_correct() { - let table = UnsizedUpdateWorkTable::default(); - table - .insert(UnsizedUpdateRow { - id: 1, - payload: "abcdefghij".to_string(), - }) - .unwrap(); - - table - .update_payload( - PayloadQuery { - payload: "xy".to_string(), - }, - 1, - ) - .await - .unwrap(); - assert_eq!(table.select(1).unwrap().payload, "xy"); - - table - .update_payload( - PayloadQuery { - payload: "much longer payload".to_string(), - }, - 1, - ) - .await - .unwrap(); - assert_eq!(table.select(1).unwrap().payload, "much longer payload"); + assert_eq!(table.select(1).unwrap().payload, "12345678"); + assert_eq!( + before, after, + "same-length unsized update must not reinsert (link changed: {before:?} -> {after:?})" + ); + } + + /// A length change must round-trip correctly (via reinsert), in both + /// directions (shrink then grow). + #[tokio::test] + async fn different_length_update_is_correct() { + let table = UnsizedUpdateWorkTable::default(); + table + .insert(UnsizedUpdateRow { + id: 1, + payload: "abcdefghij".to_string(), + }) + .unwrap(); + + table + .update_payload( + PayloadQuery { + payload: "xy".to_string(), + }, + 1, + ) + .await + .unwrap(); + assert_eq!(table.select(1).unwrap().payload, "xy"); + + table + .update_payload( + PayloadQuery { + payload: "much longer payload".to_string(), + }, + 1, + ) + .await + .unwrap(); + assert_eq!(table.select(1).unwrap().payload, "much longer payload"); + } + + /// A reader resolving key K concurrently with same-size in-place + /// updates must always observe a VALID payload — one of the values + /// written, never a torn/partial read. The in-place path keeps the + /// same slot and republishes via `PublishedRow::replace` (whole + /// version swapped under a lock). + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn concurrent_reads_during_in_place_update_never_tear() { + use std::sync::Arc; + + let table = Arc::new(UnsizedUpdateWorkTable::default()); + // 4-digit payloads: every same-length update takes the in-place path. + table + .insert(UnsizedUpdateRow { + id: 1, + payload: "0000".to_string(), + }) + .unwrap(); + + let writer = { + let table = table.clone(); + tokio::spawn(async move { + for i in 0..20_000u64 { + table + .update_payload( + PayloadQuery { + payload: format!("{:04}", i % 10000), + }, + 1, + ) + .await + .unwrap(); + } + }) + }; + + let mut readers = Vec::new(); + for _ in 0..3 { + let table = table.clone(); + readers.push(tokio::spawn(async move { + for _ in 0..50_000u64 { + if let Some(row) = table.select(1) { + // Any observed value must be a valid 4-char + // ASCII-digit string — never garbage bytes. + assert_eq!( + row.payload.len(), + 4, + "torn read: payload {:?}", + row.payload + ); + assert!( + row.payload.bytes().all(|b| b.is_ascii_digit()), + "torn read: non-digit payload {:?}", + row.payload + ); + } + } + })); + } + + writer.await.unwrap(); + for r in readers { + r.await.unwrap(); + } + + // Final value is the writer's last write. + assert_eq!( + table.select(1).unwrap().payload, + format!("{:04}", (20_000u64 - 1) % 10000) + ); + } + } + }; } + +unsized_in_place_suite!(wti, worktables_index); +unsized_in_place_suite!(congee, congee); +unsized_in_place_suite!(arctic, arctic); diff --git a/tests/worktable/upsert.rs b/tests/worktable/upsert.rs index d297a8a4..c189856e 100644 --- a/tests/worktable/upsert.rs +++ b/tests/worktable/upsert.rs @@ -1,3 +1,6 @@ +macro_rules! upsert_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use std::sync::Arc; use std::time::Duration; @@ -7,8 +10,9 @@ use worktable::worktable; worktable!( name: UpsertChurn, + persist: false, columns: { - id: u64 primary_key, + id: u64 primary_key using $using, val: u64, }, ); @@ -210,3 +214,11 @@ async fn churn_run(churn_flips: u64, upserts_per_task: u64) { table.upsert(UpsertChurnRow { id: KEY, val: 424_242 }).await.unwrap(); assert_eq!(table.select(KEY).map(|r| r.val), Some(424_242)); } + + } + }; +} + +upsert_backend_suite!(wti, worktables_index); +upsert_backend_suite!(congee, congee); +upsert_backend_suite!(arctic, arctic); diff --git a/tests/worktable/vacuum.rs b/tests/worktable/vacuum.rs index 85c18d25..e793b4c6 100644 --- a/tests/worktable/vacuum.rs +++ b/tests/worktable/vacuum.rs @@ -1,3 +1,6 @@ +macro_rules! vacuum_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use chrono::TimeDelta; use parking_lot::Mutex; use std::collections::HashMap; @@ -9,8 +12,9 @@ use worktable_codegen::worktable; worktable!( name: VacuumTest, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, value: i64, data: String }, @@ -283,3 +287,11 @@ async fn vacuum_loop_test() { task.await.unwrap(); vacuum_task.abort(); } + + } + }; +} + +vacuum_backend_suite!(wti, worktables_index); +vacuum_backend_suite!(congee, congee); +vacuum_backend_suite!(arctic, arctic); diff --git a/tests/worktable/vacuum_no_row_loss.rs b/tests/worktable/vacuum_no_row_loss.rs index 98159338..6eaa57c3 100644 --- a/tests/worktable/vacuum_no_row_loss.rs +++ b/tests/worktable/vacuum_no_row_loss.rs @@ -12,6 +12,9 @@ //! that EVERY surviving row — by primary key AND by secondary index — is still //! present and correct after vacuum quiesces. +macro_rules! vacuum_no_loss_backend_suite { + ($module:ident, $using:ident) => { + mod $module { use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; @@ -22,8 +25,9 @@ use worktable_codegen::worktable; worktable!( name: VacuumLoss, + persist: false, columns: { - id: u64 primary_key autoincrement, + id: u64 primary_key autoincrement using $using, value: i64, data: String }, @@ -112,3 +116,11 @@ async fn vacuum_never_loses_surviving_rows() { assert_eq!(table.select(id), None, "deleted row {id} resurrected by vacuum"); } } + + } + }; +} + +vacuum_no_loss_backend_suite!(wti, worktables_index); +vacuum_no_loss_backend_suite!(congee, congee); +vacuum_no_loss_backend_suite!(arctic, arctic);