diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 3952c5c2a4..4082df5c82 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -3444,13 +3444,7 @@ impl Timeline { deltas_to_compact }; - // Now that we have reshuffled the data to set of new delta layers, we can - // delete the old ones - let mut layer_names_to_delete = Vec::with_capacity(remove_layers.len()); - - for delta in &remove_layers { - layer_names_to_delete.push(delta.layer_desc().filename()); - } + // deletion will happen later, the layer file manager sets wanted_garbage_collected guard.finish_compact_l0(&layer_removal_cs, remove_layers, &insert_layers)?; @@ -3756,7 +3750,7 @@ impl Timeline { l.filename(), l.is_incremental(), ); - layers_to_remove.push(Arc::clone(&l)); + layers_to_remove.push(l); } self.wanted_image_layers .lock() @@ -3771,15 +3765,13 @@ impl Timeline { // Actually delete the layers from disk and remove them from the map. // (couldn't do this in the loop above, because you cannot modify a collection // while iterating it. BTreeMap::retain() would be another option) - let mut layer_names_to_delete = Vec::with_capacity(layers_to_remove.len()); let gc_layers = layers_to_remove .iter() .map(|x| guard.get_from_desc(x)) - .collect(); - for doomed_layer in layers_to_remove { - layer_names_to_delete.push(doomed_layer.filename()); - result.layers_removed += 1; - } + .collect::>>(); + + result.layers_removed = gc_layers.len() as u64; + let apply = guard.finish_gc_timeline(&layer_removal_cs, gc_layers)?; if result.layers_removed != 0 { diff --git a/pageserver/src/tenant/timeline/layer_manager.rs b/pageserver/src/tenant/timeline/layer_manager.rs index ca7f14c6e0..9e7d54c249 100644 --- a/pageserver/src/tenant/timeline/layer_manager.rs +++ b/pageserver/src/tenant/timeline/layer_manager.rs @@ -232,7 +232,7 @@ impl LayerManager { doomed_layer, &mut updates, &mut self.layer_fmgr, - )?; // FIXME: schedule succeeded deletions in timeline.rs `gc_timeline` instead of in batch? + )?; } Ok(ApplyGcResultGuard(updates)) } @@ -257,7 +257,6 @@ impl LayerManager { mapping: &mut LayerFileManager, ) -> anyhow::Result<()> { let desc = layer.layer_desc(); - layer.garbage_collect(); // TODO Removing from the bottom of the layer map is expensive. // Maybe instead discard all layer map historic versions that @@ -265,7 +264,8 @@ impl LayerManager { // and mark what we can't delete yet as deleted from the layer // map index without actually rebuilding the index. updates.remove_historic(desc); - mapping.remove(layer); + mapping.remove(&layer); + layer.garbage_collect(); Ok(()) } @@ -303,7 +303,7 @@ impl LayerFileManager { Self(HashMap::new()) } - pub(crate) fn remove(&mut self, layer: Arc) { + pub(crate) fn remove(&mut self, layer: &Arc) { let present = self.0.remove(&layer.layer_desc().key()); if present.is_none() && cfg!(debug_assertions) { panic!(