remove_remote_layer: uninteresting refactorings

[koivunej]

In the quest to solve #4745 by moving the download/evictedness to be internally mutable factor of a Layer and get rid of trait PersistentLayer at least for prod usage, layer_removal_cs, we present some misc cleanups.

[koivunej]

Drafting while we see "uninteresting refactorings" produce a new personal record of broken tests.

Narrator: it never did have test failures, only compilation errors, messed up fixups.

[koivunej]

I'll fix the fixup mistake noted above, and split away the preparatory refactorings from these more unrelated fixes/refactorings.

Done in latest force-push, split to #4937.

[github-actions]

1264 tests run: 1212 passed, 0 failed, 52 skipped (full report)

---

Flaky tests (1)

Postgres 15

• test_branching_with_pgbench[cascade-1-10]: release

_{The comment gets automatically updated with the latest test results
2f37588 at 2023-08-09T10:33:11.268Z :recycle:}

[LizardWizzard]

Nice! LGTM

Small diff to reduce number of LayerDesc clones a bit:

diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs
index c4b894fcf..59813b19d 100644
--- a/pageserver/src/tenant/layer_map.rs
+++ b/pageserver/src/tenant/layer_map.rs
@@ -121,7 +121,7 @@ impl BatchedUpdates<'_> {
     ///
     /// This should be called when the corresponding file on disk has been deleted.
     ///
-    pub fn remove_historic(&mut self, layer_desc: PersistentLayerDesc) {
+    pub fn remove_historic(&mut self, layer_desc: &PersistentLayerDesc) {
         self.layer_map.remove_historic_noflush(layer_desc)
     }
 
@@ -253,11 +253,11 @@ impl LayerMap {
     ///
     /// Helper function for BatchedUpdates::remove_historic
     ///
-    pub fn remove_historic_noflush(&mut self, layer_desc: PersistentLayerDesc) {
+    pub fn remove_historic_noflush(&mut self, layer_desc: &PersistentLayerDesc) {
         self.historic
-            .remove(historic_layer_coverage::LayerKey::from(&layer_desc));
+            .remove(historic_layer_coverage::LayerKey::from(layer_desc));
         let layer_key = layer_desc.key();
-        if Self::is_l0(&layer_desc) {
+        if Self::is_l0(layer_desc) {
             let len_before = self.l0_delta_layers.len();
             let mut l0_delta_layers = std::mem::take(&mut self.l0_delta_layers);
             l0_delta_layers.retain(|other| other.key() != layer_key);
@@ -766,8 +766,7 @@ mod tests {
                 expected_in_counts
             );
 
-            map.batch_update()
-                .remove_historic(downloaded.layer_desc().clone());
+            map.batch_update().remove_historic(downloaded.layer_desc());
             assert_eq!(count_layer_in(&map, downloaded.layer_desc()), (0, 0));
         }
 
diff --git a/pageserver/src/tenant/timeline/layer_manager.rs b/pageserver/src/tenant/timeline/layer_manager.rs
index e2f1634ad..824d869be 100644
--- a/pageserver/src/tenant/timeline/layer_manager.rs
+++ b/pageserver/src/tenant/timeline/layer_manager.rs
@@ -277,7 +277,7 @@ impl LayerManager {
         updates: &mut BatchedUpdates<'_>,
         mapping: &mut LayerFileManager,
     ) {
-        updates.remove_historic(layer.layer_desc().clone());
+        updates.remove_historic(layer.layer_desc());
         mapping.remove(layer);
     }
 
@@ -291,7 +291,7 @@ impl LayerManager {
         metrics: &TimelineMetrics,
         mapping: &mut LayerFileManager,
     ) -> anyhow::Result<()> {
-        let desc = layer.layer_desc().to_owned();
+        let desc = layer.layer_desc();
         if !layer.is_remote_layer() {
             layer.delete_resident_layer_file()?;
             metrics.resident_physical_size_gauge.sub(desc.file_size);

[koivunej]

Above in 2f37588.

[koivunej]

test_branching_with_pgbench[cascade-1-10] pg15, release:

stderr: command failed: compute startup timed out; still in Init state

[arpad-m]

Very nice 👍

[koivunej]

Sad this is that this is basically dead code... and it might be now more expensive than doing this in a debug! because on prod debug is never enabled so the formatting would had never been done, but let's hope rustc/llvm decide to move this method call after the debug interest check :)

[arpad-m]

Good point. It's a virtual call, as the layer is a Arc<dyn PersistentLayer>, which can have arbitrary side effects, so I think the optimizer will not move after the check for debug, However, it should be a cheap operation so fine to do.