From 2f9a04099816a6290852d7110b84bfdb28a9c4d2 Mon Sep 17 00:00:00 2001
From: John Spray <john@neon.tech>
Date: Tue, 29 Oct 2024 09:58:08 +0000
Subject: [PATCH] Don't upload duplicate manifests

---
 pageserver/src/tenant.rs                      | 26 ++++++++++++++-----
 .../tenant/remote_timeline_client/manifest.rs |  4 +--
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index f1f0898afe765..04854855bef91 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -304,7 +304,9 @@ pub struct Tenant {
     /// Serialize writes of the tenant manifest to remote storage.  If there are concurrent operations
     /// affecting the manifest, such as timeline deletion and timeline offload, they must wait for
     /// each other (this could be optimized to coalesce writes if necessary).
-    tenant_manifest_upload: tokio::sync::Mutex<()>,
+    ///
+    /// The contents of the Mutex are the last manifest we successfully uploaded
+    tenant_manifest_upload: tokio::sync::Mutex<Option<TenantManifest>>,
 
     // This mutex prevents creation of new timelines during GC.
     // Adding yet another mutex (in addition to `timelines`) is needed because holding
@@ -3123,7 +3125,7 @@ impl Tenant {
             }
         }
 
-        let tenant_manifest = self.tenant_manifest();
+        let tenant_manifest = self.build_tenant_manifest();
         // TODO: generation support
         let generation = remote_timeline_client::TENANT_MANIFEST_GENERATION;
         for child_shard in child_shards {
@@ -3318,7 +3320,8 @@ impl Tenant {
             .unwrap_or(self.conf.default_tenant_conf.lsn_lease_length)
     }
 
-    fn tenant_manifest(&self) -> TenantManifest {
+    /// Generate an up-to-date TenantManifest based on the state of this Tenant.
+    fn build_tenant_manifest(&self) -> TenantManifest {
         let timelines_offloaded = self.timelines_offloaded.lock().unwrap();
 
         let mut timeline_manifests = timelines_offloaded
@@ -4712,7 +4715,7 @@ impl Tenant {
         // Only one manifest write may be done at at time, and the contents of the manifest
         // must be loaded while holding this lock. This makes it safe to call this function
         // from anywhere without worrying about colliding updates.
-        let _guard = tokio::select! {
+        let mut guard = tokio::select! {
             g = self.tenant_manifest_upload.lock() => {
                 g
             },
@@ -4721,7 +4724,12 @@ impl Tenant {
             }
         };
 
-        let manifest = self.tenant_manifest();
+        let manifest = self.build_tenant_manifest();
+        if Some(&manifest) == (*guard).as_ref() {
+            // Optimisation: skip uploads that don't change anything.
+            return Ok(());
+        }
+
         upload_tenant_manifest(
             &self.remote_storage,
             &self.tenant_shard_id,
@@ -4736,7 +4744,13 @@ impl Tenant {
             } else {
                 TenantManifestError::RemoteStorage(e)
             }
-        })
+        })?;
+
+        // Store the successfully uploaded manifest, so that future callers can avoid
+        // re-uploading the same thing.
+        *guard = Some(manifest);
+
+        Ok(())
     }
 }
 
diff --git a/pageserver/src/tenant/remote_timeline_client/manifest.rs b/pageserver/src/tenant/remote_timeline_client/manifest.rs
index 7d92d45146ae8..c4382cb6480fc 100644
--- a/pageserver/src/tenant/remote_timeline_client/manifest.rs
+++ b/pageserver/src/tenant/remote_timeline_client/manifest.rs
@@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize};
 use utils::{id::TimelineId, lsn::Lsn};
 
 /// Tenant-shard scoped manifest
-#[derive(Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct TenantManifest {
     /// Debugging aid describing the version of this manifest.
     /// Can also be used for distinguishing breaking changes later on.
@@ -23,7 +23,7 @@ pub struct TenantManifest {
 /// Very similar to [`pageserver_api::models::OffloadedTimelineInfo`],
 /// but the two datastructures serve different needs, this is for a persistent disk format
 /// that must be backwards compatible, while the other is only for informative purposes.
-#[derive(Clone, Serialize, Deserialize, Copy)]
+#[derive(Clone, Serialize, Deserialize, Copy, PartialEq, Eq)]
 pub struct OffloadedTimelineManifest {
     pub timeline_id: TimelineId,
     /// Whether the timeline has a parent it has been branched off from or not