From ba07fabd04fbdff83ffb35e804d28c5d0e019be1 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 19 Jun 2026 21:12:06 -0700 Subject: [PATCH] feat!: explicit binary directory for postgres Motivation: we're integrating tmp-postgrust into our Buck2 builds of https://github.com/mercurytechnologies/locally-euclidean, which, in the Bazel tradition, looks dimly on resolution of executables from PATH (in fact, there's not any easy way to put something *extra* into the PATH of a target, and this friction is intentional). This PR adds a field to TmpPostgrustFactoryConfig to explicitly give the postgres bin directory. This is a breaking change, the resolution for existing users is to change their initialization to create with `Default::default()`, then modify the fields they care about. Perhaps we should have a builder? idk. However, this will be the last time that API breaks in the future, since we add a `#[non_exhaustive]` to forbid consumers from writing code that breaks when we add fields. This also fixes a bug where `createdb` and `createuser` only considered PATH and not the fallback directories. Disclosure: the code here was written with Claude, but I've reviewed it fairly carefully and added some extra clarification at some points that benefit from it. The commit message is handwritten because I have standards. The approach here is exactly what I would write by hand. --- Cargo.toml | 2 +- src/asynchronous.rs | 23 +++++----- src/errors.rs | 11 +++++ src/lib.rs | 102 ++++++++++++++++++++++++++++++++++++++++---- src/search.rs | 50 ++++++++++++++++++++-- src/synchronous.rs | 25 +++++------ 6 files changed, 176 insertions(+), 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a7c8727..27a3e2a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "tmp-postgrust" -version = "0.10.1" +version = "0.11.0" authors = ["John Children "] license = "MIT" edition = "2018" diff --git a/src/asynchronous.rs b/src/asynchronous.rs index 0c72aee..b93c813 100644 --- a/src/asynchronous.rs +++ b/src/asynchronous.rs @@ -16,7 +16,7 @@ use tokio::{ use tracing::{debug, instrument}; use crate::errors::{ProcessCapture, TmpPostgrustError, TmpPostgrustResult}; -use crate::search::{all_dir_entries, build_copy_dst_path, find_postgresql_command}; +use crate::search::{all_dir_entries, build_copy_dst_path}; use crate::POSTGRES_UID_GID; #[instrument(skip(command, fail))] @@ -49,13 +49,11 @@ async fn exec_process( #[instrument] pub(crate) fn start_postgres_subprocess( + postgres_bin: &Path, data_directory: &Path, port: u32, ) -> TmpPostgrustResult { - let postgres_path = - find_postgresql_command("bin", "postgres").expect("failed to find postgres"); - - let mut command = Command::new(postgres_path); + let mut command = Command::new(postgres_bin); command .env("PGDATA", data_directory.to_str().unwrap()) .arg("-p") @@ -69,11 +67,12 @@ pub(crate) fn start_postgres_subprocess( } #[instrument] -pub(crate) async fn exec_init_db(data_directory: &Path) -> TmpPostgrustResult<()> { - let initdb_path = find_postgresql_command("bin", "initdb").expect("failed to find initdb"); - +pub(crate) async fn exec_init_db( + initdb_bin: &Path, + data_directory: &Path, +) -> TmpPostgrustResult<()> { debug!("Initializing database in: {:?}", data_directory); - let mut command = Command::new(initdb_path); + let mut command = Command::new(initdb_bin); command .env("PGDATA", data_directory.to_str().unwrap()) .arg("--username=postgres"); @@ -107,12 +106,13 @@ pub(crate) async fn exec_copy_dir(src_dir: &Path, dst_dir: &Path) -> TmpPostgrus #[instrument] pub(crate) async fn exec_create_db( + createdb_bin: &Path, socket: &Path, port: u32, owner: &str, dbname: &str, ) -> TmpPostgrustResult<()> { - let mut command = Command::new("createdb"); + let mut command = Command::new(createdb_bin); command .arg("-h") .arg(socket) @@ -130,11 +130,12 @@ pub(crate) async fn exec_create_db( #[instrument] pub(crate) async fn exec_create_user( + createuser_bin: &Path, socket: &Path, port: u32, username: &str, ) -> TmpPostgrustResult<()> { - let mut command = Command::new("createuser"); + let mut command = Command::new(createuser_bin); command .arg("-h") .arg(socket) diff --git a/src/errors.rs b/src/errors.rs index ceeb3a2..9d61e80 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -1,3 +1,5 @@ +use std::path::PathBuf; + use thiserror::Error; /// UTF-8 captures of stdout and stderr for child processes used by the library. @@ -11,6 +13,7 @@ pub struct ProcessCapture { /// Error type for possible postgresql errors. #[derive(Error, Debug)] +#[non_exhaustive] pub enum TmpPostgrustError { /// Catchall error for when a subprocess fails to run to completion #[error("subprocess failed to execute")] @@ -64,6 +67,14 @@ pub enum TmpPostgrustError { /// Error when running migrations failed. #[error("failed to run database migrations")] MigrationsFailed(#[source] Box), + /// Error when a required postgres binary cannot be found. + #[error("could not find postgres command `{command}`{}", .searched_dir.as_ref().map(|d| format!(" in {}", d.display())).unwrap_or_default())] + PostgresCommandNotFound { + /// Name of the binary that was not found. + command: String, + /// The explicit directory that was searched, if one was provided. + searched_dir: Option, + }, } /// Result type for `TmpPostgrustError`, used by functions in this crate. diff --git a/src/lib.rs b/src/lib.rs index 1e03f3b..5e2ef6c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -22,7 +22,7 @@ pub mod synchronous; use std::fmt::Write as FmtWrite; use std::fs::{metadata, set_permissions}; use std::io::{BufRead, BufReader}; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::atomic::AtomicU32; use std::sync::{Arc, OnceLock, RwLock}; use std::{fs::File, io::Write}; @@ -33,6 +33,7 @@ use tempfile::{Builder, TempDir}; use tracing::{debug, info, instrument}; use crate::errors::{TmpPostgrustError, TmpPostgrustResult}; +use crate::search::PostgresBinaries; const TMP_POSTGRUST_DB_NAME: &str = "tmp-postgrust"; const TMP_POSTGRUST_USER_NAME: &str = "tmp-postgrust-user"; @@ -78,6 +79,7 @@ pub fn new_default_process() -> TmpPostgrustResult { RwLock::new(Some( TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { disable_fsync: true, + ..Default::default() }) .expect("Failed to initialize default postgres factory."), )) @@ -111,6 +113,7 @@ pub fn new_default_process_with_migrations( let factory_mutex = DEFAULT_POSTGRES_FACTORY.get_or_init(|| { let factory = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { disable_fsync: true, + ..Default::default() }) .expect("Failed to initialize default postgres factory."); factory @@ -153,6 +156,7 @@ pub async fn new_default_process_async() -> TmpPostgrustResult, config: String, next_port: AtomicU32, + binaries: PostgresBinaries, } /// Configuration for the `TmpPostgrustFactory`. #[derive(Default, Debug)] +#[non_exhaustive] pub struct TmpPostgrustFactoryConfig { /// Disable fsync this will speed up unit tests in exchange for /// not guaranteeing that files will be written if postgresql /// crashes. pub disable_fsync: bool, + /// Directory containing the postgres binaries (`postgres`, `initdb`, + /// `createdb`, `createuser`). When `None`, binaries are resolved from + /// `$PATH` and common install locations. + pub postgresql_bin_dir: Option, } impl TmpPostgrustFactory { @@ -255,6 +266,8 @@ impl TmpPostgrustFactory { pub fn try_new( factory_config: &TmpPostgrustFactoryConfig, ) -> TmpPostgrustResult { + let binaries = PostgresBinaries::resolve(factory_config.postgresql_bin_dir.as_deref())?; + let socket_dir = Builder::new() .prefix("tmp-postgrust-socket") .tempdir() @@ -266,7 +279,7 @@ impl TmpPostgrustFactory { synchronous::chown_to_non_root(cache_dir.path())?; synchronous::chown_to_non_root(socket_dir.path())?; - synchronous::exec_init_db(cache_dir.path())?; + synchronous::exec_init_db(&binaries.initdb, cache_dir.path())?; let config = TmpPostgrustFactory::build_config(factory_config, socket_dir.path()); @@ -275,10 +288,17 @@ impl TmpPostgrustFactory { cache_dir: Arc::new(cache_dir), config, next_port: AtomicU32::new(5432), + binaries, }; let process = factory.start_postgresql(&factory.cache_dir)?; - synchronous::exec_create_user(process.socket_dir.path(), process.port, &process.user_name)?; + synchronous::exec_create_user( + &factory.binaries.createuser, + process.socket_dir.path(), + process.port, + &process.user_name, + )?; synchronous::exec_create_db( + &factory.binaries.createdb, process.socket_dir.path(), process.port, &process.user_name, @@ -294,6 +314,8 @@ impl TmpPostgrustFactory { pub async fn try_new_async( factory_config: &TmpPostgrustFactoryConfig, ) -> TmpPostgrustResult { + let binaries = PostgresBinaries::resolve(factory_config.postgresql_bin_dir.as_deref())?; + let socket_dir = Builder::new() .prefix("tmp-postgrust-socket") .tempdir() @@ -305,7 +327,7 @@ impl TmpPostgrustFactory { asynchronous::chown_to_non_root(cache_dir.path()).await?; asynchronous::chown_to_non_root(socket_dir.path()).await?; - asynchronous::exec_init_db(cache_dir.path()).await?; + asynchronous::exec_init_db(&binaries.initdb, cache_dir.path()).await?; let config = TmpPostgrustFactory::build_config(factory_config, socket_dir.path()); @@ -314,11 +336,18 @@ impl TmpPostgrustFactory { cache_dir: Arc::new(cache_dir), config, next_port: AtomicU32::new(5432), + binaries, }; let process = factory.start_postgresql_async(&factory.cache_dir).await?; - asynchronous::exec_create_user(process.socket_dir.path(), process.port, &process.user_name) - .await?; + asynchronous::exec_create_user( + &factory.binaries.createuser, + process.socket_dir.path(), + process.port, + &process.user_name, + ) + .await?; asynchronous::exec_create_db( + &factory.binaries.createdb, process.socket_dir.path(), process.port, &process.user_name, @@ -364,7 +393,7 @@ impl TmpPostgrustFactory { Result<(), Box>, >, { - let process = self.start_postgresql(&self.cache_dir)?; + let process = self.start_postgresql_async(&self.cache_dir).await?; migrate(&process.connection_string()) .await @@ -412,7 +441,8 @@ impl TmpPostgrustFactory { .fetch_add(1, std::sync::atomic::Ordering::SeqCst); synchronous::chown_to_non_root(dir.path())?; - let mut postgres_process_handle = synchronous::start_postgres_subprocess(dir.path(), port)?; + let mut postgres_process_handle = + synchronous::start_postgres_subprocess(&self.binaries.postgres, dir.path(), port)?; let stdout = postgres_process_handle.stdout.take().unwrap(); let stderr = postgres_process_handle.stderr.take().unwrap(); @@ -487,7 +517,7 @@ impl TmpPostgrustFactory { asynchronous::chown_to_non_root(dir.path()).await?; let mut postgres_process_handle = - asynchronous::start_postgres_subprocess(dir.path(), port)?; + asynchronous::start_postgres_subprocess(&self.binaries.postgres, dir.path(), port)?; let stdout = postgres_process_handle.stdout.take().unwrap(); let stderr = postgres_process_handle.stderr.take().unwrap(); @@ -528,6 +558,7 @@ mod tests { async fn it_works() { let factory = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { disable_fsync: false, + ..Default::default() }) .expect("failed to create factory"); @@ -552,6 +583,7 @@ mod tests { async fn it_works_fsync_disabled() { let factory = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { disable_fsync: true, + ..Default::default() }) .expect("failed to create factory"); @@ -577,6 +609,7 @@ mod tests { async fn it_works_async() { let factory = TmpPostgrustFactory::try_new_async(&TmpPostgrustFactoryConfig { disable_fsync: false, + ..Default::default() }) .await .expect("failed to create factory"); @@ -603,6 +636,7 @@ mod tests { async fn two_simulatenous_processes() { let factory = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { disable_fsync: false, + ..Default::default() }) .expect("failed to create factory"); @@ -643,6 +677,7 @@ mod tests { async fn two_simulatenous_processes_async() { let factory = TmpPostgrustFactory::try_new_async(&TmpPostgrustFactoryConfig { disable_fsync: false, + ..Default::default() }) .await .expect("failed to create factory"); @@ -756,4 +791,53 @@ mod tests { // Chance to catch concurrent tests or database that have already been used. client.execute("CREATE TABLE lock ();", &[]).await.unwrap(); } + + #[test(tokio::test)] + async fn explicit_bin_dir_works() { + let postgres_path = crate::search::find_postgresql_command(None, "postgres").expect( + "cannot derive bin_dir: postgres not found via PATH or known install locations", + ); + let bin_dir = postgres_path + .parent() + .expect("postgres path has no parent dir") + .to_owned(); + + let factory = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { + disable_fsync: true, + postgresql_bin_dir: Some(bin_dir), + }) + .expect("failed to create factory with explicit bin dir"); + + let proc = factory + .new_instance() + .expect("failed to create a new instance"); + + let (client, conn) = tokio_postgres::connect(&proc.connection_string(), NoTls) + .await + .expect("failed to connect to postgresql"); + + tokio::spawn(async move { + if let Err(e) = conn.await { + error!("connection error: {}", e); + } + }); + + client.query("SELECT 1;", &[]).await.unwrap(); + } + + #[test] + fn bogus_bin_dir_returns_error() { + let result = TmpPostgrustFactory::try_new(&TmpPostgrustFactoryConfig { + disable_fsync: true, + postgresql_bin_dir: Some(std::path::PathBuf::from("/nonexistent/bin/dir")), + }); + assert!( + matches!( + result, + Err(TmpPostgrustError::PostgresCommandNotFound { .. }) + ), + "expected PostgresCommandNotFound, got: {:?}", + result + ); + } } diff --git a/src/search.rs b/src/search.rs index f2f034b..7930354 100644 --- a/src/search.rs +++ b/src/search.rs @@ -5,7 +5,7 @@ use which::which; use crate::errors::{TmpPostgrustError, TmpPostgrustResult}; -/// Addtional file system locations to search for binaries +/// Additional file system locations to search for binaries /// if `initdb` and `postgres` are not in the $PATH. const SEARCH_PATHS: [&str; 5] = [ "/usr/local/pgsql", @@ -15,7 +15,25 @@ const SEARCH_PATHS: [&str; 5] = [ "/opt/local/lib/postgresql*", ]; -pub(crate) fn find_postgresql_command(dir: &str, name: &str) -> Result { +/// Locate a postgres binary by name. +/// +/// If `bin_dir` is `Some`, only that directory is searched, with no `$PATH` fallback. +/// If `bin_dir` is `None`, `$PATH` is tried first, then well-known install locations are tried as a fallback. +pub(crate) fn find_postgresql_command( + bin_dir: Option<&Path>, + name: &str, +) -> TmpPostgrustResult { + if let Some(dir) = bin_dir { + // N.B. effectively this "." parameter doesn't do anything since we + // never pass in any relative paths, only command names. + return which::which_in(name, Some(dir), std::path::Path::new(".")).map_err(|_| { + TmpPostgrustError::PostgresCommandNotFound { + command: name.to_string(), + searched_dir: Some(dir.to_owned()), + } + }); + } + // Use binaries from $PATH if available. if let Ok(path) = which(name) { return Ok(path); @@ -23,7 +41,7 @@ pub(crate) fn find_postgresql_command(dir: &str, name: &str) -> Result Result) -> TmpPostgrustResult { + Ok(Self { + postgres: find_postgresql_command(bin_dir, "postgres")?, + initdb: find_postgresql_command(bin_dir, "initdb")?, + createdb: find_postgresql_command(bin_dir, "createdb")?, + createuser: find_postgresql_command(bin_dir, "createuser")?, + }) + } } /// Return a tuple of directory paths and other paths in a sub path by recursing through diff --git a/src/synchronous.rs b/src/synchronous.rs index 5c38d95..81d5499 100644 --- a/src/synchronous.rs +++ b/src/synchronous.rs @@ -21,7 +21,6 @@ use tracing::{debug, instrument}; use crate::errors::{ProcessCapture, TmpPostgrustError, TmpPostgrustResult}; use crate::search::all_dir_entries; use crate::search::build_copy_dst_path; -use crate::search::find_postgresql_command; use crate::POSTGRES_UID_GID; #[instrument(skip(command, fail))] @@ -53,13 +52,11 @@ fn exec_process( #[instrument] pub(crate) fn start_postgres_subprocess( + postgres_bin: &Path, data_directory: &Path, port: u32, ) -> TmpPostgrustResult { - let postgres_path = - find_postgresql_command("bin", "postgres").expect("failed to find postgres"); - - let mut command = Command::new(postgres_path); + let mut command = Command::new(postgres_bin); command .env("PGDATA", data_directory.to_str().unwrap()) .arg("-p") @@ -73,12 +70,10 @@ pub(crate) fn start_postgres_subprocess( } #[instrument] -pub(crate) fn exec_init_db(data_directory: &Path) -> TmpPostgrustResult<()> { - let initdb_path = find_postgresql_command("bin", "initdb").expect("failed to find initdb"); - +pub(crate) fn exec_init_db(initdb_bin: &Path, data_directory: &Path) -> TmpPostgrustResult<()> { debug!("Initializing database in: {:?}", data_directory); - let mut command = Command::new(initdb_path); + let mut command = Command::new(initdb_bin); command .env("PGDATA", data_directory.to_str().unwrap()) .arg("--username=postgres"); @@ -125,12 +120,13 @@ pub(crate) fn chown_to_non_root(dir: &Path) -> TmpPostgrustResult<()> { #[instrument] pub(crate) fn exec_create_db( + createdb_bin: &Path, socket: &Path, port: u32, owner: &str, dbname: &str, ) -> TmpPostgrustResult<()> { - let mut command = Command::new("createdb"); + let mut command = Command::new(createdb_bin); command .arg("-h") .arg(socket) @@ -147,8 +143,13 @@ pub(crate) fn exec_create_db( } #[instrument] -pub(crate) fn exec_create_user(socket: &Path, port: u32, username: &str) -> TmpPostgrustResult<()> { - let mut command = Command::new("createuser"); +pub(crate) fn exec_create_user( + createuser_bin: &Path, + socket: &Path, + port: u32, + username: &str, +) -> TmpPostgrustResult<()> { + let mut command = Command::new(createuser_bin); command .arg("-h") .arg(socket)