From 92d1322cd5e6d1c5168c40ea6eadbd81085331f0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@zenith.tech>
Date: Wed, 9 Mar 2022 15:51:57 +0200
Subject: [PATCH] comments, other cleanup

---
 pageserver/src/layered_repository.rs | 10 +++++++---
 pageserver/src/repository.rs         | 21 +++++++++++----------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/pageserver/src/layered_repository.rs b/pageserver/src/layered_repository.rs
index 49298ff853..43fb6c5c9a 100644
--- a/pageserver/src/layered_repository.rs
+++ b/pageserver/src/layered_repository.rs
@@ -1986,8 +1986,8 @@ mod tests {
     }
 
     //
-    // Insert a bunch of key-value pairs with increasing keys, checkpoint,
-    // repeat 100 times.
+    // Insert 1000 key-value pairs with increasing keys, checkpoint,
+    // repeat 50 times.
     //
     #[test]
     fn test_bulk_insert() -> Result<()> {
@@ -2042,6 +2042,8 @@ mod tests {
 
         let mut parts = KeyPartitioning::new();
 
+        // Track when each page was last modified. Used to assert that
+        // a read sees the latest page version.
         let mut updated = [Lsn(0); NUM_KEYS];
 
         let mut lsn = Lsn(0);
@@ -2081,6 +2083,7 @@ mod tests {
                 updated[blknum] = lsn;
             }
 
+            // Read all the blocks
             for (blknum, last_lsn) in updated.iter().enumerate() {
                 test_key.field6 = blknum as u32;
                 assert_eq!(
@@ -2088,8 +2091,9 @@ mod tests {
                     TEST_IMG(&format!("{} at {}", blknum, last_lsn))
                 );
             }
-            println!("checkpointing {}", lsn);
 
+            // Perform a cycle of checkpoint, compaction, and GC
+            println!("checkpointing {}", lsn);
             let cutoff = tline.get_last_record_lsn();
             tline.update_gc_info(Vec::new(), cutoff);
             tline.checkpoint(CheckpointConfig::Forced)?;
diff --git a/pageserver/src/repository.rs b/pageserver/src/repository.rs
index c92dff661a..05ff449d21 100644
--- a/pageserver/src/repository.rs
+++ b/pageserver/src/repository.rs
@@ -139,16 +139,6 @@ impl Key {
             field6: u32::from_str_radix(&s[28..36], 16)?,
         })
     }
-
-    pub fn to_prefix_128(&self) -> u128 {
-        assert!(self.field1 & 0xf0 == 0);
-        (self.field1 as u128) << 124
-            | (self.field2 as u128) << 92
-            | (self.field3 as u128) << 60
-            | (self.field4 as u128) << 28
-            | (self.field5 as u128) << 20
-            | (self.field6 as u128) >> 12
-    }
 }
 
 //
@@ -365,6 +355,17 @@ pub trait Timeline: Send + Sync {
     /// know anything about them here in the repository.
     fn checkpoint(&self, cconf: CheckpointConfig) -> Result<()>;
 
+    ///
+    /// Tell the implementation how the keyspace should be partitioned.
+    ///
+    /// FIXME: This is quite a hack. The code in pgdatadir_mapping.rs knows
+    /// which keys exist and what is the logical grouping of them. That's why
+    /// the code there (and in keyspace.rs) decides the partitioning, not the
+    /// layered_repository.rs implementation. That's a layering violation:
+    /// the Repository implementation ought to be responsible for the physical
+    /// layout, but currently it's more convenient to do it in pgdatadir_mapping.rs
+    /// rather than in layered_repository.rs.
+    ///
     fn hint_partitioning(&self, partitioning: KeyPartitioning) -> Result<()>;
 
     ///