From 39f3ec8b9cb0e2285b0e9eeaadb82013cd263289 Mon Sep 17 00:00:00 2001 From: Matthias van de Meent Date: Mon, 23 Jun 2025 15:52:50 +0200 Subject: [PATCH] Cleanup: Comments, imports, statics, missing type annotation --- Cargo.lock | 1 + libs/pageserver_api/src/models.rs | 2 +- libs/postgres_versioninfo/src/lib.rs | 27 ++++++++++++++++++-------- pageserver/src/walingest.rs | 2 +- safekeeper/Cargo.toml | 1 + safekeeper/src/control_file_upgrade.rs | 13 +++++++------ safekeeper/src/safekeeper.rs | 2 +- safekeeper/src/wal_storage.rs | 5 +++-- 8 files changed, 34 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index afc342a9c7..51724da061 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6131,6 +6131,7 @@ dependencies = [ "postgres-protocol", "postgres_backend", "postgres_ffi", + "postgres_versioninfo", "pprof", "pq_proto", "rand 0.8.5", diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index ebae3e675a..dc75a666f6 100644 --- a/libs/pageserver_api/src/models.rs +++ b/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, + pg_version: Option, #[serde(default, skip_serializing_if = "std::ops::Not::not")] read_only: bool, }, diff --git a/libs/postgres_versioninfo/src/lib.rs b/libs/postgres_versioninfo/src/lib.rs index c81ce4b301..481e30d015 100644 --- a/libs/postgres_versioninfo/src/lib.rs +++ b/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 { diff --git a/pageserver/src/walingest.rs b/pageserver/src/walingest.rs index 4adc4c5475..00f3b4cd1c 100644 --- a/pageserver/src/walingest.rs +++ b/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)?; }); diff --git a/safekeeper/Cargo.toml b/safekeeper/Cargo.toml index 0a8cc415be..6955028c73 100644 --- a/safekeeper/Cargo.toml +++ b/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 diff --git a/safekeeper/src/control_file_upgrade.rs b/safekeeper/src/control_file_upgrade.rs index 1ad9e62f9b..e21c6e143a 100644 --- a/safekeeper/src/control_file_upgrade.rs +++ b/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, }, diff --git a/safekeeper/src/safekeeper.rs b/safekeeper/src/safekeeper.rs index 886cac869d..582ceebf25 100644 --- a/safekeeper/src/safekeeper.rs +++ b/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, }, diff --git a/safekeeper/src/wal_storage.rs b/safekeeper/src/wal_storage.rs index 4782b15eb0..e74f4353b0 100644 --- a/safekeeper/src/wal_storage.rs +++ b/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.