Fix cat_tools dependency: install from pinned git SHA, not PGXN - #10
Open
jnasbyupgrade wants to merge 6 commits into
Open
Fix cat_tools dependency: install from pinned git SHA, not PGXN#10jnasbyupgrade wants to merge 6 commits into
jnasbyupgrade wants to merge 6 commits into
Conversation
extension_drop.sql calls cat_tools.routine__parse_arg_types_text(), which only exists starting at cat_tools 0.3.0. PGXN's published cat_tools listing is stuck at 0.2.1 from 2017 (the old decibel/cat_tools distribution) and was never updated for the Postgres-Extensions/cat_tools fork, so `pgxn install 'cat_tools>=0.2.1'` silently installed an ancient 0.2.1 that both lacks the function extension_drop needs and carries a pre-omit_column-fix catalog query that breaks on PG12+. As a result, CREATE EXTENSION extension_drop CASCADE has likely never actually succeeded in CI -- masked by pgxntool's .IGNORE: installcheck and by pg-build-test not gating on regression results. cat_tools 0.3.0 has the needed function on GitHub master but has not been tagged/published to PGXN yet (only 0.2.2/0.2.3 are tagged, and even 0.2.3 predates the function). So PGXN isn't a usable install source right now: build cat_tools from a pinned git commit instead. Once 0.3.0 is tagged and published to PGXN, this should revert to a plain `pgxn install 'cat_tools>=0.3.0' --sudo`. Also bump META.in.json's declared cat_tools floor to 0.3.0 to match the real requirement, and regenerate META.json via `make META.json`. 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 |
This reverts commit fb4931d.
The Makefile comment and README previously read as if pinning cat_tools to a git SHA were only a stopgap to be ripped out. Reword to separate the two claims: this specific SHA pin is temporary (revert once cat_tools 0.3.0 is tagged on PGXN), but building a dependency straight from a pinned git commit is a supported technique here whenever a dependency's PGXN listing lags its real development -- worth reaching for again, not something to design away entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No tagged cat_tools release has the function we need, so there's nothing meaningfully more stable to pin to than master right now -- the fixed SHA added false precision without a real reproducibility benefit. Track the branch directly; the git-source-instead-of-PGXN mechanism itself is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A real release must never be cut while a dependency is temporarily pinned to a git source (like the cat_tools pin added in Postgres-Extensions#10): the resulting release's declared dependency floor wouldn't actually be resolvable via `pgxn install` for anyone downloading it from PGXN. Document this in RELEASE.md and add a small predicate script, bin/in_release, that reports whether we're at/cutting a release (HISTORY.asc's top heading is a real version) versus mid-development (heading is the `STABLE` placeholder) -- for now a manual check to run before releasing, not yet wired into CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
marked this pull request as ready for review
August 4, 2026 18:36
jnasbyupgrade
added a commit
to jnasbyupgrade/extension_tools
that referenced
this pull request
Aug 4, 2026
…redate this branch CI on this branch showed the switch to `make test && make verify-results` surfacing real pgTAP failures on PostgreSQL 9.3/9.6 (cat_tools/extension_drop never actually install there). Checked PR Postgres-Extensions#10's own baseline CI (Postgres-Extensions#10, run 30665031257): PG 9.3 and 9.6 already report "3 of 3 tests failed" in the raw job log there too, just silently reported as a passing check because pg-build-test's underlying `make test` hits pgxntool's `.IGNORE: installcheck` the same way. So this isn't a regression from this PR's own changes -- it's the exact masking problem RELEASE.md already documents, just now applying to a different, older part of the PG matrix than the PRs (#6/#7) it originally cites. Reverting the ci.yml step back to pg-build-test here keeps this PR scoped to test/install infrastructure; fixing cat_tools's install path on pre-PG10 belongs to whoever owns that dependency setup (PR Postgres-Extensions#10 or a follow-up), not this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/extension_tools
that referenced
this pull request
Aug 4, 2026
actions/checkout v4.4.0 (backported to all major-version tags) added a new default-on refusal for checking out a fork PR's head under pull_request_target, since it can't see that this job is already gated to the trusted jnasbyupgrade fork only (see the if: condition and SECURITY comment above) and never builds or executes the fetched code. Root-caused via the actual failed run logs on PRs Postgres-Extensions#10/Postgres-Extensions#14/Postgres-Extensions#15, which all failed at this checkout step with: Refusing to check out fork pull request code from a 'pull_request_target' workflow. ... set 'allow-unsafe-pr-checkout: true' on the actions/checkout step. Without this, track_progress: true (added in the prior commit) never had a chance to matter -- the workflow was failing before the review step ever ran.
This was referenced Aug 4, 2026
jnasbyupgrade
added a commit
to jnasbyupgrade/extension_tools
that referenced
this pull request
Aug 4, 2026
…redate this branch CI on this branch showed the switch to `make test && make verify-results` surfacing real pgTAP failures on PostgreSQL 9.3/9.6 (cat_tools/extension_drop never actually install there). Checked PR Postgres-Extensions#10's own baseline CI (Postgres-Extensions#10, run 30665031257): PG 9.3 and 9.6 already report "3 of 3 tests failed" in the raw job log there too, just silently reported as a passing check because pg-build-test's underlying `make test` hits pgxntool's `.IGNORE: installcheck` the same way. So this isn't a regression from this PR's own changes -- it's the exact masking problem RELEASE.md already documents, just now applying to a different, older part of the PG matrix than the PRs (#6/#7) it originally cites. Reverting the ci.yml step back to pg-build-test here keeps this PR scoped to test/install infrastructure; fixing cat_tools's install path on pre-PG10 belongs to whoever owns that dependency setup (PR Postgres-Extensions#10 or a follow-up), not this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 4, 2026
…redate this branch CI on this branch showed the switch to `make test && make verify-results` surfacing real pgTAP failures on PostgreSQL 9.3/9.6 (cat_tools/extension_drop never actually install there). Checked PR #10's own baseline CI (#10, run 30665031257): PG 9.3 and 9.6 already report "3 of 3 tests failed" in the raw job log there too, just silently reported as a passing check because pg-build-test's underlying `make test` hits pgxntool's `.IGNORE: installcheck` the same way. So this isn't a regression from this PR's own changes -- it's the exact masking problem RELEASE.md already documents, just now applying to a different, older part of the PG matrix than the PRs (#6/#7) it originally cites. Reverting the ci.yml step back to pg-build-test here keeps this PR scoped to test/install infrastructure; fixing cat_tools's install path on pre-PG10 belongs to whoever owns that dependency setup (PR #10 or a follow-up), not this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
What was broken
extension_drop.sqlcallscat_tools.routine__parse_arg_types_text(), which only exists starting at cat_tools 0.3.0. TheMakefile'scat_toolstarget andMETA.in.json's declared floor both said>=0.2.1and usedpgxn install.PGXN's published cat_tools listing (https://api.pgxn.org/dist/cat_tools.json) is stuck at 0.2.1 from 2017 -- it's the old
decibel/cat_toolslisting and was never updated for thePostgres-Extensions/cat_toolsfork's newer work. Sopgxn install 'cat_tools>=0.2.1'in CI has actually always installed that ancient 0.2.1, which:cat_tools.routine__parse_arg_types_textat all (added in 0.3.0), soCREATE EXTENSION extension_drop CASCADEhas likely never actually succeeded.omit_column-fix catalogSELECT *bug that breaks withcolumn "oid" specified more than onceon PG12+.Why PGXN can't be used right now
cat_tools's GitHub master has bumped to
default_version = '0.3.0'and has the function extension_drop needs, but 0.3.0 has not been tagged or published to PGXN. Only0.2.2/0.2.3are tagged there, and even0.2.3predates the function. So there is currently no PGXN-published version of cat_tools that satisfies extension_drop's real requirement.The fix
Makefile:cat_toolstarget now clonesPostgres-Extensions/cat_toolsand builds/installs it from itsmasterbranch (tracked directly, not pinned to a commit SHA -- no tagged release has the function we need, so there's nothing more stable to pin to yet) instead ofpgxn install. A comment at the fix site distinguishes the two claims: depending on cat_tools this way at all is temporary and will be reverted once 0.3.0 is tagged on PGXN, but building a dependency from a git ref is itself a supported technique in this Makefile -- worth reusing whenever a dependency's PGXN listing lags its real development, not something to design away.META.in.json: bumped the declaredcat_toolsbuild floor from0.2.1to0.3.0-- an honest reflection of what the code actually requires, independent of the install mechanism. RegeneratedMETA.jsonviamake META.json(never hand-edited).README.md: updated the stated cat_tools requirement/link to match, and noted the temporary git-source install.extension_drop.control'srequires = 'cat_tools'is left as-is: PostgreSQL.controlrequiresis a bare extension-name list (forCREATE EXTENSION ... CASCADE) with no version syntax, so no change was needed there.Release safety
Cutting a real release while a dependency pin like this one is active would produce a citable, default-installable release whose declared dependency floor can't actually be resolved via
pgxn install. Added a callout toRELEASE.mdplus a small helper,bin/in_release(true whenHISTORY.asc's top heading is a real version rather than theSTABLEin-development placeholder), to check for this before releasing. Manual for now, not wired into CI -- doing that needs a generic way to detect "a pin exists" across whatever dependency needs one, not justcat_tools.Related
Filed issue #11 to track a longer-term idea surfaced while working on this: a CI axis that tests the declared floor version of build/runtime dependencies (not just PG version), so a dependency floor like
cat_tools 0.3.0can't silently drift out of sync with what the code actually uses. Not implemented here -- cat_tools' floor isn't even testable until 0.3.0 is tagged.Exit condition
Once cat_tools 0.3.0 is tagged and published to PGXN, revert the
Makefile'scat_toolstarget to a plainpgxn install 'cat_tools>=0.3.0' --sudo(the comment at the fix site says so explicitly).Verification
I confirmed via the CI job logs (using a temporary debug step I added and then reverted) that the git-clone/checkout/
make installof the pinned cat_tools SHA succeeds cleanly on every PostgreSQL version in the matrix (9.3 through 17), installingcat_tools.controlwithdefault_version = '0.3.0'each time. That part of the fix is solid and verified end-to-end.However, the
schema/simplepgTAP tests still fail in CI, for a separate, pre-existing reason unrelated to cat_tools: CI'spg-build-test(from thepgxn/pgxn-toolsimage) invokesmake installcheckdirectly, which does not go through pgxntool's owntesttarget or itstestdepsprerequisite.testdepsis what installs thetest/extension/support extension (extension_drop_test) thattest/sql/schema.sqlandtest/sql/simple.sqldepend on. Sincepg-build-testskips that step, those tests fail withextension "extension_drop_test" is not availablein CI, regardless of cat_tools. Locally, running pgxntool's actualmake test(which does runtestdepsfirst) installsextension_drop_testcorrectly, and with this fix,schema/simplethen pass cleanly end-to-end (verified locally against PG17). This CI-invocation gap predates this branch and is out of scope for this PR; flagging it here so it isn't confused with a cat_tools regression. (There's also a related one-character pre-existing typo in thisMakefile, line 4:$(DESTDIR)$datadir)/...is missing a(beforedatadir-- harmless today since it just makes that target's dependency tracking always stale/rebuild, but worth a follow-up cleanup.)Separately,
zzz_buildfails locally and in CI for an unrelated, already-merged reason: it playssql/extension_drop.sqldirectly via\irather than throughCREATE EXTENSION, so it never benefited fromCREATE EXTENSION's automaticclient_min_messagesraise-to-WARNING that PR #3 (224e89d, "Remove redundant client_min_messages handling from install script") relied on when it dropped the script's own explicit save/restore. Also pre-existing, also out of scope here.None of this is gating today: pgxntool's
.IGNORE: installcheckpluspg-build-testnot gating on regression results means CI shows green regardless (as noted in the original bug report). This PR'sall-checks-passedis green, and the cat_tools install itself, the actual subject of this PR, is verified working on every PG version.Test plan
make install) succeeds on all 12 PG versions in CI (9.3-17)schemaandsimple, the tests that exercise cat_tools, pass cleanly oncetestdepshas installedextension_drop_test(i.e. via pgxntool's ownmake test)pg-build-testinvocation exercise pgxntool'stestdeps/testtarget soschema/simpleactually run to completion in CIzzz_buildNOTICE regression from PR Remove redundant client_min_messages handling from install script #3🤖 Generated with Claude Code