Hi, Here is a first version of the patch.
pg_upgrade now creates a file named pg_upgrade_in_progress in the new cluster's data directory right after the consistency checks, before it first modifies the new cluster (so --check never creates it), and removes it after the last step that starts the new cluster. Both the creation and the removal are fsynced. checkDataDir() refuses to start the server while the file exists, unless IsBinaryUpgrade is set. pg_upgrade always starts its servers with -b, so its own startups are unaffected. Doing the check in checkDataDir() means it runs before postmaster.pid is created or anything else is written, and it also covers single-user mode. If pg_upgrade fails or is interrupted, the file stays in place and a startup attempt fails with: FATAL: data directory "..." is being upgraded by pg_upgrade DETAIL: File ".../pg_upgrade_in_progress" exists. HINT: Wait for pg_upgrade to complete. If pg_upgrade failed, the new cluster must be re-created with initdb. Testing: - The reproduction from my previous mail (suspending pg_upgrade during "Copying user relation files" and starting the target with pg_ctl) now fails with the error above, and the upgrade completes normally afterwards. - Killing pg_upgrade during file transfer leaves the marker in place, and the new cluster cannot be started. - 002_pg_upgrade.pl now checks that --check does not create the marker, that a successful upgrade removes it, and that the server refuses to start while it exists. - check-world passes, including all transfer modes in 006_transfer_modes.pl. Open points: 1. The marker is fsynced even with --no-sync, since it is cheap and safety-related. Happy to change that if preferred. 2. The #define lives in access/xlog_internal.h next to XLOG_CONTROL_FILE, since it has to be usable from frontend code. The file name itself is of course open for discussion. 3. The rsync/standby case Nik mentioned is not handled; keeping the marker after completion could be a follow-up option. Regards, Shubhra >
From e767dd37e780ccce99ddcc56466c1733b8f9fcef Mon Sep 17 00:00:00 2001 From: Shubhra Jain <[email protected]> Date: Tue, 13 Oct 2026 17:03:32 +0530 Subject: [PATCH v1] pg_upgrade: Prevent premature startup of the new cluster pg_upgrade checks only once, at the beginning, that the new cluster is not running. Nothing prevented the new cluster from being started while pg_upgrade was still modifying it, for example during file transfer, pg_resetwal or the final sync, regardless of transfer mode. A server started in that window can write WAL, consume transaction IDs and modify files that pg_upgrade is still transferring, while pg_upgrade may still report success, leading to silent corruption. To prevent this, pg_upgrade now creates a file named pg_upgrade_in_progress in the new cluster's data directory before it first modifies the cluster, and removes it only after the upgrade has completed. Both the creation and the removal are fsynced. While the file exists, the server refuses to start, except in binary-upgrade mode, which pg_upgrade uses for the servers it starts itself. If pg_upgrade fails or is interrupted, the file is left in place, so that a partially upgraded cluster cannot be started by accident. Author: Shubhra Jain <[email protected]> Suggested-by: Nikolay Samokhvalov <[email protected]> Discussion: https://postgr.es/m/CAM527d_MAVFE8RVM_Qr8-P4opUSxemY2r=meeiv+odtu6dw...@mail.gmail.com --- doc/src/sgml/ref/pgupgrade.sgml | 12 ++++ src/backend/utils/init/miscinit.c | 28 ++++++++ src/bin/pg_upgrade/pg_upgrade.c | 88 ++++++++++++++++++++++++++ src/bin/pg_upgrade/t/002_pg_upgrade.pl | 15 +++++ src/include/access/xlog_internal.h | 8 +++ 5 files changed, 151 insertions(+) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index e4e8c02e6d..f5aaf6586e 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -957,6 +957,18 @@ psql --username=postgres --file=script.sql postgres </para> </listitem> </itemizedlist></para> + <para> + Once <application>pg_upgrade</application> starts modifying the new + cluster, it creates the file <filename>pg_upgrade_in_progress</filename> + in the new cluster's data directory, and removes it only after the + upgrade has completed successfully. While this file exists, the new + cluster cannot be started, which prevents an accidental startup from + modifying the new cluster while <application>pg_upgrade</application> + is still working on it. If <application>pg_upgrade</application> fails + or is interrupted, the file is left in place, and the new cluster must + be re-created with <application>initdb</application> before the upgrade + is attempted again. + </para> </step> </procedure> diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index eddce1ce33..fea1494787 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -30,6 +30,7 @@ #include "access/htup_details.h" #include "access/parallel.h" +#include "access/xlog_internal.h" #include "catalog/pg_authid.h" #include "common/file_perm.h" #include "libpq/libpq.h" @@ -297,6 +298,7 @@ void checkDataDir(void) { struct stat stat_buf; + char upgrade_file[MAXPGPATH]; Assert(DataDir); @@ -380,6 +382,32 @@ checkDataDir(void) /* Check for PG_VERSION */ ValidatePgVersion(DataDir); + + /* + * Refuse to start if pg_upgrade is in the middle of upgrading this data + * directory. pg_upgrade creates this file before it starts modifying the + * new cluster and removes it only once the cluster is safe to start. A + * server started in the meantime could write WAL, consume transaction IDs + * and modify files that pg_upgrade is still transferring, which can lead + * to silent corruption. pg_upgrade starts its own servers in + * binary-upgrade mode, so allow those. + */ + snprintf(upgrade_file, sizeof(upgrade_file), "%s/%s", + DataDir, PG_UPGRADE_IN_PROGRESS_FILE); + if (stat(upgrade_file, &stat_buf) == 0) + { + if (!IsBinaryUpgrade) + ereport(FATAL, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("data directory \"%s\" is being upgraded by pg_upgrade", + DataDir), + errdetail("File \"%s\" exists.", upgrade_file), + errhint("Wait for pg_upgrade to complete. If pg_upgrade failed, the new cluster must be re-created with initdb."))); + } + else if (errno != ENOENT) + ereport(FATAL, + (errcode_for_file_access(), + errmsg("could not stat file \"%s\": %m", upgrade_file))); } /* diff --git a/src/bin/pg_upgrade/pg_upgrade.c b/src/bin/pg_upgrade/pg_upgrade.c index c0fadb3f31..74fa135c30 100644 --- a/src/bin/pg_upgrade/pg_upgrade.c +++ b/src/bin/pg_upgrade/pg_upgrade.c @@ -41,11 +41,15 @@ #include "postgres_fe.h" +#include <fcntl.h> #include <time.h> +#include <unistd.h> #include "access/multixact.h" +#include "access/xlog_internal.h" #include "catalog/pg_class_d.h" #include "common/file_perm.h" +#include "common/file_utils.h" #include "common/logging.h" #include "common/restricted_token.h" #include "fe_utils/string_utils.h" @@ -67,6 +71,8 @@ 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 create_upgrade_in_progress_file(void); +static void remove_upgrade_in_progress_file(void); static void create_logical_replication_slots(void); static void create_conflict_detection_slot(void); @@ -150,6 +156,12 @@ main(int argc, char **argv) "Performing Upgrade\n" "------------------"); + /* + * From here on, the new cluster gets modified. Prevent it from being + * started by anything other than pg_upgrade until the upgrade completes. + */ + create_upgrade_in_progress_file(); + set_locale_and_encoding(); prepare_new_cluster(); @@ -254,6 +266,9 @@ main(int argc, char **argv) issue_warnings_and_set_wal_level(); + /* The new cluster is now safe to start. */ + remove_upgrade_in_progress_file(); + pg_log(PG_REPORT, "\n" "Upgrade Complete\n" @@ -268,6 +283,79 @@ main(int argc, char **argv) return 0; } +/* + * create_upgrade_in_progress_file() + * + * Create a marker file in the new cluster's data directory. While it exists, + * the server refuses to start unless it runs in binary-upgrade mode, as the + * servers started by pg_upgrade do (see checkDataDir()). The file and its + * directory entry are synced to disk, so the protection survives a crash. + */ +static void +create_upgrade_in_progress_file(void) +{ + char path[MAXPGPATH]; + int fd; + + /* translator: %s is the name of a file */ + prep_status("Creating \"%s\" in new cluster", PG_UPGRADE_IN_PROGRESS_FILE); + + snprintf(path, sizeof(path), "%s/%s", new_cluster.pgdata, + PG_UPGRADE_IN_PROGRESS_FILE); + + fd = open(path, O_WRONLY | O_CREAT | O_EXCL | PG_BINARY, + pg_file_create_mode); + if (fd < 0) + { + if (errno == EEXIST) + pg_fatal("file \"%s\" already exists\n" + "A previous run of pg_upgrade on the new cluster might have failed.\n" + "Re-create the new cluster with initdb and try again.", + path); + pg_fatal("could not create file \"%s\": %m", path); + } + if (close(fd) != 0) + pg_fatal("could not close file \"%s\": %m", path); + + /* Ensure the file and its directory entry are persisted to disk. */ + if (fsync_fname(path, false) != 0) + pg_fatal("could not synchronize file \"%s\": %m", path); + if (fsync_fname(new_cluster.pgdata, true) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", + new_cluster.pgdata); + + check_ok(); +} + +/* + * remove_upgrade_in_progress_file() + * + * Remove the marker file created by create_upgrade_in_progress_file(), once + * the new cluster is safe to start. The removal is synced to disk, so the + * file does not reappear after a crash. + */ +static void +remove_upgrade_in_progress_file(void) +{ + char path[MAXPGPATH]; + + /* translator: %s is the name of a file */ + prep_status("Removing \"%s\" from new cluster", PG_UPGRADE_IN_PROGRESS_FILE); + + snprintf(path, sizeof(path), "%s/%s", new_cluster.pgdata, + PG_UPGRADE_IN_PROGRESS_FILE); + + if (unlink(path) != 0) + pg_fatal("could not remove file \"%s\": %m", path); + + /* Ensure the removal is persisted to disk. */ + if (fsync_fname(new_cluster.pgdata, true) != 0) + pg_fatal("could not synchronize directory \"%s\": %m", + new_cluster.pgdata); + + check_ok(); +} + /* * Create and assign proper permissions to the set of output directories * used to store any data generated internally, filling in log_opts in diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl index 9590625fc3..512dab50e5 100644 --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -632,6 +632,8 @@ command_ok( 'run of pg_upgrade --check for new instance'); ok(!-d $newnode->data_dir . "/pg_upgrade_output.d", "pg_upgrade_output.d/ removed after pg_upgrade --check success"); +ok(!-e $newnode->data_dir . "/pg_upgrade_in_progress", + "pg_upgrade_in_progress not created by pg_upgrade --check"); # Actual run, pg_upgrade_output.d is removed at the end. command_ok( @@ -649,6 +651,19 @@ command_ok( 'run of pg_upgrade for new instance'); ok( !-d $newnode->data_dir . "/pg_upgrade_output.d", "pg_upgrade_output.d/ removed after pg_upgrade success"); +ok(!-e $newnode->data_dir . "/pg_upgrade_in_progress", + "pg_upgrade_in_progress removed after pg_upgrade success"); + +# The server must refuse to start while pg_upgrade_in_progress exists. +my $upgrade_marker = $newnode->data_dir . "/pg_upgrade_in_progress"; +append_to_file($upgrade_marker, ""); +ok(!$newnode->start(fail_ok => 1), + 'server does not start while pg_upgrade_in_progress exists'); +ok( $newnode->log_contains( + qr/data directory ".*" is being upgraded by pg_upgrade/), + 'server log reports that pg_upgrade is in progress'); +unlink($upgrade_marker) + or die "could not remove \"$upgrade_marker\": $!"; $newnode->start; diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index bf609c3c70..33359a794c 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -148,6 +148,14 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader; #define XLOGDIR "pg_wal" #define XLOG_CONTROL_FILE "global/pg_control" +/* + * pg_upgrade creates this file in the new cluster's data directory while an + * upgrade is in progress. The server refuses to start while it exists, + * except in binary-upgrade mode. Path is relative to $PGDATA. + */ +#define PG_UPGRADE_IN_PROGRESS_FILE "pg_upgrade_in_progress" + + /* * These macros encapsulate knowledge about the exact layout of XLog file * names, timeline history file names, and archive-status file names. -- 2.43.0
