Hi, On Tue, Sep 22, 2026 at 9:10 AM Bharath Rupireddy <[email protected]> wrote: > > Even if core drops them automatically, having pg_upgrade deal with > them is useful on its own IMHO, since the upgrade could start before > that happens. So, I prefer pg_upgrade skipping them without any > option, emitting info about the skipped ones, which will not happen > often in practice once core handles the drop.
I implemented the above approach, pg_upgrade skipping them without any option and emitting info about the skipped ones. I ensured the CI is happy. Please find the attached v2 patch. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 3f3b19b9804537efdf93ec2d233afe39b6ee23da Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Wed, 23 Sep 2026 19:04:26 +0000 Subject: [PATCH v2] Teach pg_upgrade to skip invalid databases. Previously, pg_upgrade failed when it encountered an invalid database in the old cluster, that is, a database whose DROP DATABASE got interrupted (commit c66a7d75e652). Commit f638aafd1ea8 taught the check stage to recognize such databases and report them up front, instead of failing later when trying to connect to them. The upgrade is blocked until the user intervenes. Someone has to analyze the failure, identify the issue from the logs, start the old cluster, issue DROP DATABASE on each such database, and restart the upgrade. That is time-consuming and tedious. These databases are not connectable or usable, and there is no supported recovery mechanism for them. The user has already decided to drop them, and they need to be dropped eventually anyway. Even if the data were still wanted, it is unlikely to be consistent or useful, since DROP was interrupted partway through evicting buffer pool pages, unlinking files, etc. This commit teaches pg_upgrade to skip them instead of erroring out. They are not part of the schema dump or the file transfer, so nothing of theirs reaches the new cluster in any transfer mode. Their files stay in the old cluster, which also leaves it usable as a fallback in copy and clone modes. The skipped ones are reported while checking the old cluster. This replaces an error with a skip, a behavior change, so it is also noted in the documentation. Alternatives such as an option to choose between erroring, skipping and dropping were also considered. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Nathan Bossart <[email protected]> Reviewed-by: Robert Treat <[email protected]> Reviewed-by: Andres Freund <[email protected]> Reviewed-by: Euler Taveira <[email protected]> Discussion: https://postgr.es/m/CALj2ACV3jSPaMCSRQ8XZTN_meZezNyySpMJVhVBZy_qDJK2mmQ%40mail.gmail.com --- doc/src/sgml/ref/pgupgrade.sgml | 10 +++++ src/bin/pg_upgrade/check.c | 59 +++++++++++++++++++------- src/bin/pg_upgrade/info.c | 5 ++- src/bin/pg_upgrade/t/002_pg_upgrade.pl | 57 ++++++++++--------------- 4 files changed, 80 insertions(+), 51 deletions(-) diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml index e4e8c02e6d6..547efc40953 100644 --- a/doc/src/sgml/ref/pgupgrade.sgml +++ b/doc/src/sgml/ref/pgupgrade.sgml @@ -1121,6 +1121,16 @@ psql --username=postgres --file=script.sql postgres <type>regtype</type> can be upgraded.) </para> + <para> + <application>pg_upgrade</application> skips invalid databases, that is, + databases whose <command>DROP DATABASE</command> got interrupted, leaving + their <structname>pg_database</structname>.<structfield>datconnlimit</structfield> + set to <literal>-2</literal>. Such databases are not connectable or usable, + and there is no supported recovery mechanism for them. The skipped ones are + reported while checking the old cluster. Their files stay in the old + cluster and are removed when it is deleted. + </para> + <para> If you want to use link mode and you do not want your old cluster to be modified when the new cluster is started, consider using the clone mode. diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c index 150c4fc34b9..1f19d9c7a1e 100644 --- a/src/bin/pg_upgrade/check.c +++ b/src/bin/pg_upgrade/check.c @@ -1107,6 +1107,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. + * + * Invalid databases, that is, databases whose DROP DATABASE got interrupted, + * can't be connected to and hence can't be upgraded, so they are skipped and + * reported. */ static void check_for_connection_status(ClusterInfo *cluster) @@ -1119,6 +1123,7 @@ check_for_connection_status(ClusterInfo *cluster) int i_datallowconn; int i_datconnlimit; FILE *script = NULL; + PQExpBufferData invalid_dbs; char output_path[MAXPGPATH]; prep_status("Checking database connection settings"); @@ -1138,6 +1143,8 @@ check_for_connection_status(ClusterInfo *cluster) i_datallowconn = PQfnumber(dbres, "datallowconn"); i_datconnlimit = PQfnumber(dbres, "datconnlimit"); + initPQExpBuffer(&invalid_dbs); + ntups = PQntuples(dbres); for (dbnum = 0; dbnum < ntups; dbnum++) { @@ -1152,20 +1159,26 @@ check_for_connection_status(ClusterInfo *cluster) pg_fatal("template0 must not allow connections, " "i.e. its pg_database.datallowconn must be false"); } - else + else if (strcmp(datconnlimit, "-2") == 0) + { + /* + * Collect the name of this invalid database to report below. + * Checked before datallowconn, since an invalid database can have + * datallowconn = false too, and allowing connections would still + * not make it upgradable. + */ + appendPQExpBuffer(&invalid_dbs, "\n %s", datname); + } + else if (strcmp(datallowconn, "f") == 0) { /* * Avoid datallowconn == false databases from being skipped on - * restore, and ensure that no databases are marked invalid with - * datconnlimit == -2. + * restore. */ - 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); + 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); - } + fprintf(script, "%s\n", datname); } } @@ -1178,15 +1191,25 @@ check_for_connection_status(ClusterInfo *cluster) fclose(script); pg_log(PG_REPORT, "fatal"); pg_fatal("All non-template0 databases must allow connections, i.e. their\n" - "pg_database.datallowconn must be true and pg_database.datconnlimit\n" - "must not be -2. Your installation contains non-template0 databases\n" - "which cannot be connected to. Consider allowing connection for all\n" - "non-template0 databases or drop the databases which do not allow\n" - "connections. A list of databases with the problem is in the file:\n" + "pg_database.datallowconn must be true. Your installation contains\n" + "non-template0 databases which cannot be connected to. Consider\n" + "allowing connection for all non-template0 databases or drop the\n" + "databases which do not allow connections. A list of databases with\n" + "the problem is in the file:\n" " %s", output_path); } + else if (invalid_dbs.len > 0) + { + report_status(PG_WARNING, "warning"); + pg_log(PG_WARNING, + "Your installation contains invalid databases, i.e. databases whose\n" + "DROP DATABASE was interrupted. These will not be upgraded:" + "%s", invalid_dbs.data); + } else check_ok(); + + termPQExpBuffer(&invalid_dbs); } @@ -2643,9 +2666,15 @@ check_for_oldestxid_consistency(ClusterInfo *cluster) conn_template1 = connectToServer(cluster, "template1"); + /* + * Invalid databases are skipped, and the server ignores them when it + * advances the cluster-wide oldest XID and multixact ID, so it is normal + * for their values to lag behind the control file. + */ dbres = executeQueryOrDie(conn_template1, "SELECT datname, datfrozenxid, datminmxid " - "FROM pg_catalog.pg_database"); + "FROM pg_catalog.pg_database " + "WHERE datconnlimit <> -2"); i_datname = PQfnumber(dbres, "datname"); i_datfrozenxid = PQfnumber(dbres, "datfrozenxid"); diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c index 5c59cfb32e8..d435681b892 100644 --- a/src/bin/pg_upgrade/info.c +++ b/src/bin/pg_upgrade/info.c @@ -406,6 +406,7 @@ get_db_infos(ClusterInfo *cluster) i_spclocation; char query[QUERY_ALLOC]; + /* Skip invalid databases in the old cluster */ snprintf(query, sizeof(query), "SELECT d.oid, d.datname, " "pg_catalog.pg_tablespace_location(t.oid) AS spclocation " @@ -413,7 +414,9 @@ get_db_infos(ClusterInfo *cluster) " LEFT OUTER JOIN pg_catalog.pg_tablespace t " " ON d.dattablespace = t.oid " "WHERE d.datallowconn = true " - "ORDER BY 1"); + "%s" + "ORDER BY 1", + cluster == &old_cluster ? " AND d.datconnlimit <> -2 " : ""); res = executeQueryOrDie(conn, "%s", query); diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl index 9590625fc32..375e60e8c1a 100644 --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl @@ -551,12 +551,14 @@ SKIP: 'run of pg_upgrade --check with old instance running'); } -# Create an invalid database, will be deleted below +# Create an invalid database, it must not be upgraded $oldnode->safe_psql( 'postgres', qq( CREATE DATABASE regression_invalid; UPDATE pg_database SET datconnlimit = -2 WHERE datname = 'regression_invalid'; )); +my $invalid_db_oid = $oldnode->safe_psql('postgres', + "SELECT oid FROM pg_database WHERE datname = 'regression_invalid'"); # Upgrade the instance. $oldnode->stop; @@ -584,40 +586,10 @@ 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 - if $oldnode->pg_version < 11; - - 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', - ], - 1, - [qr/datconnlimit/], - [qr/^$/], - 'invalid database causes failure'); - rmtree($newnode->data_dir . "/pg_upgrade_output.d"); -} - -# And drop it, so we can continue -$oldnode->start; -$oldnode->safe_psql('postgres', 'DROP DATABASE regression_invalid'); -$oldnode->stop; - -# --check command works here, cleans up pg_upgrade_output.d. -command_ok( +# --check command works here, cleans up pg_upgrade_output.d. The invalid +# database is left in place, so this also checks that it is reported and that +# the upgrade is not aborted because of it. +command_checks_all( [ 'pg_upgrade', '--no-sync', '--old-datadir' => $oldnode->data_dir, @@ -629,6 +601,9 @@ command_ok( '--new-port' => $newnode->port, $mode, '--check', ], + 0, + [ qr/contains invalid databases/, qr/^\s+regression_invalid$/m ], + [qr/^$/], '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"); @@ -652,6 +627,18 @@ ok( !-d $newnode->data_dir . "/pg_upgrade_output.d", $newnode->start; +# The invalid database must not have made it into the new cluster. +is( $newnode->safe_psql( + 'postgres', + "SELECT count(*) FROM pg_database WHERE datname = 'regression_invalid'" + ), + '0', + 'invalid database is not present in the new cluster'); +ok(!-d $newnode->data_dir . "/base/$invalid_db_oid", + "invalid database is not transferred to the new cluster"); +ok(-d $oldnode->data_dir . "/base/$invalid_db_oid", + "invalid database is left behind in the old cluster"); + # The 8-byte OID has been carried. if (!defined($ENV{oldinstall})) { -- 2.47.3
