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 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..b8e1fad 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; @@ -102,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; 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