From 3a7c0ca070da7f9b0ea3feab54571859eb1f1ee0 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 05:39:13 +0700 Subject: [PATCH 01/20] wip: in-place same-size unsized update (PR#48, NOT working yet) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attempt: gen_size_check now reinserts only when an unsized field's serialized size changed; the same-size case rebuilds the full row and writes it in place at the SAME slot via data.update instead of the corrupting mem::swap. STATUS: does NOT work yet. Even the 8-byte inline case reads back wrong — the data.update path publishes via stage_published_row and the subsequent read is served from a stale/duplicate published slot. Pushed dirty for visibility; the publication-path interaction is the remaining blocker. Safe reinsert path is on master; this branch is experimental. --- .../generators/in_memory/queries/update.rs | 52 ++++++++++++++----- tests/worktable/update_in_place_unsized.rs | 2 +- 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 911b714..a18356a 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -235,33 +235,59 @@ 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 { - 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); + if need_to_reinsert { + drop(_guard); + let op_lock = { #full_row_lock }; + let _guard = LockGuard::new_with_mutation( + op_lock, + self.0.lock_manager.clone(), + pk.clone(), + ); + + 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 existing 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. This + // does NOT `mem::swap` the archived field (which would leave + // an out-of-line pointer dangling into a throwaway buffer), + // and it publishes the new row correctly via `data.update`. + unsafe { + self.0.data.update::<{ #const_name }>(row_new, link) + .map_err(WorkTableError::PagesError)?; + } + self.0.update_state.remove(&pk); return core::result::Result::Ok(()); } } diff --git a/tests/worktable/update_in_place_unsized.rs b/tests/worktable/update_in_place_unsized.rs index d7e5113..d46b6a7 100644 --- a/tests/worktable/update_in_place_unsized.rs +++ b/tests/worktable/update_in_place_unsized.rs @@ -64,7 +64,7 @@ fn link_of(table: &UnsizedUpdateWorkTable, pk: u64) -> Link { } #[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"] +#[ignore = "WIP PR#48: in-place fix still corrupts via publication path — see docs"] async fn same_length_update_stays_in_place() { let table = UnsizedUpdateWorkTable::default(); table From 159e418a94c3f72adc528b1899bb5c839d366670 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 05:50:07 +0700 Subject: [PATCH 02/20] =?UTF-8?q?wip:=20in-place=20same-size=20unsized=20u?= =?UTF-8?q?pdate=20=E2=80=94=204/5=20tests=20pass=20(PR#48)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add DataPages::update_in_place: re-serialize the full row into the SAME slot and republish it LIVE (unghost + publish_wrapped_row), fixing the ghosting that made data.update read back stale. gen_size_check now reinserts only when a field's serialized size changed; same-size writes in place, re-resolving the link under the mutation gate. WORKING: same_length (inline), different_length, update_many_times, and the single/multithread in-place unsized tests — long-string corruption is FIXED. REMAINING: update_parallel_more_strings{,_more_threads} hit InvalidLink under concurrency. Root: the reinsert branch drops its guard (drop(_guard)) and a concurrent same-key reinsert can free a link that #diff_process_insert / size-check still reference. Pre-existing reinsert-path window, now exposed because more ops take the in-place path. Needs the guarded section to not use a link across the drop. --- .../generators/in_memory/queries/update.rs | 24 ++++++---- src/in_memory/pages.rs | 44 +++++++++++++++++++ tests/worktable/update_in_place_unsized.rs | 1 - 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index a18356a..1ef7127 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -276,15 +276,23 @@ impl InMemoryGenerator { return core::result::Result::Ok(()); } - // Same-size in-place write at the existing 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. This - // does NOT `mem::swap` the archived field (which would leave - // an out-of-line pointer dangling into a throwaway buffer), - // and it publishes the new row correctly via `data.update`. + // Same-size in-place write at the CURRENT slot. Re-resolve + // the link under the held mutation gate: a concurrent + // different-length update on this key may have reinserted the + // row to a new slot, invalidating the link captured earlier. + // 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. + let current_link: Link = self.0 + .primary_index + .pk_map + .get_value(&pk) + .map(Into::into) + .ok_or(WorkTableError::NotFound)?; unsafe { - self.0.data.update::<{ #const_name }>(row_new, link) + self.0.data.update_in_place::<{ #const_name }>(row_new, current_link) .map_err(WorkTableError::PagesError)?; } self.0.update_state.remove(&pk); diff --git a/src/in_memory/pages.rs b/src/in_memory/pages.rs index 1aaa4e4..454f6ea 100644 --- a/src/in_memory/pages.rs +++ b/src/in_memory/pages.rs @@ -629,6 +629,50 @@ 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). + /// + /// # 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. + let gen_row = ::WrappedRow::from_inner(row.clone()); + 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>>, diff --git a/tests/worktable/update_in_place_unsized.rs b/tests/worktable/update_in_place_unsized.rs index d46b6a7..4019b51 100644 --- a/tests/worktable/update_in_place_unsized.rs +++ b/tests/worktable/update_in_place_unsized.rs @@ -64,7 +64,6 @@ fn link_of(table: &UnsizedUpdateWorkTable, pk: u64) -> Link { } #[tokio::test] -#[ignore = "WIP PR#48: in-place fix still corrupts via publication path — see docs"] async fn same_length_update_stays_in_place() { let table = UnsizedUpdateWorkTable::default(); table From 5865f27c2c00a00a512ecf45031eb2e9d9875644 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:06:25 +0700 Subject: [PATCH 03/20] =?UTF-8?q?wip:=20in-place=20update=20with=20reinser?= =?UTF-8?q?t=20fallback=20=E2=80=94=20flaky=20lost-update=20race=20remains?= =?UTF-8?q?=20(PR#48)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit update_in_place now falls back to a full reinsert when the same-slot write does not fit (equal field sizes don't guarantee equal total serialized length, and a concurrent reinsert may have moved the row). Correctness-first: reinsert always works. Fixed the InvalidLink hard failures. STATUS: deterministic tests pass (same_length, different_length, update_many_times, update_parallel_more_strings_more_threads, single/multithread in-place). REMAINING: update_parallel_more_strings is FLAKY (~4/8) — a lost-update race: two concurrent same-key updates interleave and a stale value wins (value mismatch, not InvalidLink). Suspect the size-check select reads before the per-key mutation gate fully serializes. Under analysis. --- .../generators/in_memory/queries/update.rs | 20 ++++++++++++++++--- src/in_memory/pages.rs | 6 +++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 1ef7127..5d68ed2 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -291,9 +291,23 @@ impl InMemoryGenerator { .get_value(&pk) .map(Into::into) .ok_or(WorkTableError::NotFound)?; - unsafe { - self.0.data.update_in_place::<{ #const_name }>(row_new, current_link) - .map_err(WorkTableError::PagesError)?; + // Try the same-slot in-place write. Equal field sizes do not + // guarantee an equal TOTAL serialized length (alignment), and + // a concurrent reinsert may have moved the row, so a mismatch + // is expected sometimes — fall back to a full reinsert rather + // than fail. Correctness-first: reinsert always works. + 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(()); + } + + let row_old_for_reinsert = self.0.select(pk.clone()).expect("should not be deleted by other thread"); + if let Err(e) = self.reinsert(row_old_for_reinsert, row_new).await { + self.0.update_state.remove(&pk); + return Err(e); } self.0.update_state.remove(&pk); return core::result::Result::Ok(()); diff --git a/src/in_memory/pages.rs b/src/in_memory/pages.rs index 454f6ea..c50393a 100644 --- a/src/in_memory/pages.rs +++ b/src/in_memory/pages.rs @@ -653,7 +653,11 @@ where 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. + // 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. let gen_row = ::WrappedRow::from_inner(row.clone()); unsafe { page.save_row_by_link(&gen_row, link) From ba74d8853c97dc345f36706a20fd3a84aeb0068a Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:15:03 +0700 Subject: [PATCH 04/20] =?UTF-8?q?fix:=20in-place=20same-size=20unsized=20u?= =?UTF-8?q?pdate=20(overwrite=20perf,=20F4)=20=E2=80=94=20COMPLETE?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same-size updates to a non-indexed unsized (String) column are now written in place at the existing slot instead of a full delete-and-reinsert. - DataPages::update_in_place: re-serialize the row into the SAME slot and republish it LIVE (unghost + publish_wrapped_row), avoiding both the dangling out-of-line ArchivedString pointer from mem::swap and the ghosting that made a plain data.update read back stale. - gen_size_check: reinsert only when a field's size changed; the in-place path holds the full-row lock (serializing the read-modify-write against concurrent same-key updates — fixes a lost-update race) and falls back to reinsert when the total serialized length does not fit the slot. - Scope: only when NO updated column is indexed. Indexed columns keep the reinsert path so secondary-index maintenance and unique-constraint checks run. Full integration suite: 356 passed / 0 failed. update_parallel_more_strings 8/8 under stress. KV overwrite ~298K -> ~459K ops/s (+54%). --- .../generators/in_memory/queries/update.rs | 102 +++++++++++++----- 1 file changed, 75 insertions(+), 27 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 5d68ed2..679493e 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -220,8 +220,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| { @@ -253,19 +265,27 @@ impl InMemoryGenerator { #(#fields_check)* { + // 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( + op_lock, + self.0.lock_manager.clone(), + 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 { - drop(_guard); - let op_lock = { #full_row_lock }; - let _guard = LockGuard::new_with_mutation( - op_lock, - self.0.lock_manager.clone(), - pk.clone(), - ); - if let Err(e) = self.reinsert(row_old, row_new).await { self.0.update_state.remove(&pk); @@ -276,26 +296,21 @@ impl InMemoryGenerator { return core::result::Result::Ok(()); } - // Same-size in-place write at the CURRENT slot. Re-resolve - // the link under the held mutation gate: a concurrent - // different-length update on this key may have reinserted the - // row to a new slot, invalidating the link captured earlier. - // 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. + // 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. let current_link: Link = self.0 .primary_index .pk_map .get_value(&pk) .map(Into::into) .ok_or(WorkTableError::NotFound)?; - // Try the same-slot in-place write. Equal field sizes do not - // guarantee an equal TOTAL serialized length (alignment), and - // a concurrent reinsert may have moved the row, so a mismatch - // is expected sometimes — fall back to a full reinsert rather - // than fail. Correctness-first: reinsert always works. + // 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. let in_place_ok = unsafe { self.0.data.update_in_place::<{ #const_name }>(row_new.clone(), current_link).is_ok() }; @@ -313,8 +328,41 @@ impl InMemoryGenerator { return core::result::Result::Ok(()); } } - } else { + } 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(()); + } + } } } @@ -500,7 +548,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(); @@ -740,7 +788,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(); From 5d2bf3a021467e63dedb12f7386fc7a59341e57b Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:31:51 +0700 Subject: [PATCH 05/20] fix: keep unsized update expansion lint-clean --- .../generators/in_memory/queries/update.rs | 68 ++++++++++++------- 1 file changed, 42 insertions(+), 26 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 679493e..7fd9b96 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -555,6 +555,25 @@ impl InMemoryGenerator { 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 @@ -579,18 +598,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 } } } @@ -804,6 +812,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) @@ -847,20 +876,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 } } } From ef3b90c3200bb0983c0408d99d69d082065e584c Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:25:02 +0700 Subject: [PATCH 06/20] fix: clean up cancelled lock acquirers --- src/lib.rs | 2 +- src/lock/map.rs | 178 ++++++++++++++++++++++++++++++++++++++++-------- src/lock/mod.rs | 2 +- 3 files changed, 150 insertions(+), 32 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 21c25cb..8764224 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -31,7 +31,7 @@ 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, diff --git a/src/lock/map.rs b/src/lock/map.rs index 2b563d5..af25c55 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,6 +29,69 @@ 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); @@ -36,7 +100,7 @@ impl Drop for MutationGuard { #[derive(Debug)] pub struct LockMap { - map: RwLock>>>, + map: RwLock>>, next_id: AtomicU16, mutation_stripes: Arc<[MutationStripe; MUTATION_STRIPE_COUNT]>, } @@ -60,11 +124,20 @@ where 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) } 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 +149,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 +159,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 +192,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 +242,40 @@ 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)); + } +} diff --git a/src/lock/mod.rs b/src/lock/mod.rs index 81e194f..75dea47 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. From 316c5accbc3595d770facf6f6678d2d86c31ea6f Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:47:38 +0700 Subject: [PATCH 07/20] fix: reuse vacuumed pages after reload --- .../generator/space_file/worktable_impls.rs | 2 +- src/persistence/engine.rs | 4 + src/persistence/mod.rs | 11 ++ src/persistence/space/data.rs | 99 +++++++++++++++ src/persistence/space/mod.rs | 1 + src/persistence/task.rs | 116 ++++++++++++++---- src/table/vacuum/mod.rs | 5 + src/table/vacuum/vacuum.rs | 13 +- tests/persistence/vacuum.rs | 63 +++++++++- 9 files changed, 288 insertions(+), 26 deletions(-) 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 0b69fe6..20745e6 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/src/persistence/engine.rs b/src/persistence/engine.rs index 6e929bc..37254f6 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/mod.rs b/src/persistence/mod.rs index df98e79..206d812 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -1,5 +1,7 @@ use std::future::Future; +use data_bucket::page::PageId; + use crate::persistence::operation::BatchOperation; pub use engine::DiskConfig; @@ -57,6 +59,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 1fbffea..caa1edd 100644 --- a/src/persistence/space/data.rs +++ b/src/persistence/space/data.rs @@ -37,6 +37,63 @@ impl SpaceData

) -> bool { + let mut changed = false; + + for used in used_links { + let used_start = u64::from(used.offset); + let used_end = used_start + u64::from(used.length); + let mut remaining = Vec::with_capacity(self.info.inner.empty_links_list.len() + 1); + + for free in self.info.inner.empty_links_list.drain(..) { + if free.page_id != used.page_id { + remaining.push(free); + continue; + } + + let free_start = u64::from(free.offset); + let free_end = free_start + u64::from(free.length); + let overlap_start = free_start.max(used_start); + let overlap_end = free_end.min(used_end); + if overlap_start >= overlap_end { + remaining.push(free); + continue; + } + + changed = true; + if free_start < overlap_start { + remaining.push(Link { + page_id: free.page_id, + offset: free.offset, + length: (overlap_start - free_start) as u32, + }); + } + if overlap_end < free_end { + remaining.push(Link { + page_id: free.page_id, + offset: overlap_end as u32, + length: (free_end - overlap_end) as u32, + }); + } + } + + self.info.inner.empty_links_list = remaining; + } + + if changed { + self.info.inner.empty_links_list.sort_by_key(|link| { + let page_id: u32 = link.page_id.into(); + (page_id, link.offset) + }); + } + changed + } } impl SpaceDataOps @@ -100,6 +157,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 +183,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.collect::>()) { + self.save_info().await?; + } + let page_ids = batch_data.keys().map(|id| (*id).into()).collect::>(); let ids_to_create = page_ids .iter() @@ -182,6 +247,40 @@ where Ok(()) } + async fn reclaim_data_pages(&mut self, page_ids: Vec) -> eyre::Result<()> { + let mut page_ids = page_ids + .into_iter() + .filter(|page_id| { + let id: u32 = (*page_id).into(); + id != 0 && id <= self.last_page_id + }) + .collect::>(); + page_ids.sort_unstable(); + page_ids.dedup(); + + if page_ids.is_empty() { + return Ok(()); + } + + self.info + .inner + .empty_links_list + .retain(|link| !page_ids.contains(&link.page_id)); + 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 } diff --git a/src/persistence/space/mod.rs b/src/persistence/space/mod.rs index dee296f..12ca9c3 100644 --- a/src/persistence/space/mod.rs +++ b/src/persistence/space/mod.rs @@ -33,6 +33,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 578fe75..f639341 100644 --- a/src/persistence/task.rs +++ b/src/persistence/task.rs @@ -396,6 +396,7 @@ mod lifecycle_tests { struct TestEngine { batches: Arc, + events: Arc>>, config: TestConfig, fail: bool, } @@ -406,6 +407,7 @@ 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, }) @@ -423,6 +425,12 @@ mod lifecycle_tests { return Err(eyre::eyre!("injected batch failure")); } 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,6 +459,7 @@ 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, }); @@ -465,6 +474,7 @@ 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, }); @@ -479,6 +489,30 @@ 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, + fail: false, + }); + + 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"]); + } +} + +#[derive(Debug)] +enum PersistenceMessage { + Operation(Operation), + ReclaimPages(Vec), } #[derive(Debug)] @@ -487,7 +521,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 +541,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 +566,10 @@ impl Queue Option> { + ) -> Option> { loop { let notified = self.notify.notified(); // Drain values @@ -554,7 +595,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 +604,6 @@ impl Queue impl Iterator> { - std::iter::from_fn(|| self.immediate_pop()) - } - pub fn len(&self) -> usize { self.len.load(Ordering::Acquire) } @@ -594,6 +631,10 @@ where link: new_link, })) } + + fn reclaim_pages(&self, page_ids: Vec) -> PersistenceResult { + self.push_message(PersistenceMessage::ReclaimPages(page_ids)) + } } #[derive(Debug)] @@ -665,9 +706,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 +750,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 +773,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 +816,11 @@ impl } else { tokio::time::sleep(Duration::from_millis(500)).await; } + } else if let Some(page_ids) = pending_reclaim.take() + && 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 bcfb414..727ceed 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 a2fced8..e555c0a 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/vacuum.rs b/tests/persistence/vacuum.rs index 9ea77bc..3a9ba92 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" + ); + } }) } From 91329e6f2dd0a67a24c1986c30f9962e2929ada7 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:50:28 +0700 Subject: [PATCH 08/20] feat: add checked offline recovery load --- README.md | 11 +- codegen/src/generators/persist/table/impls.rs | 106 ++++++++++++-- .../src/generators/read_only/table/impls.rs | 104 ++++++++++++-- .../persist_table/generator/space_file/mod.rs | 32 ++++- codegen/src/persist_table/mod.rs | 10 ++ docs/crate.md | 9 ++ docs/persistence-durability.md | 61 ++++++-- src/lib.rs | 4 +- src/persistence/mod.rs | 30 ++++ tests/persistence/mod.rs | 1 + tests/persistence/recovery_load.rs | 136 ++++++++++++++++++ 11 files changed, 459 insertions(+), 45 deletions(-) create mode 100644 tests/persistence/recovery_load.rs diff --git a/README.md b/README.md index b70f5c6..da2cf0a 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/src/generators/persist/table/impls.rs b/codegen/src/generators/persist/table/impls.rs index 50658e7..12edae5 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(()) } @@ -232,7 +306,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 +325,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/read_only/table/impls.rs b/codegen/src/generators/read_only/table/impls.rs index 2542cff..8433098 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_table/generator/space_file/mod.rs b/codegen/src/persist_table/generator/space_file/mod.rs index 8120e6e..af79d7a 100644 --- a/codegen/src/persist_table/generator/space_file/mod.rs +++ b/codegen/src/persist_table/generator/space_file/mod.rs @@ -212,6 +212,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 +248,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 +259,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 +321,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/mod.rs b/codegen/src/persist_table/mod.rs index 91d598b..e0d12cf 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/docs/crate.md b/docs/crate.md index dc382cc..f697626 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 9f2e321..bc37ac2 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/src/lib.rs b/src/lib.rs index 8764224..96624a2 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::*; @@ -35,7 +35,7 @@ pub mod prelude { pub use crate::mem_stat::MemStat; pub use crate::persistence::{ AcknowledgeOperation, ArtPersistenceKey, DeleteOperation, DiskConfig, DiskPersistenceEngine, - IndexTableOfContents, InsertOperation, Operation, OperationId, PersistedWorkTable, PersistenceConfig, + IndexTableOfContents, InsertOperation, LoadMode, Operation, OperationId, PersistedWorkTable, PersistenceConfig, PersistenceEngine, PersistenceError, PersistenceLoadError, PersistenceResult, PersistenceState, PersistenceTask, ReadOnlyPersistenceEngine, SpaceArcticIndex, SpaceCongeeIndex, SpaceData, SpaceDataOps, SpaceIndex, SpaceIndexOps, SpaceIndexUnsized, SpaceSecondaryIndexOps, UpdateOperation, load_persisted_state, diff --git a/src/persistence/mod.rs b/src/persistence/mod.rs index 206d812..9aa2376 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -33,6 +33,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, @@ -40,6 +60,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 { diff --git a/tests/persistence/mod.rs b/tests/persistence/mod.rs index 4425022..7dc62be 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 0000000..d3e0642 --- /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; +} From 7c3d374affadf795772a8bdcbedb43ada64c97e5 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:25:43 +0700 Subject: [PATCH 09/20] feat: move WTI structural persistence off mutation path --- Cargo.toml | 4 + codegen/Cargo.toml | 1 + codegen/src/generators/index_backend.rs | 4 + codegen/src/generators/persist/index/mod.rs | 9 +- codegen/src/generators/persist/table/impls.rs | 9 +- codegen/src/generators/persist/table/mod.rs | 16 + codegen/src/persist_index/generator.rs | 15 +- codegen/src/persist_index/space/index.rs | 12 + codegen/src/persist_table/generator/mod.rs | 1 + codegen/src/persist_table/generator/space.rs | 10 +- .../persist_table/generator/space_file/mod.rs | 11 +- codegen/src/persist_table/parser.rs | 5 + codegen/src/worktable/mod.rs | 26 ++ docs/wti-dirty-generation-persistence-plan.md | 67 +++- src/index/mod.rs | 2 + src/index/persistent_wti.rs | 301 ++++++++++++++++ src/index/table_index/mod.rs | 20 +- src/lib.rs | 14 +- src/mem_stat/mod.rs | 18 +- src/persistence/mod.rs | 4 +- src/persistence/space/logical_index.rs | 331 ++++++++++++++++++ src/persistence/space/mod.rs | 2 + tests/worktable/index_backends.rs | 65 ++++ 23 files changed, 916 insertions(+), 31 deletions(-) create mode 100644 src/index/persistent_wti.rs create mode 100644 src/persistence/space/logical_index.rs diff --git a/Cargo.toml b/Cargo.toml index 59b8c5f..b30b580 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"] diff --git a/codegen/Cargo.toml b/codegen/Cargo.toml index c7a5562..3871b64 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/index_backend.rs b/codegen/src/generators/index_backend.rs index ca923da..cac630a 100644 --- a/codegen/src/generators/index_backend.rs +++ b/codegen/src/generators/index_backend.rs @@ -40,6 +40,10 @@ pub(crate) fn persistent_unique_index_type( worktables_node: Option, ) -> syn::Result { 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 b2068c6..476fe72 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/table/impls.rs b/codegen/src/generators/persist/table/impls.rs index 12edae5..02f8f82 100644 --- a/codegen/src/generators/persist/table/impls.rs +++ b/codegen/src/generators/persist/table/impls.rs @@ -237,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(), }); } @@ -249,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(), }); }, diff --git a/codegen/src/generators/persist/table/mod.rs b/codegen/src/generators/persist/table/mod.rs index 272fcb1..1d339ed 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/persist_index/generator.rs b/codegen/src/persist_index/generator.rs index 9af1858..9c53628 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 d891d50..eb308ee 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 7baf87b..c93e091 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 1f81f39..dc2d279 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 af79d7a..cef7ba3 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 } diff --git a/codegen/src/persist_table/parser.rs b/codegen/src/persist_table/parser.rs index 26773d9..5803beb 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 fe99341..b5e296f 100644 --- a/codegen/src/worktable/mod.rs +++ b/codegen/src/worktable/mod.rs @@ -211,6 +211,32 @@ mod tests { 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] fn explicit_indexset_is_persistence_compatible() { let output = expand(quote! { diff --git a/docs/wti-dirty-generation-persistence-plan.md b/docs/wti-dirty-generation-persistence-plan.md index c881cac..85206e5 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,44 @@ 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. + +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. + +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 +208,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/index/mod.rs b/src/index/mod.rs index b6c4351..8ed7f27 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 0000000..3fc5cdb --- /dev/null +++ b/src/index/persistent_wti.rs @@ -0,0 +1,301 @@ +//! 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. + +use std::array; +use std::collections::hash_map::DefaultHasher; +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 crate::index::UniqueIndex; +use crate::util::OffsetEqLink; +use crate::{IndexMap, TableIndexCdc}; + +const MUTATION_STRIPES: usize = 64; + +/// 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, + 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 + } + + fn mutation_stripe(&self, key: &Q) -> MutexGuard<'_, ()> { + let mut hasher = DefaultHasher::new(); + key.hash(&mut hasher); + self.mutation_stripes[hasher.finish() as usize % MUTATION_STRIPES].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 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); + } +} diff --git a/src/index/table_index/mod.rs b/src/index/table_index/mod.rs index 75c8b71..4affdae 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 96624a2..75b2488 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -38,9 +38,9 @@ pub mod prelude { IndexTableOfContents, InsertOperation, LoadMode, 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, + 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/mem_stat/mod.rs b/src/mem_stat/mod.rs index a933b4d..666d61d 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/mod.rs b/src/persistence/mod.rs index 9aa2376..f263e4d 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -14,8 +14,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; diff --git a/src/persistence/space/logical_index.rs b/src/persistence/space/logical_index.rs new file mode 100644 index 0000000..0c6a29f --- /dev/null +++ b/src/persistence/space/logical_index.rs @@ -0,0 +1,331 @@ +//! 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 data_bucket::{Link, SizeMeasurable, SpaceId, VariableSizeMeasurable}; +use eyre::bail; +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::{SpaceIndex, SpaceIndexOps, SpaceIndexUnsized}; +use crate::prelude::WT_INDEX_EXTENSION; + +fn translate_logical_event( + shadow: &BTreeMap, + event: ChangeEvent>, +) -> eyre::Result>>> +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 (removed, events) = shadow.remove_cdc(&value.key); + if removed.as_ref().map(|(_, link)| *link) != Some(value.value) { + bail!( + "logical WTI shadow diverged while removing key {:?}: expected {:?}, found {:?}", + value.key, + value.value, + removed.map(|(_, link)| link), + ); + } + Ok(events) + } + _ => bail!("logical WTI persistence received a structural or malformed event"), + } +} + +fn translate_logical_batch( + shadow: &BTreeMap, + events: BatchChangeEvent, +) -> eyre::Result> +where + T: Debug + Eq + Hash + Clone + Send + Ord + 'static, + Node: NodeLike> + Send + 'static, +{ + let mut structural = Vec::new(); + for event in events { + structural.extend(translate_logical_event(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, +{ + 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 mut disk = SpaceIndex::new(path, SpaceId::from(0), version).await?; + let shadow = disk.parse_indexset().await?; + Ok(Self { 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.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.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, +{ + 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 mut disk = SpaceIndexUnsized::new(path, SpaceId::from(0), version).await?; + let shadow = disk.parse_indexset().await?; + Ok(Self { 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.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.shadow, events)?; + self.disk.process_change_event_batch(events).await + } +} + +#[cfg(test)] +mod tests { + 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( + &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( + &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()); + } +} diff --git a/src/persistence/space/mod.rs b/src/persistence/space/mod.rs index 12ca9c3..843ca8b 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)>>; diff --git a/tests/worktable/index_backends.rs b/tests/worktable/index_backends.rs index 0924722..f789ae3 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; From 788c8ee2c961ecd93e44f08f642aa26e79c7207d Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 06:51:04 +0700 Subject: [PATCH 10/20] fix: harden logical persistence ordering and quarantine --- codegen/src/generators/index_backend.rs | 5 + codegen/src/worktable/mod.rs | 11 +- docs/wti-dirty-generation-persistence-plan.md | 25 ++++ src/index/persistent_wti.rs | 7 + src/lib.rs | 6 +- src/persistence/error.rs | 47 ++++++ src/persistence/mod.rs | 5 +- src/persistence/space/logical_index.rs | 136 +++++++++++++++--- src/persistence/task.rs | 28 +++- 9 files changed, 241 insertions(+), 29 deletions(-) diff --git a/codegen/src/generators/index_backend.rs b/codegen/src/generators/index_backend.rs index cac630a..defcaf6 100644 --- a/codegen/src/generators/index_backend.rs +++ b/codegen/src/generators/index_backend.rs @@ -39,6 +39,11 @@ 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> }, diff --git a/codegen/src/worktable/mod.rs b/codegen/src/worktable/mod.rs index b5e296f..e9334af 100644 --- a/codegen/src/worktable/mod.rs +++ b/codegen/src/worktable/mod.rs @@ -206,9 +206,16 @@ mod tests { indexes: { value_idx: value unique, }, - }); + }) + .unwrap() + .to_string(); - assert!(output.is_ok()); + if cfg!(feature = "logical-index-persistence") { + assert!(output.contains("PersistentWtiIndex")); + } else { + assert!(output.contains("IndexMap")); + assert!(!output.contains("PersistentWtiIndex")); + } } #[cfg(feature = "logical-index-persistence")] diff --git a/docs/wti-dirty-generation-persistence-plan.md b/docs/wti-dirty-generation-persistence-plan.md index 85206e5..dd81f51 100644 --- a/docs/wti-dirty-generation-persistence-plan.md +++ b/docs/wti-dirty-generation-persistence-plan.md @@ -63,6 +63,18 @@ The first implementation deliberately avoids a new WTI disk format. With the 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 @@ -71,6 +83,19 @@ 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; diff --git a/src/index/persistent_wti.rs b/src/index/persistent_wti.rs index 3fc5cdb..0853ffd 100644 --- a/src/index/persistent_wti.rs +++ b/src/index/persistent_wti.rs @@ -5,6 +5,9 @@ //! 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::collections::hash_map::DefaultHasher; @@ -24,6 +27,10 @@ use crate::index::UniqueIndex; use crate::util::OffsetEqLink; use crate::{IndexMap, TableIndexCdc}; +// A stripe provides per-key exclusion only: DefaultHasher 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; /// A persisted WorkTablesIndex whose foreground mutations emit logical CDC. diff --git a/src/lib.rs b/src/lib.rs index 75b2488..9e28c54 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -36,9 +36,9 @@ pub mod prelude { pub use crate::persistence::{ AcknowledgeOperation, ArtPersistenceKey, DeleteOperation, DiskConfig, DiskPersistenceEngine, IndexTableOfContents, InsertOperation, LoadMode, Operation, OperationId, PersistedWorkTable, PersistenceConfig, - PersistenceEngine, PersistenceError, PersistenceLoadError, PersistenceResult, PersistenceState, - PersistenceTask, ReadOnlyPersistenceEngine, SpaceArcticIndex, SpaceCongeeIndex, SpaceData, SpaceDataOps, - SpaceIndex, SpaceIndexOps, SpaceIndexUnsized, SpaceLogicalIndex, SpaceLogicalIndexUnsized, + 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, }; diff --git a/src/persistence/error.rs b/src/persistence/error.rs index ed077fc..fbfac49 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 f263e4d..121e27a 100644 --- a/src/persistence/mod.rs +++ b/src/persistence/mod.rs @@ -6,7 +6,10 @@ 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, diff --git a/src/persistence/space/logical_index.rs b/src/persistence/space/logical_index.rs index 0c6a29f..f5d637d 100644 --- a/src/persistence/space/logical_index.rs +++ b/src/persistence/space/logical_index.rs @@ -7,9 +7,9 @@ use std::fmt::Debug; use std::hash::Hash; +use std::path::{Path, PathBuf}; use data_bucket::{Link, SizeMeasurable, SpaceId, VariableSizeMeasurable}; -use eyre::bail; use indexset::cdc::change::ChangeEvent; use indexset::concurrent::map::BTreeMap; use indexset::core::node::NodeLike; @@ -25,13 +25,14 @@ use tokio::fs::File; use crate::UnsizedNode; use crate::persistence::space::BatchChangeEvent; -use crate::persistence::{SpaceIndex, SpaceIndexOps, SpaceIndexUnsized}; +use crate::persistence::{PersistenceIndexCorruption, SpaceIndex, SpaceIndexOps, SpaceIndexUnsized}; use crate::prelude::WT_INDEX_EXTENSION; fn translate_logical_event( + index_path: &Path, shadow: &BTreeMap, event: ChangeEvent>, -) -> eyre::Result>>> +) -> Result>>, PersistenceIndexCorruption> where T: Debug + Eq + Hash + Clone + Send + Ord + 'static, Node: NodeLike> + Send + 'static, @@ -52,32 +53,43 @@ where index: 0, .. } if max_value.key == value.key && max_value.value == value.value => { - let (removed, events) = shadow.remove_cdc(&value.key); - if removed.as_ref().map(|(_, link)| *link) != Some(value.value) { - bail!( - "logical WTI shadow diverged while removing key {:?}: expected {:?}, found {:?}", - value.key, - value.value, - removed.map(|(_, link)| link), - ); + 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) } - _ => bail!("logical WTI persistence received a structural or malformed event"), + _ => Err(PersistenceIndexCorruption::new( + index_path, + "logical WTI persistence received a structural or malformed event", + )), } } fn translate_logical_batch( + index_path: &Path, shadow: &BTreeMap, - events: BatchChangeEvent, -) -> eyre::Result> + 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(shadow, event)?); + structural.extend(translate_logical_event(index_path, shadow, event)?); } Ok(structural) } @@ -89,6 +101,7 @@ pub struct SpaceLogicalIndex where T: Send + Ord + Eq + Clone + 'static, { + index_path: PathBuf, shadow: BTreeMap, disk: SpaceIndex, } @@ -123,9 +136,14 @@ where + 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 { shadow, disk }) + Ok(Self { + index_path, + shadow, + disk, + }) } } @@ -170,12 +188,12 @@ where } async fn process_change_event(&mut self, event: ChangeEvent>) -> eyre::Result<()> { - let events = translate_logical_event(&self.shadow, event)?; + 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.shadow, events)?; + let events = translate_logical_batch(&self.index_path, &self.shadow, events)?; self.disk.process_change_event_batch(events).await } } @@ -185,6 +203,7 @@ pub struct SpaceLogicalIndexUnsized where T: Send + Ord + Eq + Clone + Default + Debug + SizeMeasurable + VariableSizeMeasurable + 'static, { + index_path: PathBuf, shadow: BTreeMap>>, disk: SpaceIndexUnsized, } @@ -222,9 +241,14 @@ where + 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 { shadow, disk }) + Ok(Self { + index_path, + shadow, + disk, + }) } } @@ -270,12 +294,12 @@ where } async fn process_change_event(&mut self, event: ChangeEvent>) -> eyre::Result<()> { - let events = translate_logical_event(&self.shadow, event)?; + 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.shadow, events)?; + let events = translate_logical_batch(&self.index_path, &self.shadow, events)?; self.disk.process_change_event_batch(events).await } } @@ -299,6 +323,7 @@ mod tests { 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(), @@ -316,6 +341,7 @@ mod tests { 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(), @@ -328,4 +354,72 @@ mod tests { 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))); + } } diff --git a/src/persistence/task.rs b/src/persistence/task.rs index f639341..5e4b51e 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 } @@ -507,6 +512,25 @@ mod lifecycle_tests { assert_eq!(&*events.lock(), &["batch", "reclaim", "batch"]); } + + #[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)] From d48e1abab7fee8bca5921814109ac175d75d8312 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:08:00 +0700 Subject: [PATCH 11/20] review: address PR #48 self-review findings (F1-F5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: full-row update() now takes the in-place same-size path when the table has NO secondary indexes (a full-row update rewrites every index otherwise). Documented why gen_non_unique_update (indexed column) and indexed full-row updates correctly stay on reinsert; the custom single-column updates already get the fast path via gen_size_check. F2: update_in_place documents it emits NO persistence CDC and must not be used on persisted tables (would silently lose durability); the persisted generator keeps the reinsert path. F3: comment that correctness relies on update_in_place's slot-length re-check + reinsert fallback, not on the pre-lock size decision being current. F4: drop the redundant row.clone() inside update_in_place (row is consumed). F5: add concurrent_reads_during_in_place_update_never_tear — 3 readers vs a writer doing 20k same-size in-place updates assert no torn reads. Passes, confirming PublishedRow::replace swaps versions atomically. Full integration suite: 359 passed / 0 failed. --- .../generators/in_memory/queries/update.rs | 48 ++++++++++++++ src/in_memory/pages.rs | 12 +++- tests/worktable/update_in_place_unsized.rs | 66 +++++++++++++++++++ 3 files changed, 125 insertions(+), 1 deletion(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index 7fd9b96..a538c7f 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -55,8 +55,46 @@ 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(); + // 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 size_check = if self.columns.is_sized { quote! {} + } 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); + return core::result::Result::Ok(()); + } } else { quote! { if true { @@ -302,6 +340,14 @@ impl InMemoryGenerator { // 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 @@ -311,6 +357,8 @@ impl InMemoryGenerator { // 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() }; diff --git a/src/in_memory/pages.rs b/src/in_memory/pages.rs index c50393a..b5e8df9 100644 --- a/src/in_memory/pages.rs +++ b/src/in_memory/pages.rs @@ -636,6 +636,15 @@ where /// 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. + /// /// # Safety /// Same contract as [`Self::update`]: `link` must be valid and no other /// mutable references to the row may exist during modification. @@ -658,7 +667,8 @@ where // 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. - let gen_row = ::WrappedRow::from_inner(row.clone()); + // `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)?; diff --git a/tests/worktable/update_in_place_unsized.rs b/tests/worktable/update_in_place_unsized.rs index 4019b51..d8f8519 100644 --- a/tests/worktable/update_in_place_unsized.rs +++ b/tests/worktable/update_in_place_unsized.rs @@ -128,3 +128,69 @@ async fn different_length_update_is_correct() { .unwrap(); assert_eq!(table.select(1).unwrap().payload, "much longer payload"); } + +/// F5 (review): 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), +/// so every `select` should return a well-formed, expected-length string. +#[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 from a torn read. + 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) + ); +} From e14cf65a3d9ec6a19e1a74af4102865b5915e1bc Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:32:51 +0700 Subject: [PATCH 12/20] test: run unsized in-place update regression across all 3 index backends The overwrite in-place fast path re-resolves the row's current Link and republishes through the primary index, so a backend whose link lookup or publication differs could keep the wrong slot or tear a read. Parametrize the unsized in-place suite (same-length stays in place, length-change round-trips, concurrent reads never tear) over WorkTablesIndex, Congee, and Arctic via the `using` keyword, so any divergence fails the test rather than corrupting one index type silently. All 9 pass. --- tests/worktable/update_in_place_unsized.rs | 362 +++++++++++---------- 1 file changed, 188 insertions(+), 174 deletions(-) diff --git a/tests/worktable/update_in_place_unsized.rs b/tests/worktable/update_in_place_unsized.rs index d8f8519..9aba3a0 100644 --- a/tests/worktable/update_in_place_unsized.rs +++ b/tests/worktable/update_in_place_unsized.rs @@ -1,196 +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. + +/// 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, + } + } + ); -#[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); - - assert_eq!(table.select(1).unwrap().payload, "12345678"); - assert_eq!( - before, after, - "same-length unsized update must not reinsert (link changed: {before:?} -> {after:?})" - ); -} + /// 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") + } -/// 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"); -} + #[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); -/// F5 (review): 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), -/// so every `select` should return a well-formed, expected-length string. -#[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), + 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:?})" + ); } - }) - }; - 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 from a torn read. - 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 - ); + /// 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) + ); } - })); - } - - 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); From d37cc170606dc189c200739e3aaef4f10af159ad Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:11:15 +0700 Subject: [PATCH 13/20] perf: subtract reusable data ranges in one pass --- src/persistence/space/data.rs | 256 +++++++++++++++++++++++++++------- 1 file changed, 203 insertions(+), 53 deletions(-) diff --git a/src/persistence/space/data.rs b/src/persistence/space/data.rs index caa1edd..b4df72d 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>, @@ -44,54 +160,9 @@ impl SpaceData

) -> bool { - let mut changed = false; - - for used in used_links { - let used_start = u64::from(used.offset); - let used_end = used_start + u64::from(used.length); - let mut remaining = Vec::with_capacity(self.info.inner.empty_links_list.len() + 1); - - for free in self.info.inner.empty_links_list.drain(..) { - if free.page_id != used.page_id { - remaining.push(free); - continue; - } - - let free_start = u64::from(free.offset); - let free_end = free_start + u64::from(free.length); - let overlap_start = free_start.max(used_start); - let overlap_end = free_end.min(used_end); - if overlap_start >= overlap_end { - remaining.push(free); - continue; - } - - changed = true; - if free_start < overlap_start { - remaining.push(Link { - page_id: free.page_id, - offset: free.offset, - length: (overlap_start - free_start) as u32, - }); - } - if overlap_end < free_end { - remaining.push(Link { - page_id: free.page_id, - offset: overlap_end as u32, - length: (free_end - overlap_end) as u32, - }); - } - } - - self.info.inner.empty_links_list = remaining; - } - - if changed { - self.info.inner.empty_links_list.sort_by_key(|link| { - let page_id: u32 = link.page_id.into(); - (page_id, link.offset) - }); - } + 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 } } @@ -184,7 +255,7 @@ 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.collect::>()) { + if self.consume_reusable_ranges(used_links) { self.save_info().await?; } @@ -248,15 +319,13 @@ where } async fn reclaim_data_pages(&mut self, page_ids: Vec) -> eyre::Result<()> { - let mut page_ids = page_ids + let page_ids = page_ids .into_iter() .filter(|page_id| { let id: u32 = (*page_id).into(); id != 0 && id <= self.last_page_id }) - .collect::>(); - page_ids.sort_unstable(); - page_ids.dedup(); + .collect::>(); if page_ids.is_empty() { return Ok(()); @@ -266,6 +335,8 @@ where .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 @@ -294,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}"); + } + } +} From 9f784ae2dd06e38bda2e87b47eb8eff77cfc7fda Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:12:17 +0700 Subject: [PATCH 14/20] perf: use FxHash for logical WTI mutation stripes --- Cargo.toml | 1 + src/index/persistent_wti.rs | 32 +++++++++++++++++++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b30b580..7b274bd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,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/src/index/persistent_wti.rs b/src/index/persistent_wti.rs index 0853ffd..abbc0ad 100644 --- a/src/index/persistent_wti.rs +++ b/src/index/persistent_wti.rs @@ -10,7 +10,6 @@ //! that marker and derives the real structural metadata itself. use std::array; -use std::collections::hash_map::DefaultHasher; use std::fmt::{self, Debug}; use std::hash::{Hash, Hasher}; use std::ops::RangeBounds; @@ -22,17 +21,28 @@ 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: DefaultHasher has no relationship +// 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 @@ -45,6 +55,9 @@ where { 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], } @@ -96,10 +109,9 @@ where &self.inner } + #[inline] fn mutation_stripe(&self, key: &Q) -> MutexGuard<'_, ()> { - let mut hasher = DefaultHasher::new(); - key.hash(&mut hasher); - self.mutation_stripes[hasher.finish() as usize % MUTATION_STRIPES].lock() + self.mutation_stripes[mutation_stripe_index(key)].lock() } fn next_event_id(&self) -> Id { @@ -267,6 +279,8 @@ where #[cfg(test)] mod tests { + use std::collections::HashSet; + use data_bucket::page::PageId; use super::*; @@ -305,4 +319,12 @@ mod tests { 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); + } } From 62adcac7c42635ed47f49f8798c74332954d4d2c Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:14:22 +0700 Subject: [PATCH 15/20] perf: reuse locked row on in-place fallback --- .../generators/in_memory/queries/update.rs | 6 ++-- src/in_memory/data.rs | 4 +++ src/in_memory/pages.rs | 33 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index a538c7f..d2c5f65 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -367,8 +367,10 @@ impl InMemoryGenerator { return core::result::Result::Ok(()); } - let row_old_for_reinsert = self.0.select(pk.clone()).expect("should not be deleted by other thread"); - if let Err(e) = self.reinsert(row_old_for_reinsert, row_new).await { + // `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); } diff --git a/src/in_memory/data.rs b/src/in_memory/data.rs index 789bb83..a440dd0 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 b5e8df9..156e506 100644 --- a/src/in_memory/pages.rs +++ b/src/in_memory/pages.rs @@ -645,6 +645,12 @@ where /// 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. @@ -909,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 { @@ -1089,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(); From d6935b60057fc6a8dfecf9eaff7d89e0f9c029ee Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:15:48 +0700 Subject: [PATCH 16/20] test: cover async row-lock cancellation cleanup --- src/lock/map.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/lock/map.rs b/src/lock/map.rs index af25c55..088aedf 100644 --- a/src/lock/map.rs +++ b/src/lock/map.rs @@ -98,6 +98,16 @@ impl Drop for MutationGuard { } } +/// 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>>, @@ -119,6 +129,12 @@ 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, @@ -136,6 +152,8 @@ where .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).map(|entry| entry.lock.clone()) } @@ -278,4 +296,24 @@ mod tests { 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)); + } } From 0138bb48c4bf1019076b41fbfd2384decc63fae7 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:19:36 +0700 Subject: [PATCH 17/20] test: harden persistence barriers and quarantine --- src/persistence/space/logical_index.rs | 57 +++++++++++++++++++++ src/persistence/task.rs | 68 +++++++++++++++++++++----- 2 files changed, 113 insertions(+), 12 deletions(-) diff --git a/src/persistence/space/logical_index.rs b/src/persistence/space/logical_index.rs index f5d637d..7f8fd86 100644 --- a/src/persistence/space/logical_index.rs +++ b/src/persistence/space/logical_index.rs @@ -306,6 +306,8 @@ where #[cfg(test)] mod tests { + use std::collections::BTreeMap as StdBTreeMap; + use data_bucket::page::PageId; use super::*; @@ -422,4 +424,59 @@ mod tests { 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/task.rs b/src/persistence/task.rs index 5e4b51e..a80a3a7 100644 --- a/src/persistence/task.rs +++ b/src/persistence/task.rs @@ -403,7 +403,14 @@ mod lifecycle_tests { 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 { @@ -414,7 +421,7 @@ mod lifecycle_tests { batches: Arc::new(AtomicUsize::new(0)), events: Arc::new(ParkingMutex::new(Vec::new())), config, - fail: false, + failure: TestFailure::None, }) } @@ -426,8 +433,14 @@ 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"); @@ -466,7 +479,7 @@ mod lifecycle_tests { batches: batches.clone(), events: Arc::new(ParkingMutex::new(Vec::new())), config: TestConfig, - fail: false, + failure: TestFailure::None, }); task.apply_operation(insert_operation(1)).unwrap(); @@ -481,7 +494,7 @@ mod lifecycle_tests { 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(); @@ -502,7 +515,7 @@ mod lifecycle_tests { batches: Arc::new(AtomicUsize::new(0)), events: events.clone(), config: TestConfig, - fail: false, + failure: TestFailure::None, }); task.apply_operation(insert_operation(1)).unwrap(); @@ -513,6 +526,25 @@ mod lifecycle_tests { 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(); @@ -840,11 +872,23 @@ impl } else { tokio::time::sleep(Duration::from_millis(500)).await; } - } else if let Some(page_ids) = pending_reclaim.take() - && let Err(error) = engine.reclaim_data_pages(page_ids).await - { - engine_lifecycle.fail(error); - return; + } 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; + } } } }; From 1858173642ac47aa1f8f2d0af1eb248fd1d5bcb5 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 07:27:36 +0700 Subject: [PATCH 18/20] perf: generate distinct full-row update paths --- .../generators/in_memory/queries/update.rs | 85 ++++++++++--------- 1 file changed, 46 insertions(+), 39 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/update.rs b/codegen/src/generators/in_memory/queries/update.rs index d2c5f65..949f9dc 100644 --- a/codegen/src/generators/in_memory/queries/update.rs +++ b/codegen/src/generators/in_memory/queries/update.rs @@ -66,8 +66,36 @@ impl InMemoryGenerator { // 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 size_check = if self.columns.is_sized { - quote! {} + let update_body = if self.columns.is_sized { + quote! { + 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() + }; + + 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(()) + } } else if full_row_in_place_eligible { quote! { // No secondary indexes: same-size unsized full-row update may go @@ -93,29 +121,27 @@ impl InMemoryGenerator { return Err(e); } self.0.update_state.remove(&pk); - return core::result::Result::Ok(()); + core::result::Result::Ok(()) } } else { 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); - - return Err(e); - } - + 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 core::result::Result::Ok(()); + return Err(e); } + + self.0.update_state.remove(&pk); + + core::result::Result::Ok(()) } }; @@ -150,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 } } } From 386b1cd6b51a7e757068ac63539a8756c3bb9434 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 08:23:48 +0700 Subject: [PATCH 19/20] fix: re-resolve stale links during full-table scans --- .../generators/in_memory/queries/select.rs | 25 ++++++++++++++++--- .../src/generators/persist/queries/select.rs | 25 ++++++++++++++++--- .../generators/read_only/queries/select.rs | 25 ++++++++++++++++--- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/codegen/src/generators/in_memory/queries/select.rs b/codegen/src/generators/in_memory/queries/select.rs index 489758e..4213912 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/persist/queries/select.rs b/codegen/src/generators/persist/queries/select.rs index 6424795..7627a6f 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/read_only/queries/select.rs b/codegen/src/generators/read_only/queries/select.rs index 11dd830..0adcbe7 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(); From e15b4875e9e66ce6dbf0edaec99301deefc4cded Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 5 Aug 2026 08:23:48 +0700 Subject: [PATCH 20/20] test: exercise core operations across index backends --- tests/worktable/base.rs | 28 +++++++++++++------ tests/worktable/count.rs | 14 +++++++++- tests/worktable/float.rs | 25 ++++++++++++----- tests/worktable/in_place.rs | 14 +++++++++- tests/worktable/unsized_.rs | 39 ++++++++++++++++++--------- tests/worktable/upsert.rs | 14 +++++++++- tests/worktable/vacuum.rs | 14 +++++++++- tests/worktable/vacuum_no_row_loss.rs | 14 +++++++++- 8 files changed, 130 insertions(+), 32 deletions(-) diff --git a/tests/worktable/base.rs b/tests/worktable/base.rs index 2c8fdf4..5ca3886 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 4b98542..a6f366a 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 a924d34..1b8af9f 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 38ce26a..6d82a8a 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/unsized_.rs b/tests/worktable/unsized_.rs index 32734e8..a1b8916 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/upsert.rs b/tests/worktable/upsert.rs index d297a8a..c189856 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 85c18d2..e793b4c 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 9815933..6eaa57c 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);