From f936e2d768c6218a385a1e6436115ee3cc096076 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 30 Jul 2026 14:29:56 -0500 Subject: [PATCH 1/2] Add TEST_SCHEMA, a second independent test-harness switch that targets a schema before installing cat_tools, following the exact same make-var -> PGOPTIONS -> GUC -> psql propagation pattern already established by TEST_LOAD_SOURCE. Empty (the default) leaves search_path untouched, so CREATE EXTENSION cat_tools runs exactly as a brand-new user would type it. A non-empty value creates that schema (quoting it, so mixed-case names work) and SETs search_path to it first. cat_tools' control file pins schema = 'cat_tools' with relocatable = false, so this does not relocate the extension itself -- what it proves is that the propagation pipeline handles a quoting-requiring schema name correctly end to end, and that installing while some other, possibly hostile, schema is ambient doesn't break anything. A DO block in test/install/load.sql makes that a real check rather than a silent no-op: PostgreSQL doesn't error on a search_path entry that was never created, so a quoting bug elsewhere in the pipeline could otherwise go undetected. CI wires this into the `test` job as a dedicated, always-quoting-requiring leg (mixed-case `CatToolsSchema`, on the newest supported major) alongside the existing TEST_SCHEMA-empty legs on every supported major. The job's matrix moves from a bare `pg:` list to a pure `include:` list of {pg, test_schema} objects computed in the `changes` job, because GitHub's matrix `include` merges an object into any existing combination whose keys it matches rather than adding a new one -- pairing test_schema with a pg value already in a bare list would silently replace that version's default run instead of adding a second, separate leg. --- .github/workflows/ci.yml | 60 ++++++++++++++++++++++++++++++++++++---- Makefile | 21 +++++++++++++- test/install/load.sql | 46 ++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f08de6..f5ac3f7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,10 @@ jobs: update_pg: ${{ steps.pg.outputs.update_pg }} climb_pg: ${{ steps.pg.outputs.climb_pg }} legacy_pg: ${{ steps.pg.outputs.legacy_pg }} + # The `test` job's full {pg, test_schema} matrix (see the "Derive ..." + # step for why this is a pure include list rather than a second bare + # `test_schema` list crossed against `pg`). + test_matrix: ${{ steps.pg.outputs.test_matrix }} steps: - name: Check out the repo uses: actions/checkout@v6 @@ -232,6 +236,41 @@ jobs: # Space-separated for direct iteration in the stepwise bash loop. echo "climb_pg=$(echo $climb)" >> "$GITHUB_OUTPUT" + # test_matrix: the `test` job's ACTUAL matrix, one {pg, test_schema} + # object per combination, consumed via a pure `strategy.matrix.include` + # (no separate bare `pg:` list) so this is the ONLY source of truth for + # what that job runs -- see the `test` job for why a pure include list, + # rather than crossing a second `test_schema` list against `pg`, is + # required here: GitHub's matrix `include` MERGES an object into an + # existing combination that already matches all of the object's keys + # (rather than adding a new one), so pairing a `test_schema` value with + # a `pg` value already in a bare list would silently REPLACE that + # version's default (TEST_SCHEMA empty) run instead of adding a second, + # separate one. A pure include list sidesteps that entirely: every + # object here is its own job, unconditionally. + # + # Every supported major runs once with TEST_SCHEMA empty (the default -- + # CREATE EXTENSION cat_tools with no schema targeting). Exactly one + # extra leg, on the newest major, additionally sets TEST_SCHEMA to a + # mixed-case name that REQUIRES SQL identifier quoting -- proving the + # make -> PGOPTIONS -> GUC -> psql quoted-identifier pipeline handles + # such a name correctly end to end (see test/install/load.sql for what + # TEST_SCHEMA does and does NOT do). Only one leg needs it: the quoting + # pipeline doesn't depend on the PostgreSQL major at all, so crossing it + # against every supported version would add CI cost with no added + # confidence. + QUOTING_SCHEMA='CatToolsSchema' + test_matrix=$( + { + for pg in $supported; do + jq -n --arg pg "$pg" '{pg: ($pg | tonumber), test_schema: ""}' + done + jq -n --arg pg "$NEWEST" --arg test_schema "$QUOTING_SCHEMA" \ + '{pg: ($pg | tonumber), test_schema: $test_schema}' + } | jq -s -c . + ) + echo "test_matrix=$test_matrix" >> "$GITHUB_OUTPUT" + # =========================================================================== # Test strategy # @@ -240,8 +279,11 @@ 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, with + # TEST_SCHEMA empty (the baseline a brand-new user + # gets), PLUS one extra leg, on the newest major, + # with TEST_SCHEMA set to a quoting-requiring + # schema name. # extension-update-test -- IN-PLACE update: CREATE EXTENSION at an OLD version # then ALTER EXTENSION UPDATE (same PostgreSQL, no # pg_upgrade). @@ -281,9 +323,13 @@ jobs: if: needs.changes.outputs.docs_only != 'true' strategy: matrix: - # Current-supported majors, from the single source in the changes job. - pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} - name: 🐘 PostgreSQL ${{ matrix.pg }} + # {pg, test_schema} pairs from the single source in the changes job. + # A pure include list (no separate bare `pg:` list) -- see the + # "Derive ..." step's test_matrix comment for why. + include: ${{ fromJSON(needs.changes.outputs.test_matrix) }} + # Append the TEST_SCHEMA value to the check name only on the one leg that + # sets it, so the other legs' names are unchanged. + name: 🐘 PostgreSQL ${{ matrix.pg }}${{ matrix.test_schema != '' && format(' (TEST_SCHEMA={0})', matrix.test_schema) || '' }} runs-on: ubuntu-latest container: pgxn/pgxn-tools steps: @@ -306,7 +352,9 @@ jobs: # checks the pgtap/regression.diffs. A bare `make test` is redundant here # and would not gate anyway -- it never exits non-zero on regressions # (pgxntool marks installcheck `.IGNORE`), so failures would pass silently. - make verify-results + # TEST_SCHEMA is empty for every leg except the one dedicated + # quoting-requiring-schema leg (see the changes job's test_matrix). + make verify-results TEST_SCHEMA="${{ matrix.test_schema }}" # 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 b293fc1..21e2de5 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,26 @@ 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: +# - empty (default): load.sql does not create or target any schema at all -- +# CREATE EXTENSION cat_tools runs exactly as a brand-new user would type +# it, 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 it +# before installing. cat_tools' control file pins schema = 'cat_tools' with +# relocatable = false, so this does NOT relocate the extension itself +# (CREATE EXTENSION ignores search_path for a non-relocatable extension); +# what it DOES prove is that the make -> PGOPTIONS -> GUC -> psql +# quoted-identifier pipeline handles a quoting-requiring name correctly, +# and that installing while some other, possibly hostile, schema is +# ambient doesn't break anything. See test/install/load.sql. +# +# 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) # 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 diff --git a/test/install/load.sql b/test/install/load.sql index f0fd25a..7e32fff 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -48,6 +48,52 @@ 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. + * + * Empty (the default): do nothing -- no CREATE SCHEMA, no SET search_path. + * CREATE EXTENSION cat_tools below then runs exactly as a brand-new user + * would type it, landing wherever the session's ambient search_path already + * resolves. + * + * Non-empty: create that schema and SET search_path to it before installing. + * cat_tools' control file pins schema = 'cat_tools' with relocatable = false, + * so CREATE EXTENSION ignores search_path for ITS OWN placement -- this does + * not relocate the extension. What it does prove: the make -> PGOPTIONS -> + * GUC -> psql quoted-identifier pipeline handles a quoting-requiring schema + * name (e.g. mixed case) correctly end to end, and that installing while some + * other, possibly hostile, schema is ambient on search_path doesn't break + * anything. The DO block below is a real check of that pipeline: a quoting + * bug (e.g. an unquoted reference silently case-folding) would leave + * search_path pointing at a schema that was never created -- CREATE SCHEMA + * would create one name, SET search_path would reference another, and + * PostgreSQL does not error on a search_path entry that doesn't exist, so + * nothing downstream would fail loudly without this explicit check. + */ +SELECT current_setting('cat_tools.test_schema') AS cat_tools_test_schema \gset +SELECT :'cat_tools_test_schema' <> '' AS cat_tools_has_schema \gset + +\if :cat_tools_has_schema +CREATE SCHEMA IF NOT EXISTS :"cat_tools_test_schema"; +SET search_path = :"cat_tools_test_schema"; + +DO $DO$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_namespace WHERE nspname = current_setting('cat_tools.test_schema') + ) THEN + RAISE EXCEPTION + 'TEST_SCHEMA target schema % was not created as named -- quoting bug?' + , 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 From d6bdf90223025a7b531a1b7f95bed00eb45d9c78 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 1 Aug 2026 17:52:36 -0500 Subject: [PATCH 2/2] Move TEST_SCHEMA coverage from a dedicated CI matrix leg to a make-level loop, so every supported PostgreSQL major exercises both schema variants instead of only the newest. Per discussion with the maintainer: the `test` job's matrix reverts to a plain pg list (no more crossing test_schema against pg via a computed include list -- that mechanism, and the reasoning for why a bare cross would have collided with GitHub's matrix include-merge semantics, is removed along with it). Instead, a new TEST_SCHEMAS make variable (default: "" and a mixed-case quoting-requiring name) drives a new test-long target that loops verify-results once per schema value, and CI's test job now runs `make test-long` on every major. A test-all target sequences test, test-update and test-long for a full local pre-push check. extension-update-test and pg-upgrade-test still do not exercise TEST_SCHEMA -- called out explicitly in both the Makefile and the ci.yml "Test strategy" comment as a deliberately deferred follow-up, not an oversight. --- .github/workflows/ci.yml | 80 +++++++++++----------------------------- Makefile | 39 ++++++++++++++++++++ 2 files changed, 60 insertions(+), 59 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5ac3f7..ec95fa3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,10 +39,6 @@ jobs: update_pg: ${{ steps.pg.outputs.update_pg }} climb_pg: ${{ steps.pg.outputs.climb_pg }} legacy_pg: ${{ steps.pg.outputs.legacy_pg }} - # The `test` job's full {pg, test_schema} matrix (see the "Derive ..." - # step for why this is a pure include list rather than a second bare - # `test_schema` list crossed against `pg`). - test_matrix: ${{ steps.pg.outputs.test_matrix }} steps: - name: Check out the repo uses: actions/checkout@v6 @@ -236,41 +232,6 @@ jobs: # Space-separated for direct iteration in the stepwise bash loop. echo "climb_pg=$(echo $climb)" >> "$GITHUB_OUTPUT" - # test_matrix: the `test` job's ACTUAL matrix, one {pg, test_schema} - # object per combination, consumed via a pure `strategy.matrix.include` - # (no separate bare `pg:` list) so this is the ONLY source of truth for - # what that job runs -- see the `test` job for why a pure include list, - # rather than crossing a second `test_schema` list against `pg`, is - # required here: GitHub's matrix `include` MERGES an object into an - # existing combination that already matches all of the object's keys - # (rather than adding a new one), so pairing a `test_schema` value with - # a `pg` value already in a bare list would silently REPLACE that - # version's default (TEST_SCHEMA empty) run instead of adding a second, - # separate one. A pure include list sidesteps that entirely: every - # object here is its own job, unconditionally. - # - # Every supported major runs once with TEST_SCHEMA empty (the default -- - # CREATE EXTENSION cat_tools with no schema targeting). Exactly one - # extra leg, on the newest major, additionally sets TEST_SCHEMA to a - # mixed-case name that REQUIRES SQL identifier quoting -- proving the - # make -> PGOPTIONS -> GUC -> psql quoted-identifier pipeline handles - # such a name correctly end to end (see test/install/load.sql for what - # TEST_SCHEMA does and does NOT do). Only one leg needs it: the quoting - # pipeline doesn't depend on the PostgreSQL major at all, so crossing it - # against every supported version would add CI cost with no added - # confidence. - QUOTING_SCHEMA='CatToolsSchema' - test_matrix=$( - { - for pg in $supported; do - jq -n --arg pg "$pg" '{pg: ($pg | tonumber), test_schema: ""}' - done - jq -n --arg pg "$NEWEST" --arg test_schema "$QUOTING_SCHEMA" \ - '{pg: ($pg | tonumber), test_schema: $test_schema}' - } | jq -s -c . - ) - echo "test_matrix=$test_matrix" >> "$GITHUB_OUTPUT" - # =========================================================================== # Test strategy # @@ -279,11 +240,18 @@ jobs: # carry the details; this is the big picture): # # test -- FRESH install: CREATE EXTENSION at the current - # version on every supported PostgreSQL, with - # TEST_SCHEMA empty (the baseline a brand-new user - # gets), PLUS one extra leg, on the newest major, - # with TEST_SCHEMA set to a quoting-requiring - # schema name. + # version on every supported PostgreSQL. Its "Test + # on PostgreSQL" step runs `make test-long`, which + # loops the full suite once per TEST_SCHEMA value + # in TEST_SCHEMAS (empty, the baseline a brand-new + # user gets, and a quoting-requiring schema name) + # -- so every major covers both schema variants, + # with no separate matrix leg for it. See the + # TEST_SCHEMA/TEST_SCHEMAS comments 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, not an oversight. # extension-update-test -- IN-PLACE update: CREATE EXTENSION at an OLD version # then ALTER EXTENSION UPDATE (same PostgreSQL, no # pg_upgrade). @@ -323,13 +291,9 @@ jobs: if: needs.changes.outputs.docs_only != 'true' strategy: matrix: - # {pg, test_schema} pairs from the single source in the changes job. - # A pure include list (no separate bare `pg:` list) -- see the - # "Derive ..." step's test_matrix comment for why. - include: ${{ fromJSON(needs.changes.outputs.test_matrix) }} - # Append the TEST_SCHEMA value to the check name only on the one leg that - # sets it, so the other legs' names are unchanged. - name: 🐘 PostgreSQL ${{ matrix.pg }}${{ matrix.test_schema != '' && format(' (TEST_SCHEMA={0})', matrix.test_schema) || '' }} + # Current-supported majors, from the single source in the changes job. + pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} + name: 🐘 PostgreSQL ${{ matrix.pg }} runs-on: ubuntu-latest container: pgxn/pgxn-tools steps: @@ -347,14 +311,12 @@ 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`, so this runs the suite via its `test` prerequisite and then - # checks the pgtap/regression.diffs. A bare `make test` is redundant here - # and would not gate anyway -- it never exits non-zero on regressions - # (pgxntool marks installcheck `.IGNORE`), so failures would pass silently. - # TEST_SCHEMA is empty for every leg except the one dedicated - # quoting-requiring-schema leg (see the changes job's test_matrix). - make verify-results TEST_SCHEMA="${{ matrix.test_schema }}" + # test-long loops verify-results (this repo's documented CI-safe + # gate -- see CLAUDE.md) once per TEST_SCHEMA value in TEST_SCHEMAS, + # so this one step covers both the TEST_SCHEMA-empty baseline and the + # quoting-requiring-schema variant on EVERY supported major. See the + # Makefile's TEST_SCHEMA/TEST_SCHEMAS/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 21e2de5..5d68785 100644 --- a/Makefile +++ b/Makefile @@ -77,6 +77,33 @@ 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 variants test-long exercises on every PG major: empty (the +# ambient-search_path default) and one mixed-case name that requires SQL +# identifier quoting (proving the make -> PGOPTIONS -> GUC -> psql pipeline +# handles a quoting-requiring name end to end -- see TEST_SCHEMA above). CI's +# `test` job runs test-long on every supported major, so both variants are +# covered everywhere -- no separate matrix leg for it. +# +# Scope boundary (deliberate, not an oversight): extension-update-test and +# pg-upgrade-test do NOT exercise TEST_SCHEMA/TEST_SCHEMAS at all yet. Wiring +# TEST_SCHEMA through the update/upgrade paths is left as a follow-up. +TEST_SCHEMAS ?= "" CatToolsSchema + +# Loops the full suite once per TEST_SCHEMA value 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 -- see CLAUDE.md), +# and CI relies on test-long to fail loudly on a schema-specific regression +# the same way a single `make verify-results TEST_SCHEMA=X` already does. +# Must recurse (a fresh $(MAKE) per iteration, not a plain shell variable) +# for the same reason test-update recurses: TEST_SCHEMA only takes effect +# if it's exported into PGOPTIONS before the sub-make's own parse phase. +.PHONY: test-long +test-long: + @for schema in $(TEST_SCHEMAS); do \ + echo "=== TEST_SCHEMA=$$schema ==="; \ + $(MAKE) verify-results 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. @@ -84,6 +111,18 @@ export PGOPTIONS := $(PGOPTIONS) -c cat_tools.test_load_mode=$(TEST_LOAD_SOURCE) test-update: $(MAKE) test TEST_LOAD_SOURCE=update +# Runs every test target in sequence: fresh, update, and the schema +# variants. 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-update + $(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