Change the meaning of PageServerConf.workdir

Commit 746f667311 added the 'workdir' field and the get_*_path()
functions, with the idea that we cd into the directory at page server
startup, so that the get_*_path() functions can always return paths
relative to '.', but 'workdir' shows the original path to it. Change it
so that 'conf.workdir' is always set to '.', too, and the get_*_path()
functions include 'workdir' in the returned paths. Why? Because that
allows writing unit tests without changing the current directory.

When I was working on commit 97992226d3, I initially wrote the test so
that it changed the current working directory, just like commit 746f667311
did. But that was problematic, when I tried to add another unit test that
*also* wants to change the current working dir, because they could then
not run concurrently. In fact, they could not even run serially, unless
the current directory was carefully reset after the test. So it is better
to avoid changing the current directory in tests.

pageserver/src/bin/pageserver.rs

@@ -102,18 +102,20 @@ fn main() -> Result<()> {
         gc_horizon: DEFAULT_GC_HORIZON,
         gc_period: Duration::from_secs(DEFAULT_GC_PERIOD_SEC),
         listen_addr: "127.0.0.1:64000".parse().unwrap(),
-        workdir,
+        // we will change the current working directory to the repository below,
+        // so always set 'workdir' to '.'
+        workdir: PathBuf::from("."),
         pg_distrib_dir,
     };
 
     // Create repo and exit if init was requested
     if arg_matches.is_present("init") {
-        branches::init_repo(&conf)?;
+        branches::init_repo(&conf, &workdir)?;
         return Ok(());
     }
 
     // Set CWD to workdir for non-daemon modes
