Switch to pgxn-tools based testing - #5
Merged
Conversation
📝 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
Comment on lines
+60
to
64
| 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.
Suggested change
| 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.
Contributor
Author
|
@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 |
This was referenced Aug 5, 2026
jnasbyupgrade
force-pushed
the
master-pre-rollback
branch
from
August 7, 2026 21:53
4fe08c0 to
4531d10
Compare
Rebuilt fresh on top of current master, containing only the actual CI/pgxn-tools migration (the SQL style linter integration and the cat_tools 0.3.0 dependency fix that had scope-crept into this branch are split out to #16 and #28 respectively). - Add .github/workflows/ci.yml: a `changes` job (docs-only gate + PG-major-matrix derivation from two constants), a `test` matrix job (container: pgxn/pgxn-tools, PostgreSQL 12-18), and an `all-checks-passed` aggregation gate for use as a single stable required status check. - Remove .travis.yml and pg-travis-test.sh, superseded by the above. - test/dump/run.sh: add -X to several psql invocations, disabling ~/.psqlrc so test runs are deterministic.
jnasbyupgrade
added a commit
to jnasbyupgrade/object_reference
that referenced
this pull request
Aug 7, 2026
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for Postgres-Extensions#5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 9, 2026
pgxn install --unstable cat_tools resolves to the newest release actually published to the PGXN package index, which is still 0.2.1 (2017) and fails standalone on modern PostgreSQL with "column oid specified more than once" at CREATE EXTENSION. A fixed release, 0.3.0, is tagged in cat_tools' own git repo but hasn't been uploaded to PGXN yet, so the Makefile's cat_tools target now clones Postgres-Extensions/cat_tools at the 0.3.0 tag and builds/installs it directly. Since this is the first time object_reference's suite has actually run against a real, working cat_tools, two small fallout fixes are needed: - cat_tools.function__arg_types_text() is deprecated in 0.3.0 in favor of cat_tools.routine__parse_arg_types_text() (identical signature/body, just renamed, deprecated one emits a WARNING on every call). Switched object_reference's one call site to the non-deprecated name. - cat_tools 0.3.0's object_type enum grew two new members, "partitioned table" and "partitioned index". pg_get_object_address() doesn't recognize either (only the base table/index types they derive from), so object_reference classifies them as unsupported, matching object_reference.unsupported()'s existing handling of "event trigger" for the same reason. test/sql/all.sql's sanity-check of the unsupported set is updated to match. sql/object_reference--stable.sql and test/expected/zzz_build.out are regenerated (make results) to match. Extracted from PR #5, which had scope-crept into also carrying this fix alongside the actual CI/pgxn-tools migration; splitting it out here so it can be reviewed and merged independently.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 9, 2026
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for #5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 9, 2026
Reconciles the substantive feature delta from new_features (PR #2) onto the current 'stable' baseline (post PR #5/#16: pgxn-tools testing, cat_tools 0.3.0, and the linter): - _object_reference._object_oid: drop the per-catalog regclass/regconfig/ regdictionary/regnamespace/regoperator/regprocedure/regtype columns and their unique indexes plus the count_nulls-backed null_count trigger that enforced "exactly one is set". classid is now plain oid and object_oid (also NOT NULL) is the sole identifier column, so there's nothing left to arbitrate between. - _object_reference._object_v / _object_v__for_update: drop the reg* columns from the column list to match. - _object_reference._object_oid__add: replace the dynamic, format()-built INSERT that picked a reg* column based on cat_tools.object__reg_type() with a plain INSERT into object_oid. - Drop the count_nulls search_path DO block (dead now that the trigger using it is gone) and the count_nulls dependency throughout (control, Makefile, test setup). - Add object_reference.object__describe()/object__identity(), thin wrappers around pg_describe_object()/pg_identify_object(); and object__cleanup(), which best-effort deletes an object record (ignoring foreign_key_violation if it's still referenced elsewhere). Wire object__cleanup() up to a new AFTER DELETE trigger on object_group__object so removing an object from its last group automatically attempts cleanup. - _object_v__for_update (the getsert core): refuse to track objects living in a pg_temp*/pg_toast_temp* schema, since a tracked reference would outlive the temporary object it points to. - test/sql/object_group.sql: switch the two scratch tables from TEMP to regular tables (object__getsert now rejects temp objects) and add coverage for the new automatic-cleanup trigger. - test/sql/base.sql: replace the count_nulls-relocation test (relocation was already unsupported and the whole extension no longer depends on count_nulls) with coverage for object_oid, object__describe(), object__identity(), and temp-object rejection. sql/object_reference--0.1.0.sql (the frozen historical release) and the META files are untouched. default_version stays 'stable'; sql/object_reference--stable.sql is regenerated to match sql/object_reference.sql. make lint and make test (including the dump/restore test) pass on both PostgreSQL 12 and 17. Supersedes PR #2 (new_features) and, for the update/upgrade test infrastructure built on top of it, sets up the rebuild of PR #3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jnasbyupgrade
added a commit
to jnasbyupgrade/object_reference
that referenced
this pull request
Aug 10, 2026
pgxn install --unstable cat_tools resolves to the newest release actually published to the PGXN package index, which is still 0.2.1 (2017) and fails standalone on modern PostgreSQL with "column oid specified more than once" at CREATE EXTENSION. A fixed release, 0.3.0, is tagged in cat_tools' own git repo but hasn't been uploaded to PGXN yet, so the Makefile's cat_tools target now clones Postgres-Extensions/cat_tools at the 0.3.0 tag and builds/installs it directly. Since this is the first time object_reference's suite has actually run against a real, working cat_tools, two small fallout fixes are needed: - cat_tools.function__arg_types_text() is deprecated in 0.3.0 in favor of cat_tools.routine__parse_arg_types_text() (identical signature/body, just renamed, deprecated one emits a WARNING on every call). Switched object_reference's one call site to the non-deprecated name. - cat_tools 0.3.0's object_type enum grew two new members, "partitioned table" and "partitioned index". pg_get_object_address() doesn't recognize either (only the base table/index types they derive from), so object_reference classifies them as unsupported, matching object_reference.unsupported()'s existing handling of "event trigger" for the same reason. test/sql/all.sql's sanity-check of the unsupported set is updated to match. sql/object_reference--stable.sql and test/expected/zzz_build.out are regenerated (make results) to match. Extracted from PR Postgres-Extensions#5, which had scope-crept into also carrying this fix alongside the actual CI/pgxn-tools migration; splitting it out here so it can be reviewed and merged independently.
jnasbyupgrade
added a commit
to jnasbyupgrade/object_reference
that referenced
this pull request
Aug 10, 2026
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for Postgres-Extensions#5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 10, 2026
pgxn install --unstable cat_tools resolves to the newest release actually published to the PGXN package index, which is still 0.2.1 (2017) and fails standalone on modern PostgreSQL with "column oid specified more than once" at CREATE EXTENSION. A fixed release, 0.3.0, is tagged in cat_tools' own git repo but hasn't been uploaded to PGXN yet, so the Makefile's cat_tools target now clones Postgres-Extensions/cat_tools at the 0.3.0 tag and builds/installs it directly. Since this is the first time object_reference's suite has actually run against a real, working cat_tools, two small fallout fixes are needed: - cat_tools.function__arg_types_text() is deprecated in 0.3.0 in favor of cat_tools.routine__parse_arg_types_text() (identical signature/body, just renamed, deprecated one emits a WARNING on every call). Switched object_reference's one call site to the non-deprecated name. - cat_tools 0.3.0's object_type enum grew two new members, "partitioned table" and "partitioned index". pg_get_object_address() doesn't recognize either (only the base table/index types they derive from), so object_reference classifies them as unsupported, matching object_reference.unsupported()'s existing handling of "event trigger" for the same reason. test/sql/all.sql's sanity-check of the unsupported set is updated to match. sql/object_reference--stable.sql and test/expected/zzz_build.out are regenerated (make results) to match. Extracted from PR #5, which had scope-crept into also carrying this fix alongside the actual CI/pgxn-tools migration; splitting it out here so it can be reviewed and merged independently.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 10, 2026
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for #5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
jnasbyupgrade
added a commit
that referenced
this pull request
Aug 10, 2026
Reconciles the substantive feature delta from new_features (PR #2) onto the current 'stable' baseline (post PR #5/#16: pgxn-tools testing, cat_tools 0.3.0, and the linter): - _object_reference._object_oid: drop the per-catalog regclass/regconfig/ regdictionary/regnamespace/regoperator/regprocedure/regtype columns and their unique indexes plus the count_nulls-backed null_count trigger that enforced "exactly one is set". classid is now plain oid and object_oid (also NOT NULL) is the sole identifier column, so there's nothing left to arbitrate between. - _object_reference._object_v / _object_v__for_update: drop the reg* columns from the column list to match. - _object_reference._object_oid__add: replace the dynamic, format()-built INSERT that picked a reg* column based on cat_tools.object__reg_type() with a plain INSERT into object_oid. - Drop the count_nulls search_path DO block (dead now that the trigger using it is gone) and the count_nulls dependency throughout (control, Makefile, test setup). - Add object_reference.object__describe()/object__identity(), thin wrappers around pg_describe_object()/pg_identify_object(); and object__cleanup(), which best-effort deletes an object record (ignoring foreign_key_violation if it's still referenced elsewhere). Wire object__cleanup() up to a new AFTER DELETE trigger on object_group__object so removing an object from its last group automatically attempts cleanup. - _object_v__for_update (the getsert core): refuse to track objects living in a pg_temp*/pg_toast_temp* schema, since a tracked reference would outlive the temporary object it points to. - test/sql/object_group.sql: switch the two scratch tables from TEMP to regular tables (object__getsert now rejects temp objects) and add coverage for the new automatic-cleanup trigger. - test/sql/base.sql: replace the count_nulls-relocation test (relocation was already unsupported and the whole extension no longer depends on count_nulls) with coverage for object_oid, object__describe(), object__identity(), and temp-object rejection. sql/object_reference--0.1.0.sql (the frozen historical release) and the META files are untouched. default_version stays 'stable'; sql/object_reference--stable.sql is regenerated to match sql/object_reference.sql. make lint and make test (including the dump/restore test) pass on both PostgreSQL 12 and 17. Supersedes PR #2 (new_features) and, for the update/upgrade test infrastructure built on top of it, sets up the rebuild of PR #3. 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
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.
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).Rescoped: this branch previously scope-crept into also carrying the SQL style linter integration (a duplicate of #16's content, pulled in by a merge to resolve a
ci.ymlconflict) and a cat_tools 0.3.0 dependency fix. Both have been split out:object_typeenum members classified as unsupported) is now Adapt to cat_tools 0.3.0: renamed function, two new object types #28.This branch is rebuilt fresh on top of current
mastercontaining only the actual CI/pgxn-tools migration:.github/workflows/ci.yml(new): achangesjob (docs-only gate + PG-major-matrix derivation from two constants,NEWEST=18/CURRENT_FLOOR=12), atestmatrix job (container: pgxn/pgxn-tools, PostgreSQL 12-18), and anall-checks-passedaggregation gate..travis.ymlandpg-travis-test.shdeleted (superseded by the above).test/dump/run.sh: added-Xflags to severalpsqlinvocations, disabling~/.psqlrcso test runs are deterministic.(
.gitignore's.claude/settings.local.jsonline from the original diff was dropped — currentmasteralready ignores it via the broader existing.claude/*.local.jsonpattern.)CI status
Live and green: this branch's own CI (all 7 PostgreSQL majors, 12-18, plus
all-checks-passed) passes cleanly as-is. Earlier drafts of this split assumed it would need #28 (cat_tools 0.3.0) to merge first, since PGXN's package index used to be stuck on a broken0.2.1cat_tools release — that's no longer the case, PGXN now serves0.3.0directly, somake test'spgxn install --unstable cat_toolsalready resolves correctly without any change here.Once #16 (SQL style linter) also merges, a small follow-up will be needed here to fold its
lintjob into thisci.yml(as the first, top-priority job ahead of thetestmatrix, per the reasoning previously written up when the two were briefly merged together). Not done in this PR — keeping this branch scoped to just the CI/pgxn-tools migration.#28 (cat_tools 0.3.0 adaptation) is unrelated to this PR going green, but is still a real, worthwhile correctness fix independent of CI — no required merge order between the two now.