diff --git a/Cargo.lock b/Cargo.lock index 36e7069eb1..be3f179d5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -798,6 +798,22 @@ dependencies = [ "either", ] +[[package]] +name = "camino" +version = "1.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c59e92b5a388f549b863a7bea62612c09f24c8393560709a54558a9abdfb3b9c" + +[[package]] +name = "camino-tempfile" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d2ab15a83d13f75dbd86f082bdefd160b628476ef58d3b900a0ef74e001bb097" +dependencies = [ + "camino", + "tempfile", +] + [[package]] name = "cast" version = "0.3.0" @@ -1053,6 +1069,7 @@ name = "control_plane" version = "0.1.0" dependencies = [ "anyhow", + "camino", "clap", "comfy-table", "compute_api", @@ -2641,6 +2658,7 @@ version = "0.1.0" dependencies = [ "anyhow", "bytes", + "camino", "clap", "git-version", "pageserver", @@ -2661,6 +2679,8 @@ dependencies = [ "async-trait", "byteorder", "bytes", + "camino", + "camino-tempfile", "chrono", "clap", "close_fds", @@ -2712,7 +2732,6 @@ dependencies = [ "strum_macros", "svg_fmt", "sync_wrapper", - "tempfile", "tenant_size_model", "thiserror", "tokio", @@ -3405,6 +3424,8 @@ dependencies = [ "aws-sdk-s3", "aws-smithy-http", "aws-types", + "camino", + "camino-tempfile", "hyper", "metrics", "once_cell", @@ -3413,7 +3434,6 @@ dependencies = [ "scopeguard", "serde", "serde_json", - "tempfile", "test-context", "tokio", "tokio-util", @@ -3765,6 +3785,8 @@ dependencies = [ "async-trait", "byteorder", "bytes", + "camino", + "camino-tempfile", "chrono", "clap", "const_format", @@ -3793,7 +3815,6 @@ dependencies = [ "serde_with", "signal-hook", "storage_broker", - "tempfile", "thiserror", "tokio", "tokio-io-timeout", @@ -5092,6 +5113,8 @@ dependencies = [ "bincode", "byteorder", "bytes", + "camino", + "camino-tempfile", "chrono", "const_format", "criterion", @@ -5117,7 +5140,6 @@ dependencies = [ "signal-hook", "strum", "strum_macros", - "tempfile", "thiserror", "tokio", "tokio-stream", @@ -5191,6 +5213,7 @@ name = "wal_craft" version = "0.1.0" dependencies = [ "anyhow", + "camino-tempfile", "clap", "env_logger", "log", @@ -5198,7 +5221,6 @@ dependencies = [ "postgres", "postgres_ffi", "regex", - "tempfile", "utils", "workspace_hack", ] diff --git a/Cargo.toml b/Cargo.toml index b0bcf69039..2b9da977e5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ bindgen = "0.65" bstr = "1.0" byteorder = "1.4" bytes = "1.0" +camino = "1.1.6" cfg-if = "1.0.0" chrono = { version = "0.4", default-features = false, features = ["clock"] } clap = { version = "4.0", features = ["derive"] } @@ -187,7 +188,7 @@ workspace_hack = { version = "0.1", path = "./workspace_hack/" } criterion = "0.5.1" rcgen = "0.11" rstest = "0.18" -tempfile = "3.4" +camino-tempfile = "1.0.2" tonic-build = "0.9" [patch.crates-io] diff --git a/compute_tools/src/compute.rs b/compute_tools/src/compute.rs index 5c08ebe06a..62e541ebce 100644 --- a/compute_tools/src/compute.rs +++ b/compute_tools/src/compute.rs @@ -1039,7 +1039,7 @@ LIMIT 100", let remote_extensions = spec .remote_extensions .as_ref() - .ok_or(anyhow::anyhow!("Remote extensions are not configured",))?; + .ok_or(anyhow::anyhow!("Remote extensions are not configured"))?; info!("parse shared_preload_libraries from spec.cluster.settings"); let mut libs_vec = Vec::new(); diff --git a/control_plane/Cargo.toml b/control_plane/Cargo.toml index ec685915f9..7ccddc161e 100644 --- a/control_plane/Cargo.toml +++ b/control_plane/Cargo.toml @@ -6,6 +6,7 @@ license.workspace = true [dependencies] anyhow.workspace = true +camino.workspace = true clap.workspace = true comfy-table.workspace = true git-version.workspace = true diff --git a/control_plane/src/attachment_service.rs b/control_plane/src/attachment_service.rs index f0e649cfa8..d7828cdba7 100644 --- a/control_plane/src/attachment_service.rs +++ b/control_plane/src/attachment_service.rs @@ -1,5 +1,6 @@ use crate::{background_process, local_env::LocalEnv}; use anyhow::anyhow; +use camino::Utf8PathBuf; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use std::{path::PathBuf, process::Child}; @@ -47,8 +48,9 @@ impl AttachmentService { } } - fn pid_file(&self) -> PathBuf { - self.env.base_data_dir.join("attachment_service.pid") + fn pid_file(&self) -> Utf8PathBuf { + Utf8PathBuf::from_path_buf(self.env.base_data_dir.join("attachment_service.pid")) + .expect("non-Unicode path") } pub fn start(&self) -> anyhow::Result { diff --git a/control_plane/src/background_process.rs b/control_plane/src/background_process.rs index 64664d65ff..186d49fe8b 100644 --- a/control_plane/src/background_process.rs +++ b/control_plane/src/background_process.rs @@ -16,12 +16,13 @@ use std::ffi::OsStr; use std::io::Write; use std::os::unix::prelude::AsRawFd; use std::os::unix::process::CommandExt; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::{Child, Command}; use std::time::Duration; use std::{fs, io, thread}; use anyhow::Context; +use camino::{Utf8Path, Utf8PathBuf}; use nix::errno::Errno; use nix::fcntl::{FcntlArg, FdFlag}; use nix::sys::signal::{kill, Signal}; @@ -45,9 +46,9 @@ const NOTICE_AFTER_RETRIES: u64 = 50; /// it itself. pub enum InitialPidFile<'t> { /// Create a pidfile, to allow future CLI invocations to manipulate the process. - Create(&'t Path), + Create(&'t Utf8Path), /// The process will create the pidfile itself, need to wait for that event. - Expect(&'t Path), + Expect(&'t Utf8Path), } /// Start a background child process using the parameters given. @@ -137,7 +138,11 @@ where } /// Stops the process, using the pid file given. Returns Ok also if the process is already not running. -pub fn stop_process(immediate: bool, process_name: &str, pid_file: &Path) -> anyhow::Result<()> { +pub fn stop_process( + immediate: bool, + process_name: &str, + pid_file: &Utf8Path, +) -> anyhow::Result<()> { let pid = match pid_file::read(pid_file) .with_context(|| format!("read pid_file {pid_file:?}"))? { @@ -252,9 +257,9 @@ fn fill_aws_secrets_vars(mut cmd: &mut Command) -> &mut Command { /// will remain held until the cmd exits. fn pre_exec_create_pidfile

(cmd: &mut Command, path: P) -> &mut Command where - P: Into, + P: Into, { - let path: PathBuf = path.into(); + let path: Utf8PathBuf = path.into(); // SAFETY // pre_exec is marked unsafe because it runs between fork and exec. // Why is that dangerous in various ways? @@ -311,7 +316,7 @@ where fn process_started( pid: Pid, - pid_file_to_check: Option<&Path>, + pid_file_to_check: Option<&Utf8Path>, status_check: &F, ) -> anyhow::Result where diff --git a/control_plane/src/broker.rs b/control_plane/src/broker.rs index 8d40c7afc1..6be865cc2e 100644 --- a/control_plane/src/broker.rs +++ b/control_plane/src/broker.rs @@ -7,7 +7,7 @@ //! ``` use anyhow::Context; -use std::path::PathBuf; +use camino::Utf8PathBuf; use crate::{background_process, local_env}; @@ -30,7 +30,7 @@ pub fn start_broker_process(env: &local_env::LocalEnv) -> anyhow::Result<()> { || { let url = broker.client_url(); let status_url = url.join("status").with_context(|| { - format!("Failed to append /status path to broker endpoint {url}",) + format!("Failed to append /status path to broker endpoint {url}") })?; let request = client .get(status_url) @@ -50,6 +50,7 @@ pub fn stop_broker_process(env: &local_env::LocalEnv) -> anyhow::Result<()> { background_process::stop_process(true, "storage_broker", &storage_broker_pid_file_path(env)) } -fn storage_broker_pid_file_path(env: &local_env::LocalEnv) -> PathBuf { - env.base_data_dir.join("storage_broker.pid") +fn storage_broker_pid_file_path(env: &local_env::LocalEnv) -> Utf8PathBuf { + Utf8PathBuf::from_path_buf(env.base_data_dir.join("storage_broker.pid")) + .expect("non-Unicode path") } diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index a6b675fdb5..0746dde4ef 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -14,6 +14,7 @@ use std::process::{Child, Command}; use std::{io, result}; use anyhow::{bail, Context}; +use camino::Utf8PathBuf; use pageserver_api::models::{self, TenantInfo, TimelineInfo}; use postgres_backend::AuthType; use postgres_connection::{parse_host_port, PgConnectionConfig}; @@ -144,7 +145,7 @@ impl PageServerNode { pub fn initialize(&self, config_overrides: &[&str]) -> anyhow::Result<()> { // First, run `pageserver --init` and wait for it to write a config into FS and exit. self.pageserver_init(config_overrides) - .with_context(|| format!("Failed to run init for pageserver node {}", self.conf.id,)) + .with_context(|| format!("Failed to run init for pageserver node {}", self.conf.id)) } pub fn repo_path(&self) -> PathBuf { @@ -154,8 +155,9 @@ impl PageServerNode { /// The pid file is created by the pageserver process, with its pid stored inside. /// Other pageservers cannot lock the same file and overwrite it for as long as the current /// pageserver runs. (Unless someone removes the file manually; never do that!) - fn pid_file(&self) -> PathBuf { - self.repo_path().join("pageserver.pid") + fn pid_file(&self) -> Utf8PathBuf { + Utf8PathBuf::from_path_buf(self.repo_path().join("pageserver.pid")) + .expect("non-Unicode path") } pub fn start(&self, config_overrides: &[&str]) -> anyhow::Result { diff --git a/control_plane/src/safekeeper.rs b/control_plane/src/safekeeper.rs index eb8fe1af17..a8baa0ac53 100644 --- a/control_plane/src/safekeeper.rs +++ b/control_plane/src/safekeeper.rs @@ -11,6 +11,7 @@ use std::process::Child; use std::{io, result}; use anyhow::Context; +use camino::Utf8PathBuf; use postgres_connection::PgConnectionConfig; use reqwest::blocking::{Client, RequestBuilder, Response}; use reqwest::{IntoUrl, Method}; @@ -97,8 +98,9 @@ impl SafekeeperNode { SafekeeperNode::datadir_path_by_id(&self.env, self.id) } - pub fn pid_file(&self) -> PathBuf { - self.datadir_path().join("safekeeper.pid") + pub fn pid_file(&self) -> Utf8PathBuf { + Utf8PathBuf::from_path_buf(self.datadir_path().join("safekeeper.pid")) + .expect("non-Unicode path") } pub fn start(&self, extra_opts: Vec) -> anyhow::Result { diff --git a/libs/postgres_ffi/wal_craft/Cargo.toml b/libs/postgres_ffi/wal_craft/Cargo.toml index bea888b23e..0edc642402 100644 --- a/libs/postgres_ffi/wal_craft/Cargo.toml +++ b/libs/postgres_ffi/wal_craft/Cargo.toml @@ -12,7 +12,7 @@ log.workspace = true once_cell.workspace = true postgres.workspace = true postgres_ffi.workspace = true -tempfile.workspace = true +camino-tempfile.workspace = true workspace_hack.workspace = true diff --git a/libs/postgres_ffi/wal_craft/src/lib.rs b/libs/postgres_ffi/wal_craft/src/lib.rs index fb627ca258..75ffd3f055 100644 --- a/libs/postgres_ffi/wal_craft/src/lib.rs +++ b/libs/postgres_ffi/wal_craft/src/lib.rs @@ -1,4 +1,5 @@ use anyhow::{bail, ensure}; +use camino_tempfile::{tempdir, Utf8TempDir}; use log::*; use postgres::types::PgLsn; use postgres::Client; @@ -8,7 +9,6 @@ use std::cmp::Ordering; use std::path::{Path, PathBuf}; use std::process::Command; use std::time::{Duration, Instant}; -use tempfile::{tempdir, TempDir}; macro_rules! xlog_utils_test { ($version:ident) => { @@ -33,7 +33,7 @@ pub struct Conf { pub struct PostgresServer { process: std::process::Child, - _unix_socket_dir: TempDir, + _unix_socket_dir: Utf8TempDir, client_config: postgres::Config, } diff --git a/libs/remote_storage/Cargo.toml b/libs/remote_storage/Cargo.toml index 2b808779f4..d938648750 100644 --- a/libs/remote_storage/Cargo.toml +++ b/libs/remote_storage/Cargo.toml @@ -13,6 +13,7 @@ aws-types.workspace = true aws-config.workspace = true aws-sdk-s3.workspace = true aws-credential-types.workspace = true +camino.workspace = true hyper = { workspace = true, features = ["stream"] } serde.workspace = true serde_json.workspace = true @@ -27,6 +28,6 @@ pin-project-lite.workspace = true workspace_hack.workspace = true [dev-dependencies] -tempfile.workspace = true +camino-tempfile.workspace = true test-context.workspace = true rand.workspace = true diff --git a/libs/remote_storage/src/lib.rs b/libs/remote_storage/src/lib.rs index a92b87632b..3560c94c71 100644 --- a/libs/remote_storage/src/lib.rs +++ b/libs/remote_storage/src/lib.rs @@ -13,12 +13,12 @@ use std::{ collections::HashMap, fmt::Debug, num::{NonZeroU32, NonZeroUsize}, - path::{Path, PathBuf}, pin::Pin, sync::Arc, }; use anyhow::{bail, Context}; +use camino::{Utf8Path, Utf8PathBuf}; use serde::{Deserialize, Serialize}; use tokio::io; @@ -52,7 +52,7 @@ const REMOTE_STORAGE_PREFIX_SEPARATOR: char = '/'; /// The prefix is an implementation detail, that allows representing local paths /// as the remote ones, stripping the local storage prefix away. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct RemotePath(PathBuf); +pub struct RemotePath(Utf8PathBuf); impl Serialize for RemotePath { fn serialize(&self, serializer: S) -> Result @@ -69,18 +69,18 @@ impl<'de> Deserialize<'de> for RemotePath { D: serde::Deserializer<'de>, { let str = String::deserialize(deserializer)?; - Ok(Self(PathBuf::from(&str))) + Ok(Self(Utf8PathBuf::from(&str))) } } impl std::fmt::Display for RemotePath { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0.display()) + std::fmt::Display::fmt(&self.0, f) } } impl RemotePath { - pub fn new(relative_path: &Path) -> anyhow::Result { + pub fn new(relative_path: &Utf8Path) -> anyhow::Result { anyhow::ensure!( relative_path.is_relative(), "Path {relative_path:?} is not relative" @@ -89,30 +89,30 @@ impl RemotePath { } pub fn from_string(relative_path: &str) -> anyhow::Result { - Self::new(Path::new(relative_path)) + Self::new(Utf8Path::new(relative_path)) } - pub fn with_base(&self, base_path: &Path) -> PathBuf { + pub fn with_base(&self, base_path: &Utf8Path) -> Utf8PathBuf { base_path.join(&self.0) } pub fn object_name(&self) -> Option<&str> { - self.0.file_name().and_then(|os_str| os_str.to_str()) + self.0.file_name() } - pub fn join(&self, segment: &Path) -> Self { + pub fn join(&self, segment: &Utf8Path) -> Self { Self(self.0.join(segment)) } - pub fn get_path(&self) -> &PathBuf { + pub fn get_path(&self) -> &Utf8PathBuf { &self.0 } pub fn extension(&self) -> Option<&str> { - self.0.extension()?.to_str() + self.0.extension() } - pub fn strip_prefix(&self, p: &RemotePath) -> Result<&Path, std::path::StripPrefixError> { + pub fn strip_prefix(&self, p: &RemotePath) -> Result<&Utf8Path, std::path::StripPrefixError> { self.0.strip_prefix(&p.0) } } @@ -311,7 +311,7 @@ impl GenericRemoteStorage { pub fn from_config(storage_config: &RemoteStorageConfig) -> anyhow::Result { Ok(match &storage_config.storage { RemoteStorageKind::LocalFs(root) => { - info!("Using fs root '{}' as a remote storage", root.display()); + info!("Using fs root '{root}' as a remote storage"); Self::LocalFs(LocalFs::new(root.clone())?) } RemoteStorageKind::AwsS3(s3_config) => { @@ -379,7 +379,7 @@ pub struct RemoteStorageConfig { pub enum RemoteStorageKind { /// Storage based on local file system. /// Specify a root folder to place all stored files into. - LocalFs(PathBuf), + LocalFs(Utf8PathBuf), /// AWS S3 based storage, storing all files in the S3 bucket /// specified by the config AwsS3(S3Config), @@ -474,7 +474,7 @@ impl RemoteStorageConfig { concurrency_limit, max_keys_per_list_response, }), - (Some(local_path), None, None) => RemoteStorageKind::LocalFs(PathBuf::from( + (Some(local_path), None, None) => RemoteStorageKind::LocalFs(Utf8PathBuf::from( parse_toml_string("local_path", local_path)?, )), (Some(_), Some(_), _) => bail!("local_path and bucket_name are mutually exclusive"), @@ -519,23 +519,23 @@ mod tests { #[test] fn test_object_name() { - let k = RemotePath::new(Path::new("a/b/c")).unwrap(); + let k = RemotePath::new(Utf8Path::new("a/b/c")).unwrap(); assert_eq!(k.object_name(), Some("c")); - let k = RemotePath::new(Path::new("a/b/c/")).unwrap(); + let k = RemotePath::new(Utf8Path::new("a/b/c/")).unwrap(); assert_eq!(k.object_name(), Some("c")); - let k = RemotePath::new(Path::new("a/")).unwrap(); + let k = RemotePath::new(Utf8Path::new("a/")).unwrap(); assert_eq!(k.object_name(), Some("a")); // XXX is it impossible to have an empty key? - let k = RemotePath::new(Path::new("")).unwrap(); + let k = RemotePath::new(Utf8Path::new("")).unwrap(); assert_eq!(k.object_name(), None); } #[test] fn rempte_path_cannot_be_created_from_absolute_ones() { - let err = RemotePath::new(Path::new("/")).expect_err("Should fail on absolute paths"); + let err = RemotePath::new(Utf8Path::new("/")).expect_err("Should fail on absolute paths"); assert_eq!(err.to_string(), "Path \"/\" is not relative"); } } diff --git a/libs/remote_storage/src/local_fs.rs b/libs/remote_storage/src/local_fs.rs index 5040183045..3d32b6b631 100644 --- a/libs/remote_storage/src/local_fs.rs +++ b/libs/remote_storage/src/local_fs.rs @@ -4,15 +4,10 @@ //! This storage used in tests, but can also be used in cases when a certain persistent //! volume is mounted to the local FS. -use std::{ - borrow::Cow, - future::Future, - io::ErrorKind, - path::{Path, PathBuf}, - pin::Pin, -}; +use std::{borrow::Cow, future::Future, io::ErrorKind, pin::Pin}; use anyhow::{bail, ensure, Context}; +use camino::{Utf8Path, Utf8PathBuf}; use tokio::{ fs, io::{self, AsyncReadExt, AsyncSeekExt, AsyncWriteExt}, @@ -28,20 +23,20 @@ const LOCAL_FS_TEMP_FILE_SUFFIX: &str = "___temp"; #[derive(Debug, Clone)] pub struct LocalFs { - storage_root: PathBuf, + storage_root: Utf8PathBuf, } impl LocalFs { /// Attempts to create local FS storage, along with its root directory. /// Storage root will be created (if does not exist) and transformed into an absolute path (if passed as relative). - pub fn new(mut storage_root: PathBuf) -> anyhow::Result { + pub fn new(mut storage_root: Utf8PathBuf) -> anyhow::Result { if !storage_root.exists() { std::fs::create_dir_all(&storage_root).with_context(|| { format!("Failed to create all directories in the given root path {storage_root:?}") })?; } if !storage_root.is_absolute() { - storage_root = storage_root.canonicalize().with_context(|| { + storage_root = storage_root.canonicalize_utf8().with_context(|| { format!("Failed to represent path {storage_root:?} as an absolute path") })?; } @@ -50,7 +45,7 @@ impl LocalFs { } // mirrors S3Bucket::s3_object_to_relative_path - fn local_file_to_relative_path(&self, key: PathBuf) -> RemotePath { + fn local_file_to_relative_path(&self, key: Utf8PathBuf) -> RemotePath { let relative_path = key .strip_prefix(&self.storage_root) .expect("relative path must contain storage_root as prefix"); @@ -59,22 +54,18 @@ impl LocalFs { async fn read_storage_metadata( &self, - file_path: &Path, + file_path: &Utf8Path, ) -> anyhow::Result> { let metadata_path = storage_metadata_path(file_path); if metadata_path.exists() && metadata_path.is_file() { let metadata_string = fs::read_to_string(&metadata_path).await.with_context(|| { - format!( - "Failed to read metadata from the local storage at '{}'", - metadata_path.display() - ) + format!("Failed to read metadata from the local storage at '{metadata_path}'") })?; serde_json::from_str(&metadata_string) .with_context(|| { format!( - "Failed to deserialize metadata from the local storage at '{}'", - metadata_path.display() + "Failed to deserialize metadata from the local storage at '{metadata_path}'", ) }) .map(|metadata| Some(StorageMetadata(metadata))) @@ -171,25 +162,21 @@ impl RemoteStorage for LocalFs { } } - // Note that PathBuf starts_with only considers full path segments, but + // Note that Utf8PathBuf starts_with only considers full path segments, but // object prefixes are arbitrary strings, so we need the strings for doing // starts_with later. - let prefix = full_path.to_string_lossy(); + let prefix = full_path.as_str(); let mut files = vec![]; - let mut directory_queue = vec![initial_dir.clone()]; + let mut directory_queue = vec![initial_dir]; while let Some(cur_folder) = directory_queue.pop() { - let mut entries = fs::read_dir(cur_folder.clone()).await?; - while let Some(entry) = entries.next_entry().await? { - let file_name: PathBuf = entry.file_name().into(); - let full_file_name = cur_folder.clone().join(&file_name); - if full_file_name - .to_str() - .map(|s| s.starts_with(prefix.as_ref())) - .unwrap_or(false) - { + let mut entries = cur_folder.read_dir_utf8()?; + while let Some(Ok(entry)) = entries.next() { + let file_name = entry.file_name(); + let full_file_name = cur_folder.join(file_name); + if full_file_name.as_str().starts_with(prefix) { let file_remote_path = self.local_file_to_relative_path(full_file_name.clone()); - files.push(file_remote_path.clone()); + files.push(file_remote_path); if full_file_name.is_dir() { directory_queue.push(full_file_name); } @@ -230,10 +217,7 @@ impl RemoteStorage for LocalFs { .open(&temp_file_path) .await .with_context(|| { - format!( - "Failed to open target fs destination at '{}'", - target_file_path.display() - ) + format!("Failed to open target fs destination at '{target_file_path}'") })?, ); @@ -244,8 +228,7 @@ impl RemoteStorage for LocalFs { .await .with_context(|| { format!( - "Failed to upload file (write temp) to the local storage at '{}'", - temp_file_path.display() + "Failed to upload file (write temp) to the local storage at '{temp_file_path}'", ) })?; @@ -262,8 +245,7 @@ impl RemoteStorage for LocalFs { destination.flush().await.with_context(|| { format!( - "Failed to upload (flush temp) file to the local storage at '{}'", - temp_file_path.display() + "Failed to upload (flush temp) file to the local storage at '{temp_file_path}'", ) })?; @@ -271,8 +253,7 @@ impl RemoteStorage for LocalFs { .await .with_context(|| { format!( - "Failed to upload (rename) file to the local storage at '{}'", - target_file_path.display() + "Failed to upload (rename) file to the local storage at '{target_file_path}'", ) })?; @@ -286,8 +267,7 @@ impl RemoteStorage for LocalFs { .await .with_context(|| { format!( - "Failed to write metadata to the local storage at '{}'", - storage_metadata_path.display() + "Failed to write metadata to the local storage at '{storage_metadata_path}'", ) })?; } @@ -393,16 +373,16 @@ impl RemoteStorage for LocalFs { } } -fn storage_metadata_path(original_path: &Path) -> PathBuf { +fn storage_metadata_path(original_path: &Utf8Path) -> Utf8PathBuf { path_with_suffix_extension(original_path, "metadata") } fn get_all_files<'a, P>( directory_path: P, recursive: bool, -) -> Pin>> + Send + Sync + 'a>> +) -> Pin>> + Send + Sync + 'a>> where - P: AsRef + Send + Sync + 'a, + P: AsRef + Send + Sync + 'a, { Box::pin(async move { let directory_path = directory_path.as_ref(); @@ -412,7 +392,13 @@ where let mut dir_contents = fs::read_dir(directory_path).await?; while let Some(dir_entry) = dir_contents.next_entry().await? { let file_type = dir_entry.file_type().await?; - let entry_path = dir_entry.path(); + let entry_path = + Utf8PathBuf::from_path_buf(dir_entry.path()).map_err(|pb| { + anyhow::Error::msg(format!( + "non-Unicode path: {}", + pb.to_string_lossy() + )) + })?; if file_type.is_symlink() { debug!("{entry_path:?} is a symlink, skipping") } else if file_type.is_dir() { @@ -435,13 +421,10 @@ where }) } -async fn create_target_directory(target_file_path: &Path) -> anyhow::Result<()> { +async fn create_target_directory(target_file_path: &Utf8Path) -> anyhow::Result<()> { let target_dir = match target_file_path.parent() { Some(parent_dir) => parent_dir, - None => bail!( - "File path '{}' has no parent directory", - target_file_path.display() - ), + None => bail!("File path '{target_file_path}' has no parent directory"), }; if !target_dir.exists() { fs::create_dir_all(target_dir).await?; @@ -449,13 +432,9 @@ async fn create_target_directory(target_file_path: &Path) -> anyhow::Result<()> Ok(()) } -fn file_exists(file_path: &Path) -> anyhow::Result { +fn file_exists(file_path: &Utf8Path) -> anyhow::Result { if file_path.exists() { - ensure!( - file_path.is_file(), - "file path '{}' is not a file", - file_path.display() - ); + ensure!(file_path.is_file(), "file path '{file_path}' is not a file"); Ok(true) } else { Ok(false) @@ -466,13 +445,13 @@ fn file_exists(file_path: &Path) -> anyhow::Result { mod fs_tests { use super::*; + use camino_tempfile::tempdir; use std::{collections::HashMap, io::Write}; - use tempfile::tempdir; async fn read_and_assert_remote_file_contents( storage: &LocalFs, #[allow(clippy::ptr_arg)] - // have to use &PathBuf due to `storage.local_path` parameter requirements + // have to use &Utf8PathBuf due to `storage.local_path` parameter requirements remote_storage_path: &RemotePath, expected_metadata: Option<&StorageMetadata>, ) -> anyhow::Result { @@ -519,7 +498,7 @@ mod fs_tests { async fn upload_file_negatives() -> anyhow::Result<()> { let storage = create_storage()?; - let id = RemotePath::new(Path::new("dummy"))?; + let id = RemotePath::new(Utf8Path::new("dummy"))?; let content = std::io::Cursor::new(b"12345"); // Check that you get an error if the size parameter doesn't match the actual @@ -544,7 +523,8 @@ mod fs_tests { } fn create_storage() -> anyhow::Result { - LocalFs::new(tempdir()?.path().to_owned()) + let storage_root = tempdir()?.path().to_path_buf(); + LocalFs::new(storage_root) } #[tokio::test] @@ -561,7 +541,7 @@ mod fs_tests { ); let non_existing_path = "somewhere/else"; - match storage.download(&RemotePath::new(Path::new(non_existing_path))?).await { + match storage.download(&RemotePath::new(Utf8Path::new(non_existing_path))?).await { Err(DownloadError::NotFound) => {} // Should get NotFound for non existing keys other => panic!("Should get a NotFound error when downloading non-existing storage files, but got: {other:?}"), } @@ -775,7 +755,7 @@ mod fs_tests { } async fn create_file_for_upload( - path: &Path, + path: &Utf8Path, contents: &str, ) -> anyhow::Result<(io::BufReader, usize)> { std::fs::create_dir_all(path.parent().unwrap())?; diff --git a/libs/remote_storage/src/s3_bucket.rs b/libs/remote_storage/src/s3_bucket.rs index fc6d7fa61b..db7eef91e2 100644 --- a/libs/remote_storage/src/s3_bucket.rs +++ b/libs/remote_storage/src/s3_bucket.rs @@ -180,12 +180,11 @@ impl S3Bucket { assert_eq!(std::path::MAIN_SEPARATOR, REMOTE_STORAGE_PREFIX_SEPARATOR); let path_string = path .get_path() - .to_string_lossy() - .trim_end_matches(REMOTE_STORAGE_PREFIX_SEPARATOR) - .to_string(); + .as_str() + .trim_end_matches(REMOTE_STORAGE_PREFIX_SEPARATOR); match &self.prefix_in_bucket { - Some(prefix) => prefix.clone() + "/" + &path_string, - None => path_string, + Some(prefix) => prefix.clone() + "/" + path_string, + None => path_string.to_string(), } } @@ -601,8 +600,8 @@ fn start_measuring_requests( #[cfg(test)] mod tests { + use camino::Utf8Path; use std::num::NonZeroUsize; - use std::path::Path; use crate::{RemotePath, S3Bucket, S3Config}; @@ -611,7 +610,7 @@ mod tests { let all_paths = ["", "some/path", "some/path/"]; let all_paths: Vec = all_paths .iter() - .map(|x| RemotePath::new(Path::new(x)).expect("bad path")) + .map(|x| RemotePath::new(Utf8Path::new(x)).expect("bad path")) .collect(); let prefixes = [ None, diff --git a/libs/remote_storage/tests/test_real_s3.rs b/libs/remote_storage/tests/test_real_s3.rs index b220349829..7e2aa9f6d7 100644 --- a/libs/remote_storage/tests/test_real_s3.rs +++ b/libs/remote_storage/tests/test_real_s3.rs @@ -2,11 +2,12 @@ use std::collections::HashSet; use std::env; use std::num::{NonZeroU32, NonZeroUsize}; use std::ops::ControlFlow; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::sync::Arc; use std::time::UNIX_EPOCH; use anyhow::Context; +use camino::Utf8Path; use once_cell::sync::OnceCell; use remote_storage::{ GenericRemoteStorage, RemotePath, RemoteStorageConfig, RemoteStorageKind, S3Config, @@ -55,7 +56,7 @@ async fn s3_pagination_should_work(ctx: &mut MaybeEnabledS3WithTestBlobs) -> any let test_client = Arc::clone(&ctx.enabled.client); let expected_remote_prefixes = ctx.remote_prefixes.clone(); - let base_prefix = RemotePath::new(Path::new(ctx.enabled.base_prefix)) + let base_prefix = RemotePath::new(Utf8Path::new(ctx.enabled.base_prefix)) .context("common_prefix construction")?; let root_remote_prefixes = test_client .list_prefixes(None) @@ -108,7 +109,7 @@ async fn s3_list_files_works(ctx: &mut MaybeEnabledS3WithSimpleTestBlobs) -> any }; let test_client = Arc::clone(&ctx.enabled.client); let base_prefix = - RemotePath::new(Path::new("folder1")).context("common_prefix construction")?; + RemotePath::new(Utf8Path::new("folder1")).context("common_prefix construction")?; let root_files = test_client .list_files(None) .await @@ -129,9 +130,9 @@ async fn s3_list_files_works(ctx: &mut MaybeEnabledS3WithSimpleTestBlobs) -> any let trim_remote_blobs: HashSet<_> = ctx .remote_blobs .iter() - .map(|x| x.get_path().to_str().expect("must be valid name")) + .map(|x| x.get_path()) .filter(|x| x.starts_with("folder1")) - .map(|x| RemotePath::new(Path::new(x)).expect("must be valid name")) + .map(|x| RemotePath::new(x).expect("must be valid path")) .collect(); assert_eq!( nested_remote_files, trim_remote_blobs, @@ -148,10 +149,9 @@ async fn s3_delete_non_exising_works(ctx: &mut MaybeEnabledS3) -> anyhow::Result MaybeEnabledS3::Disabled => return Ok(()), }; - let path = RemotePath::new(&PathBuf::from(format!( - "{}/for_sure_there_is_nothing_there_really", - ctx.base_prefix, - ))) + let path = RemotePath::new(Utf8Path::new( + format!("{}/for_sure_there_is_nothing_there_really", ctx.base_prefix).as_str(), + )) .with_context(|| "RemotePath conversion")?; ctx.client.delete(&path).await.expect("should succeed"); @@ -167,13 +167,13 @@ async fn s3_delete_objects_works(ctx: &mut MaybeEnabledS3) -> anyhow::Result<()> MaybeEnabledS3::Disabled => return Ok(()), }; - let path1 = RemotePath::new(&PathBuf::from(format!("{}/path1", ctx.base_prefix,))) + let path1 = RemotePath::new(Utf8Path::new(format!("{}/path1", ctx.base_prefix).as_str())) .with_context(|| "RemotePath conversion")?; - let path2 = RemotePath::new(&PathBuf::from(format!("{}/path2", ctx.base_prefix,))) + let path2 = RemotePath::new(Utf8Path::new(format!("{}/path2", ctx.base_prefix).as_str())) .with_context(|| "RemotePath conversion")?; - let path3 = RemotePath::new(&PathBuf::from(format!("{}/path3", ctx.base_prefix,))) + let path3 = RemotePath::new(Utf8Path::new(format!("{}/path3", ctx.base_prefix).as_str())) .with_context(|| "RemotePath conversion")?; let data1 = "remote blob data1".as_bytes(); @@ -427,10 +427,10 @@ async fn upload_s3_data( for i in 1..upload_tasks_count + 1 { let task_client = Arc::clone(client); upload_tasks.spawn(async move { - let prefix = PathBuf::from(format!("{base_prefix_str}/sub_prefix_{i}/")); - let blob_prefix = RemotePath::new(&prefix) + let prefix = format!("{base_prefix_str}/sub_prefix_{i}/"); + let blob_prefix = RemotePath::new(Utf8Path::new(&prefix)) .with_context(|| format!("{prefix:?} to RemotePath conversion"))?; - let blob_path = blob_prefix.join(Path::new(&format!("blob_{i}"))); + let blob_path = blob_prefix.join(Utf8Path::new(&format!("blob_{i}"))); debug!("Creating remote item {i} at path {blob_path:?}"); let data = format!("remote blob data {i}").into_bytes(); @@ -512,8 +512,10 @@ async fn upload_simple_s3_data( let task_client = Arc::clone(client); upload_tasks.spawn(async move { let blob_path = PathBuf::from(format!("folder{}/blob_{}.txt", i / 7, i)); - let blob_path = RemotePath::new(&blob_path) - .with_context(|| format!("{blob_path:?} to RemotePath conversion"))?; + let blob_path = RemotePath::new( + Utf8Path::from_path(blob_path.as_path()).expect("must be valid blob path"), + ) + .with_context(|| format!("{blob_path:?} to RemotePath conversion"))?; debug!("Creating remote item {i} at path {blob_path:?}"); let data = format!("remote blob data {i}").into_bytes(); diff --git a/libs/utils/Cargo.toml b/libs/utils/Cargo.toml index 1eb9e6ab4d..27df0265b4 100644 --- a/libs/utils/Cargo.toml +++ b/libs/utils/Cargo.toml @@ -10,6 +10,7 @@ async-trait.workspace = true anyhow.workspace = true bincode.workspace = true bytes.workspace = true +camino.workspace = true chrono.workspace = true heapless.workspace = true hex = { workspace = true, features = ["serde"] } @@ -53,7 +54,7 @@ byteorder.workspace = true bytes.workspace = true criterion.workspace = true hex-literal.workspace = true -tempfile.workspace = true +camino-tempfile.workspace = true [[bench]] name = "benchmarks" diff --git a/libs/utils/src/auth.rs b/libs/utils/src/auth.rs index 716984b64e..54b90fa070 100644 --- a/libs/utils/src/auth.rs +++ b/libs/utils/src/auth.rs @@ -2,9 +2,9 @@ use serde; use std::fs; -use std::path::Path; use anyhow::Result; +use camino::Utf8Path; use jsonwebtoken::{ decode, encode, Algorithm, DecodingKey, EncodingKey, Header, TokenData, Validation, }; @@ -65,7 +65,7 @@ impl JwtAuth { } } - pub fn from_key_path(key_path: &Path) -> Result { + pub fn from_key_path(key_path: &Utf8Path) -> Result { let public_key = fs::read(key_path)?; Ok(Self::new(DecodingKey::from_ed_pem(&public_key)?)) } diff --git a/libs/utils/src/crashsafe.rs b/libs/utils/src/crashsafe.rs index fd20d2d2ed..b089af4a02 100644 --- a/libs/utils/src/crashsafe.rs +++ b/libs/utils/src/crashsafe.rs @@ -1,14 +1,14 @@ use std::{ borrow::Cow, - ffi::OsStr, fs::{self, File}, io, - path::{Path, PathBuf}, }; +use camino::{Utf8Path, Utf8PathBuf}; + /// Similar to [`std::fs::create_dir`], except we fsync the /// created directory and its parent. -pub fn create_dir(path: impl AsRef) -> io::Result<()> { +pub fn create_dir(path: impl AsRef) -> io::Result<()> { let path = path.as_ref(); fs::create_dir(path)?; @@ -18,7 +18,7 @@ pub fn create_dir(path: impl AsRef) -> io::Result<()> { /// Similar to [`std::fs::create_dir_all`], except we fsync all /// newly created directories and the pre-existing parent. -pub fn create_dir_all(path: impl AsRef) -> io::Result<()> { +pub fn create_dir_all(path: impl AsRef) -> io::Result<()> { let mut path = path.as_ref(); let mut dirs_to_create = Vec::new(); @@ -30,7 +30,7 @@ pub fn create_dir_all(path: impl AsRef) -> io::Result<()> { Ok(_) => { return Err(io::Error::new( io::ErrorKind::AlreadyExists, - format!("non-directory found in path: {}", path.display()), + format!("non-directory found in path: {path}"), )); } Err(ref e) if e.kind() == io::ErrorKind::NotFound => {} @@ -44,7 +44,7 @@ pub fn create_dir_all(path: impl AsRef) -> io::Result<()> { None => { return Err(io::Error::new( io::ErrorKind::InvalidInput, - format!("can't find parent of path '{}'", path.display()).as_str(), + format!("can't find parent of path '{path}'"), )); } } @@ -70,21 +70,18 @@ pub fn create_dir_all(path: impl AsRef) -> io::Result<()> { /// Adds a suffix to the file(directory) name, either appending the suffix to the end of its extension, /// or if there's no extension, creates one and puts a suffix there. -pub fn path_with_suffix_extension(original_path: impl AsRef, suffix: &str) -> PathBuf { - let new_extension = match original_path - .as_ref() - .extension() - .map(OsStr::to_string_lossy) - { +pub fn path_with_suffix_extension( + original_path: impl AsRef, + suffix: &str, +) -> Utf8PathBuf { + let new_extension = match original_path.as_ref().extension() { Some(extension) => Cow::Owned(format!("{extension}.{suffix}")), None => Cow::Borrowed(suffix), }; - original_path - .as_ref() - .with_extension(new_extension.as_ref()) + original_path.as_ref().with_extension(new_extension) } -pub fn fsync_file_and_parent(file_path: &Path) -> io::Result<()> { +pub fn fsync_file_and_parent(file_path: &Utf8Path) -> io::Result<()> { let parent = file_path.parent().ok_or_else(|| { io::Error::new( io::ErrorKind::Other, @@ -97,7 +94,7 @@ pub fn fsync_file_and_parent(file_path: &Path) -> io::Result<()> { Ok(()) } -pub fn fsync(path: &Path) -> io::Result<()> { +pub fn fsync(path: &Utf8Path) -> io::Result<()> { File::open(path) .map_err(|e| io::Error::new(e.kind(), format!("Failed to open the file {path:?}: {e}"))) .and_then(|file| { @@ -111,19 +108,18 @@ pub fn fsync(path: &Path) -> io::Result<()> { .map_err(|e| io::Error::new(e.kind(), format!("Failed to fsync file {path:?}: {e}"))) } -pub async fn fsync_async(path: impl AsRef) -> Result<(), std::io::Error> { - tokio::fs::File::open(path).await?.sync_all().await +pub async fn fsync_async(path: impl AsRef) -> Result<(), std::io::Error> { + tokio::fs::File::open(path.as_ref()).await?.sync_all().await } #[cfg(test)] mod tests { - use tempfile::tempdir; use super::*; #[test] fn test_create_dir_fsyncd() { - let dir = tempdir().unwrap(); + let dir = camino_tempfile::tempdir().unwrap(); let existing_dir_path = dir.path(); let err = create_dir(existing_dir_path).unwrap_err(); @@ -139,7 +135,7 @@ mod tests { #[test] fn test_create_dir_all_fsyncd() { - let dir = tempdir().unwrap(); + let dir = camino_tempfile::tempdir().unwrap(); let existing_dir_path = dir.path(); create_dir_all(existing_dir_path).unwrap(); @@ -166,29 +162,29 @@ mod tests { #[test] fn test_path_with_suffix_extension() { - let p = PathBuf::from("/foo/bar"); + let p = Utf8PathBuf::from("/foo/bar"); assert_eq!( - &path_with_suffix_extension(p, "temp").to_string_lossy(), + &path_with_suffix_extension(p, "temp").to_string(), "/foo/bar.temp" ); - let p = PathBuf::from("/foo/bar"); + let p = Utf8PathBuf::from("/foo/bar"); assert_eq!( - &path_with_suffix_extension(p, "temp.temp").to_string_lossy(), + &path_with_suffix_extension(p, "temp.temp").to_string(), "/foo/bar.temp.temp" ); - let p = PathBuf::from("/foo/bar.baz"); + let p = Utf8PathBuf::from("/foo/bar.baz"); assert_eq!( - &path_with_suffix_extension(p, "temp.temp").to_string_lossy(), + &path_with_suffix_extension(p, "temp.temp").to_string(), "/foo/bar.baz.temp.temp" ); - let p = PathBuf::from("/foo/bar.baz"); + let p = Utf8PathBuf::from("/foo/bar.baz"); assert_eq!( - &path_with_suffix_extension(p, ".temp").to_string_lossy(), + &path_with_suffix_extension(p, ".temp").to_string(), "/foo/bar.baz..temp" ); - let p = PathBuf::from("/foo/bar/dir/"); + let p = Utf8PathBuf::from("/foo/bar/dir/"); assert_eq!( - &path_with_suffix_extension(p, ".temp").to_string_lossy(), + &path_with_suffix_extension(p, ".temp").to_string(), "/foo/bar/dir..temp" ); } diff --git a/libs/utils/src/fs_ext.rs b/libs/utils/src/fs_ext.rs index dfb7d5abbf..90ba348a02 100644 --- a/libs/utils/src/fs_ext.rs +++ b/libs/utils/src/fs_ext.rs @@ -55,8 +55,6 @@ where #[cfg(test)] mod test { - use std::path::PathBuf; - use crate::fs_ext::{is_directory_empty, list_dir}; use super::ignore_absent_files; @@ -65,7 +63,7 @@ mod test { fn is_empty_dir() { use super::PathExt; - let dir = tempfile::tempdir().unwrap(); + let dir = camino_tempfile::tempdir().unwrap(); let dir_path = dir.path(); // test positive case @@ -75,7 +73,7 @@ mod test { ); // invoke on a file to ensure it returns an error - let file_path: PathBuf = dir_path.join("testfile"); + let file_path = dir_path.join("testfile"); let f = std::fs::File::create(&file_path).unwrap(); drop(f); assert!(file_path.is_empty_dir().is_err()); @@ -87,7 +85,7 @@ mod test { #[tokio::test] async fn is_empty_dir_async() { - let dir = tempfile::tempdir().unwrap(); + let dir = camino_tempfile::tempdir().unwrap(); let dir_path = dir.path(); // test positive case @@ -97,7 +95,7 @@ mod test { ); // invoke on a file to ensure it returns an error - let file_path: PathBuf = dir_path.join("testfile"); + let file_path = dir_path.join("testfile"); let f = std::fs::File::create(&file_path).unwrap(); drop(f); assert!(is_directory_empty(&file_path).await.is_err()); @@ -109,10 +107,9 @@ mod test { #[test] fn ignore_absent_files_works() { - let dir = tempfile::tempdir().unwrap(); - let dir_path = dir.path(); + let dir = camino_tempfile::tempdir().unwrap(); - let file_path: PathBuf = dir_path.join("testfile"); + let file_path = dir.path().join("testfile"); ignore_absent_files(|| std::fs::remove_file(&file_path)).expect("should execute normally"); @@ -126,17 +123,17 @@ mod test { #[tokio::test] async fn list_dir_works() { - let dir = tempfile::tempdir().unwrap(); + let dir = camino_tempfile::tempdir().unwrap(); let dir_path = dir.path(); assert!(list_dir(dir_path).await.unwrap().is_empty()); - let file_path: PathBuf = dir_path.join("testfile"); + let file_path = dir_path.join("testfile"); let _ = std::fs::File::create(&file_path).unwrap(); assert_eq!(&list_dir(dir_path).await.unwrap(), &["testfile"]); - let another_dir_path: PathBuf = dir_path.join("testdir"); + let another_dir_path = dir_path.join("testdir"); std::fs::create_dir(another_dir_path).unwrap(); let expected = &["testdir", "testfile"]; diff --git a/libs/utils/src/id.rs b/libs/utils/src/id.rs index 2ce92ee914..ec13c2f96f 100644 --- a/libs/utils/src/id.rs +++ b/libs/utils/src/id.rs @@ -1,4 +1,3 @@ -use std::ffi::OsStr; use std::{fmt, str::FromStr}; use anyhow::Context; @@ -215,12 +214,11 @@ pub struct TimelineId(Id); id_newtype!(TimelineId); -impl TryFrom> for TimelineId { +impl TryFrom> for TimelineId { type Error = anyhow::Error; - fn try_from(value: Option<&OsStr>) -> Result { + fn try_from(value: Option<&str>) -> Result { value - .and_then(OsStr::to_str) .unwrap_or_default() .parse::() .with_context(|| format!("Could not parse timeline id from {:?}", value)) diff --git a/libs/utils/src/lock_file.rs b/libs/utils/src/lock_file.rs index ca8295040c..987b9d9ad2 100644 --- a/libs/utils/src/lock_file.rs +++ b/libs/utils/src/lock_file.rs @@ -11,10 +11,10 @@ use std::{ io::{Read, Write}, ops::Deref, os::unix::prelude::AsRawFd, - path::{Path, PathBuf}, }; use anyhow::Context; +use camino::{Utf8Path, Utf8PathBuf}; use nix::{errno::Errno::EAGAIN, fcntl}; use crate::crashsafe; @@ -23,7 +23,7 @@ use crate::crashsafe; /// Returned by [`create_exclusive`]. #[must_use] pub struct UnwrittenLockFile { - path: PathBuf, + path: Utf8PathBuf, file: fs::File, } @@ -60,7 +60,7 @@ impl UnwrittenLockFile { /// /// It is not an error if the file already exists. /// It is an error if the file is already locked. -pub fn create_exclusive(lock_file_path: &Path) -> anyhow::Result { +pub fn create_exclusive(lock_file_path: &Utf8Path) -> anyhow::Result { let lock_file = fs::OpenOptions::new() .create(true) // O_CREAT .write(true) @@ -101,7 +101,7 @@ pub enum LockFileRead { /// Open & try to lock the lock file at the given `path`, returning a [handle][`LockFileRead`] to /// inspect its content. It is not an `Err(...)` if the file does not exist or is already locked. /// Check the [`LockFileRead`] variants for details. -pub fn read_and_hold_lock_file(path: &Path) -> anyhow::Result { +pub fn read_and_hold_lock_file(path: &Utf8Path) -> anyhow::Result { let res = fs::OpenOptions::new().read(true).open(path); let mut lock_file = match res { Ok(f) => f, diff --git a/libs/utils/src/lsn.rs b/libs/utils/src/lsn.rs index 0493d43088..7d9baf7d49 100644 --- a/libs/utils/src/lsn.rs +++ b/libs/utils/src/lsn.rs @@ -1,9 +1,9 @@ #![warn(missing_docs)] +use camino::Utf8Path; use serde::{Deserialize, Serialize}; use std::fmt; use std::ops::{Add, AddAssign}; -use std::path::Path; use std::str::FromStr; use std::sync::atomic::{AtomicU64, Ordering}; @@ -44,11 +44,9 @@ impl Lsn { /// Parse an LSN from a filename in the form `0000000000000000` pub fn from_filename(filename: F) -> Result where - F: AsRef, + F: AsRef, { - let filename: &Path = filename.as_ref(); - let filename = filename.to_str().ok_or(LsnParseError)?; - Lsn::from_hex(filename) + Lsn::from_hex(filename.as_ref().as_str()) } /// Parse an LSN from a string in the form `0000000000000000` diff --git a/libs/utils/src/pid_file.rs b/libs/utils/src/pid_file.rs index e634b08f2a..06f5d950d1 100644 --- a/libs/utils/src/pid_file.rs +++ b/libs/utils/src/pid_file.rs @@ -49,9 +49,10 @@ //! At this point, `B` and `C` are running, which is hazardous. //! Morale of the story: don't unlink pidfiles, ever. -use std::{ops::Deref, path::Path}; +use std::ops::Deref; use anyhow::Context; +use camino::Utf8Path; use nix::unistd::Pid; use crate::lock_file::{self, LockFileRead}; @@ -84,7 +85,7 @@ impl Deref for PidFileGuard { /// The claim ends as soon as the returned guard object is dropped. /// To maintain the claim for the remaining lifetime of the current process, /// use [`std::mem::forget`] or similar. -pub fn claim_for_current_process(path: &Path) -> anyhow::Result { +pub fn claim_for_current_process(path: &Utf8Path) -> anyhow::Result { let unwritten_lock_file = lock_file::create_exclusive(path).context("lock file")?; // if any of the next steps fail, we drop the file descriptor and thereby release the lock let guard = unwritten_lock_file @@ -132,7 +133,7 @@ pub enum PidFileRead { /// /// On success, this function returns a [`PidFileRead`]. /// Check its docs for a description of the meaning of its different variants. -pub fn read(pidfile: &Path) -> anyhow::Result { +pub fn read(pidfile: &Utf8Path) -> anyhow::Result { let res = lock_file::read_and_hold_lock_file(pidfile).context("read and hold pid file")?; let ret = match res { LockFileRead::NotExist => PidFileRead::NotExist, diff --git a/pageserver/Cargo.toml b/pageserver/Cargo.toml index 9cb71dea09..3eb01003df 100644 --- a/pageserver/Cargo.toml +++ b/pageserver/Cargo.toml @@ -17,6 +17,8 @@ async-stream.workspace = true async-trait.workspace = true byteorder.workspace = true bytes.workspace = true +camino.workspace = true +camino-tempfile.workspace = true chrono = { workspace = true, features = ["serde"] } clap = { workspace = true, features = ["string"] } close_fds.workspace = true @@ -80,7 +82,6 @@ enum-map.workspace = true enumset.workspace = true strum.workspace = true strum_macros.workspace = true -tempfile.workspace = true [dev-dependencies] criterion.workspace = true diff --git a/pageserver/benches/bench_walredo.rs b/pageserver/benches/bench_walredo.rs index 49216c68f1..6cda86fafa 100644 --- a/pageserver/benches/bench_walredo.rs +++ b/pageserver/benches/bench_walredo.rs @@ -25,7 +25,7 @@ fn redo_scenarios(c: &mut Criterion) { // input to the stderr. // utils::logging::init(utils::logging::LogFormat::Plain).unwrap(); - let repo_dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let repo_dir = camino_tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); let conf = PageServerConf::dummy_conf(repo_dir.path().to_path_buf()); let conf = Box::leak(Box::new(conf)); diff --git a/pageserver/ctl/Cargo.toml b/pageserver/ctl/Cargo.toml index b3f12b72b3..ff0c530125 100644 --- a/pageserver/ctl/Cargo.toml +++ b/pageserver/ctl/Cargo.toml @@ -9,6 +9,7 @@ license.workspace = true [dependencies] anyhow.workspace = true bytes.workspace = true +camino.workspace = true clap = { workspace = true, features = ["string"] } git-version.workspace = true pageserver = { path = ".." } diff --git a/pageserver/ctl/src/layer_map_analyzer.rs b/pageserver/ctl/src/layer_map_analyzer.rs index de7b4861cb..15d4eb09e0 100644 --- a/pageserver/ctl/src/layer_map_analyzer.rs +++ b/pageserver/ctl/src/layer_map_analyzer.rs @@ -3,13 +3,14 @@ //! Currently it only analyzes holes, which are regions within the layer range that the layer contains no updates for. In the future it might do more analysis (maybe key quantiles?) but it should never return sensitive data. use anyhow::Result; +use camino::{Utf8Path, Utf8PathBuf}; use pageserver::context::{DownloadBehavior, RequestContext}; use pageserver::task_mgr::TaskKind; use pageserver::tenant::{TENANTS_SEGMENT_NAME, TIMELINES_SEGMENT_NAME}; use std::cmp::Ordering; use std::collections::BinaryHeap; use std::ops::Range; -use std::{fs, path::Path, str}; +use std::{fs, str}; use pageserver::page_cache::PAGE_SZ; use pageserver::repository::{Key, KEY_SIZE}; @@ -98,7 +99,7 @@ pub(crate) fn parse_filename(name: &str) -> Option { } // Finds the max_holes largest holes, ignoring any that are smaller than MIN_HOLE_LENGTH" -async fn get_holes(path: &Path, max_holes: usize, ctx: &RequestContext) -> Result> { +async fn get_holes(path: &Utf8Path, max_holes: usize, ctx: &RequestContext) -> Result> { let file = FileBlockReader::new(VirtualFile::open(path).await?); let summary_blk = file.read_blk(0, ctx).await?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; @@ -167,7 +168,9 @@ pub(crate) async fn main(cmd: &AnalyzeLayerMapCmd) -> Result<()> { parse_filename(&layer.file_name().into_string().unwrap()) { if layer_file.is_delta { - layer_file.holes = get_holes(&layer.path(), max_holes, &ctx).await?; + let layer_path = + Utf8PathBuf::from_path_buf(layer.path()).expect("non-Unicode path"); + layer_file.holes = get_holes(&layer_path, max_holes, &ctx).await?; n_deltas += 1; } layers.push(layer_file); diff --git a/pageserver/ctl/src/layers.rs b/pageserver/ctl/src/layers.rs index e8d16d31f1..22ebe70b16 100644 --- a/pageserver/ctl/src/layers.rs +++ b/pageserver/ctl/src/layers.rs @@ -1,6 +1,7 @@ use std::path::{Path, PathBuf}; use anyhow::Result; +use camino::Utf8Path; use clap::Subcommand; use pageserver::context::{DownloadBehavior, RequestContext}; use pageserver::task_mgr::TaskKind; @@ -47,7 +48,7 @@ pub(crate) enum LayerCmd { } async fn read_delta_file(path: impl AsRef, ctx: &RequestContext) -> Result<()> { - let path = path.as_ref(); + let path = Utf8Path::from_path(path.as_ref()).expect("non-Unicode path"); virtual_file::init(10); page_cache::init(100); let file = FileBlockReader::new(VirtualFile::open(path).await?); diff --git a/pageserver/ctl/src/main.rs b/pageserver/ctl/src/main.rs index 0d154aca0c..c4d6e8d883 100644 --- a/pageserver/ctl/src/main.rs +++ b/pageserver/ctl/src/main.rs @@ -8,6 +8,7 @@ mod draw_timeline_dir; mod layer_map_analyzer; mod layers; +use camino::{Utf8Path, Utf8PathBuf}; use clap::{Parser, Subcommand}; use layers::LayerCmd; use pageserver::{ @@ -18,7 +19,6 @@ use pageserver::{ virtual_file, }; use postgres_ffi::ControlFileData; -use std::path::{Path, PathBuf}; use utils::{lsn::Lsn, project_git_version}; project_git_version!(GIT_VERSION); @@ -49,7 +49,7 @@ enum Commands { #[derive(Parser)] struct MetadataCmd { /// Input metadata file path - metadata_path: PathBuf, + metadata_path: Utf8PathBuf, /// Replace disk consistent Lsn disk_consistent_lsn: Option, /// Replace previous record Lsn @@ -61,13 +61,13 @@ struct MetadataCmd { #[derive(Parser)] struct PrintLayerFileCmd { /// Pageserver data path - path: PathBuf, + path: Utf8PathBuf, } #[derive(Parser)] struct AnalyzeLayerMapCmd { /// Pageserver data path - path: PathBuf, + path: Utf8PathBuf, /// Max holes max_holes: Option, } @@ -102,7 +102,7 @@ async fn main() -> anyhow::Result<()> { Ok(()) } -fn read_pg_control_file(control_file_path: &Path) -> anyhow::Result<()> { +fn read_pg_control_file(control_file_path: &Utf8Path) -> anyhow::Result<()> { let control_file = ControlFileData::decode(&std::fs::read(control_file_path)?)?; println!("{control_file:?}"); let control_file_initdb = Lsn(control_file.checkPoint); @@ -114,7 +114,7 @@ fn read_pg_control_file(control_file_path: &Path) -> anyhow::Result<()> { Ok(()) } -async fn print_layerfile(path: &Path) -> anyhow::Result<()> { +async fn print_layerfile(path: &Utf8Path) -> anyhow::Result<()> { // Basic initialization of things that don't change after startup virtual_file::init(10); page_cache::init(100); diff --git a/pageserver/src/bin/pageserver.rs b/pageserver/src/bin/pageserver.rs index d8a00b677b..7e2b376212 100644 --- a/pageserver/src/bin/pageserver.rs +++ b/pageserver/src/bin/pageserver.rs @@ -2,9 +2,10 @@ use std::env::{var, VarError}; use std::sync::Arc; -use std::{env, ops::ControlFlow, path::Path, str::FromStr}; +use std::{env, ops::ControlFlow, str::FromStr}; use anyhow::{anyhow, Context}; +use camino::Utf8Path; use clap::{Arg, ArgAction, Command}; use metrics::launch_timestamp::{set_launch_timestamp_metric, LaunchTimestamp}; @@ -65,21 +66,17 @@ fn main() -> anyhow::Result<()> { let workdir = arg_matches .get_one::("workdir") - .map(Path::new) - .unwrap_or_else(|| Path::new(".neon")); + .map(Utf8Path::new) + .unwrap_or_else(|| Utf8Path::new(".neon")); let workdir = workdir - .canonicalize() - .with_context(|| format!("Error opening workdir '{}'", workdir.display()))?; + .canonicalize_utf8() + .with_context(|| format!("Error opening workdir '{workdir}'"))?; let cfg_file_path = workdir.join("pageserver.toml"); // Set CWD to workdir for non-daemon modes - env::set_current_dir(&workdir).with_context(|| { - format!( - "Failed to set application's current dir to '{}'", - workdir.display() - ) - })?; + env::set_current_dir(&workdir) + .with_context(|| format!("Failed to set application's current dir to '{workdir}'"))?; let conf = match initialize_config(&cfg_file_path, arg_matches, &workdir)? { ControlFlow::Continue(conf) => conf, @@ -115,12 +112,8 @@ fn main() -> anyhow::Result<()> { let tenants_path = conf.tenants_path(); if !tenants_path.exists() { - utils::crashsafe::create_dir_all(conf.tenants_path()).with_context(|| { - format!( - "Failed to create tenants root dir at '{}'", - tenants_path.display() - ) - })?; + utils::crashsafe::create_dir_all(conf.tenants_path()) + .with_context(|| format!("Failed to create tenants root dir at '{tenants_path}'"))?; } // Initialize up failpoints support @@ -137,9 +130,9 @@ fn main() -> anyhow::Result<()> { } fn initialize_config( - cfg_file_path: &Path, + cfg_file_path: &Utf8Path, arg_matches: clap::ArgMatches, - workdir: &Path, + workdir: &Utf8Path, ) -> anyhow::Result> { let init = arg_matches.get_flag("init"); let update_config = init || arg_matches.get_flag("update-config"); @@ -147,33 +140,22 @@ fn initialize_config( let (mut toml, config_file_exists) = if cfg_file_path.is_file() { if init { anyhow::bail!( - "Config file '{}' already exists, cannot init it, use --update-config to update it", - cfg_file_path.display() + "Config file '{cfg_file_path}' already exists, cannot init it, use --update-config to update it", ); } // Supplement the CLI arguments with the config file - let cfg_file_contents = std::fs::read_to_string(cfg_file_path).with_context(|| { - format!( - "Failed to read pageserver config at '{}'", - cfg_file_path.display() - ) - })?; + let cfg_file_contents = std::fs::read_to_string(cfg_file_path) + .with_context(|| format!("Failed to read pageserver config at '{cfg_file_path}'"))?; ( cfg_file_contents .parse::() .with_context(|| { - format!( - "Failed to parse '{}' as pageserver config", - cfg_file_path.display() - ) + format!("Failed to parse '{cfg_file_path}' as pageserver config") })?, true, ) } else if cfg_file_path.exists() { - anyhow::bail!( - "Config file '{}' exists but is not a regular file", - cfg_file_path.display() - ); + anyhow::bail!("Config file '{cfg_file_path}' exists but is not a regular file"); } else { // We're initializing the tenant, so there's no config file yet ( @@ -192,7 +174,7 @@ fn initialize_config( for (key, item) in doc.iter() { if config_file_exists && update_config && key == "id" && toml.contains_key(key) { - anyhow::bail!("Pageserver config file exists at '{}' and has node id already, it cannot be overridden", cfg_file_path.display()); + anyhow::bail!("Pageserver config file exists at '{cfg_file_path}' and has node id already, it cannot be overridden"); } toml.insert(key, item.clone()); } @@ -204,18 +186,11 @@ fn initialize_config( .context("Failed to parse pageserver configuration")?; if update_config { - info!("Writing pageserver config to '{}'", cfg_file_path.display()); + info!("Writing pageserver config to '{cfg_file_path}'"); - std::fs::write(cfg_file_path, toml.to_string()).with_context(|| { - format!( - "Failed to write pageserver config to '{}'", - cfg_file_path.display() - ) - })?; - info!( - "Config successfully written to '{}'", - cfg_file_path.display() - ) + std::fs::write(cfg_file_path, toml.to_string()) + .with_context(|| format!("Failed to write pageserver config to '{cfg_file_path}'"))?; + info!("Config successfully written to '{cfg_file_path}'") } Ok(if init { diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index c3f2f14a74..19c4a32bde 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -16,13 +16,13 @@ use utils::logging::SecretString; use once_cell::sync::OnceCell; use reqwest::Url; use std::num::NonZeroUsize; -use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::Arc; use std::time::Duration; use toml_edit; use toml_edit::{Document, Item}; +use camino::{Utf8Path, Utf8PathBuf}; use postgres_backend::AuthType; use utils::{ id::{NodeId, TenantId, TimelineId}, @@ -153,9 +153,9 @@ pub struct PageServerConf { // 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 workdir: Utf8PathBuf, - pub pg_distrib_dir: PathBuf, + pub pg_distrib_dir: Utf8PathBuf, // Authentication /// authentication method for the HTTP mgmt API @@ -164,7 +164,7 @@ pub struct PageServerConf { pub pg_auth_type: AuthType, /// Path to a file containing public key for verifying JWT tokens. /// Used for both mgmt and compute auth, if enabled. - pub auth_validation_public_key_path: Option, + pub auth_validation_public_key_path: Option, pub remote_storage_config: Option, @@ -253,15 +253,15 @@ struct PageServerConfigBuilder { page_cache_size: BuilderValue, max_file_descriptors: BuilderValue, - workdir: BuilderValue, + workdir: BuilderValue, - pg_distrib_dir: BuilderValue, + pg_distrib_dir: BuilderValue, http_auth_type: BuilderValue, pg_auth_type: BuilderValue, // - auth_validation_public_key_path: BuilderValue>, + auth_validation_public_key_path: BuilderValue>, remote_storage_config: BuilderValue>, id: BuilderValue, @@ -305,10 +305,12 @@ impl Default for PageServerConfigBuilder { superuser: Set(DEFAULT_SUPERUSER.to_string()), page_cache_size: Set(DEFAULT_PAGE_CACHE_SIZE), max_file_descriptors: Set(DEFAULT_MAX_FILE_DESCRIPTORS), - workdir: Set(PathBuf::new()), - pg_distrib_dir: Set(env::current_dir() - .expect("cannot access current directory") - .join("pg_install")), + workdir: Set(Utf8PathBuf::new()), + pg_distrib_dir: Set(Utf8PathBuf::from_path_buf( + env::current_dir().expect("cannot access current directory"), + ) + .expect("non-Unicode path") + .join("pg_install")), http_auth_type: Set(AuthType::Trust), pg_auth_type: Set(AuthType::Trust), auth_validation_public_key_path: Set(None), @@ -390,11 +392,11 @@ impl PageServerConfigBuilder { self.max_file_descriptors = BuilderValue::Set(max_file_descriptors) } - pub fn workdir(&mut self, workdir: PathBuf) { + pub fn workdir(&mut self, workdir: Utf8PathBuf) { self.workdir = BuilderValue::Set(workdir) } - pub fn pg_distrib_dir(&mut self, pg_distrib_dir: PathBuf) { + pub fn pg_distrib_dir(&mut self, pg_distrib_dir: Utf8PathBuf) { self.pg_distrib_dir = BuilderValue::Set(pg_distrib_dir) } @@ -408,7 +410,7 @@ impl PageServerConfigBuilder { pub fn auth_validation_public_key_path( &mut self, - auth_validation_public_key_path: Option, + auth_validation_public_key_path: Option, ) { self.auth_validation_public_key_path = BuilderValue::Set(auth_validation_public_key_path) } @@ -585,15 +587,15 @@ impl PageServerConf { // Repository paths, relative to workdir. // - pub fn tenants_path(&self) -> PathBuf { + pub fn tenants_path(&self) -> Utf8PathBuf { self.workdir.join(TENANTS_SEGMENT_NAME) } - pub fn deletion_prefix(&self) -> PathBuf { + pub fn deletion_prefix(&self) -> Utf8PathBuf { self.workdir.join("deletion") } - pub fn deletion_list_path(&self, sequence: u64) -> PathBuf { + pub fn deletion_list_path(&self, sequence: u64) -> Utf8PathBuf { // Encode a version in the filename, so that if we ever switch away from JSON we can // increment this. const VERSION: u8 = 1; @@ -602,7 +604,7 @@ impl PageServerConf { .join(format!("{sequence:016x}-{VERSION:02x}.list")) } - pub fn deletion_header_path(&self) -> PathBuf { + pub fn deletion_header_path(&self) -> Utf8PathBuf { // Encode a version in the filename, so that if we ever switch away from JSON we can // increment this. const VERSION: u8 = 1; @@ -610,30 +612,30 @@ impl PageServerConf { self.deletion_prefix().join(format!("header-{VERSION:02x}")) } - pub fn tenant_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenants_path().join(tenant_id.to_string()) } - pub fn tenant_attaching_mark_file_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_attaching_mark_file_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id) .join(TENANT_ATTACHING_MARKER_FILENAME) } - pub fn tenant_ignore_mark_file_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_ignore_mark_file_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id).join(IGNORED_TENANT_FILE_NAME) } /// Points to a place in pageserver's local directory, /// where certain tenant's tenantconf file should be located. - pub fn tenant_config_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_config_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id).join(TENANT_CONFIG_NAME) } - pub fn timelines_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn timelines_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id).join(TIMELINES_SEGMENT_NAME) } - pub fn timeline_path(&self, tenant_id: &TenantId, timeline_id: &TimelineId) -> PathBuf { + pub fn timeline_path(&self, tenant_id: &TenantId, timeline_id: &TimelineId) -> Utf8PathBuf { self.timelines_path(tenant_id).join(timeline_id.to_string()) } @@ -641,7 +643,7 @@ impl PageServerConf { &self, tenant_id: TenantId, timeline_id: TimelineId, - ) -> PathBuf { + ) -> Utf8PathBuf { path_with_suffix_extension( self.timeline_path(&tenant_id, &timeline_id), TIMELINE_UNINIT_MARK_SUFFIX, @@ -652,19 +654,19 @@ impl PageServerConf { &self, tenant_id: TenantId, timeline_id: TimelineId, - ) -> PathBuf { + ) -> Utf8PathBuf { path_with_suffix_extension( self.timeline_path(&tenant_id, &timeline_id), TIMELINE_DELETE_MARK_SUFFIX, ) } - pub fn tenant_deleted_mark_file_path(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_deleted_mark_file_path(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.tenant_path(tenant_id) .join(TENANT_DELETED_MARKER_FILE_NAME) } - pub fn traces_path(&self) -> PathBuf { + pub fn traces_path(&self) -> Utf8PathBuf { self.workdir.join("traces") } @@ -673,7 +675,7 @@ impl PageServerConf { tenant_id: &TenantId, timeline_id: &TimelineId, connection_id: &ConnectionId, - ) -> PathBuf { + ) -> Utf8PathBuf { self.traces_path() .join(tenant_id.to_string()) .join(timeline_id.to_string()) @@ -682,20 +684,20 @@ impl PageServerConf { /// Points to a place in pageserver's local directory, /// where certain timeline's metadata file should be located. - pub fn metadata_path(&self, tenant_id: &TenantId, timeline_id: &TimelineId) -> PathBuf { + pub fn metadata_path(&self, tenant_id: &TenantId, timeline_id: &TimelineId) -> Utf8PathBuf { self.timeline_path(tenant_id, timeline_id) .join(METADATA_FILE_NAME) } /// Turns storage remote path of a file into its local path. - pub fn local_path(&self, remote_path: &RemotePath) -> PathBuf { + pub fn local_path(&self, remote_path: &RemotePath) -> Utf8PathBuf { remote_path.with_base(&self.workdir) } // // Postgres distribution paths // - pub fn pg_distrib_dir(&self, pg_version: u32) -> anyhow::Result { + pub fn pg_distrib_dir(&self, pg_version: u32) -> anyhow::Result { let path = self.pg_distrib_dir.clone(); #[allow(clippy::manual_range_patterns)] @@ -705,10 +707,10 @@ impl PageServerConf { } } - pub fn pg_bin_dir(&self, pg_version: u32) -> anyhow::Result { + pub fn pg_bin_dir(&self, pg_version: u32) -> anyhow::Result { Ok(self.pg_distrib_dir(pg_version)?.join("bin")) } - pub fn pg_lib_dir(&self, pg_version: u32) -> anyhow::Result { + pub fn pg_lib_dir(&self, pg_version: u32) -> anyhow::Result { Ok(self.pg_distrib_dir(pg_version)?.join("lib")) } @@ -716,7 +718,7 @@ impl PageServerConf { /// validating the input and failing on errors. /// /// This leaves any options not present in the file in the built-in defaults. - pub fn parse_and_validate(toml: &Document, workdir: &Path) -> anyhow::Result { + pub fn parse_and_validate(toml: &Document, workdir: &Utf8Path) -> anyhow::Result { let mut builder = PageServerConfigBuilder::default(); builder.workdir(workdir.to_owned()); @@ -735,10 +737,10 @@ impl PageServerConf { builder.max_file_descriptors(parse_toml_u64(key, item)? as usize) } "pg_distrib_dir" => { - builder.pg_distrib_dir(PathBuf::from(parse_toml_string(key, item)?)) + builder.pg_distrib_dir(Utf8PathBuf::from(parse_toml_string(key, item)?)) } "auth_validation_public_key_path" => builder.auth_validation_public_key_path(Some( - PathBuf::from(parse_toml_string(key, item)?), + Utf8PathBuf::from(parse_toml_string(key, item)?), )), "http_auth_type" => builder.http_auth_type(parse_toml_from_str(key, item)?), "pg_auth_type" => builder.pg_auth_type(parse_toml_from_str(key, item)?), @@ -798,8 +800,7 @@ impl PageServerConf { ensure!( auth_validation_public_key_path.exists(), format!( - "Can't find auth_validation_public_key at '{}'", - auth_validation_public_key_path.display() + "Can't find auth_validation_public_key at '{auth_validation_public_key_path}'", ) ); } @@ -915,12 +916,12 @@ impl PageServerConf { } #[cfg(test)] - pub fn test_repo_dir(test_name: &str) -> PathBuf { - PathBuf::from(format!("../tmp_check/test_{test_name}")) + pub fn test_repo_dir(test_name: &str) -> Utf8PathBuf { + Utf8PathBuf::from(format!("../tmp_check/test_{test_name}")) } - pub fn dummy_conf(repo_dir: PathBuf) -> Self { - let pg_distrib_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../pg_install"); + pub fn dummy_conf(repo_dir: Utf8PathBuf) -> Self { + let pg_distrib_dir = Utf8PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../pg_install"); PageServerConf { id: NodeId(0), @@ -1087,8 +1088,8 @@ mod tests { num::{NonZeroU32, NonZeroUsize}, }; + use camino_tempfile::{tempdir, Utf8TempDir}; use remote_storage::{RemoteStorageKind, S3Config}; - use tempfile::{tempdir, TempDir}; use utils::serde_percent::Percent; use super::*; @@ -1127,8 +1128,7 @@ background_task_maximum_delay = '334 s' let broker_endpoint = storage_broker::DEFAULT_ENDPOINT; // we have to create dummy values to overcome the validation errors let config_string = format!( - "pg_distrib_dir='{}'\nid=10\nbroker_endpoint = '{broker_endpoint}'", - pg_distrib_dir.display() + "pg_distrib_dir='{pg_distrib_dir}'\nid=10\nbroker_endpoint = '{broker_endpoint}'", ); let toml = config_string.parse()?; @@ -1194,8 +1194,7 @@ background_task_maximum_delay = '334 s' let broker_endpoint = storage_broker::DEFAULT_ENDPOINT; let config_string = format!( - "{ALL_BASE_VALUES_TOML}pg_distrib_dir='{}'\nbroker_endpoint = '{broker_endpoint}'", - pg_distrib_dir.display() + "{ALL_BASE_VALUES_TOML}pg_distrib_dir='{pg_distrib_dir}'\nbroker_endpoint = '{broker_endpoint}'", ); let toml = config_string.parse()?; @@ -1255,23 +1254,18 @@ background_task_maximum_delay = '334 s' let identical_toml_declarations = &[ format!( r#"[remote_storage] -local_path = '{}'"#, - local_storage_path.display() - ), - format!( - "remote_storage={{local_path='{}'}}", - local_storage_path.display() +local_path = '{local_storage_path}'"#, ), + format!("remote_storage={{local_path='{local_storage_path}'}}"), ]; for remote_storage_config_str in identical_toml_declarations { let config_string = format!( r#"{ALL_BASE_VALUES_TOML} -pg_distrib_dir='{}' +pg_distrib_dir='{pg_distrib_dir}' broker_endpoint = '{broker_endpoint}' {remote_storage_config_str}"#, - pg_distrib_dir.display(), ); let toml = config_string.parse()?; @@ -1334,11 +1328,10 @@ concurrency_limit = {s3_concurrency_limit}"# for remote_storage_config_str in identical_toml_declarations { let config_string = format!( r#"{ALL_BASE_VALUES_TOML} -pg_distrib_dir='{}' +pg_distrib_dir='{pg_distrib_dir}' broker_endpoint = '{broker_endpoint}' {remote_storage_config_str}"#, - pg_distrib_dir.display(), ); let toml = config_string.parse()?; @@ -1380,12 +1373,11 @@ broker_endpoint = '{broker_endpoint}' let config_string = format!( r#"{ALL_BASE_VALUES_TOML} -pg_distrib_dir='{}' +pg_distrib_dir='{pg_distrib_dir}' broker_endpoint = '{broker_endpoint}' [tenant_config] trace_read_requests = {trace_read_requests}"#, - pg_distrib_dir.display(), ); let toml = config_string.parse()?; @@ -1405,7 +1397,7 @@ trace_read_requests = {trace_read_requests}"#, let (workdir, pg_distrib_dir) = prepare_fs(&tempdir)?; let pageserver_conf_toml = format!( - r#"pg_distrib_dir = "{}" + r#"pg_distrib_dir = "{pg_distrib_dir}" metric_collection_endpoint = "http://sample.url" metric_collection_interval = "10min" id = 222 @@ -1423,7 +1415,6 @@ kind = "LayerAccessThreshold" period = "20m" threshold = "20m" "#, - pg_distrib_dir.display(), ); let toml: Document = pageserver_conf_toml.parse()?; let conf = PageServerConf::parse_and_validate(&toml, &workdir)?; @@ -1464,7 +1455,7 @@ threshold = "20m" Ok(()) } - fn prepare_fs(tempdir: &TempDir) -> anyhow::Result<(PathBuf, PathBuf)> { + fn prepare_fs(tempdir: &Utf8TempDir) -> anyhow::Result<(Utf8PathBuf, Utf8PathBuf)> { let tempdir_path = tempdir.path(); let workdir = tempdir_path.join("workdir"); diff --git a/pageserver/src/consumption_metrics.rs b/pageserver/src/consumption_metrics.rs index 5f64bb2b3b..72a2099d92 100644 --- a/pageserver/src/consumption_metrics.rs +++ b/pageserver/src/consumption_metrics.rs @@ -3,11 +3,11 @@ use crate::context::{DownloadBehavior, RequestContext}; use crate::task_mgr::{self, TaskKind, BACKGROUND_RUNTIME}; use crate::tenant::{mgr, LogicalSizeCalculationCause}; +use camino::Utf8PathBuf; use consumption_metrics::EventType; use pageserver_api::models::TenantState; use reqwest::Url; use std::collections::HashMap; -use std::path::PathBuf; use std::sync::Arc; use std::time::{Duration, SystemTime}; use tracing::*; @@ -41,7 +41,7 @@ pub async fn collect_metrics( _cached_metric_collection_interval: Duration, synthetic_size_calculation_interval: Duration, node_id: NodeId, - local_disk_storage: PathBuf, + local_disk_storage: Utf8PathBuf, ctx: RequestContext, ) -> anyhow::Result<()> { if _cached_metric_collection_interval != Duration::ZERO { @@ -68,7 +68,7 @@ pub async fn collect_metrics( }, ); - let path: Arc = Arc::new(local_disk_storage); + let path: Arc = Arc::new(local_disk_storage); let cancel = task_mgr::shutdown_token(); @@ -153,7 +153,7 @@ pub async fn collect_metrics( /// /// Cancellation safe. async fn restore_and_reschedule( - path: &Arc, + path: &Arc, metric_collection_interval: Duration, ) -> Cache { let (cached, earlier_metric_at) = match disk_cache::read_metrics_from_disk(path.clone()).await { diff --git a/pageserver/src/consumption_metrics/disk_cache.rs b/pageserver/src/consumption_metrics/disk_cache.rs index 4b1cd79c6d..387bf7a0f9 100644 --- a/pageserver/src/consumption_metrics/disk_cache.rs +++ b/pageserver/src/consumption_metrics/disk_cache.rs @@ -1,10 +1,12 @@ use anyhow::Context; -use std::path::PathBuf; +use camino::{Utf8Path, Utf8PathBuf}; use std::sync::Arc; use super::RawMetric; -pub(super) async fn read_metrics_from_disk(path: Arc) -> anyhow::Result> { +pub(super) async fn read_metrics_from_disk( + path: Arc, +) -> anyhow::Result> { // do not add context to each error, callsite will log with full path let span = tracing::Span::current(); tokio::task::spawn_blocking(move || { @@ -25,10 +27,10 @@ pub(super) async fn read_metrics_from_disk(path: Arc) -> anyhow::Result .and_then(|x| x) } -fn scan_and_delete_with_same_prefix(path: &std::path::Path) -> std::io::Result<()> { +fn scan_and_delete_with_same_prefix(path: &Utf8Path) -> std::io::Result<()> { let it = std::fs::read_dir(path.parent().expect("caller checked"))?; - let prefix = path.file_name().expect("caller checked").to_string_lossy(); + let prefix = path.file_name().expect("caller checked").to_string(); for entry in it { let entry = entry?; @@ -62,7 +64,7 @@ fn scan_and_delete_with_same_prefix(path: &std::path::Path) -> std::io::Result<( pub(super) async fn flush_metrics_to_disk( current_metrics: &Arc>, - path: &Arc, + path: &Arc, ) -> anyhow::Result<()> { use std::io::Write; @@ -81,7 +83,7 @@ pub(super) async fn flush_metrics_to_disk( let parent = path.parent().expect("existence checked"); let file_name = path.file_name().expect("existence checked"); - let mut tempfile = tempfile::Builder::new() + let mut tempfile = camino_tempfile::Builder::new() .prefix(file_name) .suffix(".tmp") .tempfile_in(parent)?; diff --git a/pageserver/src/deletion_queue.rs b/pageserver/src/deletion_queue.rs index 4c0d399789..487a9d47ee 100644 --- a/pageserver/src/deletion_queue.rs +++ b/pageserver/src/deletion_queue.rs @@ -3,7 +3,6 @@ mod list_writer; mod validator; use std::collections::HashMap; -use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -13,6 +12,7 @@ use crate::tenant::remote_timeline_client::remote_layer_path; use crate::tenant::remote_timeline_client::remote_timeline_path; use crate::virtual_file::VirtualFile; use anyhow::Context; +use camino::Utf8PathBuf; use hex::FromHex; use remote_storage::{GenericRemoteStorage, RemotePath}; use serde::Deserialize; @@ -336,7 +336,6 @@ impl DeletionList { timeline_entry.extend(objects.drain(..).map(|p| { p.strip_prefix(&timeline_remote_path) .expect("Timeline paths always start with the timeline prefix") - .to_string_lossy() .to_string() })); true @@ -350,7 +349,7 @@ impl DeletionList { result.extend( timeline_layers .into_iter() - .map(|l| timeline_remote_path.join(&PathBuf::from(l))), + .map(|l| timeline_remote_path.join(&Utf8PathBuf::from(l))), ); } } @@ -727,12 +726,9 @@ impl DeletionQueue { #[cfg(test)] mod test { + use camino::Utf8Path; use hex_literal::hex; - use std::{ - io::ErrorKind, - path::{Path, PathBuf}, - time::Duration, - }; + use std::{io::ErrorKind, time::Duration}; use tracing::info; use remote_storage::{RemoteStorageConfig, RemoteStorageKind}; @@ -764,7 +760,7 @@ mod test { struct TestSetup { harness: TenantHarness, - remote_fs_dir: PathBuf, + remote_fs_dir: Utf8PathBuf, storage: GenericRemoteStorage, mock_control_plane: MockControlPlane, deletion_queue: DeletionQueue, @@ -873,7 +869,7 @@ mod test { // Set up a GenericRemoteStorage targetting a directory let remote_fs_dir = harness.conf.workdir.join("remote_fs"); std::fs::create_dir_all(remote_fs_dir)?; - let remote_fs_dir = std::fs::canonicalize(harness.conf.workdir.join("remote_fs"))?; + let remote_fs_dir = harness.conf.workdir.join("remote_fs").canonicalize_utf8()?; let storage_config = RemoteStorageConfig { max_concurrent_syncs: std::num::NonZeroUsize::new( remote_storage::DEFAULT_REMOTE_STORAGE_MAX_CONCURRENT_SYNCS, @@ -909,7 +905,7 @@ mod test { } // TODO: put this in a common location so that we can share with remote_timeline_client's tests - fn assert_remote_files(expected: &[&str], remote_path: &Path) { + fn assert_remote_files(expected: &[&str], remote_path: &Utf8Path) { let mut expected: Vec = expected.iter().map(|x| String::from(*x)).collect(); expected.sort(); @@ -926,10 +922,7 @@ mod test { unreachable!(); } } else { - panic!( - "Unexpected error listing {}: {e}", - remote_path.to_string_lossy() - ); + panic!("Unexpected error listing {remote_path}: {e}"); } } }; @@ -944,7 +937,7 @@ mod test { assert_eq!(expected, found); } - fn assert_local_files(expected: &[&str], directory: &Path) { + fn assert_local_files(expected: &[&str], directory: &Utf8Path) { let dir = match std::fs::read_dir(directory) { Ok(d) => d, Err(_) => { diff --git a/pageserver/src/deletion_queue/list_writer.rs b/pageserver/src/deletion_queue/list_writer.rs index 618a59f8fe..75fc0524b7 100644 --- a/pageserver/src/deletion_queue/list_writer.rs +++ b/pageserver/src/deletion_queue/list_writer.rs @@ -180,8 +180,7 @@ impl ListWriter { Ok(h) => Ok(Some(h.validated_sequence)), Err(e) => { warn!( - "Failed to deserialize deletion header, ignoring {}: {e:#}", - header_path.display() + "Failed to deserialize deletion header, ignoring {header_path}: {e:#}", ); // This should never happen unless we make a mistake with our serialization. // Ignoring a deletion header is not consequential for correctnes because all deletions @@ -193,10 +192,7 @@ impl ListWriter { } Err(e) => { if e.kind() == std::io::ErrorKind::NotFound { - debug!( - "Deletion header {} not found, first start?", - header_path.display() - ); + debug!("Deletion header {header_path} not found, first start?"); Ok(None) } else { Err(anyhow::anyhow!(e)) @@ -223,10 +219,7 @@ impl ListWriter { let mut dir = match tokio::fs::read_dir(&deletion_directory).await { Ok(d) => d, Err(e) => { - warn!( - "Failed to open deletion list directory {}: {e:#}", - deletion_directory.display(), - ); + warn!("Failed to open deletion list directory {deletion_directory}: {e:#}"); // Give up: if we can't read the deletion list directory, we probably can't // write lists into it later, so the queue won't work. @@ -243,21 +236,19 @@ impl ListWriter { let file_name = dentry.file_name(); let dentry_str = file_name.to_string_lossy(); - if Some(file_name.as_os_str()) == header_path.file_name() { + if file_name == header_path.file_name().unwrap_or("") { // Don't try and parse the header's name like a list continue; } if dentry_str.ends_with(TEMP_SUFFIX) { info!("Cleaning up temporary file {dentry_str}"); - let absolute_path = deletion_directory.join(dentry.file_name()); + let absolute_path = + deletion_directory.join(dentry.file_name().to_str().expect("non-Unicode path")); if let Err(e) = tokio::fs::remove_file(&absolute_path).await { // Non-fatal error: we will just leave the file behind but not // try and load it. - warn!( - "Failed to clean up temporary file {}: {e:#}", - absolute_path.display() - ); + warn!("Failed to clean up temporary file {absolute_path}: {e:#}"); } continue; @@ -360,7 +351,7 @@ impl ListWriter { if let Err(e) = create_dir_all(&self.conf.deletion_prefix()) { tracing::error!( "Failed to create deletion list directory {}, deletions will not be executed ({e})", - self.conf.deletion_prefix().display() + self.conf.deletion_prefix(), ); metrics::DELETION_QUEUE.unexpected_errors.inc(); return; diff --git a/pageserver/src/deletion_queue/validator.rs b/pageserver/src/deletion_queue/validator.rs index 64603045d2..c44564acfb 100644 --- a/pageserver/src/deletion_queue/validator.rs +++ b/pageserver/src/deletion_queue/validator.rs @@ -15,10 +15,10 @@ //! Deletions are passed onward to the Deleter. use std::collections::HashMap; -use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +use camino::Utf8PathBuf; use tokio_util::sync::CancellationToken; use tracing::debug; use tracing::info; @@ -282,16 +282,16 @@ where Ok(()) } - async fn cleanup_lists(&mut self, list_paths: Vec) { + async fn cleanup_lists(&mut self, list_paths: Vec) { for list_path in list_paths { - debug!("Removing deletion list {}", list_path.display()); + debug!("Removing deletion list {list_path}"); if let Err(e) = tokio::fs::remove_file(&list_path).await { // Unexpected: we should have permissions and nothing else should // be touching these files. We will leave the file behind. Subsequent // pageservers will try and load it again: hopefully whatever storage // issue (probably permissions) has been fixed by then. - tracing::error!("Failed to delete {}: {e:#}", list_path.display()); + tracing::error!("Failed to delete {list_path}: {e:#}"); metrics::DELETION_QUEUE.unexpected_errors.inc(); break; } diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index d5bdfc84b9..b8af4c3d11 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -43,12 +43,12 @@ use std::{ collections::HashMap, - path::Path, sync::Arc, time::{Duration, SystemTime}, }; use anyhow::Context; +use camino::Utf8Path; use remote_storage::GenericRemoteStorage; use serde::{Deserialize, Serialize}; use tokio::time::Instant; @@ -122,7 +122,7 @@ async fn disk_usage_eviction_task( state: &State, task_config: &DiskUsageEvictionTaskConfig, storage: GenericRemoteStorage, - tenants_dir: &Path, + tenants_dir: &Utf8Path, cancel: CancellationToken, ) { scopeguard::defer! { @@ -184,7 +184,7 @@ async fn disk_usage_eviction_task_iteration( state: &State, task_config: &DiskUsageEvictionTaskConfig, storage: &GenericRemoteStorage, - tenants_dir: &Path, + tenants_dir: &Utf8Path, cancel: &CancellationToken, ) -> anyhow::Result<()> { let usage_pre = filesystem_level_usage::get(tenants_dir, task_config) @@ -620,9 +620,8 @@ impl std::ops::Deref for TimelineKey { } mod filesystem_level_usage { - use std::path::Path; - use anyhow::Context; + use camino::Utf8Path; use crate::statvfs::Statvfs; @@ -664,7 +663,7 @@ mod filesystem_level_usage { } pub fn get<'a>( - tenants_dir: &Path, + tenants_dir: &Utf8Path, config: &'a DiskUsageEvictionTaskConfig, ) -> anyhow::Result> { let mock_config = { diff --git a/pageserver/src/import_datadir.rs b/pageserver/src/import_datadir.rs index 5a1affdb11..30975c1fc9 100644 --- a/pageserver/src/import_datadir.rs +++ b/pageserver/src/import_datadir.rs @@ -6,6 +6,7 @@ use std::path::{Path, PathBuf}; use anyhow::{bail, ensure, Context, Result}; use bytes::Bytes; +use camino::Utf8Path; use futures::StreamExt; use tokio::io::{AsyncRead, AsyncReadExt}; use tokio_tar::Archive; @@ -29,7 +30,7 @@ use postgres_ffi::{BLCKSZ, WAL_SEGMENT_SIZE}; use utils::lsn::Lsn; // Returns checkpoint LSN from controlfile -pub fn get_lsn_from_controlfile(path: &Path) -> Result { +pub fn get_lsn_from_controlfile(path: &Utf8Path) -> Result { // Read control file to extract the LSN let controlfile_path = path.join("global").join("pg_control"); let controlfile = ControlFileData::decode(&std::fs::read(controlfile_path)?)?; @@ -46,7 +47,7 @@ pub fn get_lsn_from_controlfile(path: &Path) -> Result { /// cluster was not shut down cleanly. pub async fn import_timeline_from_postgres_datadir( tline: &Timeline, - pgdata_path: &Path, + pgdata_path: &Utf8Path, pgdata_lsn: Lsn, ctx: &RequestContext, ) -> Result<()> { @@ -256,7 +257,7 @@ async fn import_slru( /// Scan PostgreSQL WAL files in given directory and load all records between /// 'startpoint' and 'endpoint' into the repository. async fn import_wal( - walpath: &Path, + walpath: &Utf8Path, tline: &Timeline, startpoint: Lsn, endpoint: Lsn, diff --git a/pageserver/src/lib.rs b/pageserver/src/lib.rs index e370e063ba..5f62c164bb 100644 --- a/pageserver/src/lib.rs +++ b/pageserver/src/lib.rs @@ -25,9 +25,8 @@ pub mod walredo; pub mod failpoint_support; -use std::path::Path; - use crate::task_mgr::TaskKind; +use camino::Utf8Path; use deletion_queue::DeletionQueue; use tracing::info; @@ -132,25 +131,25 @@ pub const TIMELINE_DELETE_MARK_SUFFIX: &str = "___delete"; /// Full path: `tenants//___ignored_tenant`. pub const IGNORED_TENANT_FILE_NAME: &str = "___ignored_tenant"; -pub fn is_temporary(path: &Path) -> bool { +pub fn is_temporary(path: &Utf8Path) -> bool { match path.file_name() { - Some(name) => name.to_string_lossy().ends_with(TEMP_FILE_SUFFIX), + Some(name) => name.ends_with(TEMP_FILE_SUFFIX), None => false, } } -fn ends_with_suffix(path: &Path, suffix: &str) -> bool { +fn ends_with_suffix(path: &Utf8Path, suffix: &str) -> bool { match path.file_name() { - Some(name) => name.to_string_lossy().ends_with(suffix), + Some(name) => name.ends_with(suffix), None => false, } } -pub fn is_uninit_mark(path: &Path) -> bool { +pub fn is_uninit_mark(path: &Utf8Path) -> bool { ends_with_suffix(path, TIMELINE_UNINIT_MARK_SUFFIX) } -pub fn is_delete_mark(path: &Path) -> bool { +pub fn is_delete_mark(path: &Utf8Path) -> bool { ends_with_suffix(path, TIMELINE_DELETE_MARK_SUFFIX) } diff --git a/pageserver/src/statvfs.rs b/pageserver/src/statvfs.rs index 28d950b5e6..08b5264290 100644 --- a/pageserver/src/statvfs.rs +++ b/pageserver/src/statvfs.rs @@ -1,6 +1,6 @@ //! Wrapper around nix::sys::statvfs::Statvfs that allows for mocking. -use std::path::Path; +use camino::Utf8Path; pub enum Statvfs { Real(nix::sys::statvfs::Statvfs), @@ -12,11 +12,13 @@ pub enum Statvfs { // Sincce it should only be a problem on > 2TiB disks, let's ignore // the problem for now and upcast to u64. impl Statvfs { - pub fn get(tenants_dir: &Path, mocked: Option<&mock::Behavior>) -> nix::Result { + pub fn get(tenants_dir: &Utf8Path, mocked: Option<&mock::Behavior>) -> nix::Result { if let Some(mocked) = mocked { Ok(Statvfs::Mock(mock::get(tenants_dir, mocked)?)) } else { - Ok(Statvfs::Real(nix::sys::statvfs::statvfs(tenants_dir)?)) + Ok(Statvfs::Real(nix::sys::statvfs::statvfs( + tenants_dir.as_std_path(), + )?)) } } @@ -55,8 +57,8 @@ impl Statvfs { pub mod mock { use anyhow::Context; + use camino::Utf8Path; use regex::Regex; - use std::path::Path; use tracing::log::info; #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] @@ -86,7 +88,7 @@ pub mod mock { } } - pub fn get(tenants_dir: &Path, behavior: &Behavior) -> nix::Result { + pub fn get(tenants_dir: &Utf8Path, behavior: &Behavior) -> nix::Result { info!("running mocked statvfs"); match behavior { @@ -119,7 +121,7 @@ pub mod mock { } } - fn walk_dir_disk_usage(path: &Path, name_filter: Option<&Regex>) -> anyhow::Result { + fn walk_dir_disk_usage(path: &Utf8Path, name_filter: Option<&Regex>) -> anyhow::Result { let mut total = 0; for entry in walkdir::WalkDir::new(path) { let entry = entry?; diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 47bfd4a8ef..f962549ffe 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -12,6 +12,7 @@ //! use anyhow::{bail, Context}; +use camino::{Utf8Path, Utf8PathBuf}; use futures::FutureExt; use pageserver_api::models::TimelineState; use remote_storage::DownloadError; @@ -34,8 +35,6 @@ use std::fs; use std::fs::File; use std::io; use std::ops::Bound::Included; -use std::path::Path; -use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use std::sync::atomic::AtomicU64; @@ -772,7 +771,7 @@ impl Tenant { } std::fs::remove_file(&marker_file) - .with_context(|| format!("unlink attach marker file {}", marker_file.display()))?; + .with_context(|| format!("unlink attach marker file {marker_file}"))?; crashsafe::fsync(marker_file.parent().expect("marker file has parent dir")) .context("fsync tenant directory after unlinking attach marker file")?; @@ -1024,58 +1023,47 @@ impl Tenant { let timelines_dir = self.conf.timelines_path(&self.tenant_id); - for entry in - std::fs::read_dir(&timelines_dir).context("list timelines directory for tenant")? + for entry in timelines_dir + .read_dir_utf8() + .context("list timelines directory for tenant")? { let entry = entry.context("read timeline dir entry")?; let timeline_dir = entry.path(); - if crate::is_temporary(&timeline_dir) { - info!( - "Found temporary timeline directory, removing: {}", - timeline_dir.display() - ); - if let Err(e) = std::fs::remove_dir_all(&timeline_dir) { - error!( - "Failed to remove temporary directory '{}': {:?}", - timeline_dir.display(), - e - ); + if crate::is_temporary(timeline_dir) { + info!("Found temporary timeline directory, removing: {timeline_dir}"); + if let Err(e) = std::fs::remove_dir_all(timeline_dir) { + error!("Failed to remove temporary directory '{timeline_dir}': {e:?}"); } - } else if is_uninit_mark(&timeline_dir) { + } else if is_uninit_mark(timeline_dir) { if !timeline_dir.exists() { - warn!( - "Timeline dir entry become invalid: {}", - timeline_dir.display() - ); + warn!("Timeline dir entry become invalid: {timeline_dir}"); continue; } let timeline_uninit_mark_file = &timeline_dir; info!( - "Found an uninit mark file {}, removing the timeline and its uninit mark", - timeline_uninit_mark_file.display() + "Found an uninit mark file {timeline_uninit_mark_file}, removing the timeline and its uninit mark", ); - let timeline_id = TimelineId::try_from(timeline_uninit_mark_file.file_stem()) - .with_context(|| { - format!( - "Could not parse timeline id out of the timeline uninit mark name {}", - timeline_uninit_mark_file.display() + let timeline_id = + TimelineId::try_from(timeline_uninit_mark_file.file_stem()) + .with_context(|| { + format!( + "Could not parse timeline id out of the timeline uninit mark name {timeline_uninit_mark_file}", ) - })?; + })?; let timeline_dir = self.conf.timeline_path(&self.tenant_id, &timeline_id); if let Err(e) = remove_timeline_and_uninit_mark(&timeline_dir, timeline_uninit_mark_file) { error!("Failed to clean up uninit marked timeline: {e:?}"); } - } else if crate::is_delete_mark(&timeline_dir) { + } else if crate::is_delete_mark(timeline_dir) { // If metadata exists, load as usual, continue deletion - let timeline_id = - TimelineId::try_from(timeline_dir.file_stem()).with_context(|| { + let timeline_id = TimelineId::try_from(timeline_dir.file_stem()) + .with_context(|| { format!( - "Could not parse timeline id out of the timeline uninit mark name {}", - timeline_dir.display() + "Could not parse timeline id out of the timeline uninit mark name {timeline_dir}", ) })?; @@ -1114,17 +1102,13 @@ impl Tenant { } } else { if !timeline_dir.exists() { - warn!( - "Timeline dir entry become invalid: {}", - timeline_dir.display() - ); + warn!("Timeline dir entry become invalid: {timeline_dir}"); continue; } - let timeline_id = - TimelineId::try_from(timeline_dir.file_name()).with_context(|| { + let timeline_id = TimelineId::try_from(timeline_dir.file_name()) + .with_context(|| { format!( - "Could not parse timeline id out of the timeline dir name {}", - timeline_dir.display() + "Could not parse timeline id out of the timeline dir name {timeline_dir}", ) })?; let timeline_uninit_mark_file = self @@ -1136,7 +1120,7 @@ impl Tenant { "Found an uninit mark file, removing the timeline and its uninit mark", ); if let Err(e) = - remove_timeline_and_uninit_mark(&timeline_dir, &timeline_uninit_mark_file) + remove_timeline_and_uninit_mark(timeline_dir, &timeline_uninit_mark_file) { error!("Failed to clean up uninit marked timeline: {e:?}"); } @@ -1152,18 +1136,13 @@ impl Tenant { } let file_name = entry.file_name(); - if let Ok(timeline_id) = - file_name.to_str().unwrap_or_default().parse::() - { + if let Ok(timeline_id) = file_name.parse::() { let metadata = load_metadata(self.conf, &self.tenant_id, &timeline_id) .context("failed to load metadata")?; timelines_to_load.insert(timeline_id, metadata); } else { // A file or directory that doesn't look like a timeline ID - warn!( - "unexpected file or directory in timelines directory: {}", - file_name.to_string_lossy() - ); + warn!("unexpected file or directory in timelines directory: {file_name}"); } } } @@ -2354,26 +2333,24 @@ impl Tenant { tenant_id: &TenantId, ) -> anyhow::Result { let target_config_path = conf.tenant_config_path(tenant_id); - let target_config_display = target_config_path.display(); - info!("loading tenantconf from {target_config_display}"); + info!("loading tenantconf from {target_config_path}"); // FIXME If the config file is not found, assume that we're attaching // a detached tenant and config is passed via attach command. // https://github.com/neondatabase/neon/issues/1555 // OR: we're loading after incomplete deletion that managed to remove config. if !target_config_path.exists() { - info!("tenant config not found in {target_config_display}"); + info!("tenant config not found in {target_config_path}"); return Ok(TenantConfOpt::default()); } // load and parse file - let config = fs::read_to_string(&target_config_path).with_context(|| { - format!("Failed to load config from path '{target_config_display}'") - })?; + let config = fs::read_to_string(&target_config_path) + .with_context(|| format!("Failed to load config from path '{target_config_path}'"))?; let toml = config.parse::().with_context(|| { - format!("Failed to parse config from file '{target_config_display}' as toml file") + format!("Failed to parse config from file '{target_config_path}' as toml file") })?; let mut tenant_conf = TenantConfOpt::default(); @@ -2381,11 +2358,12 @@ impl Tenant { match key { "tenant_config" => { tenant_conf = PageServerConf::parse_toml_tenant_conf(item).with_context(|| { - format!("Failed to parse config from file '{target_config_display}' as pageserver config") + format!("Failed to parse config from file '{target_config_path}' as pageserver config") })?; } - _ => bail!("config file {target_config_display} has unrecognized pageserver option '{key}'"), - + _ => bail!( + "config file {target_config_path} has unrecognized pageserver option '{key}'" + ), } } @@ -2395,11 +2373,11 @@ impl Tenant { #[tracing::instrument(skip_all, fields(%tenant_id))] pub(super) async fn persist_tenant_config( tenant_id: &TenantId, - target_config_path: &Path, + target_config_path: &Utf8Path, tenant_conf: TenantConfOpt, ) -> anyhow::Result<()> { // imitate a try-block with a closure - info!("persisting tenantconf to {}", target_config_path.display()); + info!("persisting tenantconf to {target_config_path}"); let mut conf_content = r#"# This file contains a specific per-tenant's config. # It is read in case of pageserver restart. @@ -2416,12 +2394,7 @@ impl Tenant { let temp_path = path_with_suffix_extension(target_config_path, TEMP_FILE_SUFFIX); VirtualFile::crashsafe_overwrite(target_config_path, &temp_path, conf_content) .await - .with_context(|| { - format!( - "write tenant {tenant_id} config to {}", - target_config_path.display() - ) - })?; + .with_context(|| format!("write tenant {tenant_id} config to {target_config_path}"))?; Ok(()) } @@ -2788,10 +2761,7 @@ impl Tenant { // current initdb was not run yet, so remove whatever was left from the previous runs if initdb_path.exists() { fs::remove_dir_all(&initdb_path).with_context(|| { - format!( - "Failed to remove already existing initdb directory: {}", - initdb_path.display() - ) + format!("Failed to remove already existing initdb directory: {initdb_path}") })?; } // Init temporarily repo to get bootstrap data, this creates a directory in the `initdb_path` path @@ -2800,7 +2770,7 @@ impl Tenant { scopeguard::defer! { if let Err(e) = fs::remove_dir_all(&initdb_path) { // this is unlikely, but we will remove the directory on pageserver restart or another bootstrap call - error!("Failed to remove temporary initdb directory '{}': {}", initdb_path.display(), e); + error!("Failed to remove temporary initdb directory '{initdb_path}': {e}"); } } let pgdata_path = &initdb_path; @@ -2950,7 +2920,7 @@ impl Tenant { async fn create_timeline_files( &self, - timeline_path: &Path, + timeline_path: &Utf8Path, new_timeline_id: &TimelineId, new_metadata: &TimelineMetadata, ) -> anyhow::Result<()> { @@ -2984,8 +2954,7 @@ impl Tenant { let timeline_path = self.conf.timeline_path(&tenant_id, &timeline_id); anyhow::ensure!( !timeline_path.exists(), - "Timeline {} already exists, cannot create its uninit mark file", - timeline_path.display() + "Timeline {timeline_path} already exists, cannot create its uninit mark file", ); let uninit_mark_path = self @@ -3077,7 +3046,10 @@ impl Tenant { } } -fn remove_timeline_and_uninit_mark(timeline_dir: &Path, uninit_mark: &Path) -> anyhow::Result<()> { +fn remove_timeline_and_uninit_mark( + timeline_dir: &Utf8Path, + uninit_mark: &Utf8Path, +) -> anyhow::Result<()> { fs::remove_dir_all(timeline_dir) .or_else(|e| { if e.kind() == std::io::ErrorKind::NotFound { @@ -3089,17 +3061,10 @@ fn remove_timeline_and_uninit_mark(timeline_dir: &Path, uninit_mark: &Path) -> a } }) .with_context(|| { - format!( - "Failed to remove unit marked timeline directory {}", - timeline_dir.display() - ) + format!("Failed to remove unit marked timeline directory {timeline_dir}") })?; - fs::remove_file(uninit_mark).with_context(|| { - format!( - "Failed to remove timeline uninit mark file {}", - uninit_mark.display() - ) - })?; + fs::remove_file(uninit_mark) + .with_context(|| format!("Failed to remove timeline uninit mark file {uninit_mark}"))?; Ok(()) } @@ -3114,7 +3079,7 @@ pub(crate) async fn create_tenant_files( tenant_conf: TenantConfOpt, tenant_id: &TenantId, mode: CreateTenantFilesMode, -) -> anyhow::Result { +) -> anyhow::Result { let target_tenant_directory = conf.tenant_path(tenant_id); anyhow::ensure!( !target_tenant_directory @@ -3125,17 +3090,11 @@ pub(crate) async fn create_tenant_files( let temporary_tenant_dir = path_with_suffix_extension(&target_tenant_directory, TEMP_FILE_SUFFIX); - debug!( - "Creating temporary directory structure in {}", - temporary_tenant_dir.display() - ); + debug!("Creating temporary directory structure in {temporary_tenant_dir}"); // top-level dir may exist if we are creating it through CLI crashsafe::create_dir_all(&temporary_tenant_dir).with_context(|| { - format!( - "could not create temporary tenant directory {}", - temporary_tenant_dir.display() - ) + format!("could not create temporary tenant directory {temporary_tenant_dir}") })?; let creation_result = try_create_target_tenant_dir( @@ -3169,8 +3128,8 @@ async fn try_create_target_tenant_dir( tenant_conf: TenantConfOpt, tenant_id: &TenantId, mode: CreateTenantFilesMode, - temporary_tenant_dir: &Path, - target_tenant_directory: &Path, + temporary_tenant_dir: &Utf8Path, + target_tenant_directory: &Utf8Path, ) -> Result<(), anyhow::Error> { match mode { CreateTenantFilesMode::Create => {} // needs no attach marker, writing tenant conf + atomic rename of dir is good enough @@ -3208,8 +3167,7 @@ async fn try_create_target_tenant_dir( crashsafe::create_dir(&temporary_tenant_timelines_dir).with_context(|| { format!( "create tenant {} temporary timelines directory {}", - tenant_id, - temporary_tenant_timelines_dir.display() + tenant_id, temporary_tenant_timelines_dir, ) })?; fail::fail_point!("tenant-creation-before-tmp-rename", |_| { @@ -3224,35 +3182,34 @@ async fn try_create_target_tenant_dir( fs::rename(temporary_tenant_dir, target_tenant_directory).with_context(|| { format!( "move tenant {} temporary directory {} into the permanent one {}", - tenant_id, - temporary_tenant_dir.display(), - target_tenant_directory.display() + tenant_id, temporary_tenant_dir, target_tenant_directory ) })?; let target_dir_parent = target_tenant_directory.parent().with_context(|| { format!( "get tenant {} dir parent for {}", - tenant_id, - target_tenant_directory.display() + tenant_id, target_tenant_directory, ) })?; crashsafe::fsync(target_dir_parent).with_context(|| { format!( "fsync renamed directory's parent {} for tenant {}", - target_dir_parent.display(), - tenant_id, + target_dir_parent, tenant_id, ) })?; Ok(()) } -fn rebase_directory(original_path: &Path, base: &Path, new_base: &Path) -> anyhow::Result { +fn rebase_directory( + original_path: &Utf8Path, + base: &Utf8Path, + new_base: &Utf8Path, +) -> anyhow::Result { let relative_path = original_path.strip_prefix(base).with_context(|| { format!( "Failed to strip base prefix '{}' off path '{}'", - base.display(), - original_path.display() + base, original_path ) })?; Ok(new_base.join(relative_path)) @@ -3262,20 +3219,18 @@ fn rebase_directory(original_path: &Path, base: &Path, new_base: &Path) -> anyho /// to get bootstrap data for timeline initialization. fn run_initdb( conf: &'static PageServerConf, - initdb_target_dir: &Path, + initdb_target_dir: &Utf8Path, pg_version: u32, ) -> anyhow::Result<()> { let initdb_bin_path = conf.pg_bin_dir(pg_version)?.join("initdb"); let initdb_lib_dir = conf.pg_lib_dir(pg_version)?; info!( "running {} in {}, libdir: {}", - initdb_bin_path.display(), - initdb_target_dir.display(), - initdb_lib_dir.display(), + initdb_bin_path, initdb_target_dir, initdb_lib_dir, ); let initdb_output = Command::new(&initdb_bin_path) - .args(["-D", &initdb_target_dir.to_string_lossy()]) + .args(["-D", initdb_target_dir.as_ref()]) .args(["-U", &conf.superuser]) .args(["-E", "utf8"]) .arg("--no-instructions") @@ -3290,8 +3245,7 @@ fn run_initdb( .with_context(|| { format!( "failed to execute {} at target dir {}", - initdb_bin_path.display(), - initdb_target_dir.display() + initdb_bin_path, initdb_target_dir, ) })?; if !initdb_output.status.success() { @@ -3311,7 +3265,7 @@ impl Drop for Tenant { } /// Dump contents of a layer file to stdout. pub async fn dump_layerfile_from_path( - path: &Path, + path: &Utf8Path, verbose: bool, ctx: &RequestContext, ) -> anyhow::Result<()> { @@ -3344,8 +3298,8 @@ pub async fn dump_layerfile_from_path( pub mod harness { use bytes::{Bytes, BytesMut}; use once_cell::sync::OnceCell; + use std::fs; use std::sync::Arc; - use std::{fs, path::PathBuf}; use utils::logging; use utils::lsn::Lsn; @@ -3410,7 +3364,7 @@ pub mod harness { pub tenant_id: TenantId, pub generation: Generation, pub remote_storage: GenericRemoteStorage, - pub remote_fs_dir: PathBuf, + pub remote_fs_dir: Utf8PathBuf, pub deletion_queue: MockDeletionQueue, } @@ -3509,7 +3463,7 @@ pub mod harness { Ok(tenant) } - pub fn timeline_path(&self, timeline_id: &TimelineId) -> PathBuf { + pub fn timeline_path(&self, timeline_id: &TimelineId) -> Utf8PathBuf { self.conf.timeline_path(&self.tenant_id, timeline_id) } } diff --git a/pageserver/src/tenant/blob_io.rs b/pageserver/src/tenant/blob_io.rs index 21327deb70..bedf09a40c 100644 --- a/pageserver/src/tenant/blob_io.rs +++ b/pageserver/src/tenant/blob_io.rs @@ -238,14 +238,14 @@ mod tests { use rand::{Rng, SeedableRng}; async fn round_trip_test(blobs: &[Vec]) -> Result<(), Error> { - let temp_dir = tempfile::tempdir()?; - let path = temp_dir.path().join("file"); + let temp_dir = camino_tempfile::tempdir()?; + let pathbuf = temp_dir.path().join("file"); let ctx = RequestContext::new(TaskKind::UnitTest, DownloadBehavior::Error); // Write part (in block to drop the file) let mut offsets = Vec::new(); { - let file = VirtualFile::create(&path).await?; + let file = VirtualFile::create(pathbuf.as_path()).await?; let mut wtr = BlobWriter::::new(file, 0); for blob in blobs.iter() { let offs = wtr.write_blob(blob).await?; @@ -258,7 +258,7 @@ mod tests { wtr.flush_buffer().await?; } - let file = VirtualFile::open(&path).await?; + let file = VirtualFile::open(pathbuf.as_path()).await?; let rdr = BlockReaderRef::VirtualFile(&file); let rdr = BlockCursor::new(rdr); for (idx, (blob, offset)) in blobs.iter().zip(offsets.iter()).enumerate() { diff --git a/pageserver/src/tenant/delete.rs b/pageserver/src/tenant/delete.rs index 5c23af8036..80e3f30150 100644 --- a/pageserver/src/tenant/delete.rs +++ b/pageserver/src/tenant/delete.rs @@ -1,9 +1,7 @@ -use std::{ - path::{Path, PathBuf}, - sync::Arc, -}; +use std::sync::Arc; use anyhow::Context; +use camino::{Utf8Path, Utf8PathBuf}; use pageserver_api::models::TenantState; use remote_storage::{DownloadError, GenericRemoteStorage, RemotePath}; use tokio::sync::OwnedMutexGuard; @@ -62,7 +60,7 @@ fn remote_tenant_delete_mark_path( .context("Failed to strip workdir prefix") .and_then(RemotePath::new) .context("tenant path")?; - Ok(tenant_remote_path.join(Path::new("deleted"))) + Ok(tenant_remote_path.join(Utf8Path::new("deleted"))) } async fn create_remote_delete_mark( @@ -148,7 +146,7 @@ async fn schedule_ordered_timeline_deletions( Ok(already_running_deletions) } -async fn ensure_timelines_dir_empty(timelines_path: &Path) -> Result<(), DeleteTenantError> { +async fn ensure_timelines_dir_empty(timelines_path: &Utf8Path) -> Result<(), DeleteTenantError> { // Assert timelines dir is empty. if !fs_ext::is_directory_empty(timelines_path).await? { // Display first 10 items in directory @@ -188,17 +186,14 @@ async fn cleanup_remaining_fs_traces( conf: &PageServerConf, tenant_id: &TenantId, ) -> Result<(), DeleteTenantError> { - let rm = |p: PathBuf, is_dir: bool| async move { + let rm = |p: Utf8PathBuf, is_dir: bool| async move { if is_dir { tokio::fs::remove_dir(&p).await } else { tokio::fs::remove_file(&p).await } .or_else(fs_ext::ignore_not_found) - .with_context(|| { - let to_display = p.display(); - format!("failed to delete {to_display}") - }) + .with_context(|| format!("failed to delete {p}")) }; rm(conf.tenant_config_path(tenant_id), false).await?; diff --git a/pageserver/src/tenant/ephemeral_file.rs b/pageserver/src/tenant/ephemeral_file.rs index 8785f51c06..b99be0c075 100644 --- a/pageserver/src/tenant/ephemeral_file.rs +++ b/pageserver/src/tenant/ephemeral_file.rs @@ -6,11 +6,11 @@ use crate::context::RequestContext; use crate::page_cache::{self, PAGE_SZ}; use crate::tenant::block_io::{BlockCursor, BlockLease, BlockReader}; use crate::virtual_file::VirtualFile; +use camino::Utf8PathBuf; use std::cmp::min; use std::fs::OpenOptions; use std::io::{self, ErrorKind}; use std::ops::DerefMut; -use std::path::PathBuf; use std::sync::atomic::AtomicU64; use tracing::*; use utils::id::{TenantId, TimelineId}; @@ -40,7 +40,9 @@ impl EphemeralFile { let filename = conf .timeline_path(&tenant_id, &timeline_id) - .join(PathBuf::from(format!("ephemeral-{filename_disambiguator}"))); + .join(Utf8PathBuf::from(format!( + "ephemeral-{filename_disambiguator}" + ))); let file = VirtualFile::open_with_options( &filename, @@ -80,9 +82,7 @@ impl EphemeralFile { // order path before error because error is anyhow::Error => might have many contexts format!( "ephemeral file: read immutable page #{}: {}: {:#}", - blknum, - self.file.path.display(), - e, + blknum, self.file.path, e, ), ) })? { @@ -195,7 +195,7 @@ impl EphemeralFile { "ephemeral_file: write_blob: write-back full tail blk #{}: {:#}: {}", self.blknum, e, - self.ephemeral_file.file.path.display(), + self.ephemeral_file.file.path, ), )); } @@ -258,8 +258,7 @@ impl Drop for EphemeralFile { // not found files might also be related to https://github.com/neondatabase/neon/issues/2442 error!( "could not remove ephemeral file '{}': {}", - self.file.path.display(), - e + self.file.path, e ); } } diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index ba86aefe44..3abf50cf1e 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -1,10 +1,9 @@ //! This module acts as a switchboard to access different repositories managed by this //! page server. +use camino::{Utf8Path, Utf8PathBuf}; use rand::{distributions::Alphanumeric, Rng}; use std::collections::{hash_map, HashMap}; -use std::ffi::OsStr; -use std::path::{Path, PathBuf}; use std::sync::Arc; use tokio::fs; @@ -73,12 +72,12 @@ impl TenantsMap { /// /// This is pageserver-specific, as it relies on future processes after a crash to check /// for TEMP_FILE_SUFFIX when loading things. -async fn safe_remove_tenant_dir_all(path: impl AsRef) -> std::io::Result<()> { +async fn safe_remove_tenant_dir_all(path: impl AsRef) -> std::io::Result<()> { let tmp_path = safe_rename_tenant_dir(path).await?; fs::remove_dir_all(tmp_path).await } -async fn safe_rename_tenant_dir(path: impl AsRef) -> std::io::Result { +async fn safe_rename_tenant_dir(path: impl AsRef) -> std::io::Result { let parent = path .as_ref() .parent() @@ -95,7 +94,7 @@ async fn safe_rename_tenant_dir(path: impl AsRef) -> std::io::Result() + TEMP_FILE_SUFFIX; let tmp_path = path_with_suffix_extension(&path, &rand_suffix); - fs::rename(&path, &tmp_path).await?; + fs::rename(path.as_ref(), &tmp_path).await?; fs::File::open(parent).await?.sync_all().await?; Ok(tmp_path) } @@ -146,29 +145,25 @@ pub async fn init_tenant_mgr( None }; - let mut dir_entries = fs::read_dir(&tenants_dir) - .await + let mut dir_entries = tenants_dir + .read_dir_utf8() .with_context(|| format!("Failed to list tenants dir {tenants_dir:?}"))?; let ctx = RequestContext::todo_child(TaskKind::Startup, DownloadBehavior::Warn); loop { - match dir_entries.next_entry().await { - Ok(None) => break, - Ok(Some(dir_entry)) => { - let tenant_dir_path = dir_entry.path(); + match dir_entries.next() { + None => break, + Some(Ok(dir_entry)) => { + let tenant_dir_path = dir_entry.path().to_path_buf(); if crate::is_temporary(&tenant_dir_path) { - info!( - "Found temporary tenant directory, removing: {}", - tenant_dir_path.display() - ); + info!("Found temporary tenant directory, removing: {tenant_dir_path}"); // No need to use safe_remove_tenant_dir_all because this is already // a temporary path if let Err(e) = fs::remove_dir_all(&tenant_dir_path).await { error!( "Failed to remove temporary directory '{}': {:?}", - tenant_dir_path.display(), - e + tenant_dir_path, e ); } } else { @@ -183,7 +178,7 @@ pub async fn init_tenant_mgr( if let Err(e) = fs::remove_dir(&tenant_dir_path).await { error!( "Failed to remove empty tenant directory '{}': {e:#}", - tenant_dir_path.display() + tenant_dir_path ) } continue; @@ -197,7 +192,6 @@ pub async fn init_tenant_mgr( let tenant_id = match tenant_dir_path .file_name() - .and_then(OsStr::to_str) .unwrap_or_default() .parse::() { @@ -205,7 +199,7 @@ pub async fn init_tenant_mgr( Err(_) => { warn!( "Invalid tenant path (garbage in our repo directory?): {}", - tenant_dir_path.display() + tenant_dir_path ); continue; } @@ -221,8 +215,7 @@ pub async fn init_tenant_mgr( if let Err(e) = safe_remove_tenant_dir_all(&tenant_dir_path).await { error!( "Failed to remove detached tenant directory '{}': {:?}", - tenant_dir_path.display(), - e + tenant_dir_path, e ); } continue; @@ -232,7 +225,7 @@ pub async fn init_tenant_mgr( // on local disk may activate info!( "Starting tenant {} in legacy mode, no generation", - tenant_dir_path.display() + tenant_dir_path ); Generation::none() }; @@ -256,7 +249,7 @@ pub async fn init_tenant_mgr( } } } - Err(e) => { + Some(Err(e)) => { // On error, print it, but continue with the other tenants. If we error out // here, the pageserver startup fails altogether, causing outage for *all* // tenants. That seems worse. @@ -279,7 +272,7 @@ pub async fn init_tenant_mgr( pub(crate) fn schedule_local_tenant_processing( conf: &'static PageServerConf, tenant_id: TenantId, - tenant_path: &Path, + tenant_path: &Utf8Path, generation: Generation, resources: TenantSharedResources, init_order: Option, @@ -615,7 +608,7 @@ async fn detach_tenant0( tenants: &tokio::sync::RwLock, tenant_id: TenantId, detach_ignored: bool, -) -> Result { +) -> Result { let tenant_dir_rename_operation = |tenant_id_to_clean| async move { let local_tenant_directory = conf.tenant_path(&tenant_id_to_clean); safe_rename_tenant_dir(&local_tenant_directory) diff --git a/pageserver/src/tenant/par_fsync.rs b/pageserver/src/tenant/par_fsync.rs index 705b42aff7..3b1526e910 100644 --- a/pageserver/src/tenant/par_fsync.rs +++ b/pageserver/src/tenant/par_fsync.rs @@ -1,16 +1,17 @@ use std::{ io, - path::{Path, PathBuf}, sync::atomic::{AtomicUsize, Ordering}, }; -fn fsync_path(path: &Path) -> io::Result<()> { +use camino::{Utf8Path, Utf8PathBuf}; + +fn fsync_path(path: &Utf8Path) -> io::Result<()> { // TODO use VirtualFile::fsync_all once we fully go async. let file = std::fs::File::open(path)?; file.sync_all() } -fn parallel_worker(paths: &[PathBuf], next_path_idx: &AtomicUsize) -> io::Result<()> { +fn parallel_worker(paths: &[Utf8PathBuf], next_path_idx: &AtomicUsize) -> io::Result<()> { while let Some(path) = paths.get(next_path_idx.fetch_add(1, Ordering::Relaxed)) { fsync_path(path)?; } @@ -18,7 +19,7 @@ fn parallel_worker(paths: &[PathBuf], next_path_idx: &AtomicUsize) -> io::Result Ok(()) } -fn fsync_in_thread_pool(paths: &[PathBuf]) -> io::Result<()> { +fn fsync_in_thread_pool(paths: &[Utf8PathBuf]) -> io::Result<()> { // TODO: remove this function in favor of `par_fsync_async` once we asyncify everything. /// Use at most this number of threads. @@ -47,7 +48,7 @@ fn fsync_in_thread_pool(paths: &[PathBuf]) -> io::Result<()> { } /// Parallel fsync all files. Can be used in non-async context as it is using rayon thread pool. -pub fn par_fsync(paths: &[PathBuf]) -> io::Result<()> { +pub fn par_fsync(paths: &[Utf8PathBuf]) -> io::Result<()> { if paths.len() == 1 { fsync_path(&paths[0])?; return Ok(()); @@ -58,7 +59,7 @@ pub fn par_fsync(paths: &[PathBuf]) -> io::Result<()> { /// Parallel fsync asynchronously. If number of files are less than PARALLEL_PATH_THRESHOLD, fsync is done in the current /// execution thread. Otherwise, we will spawn_blocking and run it in tokio. -pub async fn par_fsync_async(paths: &[PathBuf]) -> io::Result<()> { +pub async fn par_fsync_async(paths: &[Utf8PathBuf]) -> io::Result<()> { const MAX_CONCURRENT_FSYNC: usize = 64; let mut next = paths.iter().peekable(); let mut js = tokio::task::JoinSet::new(); diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index ee99151ef2..6fedf781d4 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -209,6 +209,7 @@ pub mod index; mod upload; use anyhow::Context; +use camino::Utf8Path; use chrono::{NaiveDateTime, Utc}; // re-export these pub use download::{is_temp_download_file, list_remote_timelines}; @@ -219,7 +220,6 @@ use utils::backoff::{ }; use std::collections::{HashMap, VecDeque}; -use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU32, Ordering}; use std::sync::{Arc, Mutex}; @@ -924,7 +924,7 @@ impl RemoteTimelineClient { ))? }); - let index_file_path = timeline_storage_path.join(Path::new(IndexPart::FILE_NAME)); + let index_file_path = timeline_storage_path.join(Utf8Path::new(IndexPart::FILE_NAME)); debug!("enqueuing index part deletion"); self.deletion_queue_client @@ -1409,7 +1409,7 @@ pub fn remote_timelines_path(tenant_id: &TenantId) -> RemotePath { } pub fn remote_timeline_path(tenant_id: &TenantId, timeline_id: &TimelineId) -> RemotePath { - remote_timelines_path(tenant_id).join(&PathBuf::from(timeline_id.to_string())) + remote_timelines_path(tenant_id).join(Utf8Path::new(&timeline_id.to_string())) } pub fn remote_layer_path( @@ -1452,14 +1452,7 @@ pub(crate) fn parse_remote_index_path(path: RemotePath) -> Option { } }; - let file_name_str = match file_name.to_str() { - Some(s) => s, - None => { - tracing::warn!("Malformed index key {:?}", path); - return None; - } - }; - match file_name_str.split_once('-') { + match file_name.split_once('-') { Some((_, gen_suffix)) => Generation::parse_suffix(gen_suffix), None => None, } @@ -1471,20 +1464,16 @@ pub(crate) fn parse_remote_index_path(path: RemotePath) -> Option { /// Errors if the path provided does not start from pageserver's workdir. pub fn remote_path( conf: &PageServerConf, - local_path: &Path, + local_path: &Utf8Path, generation: Generation, ) -> anyhow::Result { let stripped = local_path .strip_prefix(&conf.workdir) .context("Failed to strip workdir prefix")?; - let suffixed = format!( - "{0}{1}", - stripped.to_string_lossy(), - generation.get_suffix() - ); + let suffixed = format!("{0}{1}", stripped, generation.get_suffix()); - RemotePath::new(&PathBuf::from(suffixed)).with_context(|| { + RemotePath::new(Utf8Path::new(&suffixed)).with_context(|| { format!( "to resolve remote part of path {:?} for base {:?}", local_path, conf.workdir @@ -1504,7 +1493,7 @@ mod tests { DEFAULT_PG_VERSION, }; - use std::{collections::HashSet, path::Path}; + use std::collections::HashSet; use utils::lsn::Lsn; pub(super) fn dummy_contents(name: &str) -> Vec { @@ -1538,7 +1527,7 @@ mod tests { assert_eq!(avec, bvec); } - fn assert_remote_files(expected: &[&str], remote_path: &Path, generation: Generation) { + fn assert_remote_files(expected: &[&str], remote_path: &Utf8Path, generation: Generation) { let mut expected: Vec = expected .iter() .map(|x| format!("{}{}", x, generation.get_suffix())) @@ -1657,12 +1646,12 @@ mod tests { let timeline_path = harness.timeline_path(&TIMELINE_ID); - println!("workdir: {}", harness.conf.workdir.display()); + println!("workdir: {}", harness.conf.workdir); let remote_timeline_dir = harness .remote_fs_dir .join(timeline_path.strip_prefix(&harness.conf.workdir).unwrap()); - println!("remote_timeline_dir: {}", remote_timeline_dir.display()); + println!("remote_timeline_dir: {remote_timeline_dir}"); let generation = harness.generation; @@ -1909,7 +1898,7 @@ mod tests { let index_path = test_state.harness.remote_fs_dir.join( remote_index_path(&test_state.harness.tenant_id, &TIMELINE_ID, generation).get_path(), ); - eprintln!("Writing {}", index_path.display()); + eprintln!("Writing {index_path}"); std::fs::write(&index_path, index_part_bytes).unwrap(); example_index_part } diff --git a/pageserver/src/tenant/remote_timeline_client/download.rs b/pageserver/src/tenant/remote_timeline_client/download.rs index 5c173c613f..ef8d217be4 100644 --- a/pageserver/src/tenant/remote_timeline_client/download.rs +++ b/pageserver/src/tenant/remote_timeline_client/download.rs @@ -5,10 +5,10 @@ use std::collections::HashSet; use std::future::Future; -use std::path::Path; use std::time::Duration; use anyhow::{anyhow, Context}; +use camino::Utf8Path; use tokio::fs; use tokio::io::AsyncWriteExt; use tokio_util::sync::CancellationToken; @@ -74,12 +74,7 @@ pub async fn download_layer_file<'a>( // TODO: this doesn't use the cached fd for some reason? let mut destination_file = fs::File::create(&temp_file_path) .await - .with_context(|| { - format!( - "create a destination file for layer '{}'", - temp_file_path.display() - ) - }) + .with_context(|| format!("create a destination file for layer '{temp_file_path}'")) .map_err(DownloadError::Other)?; let mut download = storage .download(&remote_path) @@ -121,7 +116,7 @@ pub async fn download_layer_file<'a>( destination_file .flush() .await - .with_context(|| format!("flush source file at {}", temp_file_path.display())) + .with_context(|| format!("flush source file at {temp_file_path}")) .map_err(DownloadError::Other)?; let expected = layer_metadata.file_size(); @@ -135,12 +130,7 @@ pub async fn download_layer_file<'a>( destination_file .sync_all() .await - .with_context(|| { - format!( - "failed to fsync source file at {}", - temp_file_path.display() - ) - }) + .with_context(|| format!("failed to fsync source file at {temp_file_path}")) .map_err(DownloadError::Other)?; drop(destination_file); @@ -152,27 +142,23 @@ pub async fn download_layer_file<'a>( fs::rename(&temp_file_path, &local_path) .await - .with_context(|| format!("rename download layer file to {}", local_path.display(),)) + .with_context(|| format!("rename download layer file to {local_path}")) .map_err(DownloadError::Other)?; crashsafe::fsync_async(&local_path) .await - .with_context(|| format!("fsync layer file {}", local_path.display(),)) + .with_context(|| format!("fsync layer file {local_path}")) .map_err(DownloadError::Other)?; - tracing::debug!("download complete: {}", local_path.display()); + tracing::debug!("download complete: {local_path}"); Ok(bytes_amount) } const TEMP_DOWNLOAD_EXTENSION: &str = "temp_download"; -pub fn is_temp_download_file(path: &Path) -> bool { - let extension = path.extension().map(|pname| { - pname - .to_str() - .expect("paths passed to this function must be valid Rust strings") - }); +pub fn is_temp_download_file(path: &Utf8Path) -> bool { + let extension = path.extension(); match extension { Some(TEMP_DOWNLOAD_EXTENSION) => true, Some(_) => false, diff --git a/pageserver/src/tenant/remote_timeline_client/upload.rs b/pageserver/src/tenant/remote_timeline_client/upload.rs index c442c4f445..dcb49794d4 100644 --- a/pageserver/src/tenant/remote_timeline_client/upload.rs +++ b/pageserver/src/tenant/remote_timeline_client/upload.rs @@ -1,8 +1,9 @@ //! Helper functions to upload files to remote storage with a RemoteStorage use anyhow::{bail, Context}; +use camino::Utf8Path; use fail::fail_point; -use std::{io::ErrorKind, path::Path}; +use std::io::ErrorKind; use tokio::fs; use super::Generation; @@ -50,7 +51,7 @@ pub(super) async fn upload_index_part<'a>( pub(super) async fn upload_timeline_layer<'a>( conf: &'static PageServerConf, storage: &'a GenericRemoteStorage, - source_path: &'a Path, + source_path: &'a Utf8Path, known_metadata: &'a LayerFileMetadata, generation: Generation, ) -> anyhow::Result<()> { @@ -68,7 +69,7 @@ pub(super) async fn upload_timeline_layer<'a>( // upload. However, a nonexistent file can also be indicative of // something worse, like when a file is scheduled for upload before // it has been written to disk yet. - info!(path = %source_path.display(), "File to upload doesn't exist. Likely the file has been deleted and an upload is not required any more."); + info!(path = %source_path, "File to upload doesn't exist. Likely the file has been deleted and an upload is not required any more."); return Ok(()); } Err(e) => { @@ -93,7 +94,7 @@ pub(super) async fn upload_timeline_layer<'a>( storage .upload(source_file, fs_size, &storage_path, None) .await - .with_context(|| format!("upload layer from local path '{}'", source_path.display()))?; + .with_context(|| format!("upload layer from local path '{source_path}'"))?; Ok(()) } diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index a39e041eaf..b3aacb20d2 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -14,6 +14,7 @@ use crate::task_mgr::TaskKind; use crate::walrecord::NeonWalRecord; use anyhow::Result; use bytes::Bytes; +use camino::Utf8PathBuf; use enum_map::EnumMap; use enumset::EnumSet; use once_cell::sync::Lazy; @@ -22,7 +23,6 @@ use pageserver_api::models::{ HistoricLayerInfo, LayerResidenceEvent, LayerResidenceEventReason, LayerResidenceStatus, }; use std::ops::Range; -use std::path::PathBuf; use std::sync::{Arc, Mutex}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tracing::warn; @@ -378,7 +378,7 @@ pub trait PersistentLayer: Layer + AsLayerDesc { // Path to the layer file in the local filesystem. // `None` for `RemoteLayer`. - fn local_path(&self) -> Option; + fn local_path(&self) -> Option; /// Permanently remove this layer from disk. fn delete_resident_layer_file(&self) -> Result<()>; @@ -456,7 +456,7 @@ pub mod tests { /// config. In that case, we use the Path variant to hold the full path to the file on /// disk. enum PathOrConf { - Path(PathBuf), + Path(Utf8PathBuf), Conf(&'static PageServerConf), } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index fbc5ecc9c0..8f0f3780c2 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -41,6 +41,7 @@ use crate::virtual_file::VirtualFile; use crate::{walrecord, TEMP_FILE_SUFFIX}; use crate::{DELTA_FILE_MAGIC, STORAGE_FORMAT_VERSION}; use anyhow::{bail, ensure, Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; use pageserver_api::models::{HistoricLayerInfo, LayerAccessKind}; use rand::{distributions::Alphanumeric, Rng}; use serde::{Deserialize, Serialize}; @@ -48,7 +49,6 @@ use std::fs::{self, File}; use std::io::SeekFrom; use std::ops::Range; use std::os::unix::fs::FileExt; -use std::path::{Path, PathBuf}; use std::sync::Arc; use tokio::sync::OnceCell; use tracing::*; @@ -267,7 +267,7 @@ impl PersistentLayer for DeltaLayer { Some(self) } - fn local_path(&self) -> Option { + fn local_path(&self) -> Option { self.local_path() } @@ -374,7 +374,7 @@ impl DeltaLayer { .await } - pub(crate) fn local_path(&self) -> Option { + pub(crate) fn local_path(&self) -> Option { Some(self.path()) } @@ -409,7 +409,7 @@ impl DeltaLayer { tenant_id: &TenantId, timeline_id: &TimelineId, fname: &DeltaFileName, - ) -> PathBuf { + ) -> Utf8PathBuf { match path_or_conf { PathOrConf::Path(path) => path.clone(), PathOrConf::Conf(conf) => conf @@ -424,7 +424,7 @@ impl DeltaLayer { timeline_id: &TimelineId, key_start: Key, lsn_range: &Range, - ) -> PathBuf { + ) -> Utf8PathBuf { let rand_string: String = rand::thread_rng() .sample_iter(&Alphanumeric) .take(8) @@ -455,7 +455,7 @@ impl DeltaLayer { self.inner .get_or_try_init(|| self.load_inner(ctx)) .await - .with_context(|| format!("Failed to load delta layer {}", self.path().display())) + .with_context(|| format!("Failed to load delta layer {}", self.path())) } async fn load_inner(&self, ctx: &RequestContext) -> Result> { @@ -471,7 +471,7 @@ impl DeltaLayer { if let PathOrConf::Path(ref path) = self.path_or_conf { // not production code - let actual_filename = path.file_name().unwrap().to_str().unwrap().to_owned(); + let actual_filename = path.file_name().unwrap().to_owned(); let expected_filename = self.filename().file_name(); if actual_filename != expected_filename { @@ -510,7 +510,7 @@ impl DeltaLayer { /// Create a DeltaLayer struct representing an existing file on disk. /// /// This variant is only used for debugging purposes, by the 'pagectl' binary. - pub fn new_for_path(path: &Path, file: File) -> Result { + pub fn new_for_path(path: &Utf8Path, file: File) -> Result { let mut summary_buf = Vec::new(); summary_buf.resize(PAGE_SZ, 0); file.read_exact_at(&mut summary_buf, 0)?; @@ -538,7 +538,7 @@ impl DeltaLayer { self.desc.delta_file_name() } /// Path to the layer file in pageserver workdir. - pub fn path(&self) -> PathBuf { + pub fn path(&self) -> Utf8PathBuf { Self::path_for( &self.path_or_conf, &self.desc.tenant_id, @@ -573,7 +573,7 @@ impl DeltaLayer { /// struct DeltaLayerWriterInner { conf: &'static PageServerConf, - pub path: PathBuf, + pub path: Utf8PathBuf, timeline_id: TimelineId, tenant_id: TenantId, @@ -711,7 +711,7 @@ impl DeltaLayerWriterInner { ensure!( metadata.len() <= S3_UPLOAD_LIMIT, "Created delta layer file at {} of size {} above limit {S3_UPLOAD_LIMIT}!", - file.path.display(), + file.path, metadata.len() ); @@ -748,7 +748,7 @@ impl DeltaLayerWriterInner { ); std::fs::rename(self.path, &final_path)?; - trace!("created delta layer {}", final_path.display()); + trace!("created delta layer {final_path}"); Ok(layer) } @@ -847,13 +847,13 @@ impl Drop for DeltaLayerWriter { impl DeltaLayerInner { pub(super) async fn load( - path: &std::path::Path, + path: &Utf8Path, summary: Option

, ctx: &RequestContext, ) -> anyhow::Result { let file = VirtualFile::open(path) .await - .with_context(|| format!("Failed to open file '{}'", path.display()))?; + .with_context(|| format!("Failed to open file '{path}'"))?; let file = FileBlockReader::new(file); let summary_blk = file.read_blk(0, ctx).await?; @@ -933,15 +933,12 @@ impl DeltaLayerInner { .read_blob_into_buf(pos, &mut buf, ctx) .await .with_context(|| { - format!( - "Failed to read blob from virtual file {}", - file.file.path.display() - ) + format!("Failed to read blob from virtual file {}", file.file.path) })?; let val = Value::des(&buf).with_context(|| { format!( "Failed to deserialize file blob from virtual file {}", - file.file.path.display() + file.file.path ) })?; match val { diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index a5470a9f9d..3608cfbfda 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -37,6 +37,7 @@ use crate::virtual_file::VirtualFile; use crate::{IMAGE_FILE_MAGIC, STORAGE_FORMAT_VERSION, TEMP_FILE_SUFFIX}; use anyhow::{bail, ensure, Context, Result}; use bytes::Bytes; +use camino::{Utf8Path, Utf8PathBuf}; use hex; use pageserver_api::models::{HistoricLayerInfo, LayerAccessKind}; use rand::{distributions::Alphanumeric, Rng}; @@ -45,7 +46,6 @@ use std::fs::{self, File}; use std::io::SeekFrom; use std::ops::Range; use std::os::unix::prelude::FileExt; -use std::path::{Path, PathBuf}; use tokio::sync::OnceCell; use tracing::*; @@ -195,7 +195,7 @@ impl AsLayerDesc for ImageLayer { } impl PersistentLayer for ImageLayer { - fn local_path(&self) -> Option { + fn local_path(&self) -> Option { self.local_path() } @@ -269,10 +269,10 @@ impl ImageLayer { .get_value_reconstruct_data(key, reconstruct_state, ctx) .await // FIXME: makes no sense to dump paths - .with_context(|| format!("read {}", self.path().display())) + .with_context(|| format!("read {}", self.path())) } - pub(crate) fn local_path(&self) -> Option { + pub(crate) fn local_path(&self) -> Option { Some(self.path()) } @@ -304,7 +304,7 @@ impl ImageLayer { timeline_id: TimelineId, tenant_id: TenantId, fname: &ImageFileName, - ) -> PathBuf { + ) -> Utf8PathBuf { match path_or_conf { PathOrConf::Path(path) => path.to_path_buf(), PathOrConf::Conf(conf) => conf @@ -318,7 +318,7 @@ impl ImageLayer { timeline_id: TimelineId, tenant_id: TenantId, fname: &ImageFileName, - ) -> PathBuf { + ) -> Utf8PathBuf { let rand_string: String = rand::thread_rng() .sample_iter(&Alphanumeric) .take(8) @@ -342,7 +342,7 @@ impl ImageLayer { self.inner .get_or_try_init(|| self.load_inner(ctx)) .await - .with_context(|| format!("Failed to load image layer {}", self.path().display())) + .with_context(|| format!("Failed to load image layer {}", self.path())) } async fn load_inner(&self, ctx: &RequestContext) -> Result { @@ -359,7 +359,7 @@ impl ImageLayer { if let PathOrConf::Path(ref path) = self.path_or_conf { // not production code - let actual_filename = path.file_name().unwrap().to_str().unwrap().to_owned(); + let actual_filename = path.file_name().unwrap().to_owned(); let expected_filename = self.filename().file_name(); if actual_filename != expected_filename { @@ -399,7 +399,7 @@ impl ImageLayer { /// Create an ImageLayer struct representing an existing file on disk. /// /// This variant is only used for debugging purposes, by the 'pagectl' binary. - pub fn new_for_path(path: &Path, file: File) -> Result { + pub fn new_for_path(path: &Utf8Path, file: File) -> Result { let mut summary_buf = Vec::new(); summary_buf.resize(PAGE_SZ, 0); file.read_exact_at(&mut summary_buf, 0)?; @@ -427,7 +427,7 @@ impl ImageLayer { } /// Path to the layer file in pageserver workdir. - pub fn path(&self) -> PathBuf { + pub fn path(&self) -> Utf8PathBuf { Self::path_for( &self.path_or_conf, self.desc.timeline_id, @@ -439,14 +439,14 @@ impl ImageLayer { impl ImageLayerInner { pub(super) async fn load( - path: &std::path::Path, + path: &Utf8Path, lsn: Lsn, summary: Option, ctx: &RequestContext, ) -> anyhow::Result { let file = VirtualFile::open(path) .await - .with_context(|| format!("Failed to open file '{}'", path.display()))?; + .with_context(|| format!("Failed to open file '{}'", path))?; let file = FileBlockReader::new(file); let summary_blk = file.read_blk(0, ctx).await?; let actual_summary = Summary::des_prefix(summary_blk.as_ref())?; @@ -526,7 +526,7 @@ impl ImageLayerInner { /// struct ImageLayerWriterInner { conf: &'static PageServerConf, - path: PathBuf, + path: Utf8PathBuf, timeline_id: TimelineId, tenant_id: TenantId, key_range: Range, @@ -558,7 +558,7 @@ impl ImageLayerWriterInner { lsn, }, ); - info!("new image layer {}", path.display()); + info!("new image layer {path}"); let mut file = VirtualFile::open_with_options( &path, std::fs::OpenOptions::new().write(true).create_new(true), @@ -685,7 +685,7 @@ impl ImageLayerWriterInner { ); std::fs::rename(self.path, final_path)?; - trace!("created image layer {}", layer.path().display()); + trace!("created image layer {}", layer.path()); Ok(layer) } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 3968c16c31..cafe5f6bb6 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -8,9 +8,9 @@ use crate::tenant::remote_timeline_client::index::LayerFileMetadata; use crate::tenant::storage_layer::{Layer, ValueReconstructResult, ValueReconstructState}; use crate::tenant::timeline::layer_manager::LayerManager; use anyhow::{bail, Result}; +use camino::Utf8PathBuf; use pageserver_api::models::HistoricLayerInfo; use std::ops::Range; -use std::path::PathBuf; use std::sync::Arc; use utils::{ @@ -92,7 +92,7 @@ impl AsLayerDesc for RemoteLayer { } impl PersistentLayer for RemoteLayer { - fn local_path(&self) -> Option { + fn local_path(&self) -> Option { None } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 8a3f6df2ae..b67bb3d857 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -9,6 +9,7 @@ mod walreceiver; use anyhow::{anyhow, bail, ensure, Context, Result}; use bytes::Bytes; +use camino::{Utf8Path, Utf8PathBuf}; use fail::fail_point; use futures::StreamExt; use itertools::Itertools; @@ -29,7 +30,6 @@ use utils::id::TenantTimelineId; use std::cmp::{max, min, Ordering}; use std::collections::{BinaryHeap, HashMap, HashSet}; use std::ops::{Deref, Range}; -use std::path::{Path, PathBuf}; use std::pin::pin; use std::sync::atomic::Ordering as AtomicOrdering; use std::sync::{Arc, Mutex, RwLock, Weak}; @@ -1736,7 +1736,7 @@ impl Timeline { Discovered::Temporary(name) => (name, "temporary timeline file"), Discovered::TemporaryDownload(name) => (name, "temporary download"), }; - path.push(name); + path.push(Utf8Path::new(&name)); init::cleanup(&path, kind)?; path.pop(); } @@ -2217,10 +2217,10 @@ impl TraversalLayerExt for Arc { let timeline_id = self.layer_desc().timeline_id; match self.local_path() { Some(local_path) => { - debug_assert!(local_path.to_str().unwrap().contains(&format!("{}", timeline_id)), + debug_assert!(local_path.to_string().contains(&format!("{}", timeline_id)), "need timeline ID to uniquely identify the layer when traversal crosses ancestor boundary", ); - format!("{}", local_path.display()) + format!("{local_path}") } None => { format!("remote {}/{self}", timeline_id) @@ -3743,7 +3743,7 @@ impl Timeline { ); } } - let mut layer_paths: Vec = new_layers.iter().map(|l| l.path()).collect(); + let mut layer_paths: Vec = new_layers.iter().map(|l| l.path()).collect(); // Fsync all the layer files and directory using multiple threads to // minimize latency. @@ -3853,10 +3853,7 @@ impl Timeline { let new_delta_path = l.path(); let metadata = new_delta_path.metadata().with_context(|| { - format!( - "read file metadata for new created layer {}", - new_delta_path.display() - ) + format!("read file metadata for new created layer {new_delta_path}") })?; if let Some(remote_client) = &self.remote_client { @@ -4790,11 +4787,10 @@ fn is_send() { /// Add a suffix to a layer file's name: .{num}.old /// Uses the first available num (starts at 0) -fn rename_to_backup(path: &Path) -> anyhow::Result<()> { +fn rename_to_backup(path: &Utf8Path) -> anyhow::Result<()> { let filename = path .file_name() - .ok_or_else(|| anyhow!("Path {} don't have a file name", path.display()))? - .to_string_lossy(); + .ok_or_else(|| anyhow!("Path {path} don't have a file name"))?; let mut new_path = path.to_owned(); for i in 0u32.. { diff --git a/pageserver/src/tenant/timeline/init.rs b/pageserver/src/tenant/timeline/init.rs index 22976a514d..3902afe89a 100644 --- a/pageserver/src/tenant/timeline/init.rs +++ b/pageserver/src/tenant/timeline/init.rs @@ -12,7 +12,8 @@ use crate::{ METADATA_FILE_NAME, }; use anyhow::Context; -use std::{collections::HashMap, ffi::OsString, path::Path, str::FromStr}; +use camino::Utf8Path; +use std::{collections::HashMap, str::FromStr}; use utils::lsn::Lsn; /// Identified files in the timeline directory. @@ -20,46 +21,43 @@ pub(super) enum Discovered { /// The only one we care about Layer(LayerFileName, u64), /// Old ephmeral files from previous launches, should be removed - Ephemeral(OsString), + Ephemeral(String), /// Old temporary timeline files, unsure what these really are, should be removed - Temporary(OsString), + Temporary(String), /// Temporary on-demand download files, should be removed - TemporaryDownload(OsString), + TemporaryDownload(String), /// "metadata" file we persist locally and include in `index_part.json` Metadata, /// Backup file from previously future layers IgnoredBackup, /// Unrecognized, warn about these - Unknown(OsString), + Unknown(String), } /// Scans the timeline directory for interesting files. -pub(super) fn scan_timeline_dir(path: &Path) -> anyhow::Result> { +pub(super) fn scan_timeline_dir(path: &Utf8Path) -> anyhow::Result> { let mut ret = Vec::new(); - for direntry in std::fs::read_dir(path)? { + for direntry in path.read_dir_utf8()? { let direntry = direntry?; - let direntry_path = direntry.path(); - let file_name = direntry.file_name(); + let file_name = direntry.file_name().to_string(); - let fname = file_name.to_string_lossy(); - - let discovered = match LayerFileName::from_str(&fname) { + let discovered = match LayerFileName::from_str(&file_name) { Ok(file_name) => { let file_size = direntry.metadata()?.len(); Discovered::Layer(file_name, file_size) } Err(_) => { - if fname == METADATA_FILE_NAME { + if file_name == METADATA_FILE_NAME { Discovered::Metadata - } else if fname.ends_with(".old") { + } else if file_name.ends_with(".old") { // ignore these Discovered::IgnoredBackup - } else if remote_timeline_client::is_temp_download_file(&direntry_path) { + } else if remote_timeline_client::is_temp_download_file(direntry.path()) { Discovered::TemporaryDownload(file_name) - } else if is_ephemeral_file(&fname) { + } else if is_ephemeral_file(&file_name) { Discovered::Ephemeral(file_name) - } else if is_temporary(&direntry_path) { + } else if is_temporary(direntry.path()) { Discovered::Temporary(file_name) } else { Discovered::Unknown(file_name) @@ -162,15 +160,14 @@ pub(super) fn reconcile( .collect::>() } -pub(super) fn cleanup(path: &Path, kind: &str) -> anyhow::Result<()> { +pub(super) fn cleanup(path: &Utf8Path, kind: &str) -> anyhow::Result<()> { let file_name = path.file_name().expect("must be file path"); tracing::debug!(kind, ?file_name, "cleaning up"); - std::fs::remove_file(path) - .with_context(|| format!("failed to remove {kind} at {}", path.display())) + std::fs::remove_file(path).with_context(|| format!("failed to remove {kind} at {path}")) } pub(super) fn cleanup_local_file_for_remote( - path: &Path, + path: &Utf8Path, local: &LayerFileMetadata, remote: &LayerFileMetadata, ) -> anyhow::Result<()> { @@ -182,8 +179,7 @@ pub(super) fn cleanup_local_file_for_remote( if let Err(err) = crate::tenant::timeline::rename_to_backup(path) { assert!( path.exists(), - "we would leave the local_layer without a file if this does not hold: {}", - path.display() + "we would leave the local_layer without a file if this does not hold: {path}", ); Err(err) } else { @@ -192,7 +188,7 @@ pub(super) fn cleanup_local_file_for_remote( } pub(super) fn cleanup_future_layer( - path: &Path, + path: &Utf8Path, name: &LayerFileName, disk_consistent_lsn: Lsn, ) -> anyhow::Result<()> { diff --git a/pageserver/src/tenant/timeline/uninit.rs b/pageserver/src/tenant/timeline/uninit.rs index 5a15e86458..6b68fdeb84 100644 --- a/pageserver/src/tenant/timeline/uninit.rs +++ b/pageserver/src/tenant/timeline/uninit.rs @@ -1,6 +1,7 @@ -use std::{collections::hash_map::Entry, fs, path::PathBuf, sync::Arc}; +use std::{collections::hash_map::Entry, fs, sync::Arc}; use anyhow::Context; +use camino::Utf8PathBuf; use tracing::{error, info, info_span, warn}; use utils::{crashsafe, fs_ext, id::TimelineId, lsn::Lsn}; @@ -155,12 +156,12 @@ pub(crate) fn cleanup_timeline_directory(uninit_mark: TimelineUninitMark) { #[must_use] pub(crate) struct TimelineUninitMark { uninit_mark_deleted: bool, - uninit_mark_path: PathBuf, - pub(crate) timeline_path: PathBuf, + uninit_mark_path: Utf8PathBuf, + pub(crate) timeline_path: Utf8PathBuf, } impl TimelineUninitMark { - pub(crate) fn new(uninit_mark_path: PathBuf, timeline_path: PathBuf) -> Self { + pub(crate) fn new(uninit_mark_path: Utf8PathBuf, timeline_path: Utf8PathBuf) -> Self { Self { uninit_mark_deleted: false, uninit_mark_path, @@ -197,14 +198,13 @@ impl Drop for TimelineUninitMark { if self.timeline_path.exists() { error!( "Uninit mark {} is not removed, timeline {} stays uninitialized", - self.uninit_mark_path.display(), - self.timeline_path.display() + self.uninit_mark_path, self.timeline_path ) } else { // unblock later timeline creation attempts warn!( "Removing intermediate uninit mark file {}", - self.uninit_mark_path.display() + self.uninit_mark_path ); if let Err(e) = self.delete_mark_file_if_present() { error!("Failed to remove the uninit mark file: {e}") diff --git a/pageserver/src/tenant/upload_queue.rs b/pageserver/src/tenant/upload_queue.rs index 08b1cb8866..8150e71c95 100644 --- a/pageserver/src/tenant/upload_queue.rs +++ b/pageserver/src/tenant/upload_queue.rs @@ -253,7 +253,7 @@ impl std::fmt::Display for UploadOp { write!(f, "UploadMetadata(lsn: {})", lsn) } UploadOp::Delete(delete) => { - write!(f, "Delete({} layers)", delete.layers.len(),) + write!(f, "Delete({} layers)", delete.layers.len()) } UploadOp::Barrier(_) => write!(f, "Barrier"), } diff --git a/pageserver/src/trace.rs b/pageserver/src/trace.rs index 9e466dd9b0..18ec269198 100644 --- a/pageserver/src/trace.rs +++ b/pageserver/src/trace.rs @@ -1,8 +1,8 @@ use bytes::Bytes; +use camino::Utf8PathBuf; use std::{ fs::{create_dir_all, File}, io::{BufWriter, Write}, - path::PathBuf, }; pub struct Tracer { @@ -16,7 +16,7 @@ impl Drop for Tracer { } impl Tracer { - pub fn new(path: PathBuf) -> Self { + pub fn new(path: Utf8PathBuf) -> Self { let parent = path.parent().expect("failed to parse parent path"); create_dir_all(parent).expect("failed to create trace dir"); diff --git a/pageserver/src/virtual_file.rs b/pageserver/src/virtual_file.rs index 6a9df79e68..a2e8f30e15 100644 --- a/pageserver/src/virtual_file.rs +++ b/pageserver/src/virtual_file.rs @@ -12,11 +12,11 @@ //! use crate::metrics::{StorageIoOperation, STORAGE_IO_SIZE, STORAGE_IO_TIME_METRIC}; use crate::tenant::TENANTS_SEGMENT_NAME; +use camino::{Utf8Path, Utf8PathBuf}; use once_cell::sync::OnceCell; use std::fs::{self, File, OpenOptions}; use std::io::{Error, ErrorKind, Seek, SeekFrom}; use std::os::unix::fs::FileExt; -use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::{RwLock, RwLockWriteGuard}; @@ -51,7 +51,7 @@ pub struct VirtualFile { /// if a new file is created, we only pass the create flag when it's initially /// opened, in the VirtualFile::create() function, and strip the flag before /// storing it here. - pub path: PathBuf, + pub path: Utf8PathBuf, open_options: OpenOptions, // These are strings becase we only use them for metrics, and those expect strings. @@ -210,13 +210,13 @@ impl CrashsafeOverwriteError { impl VirtualFile { /// Open a file in read-only mode. Like File::open. - pub async fn open(path: &Path) -> Result { + pub async fn open(path: &Utf8Path) -> Result { Self::open_with_options(path, OpenOptions::new().read(true)).await } /// Create a new file for writing. If the file exists, it will be truncated. /// Like File::create. - pub async fn create(path: &Path) -> Result { + pub async fn create(path: &Utf8Path) -> Result { Self::open_with_options( path, OpenOptions::new().write(true).create(true).truncate(true), @@ -230,10 +230,10 @@ impl VirtualFile { /// they will be applied also when the file is subsequently re-opened, not only /// on the first time. Make sure that's sane! pub async fn open_with_options( - path: &Path, + path: &Utf8Path, open_options: &OpenOptions, ) -> Result { - let path_str = path.to_string_lossy(); + let path_str = path.to_string(); let parts = path_str.split('/').collect::>(); let tenant_id; let timeline_id; @@ -281,8 +281,8 @@ impl VirtualFile { /// atomic, a crash during the write operation will never leave behind a /// partially written file. pub async fn crashsafe_overwrite( - final_path: &Path, - tmp_path: &Path, + final_path: &Utf8Path, + tmp_path: &Utf8Path, content: &[u8], ) -> Result<(), CrashsafeOverwriteError> { let Some(final_path_parent) = final_path.parent() else { @@ -734,7 +734,7 @@ mod tests { async fn test_files(testname: &str, openfunc: OF) -> Result<(), Error> where - OF: Fn(PathBuf, OpenOptions) -> FT, + OF: Fn(Utf8PathBuf, OpenOptions) -> FT, FT: Future>, { let testdir = crate::config::PageServerConf::test_repo_dir(testname); diff --git a/pageserver/src/walredo.rs b/pageserver/src/walredo.rs index 507b9cbac2..370bf6242d 100644 --- a/pageserver/src/walredo.rs +++ b/pageserver/src/walredo.rs @@ -1278,13 +1278,13 @@ mod tests { struct RedoHarness { // underscored because unused, except for removal at drop - _repo_dir: tempfile::TempDir, + _repo_dir: camino_tempfile::Utf8TempDir, manager: PostgresRedoManager, } impl RedoHarness { fn new() -> anyhow::Result { - let repo_dir = tempfile::tempdir()?; + let repo_dir = camino_tempfile::tempdir()?; let conf = PageServerConf::dummy_conf(repo_dir.path().to_path_buf()); let conf = Box::leak(Box::new(conf)); let tenant_id = TenantId::generate(); diff --git a/safekeeper/Cargo.toml b/safekeeper/Cargo.toml index 393570df6a..64ef9f6997 100644 --- a/safekeeper/Cargo.toml +++ b/safekeeper/Cargo.toml @@ -10,6 +10,8 @@ anyhow.workspace = true async-trait.workspace = true byteorder.workspace = true bytes.workspace = true +camino.workspace = true +camino-tempfile.workspace = true chrono.workspace = true clap = { workspace = true, features = ["derive"] } const_format.workspace = true @@ -36,7 +38,6 @@ tokio = { workspace = true, features = ["fs"] } tokio-io-timeout.workspace = true tokio-postgres.workspace = true toml_edit.workspace = true -tempfile.workspace = true tracing.workspace = true url.workspace = true metrics.workspace = true diff --git a/safekeeper/src/bin/safekeeper.rs b/safekeeper/src/bin/safekeeper.rs index 848b1d7644..763dcd9eb8 100644 --- a/safekeeper/src/bin/safekeeper.rs +++ b/safekeeper/src/bin/safekeeper.rs @@ -2,6 +2,7 @@ // Main entry point for the safekeeper executable // use anyhow::{bail, Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; use clap::Parser; use futures::future::BoxFuture; use futures::stream::FuturesUnordered; @@ -14,7 +15,6 @@ use toml_edit::Document; use std::fs::{self, File}; use std::io::{ErrorKind, Write}; -use std::path::{Path, PathBuf}; use std::str::FromStr; use std::sync::Arc; use std::time::Duration; @@ -63,7 +63,7 @@ split), and serving the hardened part further downstream to pageserver(s). struct Args { /// Path to the safekeeper data directory. #[arg(short = 'D', long, default_value = "./")] - datadir: PathBuf, + datadir: Utf8PathBuf, /// Safekeeper node id. #[arg(long)] id: Option, @@ -92,7 +92,7 @@ struct Args { no_sync: bool, /// Dump control file at path specified by this argument and exit. #[arg(long)] - dump_control_file: Option, + dump_control_file: Option, /// Broker endpoint for storage nodes coordination in the form /// http[s]://host:port. In case of https schema TLS is connection is /// established; plaintext otherwise. @@ -128,19 +128,19 @@ struct Args { /// validations of JWT tokens. Empty string is allowed and means disabling /// auth. #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] - pg_auth_public_key_path: Option, + pg_auth_public_key_path: Option, /// If given, enables auth on incoming connections to tenant only WAL /// service endpoint (--listen-pg-tenant-only). Value specifies path to a /// .pem public key used for validations of JWT tokens. Empty string is /// allowed and means disabling auth. #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] - pg_tenant_only_auth_public_key_path: Option, + pg_tenant_only_auth_public_key_path: Option, /// If given, enables auth on incoming connections to http management /// service endpoint (--listen-http). Value specifies path to a .pem public /// key used for validations of JWT tokens. Empty string is allowed and /// means disabling auth. #[arg(long, verbatim_doc_comment, value_parser = opt_pathbuf_parser)] - http_auth_public_key_path: Option, + http_auth_public_key_path: Option, /// Format for logging, either 'plain' or 'json'. #[arg(long, default_value = "plain")] log_format: String, @@ -151,8 +151,8 @@ struct Args { } // Like PathBufValueParser, but allows empty string. -fn opt_pathbuf_parser(s: &str) -> Result { - Ok(PathBuf::from_str(s).unwrap()) +fn opt_pathbuf_parser(s: &str) -> Result { + Ok(Utf8PathBuf::from_str(s).unwrap()) } #[tokio::main(flavor = "current_thread")] @@ -203,7 +203,7 @@ async fn main() -> anyhow::Result<()> { info!("version: {GIT_VERSION}"); let args_workdir = &args.datadir; - let workdir = args_workdir.canonicalize().with_context(|| { + let workdir = args_workdir.canonicalize_utf8().with_context(|| { format!("Failed to get the absolute path for input workdir {args_workdir:?}") })?; @@ -222,7 +222,7 @@ async fn main() -> anyhow::Result<()> { None } Some(path) => { - info!("loading pg auth JWT key from {}", path.display()); + info!("loading pg auth JWT key from {path}"); Some(Arc::new( JwtAuth::from_key_path(path).context("failed to load the auth key")?, )) @@ -234,10 +234,7 @@ async fn main() -> anyhow::Result<()> { None } Some(path) => { - info!( - "loading pg tenant only auth JWT key from {}", - path.display() - ); + info!("loading pg tenant only auth JWT key from {path}"); Some(Arc::new( JwtAuth::from_key_path(path).context("failed to load the auth key")?, )) @@ -249,7 +246,7 @@ async fn main() -> anyhow::Result<()> { None } Some(path) => { - info!("loading http auth JWT key from {}", path.display()); + info!("loading http auth JWT key from {path}"); Some(Arc::new( JwtAuth::from_key_path(path).context("failed to load the auth key")?, )) @@ -447,7 +444,7 @@ async fn start_safekeeper(conf: SafeKeeperConf) -> Result<()> { } /// Determine safekeeper id. -fn set_id(workdir: &Path, given_id: Option) -> Result { +fn set_id(workdir: &Utf8Path, given_id: Option) -> Result { let id_file_path = workdir.join(ID_FILE_NAME); let my_id: NodeId; diff --git a/safekeeper/src/control_file.rs b/safekeeper/src/control_file.rs index 504c2d355d..7aadd67ac6 100644 --- a/safekeeper/src/control_file.rs +++ b/safekeeper/src/control_file.rs @@ -2,12 +2,13 @@ use anyhow::{bail, ensure, Context, Result}; use byteorder::{LittleEndian, ReadBytesExt, WriteBytesExt}; +use camino::Utf8PathBuf; use tokio::fs::{self, File}; use tokio::io::AsyncWriteExt; use std::io::Read; use std::ops::Deref; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::time::Instant; use crate::control_file_upgrade::upgrade_control_file; @@ -39,7 +40,7 @@ pub trait Storage: Deref { #[derive(Debug)] pub struct FileStorage { // save timeline dir to avoid reconstructing it every time - timeline_dir: PathBuf, + timeline_dir: Utf8PathBuf, conf: SafeKeeperConf, /// Last state persisted to disk. @@ -174,7 +175,7 @@ impl Storage for FileStorage { let mut control_partial = File::create(&control_partial_path).await.with_context(|| { format!( "failed to create partial control file at: {}", - &control_partial_path.display() + &control_partial_path ) })?; let mut buf: Vec = Vec::new(); @@ -189,13 +190,13 @@ impl Storage for FileStorage { control_partial.write_all(&buf).await.with_context(|| { format!( "failed to write safekeeper state into control file at: {}", - control_partial_path.display() + control_partial_path ) })?; control_partial.flush().await.with_context(|| { format!( "failed to flush safekeeper state into control file at: {}", - control_partial_path.display() + control_partial_path ) })?; @@ -204,7 +205,7 @@ impl Storage for FileStorage { control_partial.sync_all().await.with_context(|| { format!( "failed to sync partial control file at {}", - control_partial_path.display() + control_partial_path ) })?; } @@ -216,12 +217,10 @@ impl Storage for FileStorage { // this sync is not required by any standard but postgres does this (see durable_rename) if !self.conf.no_sync { let new_f = File::open(&control_path).await?; - new_f.sync_all().await.with_context(|| { - format!( - "failed to sync control file at: {}", - &control_path.display() - ) - })?; + new_f + .sync_all() + .await + .with_context(|| format!("failed to sync control file at: {}", &control_path))?; // fsync the directory (linux specific) let tli_dir = File::open(&self.timeline_dir).await?; @@ -250,7 +249,7 @@ mod test { use utils::{id::TenantTimelineId, lsn::Lsn}; fn stub_conf() -> SafeKeeperConf { - let workdir = tempfile::tempdir().unwrap().into_path(); + let workdir = camino_tempfile::tempdir().unwrap().into_path(); SafeKeeperConf { workdir, ..SafeKeeperConf::dummy() diff --git a/safekeeper/src/debug_dump.rs b/safekeeper/src/debug_dump.rs index 387b577a13..ee9d7118c6 100644 --- a/safekeeper/src/debug_dump.rs +++ b/safekeeper/src/debug_dump.rs @@ -7,6 +7,7 @@ use std::io::Read; use std::path::PathBuf; use anyhow::Result; +use camino::Utf8Path; use chrono::{DateTime, Utc}; use postgres_ffi::XLogSegNo; use serde::Deserialize; @@ -201,7 +202,7 @@ pub async fn build(args: Args) -> Result { /// Builds DiskContent from a directory path. It can fail if the directory /// is deleted between the time we get the path and the time we try to open it. -fn build_disk_content(path: &std::path::Path) -> Result { +fn build_disk_content(path: &Utf8Path) -> Result { let mut files = Vec::new(); for entry in fs::read_dir(path)? { if entry.is_err() { @@ -256,7 +257,7 @@ fn build_file_info(entry: DirEntry) -> Result { fn build_config(config: SafeKeeperConf) -> Config { Config { id: config.my_id, - workdir: config.workdir, + workdir: config.workdir.into(), listen_pg_addr: config.listen_pg_addr, listen_http_addr: config.listen_http_addr, no_sync: config.no_sync, diff --git a/safekeeper/src/lib.rs b/safekeeper/src/lib.rs index d785d0e53a..00aa405047 100644 --- a/safekeeper/src/lib.rs +++ b/safekeeper/src/lib.rs @@ -1,8 +1,8 @@ +use camino::Utf8PathBuf; use once_cell::sync::Lazy; use remote_storage::RemoteStorageConfig; use tokio::runtime::Runtime; -use std::path::PathBuf; use std::time::Duration; use storage_broker::Uri; @@ -51,7 +51,7 @@ pub struct SafeKeeperConf { // that during unit testing, because the current directory is global // to the process but different unit tests work on different // data directories to avoid clashing with each other. - pub workdir: PathBuf, + pub workdir: Utf8PathBuf, pub my_id: NodeId, pub listen_pg_addr: String, pub listen_pg_addr_tenant_only: Option, @@ -73,11 +73,11 @@ pub struct SafeKeeperConf { } impl SafeKeeperConf { - pub fn tenant_dir(&self, tenant_id: &TenantId) -> PathBuf { + pub fn tenant_dir(&self, tenant_id: &TenantId) -> Utf8PathBuf { self.workdir.join(tenant_id.to_string()) } - pub fn timeline_dir(&self, ttid: &TenantTimelineId) -> PathBuf { + pub fn timeline_dir(&self, ttid: &TenantTimelineId) -> Utf8PathBuf { self.tenant_dir(&ttid.tenant_id) .join(ttid.timeline_id.to_string()) } @@ -87,7 +87,7 @@ impl SafeKeeperConf { #[cfg(test)] fn dummy() -> Self { SafeKeeperConf { - workdir: PathBuf::from("./"), + workdir: Utf8PathBuf::from("./"), no_sync: false, listen_pg_addr: defaults::DEFAULT_PG_LISTEN_ADDR.to_string(), listen_pg_addr_tenant_only: None, diff --git a/safekeeper/src/pull_timeline.rs b/safekeeper/src/pull_timeline.rs index a2ed4c0cf4..1343bba5cc 100644 --- a/safekeeper/src/pull_timeline.rs +++ b/safekeeper/src/pull_timeline.rs @@ -167,11 +167,11 @@ async fn pull_timeline(status: TimelineStatus, host: String) -> Result tokio::fs::create_dir_all(&temp_base).await?; - let tli_dir = tempfile::Builder::new() + let tli_dir = camino_tempfile::Builder::new() .suffix("_temptli") .prefix(&format!("{}_{}_", ttid.tenant_id, ttid.timeline_id)) .tempdir_in(temp_base)?; - let tli_dir_path = tli_dir.path().to_owned(); + let tli_dir_path = tli_dir.path().to_path_buf(); // Note: some time happens between fetching list of files and fetching files themselves. // It's possible that some files will be removed from safekeeper and we will fail to fetch them. @@ -220,9 +220,7 @@ async fn pull_timeline(status: TimelineStatus, host: String) -> Result info!( "Moving timeline {} from {} to {}", - ttid, - tli_dir_path.display(), - timeline_path.display() + ttid, tli_dir_path, timeline_path ); tokio::fs::create_dir_all(conf.tenant_dir(&ttid.tenant_id)).await?; tokio::fs::rename(tli_dir_path, &timeline_path).await?; diff --git a/safekeeper/src/safekeeper.rs b/safekeeper/src/safekeeper.rs index b9fcd2c0b2..85e556aff2 100644 --- a/safekeeper/src/safekeeper.rs +++ b/safekeeper/src/safekeeper.rs @@ -456,7 +456,7 @@ impl ProposerAcceptorMessage { Ok(ProposerAcceptorMessage::AppendRequest(msg)) } - _ => bail!("unknown proposer-acceptor message tag: {}", tag,), + _ => bail!("unknown proposer-acceptor message tag: {}", tag), } } } diff --git a/safekeeper/src/timeline.rs b/safekeeper/src/timeline.rs index 3e066de34f..319e4047ee 100644 --- a/safekeeper/src/timeline.rs +++ b/safekeeper/src/timeline.rs @@ -2,6 +2,7 @@ //! to glue together SafeKeeper and all other background services. use anyhow::{anyhow, bail, Result}; +use camino::Utf8PathBuf; use postgres_ffi::XLogSegNo; use serde::{Deserialize, Serialize}; use serde_with::serde_as; @@ -9,7 +10,6 @@ use tokio::fs; use serde_with::DisplayFromStr; use std::cmp::max; -use std::path::PathBuf; use std::sync::Arc; use tokio::sync::{Mutex, MutexGuard}; use tokio::{ @@ -331,7 +331,7 @@ pub struct Timeline { cancellation_rx: watch::Receiver, /// Directory where timeline state is stored. - pub timeline_dir: PathBuf, + pub timeline_dir: Utf8PathBuf, } impl Timeline { @@ -805,7 +805,7 @@ impl Timeline { } /// Deletes directory and it's contents. Returns false if directory does not exist. -async fn delete_dir(path: &PathBuf) -> Result { +async fn delete_dir(path: &Utf8PathBuf) -> Result { match fs::remove_dir_all(path).await { Ok(_) => Ok(true), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), diff --git a/safekeeper/src/timelines_global_map.rs b/safekeeper/src/timelines_global_map.rs index 2f591655a9..1434644123 100644 --- a/safekeeper/src/timelines_global_map.rs +++ b/safekeeper/src/timelines_global_map.rs @@ -6,10 +6,10 @@ use crate::safekeeper::ServerInfo; use crate::timeline::{Timeline, TimelineError}; use crate::SafeKeeperConf; use anyhow::{bail, Context, Result}; +use camino::Utf8PathBuf; use once_cell::sync::Lazy; use serde::Serialize; use std::collections::HashMap; -use std::path::PathBuf; use std::str::FromStr; use std::sync::{Arc, Mutex}; use tokio::sync::mpsc::Sender; @@ -89,7 +89,7 @@ impl GlobalTimelines { }; let mut tenant_count = 0; for tenants_dir_entry in std::fs::read_dir(&tenants_dir) - .with_context(|| format!("failed to list tenants dir {}", tenants_dir.display()))? + .with_context(|| format!("failed to list tenants dir {}", tenants_dir))? { match &tenants_dir_entry { Ok(tenants_dir_entry) => { @@ -102,9 +102,7 @@ impl GlobalTimelines { } Err(e) => error!( "failed to list tenants dir entry {:?} in directory {}, reason: {:?}", - tenants_dir_entry, - tenants_dir.display(), - e + tenants_dir_entry, tenants_dir, e ), } } @@ -136,7 +134,7 @@ impl GlobalTimelines { let timelines_dir = conf.tenant_dir(&tenant_id); for timelines_dir_entry in std::fs::read_dir(&timelines_dir) - .with_context(|| format!("failed to list timelines dir {}", timelines_dir.display()))? + .with_context(|| format!("failed to list timelines dir {}", timelines_dir))? { match &timelines_dir_entry { Ok(timeline_dir_entry) => { @@ -168,9 +166,7 @@ impl GlobalTimelines { } Err(e) => error!( "failed to list timelines dir entry {:?} in directory {}, reason: {:?}", - timelines_dir_entry, - timelines_dir.display(), - e + timelines_dir_entry, timelines_dir, e ), } } @@ -421,7 +417,7 @@ pub struct TimelineDeleteForceResult { } /// Deletes directory and it's contents. Returns false if directory does not exist. -fn delete_dir(path: PathBuf) -> Result { +fn delete_dir(path: Utf8PathBuf) -> Result { match std::fs::remove_dir_all(path) { Ok(_) => Ok(true), Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), diff --git a/safekeeper/src/wal_backup.rs b/safekeeper/src/wal_backup.rs index eae3f3fe86..da8c197411 100644 --- a/safekeeper/src/wal_backup.rs +++ b/safekeeper/src/wal_backup.rs @@ -1,5 +1,6 @@ use anyhow::{Context, Result}; +use camino::{Utf8Path, Utf8PathBuf}; use futures::stream::FuturesOrdered; use futures::StreamExt; use tokio::task::JoinHandle; @@ -7,7 +8,6 @@ use utils::id::NodeId; use std::cmp::min; use std::collections::HashMap; -use std::path::{Path, PathBuf}; use std::pin::Pin; use std::sync::Arc; use std::time::Duration; @@ -230,8 +230,8 @@ pub async fn wal_backup_launcher_task_main( struct WalBackupTask { timeline: Arc, - timeline_dir: PathBuf, - workspace_dir: PathBuf, + timeline_dir: Utf8PathBuf, + workspace_dir: Utf8PathBuf, wal_seg_size: usize, parallel_jobs: usize, commit_lsn_watch_rx: watch::Receiver, @@ -240,8 +240,8 @@ struct WalBackupTask { /// Offload single timeline. async fn backup_task_main( ttid: TenantTimelineId, - timeline_dir: PathBuf, - workspace_dir: PathBuf, + timeline_dir: Utf8PathBuf, + workspace_dir: Utf8PathBuf, parallel_jobs: usize, mut shutdown_rx: Receiver<()>, ) { @@ -351,8 +351,8 @@ pub async fn backup_lsn_range( backup_lsn: &mut Lsn, end_lsn: Lsn, wal_seg_size: usize, - timeline_dir: &Path, - workspace_dir: &Path, + timeline_dir: &Utf8Path, + workspace_dir: &Utf8Path, parallel_jobs: usize, ) -> Result<()> { if parallel_jobs < 1 { @@ -408,8 +408,8 @@ pub async fn backup_lsn_range( async fn backup_single_segment( seg: &Segment, - timeline_dir: &Path, - workspace_dir: &Path, + timeline_dir: &Utf8Path, + workspace_dir: &Utf8Path, ) -> Result { let segment_file_path = seg.file_path(timeline_dir)?; let remote_segment_path = segment_file_path @@ -429,7 +429,7 @@ async fn backup_single_segment( BACKUP_ERRORS.inc(); } res?; - debug!("Backup of {} done", segment_file_path.display()); + debug!("Backup of {} done", segment_file_path); Ok(*seg) } @@ -454,7 +454,7 @@ impl Segment { XLogFileName(PG_TLI, self.seg_no, self.size()) } - pub fn file_path(self, timeline_dir: &Path) -> Result { + pub fn file_path(self, timeline_dir: &Utf8Path) -> Result { Ok(timeline_dir.join(self.object_name())) } @@ -479,19 +479,22 @@ fn get_segments(start: Lsn, end: Lsn, seg_size: usize) -> Vec { static REMOTE_STORAGE: OnceCell> = OnceCell::new(); -async fn backup_object(source_file: &Path, target_file: &RemotePath, size: usize) -> Result<()> { +async fn backup_object( + source_file: &Utf8Path, + target_file: &RemotePath, + size: usize, +) -> Result<()> { let storage = REMOTE_STORAGE .get() .expect("failed to get remote storage") .as_ref() .unwrap(); - let file = tokio::io::BufReader::new(File::open(&source_file).await.with_context(|| { - format!( - "Failed to open file {} for wal backup", - source_file.display() - ) - })?); + let file = tokio::io::BufReader::new( + File::open(&source_file) + .await + .with_context(|| format!("Failed to open file {} for wal backup", source_file))?, + ); storage .upload_storage_object(Box::new(file), size, target_file) diff --git a/safekeeper/src/wal_storage.rs b/safekeeper/src/wal_storage.rs index 4ee66ddc8e..2070122e8e 100644 --- a/safekeeper/src/wal_storage.rs +++ b/safekeeper/src/wal_storage.rs @@ -9,13 +9,13 @@ use anyhow::{bail, Context, Result}; use bytes::Bytes; +use camino::{Utf8Path, Utf8PathBuf}; use futures::future::BoxFuture; use postgres_ffi::v14::xlog_utils::{IsPartialXLogFileName, IsXLogFileName, XLogFromFileName}; use postgres_ffi::{dispatch_pgversion, XLogSegNo, PG_TLI}; use remote_storage::RemotePath; use std::cmp::{max, min}; use std::io::{self, SeekFrom}; -use std::path::{Path, PathBuf}; use std::pin::Pin; use tokio::fs::{self, remove_file, File, OpenOptions}; use tokio::io::{AsyncRead, AsyncWriteExt}; @@ -72,7 +72,7 @@ pub trait Storage { /// When storage is created first time, all LSNs are zeroes and there are no segments on disk. pub struct PhysicalStorage { metrics: WalStorageMetrics, - timeline_dir: PathBuf, + timeline_dir: Utf8PathBuf, conf: SafeKeeperConf, /// Size of WAL segment in bytes. @@ -123,7 +123,7 @@ impl PhysicalStorage { /// the disk. Otherwise, all LSNs are set to zero. pub fn new( ttid: &TenantTimelineId, - timeline_dir: PathBuf, + timeline_dir: Utf8PathBuf, conf: &SafeKeeperConf, state: &SafeKeeperState, ) -> Result { @@ -142,7 +142,11 @@ impl PhysicalStorage { dispatch_pgversion!( version, - pgv::xlog_utils::find_end_of_wal(&timeline_dir, wal_seg_size, state.commit_lsn,)?, + pgv::xlog_utils::find_end_of_wal( + timeline_dir.as_std_path(), + wal_seg_size, + state.commit_lsn, + )?, bail!("unsupported postgres version: {}", version) ) }; @@ -458,7 +462,7 @@ impl Storage for PhysicalStorage { /// Remove all WAL segments in timeline_dir that match the given predicate. async fn remove_segments_from_disk( - timeline_dir: &Path, + timeline_dir: &Utf8Path, wal_seg_size: usize, remove_predicate: impl Fn(XLogSegNo) -> bool, ) -> Result<()> { @@ -497,8 +501,8 @@ async fn remove_segments_from_disk( } pub struct WalReader { - workdir: PathBuf, - timeline_dir: PathBuf, + workdir: Utf8PathBuf, + timeline_dir: Utf8PathBuf, wal_seg_size: usize, pos: Lsn, wal_segment: Option>>, @@ -519,8 +523,8 @@ pub struct WalReader { impl WalReader { pub fn new( - workdir: PathBuf, - timeline_dir: PathBuf, + workdir: Utf8PathBuf, + timeline_dir: Utf8PathBuf, state: &SafeKeeperState, start_pos: Lsn, enable_remote_read: bool, @@ -687,7 +691,7 @@ impl WalReader { } /// Helper function for opening a wal file. - async fn open_wal_file(wal_file_path: &Path) -> Result { + async fn open_wal_file(wal_file_path: &Utf8Path) -> Result { // First try to open the .partial file. let mut partial_path = wal_file_path.to_owned(); partial_path.set_extension("partial"); @@ -722,10 +726,10 @@ async fn write_zeroes(file: &mut File, mut count: usize) -> Result<()> { /// Helper returning full path to WAL segment file and its .partial brother. fn wal_file_paths( - timeline_dir: &Path, + timeline_dir: &Utf8Path, segno: XLogSegNo, wal_seg_size: usize, -) -> Result<(PathBuf, PathBuf)> { +) -> Result<(Utf8PathBuf, Utf8PathBuf)> { let wal_file_name = XLogFileName(PG_TLI, segno, wal_seg_size); let wal_file_path = timeline_dir.join(wal_file_name.clone()); let wal_file_partial_path = timeline_dir.join(wal_file_name + ".partial");