Structural fresh-vs-update comparison, wired into test and pg-upgrade-test

[jnasbyupgrade]

Adds bin/compare_fresh_vs_update, a standalone script that structurally compares every object count_nulls owns between a fresh install and an updated one, catching update-script divergence a fixed pgTAP suite has no way to know to assert. It diffs pg_get_functiondef(), each object's comment, and its ACL for every object pg_depend records as owned by the extension - discovered live, not a hardcoded list, so a newly added function is automatically covered without editing this script.

Both installs it creates itself use a plain, unqualified CREATE EXTENSION count_nulls, so they land in the same default schema (ordinarily public) by construction - enough to compare object definitions without needing the main suite's schema-randomization/quoting machinery, which serves a different purpose (proving qualification-correctness, in test/helpers/create_test_schema.sql).

The script also accepts an optional EXISTING_DB argument to compare a fresh install against an already-populated database instead of creating and updating its own scratch one. Since that database's schema was chosen by whatever created it, the fresh install discovers and reuses that same schema rather than generating a new one, so both sides still land in the same place.

Wired into CI in two places: the test job's update leg (bin/compare_fresh_vs_update 0.9.6, once per push) and pg-upgrade-test, which passes each of its twin real-pg_upgraded databases as EXISTING_DB. Both catch the same divergence class the fixed pgTAP suite doesn't - an object left subtly different (body, comment, ACL) by an update path, whether an in-place ALTER EXTENSION UPDATE or a real binary pg_upgrade.

Modeled on the manual technique used in Postgres-Extensions/cat_tools#46, which found a real bug this way (a pre-0.2.2 update path left EXECUTE PROCEDURE hardcoded where a fresh install already used EXECUTE FUNCTION). Postgres-Extensions/cat_tools#67 proposes generalizing this exact capability - listing every object an extension owns along with each object's actual definition - into a real, user-facing cat_tools feature rather than test-only plumbing tied to one extension's update path.

