schema: review-report-v2
target: CommitFest 7262: pg_upgrade --invalid-databases=skip|error option for handling invalid databases
base: 140fdfcdf12d524541ff26491b8e2076a690e434
head: 140fdfcdf12d524541ff26491b8e2076a690e434
verdict: needs-review
blocker_counts: verified=0 unverified=0
scope_identity: d956c637efb0a14c784e5c8024811af435c81409e61e9f268501e06474ba8c7b
model: glm-5.2:cloud
source: postgresql-master
slug: commitfest-7262-review
commitfest: "7262"
diff_sha: d956c637efb0a14c784e5c8024811af435c81409e61e9f268501e06474ba8c7b
diff_type: staged-uncommitted
artifact_sha256: 9de79ec59e85af5028b7ed825cbab7e8d94e3e9a2c3f7aab8b4a6ad053d51a2a
router_policy_version: 1
router_policy_hash: 997c035512fa1b35aa17149ea386a861af531ec05839ce06810f38268a3caebe

CommitFest 7262 Review: pg_upgrade --invalid-databases=skip|error

Verdict: needs-review — 0 blockers, 1 test MAJOR (PATCH-1: full-upgrade skip path untested), 1 design MAJOR (THREAD-1: unresolved default-skip-vs-default-error debate on pgsql-hackers), 6 MINORs (PATCH-2/3/4/5, SEC-1, COV-1), 2 NITs (PATCH-6/7), 1 refuted (SEC-2). Changed-code coverage measured: 17/19 lines (89.5%), 14/20 branches (70.0%). All findings verified with concrete repair patches; code patch fully validated (apply+compile+smoke); test patch apply-validated + perl syntax OK (TAP two-run delegated to pg-regress-expert).

Target

• Files: doc/src/sgml/ref/pgupgrade.sgml, src/bin/pg_upgrade/check.c (check_for_connection_status), src/bin/pg_upgrade/info.c (get_db_infos), src/bin/pg_upgrade/option.c (parseCommandLine, usage), src/bin/pg_upgrade/pg_upgrade.h (new invalidDbMode enum), src/bin/pg_upgrade/t/002_pg_upgrade.pl, src/tools/pgindent/typedefs.list (7 files, +112/-19)
• Diff type: staged uncommitted; no commit object
• HEAD = base = merge-base: 140fdfcdf12d524541ff26491b8e2076a690e434
• Diff SHA: d956c637efb0a14c784e5c8024811af435c81409e61e9f268501e06474ba8c7b
• What: Adds --invalid-databases=skip|error to pg_upgrade. Invalid databases (datconnlimit=-2, from an interrupted DROP DATABASE per commit c66a7d75e652) are skipped by default (SKIP) and reported; with --invalid-databases=error they abort the upgrade. get_db_infos filters AND d.datconnlimit <> -2 so invalid DBs are excluded from dbarr. check_for_connection_status gains a SKIP branch that logs a PG_WARNING per invalid DB and sets skipped_invalid; otherwise datallowconn=f or datconnlimit=-2 databases are written to the failure script and abort.

Pipeline and provenance

Model routing: route-review-models policy v1 (hash 997c0355…). User-fixed model glm-5.2:cloud bound as session default for all seats.

Patch summary

The patch adds a --invalid-databases=skip|error option to pg_upgrade. Invalid databases (whose DROP DATABASE was interrupted, leaving datconnlimit=-2) are skipped by default and reported with a warning; with --invalid-databases=error they abort the upgrade (preserving the historical behavior). The get_db_infos query now filters AND d.datconnlimit <> -2 so invalid DBs are excluded from the dbarr list — they are neither dumped, restored, nor file-transferred. check_for_connection_status gains a new else if SKIP branch (before the catch-all abort branch) that logs a per-DB warning and sets skipped_invalid; the final check_ok() is suppressed when invalid DBs were skipped. The TAP test 002_pg_upgrade.pl adds --invalid-databases=error (abort) and default-skip (--check) sub-tests. Docs, typedefs.list, and option parsing are updated.

Findings summary

Blockers: 0 verified, 0 unverified. Majors: 2 (PATCH-1 tests, THREAD-1 design). Minors: 6 (PATCH-2/3/4/5, SEC-1, COV-1). Nits: 2 (PATCH-6/7). Refuted: 1 (SEC-2).

