From e7ca580922b3d50a72f72d6f250745327d1e55dd Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 6 May 2021 21:57:04 +0300
Subject: [PATCH] Improve comments.

---
 pageserver/src/basebackup.rs         | 26 ++++++++++++++++++++++++--
 pageserver/src/restore_local_repo.rs |  9 ++++++---
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/pageserver/src/basebackup.rs b/pageserver/src/basebackup.rs
index 55504b9135..349075f2d3 100644
--- a/pageserver/src/basebackup.rs
+++ b/pageserver/src/basebackup.rs
@@ -7,6 +7,13 @@ use walkdir::WalkDir;
 
 use crate::ZTimelineId;
 
+///
+/// Send a tarball containing a snapshot of all non-relation files in the
+/// PostgreSQL data directory, at given LSN
+///
+/// There must be a snapshot at the given LSN in the snapshots directory, we cannot
+/// reconstruct the state at an arbitrary LSN at the moment.
+///
 pub fn send_snapshot_tarball(
     write: &mut dyn Write,
     timelineid: ZTimelineId,
@@ -48,7 +55,14 @@ pub fn send_snapshot_tarball(
                 ar.append_path_with_name(fullpath, relpath)?;
             } else {
                 trace!("not sending {}", relpath.display());
-                // FIXME: send all files for now
+
+                // FIXME: For now, also send all the relation files.
+                // This really shouldn't be necessary, and kind of
+                // defeats the point of having a page server in the
+                // first place. But it is useful at least when
+                // debugging with the DEBUG_COMPARE_LOCAL option (see
+                // vendor/postgres/src/backend/storage/smgr/pagestore_smgr.c)
+
                 ar.append_path_with_name(fullpath, relpath)?;
             }
         } else {
@@ -56,7 +70,11 @@ pub fn send_snapshot_tarball(
         }
     }
 
-    // FIXME: also send all the WAL
+    // FIXME: Also send all the WAL. The compute node would only need
+    // the WAL that applies to non-relation files, because the page
+    // server handles all the relation files. But we don't have a
+    // mechanism for separating relation and non-relation WAL at the
+    // moment.
     for entry in std::fs::read_dir(&walpath)? {
         let entry = entry?;
         let fullpath = &entry.path();
@@ -146,6 +164,10 @@ fn parse_filename(fname: &str) -> Result<(u32, u32, u32), FilePathError> {
     Ok((relnode, forknum, segno))
 }
 
+///
+/// Parse a path, relative to the root of PostgreSQL data directory, as
+/// a PostgreSQL relation data file.
+///
 fn parse_rel_file_path(path: &str) -> Result<(), FilePathError> {
     /*
      * Relation data files can be in one of the following directories:
diff --git a/pageserver/src/restore_local_repo.rs b/pageserver/src/restore_local_repo.rs
index 143c84fd8d..b77253ae50 100644
--- a/pageserver/src/restore_local_repo.rs
+++ b/pageserver/src/restore_local_repo.rs
@@ -34,9 +34,9 @@ use postgres_ffi::pg_constants;
 use postgres_ffi::xlog_utils::*;
 use zenith_utils::lsn::Lsn;
 
-//
-// Load it all into the page cache.
-//
+///
+/// Load all WAL and all relation data pages from local disk into the repository.
+///
 pub fn restore_timeline(
     conf: &PageServerConf,
     timeline: &dyn Timeline,
@@ -81,6 +81,9 @@ pub fn restore_timeline(
     Ok(())
 }
 
+///
+/// Find latest snapshot in a timeline's 'snapshots' directory
+///
 pub fn find_latest_snapshot(_conf: &PageServerConf, timeline: ZTimelineId) -> Result<u64> {
     let snapshotspath = format!("timelines/{}/snapshots", timeline);