[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: f0806031-a369-4a90-9d31-1acce24ee87f

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 3m 36s —— View job

---

Code review: PR #33 (Structural fresh-vs-update comparison + convergence/divergence report)

Task list

• Gather context (PR description, changed files)
• Read bin/compare_fresh_vs_update, the ci.yml diff, and test/README.md diff in full
• Cross-check against Makefile's TEST_SCHEMA/TEST_LOAD_SOURCE conventions and the sibling bin/test_existing for consistency
• Post review findings

---

Summary

Reviewed the diff against origin/phase5-cross-schema...HEAD (3 files, +143/-0). No bugs found. The script is well-scoped and its extensive header comments accurately describe its own behavior — I verified the claims rather than just trusting them.

What I checked specifically

• bin/compare_fresh_vs_update correctness:

  • extension_members()'s discovery query walks pg_depend (deptype = 'e') joined to pg_extension/pg_proc rather than a hardcoded object list — confirmed against sql/count_nulls*.sql that the extension only ever creates functions (including trigger functions), never a standalone CREATE TRIGGER, so restricting the comparison to pg_proc is complete, not an oversight.
  • Both the fresh and updated installs are made to target the same schema for a given invocation, so pg_get_functiondef/regprocedure rendering (which is search_path-sensitive) stays consistent between the two snapshots being diffed — schema-qualification differences won't produce false-positive diffs.
  • set -euo pipefail + trap cleanup EXIT correctly drops both scratch DBs and temp files on any exit path; the if diff -u ...; then ... else ...; fi correctly avoids tripping errexit on a real (expected) diff.
  • The ci.yml step (test job, lines 261-274) runs this only after the make install step, which is required since the script's CREATE EXTENSION calls depend on the extension already being installed on the runner — ordering is correct.
  • Confirmed .github/workflows/ci.yml and the embedded shell block parse fine.

• Minor, non-blocking observation: install_in_schema() (bin/compare_fresh_vs_update:~90) interpolates $schema directly into a double-quoted SQL identifier (CREATE SCHEMA IF NOT EXISTS \"$schema\") with no escaping. Not exploitable today — the script is only ever invoked with the two fixed literals ""/"Quoted" from ci.yml — but if this script is ever called with a less-trusted value down the line, an embedded " would break out of the identifier. Not worth blocking on given current call sites; flagging only for awareness. Fix this →

• Docs/CI wiring: test/README.md's new bullet and the ci.yml step/job comments accurately describe the actual behavior added (verified line-by-line against the script and Makefile, not just read for prose quality). The pg-tle-test scope-out comment and its rationale (pgxntool installcheck/install ordering regression) matches the linked upstream issue description in the PR body.

• PR body's own convergence/divergence writeup: matches what's actually in the diff — no discrepancy between the narrative and the code.

No CLAUDE.md violations found (checked pgxntool/CLAUDE.md's guidance, which is scoped to pgxntool development and defers to the host project's own instructions — no host-project CLAUDE.md exists in this repo).

Verdict

No changes requested. The one item above is optional/informational, not a blocker.

[github-actions]

Claude finished @jnasbyupgrade's task in 4m 19s —— View job

---

Code review: PR #33 (Structural fresh-vs-update comparison + convergence/divergence report)

Task list

• Gather context (PR description, changed files, prior automated review)
• Diff origin/phase5-cross-schema...HEAD (two-dot diff, since three-dot has no merge-base in this shallow checkout) and read every changed file
• Focus incremental review on commit 1a833cd (pushed since the last automated review), which rebuilt bin/compare_fresh_vs_update on top of Always install count_nulls into a fresh, randomly named schema for tests #55's now-merged shared-random-schema design
• Post review findings

---

Summary

The bulk of the diff against phase5-cross-schema (ci.yml's floor/twin-database pg_upgrade logic, Makefile, test/helpers/*.sql, bin/test_existing) is PR #55, already merged to master and pulled into this branch by rebase — not new work from this PR. The only genuinely new commit since the last automated review is 1a833cd, which rebuilds bin/compare_fresh_vs_update on top of #55's design. That's what I focused on. Per the PR description, this whole PR is marked superseded and not meant to merge as-is, so treat the below as informational.

What's new in 1a833cd

• bin/compare_fresh_vs_update no longer takes a SCHEMA CLI argument at all — it always generates one shared, randomly-named schema (same convention as test/helpers/create_test_schema.sql) and installs both the fresh and updated copies into it. This fully resolves the only issue the prior automated review flagged (unescaped $schema interpolation into a double-quoted identifier) — not just by escaping it, but by removing the untrusted-input surface entirely, since the value is now always script-generated, never caller-supplied.
• ci.yml's test job now calls the script once (bin/compare_fresh_vs_update 0.9.6) after make verify-results TEST_LOAD_SOURCE=update, instead of looping over the old TEST_SCHEMA axis — correctly wired, and verify-results is a real pgxntool target so TEST_LOAD_SOURCE=update propagates via the Makefile's exported PGOPTIONS as expected.
• test/README.md's new bullet accurately describes the script's actual behavior.

Minor, non-blocking observations

• Comment/code name drift: the header comments repeatedly refer to a function extension_members() ("see extension_members() below", "extension_members(): every object pg_depend records as owned by..." — bin/compare_fresh_vs_update:23,29,108), but the actual function is named query() (bin/compare_fresh_vs_update:113). Looks like a rename happened without updating the surrounding prose. Fix this →
• Schema-name generation uses a bare psql -tAc with no -d (bin/compare_fresh_vs_update:96), unlike every other psql call in this script and in bin/test_existing (which always pass -d). It almost certainly works today (this CI's pg-start connects as a role with a same-named default database), but it's an inconsistent pattern worth calling out since it's the one connection in the file whose target database is implicit rather than explicit.

On the rest of the diff (inherited from #55, already merged)

Spot-checked test/helpers/create_test_schema.sql, find_test_schema.sql, and bin/test_existing's prepare-old/update/run-suite signatures against the new ci.yml twin-database (upgrade_oldest_first/upgrade_current_first) invocations — all consistent, no leftover references to the removed TEST_SCHEMA/test-schema-all mechanism anywhere in the tree.

Verdict

No blockers. Two cosmetic nits above, both optional. Given the PR is explicitly marked superseded/for-reference-only, I'd leave it as-is unless it's actually going to be revived.

[github-actions]

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

---

Code review: PR #33 (Structural fresh-vs-update comparison + convergence/divergence report)

Task list

• Gather context (PR description, prior review comments, changed files)
• Diff origin/master..HEAD and read every changed file (bin/compare_fresh_vs_update, ci.yml, test/README.md)
• Cross-check script logic (schema handling, discovery query, cleanup, CI wiring) against the rest of the test suite (bin/test_existing, test/helpers/create_test_schema.sql)
• Check whether prior review findings are still present
• Post final review

---

Summary

Reviewed the current tip (ffe07ed) against origin/master. The script's core design is sound: shared single schema for both installs (so pg_get_functiondef schema-qualification can't cause false-positive diffs), live discovery via pg_depend rather than a hardcoded object list, correct set -euo pipefail + trap cleanup EXIT handling, and the diff -u ...; then ... else ...; fi idiom correctly avoids tripping errexit on a genuine (expected) diff. CI wiring (ci.yml step runs after make verify-results TEST_LOAD_SOURCE=update) and the test/README.md bullet both accurately describe the actual behavior.

Still open from prior review passes

Both of these were flagged in the 2026-08-09 automated review of commit 1a833cd and are unchanged at the current tip:

• Comment/code name drift — the header comments (bin/compare_fresh_vs_update:23,29,108) still refer to a function extension_members(), but the function is actually named query() (line 113). Fix this →
• Schema-name generation psql call has no -d (bin/compare_fresh_vs_update:96, schema=$(psql -tAc "SELECT ...")) — every other psql call in this script, and every call in bin/test_existing, passes -d explicitly. Works today because CI's default connection targets a same-named database, but it's the one connection in the file with an implicit target.

New observation

• from_version is interpolated unescaped into a SQL string literal (bin/compare_fresh_vs_update:172-173, version_clause=" VERSION '$from_version'" → CREATE EXTENSION count_nulls WITH SCHEMA $schema_ident$version_clause). Today this is safe since CI always calls the script with the fixed literal 0.9.6, but the script documents itself as taking FROM_VERSION as a general CLI argument (USAGE: bin/compare_fresh_vs_update [FROM_VERSION]), so a caller-supplied value containing a ' would break out of the string. Same class of issue as the schema-escaping finding from the first review (informational/non-blocking given current call sites, but worth hardening if this script's argument surface is ever relied on with less-trusted input).

Verdict

No blockers. The two previously-flagged nits remain unaddressed after two review passes on this branch, plus one new informational observation on from_version escaping — none affect correctness of the comparison logic itself or the CI wiring.

[github-actions]

Claude finished @jnasbyupgrade's task in 1m 34s —— View job

---

Code review: PR #33 (Structural fresh-vs-update comparison + convergence/divergence report)

Task list

• Gather context (PR description, prior review comments, changed files)
• Diff origin/master..HEAD and read every changed file (bin/compare_fresh_vs_update, ci.yml, test/README.md) — three-dot diff has no merge-base in this shallow checkout, same limitation prior passes hit, so used two-dot and restricted to this PR's three changed files
• Check whether the three issues flagged in prior review passes are still present
• Post final review

---

Summary

New commit b1bc394 ("adapt to the shared random-schema install design") addresses all three issues raised in the prior automated review passes on this branch. No new issues found.

Previously-flagged issues — now resolved

• Comment/code name drift (extension_members() vs query()) — fixed. The header comments at bin/compare_fresh_vs_update:23,29,119 now consistently say query(), matching the actual function name.
• Schema-name-generation psql call missing -d — fixed. Line 107 now reads psql -d postgres -tAc "SELECT 'count_nulls test schema ' || ...", explicit and consistent with every other psql call in the file. The accompanying comment even explains why postgres specifically is the right target (neither scratch DB exists yet at that point).
• from_version interpolated unescaped into a SQL string literal — fixed. Line 83 now builds from_version_lit="${from_version//\'/\'\'}" (doubling embedded '), and line 183 uses from_version_lit in the VERSION '...' clause instead of the raw argument. The inline comment correctly documents this as hardening for a hypothetical less-trusted future caller, since CI itself only ever passes the fixed literal 0.9.6.

Re-verified still-correct behavior

• Both installs (fresh_db/update_db) share one schema name (schema_ident), generated once and reused via CREATE SCHEMA + CREATE EXTENSION ... WITH SCHEMA (never SET search_path), so pg_get_functiondef's schema-qualified output can't produce a false-positive diff.
• query() walks pg_depend/pg_extension/pg_proc live rather than a hardcoded object list, restricted to pg_proc — correct, since count_nulls only ever creates functions (including trigger functions), never a standalone CREATE TRIGGER.
• set -euo pipefail + trap cleanup EXIT drops both scratch DBs and temp files on every exit path; if diff -u ...; then ... else ...; fi correctly avoids tripping errexit on a genuine (expected) diff.
• ci.yml's new step runs after make verify-results TEST_LOAD_SOURCE=update, correctly ordered since the extension must already be installed. test/README.md's new bullet accurately describes the script's actual behavior.

Verdict

No blockers, no open issues. All prior nits are resolved and no new problems were introduced by this commit.

[github-actions]

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

---

Code review: PR #33 (Structural fresh-vs-update comparison + convergence/divergence report)

Task list

• Gather context (PR description, prior review history, changed files)
• Diff origin/master..HEAD (two-dot diff, since three-dot has no merge-base in this shallow checkout) and read every changed file
• Focus incremental review on the new commit since the last automated pass — 0365bf9 ("EXISTING_DB support, wired into pg-upgrade-test")
• Cross-check EXISTING_DB discovery logic against test/helpers/find_test_schema.sql/create_test_schema.sql and bin/test_existing's prepare-old/run-suite lifecycle
• Post final review

---

Summary

0365bf9 adds EXISTING_DB support to bin/compare_fresh_vs_update and wires it into the pg-upgrade-test job (.github/workflows/ci.yml, run once per twin database after bin/test_existing run-suite), plus a test/README.md update describing it. No bugs found.

What I checked specifically

• Schema discovery in EXISTING_DB mode (bin/compare_fresh_vs_update:~150-172) reimplements test/helpers/find_test_schema.sql's exact validate-then-select logic (hard failure on ≠1 matching schema, same 'count_nulls test schema %' pattern used by test/helpers/create_test_schema.sql and bin/test_existing prepare-old) rather than diverging from it — confirmed the LIKE pattern and validation are identical, so this can't silently pick up zero/multiple schemas differently than the rest of the suite would.
• Two-invocation split for the validation DO block vs. the SELECT nspname — the stated reason (a DO block's own command-completion tag prints under -tA, unlike RAISE NOTICE) is correct behavior, not a workaround for a self-inflicted issue.
• schema_ident quoting: the discovered schema name is re-escaped ("${schema//\"/\"\"}") before being interpolated into CREATE SCHEMA/CREATE EXTENSION ... WITH SCHEMA — this fully closes the identifier-escaping gap flagged in the very first review of this script, and here there's no untrusted-input surface at all since the value always originates from Postgres's own catalog.
• Lifecycle correctness: cleanup() only dropdbs update_db when existing_db is empty, so a caller-supplied EXISTING_DB (the real pg_upgraded database bin/test_existing still owns) is never touched — verified against bin/test_existing run_suite, which drops the guard but never the database itself, so it's still present when this script runs afterward in the CI step.
• Sequential reuse of the hardcoded fresh_db name: ci.yml's pg-upgrade-test step invokes the script twice in one run: | block (once per twin database). Since these run sequentially in the same shell step, each invocation's own trap cleanup EXIT drops compare_fresh_vs_update_fresh before the next invocation's createdb runs — no collision, though this would need revisiting if the two calls were ever parallelized.
• 9.4 compatibility note on why the two EXISTING_DB-mode SQL statements go through a single -c (multiple -c/-f per invocation wasn't supported before PG10) — checked against this repo's stated oldest-supported major and it's accurate.
• ci.yml wiring: the new pg-upgrade-test step runs after both run-suite calls (correct order — the databases must still hold their post-upgrade state), and test/README.md's new paragraph accurately describes the EXISTING_DB behavior actually implemented.

Prior-round findings

All three issues raised across earlier automated reviews on this branch (the extension_members()/query() comment drift, the schema-generation psql call missing -d, and unescaped from_version interpolation) were already resolved in b1bc394 and remain resolved at the current tip — nothing regressed by this newest commit.

Verdict

No blockers, no open issues.
(branch: phase6-compare-and-report)