diff --git a/.github/scripts/pg_upgrade_cluster b/.github/scripts/pg_upgrade_cluster new file mode 100755 index 0000000..b413643 --- /dev/null +++ b/.github/scripts/pg_upgrade_cluster @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# +# pg_upgrade_cluster - Shared binary pg_upgrade mechanics for CI. Used by BOTH +# pg-upgrade-test (filesystem-installed cat_tools) and pg-tle-upgrade-test +# (pg_tle-registered cat_tools) in .github/workflows/ci.yml -- this part of +# the flow has nothing extension-specific about it, so it lives here once +# instead of being duplicated near-verbatim in both jobs. +# +# Lives under .github/, not bin/: unlike bin/test_existing (which a developer +# can run locally against a scratch database), this is CI-mechanics-only -- +# pg_ctlcluster/pg_createcluster/pg_upgrade against the pgxn-tools image's +# "test" cluster convention isn't a locally-runnable workflow. +# +# USAGE: .github/scripts/pg_upgrade_cluster [args] +# +# recreate-old PG_VERSION +# pg-start's default "test" cluster doesn't have data checksums +# enabled, but binary pg_upgrade requires the old and new clusters to +# have MATCHING checksum/auth settings (see $INITDB_OPTS below) -- +# stop, drop, and recreate the "test" cluster for PG_VERSION with them, +# then start it and wait for readiness. +# +# upgrade OLD_PG NEW_PG [EXTRA_NEW_CONF] +# Stop the old cluster, create the new cluster's "test" cluster (same +# $INITDB_OPTS), optionally append EXTRA_NEW_CONF to its +# postgresql.conf (e.g. to enable pg_tle before pg_upgrade runs), binary +# pg_upgrade OLD_PG -> NEW_PG, then start the new cluster and wait for +# readiness. On pg_upgrade failure, dumps its logs (PG17+ writes them to +# $new_datadir/pg_upgrade_output.d/; older versions write to CWD -- both +# are searched) before failing. +# +# $INITDB_OPTS (env var, required): initdb options both clusters must share so +# pg_upgrade sees consistent settings on old and new (e.g. +# "--data-checksums --auth trust"). Deliberately left unquoted at each use +# site so its (space-separated) options word-split into separate arguments. +set -euo pipefail + +: "${INITDB_OPTS:?INITDB_OPTS must be set}" + +recreate_old() { + local pg=$1 + pg_ctlcluster "$pg" test stop + pg_dropcluster "$pg" test + # -p 5432: pg_createcluster assigns the next available port, which may not + # be 5432 after pg-start has claimed and released it. Force 5432 so + # subsequent psql/createdb calls connect without -p. + pg_createcluster -p 5432 "$pg" test -- $INITDB_OPTS + pg_ctlcluster "$pg" test start + pg_isready -t 30 +} + +upgrade() { + local old_pg=$1 new_pg=$2 extra_conf=${3:-} + pg_ctlcluster "$old_pg" test stop + pg_createcluster -p 5432 "$new_pg" test -- $INITDB_OPTS + # Use `if`, not `&&`: a false test under `set -e` would abort the script. + if [ -n "$extra_conf" ]; then + echo "$extra_conf" >> "/etc/postgresql/$new_pg/test/postgresql.conf" + fi + # PG17+ writes logs to $new_datadir/pg_upgrade_output.d/; older versions + # write to CWD. Search both on failure. + mkdir -p /tmp/pg_upgrade_logs + chown postgres:postgres /tmp/pg_upgrade_logs + su -c "cd /tmp/pg_upgrade_logs && /usr/lib/postgresql/$new_pg/bin/pg_upgrade \ + -b /usr/lib/postgresql/$old_pg/bin \ + -B /usr/lib/postgresql/$new_pg/bin \ + -d /var/lib/postgresql/$old_pg/test \ + -D /var/lib/postgresql/$new_pg/test \ + -o '-c config_file=/etc/postgresql/$old_pg/test/postgresql.conf' \ + -O '-c config_file=/etc/postgresql/$new_pg/test/postgresql.conf'" postgres \ + || { find /tmp/pg_upgrade_logs \ + "/var/lib/postgresql/$new_pg/test/pg_upgrade_output.d" \ + -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } + pg_ctlcluster "$new_pg" test start + pg_isready -t 30 +} + +usage() { + echo "usage: .github/scripts/pg_upgrade_cluster [args]" >&2 + echo " recreate-old PG_VERSION" >&2 + echo " upgrade OLD_PG NEW_PG [EXTRA_NEW_CONF]" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + recreate-old) recreate_old "$@" ;; + upgrade) upgrade "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/.github/workflows/CLAUDE.md b/.github/workflows/CLAUDE.md new file mode 100644 index 0000000..37783ae --- /dev/null +++ b/.github/workflows/CLAUDE.md @@ -0,0 +1,71 @@ +# CI Instructions + +## Reducing CI wall-clock time + +Don't chase this proactively -- it's not worth adding real complexity for. But if a *simple* +latency win is available while touching a job anyway, take it. Facts and constraints to reason +from: + +- **GitHub Actions steps within one job are strictly sequential.** There is no native + "run step A and step B concurrently" mechanism. The only real unit of parallelism is the + *job* (already exploited here via the `pg`/matrix-leg lists) -- reordering two independent + steps changes nothing about total wall-clock time unless one of them is actually + backgrounded (`cmd &` ... `wait`) within a single step's shell script. +- **`pg-start VERSION` is not a no-op or a "wait for something already running."** It installs + that PostgreSQL major version via `apt.postgresql.org.sh` (network + package install) and + then creates and starts the "test" cluster (`pg_createcluster --start`) -- both genuinely + slow, and both block until done, like every `run:` step. +- **Backgrounding a step next to `pg-start` (or any `apt-get install`) is NOT safe by + default**: apt/dpkg serialize on a lock file, so a second concurrent `apt-get install` would + either fail immediately or block waiting for the lock -- effectively serializing anyway, but + now with added flakiness risk. Only background work that doesn't touch the package manager + (e.g. a `git clone`, or a build step that's pure `gmake`/`gcc` against already-installed + headers) alongside an apt-based step. +- **A genuine, low-complexity candidate, if this is ever worth doing**: `pg-tle-test`'s + "Install pgtap" step (`make pgtap`, which builds pgTAP via `gmake`/`gcc`, not apt) and its + "Build and install pg_tle" step (`git clone` + `make`, also not apt) are mutually + independent -- neither depends on the other's output. Backgrounding one (`make pgtap &`) + while the other runs, then `wait`ing for it before the step that needs pgtap, would overlap + two of the job's slower operations. This is a small, contained change (one background job, + one `wait`, one exit-code check) -- not the kind of complexity worth avoiding on principle. +- **`actions/cache` is likely a better lever than backgrounding.** pg_tle is rebuilt from + source (git clone + C compile) fresh in every `pg-tle-test` matrix entry and on both the old + and new cluster in every `pg-tle-upgrade-test` leg -- the same pg_tle version, rebuilt + repeatedly across runs with nothing changed. Caching the built `pg_tle.so` + control/SQL + files (keyed on PG major version + `PGTLE_VERSION`) would skip the clone+compile entirely on + a cache hit, without touching the job's logic at all. Worth trying before hand-rolled + parallelism if CI time on the pg_tle jobs becomes a real pain point. +- **Double-run check**: this repo's `push:` trigger is scoped to `branches: [master]` (not a + bare `on: [push, pull_request]`), so pushes to a feature/PR branch only fire the + `pull_request` event, not both -- confirmed via run history, no double-run here. If this + workflow's triggers are ever changed, re-verify that scoping is preserved; an unscoped + `push:` trigger would double-run CI on every PR-branch commit. + +## Docs-only pushes and "what was actually tested" + +A docs-only push makes the heavy jobs report "skipped" for THAT commit, which is correct (the +code didn't change) but easy to misread on the PR's Checks tab as "never tested." The `changes` +job's "Find the last commit where real code changed" step handles this: it walks backward +through history to the newest commit that actually changed code, looks up that commit's CI run, +and `all-checks-passed` reports it (link + green/red) in its step summary -- or explicitly notes +when the entire PR has never touched anything but docs. Don't assume "skipped" means "untested" +without checking that summary first. + +## `pull_request_target` workflows can't be tested from the PR that changes them + +GitHub always executes the workflow FILE for a `pull_request_target` event from the BASE +branch (master), never the PR's own version -- a deliberate security control so a fork PR can't +alter its own reviewer's permissions/behavior. `claude-code-review.yml` runs this way. Practical +consequence: a PR that changes that file (e.g. adjusting its `permissions:` block) cannot prove +the change took effect by watching that PR's own `claude-review` check -- it's still running +master's old version. Verify changes to that file only after merging to master, on the next PR. + +## Where CI-only shell logic lives + +`.github/scripts/` holds shell scripts that are **only** meaningful inside a CI job (e.g. +`pg_upgrade_cluster`, which drives `pg_ctlcluster`/`pg_createcluster`/`pg_upgrade` against the +pgxn-tools image's "test" cluster convention -- not something a developer would run locally). +This is distinct from the repo-root `bin/` scripts (`bin/test_existing`, `bin/assert_fs_clean`), +which a developer CAN run locally against a scratch database and are also usable outside CI. +When adding a new shared CI shell helper, put it in `.github/scripts/` if it's CI-only, `bin/` +if it's usable locally too. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 336f866..499862e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,8 +20,20 @@ jobs: changes: name: 🔍 Detect changes & derive PG matrix runs-on: ubuntu-latest + permissions: + # Needed only by "Find the last commit where real code changed" below, + # to query the Actions API for a past run's conclusion/URL. + actions: read outputs: docs_only: ${{ steps.diff.outputs.docs_only }} + # Set when docs_only is true: whether there's a real "last tested" commit + # to report at all (false), or this entire PR/push chain has never + # touched anything but docs (true, docs_only_pr) -- see the "Find the + # last commit..." step below for how these are derived. + docs_only_pr: ${{ steps.last_tested.outputs.docs_only_pr }} + last_tested_sha: ${{ steps.last_tested.outputs.last_tested_sha }} + last_tested_conclusion: ${{ steps.last_tested.outputs.last_tested_conclusion }} + last_tested_url: ${{ steps.last_tested.outputs.last_tested_url }} # Derived PG-major lists (see the "Derive ..." step for their meaning). supported_pg: ${{ steps.pg.outputs.supported_pg }} update_pg: ${{ steps.pg.outputs.update_pg }} @@ -85,6 +97,91 @@ jobs: echo "$CHANGED" echo "docs_only=$DOCS_ONLY" >> "$GITHUB_OUTPUT" + - name: Find the last commit where real code changed + # This push is docs-only, so the heavy jobs below are about to skip -- + # meaning THIS run's checks don't cover the code currently on this + # branch. Someone reviewing before merge needs to know whether that + # code was actually fully tested somewhere, and whether it was green. + # Walk backward from HEAD (bounded by the PR's base, or a generous + # depth cap for a direct push) to the newest commit whose OWN diff + # from its parent touched something other than docs -- every commit + # strictly after that one only changed docs, so that commit's CI run + # is still the authoritative test of the code on this branch now. + if: steps.diff.outputs.docs_only == 'true' + id: last_tested + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + # actions/checkout defaults to checking out an EPHEMERAL MERGE + # COMMIT for pull_request events (refs/pull/N/merge), not the PR's + # real head commit -- `git rev-parse HEAD` would return that merge + # commit's SHA, and walking `^` from it follows the wrong (base + # branch) first parent, landing on an unrelated commit entirely + # (confirmed hitting exactly this in real CI). Use the actual PR + # head SHA directly instead, same event-type split already used in + # "Compute per-push changed files" above. + PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + if [ "${{ github.event_name }}" = "pull_request" ]; then + HEAD_SHA="$PR_HEAD_SHA" + else + HEAD_SHA="${{ github.sha }}" + fi + if [ -n "$PR_BASE_SHA" ] && ! [[ "$PR_BASE_SHA" =~ ^0+$ ]]; then + BOUNDARY=$(git merge-base "$PR_BASE_SHA" "$HEAD_SHA") + else + # Not a PR (e.g. a direct push to master): no PR base to bound + # the walk against, so cap it at a generous depth instead of + # walking arbitrarily far into history. + BOUNDARY=$(git rev-list --max-count=1 --skip=100 "$HEAD_SHA" 2>/dev/null || echo "$HEAD_SHA") + fi + + commit="$HEAD_SHA" + last_code_commit="" + while [ "$commit" != "$BOUNDARY" ]; do + parent=$(git rev-parse "$commit^" 2>/dev/null) || break + changed=$(git diff --name-only "$parent" "$commit") + all_docs=true + while IFS= read -r f; do + [ -z "$f" ] && continue + if ! [[ "$f" =~ \.(md|asc)$ ]]; then all_docs=false; break; fi + done <<< "$changed" + if [ "$all_docs" = true ]; then + commit="$parent" + else + last_code_commit="$commit" + break + fi + done + + if [ -z "$last_code_commit" ]; then + # Reached the PR's own base (or the depth cap) without ever + # finding a commit that changed code: this whole PR/push chain + # is docs-only, so there is no "last tested" run to point to. + echo "docs_only_pr=true" >> "$GITHUB_OUTPUT" + echo "This entire PR/push contains only documentation changes -- no code testing applies." + exit 0 + fi + + echo "docs_only_pr=false" >> "$GITHUB_OUTPUT" + echo "last_tested_sha=$last_code_commit" >> "$GITHUB_OUTPUT" + echo "last code-changing commit: $last_code_commit" + + # Find that commit's most recent completed run of this same "CI" + # workflow (there can be other workflows -- e.g. Claude Code Review + # -- triggered on the same SHA, hence the name filter). + RUN_JSON=$(gh api "repos/$REPO/actions/runs?head_sha=$last_code_commit&status=completed" --jq \ + '[.workflow_runs[] | select(.name == "CI")] | sort_by(.run_started_at) | last // empty' 2>/dev/null || echo "") + if [ -z "$RUN_JSON" ] || [ "$RUN_JSON" = "null" ]; then + echo "last_tested_conclusion=unknown" >> "$GITHUB_OUTPUT" + echo "last_tested_url=" >> "$GITHUB_OUTPUT" + echo "No completed CI run found for $last_code_commit" + else + echo "last_tested_conclusion=$(jq -r '.conclusion' <<<"$RUN_JSON")" >> "$GITHUB_OUTPUT" + echo "last_tested_url=$(jq -r '.html_url' <<<"$RUN_JSON")" >> "$GITHUB_OUTPUT" + fi + - name: Derive the supported-PostgreSQL-major lists id: pg run: | @@ -285,20 +382,12 @@ jobs: steps: - name: Start PostgreSQL ${{ matrix.old_pg }} run: pg-start ${{ matrix.old_pg }} - - name: Recreate old cluster with data checksums enabled - run: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_dropcluster ${{ matrix.old_pg }} test - # -p 5432: pg_createcluster assigns the next available port, which - # may not be 5432 after pg-start has claimed and released it. Force - # 5432 so subsequent psql/createdb calls connect without -p. - pg_createcluster -p 5432 ${{ matrix.old_pg }} test -- $INITDB_OPTS - pg_ctlcluster ${{ matrix.old_pg }} test start - pg_isready -t 30 - name: Check out the repo uses: actions/checkout@v6 - name: Install rsync run: apt-get install -y rsync + - name: Recreate old cluster with data checksums enabled + run: .github/scripts/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} - name: Install cat_tools into old cluster run: make install - name: Prepare the old cluster (install + bridge + dependency guard) @@ -320,24 +409,7 @@ jobs: # the new version's. run: make install PG_CONFIG=/usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_config - name: Stop old cluster, binary pg_upgrade to PostgreSQL ${{ matrix.new_pg }}, start new cluster - run: | - pg_ctlcluster ${{ matrix.old_pg }} test stop - pg_createcluster -p 5432 ${{ matrix.new_pg }} test -- $INITDB_OPTS - # PG17+ writes logs to $new_datadir/pg_upgrade_output.d/; older - # versions write to CWD. Search both on failure. - mkdir -p /tmp/pg_upgrade_logs - chown postgres:postgres /tmp/pg_upgrade_logs - su -c "cd /tmp/pg_upgrade_logs && /usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_upgrade \ - -b /usr/lib/postgresql/${{ matrix.old_pg }}/bin \ - -B /usr/lib/postgresql/${{ matrix.new_pg }}/bin \ - -d /var/lib/postgresql/${{ matrix.old_pg }}/test \ - -D /var/lib/postgresql/${{ matrix.new_pg }}/test \ - -o '-c config_file=/etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf' \ - -O '-c config_file=/etc/postgresql/${{ matrix.new_pg }}/test/postgresql.conf'" postgres \ - || { find /tmp/pg_upgrade_logs \ - /var/lib/postgresql/${{ matrix.new_pg }}/test/pg_upgrade_output.d \ - -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } - pg_ctlcluster ${{ matrix.new_pg }} test start + run: .github/scripts/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} - name: Update the pg_upgraded extension to the current version # Exercises ALTER EXTENSION UPDATE on genuinely pg_upgraded objects (the # extension binary pg_upgrade just migrated), running the version-to-version @@ -548,6 +620,251 @@ jobs: if: matrix.pg != '10' run: bin/test_existing update-scenario cat_tools_update 0.2.2 + pg-tle-test: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + # Current-supported majors, from the single source in the changes job + # (see its "Derive ..." step) -- same list the `test` job uses. This + # currently happens to line up with pg_tle 1.5.2's own supported range + # (12-18, see pgxntool/pgtle_versions.md) too, but that's a coincidence: + # if either range moves independently in the future, re-check they + # still overlap before assuming this job covers what it claims to. + # 1.5.2 itself is what AWS RDS for PostgreSQL 17 currently ships + # (RDS PG17.7+, our primary deployment target -- see + # https://docs.aws.amazon.com/AmazonRDS/latest/PostgreSQLReleaseNotes/postgresql-versions.html). + pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} + name: 🧩 pg_tle ${{ matrix.pg }} + runs-on: ubuntu-latest + container: pgxn/pgxn-tools + env: + PGTLE_VERSION: "1.5.2" + steps: + # A dedicated 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. + - name: Start PostgreSQL ${{ matrix.pg }} + run: pg-start ${{ matrix.pg }} + - name: Check out the repo + uses: actions/checkout@v6 + - name: Install rsync + run: apt-get install -y rsync + - name: Install pgtap (test harness dependency) + # pgTAP is a filesystem-installed dependency of the TEST HARNESS, not + # part of what this job proves is pg_tle-only -- it's not being + # deployed via pg_tle here, and never will be. `bin/test_existing`'s + # pg_tle-mode run_suite calls `make testdeps`, which would otherwise + # filesystem-install pgTAP for the first time partway through the job + # and trip the contamination checks below (confirmed happening in a + # real CI run). Installing it explicitly here, before the baseline + # snapshot, makes it part of the accepted starting state -- like any + # other extension already on disk -- instead of needing a hardcoded + # exclude-by-name rule that would erode the whole point of diffing + # against a baseline. + run: make pgtap + - name: Snapshot filesystem extension control files (pre-pg_tle baseline) + # Whatever ships on disk by default (e.g. contrib, and now pgtap) + # before we install pg_tle. bin/assert_fs_clean's later checks diff + # against this, so they flag ANY extension that lands on disk instead + # of being registered via pg_tle -- not just cat_tools -- without + # hardcoding contrib/pgtap names. + run: bin/assert_fs_clean snapshot ${{ matrix.pg }} /tmp/control_baseline.txt + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} + # flex/bison/libkrb5-dev aren't in the pgxn-tools image; pg_tle's build + # needs them (guc-file.l, and clientauth.c includes gssapi.h). + run: | + apt-get install -y flex bison libkrb5-dev + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle + make -C /tmp/pg_tle install + - name: Enable pg_tle and restart PostgreSQL ${{ matrix.pg }} + run: | + echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.pg }}/test/postgresql.conf + pg_ctlcluster ${{ matrix.pg }} test restart + pg_isready -t 30 + - name: Register pg_tle + cat_tools against template1 + # template1, not the ambient default db: pg_tle's registration catalog + # is per-database, and `createdb` only inherits it because it copies + # template1 by default. Every cat_tools database used below (the + # smoke-test db, and whatever bin/test_existing createdb's) is created + # AFTER this step specifically so it inherits both registrations. + run: | + psql -d template1 -c "CREATE EXTENSION pg_tle" + PGDATABASE=template1 make run-pgtle + - name: Verify no stray extension control files landed on the filesystem + # CRITICAL, and intentionally redundant with the cat_tools-specific + # check in the next step: a filesystem control file silently wins + # over a pg_tle-registered extension of the same name, which would + # make this whole job a false pass without ever raising an error. + # Confirmed hitting exactly this while researching pg_tle version + # compatibility (a stale filesystem cat_tools install shadowed the + # pg_tle-registered one under test). Run again after every step below + # that could plausibly write extension files to disk -- never trust a + # single check to catch everything. + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + - name: Install cat_tools purely via pg_tle (fresh install, no filesystem trace) + # cat_tools is never `make install`ed in this job, so a successful + # CREATE EXTENSION here can only be resolving through pg_tle's + # registration, not a control file on disk. Checked explicitly here + # too (not just via the comprehensive check above) as a guard + # specifically for the extension under test, in case that check's + # exclude-list logic has a bug. + run: | + test ! -e /usr/share/postgresql/${{ matrix.pg }}/extension/cat_tools.control + createdb cat_tools_smoke + psql -d cat_tools_smoke -c "CREATE EXTENSION cat_tools" + - name: Verify cat_tools works when deployed via pg_tle + run: | + INSTALLED=$(psql -d cat_tools_smoke -tAc "SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'") + EXPECTED=$(make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p') + echo "installed=$INSTALLED expected=$EXPECTED" + if [ -z "$INSTALLED" ] || [ -z "$EXPECTED" ] || [ "$INSTALLED" != "$EXPECTED" ]; then + echo "FAIL: installed='$INSTALLED' expected='$EXPECTED'"; exit 1 + fi + psql -d cat_tools_smoke -v ON_ERROR_STOP=1 -c "SELECT relname FROM cat_tools.pg_class_v LIMIT 1" > /dev/null + - name: Verify no stray extension control files after the fresh-install smoke test + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + - name: Install PostgreSQL ${{ matrix.pg }} server dev headers + # Needed by `make check-relkind-source`, which bin/test_existing's + # run_suite calls -- every other job that reaches that check already + # installs this; pg-tle-test didn't need it until now because the + # smoke test above never runs the full suite. + run: apt-get install -y postgresql-server-dev-${{ matrix.pg }} + - name: Test the update path via pg_tle (full pgTAP suite, --use-existing) + # Exercises the SAME update path extension-update-test proves for a + # filesystem install (0.2.2 -> current), but entirely through pg_tle. + # bin/test_existing's update_scenario is UNMODIFIED from the + # filesystem case -- only run_suite's internals differ, gated by this + # one env var (see the TEST_EXISTING_DEPLOY comment in + # bin/test_existing) -- so this reuses the exact same update/upgrade + # code the other jobs use instead of duplicating it for pg_tle. + run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing update-scenario cat_tools_update 0.2.2 + - name: Verify no stray extension control files after the pg_tle update test + # Runs AFTER the complete update+full-suite flow, not before: the + # whole point of pg_tle-mode testing is proving NOTHING touched the + # filesystem THROUGHOUT that flow, not merely that the environment + # started clean. A check placed before this point could not catch a + # regression introduced by anything update-scenario itself does. + run: bin/assert_fs_clean verify ${{ matrix.pg }} /tmp/control_baseline.txt + + pg-tle-upgrade-test: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + include: + # Mirrors pg-upgrade-test's old_pg=12 legs (the only ones cleanly + # within pg_tle 1.5.2's own supported range, PG12-18) so the SAME PG + # version jumps are proven for both deployment methods. Unlike + # pg-upgrade-test, there is no bridge/old_install complexity here -- + # pg_tle has no equivalent of the pre-0.2.2 filesystem install-script + # issues (those are about SELECT * over catalog columns in old SQL + # files, irrelevant to how the extension gets registered). + - old_pg: "12" + new_pg: "13" + - old_pg: "12" + new_pg: "18" + name: 🧩🔄 pg_tle binary pg_upgrade ${{ matrix.old_pg }} → ${{ matrix.new_pg }} + runs-on: ubuntu-latest + container: pgxn/pgxn-tools + env: + # Both clusters must use the same initdb options so pg_upgrade sees + # consistent settings (checksums, auth) on old and new clusters. + INITDB_OPTS: --data-checksums --auth trust + PGTLE_VERSION: "1.5.2" + steps: + # A dedicated cluster pair, never shared with the other jobs in this + # workflow -- same reasoning as pg-tle-test. + - name: Start PostgreSQL ${{ matrix.old_pg }} + run: pg-start ${{ matrix.old_pg }} + - name: Check out the repo + uses: actions/checkout@v6 + - name: Install rsync + run: apt-get install -y rsync + - name: Recreate old cluster with data checksums enabled + run: .github/scripts/pg_upgrade_cluster recreate-old ${{ matrix.old_pg }} + - name: Snapshot filesystem extension control files (old cluster baseline) + run: bin/assert_fs_clean snapshot ${{ matrix.old_pg }} /tmp/control_baseline_old.txt + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the old cluster + # flex/bison/libkrb5-dev aren't in the pgxn-tools image; pg_tle's build + # needs them (guc-file.l, and clientauth.c includes gssapi.h). + run: | + apt-get install -y flex bison libkrb5-dev + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle_old + make -C /tmp/pg_tle_old install + - name: Enable pg_tle and restart PostgreSQL ${{ matrix.old_pg }} + run: | + echo "shared_preload_libraries = 'pg_tle'" >> /etc/postgresql/${{ matrix.old_pg }}/test/postgresql.conf + pg_ctlcluster ${{ matrix.old_pg }} test restart + pg_isready -t 30 + - name: Register pg_tle + cat_tools against template1 (old cluster) + run: | + psql -d template1 -c "CREATE EXTENSION pg_tle" + PGDATABASE=template1 make run-pgtle + - name: Verify no stray extension control files on the old cluster + run: bin/assert_fs_clean verify ${{ matrix.old_pg }} /tmp/control_baseline_old.txt + - name: Prepare the old cluster (install via pg_tle + dependency guard) + # A fresh 0.2.2 install (no bridge needed -- see pg-upgrade-test's own + # comment on why its old_pg>=11 legs don't need one), created via + # pg_tle only, then the same dependency guard bin/test_existing's + # other subcommands rely on to prove existing-mode never silently + # drops+reinstalls the extension. + run: | + test ! -e /usr/share/postgresql/${{ matrix.old_pg }}/extension/cat_tools.control + createdb cat_tools_tle_upgrade + psql -d cat_tools_tle_upgrade -c "CREATE EXTENSION cat_tools VERSION '0.2.2'" + bin/test_existing plant-guard cat_tools_tle_upgrade + - name: Install PostgreSQL ${{ matrix.new_pg }} + run: apt-get install -y postgresql-${{ matrix.new_pg }} postgresql-server-dev-${{ matrix.new_pg }} + - name: Install pgtap on the new cluster (test harness dependency) + # Must happen BEFORE the new-cluster baseline snapshot below, same + # reasoning as pg-tle-test's "Install pgtap" step: pgTAP is a + # filesystem-installed dependency of the test harness, unrelated to + # cat_tools's own pg_tle-only deployment story, so it needs to be part + # of the accepted starting state rather than flagged as contamination. + # PATH is set explicitly (not just relying on apt's update-alternatives + # switch from installing postgresql-${{ matrix.new_pg }} above) since + # `pgxn install` resolves pg_config via PATH, not via a passed-through + # PG_CONFIG make variable. + run: PATH=/usr/lib/postgresql/${{ matrix.new_pg }}/bin:$PATH make pgtap + - name: Snapshot filesystem extension control files (new cluster baseline) + run: bin/assert_fs_clean snapshot ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Build and install pg_tle ${{ env.PGTLE_VERSION }} on the new cluster + # Only pg_tle's OWN framework is needed on the new binary tree -- NOT + # cat_tools. Binary pg_upgrade's schema-only restore recreates each + # object individually (binary_upgrade_create_empty_extension), and + # pg_tle's own catalog tables (holding cat_tools's registered SQL) are + # carried over as ordinary heap data during the physical copy phase -- + # no re-registration needed on the new side. A SEPARATE clone + # (pg_tle_new, not pg_tle_old rebuilt) avoids installing a .so still + # compiled against the old cluster's PostgreSQL headers: pg_tle has C + # code, unlike cat_tools, so switching PG_CONFIG on an already-built + # tree without a fresh build would risk installing the wrong binary. + run: | + git clone --branch v${{ env.PGTLE_VERSION }} --depth 1 https://github.com/aws/pg_tle.git /tmp/pg_tle_new + make -C /tmp/pg_tle_new install PG_CONFIG=/usr/lib/postgresql/${{ matrix.new_pg }}/bin/pg_config + - name: Verify no stray extension control files on the new cluster (pre-upgrade) + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Stop old cluster, create new cluster, enable pg_tle, binary pg_upgrade, start new cluster + run: .github/scripts/pg_upgrade_cluster upgrade ${{ matrix.old_pg }} ${{ matrix.new_pg }} "shared_preload_libraries = 'pg_tle'" + - name: Verify no stray extension control files after binary pg_upgrade + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + - name: Update the pg_upgraded extension to the current version + # Exercises ALTER EXTENSION UPDATE on genuinely pg_upgraded objects + # registered via pg_tle. update_ext() is pure psql (no `make` calls), + # so it's identical regardless of deployment method -- no pg_tle-mode + # flag needed here, only for run-suite below. + run: bin/test_existing update cat_tools_tle_upgrade + - name: Run the suite against the pg_upgraded database via pg_tle (existing mode) + run: TEST_EXISTING_DEPLOY=pgtle bin/test_existing run-suite cat_tools_tle_upgrade + - name: Verify no stray extension control files after the pg_tle upgrade+update+suite run + # CRITICAL: must run AFTER the complete flow, not before -- same + # principle as pg-tle-test's final checkpoint. Proves nothing touched + # the filesystem across binary pg_upgrade, the extension update, AND + # the full pgTAP suite run, not merely that the environment started + # clean. + run: bin/assert_fs_clean verify ${{ matrix.new_pg }} /tmp/control_baseline_new.txt + # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like "🐘 PostgreSQL 14" # which would all need to be listed individually and updated whenever the @@ -555,7 +872,7 @@ jobs: # the heavy jobs gated off by the `changes` job on a docs-only push), and # fails if any failed or were cancelled. all-checks-passed: - needs: [changes, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test] + needs: [changes, test, pg-upgrade-test, pg-upgrade-stepwise, extension-update-test, pg-tle-test, pg-tle-upgrade-test] if: always() runs-on: ubuntu-latest steps: @@ -585,4 +902,26 @@ jobs: echo "One or more jobs failed or were cancelled" exit 1 fi + - name: Summarize what this push actually tested + # This job's own checks report "skipped" for every heavy job on a + # docs-only push -- true for THIS push, but easy to mistake for "never + # tested" when just glancing at the PR's Checks tab. Make it explicit + # what's actually being vouched for. Reporting only -- does not affect + # pass/fail; see the "Check all jobs passed" step above for that gate. + run: | + if [ "${{ needs.changes.outputs.docs_only }}" != "true" ]; then + echo "✅ This push's own CI run tested the code (not a docs-only push)." >> "$GITHUB_STEP_SUMMARY" + elif [ "${{ needs.changes.outputs.docs_only_pr }}" = "true" ]; then + echo "📄 This entire PR/push contains only documentation changes -- no code testing applies." >> "$GITHUB_STEP_SUMMARY" + else + SHA="${{ needs.changes.outputs.last_tested_sha }}" + CONCLUSION="${{ needs.changes.outputs.last_tested_conclusion }}" + URL="${{ needs.changes.outputs.last_tested_url }}" + if [ "$CONCLUSION" = "success" ]; then ICON="✅"; else ICON="⚠️"; fi + if [ -n "$URL" ]; then + echo "$ICON This push only changed docs. The code was last fully tested at \`${SHA:0:7}\` -- [$CONCLUSION]($URL)" >> "$GITHUB_STEP_SUMMARY" + else + echo "⚠️ This push only changed docs, and no completed CI run was found for the last code-changing commit (\`${SHA:0:7}\`) -- verify manually." >> "$GITHUB_STEP_SUMMARY" + fi + fi # vi: expandtab ts=2 sw=2 diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 610f10d..0454576 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -40,6 +40,13 @@ jobs: contents: read pull-requests: write # post the review comments checks: read # read sibling check-runs for the cost gate + # GitHub has no narrower "cache write" scope -- actions: write is the + # only permission that lets a step save an Actions cache entry (it also + # grants cancelling/deleting workflow runs and managing artifacts, which + # this job doesn't use). Without it, claude-code-review's own setup step + # can never write a cache, only ever miss. Granted here on top of the + # existing trusted-fork-owner gate below, not instead of it. + actions: write steps: # COST GATE: the paid Claude review is the last thing to run. Wait for the # PR head's OTHER check-runs to finish and only proceed if they are clean. diff --git a/bin/assert_fs_clean b/bin/assert_fs_clean new file mode 100755 index 0000000..80fcc91 --- /dev/null +++ b/bin/assert_fs_clean @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# +# assert_fs_clean - Verify no stray PostgreSQL extension control files exist on +# disk, to prove an extension was deployed purely via pg_tle (not filesystem +# install). Extracted from the pg-tle-test CI job so the SAME check can run at +# every checkpoint that could plausibly write extension files to disk +# (registration, after an update, after a binary pg_upgrade, ...), instead of +# being duplicated inline in ci.yml or trusted to a single check at the end. +# +# A pre-existing filesystem control file silently wins over a pg_tle-registered +# extension of the same name -- PostgreSQL never reports an error, it just +# quietly resolves CREATE EXTENSION from disk instead of pg_tle's catalog. That +# makes "prove pg_tle-only" a real, load-bearing assertion, not a formality: it +# must run AFTER whatever it's guarding, not just before, since the whole point +# is confirming nothing wrote to disk THROUGHOUT the guarded flow, not merely +# that the environment started clean. +# +# USAGE: bin/assert_fs_clean [args] +# +# snapshot PG_MAJOR BASELINE_FILE +# Record the current *.control files in PG_MAJOR's extension directory to +# BASELINE_FILE. Run this BEFORE installing pg_tle (or anything else), so +# whatever ships on disk by default (e.g. contrib) is excluded +# automatically -- no hardcoded exclude list to keep in sync. +# +# verify PG_MAJOR BASELINE_FILE +# Fail if any *.control file exists now that wasn't in BASELINE_FILE, +# other than pg_tle.control itself (the one legitimate filesystem install +# in this flow). Run this after EVERY step that could plausibly have +# written extension files to disk. +set -euo pipefail + +extdir_of() { echo "/usr/share/postgresql/$1/extension"; } + +snapshot() { + local pg_major=$1 baseline=$2 + find "$(extdir_of "$pg_major")" -maxdepth 1 -name '*.control' | sort > "$baseline" +} + +verify() { + local pg_major=$1 baseline=$2 after new + after=$(mktemp) + find "$(extdir_of "$pg_major")" -maxdepth 1 -name '*.control' | sort > "$after" + new=$(comm -13 "$baseline" "$after" | grep -vx '.*/pg_tle\.control' || true) + rm -f "$after" + if [ -n "$new" ]; then + echo "FAIL: unexpected extension control file(s) on disk (everything but pg_tle must be registered via pg_tle, not filesystem-installed):" >&2 + echo "$new" >&2 + exit 1 + fi + echo "OK: no stray extension control files on disk (PG $pg_major)" +} + +usage() { + echo "usage: bin/assert_fs_clean [args]" >&2 + echo " snapshot PG_MAJOR BASELINE_FILE" >&2 + echo " verify PG_MAJOR BASELINE_FILE" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + snapshot) snapshot "$@" ;; + verify) verify "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/bin/test_existing b/bin/test_existing index 0d60561..32a4f4c 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -35,6 +35,27 @@ # # Run `bin/test_existing` with no subcommand to print usage. # +# TEST_EXISTING_DEPLOY=pgtle (env var, checked by run_suite): the extension +# under test was deployed via pg_tle, not the filesystem, and must NEVER be +# filesystem-installed by anything this script does. `make test` unconditionally +# depends on `install` (pgxntool/base.mk's TEST_DEPS), and `make verify-results` +# in turn depends on `test` -- so calling either would install cat_tools to disk +# as a side effect, even though the database itself was never touched via the +# filesystem. In this mode run_suite calls `testdeps`+`installcheck` directly +# (skips the `install` prerequisite that only `test` pulls in) and invokes +# verify-results-pgtap.sh directly (skips the `test` prerequisite that +# `verify-results` pulls in) -- same underlying work, no pgxntool changes +# needed. This whole workaround could be simplified away if pgxntool grows an +# install-skip override (requested upstream: +# https://github.com/Postgres-Extensions/pgxntool/issues/55) -- revisit this +# block if/when that lands. The CALLER is responsible for having already +# registered pg_tle + cat_tools against `template1` before creating any +# database this script will use (pg_tle's registration is per-database; +# `createdb` only inherits it +# because it copies `template1` by default) and for asserting filesystem +# cleanliness before/after (see bin/assert_fs_clean) -- this flag only stops +# this script from being the thing that dirties the filesystem itself. +# # Why the dependency guard: "existing" mode must run the suite against the ACTUAL # upgraded/updated objects. If anything silently dropped + reinstalled the # extension, the suite would test a FRESH install and hide a regression. As @@ -223,8 +244,21 @@ run_suite() { # the SAME existing-mode overrides or it would re-run FRESH (against a new # regression DB) instead of verifying THIS existing database. local existing_args="TEST_LOAD_SOURCE=existing CONTRIB_TESTDB=$db EXTRA_REGRESS_OPTS=--use-existing PGXNTOOL_ENABLE_TEST_BUILD=no" - make test $existing_args - make verify-results $existing_args + if [ "${TEST_EXISTING_DEPLOY:-}" = pgtle ]; then + # pg_tle mode (see the TEST_EXISTING_DEPLOY comment at the top of this + # file): `test` and `verify-results` each unconditionally pull in + # `install`, so run the same underlying work directly instead -- + # `testdeps`+`installcheck` (neither depends on `install`), then the + # pgtap-mode verify-results script directly (skips its `test` prerequisite). + make testdeps $existing_args + make installcheck $existing_args + local testout + testout=$(make -s print-TESTOUT 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p') + pgxntool/verify-results-pgtap.sh "$testout" + else + make test $existing_args + make verify-results $existing_args + fi # Post-suite guard assertion: a CASCADE drop+reinstall during the run would # have removed the guard view, meaning the suite tested a fresh install, not $db. guard_present "$db" \