-    env::set_current_dir(&conf.workdir)?;
+    env::set_current_dir(&workdir)?;
 
     if arg_matches.is_present("daemonize") {
         conf.daemonize = true;

pageserver/src/branches.rs

@@ -45,12 +45,12 @@ pub struct PointInTime {
     pub lsn: Lsn,
 }
 
-pub fn init_repo(conf: &PageServerConf) -> Result<()> {
+pub fn init_repo(conf: &PageServerConf, repo_dir: &Path) -> Result<()> {
     // top-level dir may exist if we are creating it through CLI
-    fs::create_dir_all(&conf.workdir)
-        .with_context(|| format!("could not create directory {}", &conf.workdir.display()))?;
+    fs::create_dir_all(repo_dir)
+        .with_context(|| format!("could not create directory {}", repo_dir.display()))?;
 
-    env::set_current_dir(&conf.workdir)?;
+    env::set_current_dir(repo_dir)?;
 
     fs::create_dir(std::path::Path::new("timelines"))?;
     fs::create_dir(std::path::Path::new("refs"))?;
@@ -58,12 +58,12 @@ pub fn init_repo(conf: &PageServerConf) -> Result<()> {
     fs::create_dir(std::path::Path::new("refs").join("tags"))?;
     fs::create_dir(std::path::Path::new("wal-redo"))?;
 
-    println!("created directory structure in {}", &conf.workdir.display());
+    println!("created directory structure in {}", repo_dir.display());
 
     // Create initial timeline
     let tli = create_timeline(conf, None)?;
     let timelinedir = conf.timeline_path(tli);
-    println!("created initial timeline {}", timelinedir.display());
+    println!("created initial timeline {}", tli);
 
     // Run initdb
     //
@@ -110,7 +110,6 @@ pub fn init_repo(conf: &PageServerConf) -> Result<()> {
 
     let target = timelinedir.join("snapshots").join(&lsnstr);
     fs::rename(tmppath, &target)?;
-    println!("moved 'tmp' to {}", target.display());
 
     // Create 'main' branch to refer to the initial timeline
     let data = tli.to_string();
@@ -127,7 +126,7 @@ pub fn init_repo(conf: &PageServerConf) -> Result<()> {
 
     println!(
         "new zenith repository was created in {}",
-        conf.workdir.display()
+        repo_dir.display()
     );
 
     Ok(())

pageserver/src/lib.rs

@@ -26,6 +26,13 @@ pub struct PageServerConf {
     pub listen_addr: SocketAddr,
     pub gc_horizon: u64,
     pub gc_period: Duration,
+
+    // Repository directory, relative to current working directory.
+    // Normally, the page server changes the current working directory
+    // to the repository, and 'workdir' is always '.'. But we don't do
+    // that during unit testing, because the current directory is global
+    // to the process but different unit tests work on different
+    // repositories.
     pub workdir: PathBuf,
 
     pub pg_distrib_dir: PathBuf,
@@ -37,21 +44,19 @@ impl PageServerConf {
     //
 
     fn tag_path(&self, name: &str) -> PathBuf {
-        std::path::Path::new("refs").join("tags").join(name)
+        self.workdir.join("refs").join("tags").join(name)
     }
 
     fn branch_path(&self, name: &str) -> PathBuf {
-        std::path::Path::new("refs").join("branches").join(name)
+        self.workdir.join("refs").join("branches").join(name)
     }
 
     fn timeline_path(&self, timelineid: ZTimelineId) -> PathBuf {
-        std::path::Path::new("timelines").join(timelineid.to_string())
+        self.workdir.join("timelines").join(timelineid.to_string())
     }
 
     fn snapshots_path(&self, timelineid: ZTimelineId) -> PathBuf {
-        std::path::Path::new("timelines")
-            .join(timelineid.to_string())
-            .join("snapshots")
+        self.timeline_path(timelineid).join("snapshots")
     }
 
     //

pageserver/src/repository.rs

@@ -287,24 +287,12 @@ mod tests {
     use super::*;
     use crate::walredo::{WalRedoError, WalRedoManager};
     use crate::PageServerConf;
-    use std::env;
+    use postgres_ffi::pg_constants;
     use std::fs;
-    use std::path::Path;
+    use std::path::PathBuf;
     use std::str::FromStr;
     use std::time::Duration;
 
-    fn get_test_conf() -> PageServerConf {
-        PageServerConf {
-            daemonize: false,
-            interactive: false,
-            gc_horizon: 64 * 1024 * 1024,
-            gc_period: Duration::from_secs(10),
-            listen_addr: "127.0.0.1:5430".parse().unwrap(),
-            workdir: "".into(),
-            pg_distrib_dir: "".into(),
-        }
-    }
-
     /// Arbitrary relation tag, for testing.
     const TESTREL_A: RelTag = RelTag {
         spcnode: 0,
@@ -333,6 +321,28 @@ mod tests {
         buf.freeze()
     }
 
+    fn get_test_repo(test_name: &str) -> Result<Box<dyn Repository>> {
+        let repo_dir = PathBuf::from(format!("../tmp_check/test_{}", test_name));
+        let _ = fs::remove_dir_all(&repo_dir);
+        fs::create_dir_all(&repo_dir)?;
+
+        let conf = PageServerConf {
+            daemonize: false,
+            interactive: false,
+            gc_horizon: 64 * 1024 * 1024,
+            gc_period: Duration::from_secs(10),
+            listen_addr: "127.0.0.1:5430".parse().unwrap(),
+            workdir: repo_dir.into(),
+            pg_distrib_dir: "".into(),
+        };
+
+        let walredo_mgr = TestRedoManager {};
+
+        let repo = rocksdb::RocksRepository::new(&conf, Arc::new(walredo_mgr));
+
+        Ok(Box::new(repo))
+    }
+
     /// Test get_relsize() and truncation.
     ///
     /// FIXME: The RocksRepository implementation returns wrong relation size, if
@@ -343,19 +353,12 @@ mod tests {
     /// "// CORRECT: <correct answer>"
     #[test]
     fn test_relsize() -> Result<()> {
-        let walredo_mgr = TestRedoManager {};
-
-        let repo_dir = Path::new("../tmp_check/test_relsize_repo");
-        let _ = fs::remove_dir_all(repo_dir);
-        fs::create_dir_all(repo_dir)?;
-        env::set_current_dir(repo_dir)?;
-
-        let repo = rocksdb::RocksRepository::new(&get_test_conf(), Arc::new(walredo_mgr));
 
         // get_timeline() with non-existent timeline id should fail
         //repo.get_timeline("11223344556677881122334455667788");
 
-        // Create it
+        // Create timeline to work on
+        let repo = get_test_repo("test_relsize")?;
         let timelineid = ZTimelineId::from_str("11223344556677881122334455667788").unwrap();
         let tline = repo.create_empty_timeline(timelineid)?;