Fix cat_tools dependency: install from pinned git SHA, not PGXN - #10
Draft
jnasbyupgrade wants to merge 5 commits into
Draft
Fix cat_tools dependency: install from pinned git SHA, not PGXN#10jnasbyupgrade wants to merge 5 commits into
jnasbyupgrade wants to merge 5 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>
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.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