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"); }