From 1912546e52479ef47e6e9057a33c416d0f9427fe Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 19 May 2021 08:49:16 +0300 Subject: [PATCH] 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 | 8 ++++-- pageserver/src/branches.rs | 15 +++++----- pageserver/src/lib.rs | 17 +++++++---- pageserver/src/repository.rs | 49 +++++++++++++++++--------------- 4 files changed, 49 insertions(+), 40 deletions(-) diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index 68c48fd3ae..5e849dc9a2 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/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; diff --git a/pageserver/src/branches.rs b/pageserver/src/branches.rs index e4562006c9..19f814be0a 100644 --- a/pageserver/src/branches.rs +++ b/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(()) diff --git a/pageserver/src/lib.rs b/pageserver/src/lib.rs index da5456aeb9..3e59a7ae93 100644 --- a/pageserver/src/lib.rs +++ b/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") } // diff --git a/pageserver/src/repository.rs b/pageserver/src/repository.rs index 5af2899893..ba173358ef 100644 --- a/pageserver/src/repository.rs +++ b/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> { + 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: " #[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)?;