From 4d60401eb8d281a10335a5d2009310e60125c526 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 13:29:35 -0500 Subject: [PATCH 1/9] Add TEST_SCHEMA, a second independent test-harness switch that installs cat_tools while a targeted, non-default schema is the ONLY entry on search_path, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE. 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. Empty (the default) does 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 creates that schema (quoting it, so mixed-case names work), SETs search_path to ONLY that schema, installs with an explicit WITH SCHEMA cat_tools clause, and asserts afterward that cat_tools never appears in the resolved search_path -- which is what makes any later pgTAP pass in that run actually mean something. \set ON_ERROR_STOP on is set near the top of test/install/load.sql so that assertion (and every other RAISE EXCEPTION in the file) can actually fail the pg_regress run instead of psql printing an error and continuing past it; this already caught a real pg_tle-specific regression once (the existing-mode version assertion used to compare against pg_available_extensions.default_version, which is filesystem-based and returns NULL for a pg_tle-only registration -- fixed by reading a new cat_tools.pgxn_version GUC instead, mirroring bin/test_existing's own current_version() helper). A new TEST_LONG_SCENARIOS make variable drives test-long, an explicit list of exactly the scenarios nothing else already covers -- not a full TEST_LOAD_SOURCE x TEST_SCHEMA cross product. fresh:CatToolsSchema and update:CatToolsSchema are the only places TEST_SCHEMA is exercised at all (the latter also a partial answer to Postgres-Extensions/cat_tools#65, which asks for TEST_SCHEMA coverage on the update path). Both empty-schema combinations are deliberately absent: {fresh, } is exactly the plain fresh-install/default-schema case make test/installcheck already checks, and {update, } is exactly what CI's extension-update-test job already proves, more thoroughly, on the same PostgreSQL majors. Each test-long iteration gates via verify-results, this repo's stricter, documented, pgtap-aware check. CI's test job runs an explicit make verify-results for the fresh/default-schema baseline (restoring what that step always did before this PR) followed by make test-long, so every supported PostgreSQL major covers the baseline plus both quoting-requiring-schema scenarios, with no separate matrix leg needed for any of them. test-all sequences a quick make test smoke build with make test-long for a full local pre-push check. A short top-of-file summary comment maps all four test targets (test, test-update, test-long, test-all) for a reader who doesn't want to piece the picture together from four separate comment blocks. Scope boundary, called out explicitly rather than left implicit: extension-update-test and pg-upgrade-test do not exercise TEST_SCHEMA at all yet. Wiring TEST_SCHEMA through the update and pg_upgrade paths is a deliberately deferred follow-up, not an oversight. --- .github/workflows/ci.yml | 54 ++++++++++--- Makefile | 125 ++++++++++++++++++++++++++++- test/install/load.sql | 168 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 327 insertions(+), 20 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fb8a02..fb402d3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -247,8 +247,30 @@ jobs: # carry the details; this is the big picture): # # test -- FRESH install: CREATE EXTENSION at the current - # version on every supported PostgreSQL. The baseline - # a brand-new user gets. + # version on every supported PostgreSQL. Its "Test + # on PostgreSQL" step runs a plain `make + # verify-results` for the fresh/default-schema + # baseline, THEN `make test-long`, which loops the + # full suite once per TEST_LONG_SCENARIOS entry -- + # fresh:CatToolsSchema, update:CatToolsSchema -- + # so every major covers the baseline plus both + # quoting-requiring-schema scenarios, with no + # separate matrix leg for any of them. + # Deliberately NOT a full fresh/update x + # schema-empty/quoting-requiring cross product: + # {fresh, } is the explicit verify-results + # call just described (test-long including it too + # would just re-run the same case again), and + # {update, } is dropped entirely because + # it's exactly what extension-update-test below + # already proves, more thoroughly, on the same + # PostgreSQL majors -- see the TEST_LONG_SCENARIOS + # comment in the Makefile. NOTE: + # extension-update-test and pg-upgrade-test below + # 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. # extension-update-test -- IN-PLACE update: CREATE EXTENSION at an OLD version # then ALTER EXTENSION UPDATE (same PostgreSQL, no # pg_upgrade). @@ -308,15 +330,27 @@ jobs: # Fail if the relkind drift source is empty (headers missing): the # drift check must actually 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: a plain verify-results call, + # NOT `make test-long` -- test-long deliberately excludes this exact + # {fresh, } scenario (see its TEST_LONG_SCENARIOS comment in + # the Makefile), since it exists to cover ONLY what nothing else + # does. This call is what actually exercises the baseline now, so it + # can't be dropped. 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 (pgxntool + # 2.3.0+), 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; verify-results remains the stricter, + # documented check, so it's used here rather than leaning on plain + # `test`'s newer (and less strict) non-zero exit. make verify-results + # test-long loops verify-results once per TEST_LONG_SCENARIOS entry + # -- fresh:CatToolsSchema, update:CatToolsSchema -- covering exactly + # the two scenarios nothing else (not the verify-results call above, + # not extension-update-test below) already covers. See the + # Makefile's TEST_LONG_SCENARIOS/test-long comments. + make test-long # Style linter (https://github.com/Postgres-Extensions/linter, vendored at # .vendor/linter). Deliberately checked out WITHOUT submodules -- `make diff --git a/Makefile b/Makefile index 4c200bc..fc3e186 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,16 @@ 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-update -- test, but updated from TEST_UPDATE_FROM (default 0.2.2) to +# the current version instead of a fresh install. +# test-long -- ONLY the scenarios nothing else covers (see +# TEST_LONG_SCENARIOS below): TEST_SCHEMA's quoting-requiring +# pipeline on the fresh and update paths. Deliberately +# excludes anything test/test-update/CI's other jobs already +# check. +# 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 +67,112 @@ 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) + +# Schema variant test-long exercises: one mixed-case name that requires SQL +# identifier quoting -- see TEST_SCHEMA above for what this actually proves. +# (The empty/ambient-search_path default is deliberately NOT in test-long's +# scenario list at all -- see TEST_LONG_SCENARIOS below for why.) +# +# Scope boundary (deliberate, not an oversight): CI's extension-update-test and +# pg-upgrade-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 +# test-long below doesn't reach them either. See +# https://github.com/Postgres-Extensions/cat_tools/issues/65 (still open -- +# this is a local-dev-convenience fix, not a fix for that issue). +# +# TEST_LONG_SCENARIOS is an explicit list of "TEST_LOAD_SOURCE:TEST_SCHEMA" +# pairs -- NOT a full cross product of every TEST_LOAD_SOURCE x every +# TEST_SCHEMA. test-long exists to cover ONLY what nothing else already +# covers, so BOTH empty-schema combinations are deliberately absent, each for +# a different reason: +# - {fresh, } is exactly the plain fresh-install/default-schema case +# that `make test`/`installcheck` (and CI's `test` job step, via its own +# explicit `make verify-results` call -- see ci.yml) already checks. +# test-long including it too would just be re-running that same case +# again under a different name. +# - {update, } is exactly "0.2.2 updated to the current version, +# default schema, full suite", already proven -- more thoroughly -- by +# CI's extension-update-test job (bin/test_existing's update_scenario +# additionally plants and proves the dependency guard) on the SAME +# PostgreSQL majors (12-18) that the `test` job (and so test-long) runs on. +# Repeating either here would add CI wall-clock with no added confidence. The +# two scenarios kept are exactly the ones nothing else covers: TEST_SCHEMA's +# quoting-requiring pipeline on the fresh path (fresh:CatToolsSchema) and on +# the update path (update:CatToolsSchema) -- the latter also a partial answer +# to https://github.com/Postgres-Extensions/cat_tools/issues/65, which asks +# for TEST_SCHEMA coverage on the update path. +TEST_LONG_SCENARIOS ?= fresh:CatToolsSchema update:CatToolsSchema + +# Loops the full suite once per TEST_LONG_SCENARIOS entry via `verify-results`, +# NOT plain `test`: verify-results is this repo's documented CI-safe gate +# (make test alone doesn't reliably fail on regressions the way verify-results +# does -- see CLAUDE.md), and CI relies on test-long to fail loudly on a +# regression the same way a single +# `make verify-results TEST_LOAD_SOURCE=X TEST_SCHEMA=Y` already does. Must +# recurse (a fresh $(MAKE) per iteration, not a plain shell variable) for the +# same reason test-update recurses: these GUCs only take effect if exported +# into PGOPTIONS before the sub-make's own parse phase. Each scenario is +# "load_source:schema"; %% / # parameter expansion splits on the FIRST colon +# (also correct if a schema were ever empty, e.g. "fresh:" -> schema ""), so a +# schema name containing a colon would break this, but none of ours do. +.PHONY: test-long +test-long: + @for scenario in $(TEST_LONG_SCENARIOS); do \ + load_source=$${scenario%%:*}; \ + schema=$${scenario#*:}; \ + echo "=== TEST_LOAD_SOURCE=$$load_source TEST_SCHEMA=$$schema ==="; \ + $(MAKE) verify-results TEST_LOAD_SOURCE="$$load_source" TEST_SCHEMA="$$schema" || exit 1; \ + done # 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. Kept as a +# standalone target for a quick single-mode run; test-long (below) covers the +# same update axis as part of its full loop, so test-all no longer calls this +# separately. .PHONY: test-update test-update: $(MAKE) test TEST_LOAD_SOURCE=update +# Runs test (fresh; gates via test's own regression.diffs check, pgxntool +# 2.3.0+, but not as strict as verify-results's pgtap-aware check -- still +# useful as a quick smoke build) followed by test-long, which covers the +# TEST_LONG_SCENARIOS above THROUGH verify-results, this repo's stricter, +# documented gate. Sequential $(MAKE) calls in the recipe body, NOT bare +# prerequisites -- listing them as prerequisites would let Make run them +# concurrently under -j, and they all share the same throwaway test database +# (same hazard already called out by verify-results's own dependency-ordering +# comment in pgxntool/base.mk). +.PHONY: test-all +test-all: + $(MAKE) test + $(MAKE) 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 +185,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) From c12d0bd36fb47c38403e5a3ac05fe413b4f7d537 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 13:37:24 -0500 Subject: [PATCH 2/9] CI: fold the PG12+ guard-proved update-to-current check into the `test` job, shrink extension-update-test to PG10-only extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as the `test` job (supported_pg, 12-18), but as its own matrix job: its own runner, container boot, checkout, apt-get, and `make install`, paid again per major, for a check that can run as one more step inside a container the `test` job already has running, already checked out, and already has cat_tools installed on disk in (installcheck, a TEST_DEPS prerequisite, already ran as a side effect of that job's own verify-results/test-long calls). Folded `bin/test_existing update-scenario cat_tools_update 0.2.2` in as an additional call in the `test` job's "Test on PostgreSQL" step instead. Verified before folding it in, not assumed: ran `make check-relkind-source && make verify-results && make test-long && bin/test_existing update-scenario cat_tools_update 0.2.2` in the same shell/cluster session (mirroring the new CI step exactly) against a scratch cluster. Confirmed no database-name collision (pg_regress's own throwaway db is named independently from `cat_tools_update`), the dependency-guard proof fires (twice -- once right after CREATE EXTENSION, once again after the full suite run), the structural-diff check (bin/structural_diff, landed via PR #55 since this branch's last rebase) fires and reports the updated database structurally identical to a fresh install, and the full suite passes -- exit 0 end to end. extension-update-test now runs PG10 only, with no matrix at all (single source of truth: needs.changes.outputs.legacy_pg, not a hardcoded "10") -- its entire remaining purpose is the pre-0.2.2 legacy-script checks, the only place those scripts still load. Removed the now-dead `if: matrix.pg != '10'` / `if: matrix.pg == '10'` guards throughout that job (nothing left to guard against once there's no other leg) and the "Update 0.2.2 -> current" step (moved above). The `changes` job's `update_pg` output/derivation (supported_pg + legacy_pg) is removed too -- it had exactly one consumer, and that consumer is gone. Restructured the top-of-file "Test strategy" comment around what actually matters to a reader: what runs on EVERY supported PostgreSQL major (the `test` job, now including the guard-proved update check, and `pg-tle-test` as a second, independently-isolated main-matrix job -- kept separate deliberately, since it specifically proves pg_tle deployment ISOLATION, not filesystem-install coincidence, unlike the fold above) versus SPECIAL CASES that apply to one specific scenario only (extension-update-test's PG10 legacy scripts, pg-upgrade-test's/pg-tle-upgrade-test's specific binary-pg_upgrade jump pairs, and pg-upgrade-stepwise's full climb). No coverage lost: the PG12+ update-to-current check still runs on the exact same 7 majors it always did (moved, not removed), the PG10 legacy checks are byte-for-byte unchanged, and every other job is untouched. --- .github/workflows/ci.yml | 272 +++++++++++++++++++++------------------ 1 file changed, 149 insertions(+), 123 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb402d3..c32df62 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,16 @@ 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,7 +231,9 @@ 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" @@ -242,68 +241,103 @@ jobs: # =========================================================================== # 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 below: what runs on EVERY supported PostgreSQL major + # (the MAIN MATRIX), and SPECIAL CASES that apply to one specific scenario + # only, not the whole matrix. # - # test -- FRESH install: CREATE EXTENSION at the current - # version on every supported PostgreSQL. Its "Test - # on PostgreSQL" step runs a plain `make - # verify-results` for the fresh/default-schema - # baseline, THEN `make test-long`, which loops the - # full suite once per TEST_LONG_SCENARIOS entry -- - # fresh:CatToolsSchema, update:CatToolsSchema -- - # so every major covers the baseline plus both - # quoting-requiring-schema scenarios, with no - # separate matrix leg for any of them. - # Deliberately NOT a full fresh/update x - # schema-empty/quoting-requiring cross product: - # {fresh, } is the explicit verify-results - # call just described (test-long including it too - # would just re-run the same case again), and - # {update, } is dropped entirely because - # it's exactly what extension-update-test below - # already proves, more thoroughly, on the same - # PostgreSQL majors -- see the TEST_LONG_SCENARIOS - # comment in the Makefile. NOTE: - # extension-update-test and pg-upgrade-test below - # 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. - # 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. + # MAIN MATRIX -- every supported major (12-18, single source: the changes + # job). TWO independently-isolated jobs, not one, because they prove + # genuinely different things: # - # 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). + # test -- Everything reachable through a FILESYSTEM-installed + # cat_tools on this major: + # 1. FRESH install, default schema -- a plain + # `make verify-results`, the baseline a brand-new user + # gets. + # 2. TEST_SCHEMA's quoting-requiring-schema pipeline, on + # both the fresh and update paths -- `make test-long`'s + # TEST_LONG_SCENARIOS (see the Makefile comment for + # exactly which two scenarios, and why not more). + # 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 the current version, structurally + # compare the result against a fresh install, and run + # the full suite against it in existing mode. This USED + # to be its own matrix job (extension-update-test's + # PG12+ leg) -- folded in here because it runs on the + # exact same majors as (1)/(2), and by the time they + # finish this job's container is already running this + # major, already checked out, and already has + # cat_tools installed on disk, so a separate job would + # pay its own runner/container boot, checkout, + # apt-get, and `make install` again for no added + # coverage. See that step's own comment for what was + # verified locally before folding it in. # - # 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). + # pg-tle-test -- The SAME majors, but proving cat_tools works when + # deployed via pg_tle (AWS's Trusted Language Extensions) + # instead of a filesystem .control file -- fresh install, + # dynamic version assertion, and the update path, all + # asserted to leave NO filesystem trace throughout. + # Deliberately NOT folded into `test` above like (3) was: + # this is specifically about proving pg_tle ISOLATION (a + # stale filesystem .control file silently wins over a + # pg_tle registration of the same name -- PostgreSQL does + # not error, it just resolves from disk instead), not + # filesystem-install coincidence, and pg_tle requires + # shared_preload_libraries (mixing pg_tle/non-pg_tle + # extension installs on one cluster can misbehave) -- a + # dedicated cluster/job is the only way either guarantee + # means anything. # - # 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. + # SPECIAL CASES -- apply to one specific scenario, not the whole matrix: + # + # extension-update-test -- PG10 ONLY. The pre-0.2.2 install scripts + # (0.2.0/0.2.1) load on NO other PostgreSQL major + # (PG11 added pg_attribute.attmissingval; PG12+ + # exposes the oid system column in SELECT *; both + # trip up those old scripts), so this job's whole + # 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 as + # origins 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 starting at 0.2.1 is the only + # way to exercise that script. + # pg-upgrade-test -- BINARY pg_upgrade, SPECIFIC old_pg->new_pg jump + # pairs (not every major -- that's + # pg-upgrade-stepwise below): 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. KEY + # invariant: every leg CLIMBS to a PostgreSQL + # that supports the current version, then updates + # to it 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+. + # pg-tle-upgrade-test -- The pg_tle-deployed equivalent of + # pg-upgrade-test, on the subset of 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, + # exercising each individual major-to-major + # transition in turn -- catches a regression + # specific to one particular boundary that a + # big-jump leg (pg-upgrade-test, skipping + # intermediates) would never exercise. + # + # NOTE: extension-update-test, pg-upgrade-test, pg-tle-upgrade-test and + # pg-upgrade-stepwise 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] @@ -348,9 +382,30 @@ jobs: # test-long loops verify-results once per TEST_LONG_SCENARIOS entry # -- fresh:CatToolsSchema, update:CatToolsSchema -- covering exactly # the two scenarios nothing else (not the verify-results call above, - # not extension-update-test below) already covers. See the + # not the update-scenario call below) already covers. See the # Makefile's TEST_LONG_SCENARIOS/test-long comments. make test-long + # The PG12+ guard-proved update-to-current check, folded in here + # rather than run as its own matrix job (which extension-update-test + # used to do, on these same PostgreSQL majors): this job's container + # is already running this PG major, already checked out, and already + # has cat_tools installed on disk (installcheck, one of $(TEST_DEPS), + # ran it as a side effect of every verify-results/test-long call + # above) -- a separate job would pay its own runner/container boot, + # checkout, apt-get, and `make install` 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 the current version, structurally + # compares the result against a fresh install of the current version + # (assert_matches_fresh, via bin/structural_diff), and runs the full + # suite against it 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. 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 @@ -673,59 +728,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 @@ -755,7 +791,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 @@ -765,15 +800,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. From 0341d79c55df68168953f95db4425b48d30a668c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 14:16:52 -0500 Subject: [PATCH 3/9] Replace test-long's scenario-tuple design with named test-* targets A real timing test showed a single `make verify-results` run takes ~1.1s (a tiny suite) -- the scenario-exclusion logic TEST_LONG_SCENARIOS added was solving for "avoid wasting CI time re-running the suite," but re-running it costs about a second, not worth the complexity it bought. The real CI cost is container/cluster bring-up (~40s/job) and pg-upgrade-stepwise, not suite invocations. Removed TEST_LONG_SCENARIOS, its scenario-tuple shell-parsing loop, and every comment describing the old 2/3/4-combination cross-product design. In its place: - `test-schema` (new): `$(MAKE) test TEST_SCHEMA=CatToolsSchema`. - `test-update` (unchanged): stays simple/no-schema, symmetric with `test` -- resolves the "what does test-update do about schema" ambiguity by giving it none. - `test-update-schema` (new): `$(MAKE) test TEST_LOAD_SOURCE=update TEST_SCHEMA=CatToolsSchema` -- the one scenario nothing else covers (also a partial answer to issue #65). - `test-long`: now a thin wrapper, `test-long: test-schema test-update-schema` -- bundles every test-* target too situational for plain test/test-update but not worth its own CI step. As the suite grows, a new such target just gets added to this prerequisite list. - `test-all`: thin wrapper, `test-all: test test-update test-long`. A single `.NOTPARALLEL: test test-schema test-update test-update-schema test-long test-all` covers the whole family, so test-long/test-all can use plain bare prerequisites instead of sequential $(MAKE) calls in each recipe body. Verified this is safe here specifically: grepped ci.yml/Makefile/sql.mk/lint.mk/pgxntool's own .mk files and docs -- nothing in this build ever invokes `-j`. Also verified GNU Make 4.3's actual .NOTPARALLEL behavior empirically (a throwaway two-target Makefile under `make -j4`): listing explicit targets does NOT scope narrowly to just those targets, it forces full serialization of the whole invoked build graph -- so declaring it once for the whole family gives up nothing narrower than what .NOTPARALLEL already does even when scoped "correctly," and there's no real -j parallelism anywhere in this build to lose regardless. Verified locally against a scratch cluster: real output for test-schema, test-update-schema, test-long (2 suite runs), and test-all (4 suite runs) all passing. Then deliberately broke two independent regressions and confirmed each new target actually fails loudly rather than assuming test-update's existing precedent carries over safely: (1) reintroduced the schema-quoting bug in test/install/load.sql (unquoted SET search_path) -- `make test-schema` failed with exit 2, cascading pgTAP failures from the missing schema; (2) commented out a GRANT in sql/cat_tools--0.2.3--0.3.0.sql.in that only the update path depends on (a fresh install grants it directly) -- `make test-update-schema` failed with exit 2, a real permission-denied failure. Reverted both and confirmed clean passes again. This confirms pgxntool 2.3.0's test-exits-nonzero-on-regression behavior is a real gate for these two new targets, not just assumed from test-update's own precedent. ci.yml's `test` job step updated to match: `make verify-results` (baseline, unchanged mechanism) followed by `make test-long` (now bundling test-schema/test-update-schema) followed by the guard-proved update-to-current check (unchanged from last round). Updated the top "Test strategy" summary block's description of what test-long bundles. Left a clarifying comment on issue #65 noting its "Proposed approach" section's test-long example describes the now-replaced scenario-loop shape, though the issue's core ask (extension-update-test/pg-upgrade-test still don't exercise TEST_SCHEMA) is unaffected either way. --- .github/workflows/ci.yml | 47 ++++++------ Makefile | 150 +++++++++++++++++++-------------------- 2 files changed, 99 insertions(+), 98 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c32df62..fb3b094 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -255,9 +255,10 @@ jobs: # `make verify-results`, the baseline a brand-new user # gets. # 2. TEST_SCHEMA's quoting-requiring-schema pipeline, on - # both the fresh and update paths -- `make test-long`'s - # TEST_LONG_SCENARIOS (see the Makefile comment for - # exactly which two scenarios, and why not more). + # both the fresh and update paths -- `make test-long`, + # which bundles the test-schema/test-update-schema + # targets (see the Makefile comment for exactly what + # test-long bundles, and why it's designed to grow). # 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 @@ -364,26 +365,28 @@ jobs: # Fail if the relkind drift source is empty (headers missing): the # drift check must actually run on every version, not pass silently. make check-relkind-source - # The fresh/default-schema baseline: a plain verify-results call, - # NOT `make test-long` -- test-long deliberately excludes this exact - # {fresh, } scenario (see its TEST_LONG_SCENARIOS comment in - # the Makefile), since it exists to cover ONLY what nothing else - # does. This call is what actually exercises the baseline now, so it - # can't be dropped. 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 (pgxntool - # 2.3.0+), 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; verify-results remains the stricter, - # documented check, so it's used here rather than leaning on plain - # `test`'s newer (and less strict) non-zero exit. + # The fresh/default-schema baseline: verify-results, this repo's + # stricter, documented, pgtap-aware 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 (pgxntool 2.3.0+), 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. make verify-results - # test-long loops verify-results once per TEST_LONG_SCENARIOS entry - # -- fresh:CatToolsSchema, update:CatToolsSchema -- covering exactly - # the two scenarios nothing else (not the verify-results call above, - # not the update-scenario call below) already covers. See the - # Makefile's TEST_LONG_SCENARIOS/test-long comments. + # test-long bundles the test-* targets that don't get their own CI + # step: test-schema and test-update-schema (TEST_SCHEMA's + # quoting-requiring pipeline, on the fresh and update paths -- + # nothing else, not the verify-results call above, not the + # update-scenario call below, covers either). Each 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 two, and re-running the + # suite here is cheap regardless (~1s per invocation; the real CI + # cost is container/cluster bring-up, not suite invocations). See + # the Makefile's test-long/test-schema/test-update-schema comments. make test-long # The PG12+ guard-proved update-to-current check, folded in here # rather than run as its own matrix job (which extension-update-test diff --git a/Makefile b/Makefile index fc3e186..df23232 100644 --- a/Makefile +++ b/Makefile @@ -1,15 +1,22 @@ 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-update -- test, but updated from TEST_UPDATE_FROM (default 0.2.2) to -# the current version instead of a fresh install. -# test-long -- ONLY the scenarios nothing else covers (see -# TEST_LONG_SCENARIOS below): TEST_SCHEMA's quoting-requiring -# pipeline on the fresh and update paths. Deliberately -# excludes anything test/test-update/CI's other jobs already -# check. -# test-all -- test + test-long: the full local pre-push gate. +# 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 -- bundles every test-* target too situational for +# plain test/test-update but not worth its own CI +# step. As the suite grows, a new such target just +# gets added to its prerequisite list -- no redesign +# needed. +# test-all -- test + test-update + test-long: the full local +# pre-push gate. # Committed-once install of the extension + test roles. # @@ -92,86 +99,77 @@ 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) -# Schema variant test-long exercises: one mixed-case name that requires SQL -# identifier quoting -- see TEST_SCHEMA above for what this actually proves. -# (The empty/ambient-search_path default is deliberately NOT in test-long's -# scenario list at all -- see TEST_LONG_SCENARIOS below for why.) -# # Scope boundary (deliberate, not an oversight): CI's extension-update-test and # pg-upgrade-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 -# test-long below doesn't reach them either. See +# 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 -- -# this is a local-dev-convenience fix, not a fix for that issue). +# these are local-dev-convenience targets, not a fix for that issue). # -# TEST_LONG_SCENARIOS is an explicit list of "TEST_LOAD_SOURCE:TEST_SCHEMA" -# pairs -- NOT a full cross product of every TEST_LOAD_SOURCE x every -# TEST_SCHEMA. test-long exists to cover ONLY what nothing else already -# covers, so BOTH empty-schema combinations are deliberately absent, each for -# a different reason: -# - {fresh, } is exactly the plain fresh-install/default-schema case -# that `make test`/`installcheck` (and CI's `test` job step, via its own -# explicit `make verify-results` call -- see ci.yml) already checks. -# test-long including it too would just be re-running that same case -# again under a different name. -# - {update, } is exactly "0.2.2 updated to the current version, -# default schema, full suite", already proven -- more thoroughly -- by -# CI's extension-update-test job (bin/test_existing's update_scenario -# additionally plants and proves the dependency guard) on the SAME -# PostgreSQL majors (12-18) that the `test` job (and so test-long) runs on. -# Repeating either here would add CI wall-clock with no added confidence. The -# two scenarios kept are exactly the ones nothing else covers: TEST_SCHEMA's -# quoting-requiring pipeline on the fresh path (fresh:CatToolsSchema) and on -# the update path (update:CatToolsSchema) -- the latter also a partial answer -# to https://github.com/Postgres-Extensions/cat_tools/issues/65, which asks -# for TEST_SCHEMA coverage on the update path. -TEST_LONG_SCENARIOS ?= fresh:CatToolsSchema update:CatToolsSchema - -# Loops the full suite once per TEST_LONG_SCENARIOS entry via `verify-results`, -# NOT plain `test`: verify-results is this repo's documented CI-safe gate -# (make test alone doesn't reliably fail on regressions the way verify-results -# does -- see CLAUDE.md), and CI relies on test-long to fail loudly on a -# regression the same way a single -# `make verify-results TEST_LOAD_SOURCE=X TEST_SCHEMA=Y` already does. Must -# recurse (a fresh $(MAKE) per iteration, not a plain shell variable) for the -# same reason test-update recurses: these GUCs only take effect if exported -# into PGOPTIONS before the sub-make's own parse phase. Each scenario is -# "load_source:schema"; %% / # parameter expansion splits on the FIRST colon -# (also correct if a schema were ever empty, e.g. "fresh:" -> schema ""), so a -# schema name containing a colon would break this, but none of ours do. -.PHONY: test-long -test-long: - @for scenario in $(TEST_LONG_SCENARIOS); do \ - load_source=$${scenario%%:*}; \ - schema=$${scenario#*:}; \ - echo "=== TEST_LOAD_SOURCE=$$load_source TEST_SCHEMA=$$schema ==="; \ - $(MAKE) verify-results TEST_LOAD_SOURCE="$$load_source" TEST_SCHEMA="$$schema" || exit 1; \ - done +# 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. Kept as a -# standalone target for a quick single-mode run; test-long (below) covers the -# same update axis as part of its full loop, so test-all no longer calls this -# separately. +# 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 -# Runs test (fresh; gates via test's own regression.diffs check, pgxntool -# 2.3.0+, but not as strict as verify-results's pgtap-aware check -- still -# useful as a quick smoke build) followed by test-long, which covers the -# TEST_LONG_SCENARIOS above THROUGH verify-results, this repo's stricter, -# documented gate. Sequential $(MAKE) calls in the recipe body, NOT bare -# prerequisites -- listing them as prerequisites would let Make run them -# concurrently under -j, and they all share the same throwaway test database -# (same hazard already called out by verify-results's own dependency-ordering -# comment in pgxntool/base.mk). +# 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 + +# Bundles every test-* target too situational for plain test/test-update but +# not worth its own CI step -- right now that's just the schema-targeting +# variants (test-schema, test-update-schema) above, but as the suite grows a +# new such target just gets added to this prerequisite list, no redesign +# needed. 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-schema test-update-schema + +# Runs every test-* target that matters for a full local pre-push check: test +# (fresh, default schema), test-update (update path, default schema), and +# test-long (everything else -- see its own comment). Bare prerequisites, +# safe under the same .NOTPARALLEL declaration as test-long. .PHONY: test-all -test-all: - $(MAKE) test - $(MAKE) test-long +test-all: test test-update 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, From 9f8d5d8baf9aa99a6e913476951209c0a6ac2d29 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:00:40 -0500 Subject: [PATCH 4/9] Fix test-long: include test-update, not just the schema-targeting variants The previous round incorrectly carried over a CI-cost duplication argument (that the {update, } scenario shouldn't run twice across two separate CI jobs) into test-long's own local composition, where it doesn't apply. test-long/test-all are local dev convenience bundles, not CI cost centers -- that CI-level concern is already fully and correctly handled by the fold-into-test-job/shrink-extension-update-test change from two rounds ago, which is unaffected by this fix. The rule for test-long is simple inclusion: every test-* target except test itself, full stop. test-long now depends on test-update, test-schema, and test-update-schema (previously just the latter two). test-all simplifies to test + test-long (test-update moved out from directly under test-all since it's now reached via test-long). Verified with real output: `make test-long` now runs `make test TEST_LOAD_SOURCE=update`, `make test TEST_SCHEMA=CatToolsSchema`, and `make test TEST_LOAD_SOURCE=update TEST_SCHEMA=CatToolsSchema` (three invocations, three suite passes), and `make test-all` runs all four (adding the plain baseline). Both exit 0. `.NOTPARALLEL` already listed test-update, so no change needed there. --- Makefile | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index df23232..919972b 100644 --- a/Makefile +++ b/Makefile @@ -10,13 +10,11 @@ testdeps: $(wildcard test/*.sql test/helpers/*.sql) # Be careful not to include # test; test-update-schema below is the combination. # test-update-schema -- test-update + test-schema together: the one # scenario nothing else covers. -# test-long -- bundles every test-* target too situational for -# plain test/test-update but not worth its own CI -# step. As the suite grows, a new such target just -# gets added to its prerequisite list -- no redesign -# needed. -# test-all -- test + test-update + test-long: the full local -# pre-push gate. +# 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. # @@ -154,22 +152,25 @@ test-update-schema: # 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 -# Bundles every test-* target too situational for plain test/test-update but -# not worth its own CI step -- right now that's just the schema-targeting -# variants (test-schema, test-update-schema) above, but as the suite grows a -# new such target just gets added to this prerequisite list, no redesign -# needed. 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. +# The rule is simple inclusion, not a CI-cost judgment call: test-long is +# every test-* target EXCEPT test itself, full stop -- currently test-update, +# test-schema, test-update-schema. As new test-* targets get added, they just +# join this prerequisite list, no redesign needed. (The CI-cost concern that +# used to shape which scenarios ran where is handled entirely at the ci.yml +# level -- the guard-proved update-to-current check folded into the `test` +# job, extension-update-test shrunk to PG10-only -- and does NOT apply here: +# test-long/test-all are local dev convenience bundles, not CI cost centers.) +# 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-schema test-update-schema +test-long: test-update test-schema test-update-schema -# Runs every test-* target that matters for a full local pre-push check: test -# (fresh, default schema), test-update (update path, default schema), and -# test-long (everything else -- see its own comment). Bare prerequisites, -# safe under the same .NOTPARALLEL declaration as test-long. +# 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-update test-long +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, From 9f70026129440d3e6c12495718a88b6caf28046a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:07:27 -0500 Subject: [PATCH 5/9] ci.yml: add blank-line separation between comment-delimited sections Many run: script blocks had the next section's comment starting immediately on the line right after the previous command, with no separator -- so each comment+command section wasn't visually demarcated from the next one. Added a blank line after a command whenever a new comment block (documenting the NEXT, sibling command) immediately follows, throughout the whole file (not just this PR's own changes). Left untouched, deliberately: comments that are the first line right after an if/elif/else opener (they document what follows within that new block, not a separate sibling section after a completed one -- the pattern this fix targets), and everything inside heredocs (the verify-cancel-on-close-coupling job's embedded Python script) since altering blank-line structure there touches different-language content, not this file's own comment/command structure. Pure whitespace -- verified the file still parses as valid YAML and make lint still passes. --- .github/workflows/ci.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb3b094..a356a8a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -220,6 +220,7 @@ jobs: # supported = CURRENT_FLOOR..NEWEST (newest-first). The FRESH-install # matrix (test job, and pg-tle-test) runs exactly these. supported=$(seq "$NEWEST" -1 "$CURRENT_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. @@ -231,10 +232,12 @@ jobs: json() { printf '%s\n' "$@" | paste -sd, - | sed 's/^/[/; s/$/]/'; } echo "supported_pg=$(json $supported)" >> "$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" @@ -365,6 +368,7 @@ jobs: # Fail if the relkind drift source is empty (headers missing): the # drift check must actually run on every version, not pass silently. make check-relkind-source + # The fresh/default-schema baseline: verify-results, this repo's # stricter, documented, pgtap-aware gate. base.mk declares # `verify-results: $(TEST_DEPS)`, not `verify-results: test` -- @@ -374,6 +378,7 @@ jobs: # Either way this runs the suite (via installcheck, one of # $(TEST_DEPS)) and then checks the pgtap/regression.diffs. make verify-results + # test-long bundles the test-* targets that don't get their own CI # step: test-schema and test-update-schema (TEST_SCHEMA's # quoting-requiring pipeline, on the fresh and update paths -- @@ -388,6 +393,7 @@ jobs: # cost is container/cluster bring-up, not suite invocations). See # the Makefile's test-long/test-schema/test-update-schema comments. make test-long + # The PG12+ guard-proved update-to-current check, folded in here # rather than run as its own matrix job (which extension-update-test # used to do, on these same PostgreSQL majors): this job's container @@ -659,6 +665,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 @@ -691,11 +698,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 @@ -712,6 +721,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 From 91277a56492f02b4259e9d6b37618d246f48ae41 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:08:04 -0500 Subject: [PATCH 6/9] ci.yml: reword pg-tle-test's Test-strategy summary to lead with the goal, not isolation Isolation (dedicated cluster, filesystem-cleanliness checks) is necessary for pg_tle testing to mean anything, but it isn't the goal -- the goal is proving cat_tools actually works correctly when deployed via pg_tle, across the same fresh-install and update-path scenarios the filesystem matrix already covers. Reworded the summary to lead with that, with isolation explained as the precondition that makes the proof trustworthy rather than the point of the exercise. Comment-only: no job, matrix, or CI-behavior change. Scope stays exactly as it is today (pg-tle-test's fresh install + update path, pg-tle-upgrade-test's 2 jump legs, no pg_tle stepwise job) -- a fuller pg_tle matrix expansion is tracked separately, not described here as upcoming. --- .github/workflows/ci.yml | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a356a8a..e21be1b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -279,21 +279,26 @@ jobs: # coverage. See that step's own comment for what was # verified locally before folding it in. # - # pg-tle-test -- The SAME majors, but proving cat_tools works when - # deployed via pg_tle (AWS's Trusted Language Extensions) - # instead of a filesystem .control file -- fresh install, - # dynamic version assertion, and the update path, all - # asserted to leave NO filesystem trace throughout. + # pg-tle-test -- The SAME majors, proving cat_tools actually works + # correctly when deployed via pg_tle (AWS's Trusted + # Language Extensions) instead of a filesystem .control + # file -- fresh install, dynamic version assertion, and + # the update path, the same scenarios (1)/(3) above prove + # for a filesystem install, run again here through pg_tle's + # own registration mechanism instead. That correctness + # proof only means what it claims to if pg_tle ISOLATION + # actually holds throughout the run (a stale filesystem + # .control file silently wins over a pg_tle registration of + # the same name -- PostgreSQL does not error, it just + # resolves from disk instead), so every step here is + # bracketed by filesystem-cleanliness checks -- but + # isolation is the PRECONDITION that makes this job's + # result trustworthy, not the point of running it. # Deliberately NOT folded into `test` above like (3) was: - # this is specifically about proving pg_tle ISOLATION (a - # stale filesystem .control file silently wins over a - # pg_tle registration of the same name -- PostgreSQL does - # not error, it just resolves from disk instead), not - # filesystem-install coincidence, and pg_tle requires - # shared_preload_libraries (mixing pg_tle/non-pg_tle - # extension installs on one cluster can misbehave) -- a - # dedicated cluster/job is the only way either guarantee - # means anything. + # pg_tle requires shared_preload_libraries (mixing + # pg_tle/non-pg_tle extension installs on one cluster can + # misbehave), so a dedicated cluster/job is the only way to + # run it at all. # # SPECIAL CASES -- apply to one specific scenario, not the whole matrix: # From 9359bb7642f04c639b76042473dfcbec70a4c178 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:17:55 -0500 Subject: [PATCH 7/9] Fix stale comments claiming test-long has zero overlap with the update-scenario check Comment-only, no behavior change. test-update's inclusion in test-long (kept exactly as instructed) means CI's test job -- which calls make test-long directly, on every supported PostgreSQL major -- now also re-proves part of what that same job's guard-proved update-scenario check already proves more thoroughly. The Makefile's test-long comment claimed test-long/test-all are local dev convenience bundles, not CI cost centers, which is false: test-long is invoked directly from CI. ci.yml's step comment similarly claimed test-long covers nothing update-scenario also covers. Both now state plainly that this overlap is real and deliberately accepted: a single make test pass costs about a second, so one more of them per matrix leg is a different class of cost entirely from the separate-job overhead (~55s per leg) that folding update-scenario into the test job was specifically done to avoid two rounds ago. --- .github/workflows/ci.yml | 52 ++++++++++++++++++++++++++-------------- Makefile | 21 ++++++++++++---- 2 files changed, 50 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e21be1b..f0becbe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -257,11 +257,21 @@ jobs: # 1. FRESH install, default schema -- a plain # `make verify-results`, the baseline a brand-new user # gets. - # 2. TEST_SCHEMA's quoting-requiring-schema pipeline, on - # both the fresh and update paths -- `make test-long`, - # which bundles the test-schema/test-update-schema - # targets (see the Makefile comment for exactly what - # test-long bundles, and why it's designed to grow). + # 2. `make test-long` -- every test-* target except + # plain `test` itself (test-update, test-schema, + # test-update-schema; see the Makefile comment for + # the simple-inclusion rule and why it's designed to + # grow). Primarily this is TEST_SCHEMA's + # quoting-requiring-schema pipeline, on both the fresh + # and update paths -- but test-update (no schema + # targeting) rides along too, so this ALSO re-proves + # part of what (3) below already proves more + # thoroughly. That overlap is real and accepted, not + # an oversight: a single `make test` pass costs about + # a second, so one more of them per major is + # negligible next to the runner/container cost (3) + # itself was folded in here to avoid -- see the + # Makefile's test-long comment for the full reasoning. # 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 @@ -384,19 +394,25 @@ jobs: # $(TEST_DEPS)) and then checks the pgtap/regression.diffs. make verify-results - # test-long bundles the test-* targets that don't get their own CI - # step: test-schema and test-update-schema (TEST_SCHEMA's - # quoting-requiring pipeline, on the fresh and update paths -- - # nothing else, not the verify-results call above, not the - # update-scenario call below, covers either). Each 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 two, and re-running the - # suite here is cheap regardless (~1s per invocation; the real CI - # cost is container/cluster bring-up, not suite invocations). See - # the Makefile's test-long/test-schema/test-update-schema comments. + # test-long is every test-* target except plain `test` itself + # (test-update, test-schema, test-update-schema -- see the + # Makefile's test-long comment for the simple-inclusion rule). + # Mainly this covers TEST_SCHEMA's quoting-requiring pipeline on + # the fresh and update paths, which nothing else here does -- but + # test-update rides along too, so this ALSO re-proves part of what + # the update-scenario call below already proves more thoroughly + # (dependency-guard proof + structural diff). That overlap is real + # and deliberately accepted, not an oversight: a single `make test` + # pass costs about a second, so one more of them per major is + # negligible -- see the Makefile comment for why that's a + # different class of cost from the separate-job overhead + # update-scenario itself was folded in here to avoid. Each 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 PG12+ guard-proved update-to-current check, folded in here diff --git a/Makefile b/Makefile index 919972b..b556e3a 100644 --- a/Makefile +++ b/Makefile @@ -155,11 +155,22 @@ test-update-schema: # The rule is simple inclusion, not a CI-cost judgment call: test-long is # every test-* target EXCEPT test itself, full stop -- currently test-update, # test-schema, test-update-schema. As new test-* targets get added, they just -# join this prerequisite list, no redesign needed. (The CI-cost concern that -# used to shape which scenarios ran where is handled entirely at the ci.yml -# level -- the guard-proved update-to-current check folded into the `test` -# job, extension-update-test shrunk to PG10-only -- and does NOT apply here: -# test-long/test-all are local dev convenience bundles, not CI cost centers.) +# join this prerequisite list, no redesign needed. +# +# test-long is NOT purely a local-dev target, though -- CI's `test` job calls +# `make test-long` directly, on every supported PostgreSQL major (see that +# job's step in ci.yml). So test-update's inclusion here means CI now also +# re-proves part of what that same job's guard-proved update-scenario check +# (a few lines later in the same step) already proves more thoroughly +# (dependency-guard proof + structural diff against a fresh install) -- +# real, acknowledged overlap, not an oversight. It's kept anyway: a single +# `make test` pass costs about a second, so one more of them per matrix leg +# (test-long already ran test-schema/test-update-schema either way) is +# negligible -- a wholly different class of cost from the SEPARATE JOB this +# repo removed two rounds ago folding extension-update-test's PG12+ leg into +# this same `test` job, which cost a full extra runner/container/checkout per +# leg (~55s), not one more invocation inside a job already running. +# # 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. From 7439225c8bc4acc6a33c69139ff0267c1ffae5c7 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:30:45 -0500 Subject: [PATCH 8/9] Fix factual error: test-update and update-scenario are distinct checks The prior comment framed test-long's test-update target as "real, accepted overlap" with bin/test_existing's update-scenario check, tolerated because a `make test` pass is cheap. That's wrong: they exercise different code paths entirely. 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). A bug in load.sql's update-mode logic is only caught by test-update; a bug in the guard/structural-diff logic is only caught by update-scenario. Rewrote the Makefile's test-long comment and ci.yml's Test strategy block and `test` job step comment to say so plainly. Also added a note near pg-upgrade-test/pg-upgrade-stepwise: post-upgrade we currently only assert the version landed and run the base suite, not every dimension the main matrix covers (TEST_SCHEMA, or whatever test-long grows to). Tied to https://github.com/Postgres-Extensions/cat_tools/issues/65 (also just broadened there to explicitly cover pg-tle-test, which has the same TEST_SCHEMA gap) rather than opening a new issue. Trimmed wordiness in the comments touched this PR without dropping any of the underlying reasoning. --- .github/workflows/ci.yml | 273 +++++++++++++++++++-------------------- Makefile | 34 ++--- 2 files changed, 148 insertions(+), 159 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f0becbe..186d5c6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -244,117 +244,111 @@ jobs: # =========================================================================== # Test strategy # - # Two kinds of coverage below: what runs on EVERY supported PostgreSQL major - # (the MAIN MATRIX), and SPECIAL CASES that apply to one specific scenario - # only, not the whole matrix. + # 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, not one, because they prove - # genuinely different things: + # 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 -- a plain - # `make verify-results`, the baseline a brand-new user - # gets. - # 2. `make test-long` -- every test-* target except - # plain `test` itself (test-update, test-schema, - # test-update-schema; see the Makefile comment for - # the simple-inclusion rule and why it's designed to - # grow). Primarily this is TEST_SCHEMA's - # quoting-requiring-schema pipeline, on both the fresh - # and update paths -- but test-update (no schema - # targeting) rides along too, so this ALSO re-proves - # part of what (3) below already proves more - # thoroughly. That overlap is real and accepted, not - # an oversight: a single `make test` pass costs about - # a second, so one more of them per major is - # negligible next to the runner/container cost (3) - # itself was folded in here to avoid -- see the - # Makefile's test-long comment for the full reasoning. + # 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 the current version, structurally - # compare the result against a fresh install, and run - # the full suite against it in existing mode. This USED - # to be its own matrix job (extension-update-test's - # PG12+ leg) -- folded in here because it runs on the - # exact same majors as (1)/(2), and by the time they - # finish this job's container is already running this - # major, already checked out, and already has - # cat_tools installed on disk, so a separate job would - # pay its own runner/container boot, checkout, - # apt-get, and `make install` again for no added - # coverage. See that step's own comment for what was - # verified locally before folding it in. + # 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 - # correctly when deployed via pg_tle (AWS's Trusted - # Language Extensions) instead of a filesystem .control - # file -- fresh install, dynamic version assertion, and - # the update path, the same scenarios (1)/(3) above prove - # for a filesystem install, run again here through pg_tle's - # own registration mechanism instead. That correctness - # proof only means what it claims to if pg_tle ISOLATION - # actually holds throughout the run (a stale filesystem - # .control file silently wins over a pg_tle registration of - # the same name -- PostgreSQL does not error, it just - # resolves from disk instead), so every step here is - # bracketed by filesystem-cleanliness checks -- but - # isolation is the PRECONDITION that makes this job's - # result trustworthy, not the point of running it. - # Deliberately NOT folded into `test` above like (3) was: - # pg_tle requires shared_preload_libraries (mixing - # pg_tle/non-pg_tle extension installs on one cluster can - # misbehave), so a dedicated cluster/job is the only way to - # run it at all. + # 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. # - # SPECIAL CASES -- apply to one specific scenario, not the whole matrix: + # SPECIAL CASES -- apply to one scenario, not the whole matrix: # # extension-update-test -- PG10 ONLY. The pre-0.2.2 install scripts - # (0.2.0/0.2.1) load on NO other PostgreSQL major - # (PG11 added pg_attribute.attmissingval; PG12+ - # exposes the oid system column in SELECT *; both - # trip up those old scripts), so this job's whole - # 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 as - # origins 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 starting at 0.2.1 is the only - # way to exercise that script. - # pg-upgrade-test -- BINARY pg_upgrade, SPECIFIC old_pg->new_pg jump - # pairs (not every major -- that's - # pg-upgrade-stepwise below): 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. KEY - # invariant: every leg CLIMBS to a PostgreSQL - # that supports the current version, then updates - # to it 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+. + # (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 subset of 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, - # exercising each individual major-to-major - # transition in turn -- catches a regression - # specific to one particular boundary that a - # big-jump leg (pg-upgrade-test, skipping - # intermediates) would never exercise. + # 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. # - # NOTE: extension-update-test, pg-upgrade-test, pg-tle-upgrade-test and - # pg-upgrade-stepwise do NOT exercise TEST_SCHEMA at all yet -- a - # deliberately deferred follow-up (see + # 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. + # + # 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. # =========================================================================== @@ -381,60 +375,55 @@ 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 # The fresh/default-schema baseline: verify-results, this repo's - # stricter, documented, pgtap-aware 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 (pgxntool 2.3.0+), 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. + # 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` itself - # (test-update, test-schema, test-update-schema -- see the + # 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). - # Mainly this covers TEST_SCHEMA's quoting-requiring pipeline on - # the fresh and update paths, which nothing else here does -- but - # test-update rides along too, so this ALSO re-proves part of what - # the update-scenario call below already proves more thoroughly - # (dependency-guard proof + structural diff). That overlap is real - # and deliberately accepted, not an oversight: a single `make test` - # pass costs about a second, so one more of them per major is - # negligible -- see the Makefile comment for why that's a - # different class of cost from the separate-job overhead - # update-scenario itself was folded in here to avoid. Each 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. + # 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 PG12+ guard-proved update-to-current check, folded in here - # rather than run as its own matrix job (which extension-update-test - # used to do, on these same PostgreSQL majors): this job's container - # is already running this PG major, already checked out, and already - # has cat_tools installed on disk (installcheck, one of $(TEST_DEPS), - # ran it as a side effect of every verify-results/test-long call - # above) -- a separate job would pay its own runner/container boot, - # checkout, apt-get, and `make install` again for no added coverage. + # 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 the current version, structurally - # compares the result against a fresh install of the current version - # (assert_matches_fresh, via bin/structural_diff), and runs the full - # suite against it 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. 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). + # 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 diff --git a/Makefile b/Makefile index b556e3a..2d6f4ca 100644 --- a/Makefile +++ b/Makefile @@ -152,24 +152,24 @@ test-update-schema: # 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, not a CI-cost judgment call: test-long is -# every test-* target EXCEPT test itself, full stop -- currently test-update, -# test-schema, test-update-schema. As new test-* targets get added, they just -# join this prerequisite list, no redesign needed. +# 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 is NOT purely a local-dev target, though -- CI's `test` job calls -# `make test-long` directly, on every supported PostgreSQL major (see that -# job's step in ci.yml). So test-update's inclusion here means CI now also -# re-proves part of what that same job's guard-proved update-scenario check -# (a few lines later in the same step) already proves more thoroughly -# (dependency-guard proof + structural diff against a fresh install) -- -# real, acknowledged overlap, not an oversight. It's kept anyway: a single -# `make test` pass costs about a second, so one more of them per matrix leg -# (test-long already ran test-schema/test-update-schema either way) is -# negligible -- a wholly different class of cost from the SEPARATE JOB this -# repo removed two rounds ago folding extension-update-test's PG12+ leg into -# this same `test` job, which cost a full extra runner/container/checkout per -# leg (~55s), not one more invocation inside a job already running. +# 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 From 9a3c44bdd87ef889d949eaf5ad991287016ae27e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 3 Aug 2026 15:39:31 -0500 Subject: [PATCH 9/9] Makefile: fix stale 2-job list in the TEST_SCHEMA scope-boundary comment ci.yml's own "Test strategy" NOTE already names five jobs that don't exercise TEST_SCHEMA (extension-update-test, pg-upgrade-test, pg-tle-upgrade-test, pg-upgrade-stepwise, pg-tle-test) -- this comment still only named the first two. All five equally drive the extension through bin/test_existing's shell-level flow, bypassing this Makefile's TEST_SCHEMA machinery the same way. --- Makefile | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 2d6f4ca..1cb7cad 100644 --- a/Makefile +++ b/Makefile @@ -97,11 +97,12 @@ 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 and -# pg-upgrade-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 +# 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). #