From d3d17f2c7c36b0fdf6aa0489b4a72541f3c10cde Mon Sep 17 00:00:00 2001 From: Bojan Serafimov Date: Tue, 10 Jan 2023 17:30:40 -0500 Subject: [PATCH] Simplify --- pageserver/src/tenant/bst_layer_map.rs | 166 +++++++++++++--------- pageserver/src/tenant/latest_layer_map.rs | 48 +------ pageserver/src/tenant/layer_map.rs | 18 ++- 3 files changed, 119 insertions(+), 113 deletions(-) diff --git a/pageserver/src/tenant/bst_layer_map.rs b/pageserver/src/tenant/bst_layer_map.rs index ea7bfdb691..48fbe451a3 100644 --- a/pageserver/src/tenant/bst_layer_map.rs +++ b/pageserver/src/tenant/bst_layer_map.rs @@ -43,20 +43,19 @@ impl PersistentLayerMap { } } - self.head.insert(key, lsn.clone(), value, is_image); + // Insert into data structure + if is_image { + self.head.image_coverage.insert(key, lsn.clone(), value); + } else { + self.head + .delta_coverage + .insert(key.clone(), lsn.clone(), value); + } // Remember history. Clone is O(1) self.historic.insert(lsn.start, self.head.clone()); } - pub fn query(self: &Self, key: i128, lsn: u64) -> (Option, Option) { - let version = match self.historic.range(..=lsn).rev().next() { - Some((_, v)) => v, - None => return (None, None), - }; - version.query(key) - } - pub fn get_version(self: &Self, lsn: u64) -> Option<&LatestLayerMap> { match self.historic.range(..=lsn).rev().next() { Some((_, v)) => Some(v), @@ -86,18 +85,21 @@ fn test_persistent_simple() { map.insert(5..6, 120..121, "Layer 3".to_string(), true); // After Layer 1 insertion - assert_eq!(map.query(1, 105).1, Some("Layer 1".to_string())); - assert_eq!(map.query(4, 105).1, Some("Layer 1".to_string())); + let version = map.get_version(105).unwrap(); + assert_eq!(version.image_coverage.query(1), Some("Layer 1".to_string())); + assert_eq!(version.image_coverage.query(4), Some("Layer 1".to_string())); // After Layer 2 insertion - assert_eq!(map.query(4, 115).1, Some("Layer 2".to_string())); - assert_eq!(map.query(8, 115).1, Some("Layer 2".to_string())); - assert_eq!(map.query(11, 115).1, None); + let version = map.get_version(115).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Layer 2".to_string())); + assert_eq!(version.image_coverage.query(8), Some("Layer 2".to_string())); + assert_eq!(version.image_coverage.query(11), None); // After Layer 3 insertion - assert_eq!(map.query(4, 125).1, Some("Layer 2".to_string())); - assert_eq!(map.query(5, 125).1, Some("Layer 3".to_string())); - assert_eq!(map.query(7, 125).1, Some("Layer 2".to_string())); + let version = map.get_version(125).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Layer 2".to_string())); + assert_eq!(version.image_coverage.query(5), Some("Layer 3".to_string())); + assert_eq!(version.image_coverage.query(7), Some("Layer 2".to_string())); } /// Cover simple off-by-one edge cases @@ -107,15 +109,19 @@ fn test_off_by_one() { map.insert(3..5, 100..110, "Layer 1".to_string(), true); // Check different LSNs - assert_eq!(map.query(4, 99).1, None); - assert_eq!(map.query(4, 100).1, Some("Layer 1".to_string())); - assert_eq!(map.query(4, 110).1, Some("Layer 1".to_string())); + let version = map.get_version(99); + assert!(version.is_none()); + let version = map.get_version(100).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Layer 1".to_string())); + let version = map.get_version(110).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Layer 1".to_string())); // Check different keys - assert_eq!(map.query(2, 105).1, None); - assert_eq!(map.query(3, 105).1, Some("Layer 1".to_string())); - assert_eq!(map.query(4, 105).1, Some("Layer 1".to_string())); - assert_eq!(map.query(5, 105).1, None); + let version = map.get_version(105).unwrap(); + assert_eq!(version.image_coverage.query(2), None); + assert_eq!(version.image_coverage.query(3), Some("Layer 1".to_string())); + assert_eq!(version.image_coverage.query(4), Some("Layer 1".to_string())); + assert_eq!(version.image_coverage.query(5), None); } /// Cover edge cases where layers begin or end on the same key @@ -129,18 +135,38 @@ fn test_key_collision() { map.insert(3..4, 200..210, "Layer 20".to_string(), true); // Check after layer 11 - assert_eq!(map.query(2, 105).1, None); - assert_eq!(map.query(3, 105).1, Some("Layer 10".to_string())); - assert_eq!(map.query(5, 105).1, Some("Layer 11".to_string())); - assert_eq!(map.query(7, 105).1, Some("Layer 11".to_string())); - assert_eq!(map.query(8, 105).1, None); + let version = map.get_version(105).unwrap(); + assert_eq!(version.image_coverage.query(2), None); + assert_eq!( + version.image_coverage.query(3), + Some("Layer 10".to_string()) + ); + assert_eq!( + version.image_coverage.query(5), + Some("Layer 11".to_string()) + ); + assert_eq!( + version.image_coverage.query(7), + Some("Layer 11".to_string()) + ); + assert_eq!(version.image_coverage.query(8), None); // Check after layer 20 - assert_eq!(map.query(2, 205).1, None); - assert_eq!(map.query(3, 205).1, Some("Layer 20".to_string())); - assert_eq!(map.query(5, 205).1, Some("Layer 11".to_string())); - assert_eq!(map.query(7, 205).1, Some("Layer 11".to_string())); - assert_eq!(map.query(8, 205).1, None); + let version = map.get_version(205).unwrap(); + assert_eq!(version.image_coverage.query(2), None); + assert_eq!( + version.image_coverage.query(3), + Some("Layer 20".to_string()) + ); + assert_eq!( + version.image_coverage.query(5), + Some("Layer 11".to_string()) + ); + assert_eq!( + version.image_coverage.query(7), + Some("Layer 11".to_string()) + ); + assert_eq!(version.image_coverage.query(8), None); } /// Test when rectangles have nontrivial height and possibly overlap @@ -163,31 +189,34 @@ fn test_persistent_overlapping() { map.insert(0..9, 150..301, "Layer 6".to_string(), true); // After layer 4 insertion - assert_eq!(map.query(0, 135).1, Some("Layer 4".to_string())); - assert_eq!(map.query(1, 135).1, Some("Layer 1".to_string())); - assert_eq!(map.query(2, 135).1, Some("Layer 4".to_string())); - assert_eq!(map.query(4, 135).1, Some("Layer 2".to_string())); - assert_eq!(map.query(5, 135).1, Some("Layer 4".to_string())); - assert_eq!(map.query(7, 135).1, Some("Layer 3".to_string())); - assert_eq!(map.query(8, 135).1, Some("Layer 4".to_string())); + let version = map.get_version(135).unwrap(); + assert_eq!(version.image_coverage.query(0), Some("Layer 4".to_string())); + assert_eq!(version.image_coverage.query(1), Some("Layer 1".to_string())); + assert_eq!(version.image_coverage.query(2), Some("Layer 4".to_string())); + assert_eq!(version.image_coverage.query(4), Some("Layer 2".to_string())); + assert_eq!(version.image_coverage.query(5), Some("Layer 4".to_string())); + assert_eq!(version.image_coverage.query(7), Some("Layer 3".to_string())); + assert_eq!(version.image_coverage.query(8), Some("Layer 4".to_string())); // After layer 5 insertion - assert_eq!(map.query(0, 145).1, Some("Layer 5".to_string())); - assert_eq!(map.query(1, 145).1, Some("Layer 5".to_string())); - assert_eq!(map.query(2, 145).1, Some("Layer 5".to_string())); - assert_eq!(map.query(4, 145).1, Some("Layer 5".to_string())); - assert_eq!(map.query(5, 145).1, Some("Layer 5".to_string())); - assert_eq!(map.query(7, 145).1, Some("Layer 3".to_string())); - assert_eq!(map.query(8, 145).1, Some("Layer 5".to_string())); + let version = map.get_version(145).unwrap(); + assert_eq!(version.image_coverage.query(0), Some("Layer 5".to_string())); + assert_eq!(version.image_coverage.query(1), Some("Layer 5".to_string())); + assert_eq!(version.image_coverage.query(2), Some("Layer 5".to_string())); + assert_eq!(version.image_coverage.query(4), Some("Layer 5".to_string())); + assert_eq!(version.image_coverage.query(5), Some("Layer 5".to_string())); + assert_eq!(version.image_coverage.query(7), Some("Layer 3".to_string())); + assert_eq!(version.image_coverage.query(8), Some("Layer 5".to_string())); // After layer 6 insertion - assert_eq!(map.query(0, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(1, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(2, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(4, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(5, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(7, 155).1, Some("Layer 6".to_string())); - assert_eq!(map.query(8, 155).1, Some("Layer 6".to_string())); + let version = map.get_version(155).unwrap(); + assert_eq!(version.image_coverage.query(0), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(1), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(2), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(4), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(5), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(7), Some("Layer 6".to_string())); + assert_eq!(version.image_coverage.query(8), Some("Layer 6".to_string())); } /// Wrapper for PersistentLayerMap that allows us to hack around the lack @@ -335,7 +364,6 @@ impl RetroactiveLayerMap { Ok(&self.map) } - } #[test] @@ -351,7 +379,11 @@ fn test_retroactive_regression_1() { map.rebuild(); - assert_eq!(map.get().unwrap().query(100, 23761457).0, Some("sdfsdfs".to_string())); + let version = map.get().unwrap().get_version(23761457).unwrap(); + assert_eq!( + version.delta_coverage.query(100), + Some("sdfsdfs".to_string()) + ); } #[test] @@ -371,17 +403,23 @@ fn test_retroactive_simple() { map.rebuild(); // Query key 4 - assert_eq!(map.get().unwrap().query(4, 90).1, None); - assert_eq!(map.get().unwrap().query(4, 102).1, Some("Image 1".to_string())); - assert_eq!(map.get().unwrap().query(4, 107).1, Some("Delta 1".to_string())); - assert_eq!(map.get().unwrap().query(4, 115).1, Some("Image 2".to_string())); - assert_eq!(map.get().unwrap().query(4, 125).1, Some("Image 3".to_string())); + let version = map.get().unwrap().get_version(90); + assert!(version.is_none()); + let version = map.get().unwrap().get_version(102).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Image 1".to_string())); + let version = map.get().unwrap().get_version(107).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Delta 1".to_string())); + let version = map.get().unwrap().get_version(115).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Image 2".to_string())); + let version = map.get().unwrap().get_version(125).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Image 3".to_string())); // Remove Image 3 map.remove(4..6, 120..121, true); map.rebuild(); // Check deletion worked - assert_eq!(map.get().unwrap().query(4, 125).1, Some("Image 2".to_string())); - assert_eq!(map.get().unwrap().query(8, 125).1, Some("Image 4".to_string())); + let version = map.get().unwrap().get_version(125).unwrap(); + assert_eq!(version.image_coverage.query(4), Some("Image 2".to_string())); + assert_eq!(version.image_coverage.query(8), Some("Image 4".to_string())); } diff --git a/pageserver/src/tenant/latest_layer_map.rs b/pageserver/src/tenant/latest_layer_map.rs index 0e84a1e8ff..8b625862e4 100644 --- a/pageserver/src/tenant/latest_layer_map.rs +++ b/pageserver/src/tenant/latest_layer_map.rs @@ -1,10 +1,12 @@ -use std::ops::Range; - use super::layer_coverage::LayerCoverage; +/// Separate coverage data structure for each layer type. +/// +/// This is just a tuple but with the elements named to +/// prevent bugs. pub struct LatestLayerMap { - image_coverage: LayerCoverage, - delta_coverage: LayerCoverage, + pub image_coverage: LayerCoverage, + pub delta_coverage: LayerCoverage, } impl Default for LatestLayerMap { @@ -17,44 +19,6 @@ impl Default for LatestLayerMap { } impl LatestLayerMap { - pub fn insert( - self: &mut Self, - key: Range, - lsn: Range, - value: Value, - is_image: bool, - ) { - if is_image { - self.image_coverage.insert(key, lsn, value); - } else { - self.delta_coverage.insert(key.clone(), lsn.clone(), value); - } - } - - pub fn query(self: &Self, key: i128) -> (Option, Option) { - let delta = self.delta_coverage.query(key); - let image = self.image_coverage.query(key); - (delta, image) - } - - pub fn image_coverage( - self: &Self, - key: Range, - ) -> impl '_ + Iterator)> { - self.image_coverage.range(key) - } - - pub fn image_iter(self: &Self) -> impl '_ + Iterator)> { - self.image_coverage.iter() - } - - pub fn delta_coverage( - self: &Self, - key: Range, - ) -> impl '_ + Iterator)> { - self.delta_coverage.range(key) - } - pub fn clone(self: &Self) -> Self { Self { image_coverage: self.image_coverage.clone(), diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 9ddfe732cd..34c80951cf 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -91,7 +91,11 @@ where /// 'open' and 'frozen' layers! /// pub fn search(&self, key: Key, end_lsn: Lsn) -> Option> { - match self.index.get().unwrap().query(key.to_i128(), end_lsn.0 - 1) { + let version = self.index.get().unwrap().get_version(end_lsn.0 - 1)?; + let latest_delta = version.delta_coverage.query(key.to_i128()); + let latest_image = version.image_coverage.query(key.to_i128()); + + match (latest_delta, latest_image) { (None, None) => None, (None, Some(image)) => { let lsn_floor = image.get_lsn_range().start; @@ -206,12 +210,12 @@ where }; // Check the start is covered - if !layer_covers(version.query(start).1) { + if !layer_covers(version.image_coverage.query(start)) { return Ok(false); } // Check after all changes of coverage - for (_, change_val) in version.image_coverage(start..end) { + for (_, change_val) in version.image_coverage.range(start..end) { if !layer_covers(change_val) { return Ok(false); } @@ -248,10 +252,10 @@ where // Initialize loop variables let mut coverage: Vec<(Range, Option>)> = vec![]; let mut current_key = start.clone(); - let mut current_val = version.query(start).1; + let mut current_val = version.image_coverage.query(start); // Loop through the change events and push intervals - for (change_key, change_val) in version.image_coverage(start..end) { + for (change_key, change_val) in version.image_coverage.range(start..end) { let kr = Key::from_i128(current_key)..Key::from_i128(change_key); coverage.push((kr, current_val.take())); current_key = change_key.clone(); @@ -290,10 +294,10 @@ where // Initialize loop variables let mut max_stacked_deltas = 0; let mut current_key = start.clone(); - let mut current_val = version.query(start).0; + let mut current_val = version.delta_coverage.query(start); // Loop through the delta coverage and recurse on each part - for (change_key, change_val) in version.delta_coverage(start..end) { + for (change_key, change_val) in version.delta_coverage.range(start..end) { // If there's a relevant delta in this part, add 1 and recurse down if let Some(val) = current_val { if val.get_lsn_range().end.0 >= lsn.start.0 {