From ec53c5ca2ecb79be760d604f53dec49595ec5605 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 28 Apr 2023 17:20:18 +0300 Subject: [PATCH] revert: "Add check for duplicates of generated image layers" (#4104) This reverts commit 732acc5. Reverted PR: #3869 As noted in PR #4094, we do in fact try to insert duplicates to the layer map, if L0->L1 compaction is interrupted. We do not have a proper fix for that right now, and we are in a hurry to make a release to production, so revert the changes related to this to the state that we have in production currently. We know that we have a bug here, but better to live with the bug that we've had in production for a long time, than rush a fix to production without testing it in staging first. Cc: #4094, #4088 --- pageserver/benches/bench_layer_map.rs | 4 +-- pageserver/src/tenant.rs | 7 +----- pageserver/src/tenant/layer_map.rs | 23 +++++++---------- .../layer_map/historic_layer_coverage.rs | 8 ------ pageserver/src/tenant/timeline.rs | 25 +++++++------------ 5 files changed, 21 insertions(+), 46 deletions(-) diff --git a/pageserver/benches/bench_layer_map.rs b/pageserver/benches/bench_layer_map.rs index 8f139a6596..ee5980212e 100644 --- a/pageserver/benches/bench_layer_map.rs +++ b/pageserver/benches/bench_layer_map.rs @@ -33,7 +33,7 @@ fn build_layer_map(filename_dump: PathBuf) -> LayerMap { min_lsn = min(min_lsn, lsn_range.start); max_lsn = max(max_lsn, Lsn(lsn_range.end.0 - 1)); - updates.insert_historic(Arc::new(layer)).unwrap(); + updates.insert_historic(Arc::new(layer)); } println!("min: {min_lsn}, max: {max_lsn}"); @@ -215,7 +215,7 @@ fn bench_sequential(c: &mut Criterion) { is_incremental: false, short_id: format!("Layer {}", i), }; - updates.insert_historic(Arc::new(layer)).unwrap(); + updates.insert_historic(Arc::new(layer)); } updates.flush(); println!("Finished layer map init in {:?}", now.elapsed()); diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index d69d5e4b45..5cfc466111 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -271,10 +271,7 @@ impl UninitializedTimeline<'_> { .await .context("Failed to flush after basebackup import")?; - // Initialize without loading the layer map. We started with an empty layer map, and already - // updated it for the layers that we created during the import. - let mut timelines = self.owning_tenant.timelines.lock().unwrap(); - self.initialize_with_lock(ctx, &mut timelines, false, true) + self.initialize(ctx) } fn raw_timeline(&self) -> anyhow::Result<&Arc> { @@ -2355,8 +2352,6 @@ impl Tenant { ) })?; - // Initialize the timeline without loading the layer map, because we already updated the layer - // map above, when we imported the datadir. let timeline = { let mut timelines = self.timelines.lock().unwrap(); raw_timeline.initialize_with_lock(ctx, &mut timelines, false, true)? diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 0ee0c6f77d..8d06ccd565 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -51,7 +51,7 @@ use crate::keyspace::KeyPartitioning; use crate::repository::Key; use crate::tenant::storage_layer::InMemoryLayer; use crate::tenant::storage_layer::Layer; -use anyhow::{bail, Result}; +use anyhow::Result; use std::collections::VecDeque; use std::ops::Range; use std::sync::Arc; @@ -125,7 +125,7 @@ where /// /// Insert an on-disk layer. /// - pub fn insert_historic(&mut self, layer: Arc) -> anyhow::Result<()> { + pub fn insert_historic(&mut self, layer: Arc) { self.layer_map.insert_historic_noflush(layer) } @@ -273,21 +273,16 @@ where /// /// Helper function for BatchedUpdates::insert_historic /// - pub(self) fn insert_historic_noflush(&mut self, layer: Arc) -> anyhow::Result<()> { - let key = historic_layer_coverage::LayerKey::from(&*layer); - if self.historic.contains(&key) { - bail!( - "Attempt to insert duplicate layer {} in layer map", - layer.short_id() - ); - } - self.historic.insert(key, Arc::clone(&layer)); + pub(self) fn insert_historic_noflush(&mut self, layer: Arc) { + // TODO: See #3869, resulting #4088, attempted fix and repro #4094 + self.historic.insert( + historic_layer_coverage::LayerKey::from(&*layer), + Arc::clone(&layer), + ); if Self::is_l0(&layer) { self.l0_delta_layers.push(layer); } - - Ok(()) } /// @@ -839,7 +834,7 @@ mod tests { let expected_in_counts = (1, usize::from(expected_l0)); - map.batch_update().insert_historic(remote.clone()).unwrap(); + map.batch_update().insert_historic(remote.clone()); assert_eq!(count_layer_in(&map, &remote), expected_in_counts); let replaced = map diff --git a/pageserver/src/tenant/layer_map/historic_layer_coverage.rs b/pageserver/src/tenant/layer_map/historic_layer_coverage.rs index 1fdcd5e5a4..b63c361314 100644 --- a/pageserver/src/tenant/layer_map/historic_layer_coverage.rs +++ b/pageserver/src/tenant/layer_map/historic_layer_coverage.rs @@ -417,14 +417,6 @@ impl BufferedHistoricLayerCoverage { } } - pub fn contains(&self, layer_key: &LayerKey) -> bool { - match self.buffer.get(layer_key) { - Some(None) => false, // layer remove was buffered - Some(_) => true, // layer insert was buffered - None => self.layers.contains_key(layer_key), // no buffered ops for this layer - } - } - pub fn insert(&mut self, layer_key: LayerKey, value: Value) { self.buffer.insert(layer_key, Some(value)); } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 5c671ffd63..8768841d87 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1484,7 +1484,7 @@ impl Timeline { trace!("found layer {}", layer.path().display()); total_physical_size += file_size; - updates.insert_historic(Arc::new(layer))?; + updates.insert_historic(Arc::new(layer)); num_layers += 1; } else if let Some(deltafilename) = DeltaFileName::parse_str(&fname) { // Create a DeltaLayer struct for each delta file. @@ -1516,7 +1516,7 @@ impl Timeline { trace!("found layer {}", layer.path().display()); total_physical_size += file_size; - updates.insert_historic(Arc::new(layer))?; + updates.insert_historic(Arc::new(layer)); num_layers += 1; } else if fname == METADATA_FILE_NAME || fname.ends_with(".old") { // ignore these @@ -1590,7 +1590,7 @@ impl Timeline { // remote index file? // If so, rename_to_backup those files & replace their local layer with // a RemoteLayer in the layer map so that we re-download them on-demand. - if let Some(local_layer) = &local_layer { + if let Some(local_layer) = local_layer { let local_layer_path = local_layer .local_path() .expect("caller must ensure that local_layers only contains local layers"); @@ -1615,6 +1615,7 @@ impl Timeline { anyhow::bail!("could not rename file {local_layer_path:?}: {err:?}"); } else { self.metrics.resident_physical_size_gauge.sub(local_size); + updates.remove_historic(local_layer); // fall-through to adding the remote layer } } else { @@ -1650,11 +1651,7 @@ impl Timeline { ); let remote_layer = Arc::new(remote_layer); - if let Some(local_layer) = &local_layer { - updates.replace_historic(local_layer, remote_layer)?; - } else { - updates.insert_historic(remote_layer)?; - } + updates.insert_historic(remote_layer); } LayerFileName::Delta(deltafilename) => { // Create a RemoteLayer for the delta file. @@ -1678,11 +1675,7 @@ impl Timeline { LayerAccessStats::for_loading_layer(LayerResidenceStatus::Evicted), ); let remote_layer = Arc::new(remote_layer); - if let Some(local_layer) = &local_layer { - updates.replace_historic(local_layer, remote_layer)?; - } else { - updates.insert_historic(remote_layer)?; - } + updates.insert_historic(remote_layer); } } } @@ -2730,7 +2723,7 @@ impl Timeline { .write() .unwrap() .batch_update() - .insert_historic(Arc::new(new_delta))?; + .insert_historic(Arc::new(new_delta)); // update the timeline's physical size let sz = new_delta_path.metadata()?.len(); @@ -2935,7 +2928,7 @@ impl Timeline { self.metrics .resident_physical_size_gauge .add(metadata.len()); - updates.insert_historic(Arc::new(l))?; + updates.insert_historic(Arc::new(l)); } updates.flush(); drop(layers); @@ -3368,7 +3361,7 @@ impl Timeline { new_layer_paths.insert(new_delta_path, LayerFileMetadata::new(metadata.len())); let x: Arc = Arc::new(l); - updates.insert_historic(x)?; + updates.insert_historic(x); } // Now that we have reshuffled the data to set of new delta layers, we can