From b0b4ea609e3d8bc5ceb674b8558b27ea209e17c9 Mon Sep 17 00:00:00 2001
From: Yuchen Liang <yuchen@neon.tech>
Date: Mon, 17 Jun 2024 13:43:04 -0400
Subject: [PATCH] fix comparison to take max lsn with valid lease; fix tests

Signed-off-by: Yuchen Liang <yuchen@neon.tech>
---
 pageserver/src/tenant.rs          | 35 ++++++++++++++++++++-----------
 pageserver/src/tenant/timeline.rs | 16 +++++++-------
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index b505c09d07..2f0e1c8b58 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -4267,7 +4267,18 @@ mod tests {
     async fn test_lsn_leases() -> anyhow::Result<()> {
         let (tenant, ctx) = TenantHarness::create("test_lsn_leases")?.load().await;
         let key = Key::from_hex("010000000033333333444444445500000000").unwrap();
-        let end_lsn = Lsn(0x50);
+
+        let end_lsn = Lsn(0x100);
+        let image_layers = (0x20..=0x90)
+            .step_by(0x10)
+            .map(|n| {
+                (
+                    Lsn(n),
+                    vec![(key, test_img(&format!("data key at {:x}", n)))],
+                )
+            })
+            .collect();
+
         let timeline = tenant
             .create_test_timeline_with_layers(
                 TIMELINE_ID,
@@ -4275,16 +4286,16 @@ mod tests {
                 DEFAULT_PG_VERSION,
                 &ctx,
                 Vec::new(),
-                vec![
-                    (Lsn(0x20), vec![(key, test_img("data key 0"))]),
-                    (Lsn(0x30), vec![(key, test_img("data key 1"))]),
-                    (Lsn(0x40), vec![(key, test_img("data key 2"))]),
-                ],
+                image_layers,
                 end_lsn,
             )
             .await?;
 
-        let _ = timeline.make_lsn_lease(Lsn(0x20), &ctx)?;
+        let leased_lsns = [0x30, 0x50, 0x70];
+        let _: anyhow::Result<_> = leased_lsns.iter().try_for_each(|n| {
+            let _ = timeline.make_lsn_lease(Lsn(*n), &ctx)?;
+            Ok(())
+        });
 
         // Force set disk consistent lsn so we can get the cutoff at `end_lsn`.
 
@@ -4300,13 +4311,13 @@ mod tests {
             )
             .await?;
 
-        // Keeping everything <= Lsn(0x20) b/c leases:
+        // Keeping everything <= Lsn(0x80) b/c leases:
         // 0/10: initdb layer
-        // 0/20: image layer added when creating the timeline.
-        assert_eq!(res.layers_needed_by_leases, 2);
-        // Keeping 0/40 b/c it is the latest layer.
+        // (0/20..=0/70).step_by(0x10): image layers added when creating the timeline.
+        assert_eq!(res.layers_needed_by_leases, 7);
+        // Keeping 0/90 b/c it is the latest layer.
         assert_eq!(res.layers_not_updated, 1);
-        // Removed 0/30.
+        // Removed 0/80.
         assert_eq!(res.layers_removed, 1);
 
         Ok(())
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index 323a972a73..f7572e2335 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -4927,7 +4927,7 @@ impl Timeline {
             return Err(GcError::TimelineCancelled);
         }
 
-        let (horizon_cutoff, pitr_cutoff, retain_lsns, min_lsn_with_valid_lease) = {
+        let (horizon_cutoff, pitr_cutoff, retain_lsns, max_lsn_with_valid_lease) = {
             let gc_info = self.gc_info.read().unwrap();
 
             let horizon_cutoff = min(gc_info.cutoffs.horizon, self.get_disk_consistent_lsn());
@@ -4937,12 +4937,12 @@ impl Timeline {
             // Gets the minimum LSN that holds the valid lease.
             // Caveat: This value could be stale since we rely on refresh_gc_info to invalidate leases,
             // so there could be leases invalidated between the refresh and here.
-            let min_lsn_with_valid_lease = gc_info.leases.first_key_value().map(|(lsn, _)| *lsn);
+            let max_lsn_with_valid_lease = gc_info.leases.last_key_value().map(|(lsn, _)| *lsn);
             (
                 horizon_cutoff,
                 pitr_cutoff,
                 retain_lsns,
-                min_lsn_with_valid_lease,
+                max_lsn_with_valid_lease,
             )
         };
 
@@ -4979,7 +4979,7 @@ impl Timeline {
                 pitr_cutoff,
                 retain_lsns,
                 new_gc_cutoff,
-                min_lsn_with_valid_lease,
+                max_lsn_with_valid_lease,
             )
             .instrument(
                 info_span!("gc_timeline", timeline_id = %self.timeline_id, cutoff = %new_gc_cutoff),
@@ -4998,7 +4998,7 @@ impl Timeline {
         pitr_cutoff: Lsn,
         retain_lsns: Vec<Lsn>,
         new_gc_cutoff: Lsn,
-        min_lsn_with_valid_lease: Option<Lsn>,
+        max_lsn_with_valid_lease: Option<Lsn>,
     ) -> Result<GcResult, GcError> {
         // FIXME: if there is an ongoing detach_from_ancestor, we should just skip gc
 
@@ -5099,7 +5099,7 @@ impl Timeline {
             }
 
             // 4. Is there a valid lease that requires us to keep this layer?
-            if let Some(lsn) = &min_lsn_with_valid_lease {
+            if let Some(lsn) = &max_lsn_with_valid_lease {
                 if &l.get_lsn_range().start <= lsn {
                     debug!(
                         "keeping {} because there is a valid lease preventing GC at {}",
@@ -5133,13 +5133,13 @@ impl Timeline {
             if !layers
                 .image_layer_exists(&l.get_key_range(), &(l.get_lsn_range().end..new_gc_cutoff))
             {
-                debug!("keeping {} because it is the latest layer", l.layer_name());
+                info!("keeping {} because it is the latest layer", l.layer_name());
                 result.layers_not_updated += 1;
                 continue 'outer;
             }
 
             // We didn't find any reason to keep this file, so remove it.
-            debug!(
+            info!(
                 "garbage collecting {} is_dropped: xx is_incremental: {}",
                 l.layer_name(),
                 l.is_incremental(),