Verification Summary

All 11 findings were verified by the pg-finding-verifier seat against source in the read-only worktree and disposable scratch copies at HEAD 140fdfcdf12d524541ff26491b8e2076a690e434 (scope identity d956c637efb0a14c…). Validation methods: apply-check, diff-check, compile (make -s -C src/bin/pg_upgrade clean pg_upgrade, exit 0, 0 warnings after repair), perl syntax check (perl -c), and binary smoke test (./pg_upgrade --invalid-databases=bogus --check → exit 1 with repaired message).

• 8 findings verified with concrete repair patches (PATCH-1 through PATCH-7, SEC-1, COV-1).
• 1 finding refuted (SEC-2: correct status-line idiom, not a defect).
• 1 finding cannot-verify (THREAD-1: unresolved design objection on default-skip-vs-default-error; requires committer consensus, not execution).

The combined code repair patch (PATCH-2/3/4/6/7 + SEC-1, SHA256 731c3027626d2e57…) compiles clean with 0 warnings (down from 1 -Wmisleading-indentation warning on the un-repaired staged tree) and the binary emits the repaired error message. The test repair patch (PATCH-1 + PATCH-5, SHA256 ee1bddab48350e41…) applies cleanly with perl -c syntax OK; the targeted TAP two-run validation (FAILS on pre-patch, PASSES on post-patch) was delegated to pg-regress-expert as it requires a cross-version upgrade harness with --enable-tap-tests and a compatible older PostgreSQL install. The COV-1 test repair (SHA256 5c542d1e9cbd1849…) similarly applies cleanly with perl syntax OK.

Repair Validation Summary

All patches apply cleanly on top of the staged postimage tree with patch -p1 --dry-run (no offsets, no fuzz, no rejects). All validation performed in disposable scratch copies; the reviewed worktree was never modified.

Changed-Code Coverage

• Coverage requirement: required (changed instrumentable C: check.c, info.c, option.c)
• Counter epoch: fresh (first and only test run on this coverage build)
• Execution lane: host-disposable (nested-sandbox build succeeded; --coverage instrumented)
• Test commands: make -s -C src/bin/pg_upgrade check → 8 TAP files, 151 subtests, all passed (300 wallclock secs); t/002_pg_upgrade.pl (exercising changed code) passed
• Integrity errors: none
• gcov version: 15.2.0 (Ubuntu 15.2.0-16ubuntu1)

Raw metrics

Line coverage: 17/19 = 89.5%. Branch coverage: 14/20 = 70.0%.

Touched functions

Actionable uncovered regions

UR-1: src/bin/pg_upgrade/option.c:241 — explicit --invalid-databases=skip parse (coverable-now)

241: 				user_opts.invalid_db_mode = INVALID_DB_SKIP;

Why missed: the default init at option.c:79 already sets INVALID_DB_SKIP, so only an explicit --invalid-databases=skip reaches line 241. No existing or currently-proposed test passes the option explicitly (the default-skip TAP test omits it). Meaningful untested parse path of the new option feature.

Test example: command_checks_all([ 'pg_upgrade', ... '--check', '--invalid-databases' => 'skip' ], 0, [qr/invalid database "regression_invalid" will not be upgraded/], [qr/^$/], 'invalid database is skipped with --invalid-databases=skip');

COV-1 finding issued (MINOR tests, verified-by-coverage) — explicit --invalid-databases=skip parse path is untested. Repair patch SHA256: 5c542d1e9cbd1849….

UR-2: src/bin/pg_upgrade/option.c:245 — invalid argument rejection (coverable-now)

245: 				pg_fatal("invalid argument for option %s", "--invalid-databases");

Why missed: no existing test passes a bogus --invalid-databases value. Addressed by the pg-test-author's proposed PATCH-1 test patch (adds --invalid-databases=bogus test). No separate COV finding — gap is known and addressed by a proposed repair.

Uncovered branches

Excluded changed coordinates

Non-instrumented changes

Residual limitations

