ci: add a dedicated pg_tle test job#47
Open
jnasbyupgrade wants to merge 11 commits into
Open
Conversation
|
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:
✨ Finishing Touches🧪 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 |
jnasbyupgrade
force-pushed
the
ci/add-pg-tle-testing
branch
2 times, most recently
from
July 23, 2026 23:02
e1e0722 to
cef277e
Compare
There was no CI coverage for cat_tools' pgxntool-generated pg_tle (Trusted Language Extensions) deployment path, so a regression there (e.g. in the generated registration SQL) could ship unnoticed. This adds a pg-tle-test job that builds pg_tle from source, registers cat_tools with it via `make run-pgtle`, and verifies the extension installs and works correctly with no filesystem control file present -- i.e. purely through pg_tle's database-backed registration. The job always runs on its own freshly-started cluster, never shared with the other jobs in this workflow: pg_tle requires shared_preload_libraries and mixing pg_tle/non-pg_tle extension installs on one cluster can misbehave. The matrix covers PG 12-18, matching pg_tle 1.5.2's supported range per pgxntool/pgtle_versions.md (PG10/11 predate pg_tle support). This intentionally stops at a smoke test rather than running the full pg_regress/pgTap suite through pg_tle: pg_regress always creates its test database from the pristine template0, so any pre-registration done in another database (e.g. postgres or template1) is invisible to it. Routing the full suite through pg_tle would require adding registration SQL into test/install/*.sql, which runs inside every job's pg_regress invocation (not just this one) and would need conditional logic to avoid affecting the other, non-pg_tle jobs -- more invasive than this task calls for.
pg_tle silently loses to a filesystem control file of the same name -- confirmed hitting this directly while researching pg_tle version compatibility, where a stale filesystem cat_tools install shadowed the pg_tle-registered one under test and would have produced a false pass with no error raised. Add two layers of defense: a comprehensive check that diffs the extension directory's *.control files against a pre-pg_tle baseline (catching any extension that lands on disk instead of being registered via pg_tle, not just cat_tools, without hardcoding contrib extension names), and an explicit cat_tools-specific check that always runs regardless, in case the comprehensive check's exclude-list logic has a bug.
jnasbyupgrade
force-pushed
the
ci/add-pg-tle-testing
branch
from
July 24, 2026 20:21
cef277e to
988ab02
Compare
extension-update-test only proves the update path works via a filesystem install; there was no coverage for updating a pg_tle-deployed cat_tools. Reusing bin/test_existing's update_scenario unmodified for this hit a real bug: its run_suite() calls `make test`, which unconditionally depends on `install` (pgxntool's TEST_DEPS), and `make verify-results` in turn depends on `test` -- so the moment a pg_tle-mode run reached the suite, cat_tools would get filesystem-installed as a side effect, defeating the entire point of proving nothing ever touches disk. Fixed without touching pgxntool: a new TEST_EXISTING_DEPLOY=pgtle mode in run_suite() calls `testdeps`+`installcheck` directly (neither depends on `install`) and invokes verify-results-pgtap.sh directly (skips the `test` prerequisite `verify-results` pulls in) -- same underlying work, no pgxntool changes, no wait on a pgxntool sync. prepare_old/update_scenario/update_check needed no changes at all: pg_tle registration is transparent to `createdb`/`CREATE EXTENSION ... VERSION`/ `ALTER EXTENSION ... UPDATE` once the target database descends from a pg_tle-registered template1 (pg_tle's own registration catalog is per-database; createdb only inherits it because it copies template1 by default) -- so update_scenario runs the exact same multi-hop update path (0.2.2->0.2.3->current) unmodified in pg_tle mode. Also extracts the pg-tle-test job's inline filesystem-contamination check into bin/assert_fs_clean, a reusable script, and calls it at multiple checkpoints instead of trusting a single check: after registering with pg_tle, after the fresh-install smoke test, and -- critically -- after the full update+suite run completes. That last one has to run AFTER the whole flow, not before: the point of pg_tle-mode testing is proving nothing touched the filesystem THROUGHOUT it, not just that the environment started clean. Binary pg_upgrade of a pg_tle-deployed extension is a natural next step (bin/test_existing's prepare_old/run_suite already generalize to it, and the new PGTLE deploy mode in run_suite covers the same use-existing suite run) but isn't included here.
…m install The real CI run failed after the previous commit: the third fs-clean checkpoint (after the pg_tle update test) flagged pgtap.control as an unexpected file. bin/test_existing's pg_tle-mode run_suite calls `make testdeps`, which filesystem-installs pgTAP for pg_regress to load -- a legitimate, load-bearing dependency of the TEST HARNESS itself, unrelated to cat_tools's own pg_tle-only deployment story (pgTAP isn't being deployed via pg_tle here, and never will be). The baseline snapshot was taken before pgTAP got installed, so its arrival read as contamination. Local testing missed this because this container already had pgtap filesystem-installed from earlier, unrelated work, so the baseline silently already included it and the install was a no-op. Verified the actual bug and the fix using PG11 (present in this container but with no prior pgtap install): confirmed `make testdeps`-triggered pgtap install DOES trip the check when taken as new, and confirmed installing pgtap explicitly before the baseline snapshot correctly folds it into the accepted starting state. Fixed by adding an explicit `make pgtap` step before the baseline snapshot, rather than hardcoding "pgtap" into assert_fs_clean's exclude list -- keeps the general baseline-diff design intact (whatever exists before instrumenting is accepted, no exclude-list names to maintain beyond pg_tle.control, the one thing this flow deliberately adds).
jnasbyupgrade
marked this pull request as ready for review
July 24, 2026 22:40
pg-tle-test only proves fresh-install and update-in-place work via pg_tle; there was no coverage for binary pg_upgrade of a pg_tle-registered cat_tools (the scenario that matters for an already-running RDS/Aurora instance being upgraded to a new PostgreSQL major version). Adds pg-tle-upgrade-test, mirroring pg-upgrade-test's old_pg=12 legs (12->13, 12->18 -- the only ones within pg_tle 1.5.2's own supported range) with pg_tle deployment instead of filesystem install on the old cluster. The new cluster's binary tree needs nothing cat_tools-specific beyond pg_tle's own framework files (build + install + shared_preload_libraries): binary pg_upgrade's schema-only restore recreates each object individually via binary_upgrade_create_empty_extension, and pg_tle's own catalog tables (holding cat_tools's registered SQL) get carried over as ordinary heap data during the physical copy phase. No re-registration needed on the new side. Reuses bin/test_existing's plant-guard/update/run-suite unmodified (the same TEST_EXISTING_DEPLOY=pgtle mode added for pg-tle-test's update-path leg covers this too) -- no new pg_tle-specific test logic, just a pg_tle-based setup feeding the same shared functions the filesystem-based upgrade jobs use. Also carries pg-tle-test's fs-contamination checkpoints (before pg_tle, after registration, after pgtap install, after pg_upgrade, and critically after the full update+suite run) on both the old and new cluster. pg_tle's C code means it can't reuse a single build across PG_CONFIGs the way cat_tools's pure-SQL install does -- a stale .so compiled against the wrong PostgreSQL headers would silently install wrong, so the old and new clusters each get their own fresh clone+build rather than reinstalling one build with a different PG_CONFIG. Verified locally: the plant-guard -> update -> run-suite(TEST_EXISTING_DEPLOY =pgtle) sequence runs cleanly end to end (all 14 pgTAP tests, zero filesystem trace) against a scratch cluster. The actual binary pg_upgrade invocation is unchanged from pg-upgrade-test's already-CI-proven form (only adding the shared_preload_libraries line), and the pg_upgrade+pg_tle mechanism itself was separately hands-on verified in earlier research (PG16->PG18, all 14 tests passing post-upgrade) -- this container had no PostgreSQL major version pairing available that was both pg_tle 1.5.2- compatible and free of pre-existing cat_tools filesystem installs from other concurrent work, so the full cross-version flow is unproven until real CI runs it.
…chanics The two jobs' cluster-recreation and binary pg_upgrade steps were near-identical (pg-tle-upgrade-test's version differs only by one extra config line to enable pg_tle before the upgrade) -- ~55 lines of copy-pasted shell duplicated across both jobs. Extracted into bin/pg_upgrade_cluster, mirroring the project's existing pattern of pushing shared CI shell logic into bin/ scripts (bin/test_existing, bin/assert_fs_clean) rather than GitHub Actions composite actions. Two subcommands: recreate-old (recreate the pg-start default cluster with data checksums enabled) and upgrade (create the new cluster, optionally append an extra config line, run pg_upgrade with its log-dumping-on-failure behavior, start the new cluster). Both jobs needed "Check out the repo" moved earlier (before the recreate-old step) since the script lives in the repo -- previously that step ran before checkout in both jobs. Verified locally end to end using scratch PG11->PG16 clusters (not the shared main clusters): recreate-old correctly enables checksums, and upgrade succeeds identically both with and without an extra config line, matching each job's respective call. Also references the pgxntool issue opened for an install-skip override (Postgres-Extensions/pgxntool#55) in bin/test_existing's TEST_EXISTING_DEPLOY comment, since that feature would let the testdeps+installcheck workaround there be simplified once it lands.
…ideas pg_upgrade_cluster is CI-mechanics-only (drives pg_ctlcluster/pg_createcluster /pg_upgrade against the pgxn-tools image's "test" cluster convention) -- unlike bin/test_existing, a developer can't meaningfully run it locally, so it belongs under .github/ rather than alongside the locally-usable bin/ scripts. Verified no Makefile/dist-packaging references depend on its bin/ location before moving. Also adds .github/workflows/CLAUDE.md capturing where things stand on CI wall-clock time: GitHub Actions steps are strictly sequential within a job (no free win from reordering independent steps), pg-start does real apt-install + cluster-create work rather than being a no-op, apt/dpkg's lock rules out backgrounding anything alongside another apt-get call, and actions/cache (for pg_tle's from-source rebuild, repeated identically across every pg-tle-test/pg-tle-upgrade-test matrix entry) is probably a better lever than hand-rolled parallelism if this ever becomes worth pursuing. Also confirms this workflow's push trigger is already scoped to branches: [master] (not a bare push/pull_request), so PR-branch commits don't double-run CI -- checked against actual run history for this PR.
This CI guidance is generic GitHub Actions advice, not specific to this project.
A docs-only push makes the heavy jobs report "skipped" for that specific commit, which is correct (the code didn't change) but easy to misread as "never tested" when just glancing at the PR's Checks tab -- the actual test results live on an earlier commit's run. The `changes` job now walks backward from HEAD (bounded by the PR's base, or a depth cap for a direct push to master) to find the newest commit whose own diff from its parent touched more than docs, then looks up that commit's most recent completed "CI" workflow run. `all-checks-passed` reports this in its step summary: a link to that run plus whether it was green, or an explicit note when the entire PR/push has never touched anything but docs (no "last tested" commit exists to report in that case). Verified the backward-walk logic and the Actions-API lookup standalone against this actual repo/branch: correctly identifies 92e9a03 (the last code-changing commit before this PR's own docs-only tail) and correctly finds its CI run as conclusion=success. Reporting only -- does not change what makes all-checks-passed pass/fail.
Also serves as a real docs-only push to exercise the new backward-walk reporting logic end-to-end in CI for the first time.
The review job's setup step (inside claude-code-review@v1) can never save an Actions cache without this -- GitHub has no narrower cache-only permission scope, so every run was hitting "Cache reservation failed: cache write denied: token has no writable scopes" (a warning, not a failure; the review still ran and posted correctly every time). Granted on top of the existing trusted-fork-owner gate, not instead of it.
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.
There was no CI coverage for cat_tools' pgxntool-generated pg_tle (Trusted Language Extensions) deployment path, so a regression there (e.g. in the generated registration SQL) could ship unnoticed. This adds a pg-tle-test job that builds pg_tle from source, registers cat_tools with it via
make run-pgtle, and verifies the extension installs and works correctly with no filesystem control file present -- i.e. purely through pg_tle's database-backed registration.The job always runs on its own freshly-started cluster, never shared with the other jobs in this workflow: pg_tle requires shared_preload_libraries and mixing pg_tle/non-pg_tle extension installs on one cluster can misbehave.
The matrix covers PG 12-18, matching pg_tle 1.5.2's supported range per pgxntool/pgtle_versions.md (PG10/11 predate pg_tle support).
This intentionally stops at a smoke test rather than running the full pg_regress/pgTap suite through pg_tle: pg_regress always creates its test database from the pristine template0, so any pre-registration done in another database (e.g. postgres or template1) is invisible to it. Routing the full suite through pg_tle would require adding registration SQL into test/install/*.sql, which runs inside every job's pg_regress invocation (not just this one) and would need conditional logic to avoid affecting the other, non-pg_tle jobs -- more invasive than this task calls for.
Verification
Locally verified end-to-end against a scratch PostgreSQL 16 cluster in an isolated container (built pg_tle 1.5.2 from source, registered cat_tools via
make run-pgtle, confirmedCREATE EXTENSION cat_toolssucceeds with zero filesystem trace, and confirmed a real query againstcat_tools.pg_class_vreturns data) before pushing this branch. CI is still pending -- especially interested to see whether all of PG 12-18 build/pass cleanly, since some versions may need to be dropped from the matrix (that's fine per the task).Test plan