From d733fcaf5cac6b88de9417f2b43e51490e36c7b1 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 14 Oct 2026 15:49:48 +0300 Subject: [PATCH] Fix order of bin and data dir checks in pg_upgrade If you called pg_upgrade with a --new-datadir that points to a configuration-only dir and no --new-bindir option, it would try to execute "postgres -C data_directory" using the new binary before resolving the correct path to it. To fix, check and resolve the binary dirs first, before calling adjust_data_dir(). For clarity, rename the setup() function to check_clusters_stopped() to better reflect what it does now. One consequence of this is that if the binaries are not found, you now get an error earlier than before, before the pg_upgrade_output.d directory and the log files have been created. That's what necessitated the test change in this commit. The log files are important for things like listing unsupported objects in the source database, but missing binaries feels like a more permanent error, similar to invalid options or incorrect directory permissions, and it seems OK to just print the error to stderr for those. This has been broken forever, but I chose to not backpatch since no one else has complained, it's a pretty minor issue, and it's easy to work around by passing the binary dir explicitly. Maybe later, if someone in the field runs into this. Reviewed-by: Chao Li Reviewed-by: Bohyun Lee Discussion: https://www.postgresql.org/message-id/6894f355-10e4-4756-89ad-ae5987f416a6@iki.fi --- src/bin/pg_upgrade/exec.c | 21 +++------------ src/bin/pg_upgrade/option.c | 7 ++--- src/bin/pg_upgrade/pg_upgrade.c | 36 +++++++++++++++++++------- src/bin/pg_upgrade/pg_upgrade.h | 4 ++- src/bin/pg_upgrade/t/002_pg_upgrade.pl | 31 +++++----------------- 5 files changed, 44 insertions(+), 55 deletions(-) diff --git a/src/bin/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c index a1bdbf373e3..c32b7a94207 100644 --- a/src/bin/pg_upgrade/exec.c +++ b/src/bin/pg_upgrade/exec.c @@ -15,8 +15,6 @@ #include "fe_utils/version.h" #include "pg_upgrade.h" -static void check_data_dir(ClusterInfo *cluster); -static void check_bin_dir(ClusterInfo *cluster, bool check_versions); static void get_bin_version(ClusterInfo *cluster); static void check_exec(const char *dir, const char *program, bool check_version); @@ -244,15 +242,10 @@ pid_lock_file_exists(const char *datadir) /* - * verify_directories() - * - * does all the hectic work of verifying directories and executables - * of old and new server. - * - * NOTE: May update the values of all parameters + * Check that the current directory is writable */ void -verify_directories(void) +check_current_dir_write_permissions(void) { #ifndef WIN32 if (access(".", R_OK | W_OK | X_OK) != 0) @@ -260,14 +253,8 @@ verify_directories(void) if (win32_check_directory_write_permissions() != 0) #endif pg_fatal("You must have read and write access in the current directory."); - - check_bin_dir(&old_cluster, false); - check_data_dir(&old_cluster); - check_bin_dir(&new_cluster, true); - check_data_dir(&new_cluster); } - #ifdef WIN32 /* * win32_check_directory_write_permissions() @@ -329,7 +316,7 @@ check_single_dir(const char *pg_data, const char *subdir) * us) we display an error message and exit() * */ -static void +void check_data_dir(ClusterInfo *cluster) { const char *pg_data = cluster->pgdata; @@ -361,7 +348,7 @@ check_data_dir(ClusterInfo *cluster) * against the version of this pg_upgrade. This is for checking the target * bindir. */ -static void +void check_bin_dir(ClusterInfo *cluster, bool check_versions) { struct stat statBuf; diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index f01d2f92d95..179eaf0d5e6 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -422,9 +422,10 @@ check_required_directory(char **dirpath, const char *envVarName, bool useCwd, * by querying the running server. This has limited checking because we * can't check for a running server because we can't find postmaster.pid. * - * On entry, cluster->pgdata has been set from command line or env variable, - * but cluster->pgconfig isn't set. We fill both variables with corrected - * values. + * On entry, cluster->bindir has already been set, and cluster->pgdata has + * been initially set from the command line or env variable. We fill + * cluster->pgconfig, and adjust cluster->pgdata with a corrected value if the + * real data directory is in a different location. */ void adjust_data_dir(ClusterInfo *cluster) diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index c0fadb3f317..6d01084e4b0 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -66,7 +66,8 @@ static void create_new_objects(void); static void copy_xact_xlog_xid(void); static void set_frozenxids(void); static void make_outputdirs(char *pgdata); -static void setup(char *argv0); +static void check_bin_dirs(char *argv0); +static void check_clusters_stopped(void); static void create_logical_replication_slots(void); static void create_conflict_detection_slot(void); @@ -106,6 +107,12 @@ main(int argc, char **argv) get_restricted_token(); + /* + * Resolve old and new binary dirs before adjust_data_dir(), because + * adjust_data_dir() might need to call the binaries. + */ + check_bin_dirs(argv[0]); + adjust_data_dir(&old_cluster); adjust_data_dir(&new_cluster); @@ -125,7 +132,11 @@ main(int argc, char **argv) */ make_outputdirs(new_cluster.pgdata); - setup(argv[0]); + check_current_dir_write_permissions(); + + check_data_dir(&old_cluster); + check_data_dir(&new_cluster); + check_clusters_stopped(); output_check_banner(); @@ -359,14 +370,8 @@ make_outputdirs(char *pgdata) static void -setup(char *argv0) +check_bin_dirs(char *argv0) { - /* - * make sure the user has a clean environment, otherwise, we may confuse - * libpq when we connect to one (or both) of the servers. - */ - check_pghost_envvar(); - /* * In case the user hasn't specified the directory for the new binaries * with -B, default to using the path of the currently executed pg_upgrade @@ -384,7 +389,18 @@ setup(char *argv0) new_cluster.bindir = pg_strdup(exec_path); } - verify_directories(); + check_bin_dir(&old_cluster, false); + check_bin_dir(&new_cluster, true); +} + +static void +check_clusters_stopped(void) +{ + /* + * make sure the user has a clean environment, otherwise, we may confuse + * libpq when we connect to one (or both) of the servers. + */ + check_pghost_envvar(); /* no postmasters should be running, except for a live check */ if (pid_lock_file_exists(old_cluster.pgdata)) diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index 4cb79bb780e..30a2e263d2d 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -391,7 +391,9 @@ void generate_old_dump(void); bool exec_prog(const char *log_filename, const char *opt_log_file, bool report_error, bool exit_on_error, const char *fmt, ...) pg_attribute_printf(5, 6); -void verify_directories(void); +void check_current_dir_write_permissions(void); +void check_bin_dir(ClusterInfo *cluster, bool check_versions); +void check_data_dir(ClusterInfo *cluster); bool pid_lock_file_exists(const char *datadir); diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl index 9590625fc32..be57b2d838c 100644 --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -561,35 +561,12 @@ $oldnode->safe_psql( # Upgrade the instance. $oldnode->stop; -# Cause a failure at the start of pg_upgrade, this should create the logging -# directory pg_upgrade_output.d but leave it around. Keep --check for an -# early exit. -command_checks_all( - [ - 'pg_upgrade', '--no-sync', - '--old-datadir' => $oldnode->data_dir, - '--new-datadir' => $newnode->data_dir, - '--old-bindir' => $oldbindir . '/does/not/exist/', - '--new-bindir' => $newbindir, - '--socketdir' => $newnode->host, - '--old-port' => $oldnode->port, - '--new-port' => $newnode->port, - $mode, '--check', - ], - 1, - [qr{check for ".*?does/not/exist" failed}], - [], - 'run of pg_upgrade --check for new instance with incorrect binary path'); -ok(-d $newnode->data_dir . "/pg_upgrade_output.d", - "pg_upgrade_output.d/ not removed after pg_upgrade failure"); -rmtree($newnode->data_dir . "/pg_upgrade_output.d"); - # Check that pg_upgrade aborts when encountering an invalid database # (However, versions that were out of support by commit c66a7d75e652 don't # know how to do this, so skip this test there.) SKIP: { - skip "database invalidation not implemented", 1 + skip "database invalidation not implemented", 4 if $oldnode->pg_version < 11; command_checks_all( @@ -608,6 +585,12 @@ SKIP: [qr/datconnlimit/], [qr/^$/], 'invalid database causes failure'); + + # Check that the failure creates the logging directory pg_upgrade_output.d + # and leaves it around + ok(-d $newnode->data_dir . "/pg_upgrade_output.d", + "pg_upgrade_output.d/ not removed after pg_upgrade failure"); + rmtree($newnode->data_dir . "/pg_upgrade_output.d"); }