From d526d60f5f07f33255ab6a73d53ec47ff33eb714 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 24 Aug 2023 11:59:34 +0300 Subject: [PATCH 1/4] stop leaking future layers in remote storage --- pageserver/src/tenant/timeline.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 6245c639dd06..4d5072939b05 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1614,7 +1614,7 @@ impl Timeline { let (conf, tenant_id, timeline_id) = (self.conf, self.tenant_id, self.timeline_id); let span = tracing::Span::current(); - let (loaded_layers, needs_upload, total_physical_size) = tokio::task::spawn_blocking({ + let (loaded_layers, sync, total_physical_size) = tokio::task::spawn_blocking({ move || { let _g = span.entered(); let discovered = init::scan_timeline_dir(&timeline_path)?; @@ -1660,6 +1660,7 @@ impl Timeline { let mut loaded_layers = Vec::new(); let mut needs_upload = Vec::new(); + let mut needs_cleanup = Vec::new(); let mut total_physical_size = 0; for (name, decision) in decided { @@ -1677,12 +1678,8 @@ impl Timeline { path.push(name.file_name()); init::cleanup_future_layer(&path, name, disk_consistent_lsn)?; path.pop(); - } else { - // we cannot do anything for remote layers, but not continuing to - // process it will leave it out index_part.json as well. } - // - // we do not currently schedule deletions for these. + needs_cleanup.push(name); continue; } }; @@ -1736,7 +1733,11 @@ impl Timeline { loaded_layers.push(layer); } - Ok((loaded_layers, needs_upload, total_physical_size)) + Ok(( + loaded_layers, + (needs_upload, needs_cleanup), + total_physical_size, + )) } }) .await @@ -1748,9 +1749,11 @@ impl Timeline { guard.initialize_local_layers(loaded_layers, disk_consistent_lsn + 1); if let Some(rtc) = self.remote_client.as_ref() { + let (needs_upload, needs_cleanup) = sync; for (layer, m) in needs_upload { rtc.schedule_layer_file_upload(&layer.layer_desc().filename(), &m)?; } + rtc.schedule_layer_file_deletion(&needs_cleanup)?; rtc.schedule_index_upload_for_file_changes()?; // Tenant::create_timeline will wait for these uploads to happen before returning, or // on retry. From d86dc6c83c0cb868b19816acc03f4c3763d501bc Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 24 Aug 2023 11:52:29 +0300 Subject: [PATCH 2/4] unrelated: fix(rtc): check if layer file was actually removed --- pageserver/src/tenant/remote_timeline_client.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 835ee6aea436..974585b38f8e 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -653,8 +653,9 @@ impl RemoteTimelineClient { // to syntactically forbid ? or bail! calls here. let no_bail_here = || { for name in names { - upload_queue.latest_files.remove(name); - upload_queue.latest_files_changes_since_metadata_upload_scheduled += 1; + if upload_queue.latest_files.remove(name).is_some() { + upload_queue.latest_files_changes_since_metadata_upload_scheduled += 1; + } } if upload_queue.latest_files_changes_since_metadata_upload_scheduled > 0 { From b45df96c2f1ba2e0400cdfeb108b81a23b1235d7 Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Fri, 25 Aug 2023 11:41:25 +0300 Subject: [PATCH 3/4] refactor: rename sync => to_sync --- pageserver/src/tenant/timeline.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 4d5072939b05..18896237377e 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1614,7 +1614,7 @@ impl Timeline { let (conf, tenant_id, timeline_id) = (self.conf, self.tenant_id, self.timeline_id); let span = tracing::Span::current(); - let (loaded_layers, sync, total_physical_size) = tokio::task::spawn_blocking({ + let (loaded_layers, to_sync, total_physical_size) = tokio::task::spawn_blocking({ move || { let _g = span.entered(); let discovered = init::scan_timeline_dir(&timeline_path)?; @@ -1749,7 +1749,7 @@ impl Timeline { guard.initialize_local_layers(loaded_layers, disk_consistent_lsn + 1); if let Some(rtc) = self.remote_client.as_ref() { - let (needs_upload, needs_cleanup) = sync; + let (needs_upload, needs_cleanup) = to_sync; for (layer, m) in needs_upload { rtc.schedule_layer_file_upload(&layer.layer_desc().filename(), &m)?; } From ecf85decd5efe2e77d05b7a6c87c43417769839f Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Thu, 24 Aug 2023 11:56:36 +0300 Subject: [PATCH 4/4] fix: remove needless move --- pageserver/src/tenant/timeline.rs | 2 +- pageserver/src/tenant/timeline/init.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 18896237377e..ddf6f0bc0afc 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1676,7 +1676,7 @@ impl Timeline { Err(FutureLayer { local }) => { if local.is_some() { path.push(name.file_name()); - init::cleanup_future_layer(&path, name, disk_consistent_lsn)?; + init::cleanup_future_layer(&path, &name, disk_consistent_lsn)?; path.pop(); } needs_cleanup.push(name); diff --git a/pageserver/src/tenant/timeline/init.rs b/pageserver/src/tenant/timeline/init.rs index cdd4227b44d9..a270d96677af 100644 --- a/pageserver/src/tenant/timeline/init.rs +++ b/pageserver/src/tenant/timeline/init.rs @@ -183,7 +183,7 @@ pub(super) fn cleanup_local_file_for_remote( pub(super) fn cleanup_future_layer( path: &Path, - name: LayerFileName, + name: &LayerFileName, disk_consistent_lsn: Lsn, ) -> anyhow::Result<()> { use LayerFileName::*;