From b3101f2eb29562e05ea595413ecbd65297f34648 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 18:26:17 -0500 Subject: [PATCH 1/8] Always install count_nulls into a fresh, randomly named schema for tests Replace the two-value TEST_SCHEMA axis (fixed "" and "Quoted" legs, run via make test-schema-all's in-Makefile loop) with a single install per run into a freshly, randomly generated schema whose constant prefix (a literal trailing space) always requires SQL identifier quoting. This exercises %I- qualification on every run instead of only on a dedicated quoting leg, and removes the Makefile/GUC-propagation infrastructure that existed solely to support the two-value axis. Cleanup-before-create matches on the constant prefix to find and drop any schema left behind by a run that crashed before its own teardown, so stale schemas don't accumulate. test/helpers/find_test_schema.sql lets separate sessions (bin/test_existing's per-step psql invocations) rediscover the randomly generated name. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 60 +++++------- Makefile | 64 +------------ bin/test_existing | 49 +++++----- bin/test_existing.sql/create_extension.sql | 28 ++++++ bin/test_existing.sql/plant_guard.sql | 22 ++--- test/README.md | 82 ++++++++-------- test/helpers/find_test_schema.sql | 26 ++++++ test/install/load.sql | 104 +++++++++++---------- test/sql/extension_tests.sql | 72 ++++++-------- 9 files changed, 234 insertions(+), 273 deletions(-) create mode 100644 bin/test_existing.sql/create_extension.sql create mode 100644 test/helpers/find_test_schema.sql diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ef7d11a..9dee907 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,15 +35,12 @@ # through AWS pg_tle's database-backed catalog # instead of a filesystem .control file. # -# Every TEST_SCHEMA value (empty - no schema targeting at all - and -# 'Quoted', a name requiring SQL identifier quoting) is exercised too, via -# `make test-schema-all`'s in-Makefile loop rather than a CI matrix -# dimension - a schema name is just an input the same assertions run -# against, not a real environment difference, so crossing it into the -# matrix would only multiply job count for no added confidence (see the -# Makefile's TEST_SCHEMA_VALUES comment). Every leg passes against the SAME -# test/expected/extension_tests.out (see test/README.md for how the suite -# keeps its output schema-invariant). +# count_nulls always installs into its own freshly, randomly generated +# schema (a name that always requires SQL identifier quoting - see +# test/install/load.sql), so every run exercises the suite's %I +# schema-qualification, not just its literal test data. test/expected/ +# extension_tests.out stays schema-invariant regardless of the actual +# generated name (see test/README.md for how). # # `changes` is a cheap gate that lets the heavy jobs above skip themselves on # doc-only pushes, and also derives the shared PostgreSQL-major list those @@ -251,16 +248,10 @@ jobs: # UPDATEs to current (no pg_upgrade, same PostgreSQL) and reruns the # suite - a single job rather than a dedicated one, since a load mode is # just an input the same assertions run against, not a real environment - # difference (same reasoning as TEST_SCHEMA below), and the per-version - # container/checkout setup would otherwise be duplicated across two jobs - # with the same PG matrix. Every TEST_SCHEMA value (empty - no schema - # targeting at all - and 'Quoted', a name requiring SQL identifier - # quoting) is exercised too, via `make test-schema-all`'s in-Makefile - # loop rather than a CI matrix dimension - a schema name is just an - # input the same assertions run against, not a real environment - # difference, so crossing it into the matrix would only multiply job - # count for no added confidence (see the Makefile's TEST_SCHEMA_VALUES - # comment). Every leg passes against the SAME + # difference, and the per-version container/checkout setup would + # otherwise be duplicated across two jobs with the same PG matrix. Each + # install (both legs) always targets its own freshly, randomly generated + # schema (see test/install/load.sql), so both legs pass against the SAME # test/expected/extension_tests.out (see test/README.md for how the # suite keeps its output schema-invariant). test: @@ -284,8 +275,8 @@ jobs: run: pg-start ${{ matrix.pg }} - name: Check out the repo uses: actions/checkout@v4 - - name: Test on PostgreSQL ${{ matrix.pg }}, across every TEST_SCHEMA value - run: make test-schema-all + - name: Test on PostgreSQL ${{ matrix.pg }} + run: make test - name: Install count_nulls run: make install - name: Update 0.9.6 -> current and run the suite @@ -300,10 +291,8 @@ jobs: # A single old_pg/new_pg leg (old_pg from the changes job's floor_pg # output, new_pg from its newest_pg output - see that job's comment; NOT # a second/third hardcoded copy of either number), which via TWIN - # databases in the SAME cluster, migrated by a SINGLE pg_upgrade call - # (the same trick this file uses to cross TEST_SCHEMA without doubling - # matrix legs elsewhere), compares BOTH update-vs-pg_upgrade orderings a - # real user could hit: + # databases in the SAME cluster, migrated by a SINGLE pg_upgrade call, + # compares BOTH update-vs-pg_upgrade orderings a real user could hit: # upgrade_oldest_first -- stays at 0.9.6 through the # binary pg_upgrade, updated to current AFTER it (on the NEW # cluster). Proves pg_upgrade itself correctly preserves objects @@ -341,9 +330,6 @@ jobs: # functions over anyarray/json/jsonb, nothing version-sensitive to # break at a specific boundary. Revisit if count_nulls ever grows # something catalog-touching. - # - # Not yet crossed with TEST_SCHEMA (a later phase adds that, once it can - # do so for both this job and the test job's update leg together). pg-upgrade-test: needs: [changes] # Skipped outright (not just matrix-reduced like `test`) on a draft PR: @@ -391,8 +377,8 @@ jobs: # anywhere in this job cannot silently make the eventual # existing-mode run test a fresh install instead. run: | - bin/test_existing prepare-old upgrade_oldest_first "" 0.9.6 - bin/test_existing prepare-old upgrade_current_first "" 0.9.6 + bin/test_existing prepare-old upgrade_oldest_first 0.9.6 + bin/test_existing prepare-old upgrade_current_first 0.9.6 - name: Update to current before pg_upgrade (proves pg_upgrade preserves CURRENT objects) # Updates ONLY upgrade_current_first # (upgrade_oldest_first must NOT be touched here - it @@ -455,8 +441,8 @@ jobs: # test a fresh install instead of the migrated objects. Once per # database, since each holds an independent ordering's result. run: | - bin/test_existing run-suite upgrade_oldest_first "" - bin/test_existing run-suite upgrade_current_first "" + bin/test_existing run-suite upgrade_oldest_first + bin/test_existing run-suite upgrade_current_first pg-tle-test: needs: [changes] @@ -585,11 +571,11 @@ jobs: # A single stable check name for use as a required status check in branch # protection rules. Matrix jobs produce check names like - # "🐘 PostgreSQL 14 (schema none)" which would all need to be listed - # individually and updated whenever the matrix changes. This job passes if - # all others passed or were skipped (e.g. the heavy jobs gated off by the - # `changes` job on a docs-only push), and fails if any failed or were - # cancelled. + # "🐘 PostgreSQL 14" (one per supported major) which would all need to be + # listed individually and updated whenever the matrix changes. This job + # passes if all others passed or were skipped (e.g. the heavy jobs gated + # off by the `changes` job on a docs-only push), and fails if any failed + # or were cancelled. all-checks-passed: needs: [changes, lint, test, pg-upgrade-test, pg-tle-test] if: always() diff --git a/Makefile b/Makefile index 1199bb6..eaf793c 100644 --- a/Makefile +++ b/Makefile @@ -12,64 +12,6 @@ testdeps: $(wildcard test/*/*.sql) $(wildcard test/*.sql) # Be careful not to in LINT_TARGETS = sql/count_nulls.sql test/ include lint.mk -# TEST_SCHEMA selects which schema test/install/load.sql installs count_nulls -# into, for the WHOLE test run (every test file sees the SAME schema in a -# given run). -# -# Empty (the default): don't target any schema at all - count_nulls installs -# wherever the session's own default search_path already resolves. Non-empty: -# explicitly CREATE SCHEMA, then CREATE EXTENSION ... WITH SCHEMA that name - -# including a name that requires SQL identifier quoting (mixed case - -# unquoted would fold to lowercase), to exercise the suite's %I -# schema-qualification rather than just its literal test data. Locally: -# `make test TEST_SCHEMA=Quoted`. -# -# Installing into two schemas only proves that %I-qualification works if the -# test session's search_path never includes count_nulls' own schema in at -# least one of those legs - otherwise an extension full of unqualified, -# resolve-by-accident references would pass every leg too (see -# test/core/functions.sql's header and test__check_ncs in -# test/sql/extension_tests.sql, which is what actually checks this). This -# suite excludes it in BOTH legs, which is stronger than the minimum needed - -# not a requirement in itself. -# -# Propagated as a GUC (count_nulls.test_schema), exported unconditionally via -# PGOPTIONS - pg_regress doesn't forward make variables, but the psql -# processes it spawns inherit the environment. Empty is a valid, deliberate -# value (not an error) - read without missing_ok, so a truly unpropagated GUC -# still fails loudly instead of looking identical to a deliberately empty one. -TEST_SCHEMA ?= -export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_schema=$(TEST_SCHEMA) - -# Every TEST_SCHEMA value the suite is tested against. A single source so -# test-schema-all/test-update-schema-all and CI can't silently drift onto -# different sets. See the TEST_SCHEMA comment above for why exercising more -# than one value here is meaningful (search_path exclusion), not just -# "install into schema A vs schema B". -TEST_SCHEMA_VALUES = "" Quoted - -# TEST_SCHEMA is deliberately NOT a CI matrix dimension: unlike PostgreSQL -# major (a real environment difference - different binaries, different -# container) or pg_tle deployment (a real isolation boundary - must never -# share a runner with a filesystem install), a schema name is just an input -# value the SAME assertions run against in the SAME environment. Crossing it -# into the matrix would only multiply job count (container boot + checkout -# per leg) for zero additional confidence per dollar. Loop it inside make -# instead - the same pattern test-update already uses for the load-mode -# axis, generalized to a list via a shell loop. Sequential recursive $(MAKE) -# calls, deliberately NOT bare prerequisites (which Make can run -# concurrently under -j and would collide on the same throwaway test -# database). `exit 1` on the first failure so a later iteration can't hide -# an earlier one; each iteration is echoed so a failure's TEST_SCHEMA value -# is still directly attributable in the log even without a separate CI -# check name per value. -.PHONY: test-schema-all -test-schema-all: - @for schema in $(TEST_SCHEMA_VALUES); do \ - echo "=== TEST_SCHEMA=$$schema ==="; \ - $(MAKE) test TEST_SCHEMA="$$schema" || exit 1; \ - done - # TEST_LOAD_SOURCE selects how test/install/load.sql installs count_nulls # for the WHOLE test run: # - fresh (default): CREATE EXTENSION count_nulls (current version). @@ -87,8 +29,10 @@ test-schema-all: # "update" (this) is extension-level (ALTER EXTENSION UPDATE); "upgrade" is # cluster-level (pg_upgrade) - 'existing' is how that axis is exercised. # -# Propagated the same way as TEST_SCHEMA: via the count_nulls.test_load_mode -# GUC, exported unconditionally through PGOPTIONS, read without missing_ok. +# Propagated as a GUC (count_nulls.test_load_mode), exported unconditionally +# through PGOPTIONS - pg_regress doesn't forward make variables, but the +# psql processes it spawns inherit the environment. Read without missing_ok: +# a genuinely unpropagated GUC must fail loudly, not look like a valid value. TEST_LOAD_SOURCE ?= fresh ifeq ($(filter $(TEST_LOAD_SOURCE),fresh update existing),) $(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)') diff --git a/bin/test_existing b/bin/test_existing index b43c987..db1fd5e 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -32,14 +32,15 @@ # # USAGE: bin/test_existing [args] # -# prepare-old DB SCHEMA INSTALL_VERSION +# prepare-old DB INSTALL_VERSION # Old-cluster prep for pg-upgrade-test: create DB + extension at -# INSTALL_VERSION in SCHEMA, then plant + prove the dependency guard. +# INSTALL_VERSION (in a freshly, randomly generated schema - see +# test/install/load.sql), then plant + prove the dependency guard. # # update DB [TO_VERSION] # ALTER EXTENSION count_nulls UPDATE [TO 'TO_VERSION'] (empty => current). # -# run-suite DB SCHEMA +# run-suite DB # Assert the current version, re-prove the guard, drop it, then run the # suite in existing mode (extension must be at the current version). # @@ -101,8 +102,8 @@ installed_version() { # Plant the guard and PROVE it blocks a non-CASCADE drop. Call right after # CREATE EXTENSION (and before any update/upgrade) so it persists through them. plant_guard() { - local db=$1 schema=$2 - psql -d "$db" -v ON_ERROR_STOP=1 -v schema="$schema" -f bin/test_existing.sql/plant_guard.sql + local db=$1 + psql -d "$db" -v ON_ERROR_STOP=1 -f bin/test_existing.sql/plant_guard.sql assert_drop_blocked "$db" } @@ -140,35 +141,31 @@ update_ext() { psql_do "$db" -c "ALTER EXTENSION count_nulls UPDATE $to" } -# CREATE EXTENSION count_nulls at VERSION, targeting SCHEMA - unless SCHEMA -# is empty, in which case it's created untouched, wherever the session's -# own default search_path resolves (ordinarily 'public'). A quoted empty -# identifier ("") is a real Postgres syntax error, so this can't just always -# emit `CREATE SCHEMA IF NOT EXISTS "$schema"` - the empty case has to skip -# that entirely, mirroring test/install/load.sql's own :count_nulls_has_schema -# branch. +# CREATE EXTENSION count_nulls at VERSION, into a freshly, randomly +# generated schema - the same cleanup-before-create + random-name pattern +# test/install/load.sql uses for its own installs (see that file for the +# full rationale). Needs multiple statements including a \gset, so this +# runs as a proper -f script file rather than being inlined into -c, same +# convention plant_guard()/drop_guard() already use. create_extension_in_schema() { - local db=$1 schema=$2 version=$3 sql="" - if [ -n "$schema" ]; then - sql="CREATE SCHEMA IF NOT EXISTS \"$schema\"; SET search_path = \"$schema\"; " - fi - psql_do "$db" -c "${sql}CREATE EXTENSION count_nulls VERSION '$version'" + local db=$1 version=$2 + psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" -f bin/test_existing.sql/create_extension.sql } # --------------------------------------------------------------------------- # Subcommand implementations # --------------------------------------------------------------------------- -# prepare-old DB SCHEMA INSTALL_VERSION +# prepare-old DB INSTALL_VERSION # Old-cluster preparation for pg-upgrade-test: create the database and the -# extension at INSTALL_VERSION in SCHEMA, then plant + prove the guard. No +# extension at INSTALL_VERSION, then plant + prove the guard. No # bridge-update step first: count_nulls ships no SELECT-*-over-catalog # views, so it has no known pg_upgrade-unsafe old version to bridge past. prepare_old() { - local db=$1 schema=$2 install=$3 + local db=$1 install=$2 createdb "$db" - create_extension_in_schema "$db" "$schema" "$install" - plant_guard "$db" "$schema" + create_extension_in_schema "$db" "$install" + plant_guard "$db" } # Run the pgTAP suite against an already-populated database in existing mode. @@ -179,7 +176,7 @@ prepare_old() { # then runs the suite via --use-existing so pg_regress does NOT drop/recreate # the database. run_suite() { - local db=$1 schema=$2 + local db=$1 assert_version "$db" current assert_drop_blocked "$db" drop_guard "$db" @@ -188,14 +185,14 @@ run_suite() { # verify-results`) is a real gate as of pgxntool 2.3.0 - it now exits # non-zero on regression failures instead of always exiting 0 regardless # of pg_regress's result (see this repo's pgxntool 2.3.0 bump). - make test TEST_LOAD_SOURCE=existing TEST_SCHEMA="$schema" CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing + make test TEST_LOAD_SOURCE=existing CONTRIB_TESTDB="$db" EXTRA_REGRESS_OPTS=--use-existing } usage() { echo "usage: bin/test_existing [args]" >&2 - echo " prepare-old DB SCHEMA INSTALL_VERSION" >&2 + echo " prepare-old DB INSTALL_VERSION" >&2 echo " update DB [TO_VERSION]" >&2 - echo " run-suite DB SCHEMA" >&2 + echo " run-suite DB" >&2 exit 2 } diff --git a/bin/test_existing.sql/create_extension.sql b/bin/test_existing.sql/create_extension.sql new file mode 100644 index 0000000..bc61b8c --- /dev/null +++ b/bin/test_existing.sql/create_extension.sql @@ -0,0 +1,28 @@ +/* + * Generates a fresh, randomly-named schema and installs count_nulls at + * :version into it, for prepare_old()'s old-cluster install - the same + * cleanup-before-create + random-name pattern test/install/load.sql uses + * for its own installs (see that file for the full rationale). Later, + * separate invocations (e.g. bin/test_existing's other steps, each a + * fresh psql session with no memory of this one's \gset variables) + * rediscover the name this creates via test/helpers/find_test_schema.sql. + * + * Usage: psql -v ON_ERROR_STOP=1 -v version= -f create_extension.sql + */ +\set ON_ERROR_STOP on + +DO $$ +DECLARE + r record; +BEGIN + FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP + EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); + END LOOP; +END +$$; + +SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema +\gset + +CREATE SCHEMA :"schema"; +CREATE EXTENSION count_nulls WITH SCHEMA :"schema" VERSION :'version'; diff --git a/bin/test_existing.sql/plant_guard.sql b/bin/test_existing.sql/plant_guard.sql index 1c3e84d..ef27e86 100644 --- a/bin/test_existing.sql/plant_guard.sql +++ b/bin/test_existing.sql/plant_guard.sql @@ -8,23 +8,17 @@ * otherwise fall through to a silent fresh reinstall and the job would * still report green). * - * Usage: psql -v ON_ERROR_STOP=1 -v schema= -f plant_guard.sql - * (empty schema means "wherever null_count already resolves unqualified" - - * i.e. count_nulls was installed without targeting a schema). + * count_nulls always installs into its own randomly generated schema (see + * test/install/load.sql) - this session didn't create it, so it has no + * other way to know its name; test/helpers/find_test_schema.sql discovers + * it live via pg_namespace. + * + * Usage: psql -v ON_ERROR_STOP=1 -f plant_guard.sql */ \set ON_ERROR_STOP on -/* - * schema_prefix: either empty, or the quoted schema name followed by a - * literal '.' - so the view definition below is a single statement with a - * plain (unquoted) substitution, rather than branching the whole CREATE - * VIEW on whether a schema was given. quote_ident(), not :"schema" - - * :schema_prefix is pasted as-is (unquoted substitution), so it must - * already be valid, properly-quoted SQL text by the time it lands there. - */ -SELECT CASE WHEN :'schema' <> '' THEN quote_ident(:'schema') || '.' ELSE '' END AS schema_prefix -\gset +\i test/helpers/find_test_schema.sql CREATE SCHEMA IF NOT EXISTS count_nulls_drop_guard; CREATE OR REPLACE VIEW count_nulls_drop_guard.guard AS - SELECT :schema_prefix null_count(NULL::int, NULL::int) AS guarded_member; + SELECT :"test_schema".null_count(NULL::int, NULL::int) AS guarded_member; diff --git a/test/README.md b/test/README.md index 3ae3a96..d5f72cf 100644 --- a/test/README.md +++ b/test/README.md @@ -28,41 +28,46 @@ then invoke via `runtests()`. `test__*` functions of its own (`test__check_ncs`, asserting count_nulls landed where expected; `test__shutdown__drop_all`, asserting it can be cleanly dropped), then runs everything via `runtests()`. +- `helpers/find_test_schema.sql` — rediscovers the randomly generated schema + count_nulls was installed into (see "Schema targeting" below), for + sessions that didn't create it themselves. -## TEST_SCHEMA +## Schema targeting -A make var/GUC (`count_nulls.test_schema`, propagated the same way as any -other placeholder GUC: `make var` -> `PGOPTIONS -c ...` -> `current_setting()` -- pg_regress doesn't forward make variables, but the psql processes it -spawns inherit the environment) selecting which schema `install/load.sql` -installs count_nulls into: +`install/load.sql` always installs count_nulls into its own freshly, +randomly generated schema - never a fixed name, and never no schema at all. +The generated name (`'count_nulls test schema ' || substr(md5(random()::text), 1, 12)`) +has two deliberate properties: -- Empty (default): no schema targeting at all - count_nulls lands wherever - the session's own default search_path resolves. Since `install/load.sql` - runs in its own bare connection (not the in-suite session pgTAP's own - `tap_setup.sql` runs in), that's `public`. -- Non-empty: explicitly `CREATE SCHEMA`, then `CREATE EXTENSION ... WITH - SCHEMA` that name - `install/load.sql` never mutates its own - search_path to do this. `TEST_SCHEMA=Quoted` locally exercises a name - requiring SQL identifier quoting (mixed case - unquoted would fold to - lowercase). +- A constant prefix (`count_nulls test schema `, with a trailing space) + that by itself already requires SQL identifier quoting - so every single + run exercises the suite's `%I`-qualification, not just a dedicated + "quoting" leg that could bitrot independently of a "plain" one. +- The same prefix doubles as a marker for stale-schema cleanup: before + generating a new name, `install/load.sql` finds and drops any + already-existing schema matching the prefix (`nspname LIKE 'count_nulls + test schema %'`), so a schema left behind by a run that crashed before + reaching its own teardown doesn't accumulate run over run. -Both legs run in CI - genuinely different code paths, not one a redundant -special case of the other. +`install/load.sql` targets the generated schema via `CREATE EXTENSION ... +WITH SCHEMA`, never by mutating its own search_path first. -**Why two legs prove anything.** Installing into two different schemas by -itself doesn't test whether count_nulls' own SQL correctly schema-qualifies -its internal references - if BOTH schemas happened to stay on the test -session's search_path (e.g. because the empty leg's `public` and the -`TEST_SCHEMA` leg's target were both reachable), an extension full of -unqualified, resolve-by-accident references would pass every leg too. What -actually matters is that at least ONE leg's install schema is verifiably -absent from search_path, so that leg's assertions only pass if `%I`-qualified -references are genuinely correct - checked by `test__check_ncs` in -`sql/extension_tests.sql`. This suite goes further and excludes the schema -from search_path in *every* leg, via the fixed `SET SEARCH_PATH` in -`core/functions.sql` - a stronger, deliberate choice, not the minimum -required. +**Cross-session discovery.** Some scripts/sessions (e.g. `bin/test_existing`'s +steps, each a fresh `psql -f ...` invocation with no memory of another +invocation's `\gset` variables) need the generated name without having +created it themselves. `helpers/find_test_schema.sql` looks it up live via +`pg_namespace`, hard-failing (not a pgTAP assertion - a genuinely broken +condition, like zero or more than one matching schema) if it can't find +exactly one, and sets `:"test_schema"` via `\gset` for the including script +to use. + +**Why this proves anything.** Because count_nulls' own schema is randomly +named, it can never coincidentally end up on the test session's +search_path - so `core/functions.sql`'s `%I`-qualified calls (via `ncs()`) +only pass if they're genuinely correct, never because count_nulls' schema +happened to be reachable unqualified. `test__check_ncs` in +`sql/extension_tests.sql` is what actually checks this, via the fixed `SET +SEARCH_PATH` in `core/functions.sql`. **Assertion descriptions deliberately never embed the schema name.** `core/functions.sql`'s assertions build the SQL they *execute* via `%I` @@ -70,17 +75,16 @@ qualification (through `ncs()`, so they're always correct no matter which real schema count_nulls landed in) but pass an *explicit*, schema-free description to every pgTAP call - overriding pgTAP's own auto-generated descriptions, which otherwise embed the schema. This is what keeps -`test/expected/extension_tests.out` a single file that both TEST_SCHEMA -legs pass against, instead of needing one file per schema value. +`test/expected/extension_tests.out` a single file that passes no matter +which randomly generated name count_nulls actually landed in. **`test__shutdown__drop_all`'s schema drop is plain cleanup, not a TAP -assertion.** Dropping the schema TEST_SCHEMA created is a real, correct -behavioral difference between "there's a schema to clean up" (non-empty) -and "there isn't" (empty, nothing to drop) - but it's teardown, not -something this suite is testing, so it's plain `EXECUTE`'d SQL with no -`lives_ok()`/`skip()` branch. That keeps its TAP output identical in every -TEST_SCHEMA leg (one `ok` row either way), so `test/expected/extension_tests.out` -needs no numbered pg_regress alternate for this function. +assertion.** Dropping the schema count_nulls was installed into isn't +something this suite is testing, just tearing down what `install/load.sql` +created - so it's plain `EXECUTE`'d SQL with no `lives_ok()`/`skip()` +branch. Its output is identical on every run (one `ok` row), so +`test/expected/extension_tests.out` needs no numbered pg_regress alternate +for this function. ## Regenerating expected output diff --git a/test/helpers/find_test_schema.sql b/test/helpers/find_test_schema.sql new file mode 100644 index 0000000..28f7f38 --- /dev/null +++ b/test/helpers/find_test_schema.sql @@ -0,0 +1,26 @@ +/* + * Discovers the schema count_nulls was installed into for this test run - + * for scripts/sessions that didn't create it themselves and have no other + * way to know its (randomly generated) name. See test/install/load.sql for + * how/why the name is randomized. + * + * Exactly one schema matching the prefix is expected. Finding zero or more + * than one means something is broken (e.g. a previous run's schema was + * never cleaned up, or this ran before installation happened) - abort + * immediately rather than silently guessing. This is a hard failure, not a + * pgTAP-style assertion. + */ +DO $$ +DECLARE + v_count int := (SELECT count(*) FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'); +BEGIN + IF v_count <> 1 THEN + RAISE EXCEPTION + 'expected exactly one schema matching ''count_nulls test schema %%'', found %' + , v_count; + END IF; +END +$$; + +SELECT nspname AS test_schema FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' +\gset diff --git a/test/install/load.sql b/test/install/load.sql index 41fa1a8..24110f9 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -12,53 +12,6 @@ * from a textual comparison - matching cat_tools' test/install/load.sql. */ -/* - * TEST_SCHEMA (the count_nulls.test_schema GUC, set via the Makefile): - * which schema to install count_nulls into. Empty (the default) means - * "don't target any schema at all" - lands wherever this session's own - * default search_path resolves (ordinarily 'public', since test/install - * runs in its own bare connection, not the in-suite session pgTAP's - * tap_setup.sql runs in - see phase 1's commit message for why that - * matters). Non-empty explicitly creates that schema and targets it via - * CREATE EXTENSION ... WITH SCHEMA below - this file never mutates its - * own search_path to do so, and not because a one-shot bare connection - * wouldn't care about a leftover mutation either way: mutating - * search_path before CREATE EXTENSION would let the install succeed via - * a coincidentally arranged search_path, masking the extension's own - * install script secretly depending on unqualified name resolution - * during install. WITH SCHEMA targets the schema directly without - * touching search_path at all, so a successful install actually proves - * the install script itself doesn't need search_path arranged any - * particular way - the same qualification-correctness principle behind - * the whole TEST_SCHEMA axis (see core/functions.sql's header and - * test__check_ncs in sql/extension_tests.sql), just applied to the - * install step itself rather than to post-install test assertions. - * - * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, - * not be indistinguishable from a deliberately empty one. - */ -SELECT current_setting('count_nulls.test_schema') AS schema -\gset -SELECT :'schema' <> '' AS count_nulls_has_schema -\gset - -/* - * A reusable ' WITH SCHEMA "..."' fragment (leading space included, empty - * when count_nulls_has_schema is false) so CREATE EXTENSION below can just - * append :with_schema_clause without repeating the has-schema branch. - * format() is used unqualified: it's pg_catalog, always resolvable - * regardless of search_path. - */ -SELECT CASE WHEN :'count_nulls_has_schema' - THEN format(' WITH SCHEMA %I', :'schema') - ELSE '' - END AS with_schema_clause -\gset - -\if :count_nulls_has_schema -CREATE SCHEMA IF NOT EXISTS :"schema"; -\endif - /* * Mode selection: 'fresh' installs the current version directly; 'update' * installs the oldest version we still ship a full script for (0.9.6) and @@ -68,7 +21,8 @@ CREATE SCHEMA IF NOT EXISTS :"schema"; * 'existing' asserts count_nulls is already installed (a real `pg_upgrade` * run, external to this invocation) and touches nothing. * - * Read without missing_ok, same reasoning as count_nulls.test_schema above. + * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, + * not be indistinguishable from a deliberately empty one. */ SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_test_load_mode , current_setting('count_nulls.test_load_mode') = 'update' AS count_nulls_update_mode @@ -106,8 +60,55 @@ BEGIN END IF; END $$; -\elif :count_nulls_update_mode -CREATE EXTENSION count_nulls:with_schema_clause VERSION '0.9.6'; +\else +/* + * fresh/update: count_nulls always installs into its own freshly, randomly + * generated schema, never a fixed name - so every reference the extension + * makes to its own members is exercised through real SQL identifier + * quoting on every single run, not just on some dedicated "quoting" leg + * that could bitrot independently of a "plain" one. The generated name's + * constant prefix (a literal trailing space included) already guarantees + * quoting is required before the random suffix is even appended - unlike a + * mixed-case-only name, which would only force quoting by coincidence of + * which characters the randomness happened to produce. + * + * Cleanup-before-create: a prior run that crashed before reaching its own + * teardown (test__shutdown__drop_all in test/sql/extension_tests.sql) + * would otherwise leave its randomly-named schema behind forever, since + * nothing else knows that name to find and drop it later. Matching on the + * constant prefix finds and drops any such leftovers before generating + * this run's own name. See test/helpers/find_test_schema.sql for how + * later, separate sessions rediscover the name this run creates. + */ +DO $$ +DECLARE + r record; +BEGIN + FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP + EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); + END LOOP; +END +$$; + +SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema +\gset + +CREATE SCHEMA :"schema"; + +/* + * WITH SCHEMA targets the schema directly without touching search_path at + * all, so a successful install actually proves the install script itself + * doesn't need search_path arranged any particular way - the same + * qualification-correctness principle behind randomizing the schema name + * in the first place (see core/functions.sql's header and test__check_ncs + * in sql/extension_tests.sql), just applied to the install step itself + * rather than to post-install test assertions. Mutating search_path before + * CREATE EXTENSION instead would let the install succeed via a + * coincidentally arranged search_path, masking the extension's own install + * script secretly depending on unqualified name resolution during install. + */ +\if :count_nulls_update_mode +CREATE EXTENSION count_nulls WITH SCHEMA :"schema" VERSION '0.9.6'; /* * Suppress the "already installed, no update" NOTICE class of messages any * update script might emit. @@ -116,5 +117,6 @@ SET client_min_messages = WARNING; ALTER EXTENSION count_nulls UPDATE; SET client_min_messages = NOTICE; \else -CREATE EXTENSION count_nulls:with_schema_clause; +CREATE EXTENSION count_nulls WITH SCHEMA :"schema"; +\endif \endif diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 1cd1bcb..2506212 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -6,52 +6,28 @@ /* * This file leaves search_path as functions.sql set it (_null_count_test, - * tap) - with an explicit TEST_SCHEMA, that keeps count_nulls' own schema - * off search_path, so every check below only passes if functions.sql's + * tap), and count_nulls always installs into its own freshly, randomly + * generated schema (see test/install/load.sql) - which is never on that + * search_path. So every check below only passes if functions.sql's * %I-qualified calls (via ncs()) are actually correct, never relying on - * count_nulls' own schema being reachable unqualified. When TEST_SCHEMA is - * empty, count_nulls lands in 'public' (test/install/load.sql runs in its - * own bare connection, with no schema targeting - see phase 1's commit - * message), which is NOT on search_path here either. - * - * Excluding it from BOTH legs is stronger than strictly required: the thing - * that actually makes a multi-schema matrix meaningful is that AT LEAST ONE - * tested schema is verifiably off search_path (otherwise installing into two - * schemas that both happen to stay reachable would let an unqualified, - * resolve-by-accident reference pass every leg without ever being caught). - * Keeping both legs off search_path is a simpler, deliberately stricter - * choice here, not evidence that every leg must be - a hypothetical future - * leg that left its schema on search_path wouldn't invalidate this design, - * as long as at least one other leg still excludes it. - * - * schema_hint reads the count_nulls.test_schema GUC directly (the Makefile - * exports it via PGOPTIONS for the whole run - see test/install/load.sql, - * which installs into it) rather than via a psql variable relayed through - * test/deps.sql: nothing in this per-test session needs deps.sql to have - * set anything, since the GUC is readable from any session in the run. - * NULLIF turns the empty-TEST_SCHEMA case into NULL, and runtests() calls - * every test__* function with no arguments, so it always gets this default. + * count_nulls' own schema being reachable unqualified. */ CREATE FUNCTION _null_count_test.test__check_ncs( - schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ DECLARE /* - * We either expect count_nulls' own schema to be in search_path, or we - * don't - and in this file we never do, in either leg: functions.sql - * unconditionally sets search_path to exclude it (see the header - * comment above), whether that's the empty leg's 'public' or the - * TEST_SCHEMA leg's known target. s is still real, independently - * determined content (via ncs() when there's no fixed target, via - * schema_hint when there is), so the membership check below genuinely - * exercises functions.sql's %I-qualification and load.sql's schema - * targeting - it isn't a tautology. + * We never expect count_nulls' own schema to be in search_path in this + * file: functions.sql unconditionally sets search_path to exclude it + * (see the header comment above). s is still real, independently + * determined content (via ncs()), so the membership check below + * genuinely exercises functions.sql's %I-qualification and load.sql's + * schema targeting - it isn't a tautology. * * SEE ALSO: teardown__search_path_unchanged in test/core/functions.sql, * which guards against some OTHER test mutating search_path mid-suite (a * different risk than this check). */ - s CONSTANT name = coalesce(schema_hint, ncs()); + s CONSTANT name = ncs(); BEGIN RETURN NEXT is( current_schemas(true) @> array[s] @@ -62,25 +38,29 @@ END $body$; CREATE FUNCTION _null_count_test.test__shutdown__drop_all( - schema_hint name DEFAULT nullif(current_setting('count_nulls.test_schema'), '')::name ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ +DECLARE + /* + * Captured before DROP EXTENSION: ncs() looks the schema up live via + * pg_extension, which can't resolve anything once the extension is + * gone. + */ + s CONSTANT name = ncs(); BEGIN RETURN NEXT lives_ok( $$DROP EXTENSION count_nulls$$ ); /* - * Plain cleanup, not a TAP assertion - dropping the schema TEST_SCHEMA - * created isn't something this suite is testing, just tearing down - * what it created. Same output in every TEST_SCHEMA leg: when - * schema_hint is NULL (empty leg), there's nothing to drop, so this is - * a no-op; if the DROP SCHEMA itself ever failed, the unhandled - * exception aborts the run loudly on its own - no lives_ok() needed - * for that. + * Plain cleanup, not a TAP assertion - dropping the schema count_nulls + * was installed into isn't something this suite is testing, just + * tearing down what test/install/load.sql created. Every run always + * has a schema to drop (there's no longer an "installed with no + * schema targeting" case). If the DROP SCHEMA itself ever failed, the + * unhandled exception aborts the run loudly on its own - no lives_ok() + * needed for that. */ - IF schema_hint IS NOT NULL THEN - EXECUTE format('DROP SCHEMA %I', schema_hint); - END IF; + EXECUTE format('DROP SCHEMA %I', s); END $body$; From e5d3ec918ce7a22962e181915daab4be2ccc06c6 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 18:33:03 -0500 Subject: [PATCH 2/8] test/core/functions.sql: update two stale TEST_SCHEMA comment pointers These two comments referenced "TEST_SCHEMA in the Makefile" / "TEST_SCHEMA" directly, a concept this PR removed elsewhere - point them at the "Schema targeting" section of test/README.md instead. teardown__search_path_unchanged's own comment (which also mentions an early design phase with no TEST_SCHEMA concept) is left as-is: it's accurate historical scoping about that function's own reasoning, not a claim about the current codebase. Co-Authored-By: Claude Sonnet 5 --- test/core/functions.sql | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/core/functions.sql b/test/core/functions.sql index 33bc30b..4d06ffc 100644 --- a/test/core/functions.sql +++ b/test/core/functions.sql @@ -75,11 +75,12 @@ BEGIN /* * Explicit descriptions below (not pgTAP's auto-generated default, - * which schema-qualifies via ncs()): this suite may run against - * count_nulls installed in ANY schema (see TEST_SCHEMA in the - * Makefile), and the description text is exact-matched by pg_regress - * against a single committed expected-output file - it must stay - * IDENTICAL no matter which schema the extension actually landed in. + * which schema-qualifies via ncs()): this suite runs against + * count_nulls installed in a freshly, randomly generated schema (see + * "Schema targeting" in test/README.md), and the description text is + * exact-matched by pg_regress against a single committed + * expected-output file - it must stay IDENTICAL no matter which + * schema the extension actually landed in. * ncs() itself is still used to locate and call the real function; * only the visible description text drops it. */ @@ -140,7 +141,8 @@ CREATE FUNCTION pg_temp.test_trigger_raw( * Schema-free stand-in for exec, used ONLY in the visible description * below - exec itself (schema-qualified via ncs(), by callers) is what * actually runs. Keeps this suite's output identical no matter which - * schema count_nulls is installed in (see TEST_SCHEMA). + * randomly generated schema count_nulls is installed in (see "Schema + * targeting" in test/README.md). */ , exec_desc text From 923cd2fa5c5eb2998ffaa9a4111966c65553bfb5 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 18:56:16 -0500 Subject: [PATCH 3/8] .github/workflows/ci.yml: scope the random-schema paragraph to testing The wording made it sound like installing into a random schema is how count_nulls behaves as a product, rather than a testing-time decision. Reword and shorten it to lead with "during testing", with a pointer to test/install/load.sql and test/README.md for the mechanics instead of restating them here. --- .github/workflows/ci.yml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9dee907..aa14bd4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,12 +35,9 @@ # through AWS pg_tle's database-backed catalog # instead of a filesystem .control file. # -# count_nulls always installs into its own freshly, randomly generated -# schema (a name that always requires SQL identifier quoting - see -# test/install/load.sql), so every run exercises the suite's %I -# schema-qualification, not just its literal test data. test/expected/ -# extension_tests.out stays schema-invariant regardless of the actual -# generated name (see test/README.md for how). +# During testing we always install into a freshly, randomly generated +# schema (see test/install/load.sql; test/README.md covers how +# test/expected/extension_tests.out stays schema-invariant). # # `changes` is a cheap gate that lets the heavy jobs above skip themselves on # doc-only pushes, and also derives the shared PostgreSQL-major list those From 1fcc42c356823ec5836e878ddf5997a3453efbf3 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 18:56:24 -0500 Subject: [PATCH 4/8] Factor schema-creation + install logic into test/helpers/create_test_schema.sql test/install/load.sql and bin/test_existing.sql/create_extension.sql duplicated the same cleanup-before-create + random-name + CREATE SCHEMA + CREATE EXTENSION logic, differing only in whether a VERSION was pinned. Move it into a single test/helpers/create_test_schema.sql, parameterized by a :version psql variable (empty = no VERSION clause), and have both callers \i/-f it instead of keeping their own copies. test/install/load.sql now does `\set version ...` then `\i`s the shared file for both its fresh and update modes; bin/test_existing's create_extension_in_schema() now points -f directly at the shared file. bin/test_existing.sql/create_extension.sql is removed, fully superseded. --- bin/test_existing | 10 ++-- bin/test_existing.sql/create_extension.sql | 28 ----------- test/helpers/create_test_schema.sql | 54 +++++++++++++++++++++ test/install/load.sql | 55 ++++------------------ 4 files changed, 67 insertions(+), 80 deletions(-) delete mode 100644 bin/test_existing.sql/create_extension.sql create mode 100644 test/helpers/create_test_schema.sql diff --git a/bin/test_existing b/bin/test_existing index db1fd5e..2fde5af 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -142,14 +142,12 @@ update_ext() { } # CREATE EXTENSION count_nulls at VERSION, into a freshly, randomly -# generated schema - the same cleanup-before-create + random-name pattern -# test/install/load.sql uses for its own installs (see that file for the -# full rationale). Needs multiple statements including a \gset, so this -# runs as a proper -f script file rather than being inlined into -c, same -# convention plant_guard()/drop_guard() already use. +# generated schema - shared with test/install/load.sql's own fresh/update +# installs via test/helpers/create_test_schema.sql (see that file for the +# full rationale). create_extension_in_schema() { local db=$1 version=$2 - psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" -f bin/test_existing.sql/create_extension.sql + psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" -f test/helpers/create_test_schema.sql } # --------------------------------------------------------------------------- diff --git a/bin/test_existing.sql/create_extension.sql b/bin/test_existing.sql/create_extension.sql deleted file mode 100644 index bc61b8c..0000000 --- a/bin/test_existing.sql/create_extension.sql +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Generates a fresh, randomly-named schema and installs count_nulls at - * :version into it, for prepare_old()'s old-cluster install - the same - * cleanup-before-create + random-name pattern test/install/load.sql uses - * for its own installs (see that file for the full rationale). Later, - * separate invocations (e.g. bin/test_existing's other steps, each a - * fresh psql session with no memory of this one's \gset variables) - * rediscover the name this creates via test/helpers/find_test_schema.sql. - * - * Usage: psql -v ON_ERROR_STOP=1 -v version= -f create_extension.sql - */ -\set ON_ERROR_STOP on - -DO $$ -DECLARE - r record; -BEGIN - FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP - EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); - END LOOP; -END -$$; - -SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema -\gset - -CREATE SCHEMA :"schema"; -CREATE EXTENSION count_nulls WITH SCHEMA :"schema" VERSION :'version'; diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql new file mode 100644 index 0000000..2179486 --- /dev/null +++ b/test/helpers/create_test_schema.sql @@ -0,0 +1,54 @@ +/* + * Creates a fresh, randomly named schema and installs count_nulls into it. + * Shared by test/install/load.sql (fresh/update modes - same psql session, + * `\set version` then `\i` this file) AND bin/test_existing's prepare-old + * (a SEPARATE invocation - `-v version=` on the command + * line). Unusual for a test/ file to also be invoked from bin/, but the + * creation logic is identical in both cases, so it lives here once instead + * of being duplicated. + * + * :version empty means "no VERSION clause" (installs whatever the current + * default is); non-empty targets that specific version. + * + * The generated name's constant prefix (a literal trailing space included) + * already guarantees SQL identifier quoting is required before the random + * suffix is even appended - unlike a mixed-case-only name, which would + * only force quoting by coincidence of which characters the randomness + * happened to produce. + * + * Cleanup-before-create: a prior run that crashed before reaching its own + * teardown would otherwise leave its randomly-named schema behind forever, + * since nothing else knows that name to find and drop it later. Matching + * on the constant prefix finds and drops any such leftovers before + * generating this run's own name. See test/helpers/find_test_schema.sql + * for how later, separate sessions rediscover the name this creates. + */ +DO $$ +DECLARE + r record; +BEGIN + FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP + EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); + END LOOP; +END +$$; + +SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema +\gset + +CREATE SCHEMA :"schema"; + +/* + * WITH SCHEMA targets the schema directly without touching search_path at + * all, so a successful install actually proves the install script itself + * doesn't need search_path arranged any particular way - the same + * qualification-correctness principle behind randomizing the schema name + * in the first place. Mutating search_path before CREATE EXTENSION + * instead would let the install succeed via a coincidentally arranged + * search_path, masking the extension's own install script secretly + * depending on unqualified name resolution during install. + */ +SELECT CASE WHEN :'version' <> '' THEN format(' VERSION %L', :'version') ELSE '' END AS version_clause +\gset + +CREATE EXTENSION count_nulls WITH SCHEMA :"schema":version_clause; diff --git a/test/install/load.sql b/test/install/load.sql index 24110f9..36c9e61 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -62,53 +62,15 @@ END $$; \else /* - * fresh/update: count_nulls always installs into its own freshly, randomly - * generated schema, never a fixed name - so every reference the extension - * makes to its own members is exercised through real SQL identifier - * quoting on every single run, not just on some dedicated "quoting" leg - * that could bitrot independently of a "plain" one. The generated name's - * constant prefix (a literal trailing space included) already guarantees - * quoting is required before the random suffix is even appended - unlike a - * mixed-case-only name, which would only force quoting by coincidence of - * which characters the randomness happened to produce. - * - * Cleanup-before-create: a prior run that crashed before reaching its own - * teardown (test__shutdown__drop_all in test/sql/extension_tests.sql) - * would otherwise leave its randomly-named schema behind forever, since - * nothing else knows that name to find and drop it later. Matching on the - * constant prefix finds and drops any such leftovers before generating - * this run's own name. See test/helpers/find_test_schema.sql for how - * later, separate sessions rediscover the name this run creates. - */ -DO $$ -DECLARE - r record; -BEGIN - FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %' LOOP - EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname); - END LOOP; -END -$$; - -SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema -\gset - -CREATE SCHEMA :"schema"; - -/* - * WITH SCHEMA targets the schema directly without touching search_path at - * all, so a successful install actually proves the install script itself - * doesn't need search_path arranged any particular way - the same - * qualification-correctness principle behind randomizing the schema name - * in the first place (see core/functions.sql's header and test__check_ncs - * in sql/extension_tests.sql), just applied to the install step itself - * rather than to post-install test assertions. Mutating search_path before - * CREATE EXTENSION instead would let the install succeed via a - * coincidentally arranged search_path, masking the extension's own install - * script secretly depending on unqualified name resolution during install. + * fresh/update: creation (fresh, randomly named schema; install at + * :version or current) is shared with bin/test_existing's prepare-old via + * test/helpers/create_test_schema.sql - see that file for the full + * rationale. 'update' additionally installs the oldest version we still + * ship a full script for, then upgrades it in place. */ \if :count_nulls_update_mode -CREATE EXTENSION count_nulls WITH SCHEMA :"schema" VERSION '0.9.6'; +\set version '0.9.6' +\i test/helpers/create_test_schema.sql /* * Suppress the "already installed, no update" NOTICE class of messages any * update script might emit. @@ -117,6 +79,7 @@ SET client_min_messages = WARNING; ALTER EXTENSION count_nulls UPDATE; SET client_min_messages = NOTICE; \else -CREATE EXTENSION count_nulls WITH SCHEMA :"schema"; +\set version '' +\i test/helpers/create_test_schema.sql \endif \endif From 2789f703823840d7fdc80e47572d8efd72175ddc Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 7 Aug 2026 18:56:30 -0500 Subject: [PATCH 5/8] test/helpers/find_test_schema.sql: use SELECT ... INTO STRICT Replace the hand-rolled count-then-RAISE EXCEPTION validation with SELECT ... INTO STRICT, which raises Postgres's own no_data_found/too_many_rows when the query doesn't resolve to exactly one row, instead of re-implementing that check. The actual :test_schema capture stays a separate plain SELECT + \gset, since a DO block can't populate a psql variable itself (a \gset following one just silently re-executes it and sets nothing). --- test/helpers/find_test_schema.sql | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/test/helpers/find_test_schema.sql b/test/helpers/find_test_schema.sql index 28f7f38..973db46 100644 --- a/test/helpers/find_test_schema.sql +++ b/test/helpers/find_test_schema.sql @@ -1,24 +1,26 @@ /* * Discovers the schema count_nulls was installed into for this test run - * for scripts/sessions that didn't create it themselves and have no other - * way to know its (randomly generated) name. See test/install/load.sql for - * how/why the name is randomized. + * way to know its (randomly generated) name. See + * test/helpers/create_test_schema.sql for how/why the name is randomized. * - * Exactly one schema matching the prefix is expected. Finding zero or more - * than one means something is broken (e.g. a previous run's schema was - * never cleaned up, or this ran before installation happened) - abort - * immediately rather than silently guessing. This is a hard failure, not a - * pgTAP-style assertion. + * SELECT ... INTO STRICT raises Postgres's own no_data_found/too_many_rows + * if this doesn't resolve to exactly one schema, instead of hand-counting + * rows and raising a custom exception for the same thing - a DO block + * can't populate a psql variable itself (confirmed directly: a `\gset` + * following one just silently re-executes it and sets nothing), so the + * actual :test_schema capture below still has to be a separate plain + * SELECT. This is a hard failure either way, not a pgTAP-style assertion - + * finding zero or more than one match means something is broken (a + * previous run's schema was never cleaned up, or this ran before + * installation happened) and must abort immediately rather than silently + * guessing. */ DO $$ DECLARE - v_count int := (SELECT count(*) FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'); + v_schema name; BEGIN - IF v_count <> 1 THEN - RAISE EXCEPTION - 'expected exactly one schema matching ''count_nulls test schema %%'', found %' - , v_count; - END IF; + SELECT nspname INTO STRICT v_schema FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'; END $$; From bada5ba2412eefc7694849e5e64f7f8291de0873 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 8 Aug 2026 18:15:10 -0500 Subject: [PATCH 6/8] test/helpers/create_test_schema.sql: require explicit :version, reject empty Empty :version was being treated as a deliberate "install current" signal. That's ambiguous: a caller whose :version was supposed to carry a real version but ended up empty due to an upstream bug would silently install current instead of failing loudly - the same class of footgun this codebase already guards against elsewhere (see test/install/load.sql's "Read without missing_ok" GUC handling). Require the same 'current' sentinel bin/test_existing's assert_version()/current_version() already use instead, and make an empty :version a hard RAISE EXCEPTION. Update test/install/load.sql's fresh-mode branch to pass 'current' accordingly; its update-mode branch already passes a real version (0.9.6) and is unchanged. No other caller of create_test_schema.sql passes an empty version. The guard bridges :version into its DO block via SET + current_setting() (matching test/install/load.sql's own count_nulls.test_load_mode pattern) rather than referencing :'version' directly inside the DO $$ ... $$ body: psql does not interpolate variables inside dollar-quoted strings, only in plain top-level SQL text such as the version_clause SELECT below it - confirmed directly, a first attempt using :'version' inside the DO block reached the server un-substituted and errored with a syntax error at ":". Also, two smaller follow-ups on this same branch: - test/helpers/find_test_schema.sql: revert the SELECT ... INTO STRICT refactor back to the original hand-counted RAISE EXCEPTION. The motivating idea - that a DO block could both validate and populate :test_schema in one step - doesn't hold: a DO block can't populate a psql variable at all (a \gset following one silently re-executes it and captures nothing), so the separate plain SELECT + \gset is needed either way, and INTO STRICT doesn't actually remove a statement. - test/sql/extension_tests.sql: note in the header comment that test/expected/extension_tests.out staying identical regardless of the random schema name isn't a coincidence - none of the pgTAP assertion descriptions embed the schema name (see test/README.md for the full rationale). Co-Authored-By: Claude Sonnet 5 --- test/helpers/create_test_schema.sql | 30 ++++++++++++++++++++++++++--- test/helpers/find_test_schema.sql | 25 +++++++++++++++--------- test/install/load.sql | 2 +- test/sql/extension_tests.sql | 7 +++++++ 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 2179486..bd236e7 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -7,8 +7,22 @@ * creation logic is identical in both cases, so it lives here once instead * of being duplicated. * - * :version empty means "no VERSION clause" (installs whatever the current - * default is); non-empty targets that specific version. + * :version must always be set explicitly to either the literal string + * 'current' (no VERSION clause - installs whatever the current default is) + * or a real version string (targets that specific version) - matching the + * same 'current' sentinel bin/test_existing's assert_version()/ + * current_version() already use, for the same reason: an empty string is a + * HARD ERROR rather than a valid signal, so an accidentally-unpropagated + * :version fails loudly instead of silently installing 'current' when + * something else was actually intended. + * + * The guard below bridges :version into the DO block via a SET + a real + * GUC (like test/install/load.sql's count_nulls.test_load_mode) rather than + * referencing :'version' directly inside the DO $$ ... $$ body: psql does + * NOT interpolate variables inside dollar-quoted strings (confirmed + * directly - a bare :'version' inside a $$ ... $$ block reaches the server + * un-substituted and is a syntax error), only in plain top-level SQL text + * such as the version_clause SELECT below. * * The generated name's constant prefix (a literal trailing space included) * already guarantees SQL identifier quoting is required before the random @@ -23,6 +37,16 @@ * generating this run's own name. See test/helpers/find_test_schema.sql * for how later, separate sessions rediscover the name this creates. */ +SET count_nulls.test_schema_version = :'version'; + +DO $$ +BEGIN + IF current_setting('count_nulls.test_schema_version') = '' THEN + RAISE EXCEPTION ':version must be set explicitly - use ''current'' to install whatever the current default is, never an empty string, so an accidentally-unpropagated value fails loudly instead of silently installing ''current'' when something else was actually intended'; + END IF; +END +$$; + DO $$ DECLARE r record; @@ -48,7 +72,7 @@ CREATE SCHEMA :"schema"; * search_path, masking the extension's own install script secretly * depending on unqualified name resolution during install. */ -SELECT CASE WHEN :'version' <> '' THEN format(' VERSION %L', :'version') ELSE '' END AS version_clause +SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause \gset CREATE EXTENSION count_nulls WITH SCHEMA :"schema":version_clause; diff --git a/test/helpers/find_test_schema.sql b/test/helpers/find_test_schema.sql index 973db46..1b1f2c4 100644 --- a/test/helpers/find_test_schema.sql +++ b/test/helpers/find_test_schema.sql @@ -4,13 +4,16 @@ * way to know its (randomly generated) name. See * test/helpers/create_test_schema.sql for how/why the name is randomized. * - * SELECT ... INTO STRICT raises Postgres's own no_data_found/too_many_rows - * if this doesn't resolve to exactly one schema, instead of hand-counting - * rows and raising a custom exception for the same thing - a DO block - * can't populate a psql variable itself (confirmed directly: a `\gset` - * following one just silently re-executes it and sets nothing), so the - * actual :test_schema capture below still has to be a separate plain - * SELECT. This is a hard failure either way, not a pgTAP-style assertion - + * If an anonymous DO block could return/populate a value back to the + * calling psql session, `SELECT ... INTO STRICT` would be the more natural + * way to write this check - Postgres's own built-in "expect exactly one + * row" enforcement, instead of manually counting rows and raising a custom + * exception. It can't (confirmed directly: a `\gset` following a DO block + * just silently re-executes the block's query text and captures nothing), + * so validation and the actual :test_schema capture below have to be two + * separate statements regardless, and the DO block falls back to a + * hand-counted check. That check is a hard failure, not a pgTAP-style + * assertion - * finding zero or more than one match means something is broken (a * previous run's schema was never cleaned up, or this ran before * installation happened) and must abort immediately rather than silently @@ -18,9 +21,13 @@ */ DO $$ DECLARE - v_schema name; + v_count int := (SELECT count(*) FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'); BEGIN - SELECT nspname INTO STRICT v_schema FROM pg_namespace WHERE nspname LIKE 'count_nulls test schema %'; + IF v_count <> 1 THEN + RAISE EXCEPTION + 'expected exactly one schema matching ''count_nulls test schema %%'', found %' + , v_count; + END IF; END $$; diff --git a/test/install/load.sql b/test/install/load.sql index 36c9e61..5a4f12c 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -79,7 +79,7 @@ SET client_min_messages = WARNING; ALTER EXTENSION count_nulls UPDATE; SET client_min_messages = NOTICE; \else -\set version '' +\set version 'current' \i test/helpers/create_test_schema.sql \endif \endif diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 2506212..1913e48 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -11,6 +11,13 @@ * search_path. So every check below only passes if functions.sql's * %I-qualified calls (via ncs()) are actually correct, never relying on * count_nulls' own schema being reachable unqualified. + * + * This also means test/expected/extension_tests.out stays identical run to + * run regardless of which random name the schema gets: none of the pgTAP + * assertion descriptions below embed the actual schema name (see + * test/README.md's "Assertion descriptions deliberately never embed the + * schema name" section for the full rationale), so no numbered pg_regress + * alternate is ever needed for it. */ CREATE FUNCTION _null_count_test.test__check_ncs( ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ From 11847e525250e885406b491b29273b83a78ae206 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 8 Aug 2026 18:24:00 -0500 Subject: [PATCH 7/8] test/sql/extension_tests.sql: drop the redundant schema cleanup in test__shutdown__drop_all test/helpers/create_test_schema.sql already unconditionally drops any leftover 'count_nulls test schema %' schema before creating a fresh one, on every single invocation - so the explicit DROP SCHEMA %I this function also did was always redundant duplicate cleanup, never the thing that actually mattered for catching a stale schema. It was also moot in practice: this file's functions run inside a transaction pgxntool's test harness rolls back, never commits, so the DROP SCHEMA here never persisted past the run anyway. test__shutdown__drop_all now just asserts DROP EXTENSION succeeds, and no longer needs ncs() to know which schema to drop. Co-Authored-By: Claude Sonnet 5 --- test/sql/extension_tests.sql | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/test/sql/extension_tests.sql b/test/sql/extension_tests.sql index 1913e48..bc049bd 100644 --- a/test/sql/extension_tests.sql +++ b/test/sql/extension_tests.sql @@ -44,30 +44,23 @@ BEGIN END $body$; +/* + * Just asserts the extension can be dropped - doesn't attempt any schema + * cleanup itself. test/helpers/create_test_schema.sql already + * unconditionally drops any leftover 'count_nulls test schema %' schema + * before creating a fresh one on every single invocation. A second + * per-run cleanup here would only ever be redundant duplicate cleanup + * logic and never the thing that actually matters. (It's also moot in + * practice: this file's functions run inside a transaction pgxntool's + * test harness rolls back, never commits, so an explicit DROP SCHEMA here + * would never persist past this run anyway.) + */ CREATE FUNCTION _null_count_test.test__shutdown__drop_all( ) RETURNS SETOF text LANGUAGE plpgsql AS $body$ -DECLARE - /* - * Captured before DROP EXTENSION: ncs() looks the schema up live via - * pg_extension, which can't resolve anything once the extension is - * gone. - */ - s CONSTANT name = ncs(); BEGIN RETURN NEXT lives_ok( $$DROP EXTENSION count_nulls$$ ); - - /* - * Plain cleanup, not a TAP assertion - dropping the schema count_nulls - * was installed into isn't something this suite is testing, just - * tearing down what test/install/load.sql created. Every run always - * has a schema to drop (there's no longer an "installed with no - * schema targeting" case). If the DROP SCHEMA itself ever failed, the - * unhandled exception aborts the run loudly on its own - no lives_ok() - * needed for that. - */ - EXECUTE format('DROP SCHEMA %I', s); END $body$; From f176c8c18d421bcbc3dedc5c5a1c25f5c5fabfe5 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 8 Aug 2026 18:31:24 -0500 Subject: [PATCH 8/8] Point stale test/install/load.sql comments at test/helpers/create_test_schema.sql bin/test_existing's prepare-old usage doc and plant_guard.sql's header both said the random schema is created by test/install/load.sql, but neither subcommand ever goes through load.sql - prepare-old calls test/helpers/create_test_schema.sql directly. Point both at the actual file. test/README.md's Layout section had no entry for helpers/create_test_schema.sql despite it being a substantial, shared, independently documented file; add one. Its Schema targeting section still attributed the random-name generation and cleanup-before-create behavior directly to install/load.sql, which is only true for the load.sql-mediated path now - reattribute it to create_test_schema.sql, noting it's shared by both install/load.sql and bin/test_existing. Also update the test__shutdown__drop_all paragraph, stale after the prior commit on this branch removed its explicit schema drop: it now only asserts DROP EXTENSION succeeds, relying entirely on create_test_schema.sql's own cleanup-before-create. test/core/functions.sql, test/sql/extension_tests.sql, and ci.yml's test/install/load.sql pointers are left untouched - those describe the pgTAP suite's own execution context, which is genuinely only ever reached via load.sql's fresh/update flow. Co-Authored-By: Claude Sonnet 5 --- bin/test_existing | 3 +- bin/test_existing.sql/plant_guard.sql | 6 ++-- test/README.md | 43 ++++++++++++++++----------- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/bin/test_existing b/bin/test_existing index 2fde5af..32c26fd 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -35,7 +35,8 @@ # prepare-old DB INSTALL_VERSION # Old-cluster prep for pg-upgrade-test: create DB + extension at # INSTALL_VERSION (in a freshly, randomly generated schema - see -# test/install/load.sql), then plant + prove the dependency guard. +# test/helpers/create_test_schema.sql), then plant + prove the +# dependency guard. # # update DB [TO_VERSION] # ALTER EXTENSION count_nulls UPDATE [TO 'TO_VERSION'] (empty => current). diff --git a/bin/test_existing.sql/plant_guard.sql b/bin/test_existing.sql/plant_guard.sql index ef27e86..da3b77c 100644 --- a/bin/test_existing.sql/plant_guard.sql +++ b/bin/test_existing.sql/plant_guard.sql @@ -9,9 +9,9 @@ * still report green). * * count_nulls always installs into its own randomly generated schema (see - * test/install/load.sql) - this session didn't create it, so it has no - * other way to know its name; test/helpers/find_test_schema.sql discovers - * it live via pg_namespace. + * test/helpers/create_test_schema.sql) - this session didn't create it, so + * it has no other way to know its name; test/helpers/find_test_schema.sql + * discovers it live via pg_namespace. * * Usage: psql -v ON_ERROR_STOP=1 -f plant_guard.sql */ diff --git a/test/README.md b/test/README.md index d5f72cf..b3b58e3 100644 --- a/test/README.md +++ b/test/README.md @@ -28,29 +28,37 @@ then invoke via `runtests()`. `test__*` functions of its own (`test__check_ncs`, asserting count_nulls landed where expected; `test__shutdown__drop_all`, asserting it can be cleanly dropped), then runs everything via `runtests()`. +- `helpers/create_test_schema.sql` — creates the freshly, randomly generated + schema and installs count_nulls into it (see "Schema targeting" below). + Shared by `install/load.sql`'s fresh/update modes and `bin/test_existing`'s + `prepare-old` - two separate call sites, one shared implementation. - `helpers/find_test_schema.sql` — rediscovers the randomly generated schema count_nulls was installed into (see "Schema targeting" below), for sessions that didn't create it themselves. ## Schema targeting -`install/load.sql` always installs count_nulls into its own freshly, -randomly generated schema - never a fixed name, and never no schema at all. -The generated name (`'count_nulls test schema ' || substr(md5(random()::text), 1, 12)`) -has two deliberate properties: +`helpers/create_test_schema.sql` always installs count_nulls into its own +freshly, randomly generated schema - never a fixed name, and never no +schema at all. It's shared by `install/load.sql`'s fresh/update modes (`\i`'d +in the same psql session) and `bin/test_existing`'s `prepare-old` (a +separate, `-f`'d invocation) - the schema-targeting behavior described here +applies to both. The generated name +(`'count_nulls test schema ' || substr(md5(random()::text), 1, 12)`) has two +deliberate properties: - A constant prefix (`count_nulls test schema `, with a trailing space) that by itself already requires SQL identifier quoting - so every single run exercises the suite's `%I`-qualification, not just a dedicated "quoting" leg that could bitrot independently of a "plain" one. - The same prefix doubles as a marker for stale-schema cleanup: before - generating a new name, `install/load.sql` finds and drops any - already-existing schema matching the prefix (`nspname LIKE 'count_nulls - test schema %'`), so a schema left behind by a run that crashed before - reaching its own teardown doesn't accumulate run over run. + generating a new name, `helpers/create_test_schema.sql` finds and drops + any already-existing schema matching the prefix (`nspname LIKE + 'count_nulls test schema %'`), so a schema left behind by a run that + crashed before reaching its own teardown doesn't accumulate run over run. -`install/load.sql` targets the generated schema via `CREATE EXTENSION ... -WITH SCHEMA`, never by mutating its own search_path first. +`helpers/create_test_schema.sql` targets the generated schema via `CREATE +EXTENSION ... WITH SCHEMA`, never by mutating its own search_path first. **Cross-session discovery.** Some scripts/sessions (e.g. `bin/test_existing`'s steps, each a fresh `psql -f ...` invocation with no memory of another @@ -78,13 +86,14 @@ descriptions, which otherwise embed the schema. This is what keeps `test/expected/extension_tests.out` a single file that passes no matter which randomly generated name count_nulls actually landed in. -**`test__shutdown__drop_all`'s schema drop is plain cleanup, not a TAP -assertion.** Dropping the schema count_nulls was installed into isn't -something this suite is testing, just tearing down what `install/load.sql` -created - so it's plain `EXECUTE`'d SQL with no `lives_ok()`/`skip()` -branch. Its output is identical on every run (one `ok` row), so -`test/expected/extension_tests.out` needs no numbered pg_regress alternate -for this function. +**`test__shutdown__drop_all` only asserts the extension can be dropped - +it doesn't clean up the schema itself.** Dropping the schema count_nulls +was installed into isn't something this suite is testing, and +`helpers/create_test_schema.sql` already unconditionally drops any +leftover schema before the next run creates its own, so a second, per-run +drop here would only ever be redundant. Its output is identical on every +run (one `ok` row), so `test/expected/extension_tests.out` needs no +numbered pg_regress alternate for this function. ## Regenerating expected output