• A line/branch hit confirms execution only, not that the test asserted correct behavior.
• Single invalid-DB fixture bounds check_for_connection_status reachability; 2nd-iteration arcs need a second invalid/abort-worthy DB.
• No fault-injection seam for fopen_priv; check.c:1147 fopen-failure branch is infeasible in the current TAP harness.
• info.c filter lines 426-427 have no executable gcov coordinate; their runtime effect is indirectly confirmed by the skip-mode test passing.
• Counter epoch freshness asserted from the harness run manifest (clean build, first run).

Findings

pgsql-hackers thread ledger

• Status: reconciled
• Ledger report: commitfest-7262-review-thread-ledger (source postgresql-master)
• Current thread (657561): "Teach pg_upgrade to deal with invalid databases" — Bharath Rupireddy (2026-09-04, patch v1, default=skip), Nathan Bossart (2026-09-04, prefers default=off), Bharath (2026-09-05, defends default=skip). Unresolved.
• Predecessor thread (592242): "pg_upgrade check for invalid databases" — Kirill Richine (2024-09-30, check+report), Daniel Gustafsson (2024-10-01, caution on skipping valuable data), Nathan Bossart (2024-10-07, fail-by-default + optional skip flag), Kirill v2 (2024-10-13, default=fail + --skip-invalid-databases).
• Development-branch catversion exception: N/A (no catalog change).

Key thread finding links:

• PATCH-4: already-raised (Nathan Bossart, ZwQ-xtmKbNAz_HK8@nathan, suggested the skip hint)
• THREAD-1: author-deferred (Nathan prefers default=error; Bharath defends default=skip; unresolved)
• All other findings: new

Aggregated Test Plan

Two-run oracle: baseline (pre-patch) — new tests FAIL (unknown option → stderr; invalid DB aborts full upgrade); post-patch — all PASS.

Consolidated repair patches

Code repair (PATCH-2, 3, 4, 6, 7 + SEC-1)

SHA256: 731c3027626d2e57de62271a22256d81090a421c0ae91e9c0faa66413866916d. Files: src/bin/pg_upgrade/check.c, src/bin/pg_upgrade/info.c, src/bin/pg_upgrade/option.c. Validation: passed (apply + diff + compile 0 warnings + smoke).

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index ef9e34f..de55dcf 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -1124,15 +1124,20 @@ check_for_connection_status(ClusterInfo *cluster)
 		 * 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.
+		 *
+		 * This else-if must precede the catch-all abort branch below: a
+		 * database that is both invalid (datconnlimit == -2) and
+		 * datallowconn == f must reach the skip path, not the abort path.
+		 * The order is load-bearing; reordering would silently turn skip
+		 * mode into an abort for such databases.  (Invalid databases are
+		 * also excluded from dbarr by get_db_infos() and refused
+		 * connections by the backend, so this precedence is a reporting
+		 * choice, not a security boundary.)
 		 */
 		else if (strcmp(datconnlimit, "-2") == 0 &&
 				 user_opts.invalid_db_mode == INVALID_DB_SKIP)
 		{
-			if (!skipped_invalid)
-			{
-				report_status(PG_WARNING, "warning");
-				skipped_invalid = true;
-			}
+			skipped_invalid = true;
 			pg_log(PG_WARNING,
 				   "invalid database \"%s\" will not be upgraded", datname);
 		}
@@ -1165,9 +1170,16 @@ check_for_connection_status(ClusterInfo *cluster)
 				 "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"
-				 "    %s", output_path);
+				 "    %s\n"
+				 "If the databases with datconnlimit -2 are invalid (their DROP\n"
+				 "DATABASE was interrupted), running pg_upgrade with\n"
+				 "--invalid-databases=skip will skip them and proceed.",
+				 output_path);
 	}
