From 300e865bbb24595c67182968aee3847a90ac229c Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 1 Sep 2026 14:12:30 +0700 Subject: [PATCH 1/2] Identify multimap entries by (key, value) and drop the discriminator WorkTablesIndex 0.0.9 removes RandomMultiPair, whose insert scanned every entry sharing the key because the value did not participate in the order. On a table whose non-unique index puts a whole generation under one key that is O(n) per insert: AgentCode measured a one-file update at 698 ms on beta.13 and 15.1 s on beta.14, with marginal cost per row going from 9.04 us to 330 us. MultiPair is now OrdMultiPair, identified and ordered by its (key, value) pair, so an entry is located by binary search and duplicate replacement falls out of the order. The discriminator plumbing that existed only to serve the random representation is gone: - MultiPairRecreate no longer takes a discriminator. A snapshot stores key and value, so reconstruction has nothing left to synthesise. - reconstruct_multi_index_nodes no longer invents a discriminator per entry, which also removes the node-maxima collision it could produce on a damaged file. - Pages are ordered by their minimum rather than their node id. A node id is a page's maximum, and ordering by maximum puts a page that merely ends late ahead of one that starts earlier. Two reconstruction fixtures were built around states only the random representation could produce, where one page's range sat inside another's. A (key, value) index partitions an ordered space, so a page's maximum and its minimum agree on the ordering and that state is unrepresentable. Both are rebuilt as valid partitions and keep what still has meaning: that reconstruction does not depend on the order the pages arrive in. This is a persisted format change. An index written by beta.14 or earlier orders entries within a key by discriminator, which is not (key, value) order, so such a file must be reindexed rather than loaded. --- Cargo.toml | 4 +- src/index/multipair.rs | 20 ++--- src/persistence/space/index/reconstruct.rs | 76 +++++++++--------- .../process_insert_at_big_amount.wt.idx | Bin 65536 -> 65536 bytes .../process_insert_at_big_amount.wt.idx | Bin 49152 -> 49152 bytes .../process_remove_at_node_id.wt.idx | Bin 49152 -> 49152 bytes .../process_split_node.wt.idx | Bin 65536 -> 65536 bytes 7 files changed, 50 insertions(+), 50 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 17e90cfd..e4e6fdc7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["codegen", "examples", "performance_measurement", "performance_measur [package] name = "worktable" -version = "1.0.0-beta.14" +version = "1.0.0-beta.15" edition = "2024" authors = ["Handy-caT"] license = "MIT" @@ -48,7 +48,7 @@ derive_more = { version = "2.0.1", features = ["from", "error", "display", "debu eyre = "0.6.12" fastrand = "2.3.0" futures = "0.3.30" -indexset = { package = "WorkTablesIndex", version = "=0.0.8", default-features = false, features = ["concurrent", "cdc", "multimap"] } +indexset = { package = "WorkTablesIndex", version = "=0.0.9", default-features = false, features = ["concurrent", "cdc", "multimap"] } vanilla_indexset = { package = "indexset", version = "=0.15.0", features = ["concurrent", "cdc", "multimap"] } # indexset = { path = "../indexset", version = "0.15.0", features = ["concurrent", "cdc", "multimap"] } # indexset = { package = "wt-indexset", version = "=0.12.12", features = ["concurrent", "cdc", "multimap"] } diff --git a/src/index/multipair.rs b/src/index/multipair.rs index 38e4503a..f9de1381 100644 --- a/src/index/multipair.rs +++ b/src/index/multipair.rs @@ -1,25 +1,21 @@ use indexset::core::multipair::MultiPair; use indexset::core::pair::Pair; +/// Rebuild a multimap entry from a persisted pair. +/// +/// A `MultiPair` is identified by its `(key, value)` pair, both of which the snapshot +/// stores, so there is nothing to recreate beyond moving them across. The discriminator +/// these methods used to take existed only for the random representation, which could not +/// locate an entry by its value and had to invent an identity instead. pub trait MultiPairRecreate { - fn with_last_discriminator(self, discriminator: u64) -> MultiPair; - fn with_discriminator(self, discriminator: u64) -> MultiPair; + fn recreate(self) -> MultiPair; } impl MultiPairRecreate for Pair { - fn with_last_discriminator(self, discriminator: u64) -> MultiPair { + fn recreate(self) -> MultiPair { MultiPair { key: self.key, value: self.value, - discriminator: fastrand::u64(discriminator..), - } - } - - fn with_discriminator(self, discriminator: u64) -> MultiPair { - MultiPair { - key: self.key, - value: self.value, - discriminator, } } } diff --git a/src/persistence/space/index/reconstruct.rs b/src/persistence/space/index/reconstruct.rs index 0104f019..c6358c11 100644 --- a/src/persistence/space/index/reconstruct.rs +++ b/src/persistence/space/index/reconstruct.rs @@ -43,7 +43,7 @@ pub type PersistedMultiNode = (Pair, Vec>); /// several nodes can share one maximum key (a key's duplicates crossing /// leaf boundaries), and the persisted `(key, link)` id alone cannot /// recover their relative order — links are row locations, uncorrelated -/// with the lost discriminators. In a valid snapshot at most one of those +/// with the lost identities. In a valid snapshot at most one of those /// nodes also carries smaller keys (a run starts only once) and must come /// first; the duplicate-only rest are mutually order-free and get a /// deterministic link tiebreak. @@ -92,16 +92,21 @@ where prepared.push((node_id, entries)); } + // Nodes partition an ordered space, so they are ordered by their minimum, which is + // their first entry. Ordering by the node id instead sorts by each node's maximum, + // which puts a node that merely ends late ahead of one that starts earlier: with + // entries identified by `(key, value)` that produces overlapping node ranges. prepared.sort_by(|(a_id, a_entries), (b_id, b_entries)| { - a_id.key - .cmp(&b_id.key) - .then_with(|| a_entries[0].key.cmp(&b_entries[0].key)) + a_entries[0] + .key + .cmp(&b_entries[0].key) + .then_with(|| a_entries[0].value.cmp(&b_entries[0].value)) + .then_with(|| a_id.key.cmp(&b_id.key)) .then_with(|| a_id.value.cmp(&b_id.value)) }); let mut nodes = Vec::with_capacity(prepared.len()); let mut prev_key: Option = None; - let mut next_discriminator = 1u64; for (node_id, entries) in prepared { let mut node = Vec::with_capacity(entries.len()); for p in entries { @@ -119,14 +124,16 @@ where ); } prev_key = Some(p.key.clone()); - next_discriminator = 1; } + // A `MultiPair` is identified by its `(key, value)` pair, both of which the + // snapshot stores, so reconstruction has nothing left to synthesise. The + // discriminator counter this replaces existed only to separate entries the + // random representation could not tell apart, and it could collide node + // maxima on a damaged file. node.push(MultiPair { key: p.key, value: p.value, - discriminator: next_discriminator.min(u64::MAX - 1), }); - next_discriminator = next_discriminator.saturating_add(1); } nodes.push(node); } @@ -175,17 +182,18 @@ mod tests { } } - /// The review counterexample: a mixed boundary page (tail of key 1, start - /// of key 2's run) whose node-id link sorts AFTER a duplicate-only page - /// of key 2, with equal duplicate counts. Ordering pages by - /// `(node_id key, node_id link)` alone would reconstruct B before A, - /// strand the key-1 entries behind a (2, _) maximum, and give both nodes - /// the maximum (2, discriminator 2) — Equal maxima, one node replacing - /// the other in the outer index. - #[test] + /// A mixed boundary page (tail of key 1, start of key 2's run) beside a + /// page holding the rest of key 2. Entries are identified by `(key, + /// value)`, so the pages partition an ordered space and reconstruction + /// must place them by that order however the input is shuffled. + /// + /// The counterexample this replaces had one page's range sitting inside + /// another's, which the random representation allowed because order + /// within a key came from a discriminator. A `(key, value)` index cannot + /// produce that: a page's maximum and its minimum agree on the ordering. fn mixed_boundary_page_with_adversarial_link_order() { - let page_a = (pair(2, 20), vec![pair(1, 11), pair(2, 21), pair(2, 20)]); - let page_b = (pair(2, 10), vec![pair(2, 12), pair(2, 10)]); + let page_a = (pair(2, 20), vec![pair(1, 11), pair(2, 10), pair(2, 20)]); + let page_b = (pair(2, 31), vec![pair(2, 30), pair(2, 31)]); // Input order must not matter; test both. for pages in [ @@ -200,16 +208,16 @@ mod tests { // Every persisted entry survives exactly once. let mut all = flatten(&nodes); all.sort_unstable(); - assert_eq!(all, vec![(1, 11), (2, 10), (2, 12), (2, 20), (2, 21)]); + assert_eq!(all, vec![(1, 11), (2, 10), (2, 20), (2, 30), (2, 31)]); // The mixed boundary node comes first (it carries key 1)... assert_eq!(nodes[0].first().unwrap().key, 1); // ...each node keeps its logical entry order verbatim... - assert_eq!(flatten(&nodes[..1]), vec![(1, 11), (2, 21), (2, 20)]); - assert_eq!(flatten(&nodes[1..]), vec![(2, 12), (2, 10)]); + assert_eq!(flatten(&nodes[..1]), vec![(1, 11), (2, 10), (2, 20)]); + assert_eq!(flatten(&nodes[1..]), vec![(2, 30), (2, 31)]); // ...and each node still ends with its persisted node id entry. assert_eq!((nodes[0].last().unwrap().key, nodes[0].last().unwrap().value), (2, 20)); - assert_eq!((nodes[1].last().unwrap().key, nodes[1].last().unwrap().value), (2, 10)); + assert_eq!((nodes[1].last().unwrap().key, nodes[1].last().unwrap().value), (2, 31)); } } @@ -219,10 +227,10 @@ mod tests { #[test] fn multi_node_straddle_chain() { let pages = vec![ - (pair(3, 5), vec![pair(2, 70), pair(3, 90), pair(3, 5)]), - (pair(2, 40), vec![pair(2, 41), pair(2, 40)]), - (pair(2, 30), vec![pair(2, 31), pair(2, 30)]), - (pair(2, 60), vec![pair(1, 2), pair(1, 1), pair(2, 61), pair(2, 60)]), + (pair(3, 90), vec![pair(2, 61), pair(2, 70), pair(3, 5), pair(3, 90)]), + (pair(2, 60), vec![pair(2, 41), pair(2, 60)]), + (pair(2, 40), vec![pair(2, 31), pair(2, 40)]), + (pair(2, 30), vec![pair(1, 1), pair(1, 2), pair(2, 30)]), ]; let expected_len: usize = pages.iter().map(|(_, e)| e.len()).sum(); @@ -235,16 +243,12 @@ mod tests { assert_eq!(nodes[0].first().unwrap().key, 1); assert_eq!(nodes[3].last().unwrap().key, 3); // Within every node the logical entry order is preserved verbatim. - assert_eq!(flatten(&nodes[..1]), vec![(1, 2), (1, 1), (2, 61), (2, 60)]); - // Key 2's discriminators grow across all four nodes. - let discs: Vec = nodes - .iter() - .flatten() - .filter(|p| p.key == 2) - .map(|p| p.discriminator) - .collect(); - for w in discs.windows(2) { - assert!(w[0] < w[1], "key 2 discriminators not strictly increasing: {discs:?}"); + assert_eq!(flatten(&nodes[..1]), vec![(1, 1), (1, 2), (2, 30)]); + // Every stored entry is distinct. That is what the discriminator counter was + // for; identity is now the `(key, value)` pair itself. + let mut seen = std::collections::BTreeSet::new(); + for p in nodes.iter().flatten() { + assert!(seen.insert((p.key, p.value)), "duplicate entry {:?}", (p.key, p.value)); } } diff --git a/tests/data/space_index_unsized/indexset/process_insert_at_big_amount.wt.idx b/tests/data/space_index_unsized/indexset/process_insert_at_big_amount.wt.idx index 94bd08f0ee809bdbaa4ccd48b8408876b7517b75..bd2fc259d35f44ba6a97b31d0c593baaa49ccbec 100644 GIT binary patch delta 18 ZcmZo@U}A(O1E*lf)+fNi=S=8VF0CP?U&j0`b diff --git a/tests/data/space_index_unsized/process_split_node.wt.idx b/tests/data/space_index_unsized/process_split_node.wt.idx index 77b2488a749b07570a4bd03c6bf72b0bedc71b33..b01ebf726562db9c2149c47f8a8b58fa97b10292 100644 GIT binary patch delta 18 ZcmZo@U} Date: Tue, 1 Sep 2026 15:20:59 +0700 Subject: [PATCH 2/2] Take data_bucket 0.5.5, and restore a test attribute data_bucket is pinned exactly here too, so it has to move with the index it carries. The reconstruction fixture rewrite also dropped the #[test] attribute off mixed_boundary_page_with_adversarial_link_order, which left it compiling as an ordinary unused function and silently not running. Clippy caught it; the suite is 267 rather than 266 with it back. --- Cargo.toml | 2 +- src/persistence/space/index/reconstruct.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index e4e6fdc7..6de183fe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,7 +41,7 @@ crc32fast = "1.5.0" # Already in the dependency graph transitively (indexset's concurrent # structures); used directly for read-side grace periods. crossbeam-epoch = "0.9.18" -data_bucket = "=0.5.4" +data_bucket = "=0.5.5" # data_bucket = { git = "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/pathscale/DataBucket", branch = "page_cdc_correction", version = "0.2.7" } # data_bucket = { path = "../DataBucket", version = "0.3.14" } derive_more = { version = "2.0.1", features = ["from", "error", "display", "debug", "into"] } diff --git a/src/persistence/space/index/reconstruct.rs b/src/persistence/space/index/reconstruct.rs index c6358c11..dab116ca 100644 --- a/src/persistence/space/index/reconstruct.rs +++ b/src/persistence/space/index/reconstruct.rs @@ -191,6 +191,7 @@ mod tests { /// another's, which the random representation allowed because order /// within a key came from a discriminator. A `(key, value)` index cannot /// produce that: a page's maximum and its minimum agree on the ordering. + #[test] fn mixed_boundary_page_with_adversarial_link_order() { let page_a = (pair(2, 20), vec![pair(1, 11), pair(2, 10), pair(2, 20)]); let page_b = (pair(2, 31), vec![pair(2, 30), pair(2, 31)]);