Run bin/compare_fresh_vs_update against the real pg_upgraded database too - #35
Closed
jnasbyupgrade wants to merge 6 commits into
Closed
Run bin/compare_fresh_vs_update against the real pg_upgraded database too#35jnasbyupgrade wants to merge 6 commits into
jnasbyupgrade wants to merge 6 commits into
Conversation
test/install/load.sql now selects fresh/update/existing via the
count_nulls.test_load_mode GUC (TEST_LOAD_SOURCE make var), matching
pgxntool/README.asc's documented U&U pattern:
- fresh: CREATE EXTENSION count_nulls (unchanged from phase 1/2).
- update: CREATE EXTENSION VERSION '0.9.6', then ALTER EXTENSION UPDATE -
committed, since test/install runs outside any per-test rolled-back
transaction.
- existing: asserts count_nulls is already installed and current,
touches nothing - for a real pg_upgrade run external to this
invocation (a later phase adds the CI job that drives this).
Added the extension-update-test CI job: just `make verify-results
TEST_LOAD_SOURCE=update`, no external script needed - unlike a real
pg_upgrade, an in-place update is pure SQL, so test/install handles the
whole fresh-vs-updated comparison within one pg_regress invocation.
Verified locally against PG17: fresh, update, and existing modes all pass
via make verify-results, including update x TEST_SCHEMA=Quoted crossed
together - zero expected-output changes needed for any combination,
confirming phase 2's schema-invariant design holds across load modes too
(load-bearing for a later phase that crosses U&U with schema in CI).
existing mode verified manually against a real out-of-band CREATE
EXTENSION + --use-existing run.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rce PG list Independent of the U&U testing work itself, but best done now that multiple CI jobs exist and before the next phase adds the most expensive one (a real pg_upgrade job): - `changes` job: computes the actual per-push diff and skips test/ extension-update-test/pg-tle-test entirely on doc-only pushes, always triggering itself (no workflow-level paths-ignore, which would leave all-checks-passed stuck Pending on doc-only pushes in branch protection). - Derives the supported-PostgreSQL-major list from ONE set of constants (NEWEST/FLOOR) in that same job, consumed by both the `test` and `extension-update-test` matrices via fromJSON - they can't silently drift onto different lists, and a new major is a one-line change. - `all-checks-passed`: single stable required-status-check name, with a self-check that its own needs list can't silently omit a newly-added job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the pg-upgrade-test CI job: install 0.9.6 on an old PostgreSQL major, plant + prove a 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. bin/test_existing is much smaller than the equivalent script would have been pre-test/install: only prepare-old and run-suite are genuinely external-to-pg_regress concerns (a real pg_upgrade binary run isn't something pg_regress can invoke itself), plus a small `update` subcommand for the post-upgrade ALTER EXTENSION UPDATE step. There's no update-scenario subcommand at all - that entire scenario is just `make test-update` now (test/install/load.sql's own 'update' mode, added in phase 3), 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` - pgxntool 2.3.0 (this repo's phase 0) already made `make test` itself exit non-zero on regression failures. Not yet crossed with TEST_SCHEMA - that's the next phase, once both this job and extension-update-test can cross it together. Verified locally against PG17 (prepare-old -> update -> run-suite, without a real pg_upgrade - this container's clusters are persistent shared infra, so the actual binary pg_upgrade leg is left for CI's ephemeral containers, same reasoning as the pg-tle-test work). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The novel piece this whole redesign was building toward: nobody in this org currently tests update/upgrade crossed with schema scenarios (checked directly - cat_tools' own extension-update-test/pg-upgrade-test matrices are PG-version-only, extension_tools has no U&U testing at all). Phase 2's schema-invariant assertion descriptions make this free - zero new expected-output files needed for either job, exactly as designed. - extension-update-test: added `schema: ["", Quoted]` to the matrix, TEST_SCHEMA env var - the job's own `make verify-results TEST_LOAD_SOURCE=update` picks it up automatically via Make's environment auto-import. - pg-upgrade-test: added the same schema axis (old_pg/new_pg were already plain matrix dimensions, not an `include:` list, so adding a third axis cross-products cleanly into the same 4 legs cat_tools would produce for 2 old_pg values x 2 schema values), and threaded matrix.schema through to bin/test_existing's prepare-old/run-suite calls (previously hardcoded to ""). Verified locally against PG17: prepare-old -> update -> run-suite passes end to end with TEST_SCHEMA=Quoted (previously only verified with an untargeted schema). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_update) Adds the one checklist item genuinely missing everywhere (cat_tools' own PR #46 only did this manually, uncommitted, per the design doc this whole series is based on): a script that installs fresh and 0.9.6-then-updated copies of count_nulls in their own scratch databases and diffs pg_get_functiondef/comments/ACLs for every object the extension owns (discovered live via pg_depend, not a hardcoded object list). Wired into extension-update-test as an automatic step, crossed with TEST_SCHEMA same as the rest of that job. Also scopes out extending pg-tle-test to the update path (documented inline, and filed as Postgres-Extensions/pgxntool#90): pgxntool 2.3.0's own fix for installcheck's ordering bug (#83) made installcheck unconditionally depend on install, which writes a real .control file to disk - defeating the entire point of proving a pg_tle deployment never touches the filesystem. There's currently no way to invoke the real pgTAP suite without a filesystem install happening first. The fresh-install pg_tle smoke test (already on master via #16) is unaffected, since it never calls installcheck. Verified locally against PG17: fresh/update x empty/Quoted all pass via make verify-results; bin/compare_fresh_vs_update reports identical definitions for both schema legs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… too extension-update-test already runs the structural comparison against its own scratch fresh/updated database pair, but pg-upgrade-test only ran the fixed pgTAP suite against the actual binary-pg_upgraded + updated database - a divergence introduced by surviving a real catalog migration (as opposed to only an in-place ALTER EXTENSION UPDATE) would go uncaught. Add an optional EXISTING_DB argument so the script can compare against an already-populated database instead of always creating and updating its own, and wire a new step into pg-upgrade-test that points it at count_nulls_upgrade. Verified locally (this container's PG17, no real pg_upgrade run - that's shared persistent dev infra): confirmed existing scratch-db usage still passes, confirmed the new EXISTING_DB path passes against a manually simulated update-only database and leaves it undropped afterward, then introduced a deliberate COMMENT ON divergence and confirmed the script correctly fails and still leaves the caller-owned database in place.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
jnasbyupgrade
force-pushed
the
phase6-compare-and-report
branch
from
August 4, 2026 18:49
4c3ea52 to
dca71f7
Compare
Contributor
Author
|
Superseded by #38: pg-upgrade-test's design changed since this branch was cut (the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
extension-update-test already runs the structural comparison against its own scratch fresh/updated database pair, but pg-upgrade-test only ran the fixed pgTAP suite against the actual binary-pg_upgraded + updated database - a divergence introduced by surviving a real catalog migration (as opposed to only an in-place ALTER EXTENSION UPDATE) would go uncaught. Add an optional EXISTING_DB argument so bin/compare_fresh_vs_update can compare against an already-populated database instead of always creating and updating its own, and wire a new step into pg-upgrade-test that points it at count_nulls_upgrade.
Test plan
COMMENT ONdivergence and confirmed the script correctly fails and still leaves the caller-owned database in place