-	else if (!skipped_invalid)
+	else if (skipped_invalid)
+		/* report once after the loop, like check_for_unicode() */
+		report_status(PG_WARNING, "warning");
+	else
 		check_ok();
 }
 
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index 09f0b73..408d793 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -423,7 +423,16 @@ 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 */
+	/*
+	 * Skip invalid databases, whose DROP DATABASE was interrupted.  This
+	 * filter is unconditional: it applies in both --invalid-databases modes.
+	 * In --invalid-databases=error, check_for_connection_status() aborts
+	 * before get_db_infos() is ever reached, so this filter never observes
+	 * an invalid database in that mode; the two defenses are coupled only
+	 * by call order.  Keeping the filter here in skip mode ensures invalid
+	 * databases never enter dbarr, so they are neither dumped, restored,
+	 * nor file-copied.
+	 */
 			 "  AND d.datconnlimit <> -2 "
 			 "ORDER BY 1");
 
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 5ef08e2..a35c270 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -242,7 +242,8 @@ parseCommandLine(int argc, char *argv[])
 				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");
+					pg_fatal("invalid argument for option %s: must be \"skip\" or \"error\"",
+					 "--invalid-databases");
 				break;
 
 			default:

Test repair (PATCH-1 + PATCH-5)

SHA256: ee1bddab48350e41d64e0083e069de702dd1902f17ec695011a65ed33dbd5eb2. Files: src/bin/pg_upgrade/t/002_pg_upgrade.pl. Validation: partial (apply + diff + perl syntax OK; TAP two-run delegated).

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index ca903bc..0ddb377 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -530,6 +530,12 @@ $oldnode->safe_psql(
   UPDATE pg_database SET datconnlimit = -2 WHERE datname = 'regression_invalid';
 ));
 
+# Capture the OID of the invalid database for later filesystem checks on the
+# new cluster.  Read while the old node is still running (it is stopped below).
+my $invalid_db_oid = $oldnode->safe_psql('postgres',
+	"SELECT oid FROM pg_database WHERE datname = 'regression_invalid'");
+chomp $invalid_db_oid;
+
 # Upgrade the instance.
 $oldnode->stop;
 
@@ -556,6 +562,26 @@ 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");
 
+# An unknown --invalid-databases value is rejected at option-parsing time,
+# before any output directory is created.  pg_fatal() reports to stdout, so
+# assert the message on stdout and empty stderr.
+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', '--invalid-databases' => 'bogus',
+	],
+	1,
+	[qr/invalid argument for option --invalid-databases/],
+	[qr/^$/],
+	'pg_upgrade rejects unknown --invalid-databases value');
+
 # 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
@@ -563,7 +589,7 @@ rmtree($newnode->data_dir . "/pg_upgrade_output.d");
 # there.)
 SKIP:
 {
-	skip "database invalidation not implemented", 2
+	skip "database invalidation not implemented", 6
 	  if $oldnode->pg_version < 11;
 
 	# --invalid-databases=error preserves the historical hard failure.
@@ -606,10 +632,21 @@ SKIP:
 	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;
+# On versions that predate the invalid-database mechanism (commit
+# c66a7d75e652), datconnlimit=-2 has no "invalid" semantics and a full
+# upgrade would reject the database, so drop it before the upgrade as the
+# historical test did.  On PG >= 11 the default --invalid-databases=skip
+# excludes it from the upgrade, so we deliberately leave it in place to
+# exercise the full-upgrade skip path end to end.
+SKIP:
+{
+	skip "invalid database invalidation not implemented", 0
+	  if $oldnode->pg_version >= 11;
+
+	$oldnode->start;
+	$oldnode->safe_psql('postgres', 'DROP DATABASE regression_invalid');
+	$oldnode->stop;
+}
 
 # --check command works here, cleans up pg_upgrade_output.d.
 command_ok(
@@ -647,6 +684,23 @@ ok( !-d $newnode->data_dir . "/pg_upgrade_output.d",
 
 $newnode->start;
 
+# On PG >= 11, the full upgrade above ran with the invalid database still
+# present and the default --invalid-databases=skip.  Assert that the invalid
+# database was not carried over to the new cluster: it must be absent from
+# pg_database, and its OID directory must be absent under base/.
+SKIP:
+{
+	skip "invalid database invalidation not implemented", 2
+	  if $oldnode->pg_version < 11;
+
+	is($newnode->safe_psql('postgres',
+			"SELECT count(*) FROM pg_database WHERE datname = 'regression_invalid'"),
+		'0',
+		'invalid database absent from pg_database on new cluster');
+	ok( !-d $newnode->data_dir . "/base/$invalid_db_oid",
+		"invalid database OID directory absent from new cluster base/");
+}
+
 # Check if there are any logs coming from pg_upgrade, that would only be
 # retained on failure.
 my $log_path = $newnode->data_dir . "/pg_upgrade_output.d";

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index ca903bc..6f1cd72 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -563,7 +563,7 @@ rmtree($newnode->data_dir . "/pg_upgrade_output.d");
 # there.)
 SKIP:
 {
-	skip "database invalidation not implemented", 2
+	skip "database invalidation not implemented", 3
 	  if $oldnode->pg_version < 11;
 
 	# --invalid-databases=error preserves the historical hard failure.
@@ -604,6 +604,28 @@ SKIP:
 		[qr/^$/],
 		'invalid database is skipped by default');
 	rmtree($newnode->data_dir . "/pg_upgrade_output.d");
+
+	# Explicitly requesting --invalid-databases=skip must behave like the
+	# default: --check succeeds while naming the database that will not be
+	# upgraded.  This exercises the explicit "skip" parse arm of the
+	# --invalid-databases option (distinct from the default initialization).
+	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', '--invalid-databases' => 'skip',
+		],
+		0,
+		[qr/invalid database "regression_invalid" will not be upgraded/],
+		[qr/^$/],
+		'invalid database is skipped with --invalid-databases=skip');
+	rmtree($newnode->data_dir . "/pg_upgrade_output.d");
 }
 
 # And drop it, so we can continue

