Hi, Reviving this thread [1] on the subject for PG20.
Currently, pg_upgrade fails when it encounters an invalid database (commit c66a7d75e65, a database whose DROP DATABASE got interrupted) in the source cluster, blocking the whole upgrade workflow. In the previous thread, various options were discussed such as skip or drop, and commit f638aafd1ea tightened checking and error reporting for invalid databases. I think we can do better here. If a user decided to DROP a database, the data in it is no longer wanted. Even if it were, the data in the files is unlikely to be consistent or useful, since DROP was interrupted mid-operation (while evicting buffer pool pages, unlinking files, etc.). I would like to propose an option to skip the invalid databases, with the default being on. This helps unblock upgrade workflows while still preserving them for users who think it is necessary. Please find the attached patch doing this. Dropping the invalid databases during the upgrade is another approach, but it could be costly, especially with large buffer pools and a large number of files to unlink. Skipping them instead is simpler, and the old directory contents would be cleaned up by the removal script that pg_upgrade already generates. Thoughts? Thanks to Nathan Bossart and Robert Treat for earlier discussions and design review. [1] https://postgr.es/m/f9315bf0-e03e-4490-9f0d-5b6f7a6d9908%40postsubmeta.net -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 6341415824448b51d1fd9b22ba0bc6105a22ac14 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Sat, 29 Aug 2026 12:44:24 +0000 Subject: [PATCH v1] Teach pg_upgrade to deal with invalid databases. Previously, pg_upgrade failed when it encountered an invalid database (commit c66a7d75e65, a database whose DROP DATABASE got interrupted) in the source cluster, blocking the whole upgrade workflow. Commit f638aafd1ea tightened checking and error reporting for invalid databases. This commit adds an --invalid-databases option, with the default being to skip. With skip, invalid databases are reported and left out of the upgrade; error retains the previous behavior of failing. Skipping is reasonable because if a user decided to DROP a database, the data in it is no longer wanted. Even if it were, the data in the files is unlikely to be consistent or useful, since DROP was interrupted mid-operation (while evicting buffer pool pages, unlinking files, etc.). Dropping invalid databases during upgrade could be costly, especially with large buffer pools and a large number of files to unlink. Skipping them instead is simpler, and the old directory contents are cleaned up by the removal script that pg_upgrade already generates. In the future, this could be extended to actively drop invalid databases during the upgrade. Author: Bharath Rupireddy <[email protected]> Discussion: https://postgr.es/m/ --- doc/src/sgml/ref/pgupgrade.sgml | 24 ++++++++++++++ src/bin/pg_upgrade/check.c | 46 ++++++++++++++++++-------- src/bin/pg_upgrade/info.c | 2 ++ src/bin/pg_upgrade/option.c | 13 ++++++++ src/bin/pg_upgrade/pg_upgrade.h | 10 ++++++ src/bin/pg_upgrade/t/002_pg_upgrade.pl | 35 ++++++++++++++++---- src/tools/pgindent/typedefs.list | 1 + 7 files changed, 112 insertions(+), 19 deletions(-) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index e4e8c02e6d6..8cd9f13b6dd 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -262,6 +262,30 @@ PostgreSQL documentation </listitem> </varlistentry> + <varlistentry> + <term><option>--invalid-databases=</option><replaceable>option</replaceable></term> + <listitem> + <para> + Choose how to treat invalid databases in the old cluster, that is + databases whose <command>DROP DATABASE</command> was interrupted, + leaving them with <structfield>datconnlimit</structfield> set to + <literal>-2</literal> in + <link linkend="catalog-pg-database"><structname>pg_database</structname></link>. + Such a database can no longer be connected to and can never be + upgraded. Possible values are <literal>skip</literal> (the default) + and <literal>error</literal>. + </para> + <para> + With <literal>skip</literal>, invalid databases are reported and left + behind while the rest of the cluster is upgraded; their files are + removed along with the old cluster by the generated + <filename>delete_old_cluster</filename> script. With + <literal>error</literal>, invalid databases are reported and the + upgrade is aborted. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><option>--no-statistics</option></term> <listitem> diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 41342561763..ef9e34f1e7e 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -1068,7 +1068,10 @@ check_is_install_user(ClusterInfo *cluster) * * Ensure that all non-template0 databases allow connections since they * otherwise won't be restored; and that template0 explicitly doesn't allow - * connections since it would make pg_dumpall --globals restore fail. + * connections since it would make pg_dumpall --globals restore fail. Invalid + * databases (whose DROP DATABASE was interrupted) are skipped by default and + * reported here; with --invalid-databases=error they abort the upgrade + * instead. */ static void check_for_connection_status(ClusterInfo *cluster) @@ -1081,6 +1084,7 @@ check_for_connection_status(ClusterInfo *cluster) int i_datallowconn; int i_datconnlimit; FILE *script = NULL; + bool skipped_invalid = false; char output_path[MAXPGPATH]; prep_status("Checking database connection settings"); @@ -1114,20 +1118,36 @@ check_for_connection_status(ClusterInfo *cluster) pg_fatal("template0 must not allow connections, " "i.e. its pg_database.datallowconn must be false"); } - else + + /* + * Skip invalid databases unless the user asked us to treat them as an + * error. They are left out of the upgrade, and their files are + * removed along with the rest of the old cluster by the generated + * delete_old_cluster script. + */ + else if (strcmp(datconnlimit, "-2") == 0 && + user_opts.invalid_db_mode == INVALID_DB_SKIP) { - /* - * Avoid datallowconn == false databases from being skipped on - * restore, and ensure that no databases are marked invalid with - * datconnlimit == -2. - */ - if ((strcmp(datallowconn, "f") == 0) || strcmp(datconnlimit, "-2") == 0) + if (!skipped_invalid) { - if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) - pg_fatal("could not open file \"%s\": %m", output_path); - - fprintf(script, "%s\n", datname); + report_status(PG_WARNING, "warning"); + skipped_invalid = true; } + pg_log(PG_WARNING, + "invalid database \"%s\" will not be upgraded", datname); + } + + /* + * Databases that disallow connections would be silently skipped on + * restore, and invalid databases in --invalid-databases=error mode + * must not be carried over, so both block the upgrade. + */ + else if (strcmp(datallowconn, "f") == 0 || strcmp(datconnlimit, "-2") == 0) + { + if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL) + pg_fatal("could not open file \"%s\": %m", output_path); + + fprintf(script, "%s\n", datname); } } @@ -1147,7 +1167,7 @@ check_for_connection_status(ClusterInfo *cluster) "connections. A list of databases with the problem is in the file:\n" " %s", output_path); } - else + else if (!skipped_invalid) check_ok(); } diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index 37fff93892f..09f0b739f26 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -423,6 +423,8 @@ get_db_infos(ClusterInfo *cluster) " LEFT OUTER JOIN pg_catalog.pg_tablespace t " " ON d.dattablespace = t.oid " "WHERE d.datallowconn = true " + /* skip invalid databases, whose DROP DATABASE was interrupted */ + " AND d.datconnlimit <> -2 " "ORDER BY 1"); res = executeQueryOrDie(conn, "%s", query); diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index f01d2f92d95..5ef08e23ffd 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -63,6 +63,7 @@ parseCommandLine(int argc, char *argv[]) {"no-statistics", no_argument, NULL, 5}, {"set-char-signedness", required_argument, NULL, 6}, {"swap", no_argument, NULL, 7}, + {"invalid-databases", required_argument, NULL, 8}, {NULL, 0, NULL, 0} }; @@ -75,6 +76,7 @@ parseCommandLine(int argc, char *argv[]) user_opts.transfer_mode = TRANSFER_MODE_COPY; user_opts.do_statistics = true; user_opts.char_signedness = -1; + user_opts.invalid_db_mode = INVALID_DB_SKIP; os_info.progname = get_progname(argv[0]); @@ -234,6 +236,15 @@ parseCommandLine(int argc, char *argv[]) user_opts.transfer_mode = TRANSFER_MODE_SWAP; break; + case 8: + if (pg_strcasecmp(optarg, "skip") == 0) + user_opts.invalid_db_mode = INVALID_DB_SKIP; + else if (pg_strcasecmp(optarg, "error") == 0) + user_opts.invalid_db_mode = INVALID_DB_ERROR; + else + pg_fatal("invalid argument for option %s", "--invalid-databases"); + break; + default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), os_info.progname); @@ -328,6 +339,8 @@ usage(void) printf(_(" --clone clone instead of copying files to new cluster\n")); printf(_(" --copy copy files to new cluster (default)\n")); printf(_(" --copy-file-range copy files to new cluster with copy_file_range\n")); + printf(_(" --invalid-databases=OPTION how to treat invalid databases, \"skip\"\n" + " (default) or \"error\"\n")); printf(_(" --no-statistics do not import statistics from old cluster\n")); printf(_(" --set-char-signedness=OPTION set new cluster char signedness to \"signed\" or\n" " \"unsigned\"\n")); diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h index c80e8fb4031..b73bb4021ed 100644 --- a/src/bin/pg_upgrade/pg_upgrade.h +++ b/src/bin/pg_upgrade/pg_upgrade.h @@ -248,6 +248,15 @@ typedef enum TRANSFER_MODE_SWAP, } transferMode; +/* + * Enumeration to denote how to treat invalid databases in the old cluster + */ +typedef enum +{ + INVALID_DB_SKIP, /* skip them, don't upgrade them (default) */ + INVALID_DB_ERROR, /* report them and abort the upgrade */ +} invalidDbMode; + /* * Enumeration to denote pg_log modes */ @@ -325,6 +334,7 @@ typedef struct int char_signedness; /* default char signedness: -1 for initial * value, 1 for "signed" and 0 for * "unsigned" */ + invalidDbMode invalid_db_mode; /* how to treat invalid databases */ } UserOpts; typedef struct diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl index 0a4121fdc4d..647a8ce5ce9 100644 --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -513,14 +513,17 @@ 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.) +# Check how pg_upgrade handles an invalid database. With +# --invalid-databases=error it is reported and the upgrade aborts; by default +# it is skipped and the upgrade proceeds. (Versions that were out of support +# by commit c66a7d75e652 can't mark a database invalid, so skip these checks +# there.) SKIP: { - skip "database invalidation not implemented", 1 + skip "database invalidation not implemented", 2 if $oldnode->pg_version < 11; + # --invalid-databases=error preserves the historical hard failure. command_checks_all( [ 'pg_upgrade', '--no-sync', @@ -531,12 +534,32 @@ SKIP: '--socketdir' => $newnode->host, '--old-port' => $oldnode->port, '--new-port' => $newnode->port, - $mode, '--check', + $mode, '--check', '--invalid-databases' => 'error', ], 1, [qr/datconnlimit/], [qr/^$/], - 'invalid database causes failure'); + 'invalid database causes failure with --invalid-databases=error'); + rmtree($newnode->data_dir . "/pg_upgrade_output.d"); + + # The default skips invalid databases, so --check succeeds while naming + # the database that will not be upgraded. + command_checks_all( + [ + 'pg_upgrade', '--no-sync', + '--old-datadir' => $oldnode->data_dir, + '--new-datadir' => $newnode->data_dir, + '--old-bindir' => $oldbindir, + '--new-bindir' => $newbindir, + '--socketdir' => $newnode->host, + '--old-port' => $oldnode->port, + '--new-port' => $newnode->port, + $mode, '--check', + ], + 0, + [qr/invalid database "regression_invalid" will not be upgraded/], + [qr/^$/], + 'invalid database is skipped by default'); rmtree($newnode->data_dir . "/pg_upgrade_output.d"); } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 15b10e1703f..5177d340dfa 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3913,6 +3913,7 @@ intset_internal_node intset_leaf_node intset_node intvKEY +invalidDbMode io_callback_fn io_stat_col itemIdCompact -- 2.47.3
