Cleanup: Comments, imports, statics, missing type annotation

Cargo.lock

@@ -6131,6 +6131,7 @@ dependencies = [
  "postgres-protocol",
  "postgres_backend",
  "postgres_ffi",
+ "postgres_versioninfo",
  "pprof",
  "pq_proto",
  "rand 0.8.5",

libs/pageserver_api/src/models.rs

@@ -399,7 +399,7 @@ pub enum TimelineCreateRequestMode {
         // inherits the ancestor's pg_version. Earlier code wasn't
         // using a flattened enum, so, it was an accepted field, and
         // we continue to accept it by having it here.
-        pg_version: Option<u32>,
+        pg_version: Option<PgMajorVersion>,
         #[serde(default, skip_serializing_if = "std::ops::Not::not")]
         read_only: bool,
     },

libs/postgres_versioninfo/src/lib.rs

@@ -2,6 +2,8 @@ use serde_repr::{Deserialize_repr, Serialize_repr};
 use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 
+/// An enum with one variant for each major version of PostgreSQL that we support.
+///
 #[derive(Debug, Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Deserialize_repr, Serialize_repr)]
 #[repr(u32)]
 pub enum PgMajorVersion {
@@ -9,11 +11,13 @@ pub enum PgMajorVersion {
     PG15 = 15,
     PG16 = 16,
     PG17 = 17,
+    // !!! When you add a new PgMajorVersion, don't forget to update PgMajorVersion::ALL
 }
 
 pub type PgVersionId = u32;
 
 impl PgMajorVersion {
+    /// Get the numerical representation of the represented Major Version
     pub const fn major_version_num(&self) -> u32 {
         match self {
             PgMajorVersion::PG14 => 14,
@@ -23,6 +27,10 @@ impl PgMajorVersion {
         }
     }
 
+    /// Get the contents of this version's PG_VERSION file.
+    ///
+    /// The PG_VERSION file is used to determine the PostgreSQL version that currently
+    /// owns the data in a PostgreSQL data directory.
     pub fn versionfile_string(&self) -> String {
         match self {
             PgMajorVersion::PG17 => "17\x0A".to_string(),
@@ -32,6 +40,10 @@ impl PgMajorVersion {
         }
     }
 
+    /// Get the v{version} string of this major PostgreSQL version.
+    ///
+    /// Because this was hand-coded in various places, this was moved into a shared
+    /// implementation.
     pub fn v_str(&self) -> String {
         match self {
             PgMajorVersion::PG17 => "v17".to_string(),
@@ -41,14 +53,13 @@ impl PgMajorVersion {
         }
     }
 
-    pub const fn all() -> [PgMajorVersion; 4] {
-        [
-            PgMajorVersion::PG14,
-            PgMajorVersion::PG15,
-            PgMajorVersion::PG16,
-            PgMajorVersion::PG17,
-        ]
-    }
+    /// All currently supported major versions of PostgreSQL.
+    pub const ALL: [PgMajorVersion; 4] = [
+        PgMajorVersion::PG14,
+        PgMajorVersion::PG15,
+        PgMajorVersion::PG16,
+        PgMajorVersion::PG17,
+    ];
 }
 
 impl Display for PgMajorVersion {

pageserver/src/walingest.rs

@@ -1627,7 +1627,7 @@ mod tests {
 
     #[tokio::test]
     async fn test_zeroed_checkpoint_decodes_correctly() -> Result<(), anyhow::Error> {
-        for i in PgMajorVersion::all() {
+        for i in PgMajorVersion::ALL {
             dispatch_pgversion!(i, {
                 pgv::CheckPoint::decode(&pgv::ZERO_CHECKPOINT)?;
             });

safekeeper/Cargo.toml

@@ -58,6 +58,7 @@ metrics.workspace = true
 pem.workspace = true
 postgres_backend.workspace = true
 postgres_ffi.workspace = true
+postgres_versioninfo.workspace = true
 pq_proto.workspace = true
 remote_storage.workspace = true
 safekeeper_api.workspace = true

safekeeper/src/control_file_upgrade.rs

@@ -2,6 +2,7 @@
 use std::vec;
 
 use anyhow::{Result, bail};
+use postgres_versioninfo::{PgMajorVersion, PgVersionId};
 use pq_proto::SystemId;
 use safekeeper_api::membership::{Configuration, INVALID_GENERATION};
 use safekeeper_api::{ServerInfo, Term};
@@ -46,7 +47,7 @@ struct SafeKeeperStateV1 {
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct ServerInfoV2 {
     /// Postgres server version
-    pub pg_version: u32,
+    pub pg_version: PgVersionId,
     pub system_id: SystemId,
     pub tenant_id: TenantId,
     pub timeline_id: TimelineId,
@@ -75,7 +76,7 @@ pub struct SafeKeeperStateV2 {
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 pub struct ServerInfoV3 {
     /// Postgres server version
-    pub pg_version: u32,
+    pub pg_version: PgVersionId,
     pub system_id: SystemId,
     #[serde(with = "hex")]
     pub tenant_id: TenantId,
@@ -563,7 +564,7 @@ mod tests {
                 epoch: 43,
             },
             server: ServerInfoV2 {
-                pg_version: 14,
+                pg_version: PgVersionId::from(PgMajorVersion::PG14),
                 system_id: 0x1234567887654321,
                 tenant_id,
                 timeline_id,
@@ -626,7 +627,7 @@ mod tests {
                 }]),
             },
             server: ServerInfoV2 {
-                pg_version: 14,
+                pg_version: PgVersionId::from(PgMajorVersion::PG14),
                 system_id: 0x1234567887654321,
                 tenant_id,
                 timeline_id,
@@ -675,7 +676,7 @@ mod tests {
                 }]),
             },
             server: ServerInfoV3 {
-                pg_version: 14,
+                pg_version: PgVersionId::from(PgMajorVersion::PG14),
                 system_id: 0x1234567887654321,
                 tenant_id,
                 timeline_id,
@@ -731,7 +732,7 @@ mod tests {
                 }]),
             },
             server: ServerInfo {
-                pg_version: 14,
+                pg_version: PgVersionId::from(PgMajorVersion::PG14),
                 system_id: 0x1234567887654321,
                 wal_seg_size: 0x12345678,
             },

safekeeper/src/safekeeper.rs

@@ -1750,7 +1750,7 @@ mod tests {
                 }]),
             },
             server: ServerInfo {
-                pg_version: 14,
+                pg_version: 140000,
                 system_id: 0x1234567887654321,
                 wal_seg_size: 0x12345678,
             },

safekeeper/src/wal_storage.rs

@@ -18,7 +18,8 @@ use camino::{Utf8Path, Utf8PathBuf};
 use futures::future::BoxFuture;
 use postgres_ffi::v14::xlog_utils::{IsPartialXLogFileName, IsXLogFileName, XLogFromFileName};
 use postgres_ffi::waldecoder::WalStreamDecoder;
-use postgres_ffi::{PG_TLI, PgMajorVersion, XLogFileName, XLogSegNo, dispatch_pgversion};
+use postgres_ffi::{PG_TLI, XLogFileName, XLogSegNo, dispatch_pgversion};
+use postgres_versioninfo::{PgMajorVersion, PgVersionId};
 use pq_proto::SystemId;
 use remote_storage::RemotePath;
 use std::sync::Arc;
@@ -92,7 +93,7 @@ pub struct PhysicalStorage {
 
     /// Size of WAL segment in bytes.
     wal_seg_size: usize,
-    pg_version: u32,
+    pg_version: PgVersionId,
     system_id: u64,
 
     /// Written to disk, but possibly still in the cache and not fully persisted.