From 97cd3e0c9d8a6ff53e0cb585a79cad7fee184a03 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 29 Aug 2026 18:41:21 -0500 Subject: [PATCH 1/3] Pick the test/install load mode server-side, and turn on ON_ERROR_STOP test/install/load.sql's RAISE EXCEPTIONs were decorative. pg_regress resolves its expected and result paths to the same file, so the diff is self-identical no matter what, and a psql that reports an error, carries on and exits 0 leaves pg_regress nothing to notice. ON_ERROR_STOP is what makes the file fail the way its own header says it does. That couldn't just be switched on, because \if is psql 10 and CI covers back to 9.4, where psql reports it as an invalid command - and then, having skipped nothing, runs every branch. On those versions load.sql already behaved badly, silently: the existing-mode assertion fired and errored in every mode, and update mode installed 0.9.6, updated it, then re-entered create_test_schema.sql, whose stale-schema cleanup dropped that schema CASCADE - taking the just-updated extension with it - and installed current fresh instead. `make test TEST_LOAD_SOURCE=update` on 9.4/9.5/9.6 has therefore never once exercised the update path. Under ON_ERROR_STOP the same \if would have aborted those legs outright. So the mode selection moves server-side, into pg_temp.count_nulls_load(), leaving load.sql with no branching of its own - matching what test/helpers/test_user.sql already does, for the same reason. The schema creation and install it shares with bin/test_existing's prepare-old moves to test/helpers/install_extension.sql, which only defines the function; test/helpers/create_test_schema.sql is now just the call, and exists as a file purely because prepare-old invokes it as a standalone psql -f. Drops the client_min_messages suppression that used to wrap the ALTER EXTENSION UPDATE. Postgres already raises it to at least WARNING for the duration of an update script and restores the caller's setting afterwards, so it was redundant, and being unconditional it lowered the level for a caller who had set something stricter. Confirmed: update mode still emits no NOTICEs without it. --- test/README.md | 20 ++- test/helpers/create_test_schema.sql | 77 +++------- test/helpers/extension_installer.sql | 78 ++++++++++ test/helpers/use_test_user.sql | 14 +- test/install/load.sql | 206 ++++++++++++++++----------- 5 files changed, 240 insertions(+), 155 deletions(-) create mode 100644 test/helpers/extension_installer.sql diff --git a/test/README.md b/test/README.md index 08da8a3..db93488 100644 --- a/test/README.md +++ b/test/README.md @@ -6,15 +6,23 @@ Unusually for a pgTAP suite, assertions aren't written per test file: ## Layout +A helper named for an action performs it when `\i`'d; one named for a thing +just defines it, leaving the caller to decide when to use it. + - `install/load.sql` — installs count_nulls once, committed, before the - `test/sql/` schedule. Its output isn't tracked; it fails loudly instead. + `test/sql/` schedule, per `TEST_LOAD_SOURCE`. Its output isn't tracked; it + fails loudly instead. - `deps.sql` — per-test-session setup; drops the session to the test user. - `core/functions.sql` — `ncs()`, plus the shared `test__*` library. - `sql/extension_tests.sql` — adds `test__check_ncs` and `test__shutdown__drop_all`, then calls `runtests()`. -- `helpers/use_test_user.sql` — switches the session to the non-superuser role. -- `helpers/create_test_schema.sql` — installs count_nulls at `:version` into - a fresh, randomly named schema. +- `helpers/use_test_user.sql` — switches the session to the non-superuser + role. +- `helpers/extension_installer.sql` — defines the functions that clean up + leftover test schemas and install count_nulls at a given version into a + fresh, randomly named one. +- `helpers/create_test_schema.sql` — calls both, with `:version`; a file + only because `bin/test_existing`'s `prepare-old` runs it standalone. - `helpers/find_test_schema.sql` — finds that schema again, from a session that didn't create it. - `../bin/compare_fresh_vs_update` — not part of this suite: diffs a fresh @@ -40,6 +48,10 @@ genuinely correct. `test__check_ncs` asserts it. Two consequences: overriding pgTAP's own (which embeds the schema). That's what keeps `expected/extension_tests.out` a single file valid for every run. +Neither `install/load.sql` nor `helpers/use_test_user.sql` may use `\if`: +it's psql 10, CI covers back to 9.4, and under `ON_ERROR_STOP` psql aborts on +it. Both branch server-side instead. + ## Regenerating expected output Never hand-edit `expected/`. Use `make results`, guarded by `make diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 28166ed..2e151b0 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -1,71 +1,32 @@ /* - * Creates a fresh, randomly named schema and installs count_nulls into it. - * Shared by test/install/load.sql (fresh/update modes, same psql session) - * and bin/test_existing's prepare-old (a separate invocation) - the - * creation logic is identical in both, so it lives here once. - */ -SET count_nulls.test_schema_version = :'version'; - -/* - * Bridged through a GUC instead of referencing :'version' directly inside - * the DO block: psql doesn't interpolate variables inside dollar-quoted - * strings. + * Installs count_nulls at :version into a fresh, randomly named schema. + * Exists as a file only because bin/test_existing's prepare-old runs it + * standalone (psql -v version= -f); the work itself lives in + * test/helpers/extension_installer.sql. * - * 'current' means install whatever the current default is, matching the - * sentinel bin/test_existing already uses; empty is a hard error so an - * unpropagated :version can't silently install 'current' instead. + * The install itself runs as a non-superuser (see + * test/helpers/use_test_user.sql), which is what proves count_nulls doesn't + * need superuser to install. Cleanup happens before that switch: a leftover + * schema can belong to any role, and only the connecting one is sure to be + * able to drop it. */ -DO $$ -BEGIN - IF current_setting('count_nulls.test_schema_version') = '' THEN - RAISE EXCEPTION $msg$:version must be set explicitly, or 'current'$msg$; - END IF; -END -$$; +\i test/helpers/extension_installer.sql /* - * A run that crashed before its own teardown leaves a schema nothing else - * knows the name of, so match the prefix (see the name generation below) - * and drop it here - before the test-user switch, since a leftover schema - * can belong to any role and only the connecting one is sure to be able to - * drop it. See find_test_schema.sql for how later sessions rediscover the - * name this creates. + * Always a fresh install here, never 'existing' - see the mode's no-op note + * on count_nulls_cleanup_test_schemas() itself. */ -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 pg_temp.count_nulls_cleanup_test_schemas('fresh'); /* - * :count_nulls_load_mode must already be set by the caller (test/install/ - * load.sql, or bin/test_existing's -v on the command line) - this file - * installs count_nulls, so it can't know on its own whether that's - * genuinely a fresh/update run rather than 'existing'. + * :count_nulls_load_mode must already be set by the caller + * (bin/test_existing's -v on the command line - see the file header) - + * this file only installs, so it can't know on its own whether that's + * genuinely a fresh run rather than 'existing'. */ \i test/helpers/use_test_user.sql -/* - * Trailing space alone forces identifier quoting, so every run exercises - * %I-qualification rather than passing by luck of the random suffix. Must - * match the prefix cleanup matches on above. - */ -SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema -\gset - -CREATE SCHEMA :"schema"; - -SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause +SELECT pg_temp.count_nulls_install_extension(:'version') AS count_nulls_test_schema \gset -/* - * WITH SCHEMA rather than arranging search_path first: this way a - * successful install proves the install script doesn't depend on - * unqualified name resolution, instead of hiding it behind a search_path - * that happened to suit. - */ -CREATE EXTENSION count_nulls WITH SCHEMA :"schema":version_clause; +-- vi: expandtab sw=2 ts=2 diff --git a/test/helpers/extension_installer.sql b/test/helpers/extension_installer.sql new file mode 100644 index 0000000..12cc161 --- /dev/null +++ b/test/helpers/extension_installer.sql @@ -0,0 +1,78 @@ +/* + * Defines, but does not call, the functions that find-and-drop leftover test + * schemas and install count_nulls into a fresh one. Split from calling them, + * and from each other, so test/install/load.sql can decide server-side + * whether to install at all, and so every caller can run cleanup before + * switching to the test user (test/helpers/use_test_user.sql) while + * installing after - a leftover schema can belong to any role, and only the + * connecting one is sure to be able to drop it. + */ +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_cleanup_test_schemas( + p_mode text +) RETURNS void LANGUAGE plpgsql AS $body$ +DECLARE + r record; +BEGIN + /* + * 'existing' asserts against a schema a prior, separate prepare-old run + * left behind on purpose - dropping it here would destroy the very thing + * pg_upgrade is being tested against, so this mode must be a no-op. + */ + IF p_mode = 'existing' THEN + RETURN; + END IF; + + /* + * A run that died before its own teardown leaves a schema nothing else + * knows the name of, so match the prefix (see + * count_nulls_install_extension()'s c_prefix below) and drop whatever's + * there. + */ + 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 +$body$; + +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_install_extension( + p_version text +) RETURNS name LANGUAGE plpgsql AS $body$ +DECLARE + /* + * The trailing space alone forces SQL identifier quoting, so every run + * exercises the suite's %I-qualification rather than passing by luck of + * which characters the random suffix drew. Must match the literal + * count_nulls_cleanup_test_schemas() matches on above. + */ + c_prefix CONSTANT text := 'count_nulls test schema '; + v_schema name; +BEGIN + /* + * 'current' means whatever the control file's default_version is, matching + * the sentinel bin/test_existing already uses. Empty is a hard error, so an + * unpropagated :version can't silently install 'current' instead. + */ + IF p_version = '' THEN + RAISE EXCEPTION $$p_version must be set explicitly, or 'current'$$; + END IF; + + v_schema := c_prefix || substr(md5(random()::text), 1, 12); + EXECUTE format('CREATE SCHEMA %I', v_schema); + + /* + * WITH SCHEMA rather than arranging search_path first: this way a + * successful install proves the install script doesn't depend on + * unqualified name resolution, instead of hiding it behind a search_path + * that happened to suit. + */ + EXECUTE format( + 'CREATE EXTENSION count_nulls WITH SCHEMA %I%s' + , v_schema + , CASE WHEN p_version = 'current' THEN '' ELSE format(' VERSION %L', p_version) END + ); + + RETURN v_schema; +END +$body$; + +-- vi: expandtab sw=2 ts=2 diff --git a/test/helpers/use_test_user.sql b/test/helpers/use_test_user.sql index 6bd01e5..2bf473c 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -7,15 +7,13 @@ * default superuser = true. * * Included from every entry point that starts a session the suite runs in: - * test/deps.sql (each test/sql/ session, via pgxntool's setup.sql) and - * test/helpers/create_test_schema.sql (the install session, and - * bin/test_existing's prepare-old). + * test/deps.sql (each test/sql/ session, via pgxntool's setup.sql), + * test/install/load.sql (the install session) and + * test/helpers/create_test_schema.sql (bin/test_existing's prepare-old). * - * The RESET ROLE below is what makes a second \i of this file in one - * session behave exactly like the first: it hands the privileges back - * before anything that needs them. That isn't hypothetical - psql older - * than 10 has no \if, so test/install/load.sql's mode branches all run and - * create_test_schema.sql gets included twice. + * RESET ROLE first, so what this file does depends only on how the session + * connected and not on anything an earlier \i of it already did - it needs + * the connecting role's privileges back before it can grant anything. */ RESET ROLE; diff --git a/test/install/load.sql b/test/install/load.sql index 919867c..72b0664 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -12,106 +12,142 @@ * from a textual comparison - matching cat_tools' test/install/load.sql. */ +-- CRITICAL: without this an error goes undetected, causing unpredictable results +\set ON_ERROR_STOP on + +-- TODO: this file's mode branching could move back to \if once PG10 is the floor + /* - * Mode selection: 'fresh' installs the current version directly; 'update' - * installs the oldest version we still ship a full script for (0.9.6) and - * runs ALTER EXTENSION UPDATE, committed (this file runs outside any - * per-test rolled-back transaction, unlike the old test/deps.sql approach - - * see pgxntool/README.asc's U&U section for why the commit matters); - * 'existing' asserts count_nulls is already installed (a real `pg_upgrade` - * run, external to this invocation) and touches nothing. - * - * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, - * not be indistinguishable from a deliberately empty one. + * Definitions only - safe to load before the test-user switch below. + * Cleanup runs next, as the connecting role (a leftover schema can belong to + * any role, and only the connecting one is sure to be able to drop it), then + * the switch, then the actual install. */ -SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode - , current_setting('count_nulls.test_load_mode') = 'update' AS count_nulls_update_mode - , current_setting('count_nulls.test_load_mode') = 'existing' AS count_nulls_existing_mode +\i test/helpers/extension_installer.sql + +/* + * count_nulls_load_mode is gset here rather than read again by + * use_test_user.sql itself, for the same reason noted on that file: the + * GUC isn't set for every invocation that \i's it, so every includer must + * supply it explicitly. + */ +SELECT pg_temp.count_nulls_cleanup_test_schemas( + current_setting('count_nulls.test_load_mode') +) AS count_nulls_cleaned_up + , current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode \gset -DO $$ -BEGIN - IF current_setting('count_nulls.test_load_mode') NOT IN ('fresh', 'update', 'existing') THEN - RAISE EXCEPTION - $msg$count_nulls.test_load_mode must be 'fresh', 'update' or 'existing', got '%'$msg$ - , current_setting('count_nulls.test_load_mode') - ; - END IF; -END -$$; +\i test/helpers/use_test_user.sql -\if :count_nulls_existing_mode /* - * Already installed by something external to this pg_regress invocation - * (a real pg_upgrade run, or a pg_tle registration - see the - * pg-upgrade-test / pg-tle-test CI jobs). Only assert it's present and at - * the current version; do NOT drop/create/update it - the whole point of - * this mode is testing the REAL migrated/deployed objects. - * - * The "current version" half of that assertion (v_default below) has two - * sources depending on count_nulls.test_existing_deploy (see the - * TEST_EXISTING_DEPLOY comment in the Makefile): - * - filesystem (default): pg_available_extensions.default_version, read - * straight from a real .control file on disk. - * - pgtle: count_nulls was registered purely through pg_tle's - * database-backed catalog, never touching the filesystem. - * pg_available_extensions does NOT see pg_tle registrations at all - it - * only ever reads .control files off disk - so it comes back NULL here - * even though CREATE EXTENSION correctly resolves the default version - * through pg_tle. pg_tle ships its own separate, non-integrated analog - * for this: pgtle.available_extensions() (see pg_tle's tleextension.c, - * which documents pg_available_extensions as merely modeled on this - * SRF, not backed by it). Use that instead when running under pg_tle, - * rather than weakening the check for the filesystem case. + * Mode selection: 'fresh' installs the current version directly; 'update' + * installs the oldest version we still ship a full script for and runs + * ALTER EXTENSION UPDATE, committed (this file runs outside any per-test + * rolled-back transaction - see pgxntool/README.asc's U&U section for why + * the commit matters); 'existing' asserts count_nulls is already installed + * (a real pg_upgrade run, or a pg_tle registration, external to this + * invocation) and touches nothing. */ -DO $$ +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_load( + p_mode text + , p_deploy text +) RETURNS void LANGUAGE plpgsql AS $body$ DECLARE - v_installed text := (SELECT extversion FROM pg_extension WHERE extname = 'count_nulls'); - v_deploy text := current_setting('count_nulls.test_existing_deploy'); + -- The oldest version we still ship a full install script for + c_oldest_full_install CONSTANT text := '0.9.6'; + v_installed text; v_default text; BEGIN - IF v_installed IS NULL THEN - RAISE EXCEPTION 'count_nulls.test_load_mode=existing but count_nulls is not installed'; - END IF; - - IF v_deploy = 'pgtle' THEN - SELECT default_version INTO v_default - FROM pgtle.available_extensions() WHERE name = 'count_nulls'; - ELSIF v_deploy = 'filesystem' THEN - SELECT default_version INTO v_default - FROM pg_available_extensions WHERE name = 'count_nulls'; - ELSE + IF p_mode NOT IN ('fresh', 'update', 'existing') THEN RAISE EXCEPTION - $msg$count_nulls.test_existing_deploy must be 'filesystem' or 'pgtle', got '%'$msg$ - , v_deploy + $msg$count_nulls.test_load_mode must be 'fresh', 'update' or 'existing', got '%'$msg$ + , p_mode ; END IF; - IF v_installed IS DISTINCT FROM v_default THEN - RAISE EXCEPTION 'count_nulls installed at % but default_version (deploy=%) is %', v_installed, v_deploy, v_default; + IF p_mode = 'existing' THEN + /* + * Already installed by something external to this pg_regress invocation + * (a real pg_upgrade run, or a pg_tle registration - see the + * pg-upgrade-test / pg-tle-test CI jobs). Only assert it's present and + * at the current version; do NOT drop/create/update it - the whole + * point of this mode is testing the REAL migrated/deployed objects. + */ + v_installed := (SELECT extversion FROM pg_extension WHERE extname = 'count_nulls'); + + IF v_installed IS NULL THEN + RAISE EXCEPTION 'count_nulls.test_load_mode=existing but count_nulls is not installed'; + END IF; + + /* + * Where "the current version" comes from depends on how count_nulls got + * here (see the TEST_EXISTING_DEPLOY comment in the Makefile): + * + * - filesystem: pg_available_extensions.default_version, read straight + * from a real .control file on disk. + * - pgtle: count_nulls was registered purely through pg_tle's + * database-backed catalog, never touching the filesystem. + * pg_available_extensions does NOT see pg_tle registrations at all - + * it only ever reads .control files off disk - so it comes back NULL + * even though CREATE EXTENSION correctly resolves the default + * version through pg_tle. pg_tle ships its own separate, + * non-integrated analog: pgtle.available_extensions() (see pg_tle's + * tleextension.c, which documents pg_available_extensions as merely + * modeled on this SRF, not backed by it). Use that here rather than + * weakening the check for the filesystem case. + * + * plpgsql parses a statement only when it first executes, so the pgtle + * reference below costs nothing on a cluster without pg_tle installed. + */ + IF p_deploy = 'pgtle' THEN + SELECT default_version INTO v_default + FROM pgtle.available_extensions() WHERE name = 'count_nulls'; + ELSIF p_deploy = 'filesystem' THEN + SELECT default_version INTO v_default + FROM pg_available_extensions WHERE name = 'count_nulls'; + ELSE + RAISE EXCEPTION + $msg$count_nulls.test_existing_deploy must be 'filesystem' or 'pgtle', got '%'$msg$ + , p_deploy + ; + END IF; + + IF v_installed IS DISTINCT FROM v_default THEN + RAISE EXCEPTION + 'count_nulls installed at % but default_version (deploy=%) is %' + , v_installed, p_deploy, v_default + ; + END IF; + + RETURN; + END IF; + + IF p_mode = 'update' THEN + PERFORM pg_temp.count_nulls_install_extension(c_oldest_full_install); + + /* + * Deliberately no client_min_messages suppression around this. Postgres + * already raises it to at least WARNING for the duration of an update + * script and restores the caller's setting afterwards, so setting it + * here would be redundant - and, being unconditional, would lower the + * level for a caller who had deliberately set something stricter. + */ + ALTER EXTENSION count_nulls UPDATE; + ELSE + PERFORM pg_temp.count_nulls_install_extension('current'); END IF; END -$$; -\else -/* - * 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 -\set version '0.9.6' -\i test/helpers/create_test_schema.sql +$body$; + /* - * Suppress the "already installed, no update" NOTICE class of messages any - * update script might emit. + * Read without missing_ok: a genuinely unpropagated GUC must fail loudly, + * not be indistinguishable from a deliberately empty one. (current_setting's + * missing_ok argument is 9.6 anyway, and CI covers 9.4.) */ -SET client_min_messages = WARNING; -ALTER EXTENSION count_nulls UPDATE; -SET client_min_messages = NOTICE; -\else -\set version 'current' -\i test/helpers/create_test_schema.sql -\endif -\endif +SELECT pg_temp.count_nulls_load( + current_setting('count_nulls.test_load_mode') + , current_setting('count_nulls.test_existing_deploy') + ) AS count_nulls_loaded +\gset + +-- vi: expandtab sw=2 ts=2 From bd1a7b1b06203c087f901df603a03ea7aee3abe2 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 3 Sep 2026 17:39:51 -0500 Subject: [PATCH 2/3] Point create_extension_in_schema()'s comment at extension_installer.sql test/install/load.sql no longer goes through create_test_schema.sql; both it and this script reach the shared install logic via test/helpers/extension_installer.sql, which is also where the rationale now lives. create_test_schema.sql is just the wrapper that calls it, kept as a file so prepare-old can run it standalone. Co-Authored-By: Claude Opus 5 (1M context) --- bin/test_existing | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bin/test_existing b/bin/test_existing index 995b7a9..154afd9 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -183,9 +183,11 @@ update_ext() { # CREATE EXTENSION count_nulls at VERSION, into a freshly, randomly # 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). This invocation sets no count_nulls.test_load_mode GUC, -# so count_nulls_load_mode is supplied here instead: it's a genuine fresh +# installs via test/helpers/extension_installer.sql (see that file for the +# full rationale). create_test_schema.sql is a thin wrapper that calls it, +# and is a file only so this script can invoke it standalone. This +# invocation sets no count_nulls.test_load_mode GUC, so +# count_nulls_load_mode is supplied here instead: it's a genuine fresh # install (prepare-old), never 'existing'. create_extension_in_schema() { local db=$1 version=$2 From a068c291167981bd82b00f2f2b3402e4ff2d1a74 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 3 Sep 2026 17:48:33 -0500 Subject: [PATCH 3/3] Document the non-atomic check-then-create for the test role Note in a comment that the CREATE ROLE guard in count_nulls_prepare_test_user() is not atomic, and why that's safe today: load.sql finishes before pg_regress starts the concurrent test/sql/ sessions, so nothing races to create the role. --- test/helpers/use_test_user.sql | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/helpers/use_test_user.sql b/test/helpers/use_test_user.sql index 2bf473c..b8e1fad 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -100,6 +100,10 @@ BEGIN END IF; IF c_admin THEN + /* + * Check-then-create, not atomic - safe only because test/install/load.sql + * finishes before pg_regress starts the concurrent test/sql/ sessions. + */ IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN EXECUTE format('CREATE ROLE %I', p_test_user); END IF;