Residual risk

• THREAD-1 (unresolved design objection): The default-skip-vs-default-error debate is unresolved on pgsql-hackers. The committer should seek consensus before committing. This is the primary reason for the needs-review verdict.
• PATCH-1 (test gap): The full-upgrade skip path is untested; the proposed test patch is apply-validated but not TAP-run-validated in this review (delegated to pg-regress-expert). Applying the proposed test before commit is recommended.
• Coverage residual: 2 uncovered lines and 6 uncovered branches; all are either coverable-now (with proposed tests) or require special harness / infeasible (pre-existing conditions). No actionable uncovered path of the new patch logic remains without a proposed test except the explicit-skip parse (COV-1, with proposed test).

Verified-clean section (explicitly checked, no issue)

• Logging convention: report_status(PG_WARNING, "warning") + pg_log(PG_WARNING, ...) is the correct pg_upgrade pattern. PG_WARNING prints to stdout; the TAP test's stdout regex is correct. else if (!skipped_invalid) check_ok() correctly suppresses the trailing "ok".
• Transfer/relfilenode mapping: transfer_all_new_dbs iterates dbinfos (excludes invalid DBs); strcmp matching won't raise "old database not found" for invalid DBs. Invalid DB files are never copied to the new cluster.
• delete_old_cluster.sh: removes the entire old PGDATA and all old tablespaces; invalid DB files are removed.
• New cluster leftovers: pg_dumpall --globals-only dumps only roles/tablespaces; dumpDatabases and dropDBs filter datconnlimit != -2; set_frozenxids enumerates the new cluster (no invalid DBs).
• typedefs.list: invalidDbMode correctly placed alphabetically.
• Option parsing: pg_strcasecmp case-insensitive; enum initialized to documented default.
• Memory context: N/A (frontend binary).
• Catalog/version: no catversion bump needed; datconnlimit=-2 convention exists since c66a7d75e652 (backpatched to PG11); TAP test correctly skips for pre-PG-11.
• SQL injection: the added filter is a literal constant; no interpolation.
• Memory safety: strcmp on libpq null-terminated strings; no UAF, no leak, no uninitialized read.

Deterministic verdict

• Verdict: needs-review
• Reasoning: No BLOCKERs → not waiting-on-author. THREAD-1 is a live design objection (unresolved thread on default-skip-vs-default-error) → needs-review. PATCH-1 is a test-only MAJOR (would yield ready-for-committer (test follow-up required) if THREAD-1 were resolved).
• Blocker split: 0 verified, 0 unverified.

Maintainer CC

Author: Bharath Rupireddy (bharath.rupireddyforpostgres@gmail.com). Reviewer: Nathan Bossart (nathandbossart@gmail.com). Predecessor: Kirill Richine (tk@postsubmeta.net), Daniel Gustafsson (daniel@yesql.se).