Real pg_upgrade support via a reduced bin/test_existing

[jnasbyupgrade]

Stacked on #30 (phase 3.5: CI hygiene). Adds binary `pg_upgrade` coverage - the one U&U scenario that genuinely can't live inside a single `pg_regress` invocation, since `pg_upgrade` is an external binary process.

What changed

• New `pg-upgrade-test` CI job: install 0.9.6 on an old PostgreSQL major, plant + prove the dependency guard, binary `pg_upgrade` to a newer major, `ALTER EXTENSION UPDATE` the migrated objects, then run the suite against the real upgraded database in existing mode. Matrix: 10→18 and 12→18 (no bridge-update leg - count_nulls has always been pure SQL functions with no `SELECT *`-over-catalog views, so it has no known pg_upgrade-unsafe old version to bridge past). No stepwise every-major climb either, for the same reason (nothing catalog-touching to break at a specific boundary).
• `bin/test_existing`: notably smaller than it would have needed to be before phase 3's `test/install` work. Only `prepare-old` (old-cluster setup before the real `pg_upgrade` binary runs) and `run-suite` (assert + re-prove the guard + run the suite via `--use-existing` afterward) are genuinely external-to-`pg_regress` concerns, plus a small `update` subcommand for the post-upgrade `ALTER EXTENSION UPDATE` step. There's no `update-scenario` subcommand at all - that whole scenario is just `make test-update` now (phase 3's `test/install`-driven update mode), since an in-place update has no external step to drive.
• `run_suite()` gates on plain `make test`, not the old belt-and-suspenders `make test && make verify-results` combo - pgxntool 2.3.0 (Update pgxntool to 2.3.0 #25) already made `make test` itself exit non-zero on regression failures, so the extra call is redundant now.

Not yet crossed with `TEST_SCHEMA` - that's the next phase, once both this job and `extension-update-test` can cross it together.

Verification

Locally against PG17: `prepare-old` → `update` → `run-suite` all pass end to end, without a real `pg_upgrade` (this container's PG12/PG17 clusters are persistent shared dev infra, not disposable, so - same reasoning as the earlier pg-tle-test work - the actual binary `pg_upgrade` leg is left for CI's ephemeral containers to exercise).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c153f6f4-ec8a-4181-9298-79dbfea7e77d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[github-actions]

Claude finished @jnasbyupgrade's task in 5m 47s —— View job

---

Code review

• Gather context (PR body, changed files)
• Review .github/workflows/ci.yml changes
• Review bin/test_existing
• Review bin/test_existing.sql/*.sql
• Post review feedback

Overall this is a well-reasoned, carefully-commented PR — the two-leg matrix design (updating on opposite sides of the binary pg_upgrade) is a nice way to get both real-world orderings out of the same CI cost, and the dependency-guard self-check (plant → prove → re-prove → drop) is a solid way to make a silent CASCADE/reinstall fail loudly instead of passing green by accident. I read through ci.yml's new pg-upgrade-test job, bin/test_existing, and all three of its SQL helpers, and traced the step ordering (install-before-upgrade, update-before-vs-after-upgrade, guard survival checks) end to end — it's internally consistent and the two legs do test what the comments claim.

One real issue, otherwise minor:

bin/test_existing:125 — latent set -e trap in assert_version()

assert_version() {
  local db=$1 expected=$2 installed
  [ "$expected" = current ] && expected=$(current_version)
  ...

Under set -euo pipefail (line 56), && used as a conditional is dangerous: if $expected is ever anything other than the literal string current, the [ ] test is false, the whole command1 && command2 construct returns non-zero, and — because there's no enclosing if/||/similar to absorb that — the script aborts right there under set -e, with no FAIL: message at all.

This is exactly the failure mode this file's own author already flagged and avoided a few lines below, in update_ext() (line 138): "Use if, not &&: a false test under set -e would abort." The same fix wasn't applied here.

It's currently dormant — assert_version is only ever called as assert_version "$db" current (in run_suite, line 183) — so it can't manifest today. But the function's own signature (DB EXPECTED) implies a specific version is a supported input, and if a future caller ever passes one, the script would die silently instead of running the intended comparison. Worth the one-line fix for consistency with the pattern already established in this same file:

if [ "$expected" = current ]; then expected=$(current_version); fi

Fix this →

Nothing else stood out: the if: matrix.old_pg == '12' / '10' step gating matches the described leg semantics, the install-into-new-cluster-before-binary-pg_upgrade ordering is correct (the new cluster needs the control/update scripts on disk before pg_upgrade replays CREATE EXTENSION), the guard SQL's exception handling correctly rolls back the probe DROP EXTENSION even in the failure branch, and FLOOR=10/NEWEST=18 in the changes job confirms the "oldest supported version" claim in the old_pg=10 leg's comment.

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 20s —— View job

---

Code review

• Gather context (PR body, changed files, prior review comments)
• Check diff since last review (new commits from pull request synchronize)
• Review .github/workflows/ci.yml changes
• Review bin/test_existing
• Review bin/test_existing.sql/*.sql
• Post review feedback

This push (d714ef5, "confine the ordering comparison to old_pg=10 via twin databases") only touches .github/workflows/ci.yml — bin/test_existing and its SQL helpers are unchanged since the prior review. I traced the new twin-database flow end to end and it's internally consistent:

• old_pg=10's two prepare-old calls (count_nulls_upgrade_oldest_first, count_nulls_upgrade_current_first, ci.yml:368-369) create both databases in the same old cluster before the single binary pg_upgrade call (ci.yml:409-415), which upgrades the whole cluster and therefore migrates both at once — matching the "one pg_upgrade call, twin databases" claim in the header comment.
• The pre-upgrade update step (ci.yml:393) now runs unconditionally but selects the right target database via the matrix.old_pg == '10' && '...' || '...' GitHub Actions ternary — correctly touching only count_nulls_upgrade_current_first on that leg and leaving count_nulls_upgrade_oldest_first frozen at 0.9.6 for the post-upgrade update (ci.yml:438).
• old_pg=12 cleanly reverts to the original single-database, single-ordering shape (count_nulls_upgrade, ci.yml:378/393/462), with no leftover if: matrix.old_pg == '12' guard on the now-unconditional pre-upgrade update step.
• Both run-suite calls for the old_pg=10 leg (ci.yml:450-451) run sequentially in the same step against the two independently-migrated databases — each gets its own version/guard assertions.

No bugs found in this diff.

One item from the previous review remains open, since it's in a file this push didn't touch:

bin/test_existing:125 — latent set -e trap in assert_version()

[ "$expected" = current ] && expected=$(current_version)

Under set -euo pipefail (line 56), if $expected is ever anything other than the literal string current, this && short-circuits false and aborts the whole script with no FAIL: message — the exact pitfall this file's own update_ext() (line 138) already documents and avoids with an if. Still dormant today (assert_version is only ever called with "$db" current, run_suite:183), but worth the one-line fix for consistency:

if [ "$expected" = current ]; then expected=$(current_version); fi

Fix this →
· branch: phase4-pg-upgrade