Switch to pgxn-tools based testing - #5
Conversation
e9c24de Fix pg_regress on versions > 12 (#5) c0af00f Improvements to HISTORY.asc 6e8f2a7 Allow use of sudo when installing an extension 705f1ec Don't run clean as part of make test 370fa8e Create test/sql during setup git-subtree-dir: pgxntool git-subtree-split: e9c24de986ddc85bbd1fb3149076888d075ce100
Ditch the old travis setup. Pulls in some pgxntool changes as well.
📝 WalkthroughWalkthroughAdds GitHub Actions CI for PostgreSQL versions 17 through 10 using the Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant pgxn/pgxn-tools
participant PostgreSQL
GitHub Actions->>pgxn/pgxn-tools: Run make test with PGUSER=postgres
pgxn/pgxn-tools->>PostgreSQL: Execute tests for matrix version
PostgreSQL-->>pgxn/pgxn-tools: Return test results
pgxn/pgxn-tools-->>GitHub Actions: Return test status
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a
permissionsblock to restrict default token permissions.The workflow has no
permissions:block, so theGITHUB_TOKENgets the repo's default permissions, which may include write access to contents, packages, etc. Since this job only runs tests, it should use read-only or empty permissions.🔒️ Proposed fix
name: CI on: [push, pull_request] +permissions: + contents: read jobs: test:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 18, Add a top-level permissions block to the CI workflow, before jobs, granting the GITHUB_TOKEN no permissions (or only the minimum read access required by actions/checkout). Keep the existing test job, PostgreSQL matrix, and steps unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 10-15: Pin the pgxn/pgxn-tools container to an immutable image
digest and pin actions/checkout to a full commit SHA instead of mutable tags. In
the checkout step, set persist-credentials to false.
In `@pgxntool/base.mk`:
- Around line 60-64: Fix the version-gated condition in the Makefile by changing
the malformed `ifeq` expression to use `$(call test, $(MAJORVER), -lt, 130)`,
matching the argument pattern used by the existing condition and the scaled
`MAJORVER` values. Keep the `REGRESS_OPTS += --load-language=plpgsql` assignment
unchanged.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-18: Add a top-level permissions block to the CI workflow, before
jobs, granting the GITHUB_TOKEN no permissions (or only the minimum read access
required by actions/checkout). Keep the existing test job, PostgreSQL matrix,
and steps unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7a47839-f028-4d74-87c1-b3482fecc296
📒 Files selected for processing (9)
.github/workflows/ci.yml.gitignore.travis.ymlpg-travis-test.shpgxntool/HISTORY.ascpgxntool/base.mkpgxntool/setup.shsql/.object_reference.sql.swotest/dump/run.sh
💤 Files with no reviewable changes (2)
- .travis.yml
- pg-travis-test.sh
| endif | ||
|
|
||
| #DATA = $(wildcard sql/*--*.sql) | ||
| ifeq ($($call test, $(MAJORVER), -lt 13), yes) | ||
| REGRESS_OPTS += --load-language=plpgsql | ||
| endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix Makefile syntax error: --load-language=plpgsql is never added for any PostgreSQL version.
Line 62 has two defects that prevent the version-gated REGRESS_OPTS addition from ever executing:
-
$($calltypo — should be$(call. The extra$causes GNU Make to interpret$cas a single-char variable reference (undefined → empty), making the outer$(all test, …)resolve to an undefined variable → empty string. Theifeqalways evaluates to false. -
Missing comma and wrong comparison value —
-lt 13is passed as a single argument to thetestfunction (which expects 3 comma-separated args), and the value should be130not13becauseMAJORVERis already multiplied by 10 (line 51: PG 12 → 120, PG 13 → 130). Even with the$(callfix,test 120 -lt 13is false for PG 12.
Compare with the correct pattern on line 56: $(call test, $(MAJORVER), -ge, 91).
🐛 Proposed fix for line 62
-ifeq ($($call test, $(MAJORVER), -lt 13), yes)
+ifeq ($(call test, $(MAJORVER), -lt, 130), yes)
REGRESS_OPTS += --load-language=plpgsql
endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| endif | |
| #DATA = $(wildcard sql/*--*.sql) | |
| ifeq ($($call test, $(MAJORVER), -lt 13), yes) | |
| REGRESS_OPTS += --load-language=plpgsql | |
| endif | |
| endif | |
| ifeq ($(call test, $(MAJORVER), -lt, 130), yes) | |
| REGRESS_OPTS += --load-language=plpgsql | |
| endif |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pgxntool/base.mk` around lines 60 - 64, Fix the version-gated condition in
the Makefile by changing the malformed `ifeq` expression to use `$(call test,
$(MAJORVER), -lt, 130)`, matching the argument pattern used by the existing
condition and the scaled `MAJORVER` values. Keep the `REGRESS_OPTS +=
--load-language=plpgsql` assignment unchanged.
|
@claude please review this PR. |
|
Claude finished @jnasbyupgrade's task in 2m 51s —— View job Review: Switch to pgxn-tools based testing
SummaryThis is a clean re-application of the original pgxn-tools CI migration (matches the 1. Confirmed: pre-existing This reproduces on every PostgreSQL version tested (10–17), confirming this is the pre-existing bug the PR description says is fixed separately in the 2. Real bug (independent of the above): ifeq ($($call test, $(MAJORVER), -lt 13), yes)
REGRESS_OPTS += --load-language=plpgsql
endifTwo problems:
Correct form: ifeq ($(call test, $(MAJORVER), -lt, 130), yes)
REGRESS_OPTS += --load-language=plpgsql
endifThis means PG <13 regression runs are silently missing 3. Optional hardening (not blocking):
None of these affect correctness or the current failures; worth doing at some point but not a reason to hold this PR. 4. Minor/unrelated: the 5. Minor suggestion: the matrix has no Bottom lineThe CI plumbing itself (workflow file, dump script |
# Conflicts: # .gitignore # pgxntool/HISTORY.asc # pgxntool/base.mk # pgxntool/setup.sh
Vendor Postgres-Extensions/linter as a git submodule at .vendor/linter,
following the same pattern already adopted in cat_tools: a thin
self-initializing lint.mk hand-off (so `make lint` works right after a
plain `git clone`, no --recurse-submodules needed), LINT_TARGETS scoped
to sql/object_reference.sql and test/ (excluding the frozen, never
hand-edited versioned install files under sql/, e.g.
object_reference--0.1.0.sql/--stable.sql), and a CI job that runs
`make lint` directly -- the same entry point a developer uses locally
-- so the self-init logic is actually exercised, not just the rule
checking.
The `include lint.mk` is guarded on .git being present: a tarball build
(PGXN distribution, `git archive` with no .git) has no submodule to
initialize, and Make resolves every `include` before running any
target regardless of which one was requested, so an unguarded rule
would break `make`/`make install` entirely for a tarball build, not
just `make lint`.
Fixes the real pre-existing style findings this first run turned up
(52 total): most were commented-out SQL marked as prose comments
instead of using the linter's `EXCLUDED CODE` disabled-code convention
(missing " * " prefixes flagged as comment-line-prefix/comment-opening
violations); one COPY data block's `secondary` column intentionally
mirrors pg_catalog's own type display name ("integer" for int4) rather
than following prefer-short-type, so it's suppressed via a scoped
disable-block region instead of being "fixed" into incorrect test data.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…able pgxn install --unstable cat_tools resolves to the newest release actually published to the PGXN package index, which is still 0.2.1 (2017) -- it fails standalone on modern PostgreSQL with "column oid specified more than once" at CREATE EXTENSION. cat_tools 0.3.0 fixes this but hasn't been uploaded to PGXN yet, only tagged in git, so build it from that tag directly until PGXN has it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cat_tools 0.3.0 is the first version of cat_tools this extension's test suite has ever actually run against (pgxn install --unstable previously resolved to a version that failed CREATE EXTENSION outright, so none of this was exercised before now): - cat_tools.function__arg_types_text() is deprecated in 0.3.0 in favor of cat_tools.routine__parse_arg_types_text() (same signature, same body, just renamed) and now emits a WARNING on every call. Switch to the non-deprecated name directly rather than carrying the warning forward. - cat_tools 0.3.0's object_type enum gained two new members, "partitioned table" and "partitioned index". object_reference has no per-object-type special-casing (object identity is tracked generically via classid/objid), so these are already handled by the existing code the same way "table" and "index" are -- they just weren't yet reflected in object_reference's own classification of which object types are covered by a test. Add them to the untested() set so object_reference.untested_srf() ∪ tested types ∪ unsupported() still accounts for every member of the enum; a follow-up can add dedicated create/drop test coverage for them. - Regenerate sql/object_reference--stable.sql (auto-generated from sql/object_reference.sql) and test/expected/zzz_build.out (line numbers in test/temp_load.not_sql shift by one now that a source comment line changed) to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…back # Conflicts: # .github/workflows/ci.yml
The linter PR's comment-style edits (block comments -> "EXCLUDED CODE") land in sql/object_reference.sql, which test/build's raw-load sanity check (zzz_build) re-parses independently of pg_regress's own SQL loading and reports line numbers for. Regenerated via `make results` after confirming zero raw "not ok" TAP assertions anywhere in test/results/*.out (the earlier copy of this file, committed before the linter-PR merge, was captured while a leftover extension install from prior manual testing was still on disk in the sandbox, which produced extra NOTICEs that don't appear in a real clean run -- this version comes from a fully clean rebuild). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Verified directly: pg_get_object_address('partitioned table', ...) throws
"unrecognized object type" -- PostgreSQL's real object-address API only
knows the base "table"/"index" object types cat_tools's "partitioned table"/
"partitioned index" are derived from. object_reference actively calls
pg_get_object_address() on every object_type it tracks (not just in the
disabled sanity CHECK), so these two types would break identity tracking
outright rather than merely lacking test coverage -- the earlier "untested"
classification undersold the actual constraint.
Matches the classification (and the exact reasoning/wording) already landed
independently on the separate, longer-running new_features branch (PR #2),
which reaches unsupported() via the same cat_tools 0.3.0 enum growth. Update
test/sql/all.sql's own sanity-check of the unsupported set to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Re-opens the pgxn-tools testing work (originally PR #1, which was merged as a merge commit rather than a squash; master has since been rolled back to the pre-merge state
d191ef1).This branch holds the original PR #1 content (the
728f815merge state), rebased onto currentupstream/master, plus the fixes below needed to get CI actually green. Intended to be squash-merged once reviewed.What was blocking this (root cause)
CI on this branch surfaced
column "oid" specified more than onceatCREATE EXTENSION cat_tools-- a real bug in cat_tools 0.2.1 (2017), which is whatpgxn install --unstable cat_toolsresolves to (still the newest release actually published to the PGXN package index). cat_tools has since fixed this in a real, tagged release,0.3.0, but that release hasn't been uploaded to PGXN yet -- only tagged in git.Fix:
Makefile'scat_toolstarget now clonesPostgres-Extensions/cat_toolsat the0.3.0git tag and builds/installs it directly, instead ofpgxn install --unstable, with a comment explaining why. Verified standalone (CREATE EXTENSION cat_tools;on a fresh database) that this actually fixes the original bug.Fallout from cat_tools actually installing for the first time
Because cat_tools never successfully installed via this CI before, this is the first time object_reference's full test suite has ever actually run against a real cat_tools. That surfaced two real, small issues needing fixes here (not just CI plumbing):
cat_tools.function__arg_types_text()is deprecated in 0.3.0 in favor ofcat_tools.routine__parse_arg_types_text()(identical signature/body, just renamed) and now emits aWARNINGon every call. Switchedsql/object_reference.sql's one call site to the non-deprecated name.object_typeenum grew two new members,partitioned tableandpartitioned index. Verified directly thatpg_get_object_address('partitioned table', ...)throwsunrecognized object type-- PostgreSQL's real object-address API only knows the basetable/indextypes these are derived from, and object_reference actively callspg_get_object_address()when tracking objects. So these two are classified as unsupported (object_reference.unsupported()), matching the same classification (and reasoning) already landed independently on the separate, longer-runningnew_featuresbranch (PR Add object functions, modernize CI, and remove reg* pseudotypes #2), which hits the same enum growth.test/sql/all.sql's sanity-check of the unsupported set is updated to match.sql/object_reference--stable.sql(auto-generated fromsql/object_reference.sql) andtest/expected/zzz_build.out(line-number shifts in the raw-load sanity check) viamake results, after confirming zero rawnot okTAP assertions.Known follow-up, not done here: PR #2 will need to drop its own copy of this same cat_tools-0.3.0-enum-growth handling when it rebases past this, to avoid duplicating/conflicting with what's here.
CI restructuring
.github/workflows/ci.yml(introduced by this PR --masterhas no CI workflow at all pre-merge) is restructured:paths-ignore. A cheapchangesjob computes whether the actual push/PR diff touches only.md/.ascfiles (fail-safe defaults todocs_only=falseas the literal first line, before anything else runs), and the heavytestmatrix gates onneeds.changes.outputs.docs_only != 'true'. A workflow-levelpaths-ignorewould skip the whole workflow (including the aggregation job) on a docs-only push, leaving a requiredall-checks-passedcheck stuck Pending in branch protection.changesjob derives the matrix from two constants (NEWEST=18,CURRENT_FLOOR=12) and emits it as a JSON job output thetestjob's matrix consumes viafromJSON.CURRENT_FLOOR=12matches cat_tools 0.3.0's own declared build floor (itsMETA.json) -- object_reference requires cat_tools at both build and runtime, so it can't usefully claim support for anything older. (No legacy/climb list yet -- this repo has no update/upgrade CI jobs, so only the one pair of constants is needed for now.)pushscoped tomasteronly (pull_requeststays unrestricted) so CI doesn't double-run every commit on this PR's own branch -- one event, not two, per SHA.all-checks-passedgate kept,needs:list kept in sync with the actual job set.Supersedes PR #16's
ci.ymladditionA parallel PR, #16 ("Add SQL style linter"), also added
.github/workflows/ci.ymlfrom scratch (master had none), containing just alintjob. Since this PR introduces the fullerci.yml(test matrix, docs-only gate, PG-major derivation), the two would conflict on merge. This branch now includes and supersedes PR #16'sci.ymladdition: PR #16's branch (add-linter) is merged into this one, and itslintjob is folded into the restructuredci.ymlas the very first, top-priority job (noneeds:, runs immediately) -- per the project's own guidance that a near-instant style check shouldn't wait in a queue behind, or race for a runner slot against, the much heavier PG matrix. Thetestjob now additionally gates onneeds: [changes, lint](not justchanges), andall-checks-passed'sneeds:list includeslint.Two options once this is reviewed:
Makefile/.gitmodules/.vendor/linter/lint.mk/SQL-comment-style changes (drop its now-redundantci.yml).ci.ymlconflict would need the same resolution done here).Either order works; this branch is correct either way it lands.
Verification
CREATE EXTENSION cat_tools;standalone, confirmed it lands at0.3.0and no longer hits theoidbug.make lint-- 0 findings.make test-- all 7 test files pass cleanly (fresh install, real cat_tools 0.3.0, nopgxn install --unstable).all-checks-passedall green.