Check for structural differences between CREATE and UPDATE EXTENSION - #55
Merged
jnasbyupgrade merged 3 commits intoAug 2, 2026
Merged
Conversation
…ire it into bin/test_existing's update-scenario/update-check flows. Postgres-Extensions#46 found a real fresh-vs-update divergence (cat_tools.trigger__parse, as produced by the pre-0.2.2 update scripts, hardcoded EXECUTE PROCEDURE and skipped an empty-args guard that the fresh-install body had, breaking every trigger parse on PG11+) by manually diffing pg_get_functiondef/pg_get_viewdef/type labels/comments/ACLs/extension membership between a fresh install and an updated database. That comparison was never committed as tooling, so nothing would catch a recurrence. bin/structural_diff generalizes it: given two databases, it dumps every object belonging to the cat_tools extension (via pg_depend) with a kind-appropriate structural signature -- function/view definitions, enum labels, the one plain table's and one standalone composite type's columns, cast definitions, comments, and sorted ACLs -- and diffs the two dumps, failing on any nonempty difference. bin/test_existing's update-scenario and update-check now call this automatically (via a new assert_matches_fresh helper) against a throwaway fresh install of the same target version, so every existing extension-update-test CI leg (0.2.0/0.2.1/0.2.2 origins reaching 0.2.2, 0.2.3, or the current version) gets this check for free with no separate wiring in ci.yml. A new diff-fresh subcommand also exposes the same check standalone for interactive use. Running the new tool surfaced two more real, if purely cosmetic (comment/whitespace-only, no logic change), fresh-vs-update divergences predating this change: sql/cat_tools--0.2.3.sql.in (the still-unpublished fresh-install script for 0.2.3) was missing the same two comment tweaks PR Postgres-Extensions#46 applied to the update script and to sql/cat_tools.sql.in, and sql/cat_tools--0.2.3--0.3.0.sql.in had drifted from sql/cat_tools.sql.in across eleven functions (stale comments, an unconverged trigger__parse body predating PR Postgres-Extensions#46's fix, and a couple of incidental trailing-whitespace/comment differences). Both are fixed here by converging the stale copies to match the current fresh source, verified via bin/structural_diff itself plus a full pgTAP suite run on PG12 and PG17. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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 |
… and stop asserting fresh-parity at a landing point that can never reach it
CI's new PG10 leg failed: bin/test_existing's assert_matches_fresh (wired
into update_check by the structural-diff-tooling work) asserted that
updating from 0.2.0/0.2.1 to 0.2.2 produces objects identical to a fresh
0.2.2 install. That assertion can never hold -- the trigger__parse body and
pg_class_v's omit_column bug are KNOWN divergences, deliberately repaired
later in cat_tools--0.2.2--0.2.3.sql.in rather than by editing the
already-published 0.2.0->0.2.2 / 0.2.1->0.2.2 scripts. Split update_check
into update_check_version (version-only) and update_check (adds the
structural diff), and use update_check_version for the 0.2.2 landing point
in ci.yml, keeping the full check where convergence is actually expected
(the 0.2.3 target).
The same run also surfaced a genuine, previously-undetected divergence: the
five enum types created back in 0.2.0 (constraint_type, procedure_type,
relation_type, relation_relkind, object_type) never receive their
cat_tools__usage USAGE grant when reached via the update path, because
ALTER DEFAULT PRIVILEGES only applies to objects created after it runs, and
these types predate it. A fresh install is unaffected because it creates
them after the statement. Added an idempotent retroactive GRANT to
sql/cat_tools--0.2.2--0.2.3.sql.in to converge the ACL for every update
origin.
Also fixed assert_matches_fresh's `local db=$1 fresh_db="${db}__fresh_ref"`,
whose self-reference within one `local` statement is bash-version-dependent
under `set -u` (failed locally on bash 5.2, apparently fine on CI's bash);
split into two statements to remove the dependency.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… fix forward into 0.2.3->0.3.0 instead. The 0.2.3 tag (upstream, cut at PR Postgres-Extensions#46's merge commit) makes sql/cat_tools--0.2.3.sql.in and sql/cat_tools--0.2.2--0.2.3.sql.in immutable per CLAUDE.md's SQL file conventions rule 5 -- the same rule PR Postgres-Extensions#46 itself followed by converging its trigger__parse fix forward into 0.2.2->0.2.3 rather than editing the already-published 0.2.0->0.2.2/0.2.1->0.2.2 scripts. This branch had violated that rule in two ways: reformatting two comments in the frozen fresh-install script (cosmetic, and wrong to touch regardless of content), and adding the retroactive type-ACL GRANT to the frozen 0.2.2->0.2.3 update script (a real fix, but landed one version too early). Both files are reverted here to be byte-identical with the 0.2.3 tag. The type-ACL fix (a GRANT USAGE ON TYPE for the five enum types that predate 0.2.2 and never picked up the cat_tools__usage grant via the update path) now lives in sql/cat_tools--0.2.3--0.3.0.sql.in instead, which is not yet tagged and is exactly where PR Postgres-Extensions#46's own precedent says a fix like this belongs: the first still-unpublished update script downstream of the gap. Reverting the comment reformatting also surfaces a second, previously-masked divergence: sql/cat_tools--0.2.3.sql.in (frozen) never received PR Postgres-Extensions#46's comment-style tweaks to relation__kind and trigger__parse, so ANY update path landing on 0.2.3 -- not just a 0.2.0/0.2.1 origin -- now diverges from a fresh install pinned at exactly 0.2.3. bin/test_existing's update-check-version (added structural-diff-free version-only checking) now covers every 0.2.3 landing point in ci.yml's extension-update-test job, not just the ones affected by the type-ACL gap; update-check (with the structural comparison) stays reserved for a target where fresh-parity is actually expected, which today means the current/0.3.0 version reached via update-scenario. Verified locally: both frozen files diff empty against the 0.2.3 tag, the full pgTAP suite and make lint pass, and a hand-simulated 0.2.0/0.2.1-shaped ACL gap is confirmed present at the 0.2.3 landing point and repaired by the time the same database reaches current, matching a fresh current install byte-for-byte. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 2, 2026
jnasbyupgrade
changed the title
jnasbyupgrade
added a commit
to jnasbyupgrade/cat_tools
that referenced
this pull request
Aug 3, 2026
…t` job, shrink extension-update-test to PG10-only extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as the `test` job (supported_pg, 12-18), but as its own matrix job: its own runner, container boot, checkout, apt-get, and `make install`, paid again per major, for a check that can run as one more step inside a container the `test` job already has running, already checked out, and already has cat_tools installed on disk in (installcheck, a TEST_DEPS prerequisite, already ran as a side effect of that job's own verify-results/test-long calls). Folded `bin/test_existing update-scenario cat_tools_update 0.2.2` in as an additional call in the `test` job's "Test on PostgreSQL" step instead. Verified before folding it in, not assumed: ran `make check-relkind-source && make verify-results && make test-long && bin/test_existing update-scenario cat_tools_update 0.2.2` in the same shell/cluster session (mirroring the new CI step exactly) against a scratch cluster. Confirmed no database-name collision (pg_regress's own throwaway db is named independently from `cat_tools_update`), the dependency-guard proof fires (twice -- once right after CREATE EXTENSION, once again after the full suite run), the structural-diff check (bin/structural_diff, landed via PR Postgres-Extensions#55 since this branch's last rebase) fires and reports the updated database structurally identical to a fresh install, and the full suite passes -- exit 0 end to end. extension-update-test now runs PG10 only, with no matrix at all (single source of truth: needs.changes.outputs.legacy_pg, not a hardcoded "10") -- its entire remaining purpose is the pre-0.2.2 legacy-script checks, the only place those scripts still load. Removed the now-dead `if: matrix.pg != '10'` / `if: matrix.pg == '10'` guards throughout that job (nothing left to guard against once there's no other leg) and the "Update 0.2.2 -> current" step (moved above). The `changes` job's `update_pg` output/derivation (supported_pg + legacy_pg) is removed too -- it had exactly one consumer, and that consumer is gone. Restructured the top-of-file "Test strategy" comment around what actually matters to a reader: what runs on EVERY supported PostgreSQL major (the `test` job, now including the guard-proved update check, and `pg-tle-test` as a second, independently-isolated main-matrix job -- kept separate deliberately, since it specifically proves pg_tle deployment ISOLATION, not filesystem-install coincidence, unlike the fold above) versus SPECIAL CASES that apply to one specific scenario only (extension-update-test's PG10 legacy scripts, pg-upgrade-test's/pg-tle-upgrade-test's specific binary-pg_upgrade jump pairs, and pg-upgrade-stepwise's full climb). No coverage lost: the PG12+ update-to-current check still runs on the exact same 7 majors it always did (moved, not removed), the PG10 legacy checks are byte-for-byte unchanged, and every other job is untouched.
This was referenced Aug 3, 2026
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.
#46 found a real fresh-vs-update divergence (cat_tools.trigger__parse, as produced by the pre-0.2.2 update scripts, hardcoded EXECUTE PROCEDURE and skipped an empty-args guard that the fresh-install body had, breaking every trigger parse on PG11+) by manually diffing pg_get_functiondef/pg_get_viewdef/type labels/comments/ACLs/extension membership between a fresh install and an updated database. That comparison was never committed as tooling, so nothing would catch a recurrence. bin/structural_diff generalizes it: given two databases, it dumps every object belonging to the cat_tools extension (via pg_depend) with a kind-appropriate structural signature -- function/view definitions, enum labels, the one plain table's and one standalone composite type's columns, cast definitions, comments, and sorted ACLs -- and diffs the two dumps, failing on any nonempty difference.
bin/test_existing's update-scenario and update-check now call this automatically (via a new assert_matches_fresh helper) against a throwaway fresh install of the same target version. A new diff-fresh subcommand also exposes the same check standalone for interactive use. A companion update-check-version subcommand covers landing points pinned at an already-tagged version (0.2.2, 0.2.3), where full structural parity can never be achieved without editing an immutable, already-shipped file (CLAUDE.md's SQL file conventions rule 5) -- it asserts only that the version landed, leaving the full structural check for a landing point the update path actually converges to (the current, still-unpublished version).
Running the new tool immediately proved its worth: it surfaced two real, previously-undetected divergences in sql/cat_tools--0.2.3--0.3.0.sql.in (a missing type-ACL grant on five pre-0.2.2 enum types, and ~11 functions that had drifted from the fresh-install source). Both are fixed in #68 (merged) rather than here, to keep this PR scoped to the tooling itself. That file still shows up in this PR's own diff below -- this branch predates #68 -- but its content is byte-identical to what #68 already merged, so it's a no-op once this merges, not a duplicate change.
What this covers, and what it doesn't
bin/structural_diff.sql finds cat_tools's extension members via pg_depend (deptype = 'e', matching what \dx+ shows) and renders a structural signature per member, keyed on catalog + kind:
Implicit row types of member relations and the array-type shadow of any member type are deliberately skipped -- they're fully described by the relation/base-type entry already, so including them too would just duplicate that comparison under a second identity. An unrecognized object kind still gets compared on identity/comment/ACL via a fallback branch, rather than silently vanishing from the dump.
This intentionally only builds out the object kinds cat_tools actually has today (checked against sql/cat_tools.sql.in) -- no domains, no operators/aggregates, etc. -- per the task's own guidance not to build generic infra for kinds this extension doesn't use.
bin/test_existing gains three pieces:
.github/workflows/ci.yml's extension-update-test job now calls update-check-version for every landing point pinned at an already-tagged version (0.2.2 and 0.2.3), so no ci.yml wiring is needed beyond that -- every future update-origin leg automatically gets the appropriate check.
Testing