diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fb8a02..186d5c6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,7 +43,6 @@ jobs: 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 }} climb_pg: ${{ steps.pg.outputs.climb_pg }} legacy_pg: ${{ steps.pg.outputs.legacy_pg }} steps: @@ -194,15 +193,15 @@ jobs: run: | # Spending 20+ lines to replace a handful of version references looks # silly on the surface, but the point is CONSISTENCY: every job -- the - # fresh-install `test` matrix, the `extension-update-test` matrix and - # the stepwise climb -- derives its PostgreSQL set from this ONE source, - # so they cannot drift onto different version lists. + # fresh-install `test`/`pg-tle-test` matrices and the stepwise climb -- + # derives its PostgreSQL set from this ONE source, so they cannot drift + # onto different version lists. # # SINGLE SOURCE OF TRUTH for the supported PostgreSQL majors. To add # or drop a PG major, edit ONLY the three constants below; the test, - # extension-update-test and pg-upgrade-stepwise jobs all derive their - # version lists from them (adding the newest major is a one-line NEWEST - # bump). Do NOT hardcode a supported major in any job matrix or loop. + # pg-tle-test and pg-upgrade-stepwise jobs all derive their version + # lists from them (adding the newest major is a one-line NEWEST bump). + # Do NOT hardcode a supported major in any job matrix or loop. # # NEWEST -- highest PostgreSQL major cat_tools is tested on. # CURRENT_FLOOR -- oldest major the CURRENT extension version supports: @@ -211,18 +210,17 @@ jobs: # LEGACY_FLOOR -- oldest major the pre-0.2.2 install scripts still # load on (PG11 added pg_attribute.attmissingval and # PG12+ exposes the oid system column in SELECT *, both - # of which those old scripts trip over). Only the - # update and stepwise paths reach back this far. + # of which those old scripts trip over). Only + # extension-update-test (PG10-only now) and the + # stepwise climb reach back this far. NEWEST=18 CURRENT_FLOOR=12 LEGACY_FLOOR=10 # supported = CURRENT_FLOOR..NEWEST (newest-first). The FRESH-install - # matrix (test job) runs exactly these. + # matrix (test job, and pg-tle-test) runs exactly these. supported=$(seq "$NEWEST" -1 "$CURRENT_FLOOR") - # update = supported plus the legacy floor: the extension-update job - # additionally exercises the PG10-only pre-0.2.2 update scripts. - update="$supported $LEGACY_FLOOR" + # climb = LEGACY_FLOOR+1 .. NEWEST (ascending). The stepwise job starts # one cluster on the legacy floor and binary-pg_upgrades through every # later major in turn, so its targets are every major above the floor. @@ -234,54 +232,125 @@ jobs: json() { printf '%s\n' "$@" | paste -sd, - | sed 's/^/[/; s/$/]/'; } echo "supported_pg=$(json $supported)" >> "$GITHUB_OUTPUT" - echo "update_pg=$(json $update)" >> "$GITHUB_OUTPUT" + + # extension-update-test is now PG10-only (its PG12+ leg folded into + # the `test` job -- see that job's step), so it consumes legacy_pg + # directly rather than a combined update_pg list (removed). echo "legacy_pg=$LEGACY_FLOOR" >> "$GITHUB_OUTPUT" + # Space-separated for direct iteration in the stepwise bash loop. echo "climb_pg=$(echo $climb)" >> "$GITHUB_OUTPUT" # =========================================================================== # Test strategy # - # A cat_tools install can be arrived at several ways, each of which can break - # differently, so each is exercised by its own job below (the per-job comments - # carry the details; this is the big picture): + # Two kinds of coverage: what runs on EVERY supported PostgreSQL major (the + # MAIN MATRIX), and SPECIAL CASES that apply to one scenario only. + # + # MAIN MATRIX -- every supported major (12-18, single source: the changes + # job). Two independently-isolated jobs, since they prove different things: + # + # test -- Everything reachable through a FILESYSTEM-installed + # cat_tools on this major: + # 1. FRESH install, default schema -- `make + # verify-results`, the baseline a brand-new user gets. + # 2. `make test-long` -- every test-* target except plain + # `test` (test-update, test-schema, + # test-update-schema; see the Makefile's test-long + # comment for the simple-inclusion rule). test-update + # and (3) below both update to the current version + # and run the suite, but through different code + # paths, not the same check twice: test-update drives + # test/install/load.sql's own committed update + # branch; (3) updates via a raw psql ALTER EXTENSION, + # outside load.sql, then runs the suite in + # TEST_LOAD_SOURCE=existing mode (load.sql's + # assert-only branch). A bug in load.sql's + # update-mode logic only test-update catches; a bug + # in (3)'s guard/structural-diff logic only (3) + # catches. + # 3. The GUARD-PROVED update-to-current check: + # bin/test_existing's update-scenario -- CREATE + # EXTENSION at the 0.2.2 backward-compat floor, ALTER + # EXTENSION UPDATE to current, structurally compare + # against a fresh install, then run the full suite in + # existing mode. USED to be its own matrix job + # (extension-update-test's PG12+ leg) -- folded in + # here since it runs on the same majors as (1)/(2), + # and this job's container, checkout, and cat_tools + # install are already there; a separate job would + # pay for all of that again for no added coverage. + # See that step's own comment for what was verified + # locally before folding it in. + # + # pg-tle-test -- The SAME majors, proving cat_tools actually works when + # deployed via pg_tle (AWS's Trusted Language Extensions) + # instead of a filesystem .control file -- the same + # fresh-install and update-path scenarios (1)/(3) prove for + # a filesystem install, run again through pg_tle's own + # registration. That proof only means what it claims if + # pg_tle ISOLATION holds throughout the run (a stale + # filesystem .control file silently wins over a pg_tle + # registration of the same name -- PostgreSQL just + # resolves from disk instead of erroring), so every step + # here is bracketed by filesystem-cleanliness checks -- + # isolation is the precondition for a trustworthy result, + # not the point of the job. NOT folded into `test` like + # (3) was: pg_tle needs shared_preload_libraries (mixing + # pg_tle/non-pg_tle installs on one cluster can misbehave), + # so it needs its own dedicated cluster. # - # test -- FRESH install: CREATE EXTENSION at the current - # version on every supported PostgreSQL. The baseline - # a brand-new user gets. - # extension-update-test -- IN-PLACE update: CREATE EXTENSION at an OLD version - # then ALTER EXTENSION UPDATE (same PostgreSQL, no - # pg_upgrade). - # pg-upgrade-test -- BINARY pg_upgrade, SINGLE jump: install an OLD - # version on an OLD major, binary-upgrade the cluster - # straight to a NEWER major (skipping intermediate - # majors), then update the extension. Proves objects - # created on an old server work when read on a new one. - # pg-upgrade-stepwise -- BINARY pg_upgrade, EVERY major in sequence: one - # cluster climbing 10 -> 11 -> ... -> 18, exercising - # each individual major-to-major transition in turn. + # SPECIAL CASES -- apply to one scenario, not the whole matrix: # - # Supported update origins are 0.2.0, 0.2.1 and 0.2.2 (0.1.x is unsupported). - # 0.2.0 and 0.2.1 are BOTH tested as origins because ALTER EXTENSION UPDATE - # takes the shortest path: a 0.2.0 origin updates straight through the - # 0.2.0--0.2.2 script and never touches 0.2.1--0.2.2, so starting at 0.2.1 is - # the only way to exercise the 0.2.1--0.2.2 update script (spelled out at the - # pg-upgrade-test matrix). + # extension-update-test -- PG10 ONLY. The pre-0.2.2 install scripts + # (0.2.0/0.2.1) load on no other major (PG11 + # added pg_attribute.attmissingval; PG12+ exposes + # oid in SELECT *; both trip up those old + # scripts), so this job's only remaining purpose + # is the 0.2.0->0.2.2, 0.2.1->0.2.2, and + # 0.2.2->0.2.3 (view-rebuild) update scripts, on + # the one major that can still run them. Both + # 0.2.0 and 0.2.1 are tested because ALTER + # EXTENSION UPDATE takes the SHORTEST path: a + # 0.2.0 origin updates straight through + # 0.2.0--0.2.2 and never touches 0.2.1--0.2.2, so + # 0.2.1 is the only way to exercise that script. + # pg-upgrade-test -- BINARY pg_upgrade, SPECIFIC old_pg->new_pg + # jump pairs (every major is pg-upgrade-stepwise + # below): install an OLD version on an OLD + # major, binary-upgrade straight to a NEWER + # major (skipping intermediates), then update + # the extension -- proving objects created on an + # old server work when read on a new one. Every + # leg CLIMBS to a PostgreSQL that supports the + # current version, then updates and runs the + # full suite -- no leg stops short. A PG10/11 + # origin HOLDS the extension at 0.2.3 (the + # highest version those majors can reach) until + # PG12+. + # pg-tle-upgrade-test -- The pg_tle-deployed equivalent of + # pg-upgrade-test, on the jump pairs within + # pg_tle's own supported PostgreSQL range. + # pg-upgrade-stepwise -- BINARY pg_upgrade, EVERY major in sequence: + # one cluster climbing 10 -> 11 -> ... -> 18, + # catching a regression specific to one + # major-to-major boundary that a big-jump leg + # (pg-upgrade-test) would skip. # - # Two PostgreSQL-version floors shape the matrices: - # - The pre-0.2.2 install scripts (0.2.0 / 0.2.1) load ONLY on PG10: PG11 - # added pg_attribute.attmissingval and PG12+ exposes the oid system column - # in SELECT *, both of which those old scripts trip over. So a 0.2.0 / 0.2.1 - # origin can only start on PG10. - # - The current version needs PG12+: the 0.2.3->0.3.0 update runs - # ALTER TYPE ... ADD VALUE, which cannot run in a pre-PG12 transaction (and - # an extension update script is one). + # Both pg_upgrade jobs above only assert the version landed and run the + # base suite post-upgrade -- not every dimension the main matrix covers + # (TEST_SCHEMA, or whatever test-long grows to). "Version" partly loses its + # usual meaning here anyway (an upgrade is inherently about two majors, and + # the stepwise job climbs through all of them), but everything else the + # main matrix verifies should still hold post-upgrade and currently + # doesn't get re-checked -- same underlying gap as the NOTE below, tracked + # there rather than as a separate issue. # - # KEY invariant: every pg_upgrade leg CLIMBS to a PostgreSQL that supports the - # current version, then updates to the current version and runs the full suite - # -- no leg stops short. A PG10/11 origin simply HOLDS the extension at 0.2.3 - # (the highest version reachable on those majors) until the cluster reaches - # PG12+, where it is updated to the current version. + # NOTE: extension-update-test, pg-upgrade-test, pg-tle-upgrade-test, + # pg-upgrade-stepwise, and pg-tle-test do NOT exercise TEST_SCHEMA at all + # yet -- a deliberately deferred follow-up (see + # https://github.com/Postgres-Extensions/cat_tools/issues/65), not an + # oversight. # =========================================================================== test: needs: [changes] @@ -306,18 +375,57 @@ jobs: - name: Test on PostgreSQL ${{ matrix.pg }} run: | # Fail if the relkind drift source is empty (headers missing): the - # drift check must actually run on every version, not pass silently. + # drift check must run on every version, not pass silently. make check-relkind-source - # verify-results is the real gate: base.mk declares `verify-results: - # $(TEST_DEPS)`, not `verify-results: test` -- deliberately, since test's own - # recipe now exits non-zero as soon as it sees a regression, which would abort - # the chain before verify-results got to inspect and report the diff. Either - # way this runs the suite (via installcheck, one of $(TEST_DEPS)) and then - # checks the pgtap/regression.diffs. A bare `make test` is redundant here: - # it now also exits non-zero on regressions (pgxntool 2.3.0+), but - # verify-results is still the stricter, documented check. + + # The fresh/default-schema baseline: verify-results, this repo's + # stricter, pgtap-aware gate. base.mk declares + # `verify-results: $(TEST_DEPS)`, not `verify-results: test`, + # because test's own recipe now exits non-zero on a regression + # (pgxntool 2.3.0+), which would abort the chain before + # verify-results could inspect and report the diff. Either way + # this runs the suite via installcheck, then checks + # pgtap/regression.diffs. make verify-results + # test-long is every test-* target except plain `test` (currently + # test-update, test-schema, test-update-schema -- see the + # Makefile's test-long comment for the simple-inclusion rule). + # test-update and the update-scenario call below both update to + # the current version and run the suite, but through different + # code paths, not the same check twice: test-update drives + # test/install/load.sql's own committed update branch; + # update-scenario updates via a raw psql ALTER EXTENSION, outside + # load.sql, then runs the suite in TEST_LOAD_SOURCE=existing mode + # (load.sql's assert-only branch). A bug in load.sql's update-mode + # logic only test-update catches; a bug in update-scenario's + # guard/structural-diff logic only update-scenario catches. Each + # test-* target recurses into a plain `make test`, not + # `verify-results` -- confirmed empirically (deliberately broke a + # schema-quoting case and an update-path case locally, in this + # exact CI-step order, and confirmed each failed loudly) that + # pgxntool 2.3.0's test-exits-nonzero-on-regression behavior is a + # real enough gate for these. + make test-long + + # The guard-proved update-to-current check. USED to be its own + # matrix job (extension-update-test's PG12+ leg) -- folded in here + # since it runs on these same majors, and this job's container, + # checkout, and cat_tools install are already there; a separate + # job would pay for all of that again for no added coverage. + # update-scenario creates its own database (cat_tools_update, not + # pg_regress's own throwaway db from the calls above -- confirmed + # no name collision), plants + proves the dependency guard, ALTER + # EXTENSION UPDATEs 0.2.2 to current, structurally compares the + # result against a fresh install (assert_matches_fresh, via + # bin/structural_diff), and runs the full suite against it in + # existing mode. Reusing the SAME suite and expected output + # asserts the updated database behaves identically to a fresh + # install. See extension-update-test below for the PG10-only + # legacy checks this does NOT cover (those pre-0.2.2 scripts don't + # load on PG12+ at all). + bin/test_existing update-scenario cat_tools_update 0.2.2 + # Style linter (https://github.com/Postgres-Extensions/linter, vendored at # .vendor/linter). Deliberately checked out WITHOUT submodules -- `make # lint` is the same command a developer runs locally, and lint.mk @@ -567,6 +675,7 @@ jobs: run: | pg_ctlcluster 10 test stop pg_dropcluster 10 test + # -p 5432: force the well-known port (see pg-upgrade-test for why). pg_createcluster -p 5432 10 test -- $INITDB_OPTS pg_ctlcluster 10 test start @@ -599,11 +708,13 @@ jobs: for new in $CLIMB_PG; do echo "=== binary pg_upgrade PostgreSQL $old -> $new ===" apt-get install -y postgresql-$new postgresql-server-dev-$new + # PG_CONFIG explicit: several majors are installed, so the default # pg_config on PATH may not be $new's. make install PG_CONFIG=/usr/lib/postgresql/$new/bin/pg_config pg_ctlcluster $old test stop pg_createcluster -p 5432 $new test -- $INITDB_OPTS + # PG17+ writes logs under the new datadir; older versions to CWD. Dump # both on failure (same handling as pg-upgrade-test). mkdir -p /tmp/pg_upgrade_logs @@ -620,6 +731,7 @@ jobs: -name '*.log' 2>/dev/null | sort | xargs -r tail -n +1; exit 1; } pg_ctlcluster $new test start pg_isready -t 30 + # Suite policy across the climb. We unfortunately CANNOT run the suite # against the OLD version (0.2.3) at the pre-PG12 steps (10->11, 11->12): # the suite matches the CURRENT version's objects and we do not maintain @@ -639,59 +751,40 @@ jobs: old=$new done - # Proves the in-place extension update path: CREATE EXTENSION at an OLD cat_tools - # version then ALTER EXTENSION UPDATE (no pg_upgrade, same PostgreSQL). On PG12+ - # it updates 0.2.2 -> current and runs the FULL suite against the updated - # database (same expected output as a fresh install, so an updated DB must behave - # identically). The PG10 leg only exercises the pre-0.2.2 update scripts, the - # sole version where they still load. Complements pg-upgrade-test, which covers - # the cross-major-version binary upgrade instead. + # Proves the pre-0.2.2 in-place update scripts (0.2.0->0.2.2, 0.2.1->0.2.2, and + # the 0.2.2->0.2.3 view rebuild they route through) still apply -- the ONLY + # PostgreSQL major they still load on (PG11 added pg_attribute.attmissingval + # and PG12+ exposes the oid system column in SELECT *, both of which those old + # scripts trip over). PG10-only, no matrix: this job's PG12+ leg (updating + # 0.2.2 -> the current version and running the full suite) moved into the + # `test` job's "Test on PostgreSQL" step (folded in there instead of its own + # matrix job, since that job already has a running PG major, a checkout, and + # cat_tools installed on disk -- see that job's comment for why). What's left + # here is PG10-only by definition, so it runs directly against + # needs.changes.outputs.legacy_pg (no update_pg combined list needed anymore). extension-update-test: # Gated behind test+lint -- see the comment on pg-upgrade-test's needs. needs: [changes, test, lint] if: success() && needs.changes.outputs.docs_only != 'true' - strategy: - matrix: - # PG12+: exercise the WIDEST update path we support — CREATE EXTENSION at - # the 0.2.2 backward-compat floor, ALTER EXTENSION UPDATE to the CURRENT - # version, and run the full suite against the updated database. 0.2.2 is - # the floor because the 0.2.0/0.2.1 install scripts fail on PG11+/PG12+; - # PG12 is the PostgreSQL floor because the update runs - # `ALTER TYPE ... ADD VALUE`, which PG11 and below cannot run in an - # extension update script (lifted in PG12). - # PG10: the ONLY version where the pre-0.2.2 install scripts still load, - # so the only place the 0.2.0->0.2.2 and 0.2.1->0.2.2 update scripts and - # the 0.2.2->0.2.3 view rebuild on the broken path can be exercised. They - # target 0.2.2/0.2.3 (not the current version) and use no - # ALTER TYPE ... ADD VALUE, so they run on PG10. The PG10 leg runs only - # those legacy checks — not the current-version suite (the current version - # needs PG12+: the 0.2.3->0.3.0 update adds enum values via ALTER TYPE ... - # ADD VALUE, unrunnable in a pre-PG12 transaction). See the per-step `if` - # guards. - # - # Current-supported majors + the legacy PG10 floor, from the single - # source in the changes job (update_pg = supported_pg plus legacy_pg). - pg: ${{ fromJSON(needs.changes.outputs.update_pg) }} - name: ⬆️ Extension update test on PostgreSQL ${{ matrix.pg }} + env: + # Single source of truth (changes job) rather than hardcoding "10" here. + PG: ${{ needs.changes.outputs.legacy_pg }} + name: ⬆️ Extension update test on PostgreSQL ${{ needs.changes.outputs.legacy_pg }} (legacy scripts) runs-on: ubuntu-latest container: pgxn/pgxn-tools steps: - - name: Start PostgreSQL ${{ matrix.pg }} - run: pg-start ${{ matrix.pg }} + - name: Start PostgreSQL ${{ env.PG }} + run: pg-start ${{ env.PG }} - name: Check out the repo uses: actions/checkout@v6 - - name: Install rsync and server headers - # server-dev provides catalog/pg_class.h for the relkind drift check - # (see the "test" job); required so check-relkind-source below passes. - # PG10 runs only the legacy-script checks (no suite), so it needs no headers. - if: matrix.pg != '10' - run: apt-get install -y rsync postgresql-server-dev-${{ matrix.pg }} - name: Install rsync - if: matrix.pg == '10' + # No server-dev headers needed: unlike the `test` job, nothing here + # calls check-relkind-source (no suite runs on this legacy-only PG + # major -- the current version needs PG12+, see below). run: apt-get install -y rsync - name: Install cat_tools (all versions) run: make install - - name: Test pre-0.2.2 update scripts + 0.2.2→0.2.3 rebuild (PG10 only) + - name: Test pre-0.2.2 update scripts + 0.2.2→0.2.3 rebuild # 0.2.0/0.2.1 install only on PG10; their update scripts target 0.2.2 (not # the current version) and are otherwise never exercised. Both origins are # checked because ALTER EXTENSION UPDATE takes the shortest path (see the @@ -721,7 +814,6 @@ jobs: # still present) -- complementing the stronger 10→18 pg_upgrade bridge # legs. `$$` is escaped as `\$\$` so the shell passes literal dollar # quotes through to psql. - if: matrix.pg == '10' run: | bin/test_existing update-check-version cat_tools_from_020 0.2.0 0.2.2 bin/test_existing update-check-version cat_tools_from_021 0.2.1 0.2.2 @@ -731,15 +823,6 @@ jobs: bin/test_existing update-check-version "$db" "$from" 0.2.3 psql -d "$db" -v ON_ERROR_STOP=1 -c "DO \$\$ BEGIN IF EXISTS (SELECT 1 FROM pg_attribute WHERE attrelid='_cat_tools.pg_class_v'::regclass AND attname='relhasoids' AND NOT attisdropped AND attnum>0) THEN RAISE EXCEPTION 'pg_class_v still exposes relhasoids after update through 0.2.2->0.2.3 -- rebuild did not fire'; END IF; END \$\$" done - - name: Update 0.2.2 → current and run the suite (existing mode, PG12+) - # update-scenario creates a real database at 0.2.2, plants + proves the - # dependency guard, ALTER EXTENSION UPDATEs to the current version, and - # runs the suite against that updated database in existing mode (asserting - # the version and that the guard still blocks a drop). Reusing the SAME - # suite and expected output asserts the updated database behaves - # identically to a fresh install. - if: matrix.pg != '10' - run: bin/test_existing update-scenario cat_tools_update 0.2.2 pg-tle-test: # Gated behind test+lint -- see the comment on pg-upgrade-test's needs. diff --git a/Makefile b/Makefile index 4c200bc..1cb7cad 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,21 @@ testdeps: $(wildcard test/*.sql test/helpers/*.sql) # Be careful not to include directories in this +# Test targets, briefly (see each target's own comment below for the why): +# test -- fresh install, default schema. The baseline check. +# test-schema -- test, but with TEST_SCHEMA set to a quoting-requiring +# schema name. +# test-update -- test, but updated from TEST_UPDATE_FROM (default +# 0.2.2) to the current version instead of a fresh +# install. No schema targeting -- symmetric with +# test; test-update-schema below is the combination. +# test-update-schema -- test-update + test-schema together: the one +# scenario nothing else covers. +# test-long -- every test-* target EXCEPT test itself (currently: +# test-update, test-schema, test-update-schema). As +# new test-* targets get added, they just join this +# prerequisite list -- no redesign needed. +# test-all -- test + test-long: the full local pre-push gate. + # Committed-once install of the extension + test roles. # # test/install/load.sql is the ONE place that installs everything the pgTAP @@ -56,15 +72,118 @@ endif TEST_UPDATE_FROM ?= 0.2.2 TEST_UPDATE_TO ?= -export PGOPTIONS := $(PGOPTIONS) -c cat_tools.test_load_mode=$(TEST_LOAD_SOURCE) -c cat_tools.test_update_from=$(TEST_UPDATE_FROM) -c cat_tools.test_update_to=$(TEST_UPDATE_TO) +# TEST_SCHEMA is a second, independent GUC switch, same propagation mechanism +# as TEST_LOAD_SOURCE above. THE POINT: prove cat_tools works correctly even +# when the 'cat_tools' schema itself is NEVER part of the active search_path -- +# a normal, legitimate deployment choice for a tooling extension (so it never +# shadows anything, and callers must always schema-qualify it). If cat_tools' +# own SQL secretly relied on unqualified name resolution somewhere, it would +# keep working by accident in an ordinary fresh-install run (which never +# touches search_path) and only break in that deployment -- TEST_SCHEMA exists +# to force that scenario here instead. See test/install/load.sql for the +# actual mechanism (a schema created and made the ONLY entry on search_path, +# CREATE EXTENSION cat_tools WITH SCHEMA cat_tools run explicitly against it, +# then an assertion that 'cat_tools' never resolves via search_path anyway): +# - empty (default): none of that -- CREATE EXTENSION cat_tools runs exactly +# as a brand-new user would type it, no WITH SCHEMA clause, landing +# wherever the session's ambient search_path already resolves. +# - non-empty: load.sql creates that schema (quoting it, so a name that +# requires quoting -- e.g. mixed case -- works) and SETs search_path to +# ONLY that schema before installing. +# +# Exported unconditionally, same reasoning as TEST_UPDATE_FROM/TO: an empty +# default is fine, and load.sql reads it without missing_ok. +TEST_SCHEMA ?= + +export PGOPTIONS := $(PGOPTIONS) -c cat_tools.test_load_mode=$(TEST_LOAD_SOURCE) -c cat_tools.test_update_from=$(TEST_UPDATE_FROM) -c cat_tools.test_update_to=$(TEST_UPDATE_TO) -c cat_tools.test_schema=$(TEST_SCHEMA) + +# Scope boundary (deliberate, not an oversight): CI's extension-update-test, +# pg-upgrade-test, pg-tle-upgrade-test, pg-upgrade-stepwise, and pg-tle-test +# jobs do NOT exercise TEST_SCHEMA at all yet -- they drive the extension +# through bin/test_existing's own createdb/CREATE EXTENSION/ALTER EXTENSION +# UPDATE flow, not this Makefile's TEST_LOAD_SOURCE path, so none of the +# test-* targets below reach them either. See +# https://github.com/Postgres-Extensions/cat_tools/issues/65 (still open -- +# these are local-dev-convenience targets, not a fix for that issue). +# +# Convenience wrapper: `make test-schema` == `make test TEST_SCHEMA=CatToolsSchema`. +# Must recurse (a fresh $(MAKE)) rather than depend on `test`, so the parse-time +# TEST_SCHEMA default above re-evaluates with CatToolsSchema set -- same +# reasoning as test-update below. CatToolsSchema is hardcoded here rather than +# a variable: there's exactly one quoting-requiring name this repo tests +# against (see TEST_SCHEMA above for what it actually proves), so a variable +# indirection would add nothing. +.PHONY: test-schema +test-schema: + $(MAKE) test TEST_SCHEMA=CatToolsSchema # Convenience wrapper: `make test-update` == `make test TEST_LOAD_SOURCE=update`. # Must recurse (a fresh $(MAKE)) rather than depend on `test`, so the parse-time -# TEST_LOAD_SOURCE conditional above re-evaluates with update set. +# TEST_LOAD_SOURCE conditional above re-evaluates with update set. Deliberately +# NO schema targeting -- symmetric with plain `test` above (each is the +# "default schema" leg of its own load mode); test-update-schema below is the +# update+schema combination. .PHONY: test-update test-update: $(MAKE) test TEST_LOAD_SOURCE=update +# Convenience wrapper: TEST_LOAD_SOURCE=update AND TEST_SCHEMA=CatToolsSchema +# together -- the one scenario nothing else here covers (test-update above +# covers the update path with no schema targeting; test-schema above covers +# schema targeting on the fresh path only). Also a partial answer to +# https://github.com/Postgres-Extensions/cat_tools/issues/65, which asks for +# TEST_SCHEMA coverage on the update path -- see the scope-boundary comment +# above for the CI-level gap (extension-update-test/pg-upgrade-test) this does +# NOT close. +.PHONY: test-update-schema +test-update-schema: + $(MAKE) test TEST_LOAD_SOURCE=update TEST_SCHEMA=CatToolsSchema + +# .NOTPARALLEL covers this whole test-* family, not just test-long/test-all +# below (the only two with real prerequisites): each of these targets +# recurses into its own $(MAKE) invocation against the SAME throwaway test +# database, and running two of them concurrently (under a hypothetical +# `make -j`) would corrupt that shared state. That lets test-long/test-all +# list bare prerequisites instead of writing out sequential $(MAKE) calls in +# every recipe body. Safe to declare this broadly: nothing in this repo's +# build ever invokes `-j` (grepped ci.yml, this Makefile, sql.mk, lint.mk, and +# pgxntool's own .mk files/docs -- none do), and empirically, GNU Make 4.3's +# `.NOTPARALLEL: a b` does NOT scope narrowly to just `a`/`b` anyway -- it +# serializes the WHOLE invoked build graph once ANY targets are listed, so +# there's no narrower behavior being given up here even in principle. +.NOTPARALLEL: test test-schema test-update test-update-schema test-long test-all + +# The rule is simple inclusion: test-long is every test-* target EXCEPT test +# itself, full stop -- currently test-update, test-schema, test-update-schema. +# New test-* targets just join this list, no redesign needed. +# +# test-long isn't purely local, though -- CI's `test` job calls `make +# test-long` directly, on every supported PostgreSQL major, then runs +# bin/test_existing's update-scenario check a few lines later in the same +# step. test-update and update-scenario LOOK redundant (both update to the +# current version and run the suite) but exercise different code paths, not +# the same check twice: test-update drives test/install/load.sql's own +# committed update branch (CREATE EXTENSION VERSION 0.2.2, then ALTER +# EXTENSION UPDATE, inside the pre-suite install step); update-scenario +# issues its ALTER EXTENSION UPDATE directly via psql, outside load.sql +# entirely, then runs the suite in TEST_LOAD_SOURCE=existing mode (load.sql's +# assert-only branch -- no CREATE/ALTER at all there). A bug in load.sql's +# update-mode logic only test-update catches; a bug in the guard/ +# structural-diff logic only update-scenario catches. Keeping both is +# correct coverage, not an accepted duplicate. +# +# Bare prerequisites, not sequential $(MAKE) calls in the recipe body -- +# simpler to read than repeating the same calls here and in each target's own +# definition, and safe only because of the .NOTPARALLEL declaration above. +.PHONY: test-long +test-long: test-update test-schema test-update-schema + +# The full local pre-push gate: test (the one target test-long deliberately +# excludes) plus test-long (everything else). Bare prerequisites, safe under +# the same .NOTPARALLEL declaration as test-long. +.PHONY: test-all +test-all: test test-long + # Versioned SQL is generated from .sql.in at build time. That generation, the # DATA list that installs it, and the relkind drift source all live in sql.mk, # which also owns `include pgxntool/base.mk` (base.mk has no include guard, so it @@ -77,6 +196,19 @@ test-update: # are set before this include so base.mk (pulled in by sql.mk) sees them. include sql.mk +# A second PGOPTIONS export, appending to (not replacing) the one above: PGXNVERSION +# (the distribution version from META.json) is only defined AFTER `include sql.mk` +# pulls in base.mk's meta.mk include, so this line cannot be merged into the +# earlier export without $(PGXNVERSION) evaluating empty there. load.sql's +# existing-mode check reads this GUC (cat_tools.pgxn_version) instead of querying +# pg_available_extensions.default_version, because that view is FILESYSTEM-based +# and returns NULL for an extension registered purely via pg_tle (no control file +# on disk) -- exactly the deployment method the pg_tle CI jobs use. This mirrors +# bin/test_existing's own current_version() helper, which already avoids +# pg_available_extensions for the identical reason (it shells out to +# `make -s print-PGXNVERSION` instead). +export PGOPTIONS := $(PGOPTIONS) -c cat_tools.pgxn_version=$(PGXNVERSION) + # Clean the cruft pg_regress writes into test/install/ (the self-comparing # result .out and its diff), which is listed in test/install/.gitignore. This is # the pgxntool test/install feature configured above (not SQL generation), so diff --git a/test/install/load.sql b/test/install/load.sql index f0fd25a..a7dc2e3 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -16,6 +16,18 @@ * deps.sql (run per-test) installs nothing; it only sets the psql variables the * suite references. * + * ON_ERROR_STOP is REQUIRED here: pg_regress treats a nonzero psql exit code as + * a real test failure (see the check-relkind-source comment in the `test` CI + * job for a concrete example of this mechanism firing), but psql only exits + * nonzero for a mid-script error when ON_ERROR_STOP is set -- without it, psql + * prints the error and CONTINUES to the next statement, and this file's own + * pg_regress entry self-compares (see test/install/.gitignore), so nothing + * would ever diff either. Without ON_ERROR_STOP, EVERY RAISE EXCEPTION / + * hard-error check in this file (the TEST_SCHEMA guard below, the + * test_load_mode validation, the existing-mode version assertion) would print + * a message and then silently let the rest of the file run to a "successful" + * exit. + * * Three modes, selected by the cat_tools.test_load_mode placeholder GUC, which * the Makefile TEST_LOAD_SOURCE block sets via PGOPTIONS (fresh is the default): * - fresh (default): plain CREATE EXTENSION cat_tools (current version). @@ -38,6 +50,16 @@ * - PG12 is the PostgreSQL floor: ALTER TYPE ... ADD VALUE cannot run inside * a transaction block (or an extension update script) at all before PG12. */ +/* + * REQUIRED -- see the file header above for why. Without this, every RAISE + * EXCEPTION guard below (TEST_SCHEMA, test_load_mode, the existing-mode + * version assertion -- which already caught a real pg_tle regression once + * this was added) prints an error and keeps going instead of aborting, and + * this file's self-comparing pg_regress entry never diffs either -- so + * removing this silently turns every one of those guards into a no-op. + */ +\set ON_ERROR_STOP on + SET client_min_messages = WARNING; /* @@ -48,6 +70,81 @@ SET client_min_messages = WARNING; */ \i test/roles.sql +/* + * TEST_SCHEMA targeting, independent of the mode selection below. The + * Makefile always exports cat_tools.test_schema via PGOPTIONS (empty by + * default); read it WITHOUT missing_ok, same reasoning as test_load_mode. + * + * THE POINT of this whole mechanism: prove cat_tools works correctly even + * when the 'cat_tools' schema itself is NEVER part of the active + * search_path. Installing WITH SCHEMA cat_tools always places the + * extension's objects there (cat_tools' control file pins schema = + * 'cat_tools' with relocatable = false), but that says nothing about + * whether cat_tools' OWN internal SQL -- views and functions referencing + * each other -- actually resolves those references without depending on + * 'cat_tools' being searchable. Deliberately keeping a tooling extension's + * schema off every role's default search_path (so it never shadows + * anything, and callers must always schema-qualify it) is a normal, + * legitimate deployment choice -- if cat_tools' own SQL secretly relied on + * unqualified name resolution somewhere, it would keep working by accident + * in this suite's ordinary fresh-install runs (which never touch + * search_path at all) and only break in that realistic deployment. TEST_SCHEMA + * exists to force that scenario and catch it here instead. + * + * Empty (the default): do nothing -- no CREATE SCHEMA, no SET search_path, + * no WITH SCHEMA clause on CREATE EXTENSION below. This is the "brand-new + * user just types CREATE EXTENSION cat_tools" path, landing wherever the + * session's ambient search_path already resolves (which, per the control + * file, is always 'cat_tools' regardless). + * + * Non-empty: create THAT schema (quoting it, so a name that requires + * quoting -- e.g. mixed case -- works) and SET search_path to ONLY that + * schema -- simulating a normal user's own working schema, unrelated to + * cat_tools, with nothing else (no public, no "$user") ambient either. + * CREATE EXTENSION below then explicitly adds WITH SCHEMA cat_tools -- + * deliberate, not implicit -- and after install (past the mode-selection + * block below) a check asserts 'cat_tools' never appears in the resolved + * search_path. The DO block immediately below is a much narrower sanity + * check: only that THIS fixture's own SET search_path actually took effect, + * not a test of cat_tools' behavior at all. + */ +SELECT current_setting('cat_tools.test_schema') AS cat_tools_test_schema \gset +SELECT :'cat_tools_test_schema' <> '' AS cat_tools_has_schema \gset + +/* + * \set (not a SQL CASE expression): cat_tools_has_schema holds Postgres's + * boolean EXTERNAL TEXT form ('t'/'f') from the \gset above, which \if + * accepts directly but which is NOT valid bare SQL (CASE WHEN t THEN ... + * would parse "t" as an undefined column reference, not a boolean literal). + */ +\if :cat_tools_has_schema +\set cat_tools_with_schema_clause 'WITH SCHEMA cat_tools' +\else +\set cat_tools_with_schema_clause '' +\endif +-- end \if :cat_tools_has_schema (with_schema_clause \set) + +\if :cat_tools_has_schema +CREATE SCHEMA IF NOT EXISTS :"cat_tools_test_schema"; +SET search_path = :"cat_tools_test_schema"; + +/* + * Sanity check on the fixture itself (see the comment above) -- not a test + * of cat_tools. + */ +DO $DO$ +BEGIN + IF current_setting('cat_tools.test_schema') <> ALL (current_schemas(false)) THEN + RAISE EXCEPTION + 'TEST_SCHEMA fixture schema % did not take effect in the resolved search_path' + , current_setting('cat_tools.test_schema') + ; + END IF; +END +$DO$; +\endif +-- end \if :cat_tools_has_schema + /* * Mode selection. The Makefile always exports cat_tools.test_load_mode via * PGOPTIONS. Read it WITHOUT missing_ok: if the GUC did not propagate (a break @@ -78,24 +175,37 @@ SELECT \if :cat_tools_mode_existing /* * existing mode: do NOT touch the extension. Assert it is installed and at the - * current default_version -- the pg_upgrade / external update the database just - * went through is exactly what the suite is validating, so dropping or + * CURRENT version -- the pg_upgrade / external update the database just went + * through is exactly what the suite is validating, so dropping or * reinstalling it would defeat the test. Fail loudly on absence or mismatch. * (CI additionally plants a dependency guard so a stray non-CASCADE drop would * error rather than silently reinstall; see bin/test_existing.) + * + * The current version comes from the cat_tools.pgxn_version GUC (set by the + * Makefile from PGXNVERSION), NOT pg_available_extensions.default_version: + * that view is FILESYSTEM-based (it reads installed .control files) and + * returns NULL for an extension registered purely via pg_tle, which is + * exactly how the pg_tle CI jobs deploy cat_tools (no control file ever + * touches disk there) -- confirmed by reproducing this existing-mode check + * against a pg_tle-only registration locally, where the pg_available_extensions + * version came back NULL and this assertion failed loudly (as it should + * whenever it can't determine the current version, rather than silently + * comparing against NULL). Mirrors bin/test_existing's own current_version() + * helper, which avoids pg_available_extensions for the identical reason (it + * shells out to `make -s print-PGXNVERSION` instead). */ DO $DO$ DECLARE v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'cat_tools'); - v_default text := (SELECT default_version FROM pg_available_extensions WHERE name = 'cat_tools'); + v_current text := current_setting('cat_tools.pgxn_version'); BEGIN IF v_installed IS NULL THEN RAISE EXCEPTION 'test_load_mode=existing but the cat_tools extension is not installed'; END IF; - IF v_installed IS DISTINCT FROM v_default THEN + IF v_installed IS DISTINCT FROM v_current THEN RAISE EXCEPTION - 'cat_tools is installed at version % but the current default_version is %' - , v_installed, v_default + 'cat_tools is installed at version % but the current version is %' + , v_installed, v_current ; END IF; END @@ -134,6 +244,21 @@ SELECT pg_temp.drop_role(:'use_role'); SELECT pg_temp.drop_role(:'no_use_role'); SELECT pg_temp.drop_role('cat_tools__usage'); +\if :cat_tools_has_schema +/* + * Unlike a bare CREATE EXTENSION cat_tools (which auto-creates the schema + * named in the control file if needed), CREATE EXTENSION ... WITH SCHEMA + * cat_tools requires that schema to ALREADY exist -- even though it's the + * exact same name the control file pins -- confirmed by testing this: it + * errors "schema \"cat_tools\" does not exist" otherwise. DROP EXTENSION + * above never drops the schema itself (only the extension's member + * objects), so IF NOT EXISTS makes this correct whether this is the first + * install or a re-run against a persistent cluster. + */ +CREATE SCHEMA IF NOT EXISTS cat_tools; +\endif +-- end \if :cat_tools_has_schema + \if :cat_tools_mode_update /* * update mode: install an older version, then ALTER EXTENSION UPDATE. The @@ -155,7 +280,7 @@ SELECT CASE WHEN :'cat_tools_test_update_to' = '' THEN '' ELSE format('TO %L', :'cat_tools_test_update_to') END AS cat_tools_update_to_clause \gset -CREATE EXTENSION cat_tools VERSION :'cat_tools_test_update_from'; +CREATE EXTENSION cat_tools :cat_tools_with_schema_clause VERSION :'cat_tools_test_update_from'; /* * Suppress the deprecation NOTICEs the update scripts emit, matching the * approach used by test/build/upgrade.sql. @@ -164,9 +289,36 @@ SET client_min_messages = ERROR; ALTER EXTENSION cat_tools UPDATE :cat_tools_update_to_clause; SET client_min_messages = WARNING; \else -CREATE EXTENSION cat_tools; +CREATE EXTENSION cat_tools :cat_tools_with_schema_clause; \endif -- end \if :cat_tools_mode_update (fresh vs. update install branch) + +\if :cat_tools_has_schema +/* + * THIS is the actual point of TEST_SCHEMA (see the comment where it's read, + * above): cat_tools just installed WITH SCHEMA cat_tools while search_path + * held only an unrelated schema -- if 'cat_tools' shows up in the resolved + * search_path anyway, something (this fixture, a role default, a prior + * statement) put it there, and cat_tools' own SQL cannot have been relying + * on it being searchable to get this far. If cat_tools' internal views/ + * functions instead depend on unqualified name resolution somewhere, THAT + * would surface as a later pgTAP failure, not here -- this check only + * proves the precondition (cat_tools' schema absent from search_path) held + * during install, which is what makes any later pgTAP pass actually mean + * something. + */ +DO $DO$ +BEGIN + IF 'cat_tools' = ANY (current_schemas(false)) THEN + RAISE EXCEPTION + 'cat_tools schema must NOT be part of the resolved search_path here -- got %' + , current_schemas(false) + ; + END IF; +END +$DO$; +\endif +-- end \if :cat_tools_has_schema \endif -- end \if :cat_tools_mode_existing (existing mode skips the whole (re)install block)