From 4aabc9a6829480851633f5e3837b3363e636d441 Mon Sep 17 00:00:00 2001 From: Eric Seppanen Date: Sat, 22 May 2021 00:42:23 -0700 Subject: [PATCH] easy clippy cleanups Various things that clippy complains about, and are really easy to fix. --- control_plane/src/local_env.rs | 2 +- integration_tests/src/lib.rs | 12 ++-- integration_tests/tests/test_wal_acceptor.rs | 6 +- pageserver/src/basebackup.rs | 4 +- pageserver/src/branches.rs | 59 ++++++++++---------- pageserver/src/page_service.rs | 4 +- pageserver/src/repository.rs | 2 +- pageserver/src/repository/rocksdb.rs | 8 +-- pageserver/src/waldecoder.rs | 2 +- pageserver/src/walredo.rs | 21 ++++--- postgres_ffi/src/lib.rs | 2 +- postgres_ffi/src/nonrelfile_utils.rs | 2 +- postgres_ffi/src/xlog_utils.rs | 52 +++++++++-------- walkeeper/src/receive_wal.rs | 2 +- walkeeper/src/replication.rs | 6 +- walkeeper/src/s3_offload.rs | 14 ++--- walkeeper/src/send_wal.rs | 2 +- zenith/src/main.rs | 10 ++-- 18 files changed, 99 insertions(+), 111 deletions(-) diff --git a/control_plane/src/local_env.rs b/control_plane/src/local_env.rs index d8f4b4f8af..fabc809bfc 100644 --- a/control_plane/src/local_env.rs +++ b/control_plane/src/local_env.rs @@ -49,7 +49,7 @@ impl LocalEnv { Ok(self .zenith_distrib_dir .as_ref() - .ok_or(anyhow!("Can not manage remote pageserver"))? + .ok_or_else(|| anyhow!("Can not manage remote pageserver"))? .join("pageserver")) } diff --git a/integration_tests/src/lib.rs b/integration_tests/src/lib.rs index a19253dc90..c8e41b3058 100644 --- a/integration_tests/src/lib.rs +++ b/integration_tests/src/lib.rs @@ -1,3 +1,6 @@ +use anyhow::{bail, Result}; +use nix::sys::signal::{kill, Signal}; +use nix::unistd::Pid; use std::collections::BTreeMap; use std::convert::TryInto; use std::fs::{self, File, OpenOptions}; @@ -8,11 +11,6 @@ use std::process::{Command, ExitStatus}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use anyhow::{bail, Result}; -use nix::sys::signal::{kill, Signal}; -use nix::unistd::Pid; -use postgres; - use control_plane::compute::PostgresNode; use control_plane::read_pidfile; use control_plane::{local_env::LocalEnv, storage::PageServerNode}; @@ -41,11 +39,11 @@ pub fn create_test_env(testname: &str) -> LocalEnv { let _ = fs::remove_dir_all(&base_path); fs::create_dir_all(&base_path) - .expect(format!("could not create directory for {}", base_path_str).as_str()); + .unwrap_or_else(|_| panic!("could not create directory for {}", base_path_str)); let pgdatadirs_path = base_path.join("pgdatadirs"); fs::create_dir(&pgdatadirs_path) - .expect(format!("could not create directory {:?}", pgdatadirs_path).as_str()); + .unwrap_or_else(|_| panic!("could not create directory {:?}", pgdatadirs_path)); LocalEnv { pageserver_connstring: "postgresql://127.0.0.1:64000".to_string(), diff --git a/integration_tests/tests/test_wal_acceptor.rs b/integration_tests/tests/test_wal_acceptor.rs index f58c915cc7..2dfe1b831e 100644 --- a/integration_tests/tests/test_wal_acceptor.rs +++ b/integration_tests/tests/test_wal_acceptor.rs @@ -5,7 +5,6 @@ use std::{thread, time}; use control_plane::compute::{ComputeControlPlane, PostgresNode}; -use integration_tests; use integration_tests::PostgresNodeExt; use integration_tests::TestStorageControlPlane; @@ -14,7 +13,7 @@ const DOWNTIME: u64 = 2; fn start_node_with_wal_proposer( timeline: &str, compute_cplane: &mut ComputeControlPlane, - wal_acceptors: &String, + wal_acceptors: &str, ) -> Arc { let node = compute_cplane.new_test_master_node(timeline); let _node = node.append_conf( @@ -100,8 +99,7 @@ fn test_many_timelines() { let wal_acceptors = storage_cplane.get_wal_acceptor_conn_info(); // Create branches - let mut timelines: Vec = Vec::new(); - timelines.push("main".to_string()); + let mut timelines: Vec = vec!["main".to_string()]; for i in 1..N_TIMELINES { let branchname = format!("experimental{}", i); diff --git a/pageserver/src/basebackup.rs b/pageserver/src/basebackup.rs index 7ac1f30cf2..1096f0ed38 100644 --- a/pageserver/src/basebackup.rs +++ b/pageserver/src/basebackup.rs @@ -51,7 +51,7 @@ fn add_slru_segments( let tag = BufferTag { rel, blknum: page }; let img = timeline.get_page_at_lsn(tag, lsn)?; // Zero length image indicates truncated segment: just skip it - if img.len() != 0 { + if !img.is_empty() { assert!(img.len() == pg_constants::BLCKSZ as usize); let segno = page / pg_constants::SLRU_PAGES_PER_SEGMENT; @@ -381,7 +381,7 @@ fn parse_rel_file_path(path: &str) -> Result<(), FilePathError> { let (_relnode, _forknum, _segno) = parse_relfilename(fname)?; Ok(()) - } else if let Some(_) = path.strip_prefix("pg_tblspc/") { + } else if path.strip_prefix("pg_tblspc/").is_some() { // TODO error!("tablespaces not implemented yet"); Err(FilePathError::InvalidFileName) diff --git a/pageserver/src/branches.rs b/pageserver/src/branches.rs index 60dca50862..8543d7c8dd 100644 --- a/pageserver/src/branches.rs +++ b/pageserver/src/branches.rs @@ -7,7 +7,6 @@ use anyhow::{anyhow, bail, Context, Result}; use bytes::Bytes; use fs::File; -use fs_extra; use postgres_ffi::{pg_constants, xlog_utils}; use rand::Rng; use serde::{Deserialize, Serialize}; @@ -340,40 +339,38 @@ fn copy_wal(src_dir: &Path, dst_dir: &Path, upto: Lsn, wal_seg_size: usize) -> R let last_segno = upto.segment_number(wal_seg_size); let last_segoff = upto.segment_offset(wal_seg_size); - for entry in fs::read_dir(src_dir).unwrap() { - if let Ok(entry) = entry { - let entry_name = entry.file_name(); - let fname = entry_name.to_str().unwrap(); + for entry in fs::read_dir(src_dir).unwrap().flatten() { + let entry_name = entry.file_name(); + let fname = entry_name.to_str().unwrap(); - // Check if the filename looks like an xlog file, or a .partial file. - if !xlog_utils::IsXLogFileName(fname) && !xlog_utils::IsPartialXLogFileName(fname) { - continue; - } - let (segno, _tli) = xlog_utils::XLogFromFileName(fname, wal_seg_size as usize); + // Check if the filename looks like an xlog file, or a .partial file. + if !xlog_utils::IsXLogFileName(fname) && !xlog_utils::IsPartialXLogFileName(fname) { + continue; + } + let (segno, _tli) = xlog_utils::XLogFromFileName(fname, wal_seg_size as usize); - let copylen; - let mut dst_fname = PathBuf::from(fname); - if segno > last_segno { - // future segment, skip - continue; - } else if segno < last_segno { - copylen = wal_seg_size; - dst_fname.set_extension(""); - } else { - copylen = last_segoff; - dst_fname.set_extension("partial"); - } + let copylen; + let mut dst_fname = PathBuf::from(fname); + if segno > last_segno { + // future segment, skip + continue; + } else if segno < last_segno { + copylen = wal_seg_size; + dst_fname.set_extension(""); + } else { + copylen = last_segoff; + dst_fname.set_extension("partial"); + } - let src_file = File::open(entry.path())?; - let mut dst_file = File::create(dst_dir.join(&dst_fname))?; - std::io::copy(&mut src_file.take(copylen as u64), &mut dst_file)?; + let src_file = File::open(entry.path())?; + let mut dst_file = File::create(dst_dir.join(&dst_fname))?; + std::io::copy(&mut src_file.take(copylen as u64), &mut dst_file)?; - if copylen < wal_seg_size { - std::io::copy( - &mut std::io::repeat(0).take((wal_seg_size - copylen) as u64), - &mut dst_file, - )?; - } + if copylen < wal_seg_size { + std::io::copy( + &mut std::io::repeat(0).take((wal_seg_size - copylen) as u64), + &mut dst_file, + )?; } } Ok(()) diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index 0408a349ff..3376a7b7e1 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -77,7 +77,7 @@ enum PagestreamBeMessage { Read(PagestreamReadResponse), } -const HELLO_WORLD_ROW: BeMessage = BeMessage::DataRow(Bytes::from_static(b"hello world")); +static HELLO_WORLD_ROW: BeMessage = BeMessage::DataRow(Bytes::from_static(b"hello world")); #[derive(Debug)] struct PagestreamRequest { @@ -921,7 +921,7 @@ impl Connection { // find latest snapshot let snapshot_lsn = restore_local_repo::find_latest_snapshot(&self.conf, timelineid).unwrap(); - let req_lsn = lsn.unwrap_or(timeline.get_last_valid_lsn()); + let req_lsn = lsn.unwrap_or_else(|| timeline.get_last_valid_lsn()); basebackup::send_tarball_at_lsn( &mut CopyDataSink { stream }, timelineid, diff --git a/pageserver/src/repository.rs b/pageserver/src/repository.rs index d9e8d2b393..1595ccb0eb 100644 --- a/pageserver/src/repository.rs +++ b/pageserver/src/repository.rs @@ -347,7 +347,7 @@ mod tests { gc_horizon: 64 * 1024 * 1024, gc_period: Duration::from_secs(10), listen_addr: "127.0.0.1:5430".parse().unwrap(), - workdir: repo_dir.into(), + workdir: repo_dir, pg_distrib_dir: "".into(), }; // Make a static copy of the config. This can never be free'd, but that's diff --git a/pageserver/src/repository/rocksdb.rs b/pageserver/src/repository/rocksdb.rs index 2ec40156d8..888c65116f 100644 --- a/pageserver/src/repository/rocksdb.rs +++ b/pageserver/src/repository/rocksdb.rs @@ -184,7 +184,7 @@ impl RocksRepository { walredo_mgr: Arc, ) -> RocksRepository { RocksRepository { - conf: conf, + conf, timelines: Mutex::new(HashMap::new()), walredo_mgr, } @@ -666,7 +666,7 @@ impl Timeline for RocksTimeline { } iter.next(); } - return Ok(gxacts); + Ok(gxacts) } /// Get databases. This function is used to local pg_filenode.map files @@ -700,7 +700,7 @@ impl Timeline for RocksTimeline { } iter.next(); } - return Ok(dbs); + Ok(dbs) } /// Get range [begin,end) of stored blocks. Used mostly for SMGR pseudorelations @@ -822,7 +822,7 @@ impl Timeline for RocksTimeline { return Ok(Some(img)); } } - return Ok(None); + Ok(None) } /// diff --git a/pageserver/src/waldecoder.rs b/pageserver/src/waldecoder.rs index 8ab7c2e507..e737ca6066 100644 --- a/pageserver/src/waldecoder.rs +++ b/pageserver/src/waldecoder.rs @@ -1113,7 +1113,7 @@ pub fn decode_wal_record(checkpoint: &mut CheckPoint, record: Bytes) -> DecodedW blocks.push(blk); } } else { - assert!(false); + panic!() } } else if xlogrec.xl_rmid == pg_constants::RM_RELMAP_ID { let xlrec = XlRelmapUpdate::decode(&mut buf); diff --git a/pageserver/src/walredo.rs b/pageserver/src/walredo.rs index 52e8218981..ab18ad9205 100644 --- a/pageserver/src/walredo.rs +++ b/pageserver/src/walredo.rs @@ -170,22 +170,21 @@ impl WalRedoManager for PostgresRedoManager { } fn mx_offset_to_flags_offset(xid: MultiXactId) -> usize { - return ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) as u16 + ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) as u16 % pg_constants::MULTIXACT_MEMBERGROUPS_PER_PAGE - * pg_constants::MULTIXACT_MEMBERGROUP_SIZE) as usize; + * pg_constants::MULTIXACT_MEMBERGROUP_SIZE) as usize } fn mx_offset_to_flags_bitshift(xid: MultiXactId) -> u16 { - return (xid as u16) % pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP - * pg_constants::MXACT_MEMBER_BITS_PER_XACT; + (xid as u16) % pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP + * pg_constants::MXACT_MEMBER_BITS_PER_XACT } /* Location (byte offset within page) of TransactionId of given member */ fn mx_offset_to_member_offset(xid: MultiXactId) -> usize { - return mx_offset_to_flags_offset(xid) + mx_offset_to_flags_offset(xid) + (pg_constants::MULTIXACT_FLAGBYTES_PER_GROUP - + (xid as u16 % pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP) * 4) - as usize; + + (xid as u16 % pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP) * 4) as usize } /// @@ -347,9 +346,9 @@ impl PostgresRedoManagerInternal { } } else if xlogrec.xl_rmid == pg_constants::RM_MULTIXACT_ID { let info = xlogrec.xl_info & pg_constants::XLR_RMGR_INFO_MASK; - if info == pg_constants::XLOG_MULTIXACT_ZERO_OFF_PAGE { - page.copy_from_slice(&ZERO_PAGE); - } else if info == pg_constants::XLOG_MULTIXACT_ZERO_MEM_PAGE { + if info == pg_constants::XLOG_MULTIXACT_ZERO_OFF_PAGE + || info == pg_constants::XLOG_MULTIXACT_ZERO_MEM_PAGE + { page.copy_from_slice(&ZERO_PAGE); } else if info == pg_constants::XLOG_MULTIXACT_CREATE_ID { let xlrec = XlMultiXactCreate::decode(&mut buf); @@ -388,7 +387,7 @@ impl PostgresRedoManagerInternal { // empty page image indicates that this SLRU page is truncated and can be removed by GC page.clear(); } else { - assert!(false); + panic!(); } } else if xlogrec.xl_rmid == pg_constants::RM_RELMAP_ID { page.clear(); diff --git a/postgres_ffi/src/lib.rs b/postgres_ffi/src/lib.rs index 4dcad7a081..29a01c3445 100644 --- a/postgres_ffi/src/lib.rs +++ b/postgres_ffi/src/lib.rs @@ -75,7 +75,7 @@ pub fn encode_pg_control(controlfile: ControlFileData) -> Bytes { pub fn encode_checkpoint(checkpoint: CheckPoint) -> Bytes { let b: [u8; SIZEOF_CHECKPOINT]; b = unsafe { std::mem::transmute::(checkpoint) }; - return Bytes::copy_from_slice(&b[..]); + Bytes::copy_from_slice(&b[..]) } pub fn decode_checkpoint(mut buf: Bytes) -> Result { diff --git a/postgres_ffi/src/nonrelfile_utils.rs b/postgres_ffi/src/nonrelfile_utils.rs index c925fdb68e..a0dee29a9c 100644 --- a/postgres_ffi/src/nonrelfile_utils.rs +++ b/postgres_ffi/src/nonrelfile_utils.rs @@ -28,5 +28,5 @@ pub fn transaction_id_get_status(xid: u32, page: &[u8]) -> u8 { let bshift: u8 = ((xid % pg_constants::CLOG_XACTS_PER_BYTE) * pg_constants::CLOG_BITS_PER_XACT as u32) as u8; - return ((page[byteno] >> bshift) & pg_constants::CLOG_XACT_BITMASK) as u8; + ((page[byteno] >> bshift) & pg_constants::CLOG_XACT_BITMASK) as u8 } diff --git a/postgres_ffi/src/xlog_utils.rs b/postgres_ffi/src/xlog_utils.rs index 836c9272a5..4a0266b077 100644 --- a/postgres_ffi/src/xlog_utils.rs +++ b/postgres_ffi/src/xlog_utils.rs @@ -199,33 +199,31 @@ pub fn find_end_of_wal( let mut high_tli: TimeLineID = 0; let mut high_ispartial = false; - for entry in fs::read_dir(data_dir).unwrap() { - if let Ok(entry) = entry { - let ispartial: bool; - let entry_name = entry.file_name(); - let fname = entry_name.to_str().unwrap(); - /* - * Check if the filename looks like an xlog file, or a .partial file. - */ - if IsXLogFileName(fname) { - ispartial = false; - } else if IsPartialXLogFileName(fname) { - ispartial = true; - } else { - continue; - } - let (segno, tli) = XLogFromFileName(fname, wal_seg_size); - if !ispartial && entry.metadata().unwrap().len() != wal_seg_size as u64 { - continue; - } - if segno > high_segno - || (segno == high_segno && tli > high_tli) - || (segno == high_segno && tli == high_tli && high_ispartial && !ispartial) - { - high_segno = segno; - high_tli = tli; - high_ispartial = ispartial; - } + for entry in fs::read_dir(data_dir).unwrap().flatten() { + let ispartial: bool; + let entry_name = entry.file_name(); + let fname = entry_name.to_str().unwrap(); + /* + * Check if the filename looks like an xlog file, or a .partial file. + */ + if IsXLogFileName(fname) { + ispartial = false; + } else if IsPartialXLogFileName(fname) { + ispartial = true; + } else { + continue; + } + let (segno, tli) = XLogFromFileName(fname, wal_seg_size); + if !ispartial && entry.metadata().unwrap().len() != wal_seg_size as u64 { + continue; + } + if segno > high_segno + || (segno == high_segno && tli > high_tli) + || (segno == high_segno && tli == high_tli && high_ispartial && !ispartial) + { + high_segno = segno; + high_tli = tli; + high_ispartial = ispartial; } } if high_segno > 0 { diff --git a/walkeeper/src/receive_wal.rs b/walkeeper/src/receive_wal.rs index 1a261f6e7b..b3a20fa1d0 100644 --- a/walkeeper/src/receive_wal.rs +++ b/walkeeper/src/receive_wal.rs @@ -22,7 +22,7 @@ use crate::WalAcceptorConf; use pageserver::ZTimelineId; use postgres_ffi::xlog_utils::{TimeLineID, XLogFileName, MAX_SEND_SIZE, XLOG_BLCKSZ}; -pub const SK_MAGIC: u32 = 0xCafeCeefu32; +pub const SK_MAGIC: u32 = 0xcafeceefu32; pub const SK_FORMAT_VERSION: u32 = 1; const SK_PROTOCOL_VERSION: u32 = 1; const UNKNOWN_SERVER_VERSION: u32 = 0; diff --git a/walkeeper/src/replication.rs b/walkeeper/src/replication.rs index d636b2731d..4dc0717ca1 100644 --- a/walkeeper/src/replication.rs +++ b/walkeeper/src/replication.rs @@ -87,7 +87,7 @@ impl ReplicationConn { /// Helper function that parses a pair of LSNs. fn parse_start_stop(cmd: &[u8]) -> Result<(Lsn, Lsn)> { let re = Regex::new(r"([[:xdigit:]]+/[[:xdigit:]]+)").unwrap(); - let caps = re.captures_iter(str::from_utf8(&cmd[..])?); + let caps = re.captures_iter(str::from_utf8(cmd)?); let mut lsns = caps.map(|cap| cap[1].parse::()); let start_pos = lsns .next() @@ -107,10 +107,10 @@ impl ReplicationConn { // If that failed, try it without the .partial extension. match File::open(&wal_file_path) { - Ok(opened_file) => return Ok(opened_file), + Ok(opened_file) => Ok(opened_file), Err(e) => { error!("Failed to open log file {:?}: {}", &wal_file_path, e); - return Err(e.into()); + Err(e.into()) } } } diff --git a/walkeeper/src/s3_offload.rs b/walkeeper/src/s3_offload.rs index a8b6cc77b1..a3743928f9 100644 --- a/walkeeper/src/s3_offload.rs +++ b/walkeeper/src/s3_offload.rs @@ -12,8 +12,7 @@ use std::collections::HashSet; use std::env; use std::fs::{self, File}; use std::io::prelude::*; -use std::iter::FromIterator; -use std::path::PathBuf; +use std::path::Path; use std::time::SystemTime; use tokio::runtime; use tokio::time::sleep; @@ -42,7 +41,7 @@ pub fn thread_main(conf: WalAcceptorConf) { async fn offload_files( bucket: &Bucket, listing: &HashSet, - dir_path: &PathBuf, + dir_path: &Path, conf: &WalAcceptorConf, ) -> Result { let horizon = SystemTime::now() - conf.ttl.unwrap(); @@ -93,11 +92,10 @@ async fn main_loop(conf: &WalAcceptorConf) -> Result<()> { let results = bucket .list("walarchive/".to_string(), Some("".to_string())) .await?; - let listing = HashSet::from_iter( - results - .iter() - .flat_map(|b| b.contents.iter().map(|o| o.key.clone())), - ); + let listing = results + .iter() + .flat_map(|b| b.contents.iter().map(|o| o.key.clone())) + .collect(); let n = offload_files(&bucket, &listing, &conf.data_dir, conf).await?; info!("Offload {} files to S3", n); diff --git a/walkeeper/src/send_wal.rs b/walkeeper/src/send_wal.rs index 1884c1aab4..fc29c3575d 100644 --- a/walkeeper/src/send_wal.rs +++ b/walkeeper/src/send_wal.rs @@ -49,7 +49,7 @@ impl SendWalConn { /// Send WAL to replica or WAL receiver using standard libpq replication protocol /// pub fn run(mut self) -> Result<()> { - let peer_addr = self.peer_addr.clone(); + let peer_addr = self.peer_addr; info!("WAL sender to {:?} is started", peer_addr); // Handle the startup message first. diff --git a/zenith/src/main.rs b/zenith/src/main.rs index 2a64e29bdb..779da7efd5 100644 --- a/zenith/src/main.rs +++ b/zenith/src/main.rs @@ -199,11 +199,11 @@ fn print_branches_tree(branches: Vec) -> Result<()> { } // Sort children by tid to bring some minimal order. - for (_tid, branch) in &mut branches_hash { + for branch in &mut branches_hash.values_mut() { branch.children.sort(); } - for (_tid, branch) in &branches_hash { + for branch in branches_hash.values() { // Start with root branches (no ancestors) first. // Now there is 'main' branch only, but things may change. if branch.info.ancestor_id.is_none() { @@ -217,7 +217,7 @@ fn print_branches_tree(branches: Vec) -> Result<()> { // Recursively print branch info with all its children. fn print_branch( nesting_level: usize, - is_last: &Vec, + is_last: &[bool], branch: &BranchTreeEl, branches: &HashMap, ) -> Result<()> { @@ -257,7 +257,7 @@ fn print_branch( let len = branch.children.len(); let mut i: usize = 0; - let mut is_last_new = is_last.clone(); + let mut is_last_new = Vec::from(is_last); is_last_new.push(false); for child in &branch.children { @@ -340,7 +340,7 @@ fn handle_pg(pg_match: &ArgMatches, env: &local_env::LocalEnv) -> Result<()> { .map(|bi| bi .latest_valid_lsn .map_or("?".to_string(), |lsn| lsn.to_string())) - .unwrap_or("?".to_string()), + .unwrap_or_else(|| "?".to_string()), node.status(), ); }