diff --git a/Cargo.lock b/Cargo.lock index f93136edc7..92443d0bc0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1341,6 +1341,7 @@ dependencies = [ "log", "rand", "regex", + "thiserror", ] [[package]] diff --git a/pageserver/src/basebackup.rs b/pageserver/src/basebackup.rs index 196f824a63..3b626f91df 100644 --- a/pageserver/src/basebackup.rs +++ b/pageserver/src/basebackup.rs @@ -126,15 +126,11 @@ fn parse_rel_file_path(path: &str) -> Result<(), FilePathError> { Ok(()) } else if let Some(dbpath) = path.strip_prefix("base/") { let mut s = dbpath.split('/'); - let dbnode_str = s - .next() - .ok_or_else(|| FilePathError::new("invalid relation data file name"))?; - let _dbnode = u32::from_str_radix(dbnode_str, 10)?; - let fname = s - .next() - .ok_or_else(|| FilePathError::new("invalid relation data file name"))?; + let dbnode_str = s.next().ok_or(FilePathError::InvalidFileName)?; + let _dbnode = dbnode_str.parse::()?; + let fname = s.next().ok_or(FilePathError::InvalidFileName)?; if s.next().is_some() { - return Err(FilePathError::new("invalid relation data file name")); + return Err(FilePathError::InvalidFileName); }; let (_relnode, _forknum, _segno) = parse_relfilename(fname)?; @@ -142,9 +138,10 @@ fn parse_rel_file_path(path: &str) -> Result<(), FilePathError> { Ok(()) } else if let Some(_) = path.strip_prefix("pg_tblspc/") { // TODO - Err(FilePathError::new("tablespaces not supported")) + error!("tablespaces not implemented yet"); + Err(FilePathError::InvalidFileName) } else { - Err(FilePathError::new("invalid relation data file name")) + Err(FilePathError::InvalidFileName) } } diff --git a/pageserver/src/restore_local_repo.rs b/pageserver/src/restore_local_repo.rs index 39fa610350..c0978adb20 100644 --- a/pageserver/src/restore_local_repo.rs +++ b/pageserver/src/restore_local_repo.rs @@ -140,7 +140,7 @@ fn restore_snapshot( for direntry in fs::read_dir(snapshotpath.join("base"))? { let direntry = direntry?; - let dboid = u32::from_str_radix(direntry.file_name().to_str().unwrap(), 10)?; + let dboid = direntry.file_name().to_str().unwrap().parse::()?; for direntry in fs::read_dir(direntry.path())? { let direntry = direntry?; diff --git a/postgres_ffi/Cargo.toml b/postgres_ffi/Cargo.toml index ca193ee02a..6f1a03d437 100644 --- a/postgres_ffi/Cargo.toml +++ b/postgres_ffi/Cargo.toml @@ -16,6 +16,7 @@ anyhow = "1.0" crc32c = "0.6.0" hex = "0.4.3" log = "0.4.14" +thiserror = "1.0" [build-dependencies] bindgen = "0.57" diff --git a/postgres_ffi/src/relfile_utils.rs b/postgres_ffi/src/relfile_utils.rs index 94ee3e74a5..fdfee8108b 100644 --- a/postgres_ffi/src/relfile_utils.rs +++ b/postgres_ffi/src/relfile_utils.rs @@ -2,38 +2,18 @@ /// Common utilities for dealing with PostgreSQL relation files. /// use regex::Regex; -use std::error::Error; -use std::fmt; -#[derive(Debug, Clone)] -pub struct FilePathError { - msg: String, -} - -impl Error for FilePathError { - fn description(&self) -> &str { - &self.msg - } -} -impl FilePathError { - pub fn new(msg: &str) -> FilePathError { - FilePathError { - msg: msg.to_string(), - } - } +#[derive(Debug, Clone, thiserror::Error, PartialEq)] +pub enum FilePathError { + #[error("invalid relation fork name")] + InvalidForkName, + #[error("invalid relation data file name")] + InvalidFileName, } impl From for FilePathError { - fn from(e: core::num::ParseIntError) -> Self { - return FilePathError { - msg: format!("invalid filename: {}", e), - }; - } -} - -impl fmt::Display for FilePathError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "invalid filename") + fn from(_e: core::num::ParseIntError) -> Self { + FilePathError::InvalidFileName } } @@ -45,7 +25,7 @@ pub fn forkname_to_number(forkname: Option<&str>) -> Result { Some("fsm") => Ok(1), Some("vm") => Ok(2), Some("init") => Ok(3), - Some(_) => Err(FilePathError::new("invalid forkname")), + Some(_) => Err(FilePathError::InvalidForkName), } } @@ -68,15 +48,16 @@ pub fn forknumber_to_name(forknum: u8) -> Option<&'static str> { /// _ /// . /// _. +/// +/// See functions relpath() and _mdfd_segpath() in PostgreSQL sources. +/// pub fn parse_relfilename(fname: &str) -> Result<(u32, u8, u32), FilePathError> { let re = Regex::new(r"^(?P\d+)(_(?P[a-z]+))?(\.(?P\d+))?$").unwrap(); - let caps = re - .captures(fname) - .ok_or_else(|| FilePathError::new("invalid relation data file name"))?; + let caps = re.captures(fname).ok_or(FilePathError::InvalidFileName)?; let relnode_str = caps.name("relnode").unwrap().as_str(); - let relnode = u32::from_str_radix(relnode_str, 10)?; + let relnode = relnode_str.parse::()?; let forkname = caps.name("forkname").map(|f| f.as_str()); let forknum = forkname_to_number(forkname)?; @@ -85,8 +66,70 @@ pub fn parse_relfilename(fname: &str) -> Result<(u32, u8, u32), FilePathError> { let segno = if segno_match.is_none() { 0 } else { - u32::from_str_radix(segno_match.unwrap().as_str(), 10)? + segno_match.unwrap().as_str().parse::()? }; Ok((relnode, forknum, segno)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_parse_valid_relfilenames() { + assert_eq!(parse_relfilename("1234"), Ok((1234, 0, 0))); + assert_eq!(parse_relfilename("1234_fsm"), Ok((1234, 1, 0))); + assert_eq!(parse_relfilename("1234_vm"), Ok((1234, 2, 0))); + assert_eq!(parse_relfilename("1234_init"), Ok((1234, 3, 0))); + + assert_eq!(parse_relfilename("1234.12"), Ok((1234, 0, 12))); + assert_eq!(parse_relfilename("1234_fsm.12"), Ok((1234, 1, 12))); + assert_eq!(parse_relfilename("1234_vm.12"), Ok((1234, 2, 12))); + assert_eq!(parse_relfilename("1234_init.12"), Ok((1234, 3, 12))); + + // relfilenode is unsigned, so it can go up to 2^32-1 + assert_eq!(parse_relfilename("3147483648"), Ok((3147483648, 0, 0))); + } + + #[test] + fn test_parse_invalid_relfilenames() { + assert_eq!( + parse_relfilename("foo"), + Err(FilePathError::InvalidFileName) + ); + assert_eq!( + parse_relfilename("1.2.3"), + Err(FilePathError::InvalidFileName) + ); + assert_eq!( + parse_relfilename("1234_invalid"), + Err(FilePathError::InvalidForkName) + ); + assert_eq!( + parse_relfilename("1234_"), + Err(FilePathError::InvalidFileName) + ); + + // too large for u32 + assert_eq!( + parse_relfilename("12345678901"), + Err(FilePathError::InvalidFileName) + ); + assert_eq!( + parse_relfilename("-1234"), + Err(FilePathError::InvalidFileName) + ); + } + + #[test] + fn test_parse_weird_relfilenames() { + // we accept 0 for the relfilenode, but PostgreSQL should never do that. + assert_eq!(parse_relfilename("0"), Ok((0, 0, 0))); + + // PostgreSQL has a limit of 2^32-2 blocks in a table. With 8k block size and + // 1 GB segments, the max segment number is 32767. But we accept larger values + // currently. + assert_eq!(parse_relfilename("1.123456"), Ok((1, 0, 123456))); + } +}