diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index cfcfcee222..206d6cdd66 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -181,8 +181,8 @@ impl LsnLease { pub const DEFAULT_LENGTH: Duration = Duration::from_secs(10 * 60); /// Checks whether the lease is expired. - pub fn is_expired(&self) -> bool { - SystemTime::now() > self.valid_until + pub fn is_expired(&self, now: &SystemTime) -> bool { + now > &self.valid_until } } diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index f913070955..538b64aa60 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -21,7 +21,6 @@ use futures::FutureExt; use futures::StreamExt; use pageserver_api::models; use pageserver_api::models::AuxFilePolicy; -use pageserver_api::models::LsnLease; use pageserver_api::models::TimelineState; use pageserver_api::models::TopTenantShardItem; use pageserver_api::models::WalRedoManagerStatus; @@ -32,6 +31,7 @@ use remote_storage::DownloadError; use remote_storage::GenericRemoteStorage; use remote_storage::TimeoutOrCancel; use std::fmt; +use std::time::SystemTime; use storage_broker::BrokerClientChannel; use tokio::io::BufReader; use tokio::sync::watch; @@ -3008,9 +3008,8 @@ impl Tenant { { let mut target = timeline.gc_info.write().unwrap(); - target - .leases - .retain(|_, lease| !LsnLease::is_expired(lease)); + let now = SystemTime::now(); + target.leases.retain(|_, lease| !lease.is_expired(&now)); match gc_cutoffs.remove(&timeline.timeline_id) { Some(cutoffs) => { @@ -4298,7 +4297,9 @@ mod tests { ) .await?; - // Keeping everything <= Lsn(0x20) b/c leases: {0/10, 0/20}; + // Keeping everything <= Lsn(0x20) 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. assert_eq!(res.layers_not_updated, 1); diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 95474b4113..18b3bb61f9 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1568,13 +1568,24 @@ impl Timeline { lsn: Lsn, _ctx: &RequestContext, ) -> anyhow::Result { - let lease = LsnLease { + let mut lease = LsnLease { valid_until: SystemTime::now() + LsnLease::DEFAULT_LENGTH, }; { let mut gc_info = self.gc_info.write().unwrap(); - gc_info.leases.insert(lsn, lease.clone()); + gc_info + .leases + .entry(lsn) + .and_modify(|existing| { + // Insert a lease only if it extends longer than the existing one. + if lease.valid_until > existing.valid_until { + *existing = lease.clone(); + } else { + lease = existing.clone(); + } + }) + .or_insert(lease.clone()); } Ok(lease) } @@ -5036,11 +5047,8 @@ impl Timeline { // 1. it is older than cutoff LSN; // 2. it is older than PITR interval; // 3. it doesn't need to be retained for 'retain_lsns'; - - // TODO(yuchen): we could consider current retain_lsns as infinite leases. - // 3.5. it does not need to be kept for LSNs holding valid leases (logic is very similar to retain_lsns) - - // 4. newer on-disk image layers cover the layer's whole key range + // 4. it does not need to be kept for LSNs holding valid leases (logic is very similar to retain_lsns); + // 5. newer on-disk image layers cover the layer's whole key range // // TODO holding a write lock is too agressive and avoidable let mut guard = self.layers.write().await; @@ -5091,7 +5099,7 @@ impl Timeline { } } - // 3.5 Is there a valid lease that requires us to keep this layer? + // 4. Is there a valid lease that requires us to keep this layer? if let Some(lsn) = &min_lsn_with_valid_lease { if &l.get_lsn_range().start <= lsn { debug!( @@ -5104,7 +5112,7 @@ impl Timeline { } } - // 4. Is there a later on-disk layer for this relation? + // 5. Is there a later on-disk layer for this relation? // // The end-LSN is exclusive, while disk_consistent_lsn is // inclusive. For example, if disk_consistent_lsn is 100, it is