Tidy up the parse_relfilename function.

A few things that Eric commented on at PR #96:

- Use thiserror to simplify the implemention of FilePathError
- Add unit tests
- Fix a few complaints from clippy

Cargo.lock

@@ -1341,6 +1341,7 @@ dependencies = [
  "log",
  "rand",
  "regex",
+ "thiserror",
 ]
 
 [[package]]

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::<u32>()?;
+        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)
     }
 }
 

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::<u32>()?;
 
         for direntry in fs::read_dir(direntry.path())? {
             let direntry = direntry?;

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"

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<core::num::ParseIntError> 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<u8, FilePathError> {
         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> {
 /// <oid>_<fork name>
 /// <oid>.<segment number>
 /// <oid>_<fork name>.<segment number>
+///
+/// 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<relnode>\d+)(_(?P<forkname>[a-z]+))?(\.(?P<segno>\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::<u32>()?;
 
     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::<u32>()?
     };
 
     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)));
+    }
+}