diff --git a/pageserver/src/tenant/layer_map/historic_layer_coverage.rs b/pageserver/src/tenant/layer_map/historic_layer_coverage.rs index 14694b3c86..da7830f7fe 100644 --- a/pageserver/src/tenant/layer_map/historic_layer_coverage.rs +++ b/pageserver/src/tenant/layer_map/historic_layer_coverage.rs @@ -34,8 +34,9 @@ impl Ord for LayerKey { // the persistent data structure must be constructed in that order. // NOTE we also care about comparing by (-lsn.end), then key.start, and // then (-key.end) next to make sure that if one layer contains - // another, the larger one is inserted first and the smaller one - // is detected as redundant. + // another, the outer one is inserted first and the inner one + // is detected as redundant. So if we ever find an L1 layer with + // many L0s in it, the L0s are redundant, not the L1. self.lsn .start .cmp(&other.lsn.start) @@ -85,7 +86,10 @@ impl HistoricLayerCoverage { } } - /// Add a layer, returning whether the operation was a noop. + /// Add a layer, returning whether the insertion had any effect. + /// + /// Returns false after inserting a layer whose Key-Lsn space is already + /// covered by a union of previously inserted layers. /// /// Panics if new layer has older lsn.start than an existing layer. /// See BufferedHistoricLayerCoverage for a more general insertion method. @@ -105,12 +109,12 @@ impl HistoricLayerCoverage { &mut self.head.delta_coverage }; - let was_noop = target.insert(layer_key.key, layer_key.lsn.clone(), value); + let changed = target.insert(layer_key.key, layer_key.lsn.clone(), value); // Remember history. Clone is O(1) self.historic.insert(layer_key.lsn.start, self.head.clone()); - was_noop + changed } /// Query at a particular LSN, inclusive @@ -402,7 +406,9 @@ pub struct BufferedHistoricLayerCoverage { /// Redundant layers are ones that are completely covered by a union of other layers. /// If two layers are identical only one of them will be marked as redundant, such that /// it is always safe to remove all redundant layers without seeing any difference in - /// results. + /// results. If a layer is contained in another, the inner one will be marked as + /// redundant. This makes sure that if we have an L1 layer and a set of L0 layers + /// that completely cover it, the L0s are marked redundant, not the L1. /// /// Redundant layers can show up if the pageserver dies during compaction, after /// creating some L1 layers but before deleting the L0 layers. In this case we'd rather @@ -528,12 +534,12 @@ impl BufferedHistoricLayerCoverage { is_image: false, }.., ) { - let was_noop = self + let changed = self .historic_coverage .insert(layer_key.clone(), layer.clone()); num_inserted += 1; - if was_noop { + if !changed { self.redundant_layers .insert(layer_key.clone(), layer.clone()); } diff --git a/pageserver/src/tenant/layer_map/layer_coverage.rs b/pageserver/src/tenant/layer_map/layer_coverage.rs index b208e93211..4c34d6bb06 100644 --- a/pageserver/src/tenant/layer_map/layer_coverage.rs +++ b/pageserver/src/tenant/layer_map/layer_coverage.rs @@ -51,7 +51,10 @@ impl LayerCoverage { self.nodes.insert_mut(key, value); } - /// Insert a layer, returning whether the operation was a NOOP. + /// Insert a layer, returning whether the insertion had any effect. + /// + /// Returns false after inserting a layer whose Key-Lsn space is already + /// covered by a union of previously inserted layers. /// /// Complexity: worst case O(N), in practice O(log N). See NOTE in implementation. pub fn insert(&mut self, key: Range, lsn: Range, value: Value) -> bool { @@ -96,7 +99,7 @@ impl LayerCoverage { self.nodes.remove_mut(k); } - to_update.is_empty() + !to_update.is_empty() } /// Get the latest (by lsn.end) layer at a given key