From f0a0515d4fc280be419502cd6a1fa98537ede775 Mon Sep 17 00:00:00 2001
From: Yuchen Liang <yuchen@neon.tech>
Date: Tue, 11 Jun 2024 14:27:49 -0400
Subject: [PATCH] fix lsn comparison logic

Signed-off-by: Yuchen Liang <yuchen@neon.tech>
---
 pageserver/src/tenant.rs          | 46 +++++++++++++++++++++++++++++++
 pageserver/src/tenant/timeline.rs | 14 +++++-----
 2 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index ee3c356c2d..640a18c13c 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -4278,6 +4278,52 @@ mod tests {
         tline.freeze_and_flush().await.map_err(|e| e.into())
     }
 
+    #[tokio::test]
+    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 timeline = tenant
+            .create_test_timeline_with_layers(
+                TIMELINE_ID,
+                Lsn(0x10),
+                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"))]),
+                ],
+                Lsn(0x40),
+            )
+            .await?;
+
+        let _ = timeline.make_lsn_lease(Lsn(0x30), &ctx)?;
+
+        let gc_info = timeline.gc_info.read().unwrap();
+        assert!(gc_info.leases.contains_key(&Lsn(0x30)));
+        info!("GcCutOff: {:?}", gc_info.cutoffs);
+        let res = tenant
+            .gc_iteration(
+                Some(TIMELINE_ID),
+                0,
+                Duration::ZERO,
+                &CancellationToken::new(),
+                &ctx,
+            )
+            .await?;
+
+        assert_eq!(res.layers_needed_by_leases, 1);
+
+        // 1. Setup tenant and timeline
+        // 2. Make LSN lease for an LSN.
+        // 3. Create two layers:
+        //   - one can be GC-ed, one cannot be GC-ed
+        // 4. Run GC, check `GCResult`.
+        Ok(())
+    }
+
     #[tokio::test]
     async fn test_prohibit_branch_creation_on_garbage_collected_data() -> anyhow::Result<()> {
         let (tenant, ctx) =
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index 86016995fa..662ceceaa5 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -4907,22 +4907,22 @@ impl Timeline {
             return Err(GcError::TimelineCancelled);
         }
 
-        let (horizon_cutoff, pitr_cutoff, retain_lsns, max_lsn_with_valid_lease) = {
+        let (horizon_cutoff, pitr_cutoff, retain_lsns, min_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());
             let pitr_cutoff = gc_info.cutoffs.pitr;
             let retain_lsns = gc_info.retain_lsns.clone();
 
-            // Gets the maximum LSN that holds the valid lease.
+            // 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 max_lsn_with_valid_lease = gc_info.leases.last_key_value().map(|(lsn, _)| *lsn);
+            let min_lsn_with_valid_lease = gc_info.leases.last_key_value().map(|(lsn, _)| *lsn);
             (
                 horizon_cutoff,
                 pitr_cutoff,
                 retain_lsns,
-                max_lsn_with_valid_lease,
+                min_lsn_with_valid_lease,
             )
         };
 
@@ -4959,7 +4959,7 @@ impl Timeline {
                 pitr_cutoff,
                 retain_lsns,
                 new_gc_cutoff,
-                max_lsn_with_valid_lease,
+                min_lsn_with_valid_lease,
             )
             .instrument(
                 info_span!("gc_timeline", timeline_id = %self.timeline_id, cutoff = %new_gc_cutoff),
@@ -4978,7 +4978,7 @@ impl Timeline {
         pitr_cutoff: Lsn,
         retain_lsns: Vec<Lsn>,
         new_gc_cutoff: Lsn,
-        max_lsn_with_valid_lease: Option<Lsn>,
+        min_lsn_with_valid_lease: Option<Lsn>,
     ) -> Result<GcResult, GcError> {
         // FIXME: if there is an ongoing detach_from_ancestor, we should just skip gc
 
@@ -5082,7 +5082,7 @@ impl Timeline {
             }
 
             // 3.5 Is there a valid lease that requires us to keep this layer?
-            if let Some(lsn) = &max_lsn_with_valid_lease {
+            if let Some(lsn) = &min_lsn_with_valid_lease {
                 if &l.get_lsn_range().start <= lsn {
                     debug!(
                         "keeping {} because there is a valid lease preventing GC at {}",