From a73047936e8df7dbb34861bd733b7b4470a6e400 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 29 Aug 2026 17:44:53 -0500 Subject: [PATCH 01/10] Install count_nulls without superuser, and prove it in the suite count_nulls is nothing but SQL functions, so requiring a superuser to CREATE EXTENSION it was never anything more than the control file's default. Set superuser = false. For that to stay true, the suite now runs as an ordinary role throughout - both the install session and every test session. test/helpers/test_user.sql creates "Test user for count_nulls" if it isn't there, grants it exactly two privileges (CREATE on the database, and USAGE on schema tap, since pgTAP is harness and pgxntool's setup.sql creates that schema as the connecting role, which grants nobody else access), and raises if the role is a superuser or a member of any managed-cloud equivalent - rds_superuser, cloudsqlsuperuser or azure_pg_admin, none of which carry rolsuper. Reverting superuser = false now fails the suite outright, with "permission denied to create extension". Every decision is made server-side, by a pg_temp function taking the role name and returning the role the session should run as, because psql can't branch before 10: \if is psql 10 and CI covers back to 9.4, where psql reports it as an invalid command and then runs the branch it should have skipped. \gset feeds that returned name into a plain SET ROLE. The file opens with RESET ROLE so a second \i in one session behaves like the first - not hypothetical, since on psql older than 10 install/load.sql's mode branches all run and this file gets reached twice. One case deliberately doesn't switch: a count_nulls already installed and owned by somebody else, which is what a real pg_upgrade leaves behind. PostgreSQL has no ALTER EXTENSION ... OWNER TO, so pg_dump can't carry an extension's ownership across - --binary-upgrade emits binary_upgrade_create_empty_extension(), which takes no owner, and the extension ends up belonging to whoever ran the restore. Member functions keep their owner; only the extension object is out of reach, which is exactly what test__shutdown__drop_all needs. Nothing is lost by staying put: existing mode installs nothing, so it was never the leg proving the install works unprivileged. Also fixes a live bug in install/load.sql's existing-mode assertion, whose nested $$-quoted RAISE message closed its own DO block's body, leaving the rest to be parsed as bare SQL - the whole assertion was a no-op. Fixed with a distinct tag. It stays silent for now because that file has no ON_ERROR_STOP; turning that on needs its \if branching to go server-side first, which is a separate change. --- HISTORY.md | 6 +- README.md | 7 +- count_nulls.control | 5 + test/README.md | 64 +++++++++++- test/deps.sql | 27 ++--- test/helpers/create_test_schema.sql | 8 ++ test/helpers/test_user.sql | 146 ++++++++++++++++++++++++++++ test/install/load.sql | 7 +- 8 files changed, 250 insertions(+), 20 deletions(-) create mode 100644 test/helpers/test_user.sql diff --git a/HISTORY.md b/HISTORY.md index f96bf99..76c6d30 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,7 +3,11 @@ stable ------ -No user-facing changes yet. +== CREATE EXTENSION no longer requires a superuser +count_nulls is pure SQL functions, so the control file now says +`superuser = false`: any role with `CREATE` on the target schema can install +it. The test suite runs as an ordinary role throughout, so this can't +silently regress. 1.0.0 ----- diff --git a/README.md b/README.md index 9a9e623..cca31ee 100644 --- a/README.md +++ b/README.md @@ -47,11 +47,14 @@ You need to run the test suite using a super user, such as the default make test PGUSER=postgres Once count_nulls is installed, you can add it to a database. If you're running -PostgreSQL 9.1.0 or greater, it's a simple as connecting to a database as a -super user and running: +PostgreSQL 9.1.0 or greater, it's as simple as connecting to a database and +running: CREATE EXTENSION count_nulls; +No superuser required: count_nulls is nothing but SQL functions, so any role +with `CREATE` on the target schema can add it. + If you've upgraded your cluster to PostgreSQL 9.1 and already had count_nulls installed, you can upgrade it to a properly packaged extension with: diff --git a/count_nulls.control b/count_nulls.control index bf8f662..998df78 100644 --- a/count_nulls.control +++ b/count_nulls.control @@ -2,3 +2,8 @@ comment = 'Count the number of null arguments' default_version = 'stable' relocatable = false +# Pure SQL functions, nothing privileged - anyone with CREATE on the target +# schema can install it. Enforced by the suite running as a non-superuser +# (test/helpers/test_user.sql), which fails outright if this reverts to the +# default. +superuser = false diff --git a/test/README.md b/test/README.md index 64219cc..4198b19 100644 --- a/test/README.md +++ b/test/README.md @@ -14,10 +14,10 @@ then invoke via `runtests()`. comes from this file failing loudly if something's wrong, not from a textual comparison. - `deps.sql` — loaded by every test file (via `load.sql` -> - `pgxntool/setup.sql` -> `deps.sql`). No longer installs count_nulls - itself (that's `install/load.sql`'s job); only for genuine per-test - dependency statements. Currently empty - see its own header comment for - why it's kept that way rather than deleted. + `pgxntool/setup.sql` -> `deps.sql`). Doesn't install count_nulls (that's + `install/load.sql`'s job); its only job is `\i`ing `helpers/test_user.sql` + so each test session runs unprivileged (see "Running as a non-superuser" + below). - `core/functions.sql` — a shared helper, `\i`'d by `sql/extension_tests.sql`. Defines `ncs()` (discovers, live, which schema count_nulls is actually installed in - never trusts a hardcoded/passed-in value) plus a battery of @@ -52,6 +52,62 @@ then invoke 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. +- `helpers/test_user.sql` — drops the session to a non-superuser role (see + "Running as a non-superuser" below). `\i`'d by `deps.sql` and + `helpers/create_test_schema.sql`. + +## Running as a non-superuser + +Every session the suite runs in - the install session and each test session +- runs as an ordinary role, `Test user for count_nulls`. That's what makes +`superuser = false` in `count_nulls.control` a tested property rather than a +claim: count_nulls is pure SQL functions with nothing privileged in it, and +a suite running as a superuser could never notice that line going missing. + +`helpers/test_user.sql` owns the whole arrangement, and is `\i`'d from both +entry points that start such a session: `deps.sql` (every `test/sql/` file, +via pgxntool's `setup.sql`) and `helpers/create_test_schema.sql` (the +install session, and `bin/test_existing`'s `prepare-old`). + +The role's name is spelled exactly once, as a psql variable at the top of +that file. Like the generated schema name it can't be written without SQL +identifier quoting, and it's deliberately sentence-like so it won't collide +with a real role on whatever cluster someone points the suite at. + +**Every decision is server-side**, in a `pg_temp` function taking the role +name and *returning the role this session should run as*, because psql can't +branch before 10: `\if` is psql 10, and CI covers back to 9.4, where psql +reports it as an invalid command and then runs the branch it should have +skipped. (`\gset`, which feeds the returned name into the `SET ROLE`, is fine +- 9.3.) The function creates the role if it's missing, grants it what it +needs, and raises if the role is a superuser or a member of any managed-cloud +equivalent - `rds_superuser` (RDS/Aurora), `cloudsqlsuperuser` (Cloud SQL) or +`azure_pg_admin` (Azure Flexible Server), none of which carry `rolsuper`, so +they have to be named. The `SET ROLE` itself is a plain statement after that +call, not something the function does. + +**One case deliberately doesn't switch**: a count_nulls that's already +installed and owned by somebody else, which is what a real `pg_upgrade` +leaves behind (existing mode). PostgreSQL has no `ALTER EXTENSION ... OWNER +TO`, so pg_dump can't carry an extension's ownership across - `--binary- +upgrade` emits `binary_upgrade_create_empty_extension()`, which takes no +owner, and the extension ends up belonging to whoever ran the restore. Its +member functions keep their owner; only the extension object itself is out of +reach, which is precisely what `test__shutdown__drop_all` needs. Switching +there would just hand the suite a role that can't drop the extension, and +nothing is lost by staying put: existing mode installs nothing, so it was +never the leg proving the install works unprivileged. + +The file opens with `RESET ROLE` so a second `\i` in one session behaves +exactly like the first - which is not hypothetical, since on psql older than +10 `install/load.sql`'s mode branches all run and this file gets reached +twice. + +Only two privileges are granted: `CREATE` on the database (count_nulls' own +schema, and `_null_count_test`) and `USAGE` on schema `tap` (pgTAP is +harness, and `setup.sql` creates that schema as the connecting role, which +grants nobody else access). Anything beyond those turning out to be +necessary is a finding about count_nulls, not something to grant here. ## Schema targeting diff --git a/test/deps.sql b/test/deps.sql index 231a50c..b04b8f2 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -1,16 +1,19 @@ /* - * Intentionally empty. test/install/load.sql now installs count_nulls once, - * committed, before test/sql/ runs (see its header comment), so this - * per-test file no longer has anything to do. + * Per-test-session setup, run by pgxntool/test/pgxntool/setup.sql (vendored, + * never hand-edited) for every file under test/sql/. It does NOT install + * count_nulls - test/install/load.sql does that once, committed, before + * test/sql/ runs (see its header comment). * - * Can't be deleted: pgxntool/test/pgxntool/setup.sql (vendored, never - * hand-edited) unconditionally does `\i test/deps.sql`, so every test - * session's setup would fail without it. It's also one of only two files - * (.gitignore, test/deps.sql) that pgxntool's subtree-sync reconciliation - * tracks and 3-way-merges on every `git subtree pull`. + * The one thing it does do is drop the session's privileges, so the suite + * proper runs as a non-superuser too and not just the install does. This is + * the last hook that runs before the test file's own SQL, and it runs after + * setup.sql has already created the tap schema and pgtap as the connecting + * (superuser) role - pgTAP is test harness, not something count_nulls' + * privileges have anything to say about. * - * Kept for future use: add per-test dependency statements here again if a - * genuine need arises - e.g. relaying a value into the per-test session via - * a psql variable - same role this file played before test/install took - * over installing count_nulls. + * Also can't be deleted regardless: setup.sql unconditionally `\i`s it, and + * it's one of only two files (.gitignore, test/deps.sql) that pgxntool's + * subtree-sync reconciliation tracks and 3-way-merges on every + * `git subtree pull`. */ +\i test/helpers/test_user.sql diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index bd236e7..8eb6025 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -36,7 +36,15 @@ * 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. + * + * Everything below runs as a non-superuser (see test/helpers/test_user.sql), + * so a successful install here is itself the proof that count_nulls doesn't + * need superuser to install. The switch has to come before the cleanup loop, + * not just before CREATE EXTENSION: a leftover schema is one this same role + * created on a previous run, so it's the role that must be able to drop it. */ +\i test/helpers/test_user.sql + SET count_nulls.test_schema_version = :'version'; DO $$ diff --git a/test/helpers/test_user.sql b/test/helpers/test_user.sql new file mode 100644 index 0000000..c4dbde8 --- /dev/null +++ b/test/helpers/test_user.sql @@ -0,0 +1,146 @@ +/* + * Switches the session to a non-superuser role, so everything that follows + * (installing count_nulls, and the pgTAP suite itself) only passes if it + * genuinely works without superuser rights. count_nulls is pure SQL + * functions - nothing in it needs superuser - so a suite that silently ran + * as one could never notice count_nulls.control regressing back to the + * 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). + * + * 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; + +/* + * The one and only definition of the test user's name. Deliberately + * sentence-like: unlikely to collide with a real role on a cluster someone + * points the suite at, and (like the generated schema name) it can't be + * spelled without SQL identifier quoting, so every run exercises that. + */ +\set test_user 'Test user for count_nulls' + +/* + * Every decision is made server-side, by a function taking the role name and + * returning the role this session should actually run as, because psql has + * no way to branch before 10: \if is psql 10, and CI covers back to 9.4, + * where psql reports it as an invalid command and then carries straight on + * into the branch it should have skipped. (\gset, below, is fine - 9.3.) + * + * TODO: collapse this into a \gset + \if once 10 is the oldest version + * supported - the plpgsql is only here to work around \if's absence. + */ +CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_user( + p_test_user name +) RETURNS name LANGUAGE plpgsql AS $body$ +DECLARE + /* + * The managed-cloud analogues of superuser. None of them carry the + * rolsuper attribute - that's the whole point of them - so the rolsuper + * check below can't see them and they have to be named explicitly. AWS + * RDS and Aurora call it rds_superuser, Cloud SQL cloudsqlsuperuser, + * Azure Flexible Server azure_pg_admin. + */ + c_managed_superuser_roles CONSTANT name[] := + '{rds_superuser,cloudsqlsuperuser,azure_pg_admin}' + ; + v_role name; + v_extension_owner CONSTANT name := ( + SELECT pg_get_userbyid(extowner) FROM pg_extension WHERE extname = 'count_nulls' + ); +BEGIN + /* + * An already-installed count_nulls belonging to somebody else is one a + * real pg_upgrade restored, and it can only be managed by its owner - so + * stay as we are rather than switching to a role that can't drop it. + * + * PostgreSQL has no ALTER EXTENSION ... OWNER TO, so pg_dump has no way + * to carry an extension's ownership across: --binary-upgrade emits + * binary_upgrade_create_empty_extension(), which takes no owner, and the + * extension ends up belonging to whoever ran the restore. Its member + * functions DO keep their owner, so only the extension object itself is + * out of reach - which is exactly what test__shutdown__drop_all needs. + * + * Nothing is lost by not switching here: existing mode installs nothing, + * so it was never the leg proving the install works unprivileged. + */ + IF v_extension_owner IS NOT NULL AND v_extension_owner <> p_test_user THEN + RETURN current_user; + END IF; + + IF current_setting('is_superuser') = 'on' THEN + IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN + EXECUTE format('CREATE ROLE %I', p_test_user); + END IF; + + /* + * CREATE on the database is the one privilege the suite can't do + * without: create_test_schema.sql creates count_nulls' own schema, and + * core/functions.sql creates _null_count_test. USAGE on tap is pgTAP, + * which is harness rather than subject matter - pgxntool's setup.sql + * creates that schema as the connecting role, and a fresh schema grants + * nobody else access, so without this runtests() is simply invisible. + * Nothing else is granted: anything further turning out to be necessary + * is a finding about count_nulls, not something to paper over here. + */ + EXECUTE format( + 'GRANT CREATE ON DATABASE %I TO %I', current_database(), p_test_user + ); + + IF EXISTS(SELECT 1 FROM pg_namespace WHERE nspname = 'tap') THEN + EXECUTE format('GRANT USAGE ON SCHEMA tap TO %I', p_test_user); + END IF; + END IF; + + IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN + RAISE EXCEPTION + 'role "%" does not exist, and this session is not a superuser so it cannot be created' + , p_test_user + ; + END IF; + + IF (SELECT rolsuper FROM pg_roles WHERE rolname = p_test_user) THEN + RAISE EXCEPTION 'test user "%" must not be a superuser', p_test_user; + END IF; + + FOREACH v_role IN ARRAY c_managed_superuser_roles LOOP + /* + * Whether the role exists has to be settled in its own statement: + * pg_has_role() errors outright on one that doesn't, and SQL promises + * no evaluation order between the two halves of an AND. + * + * MEMBER, not USAGE: being able to SET ROLE to one of these is just as + * disqualifying as inheriting it outright. + */ + IF EXISTS(SELECT 1 FROM pg_roles WHERE rolname = v_role) THEN + IF pg_has_role(p_test_user, v_role, 'MEMBER') THEN + RAISE EXCEPTION + 'test user "%" must not be granted %', p_test_user, v_role; + END IF; + END IF; + END LOOP; + + RETURN p_test_user; +END +$body$; + +/* + * \gset, not \if, is what keeps the SET ROLE below both unconditional and + * correct: the function returns whichever role this session should run as, + * so the decision stays server-side while the switch itself stays an + * ordinary statement. It also keeps the function's result out of every + * test's expected output. + */ +SELECT pg_temp.count_nulls_prepare_test_user(:'test_user') AS count_nulls_run_as +\gset + +SET ROLE :"count_nulls_run_as"; + +-- vi: expandtab sw=2 ts=2 diff --git a/test/install/load.sql b/test/install/load.sql index 5e186b5..2f5e889 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -81,8 +81,13 @@ BEGIN SELECT default_version INTO v_default FROM pg_available_extensions WHERE name = 'count_nulls'; ELSE + /* + * A distinct tag. Reusing this DO block's own (untagged) delimiter + * would close its body right here, leaving the remainder to be parsed + * as bare SQL - which is also why this comment can't spell it out. + */ RAISE EXCEPTION - $$count_nulls.test_existing_deploy must be 'filesystem' or 'pgtle', got '%'$$ + $msg$count_nulls.test_existing_deploy must be 'filesystem' or 'pgtle', got '%'$msg$ , v_deploy ; END IF; From b08f3c0f1534f9cfe56d9a86eefe7e30b3fb96f3 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sat, 29 Aug 2026 19:08:06 -0500 Subject: [PATCH 02/10] Note that losing extension ownership on pg_upgrade is an upstream shortcoming The carve-off for an extension owned by somebody else reads like something that ought to be fixable, or like something a newer PostgreSQL would make unnecessary. It isn't either: extowner has had no matching ALTER EXTENSION ... OWNER TO since it was added in 2011, because what that should do to the contained objects was never settled, and it's been reported (BUG #18625) and acknowledged as a known shortcoming without being fixed. --- test/README.md | 8 ++++++++ test/helpers/test_user.sql | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/test/README.md b/test/README.md index 4198b19..0c6288d 100644 --- a/test/README.md +++ b/test/README.md @@ -98,6 +98,14 @@ there would just hand the suite a role that can't drop the extension, and nothing is lost by staying put: existing mode installs nothing, so it was never the leg proving the install works unprivileged. +Don't expect a newer PostgreSQL to retire that branch. `extowner` has had no +matching `ALTER EXTENSION ... OWNER TO` since it was added in 2011, because +what that should do to the contained objects was never settled (handing a +non-superuser a C-language handler function is the awkward case). Reported as +BUG #18625 and acknowledged as a known shortcoming, still unfixed. +`pg_dump --use-set-session-authorization` does dodge it, but `pg_upgrade` +offers no way to ask for that. + The file opens with `RESET ROLE` so a second `\i` in one session behaves exactly like the first - which is not hypothetical, since on psql older than 10 `install/load.sql`'s mode branches all run and this file gets reached diff --git a/test/helpers/test_user.sql b/test/helpers/test_user.sql index c4dbde8..ea71ded 100644 --- a/test/helpers/test_user.sql +++ b/test/helpers/test_user.sql @@ -68,6 +68,14 @@ BEGIN * functions DO keep their owner, so only the extension object itself is * out of reach - which is exactly what test__shutdown__drop_all needs. * + * Don't expect a newer PostgreSQL to retire this branch. extowner has had + * no matching ALTER EXTENSION ... OWNER TO since it was added in 2011, + * because what that should do to the contained objects was never settled + * (handing a non-superuser a C-language handler function is the awkward + * case). Reported as BUG #18625 and acknowledged as a known shortcoming, + * still unfixed. pg_dump's --use-set-session-authorization does dodge it, + * but pg_upgrade offers no way to ask for that. + * * Nothing is lost by not switching here: existing mode installs nothing, * so it was never the leg proving the install works unprivileged. */ From f799ea77036bb3e3b70381885094fd4ce97321aa Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 30 Aug 2026 16:32:58 -0500 Subject: [PATCH 03/10] Name the extension-owner constant c_, not v_ It's declared CONSTANT, so it takes the constant prefix, matching c_managed_superuser_roles right above it. v_role stays as it is - that one really is a loop variable. --- test/helpers/test_user.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/helpers/test_user.sql b/test/helpers/test_user.sql index ea71ded..34f683d 100644 --- a/test/helpers/test_user.sql +++ b/test/helpers/test_user.sql @@ -52,7 +52,7 @@ DECLARE '{rds_superuser,cloudsqlsuperuser,azure_pg_admin}' ; v_role name; - v_extension_owner CONSTANT name := ( + c_extension_owner CONSTANT name := ( SELECT pg_get_userbyid(extowner) FROM pg_extension WHERE extname = 'count_nulls' ); BEGIN @@ -79,7 +79,7 @@ BEGIN * Nothing is lost by not switching here: existing mode installs nothing, * so it was never the leg proving the install works unprivileged. */ - IF v_extension_owner IS NOT NULL AND v_extension_owner <> p_test_user THEN + IF c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user THEN RETURN current_user; END IF; From a1903110919773af93db494d4c60ff1f5cb40cd2 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 30 Aug 2026 16:52:43 -0500 Subject: [PATCH 04/10] Trim test docs and comments to what the code doesn't already say test/README.md had grown into a paraphrase of the comments in the files it describes, so it's now a layout list plus the two cross-cutting facts a reader needs before touching anything - the non-superuser role and the randomly named schema - each pointing at the file that explains itself. test/deps.sql said it was intentionally empty, which stopped being true when it took over switching to the test user, and spent a paragraph restating pgxntool's own docs. It now points at them instead. Records why the database grant can't move into the CREATE ROLE beside it: the role is cluster-wide and outlives a run, but the grant lives in the current database's ACL, so a run against a new database has to reissue it. Corrects the MEMBER-vs-USAGE note, which claimed being able to SET ROLE to a role is equivalent to holding its privileges. It isn't; the check is deliberately over-strict, and stays that way because nothing needs looser. --- test/README.md | 221 +++++++------------------------------ test/deps.sql | 19 +--- test/helpers/test_user.sql | 25 +++-- 3 files changed, 59 insertions(+), 206 deletions(-) diff --git a/test/README.md b/test/README.md index 0c6288d..744ec08 100644 --- a/test/README.md +++ b/test/README.md @@ -1,188 +1,49 @@ # count_nulls test suite -This suite is structured differently from most pgTAP-based extension tests: -rather than each `test/sql/*.sql` file writing its own independent -assertions, `core/functions.sql` defines a shared library of `test__*` -functions (pgTAP's `runtests()` naming convention) that test files `\i` and -then invoke via `runtests()`. +Unusually for a pgTAP suite, assertions aren't written per test file: +`core/functions.sql` defines a shared library of `test__*` functions that +`sql/extension_tests.sql` `\i`s and then runs via `runtests()`. ## Layout -- `install/load.sql` — installs count_nulls once, committed, before the main - `test/sql/` schedule (see pgxntool/README.asc's `test/install` section). - Its own output isn't tracked (see `install/.gitignore`) - correctness - comes from this file failing loudly if something's wrong, not from a - textual comparison. -- `deps.sql` — loaded by every test file (via `load.sql` -> - `pgxntool/setup.sql` -> `deps.sql`). Doesn't install count_nulls (that's - `install/load.sql`'s job); its only job is `\i`ing `helpers/test_user.sql` - so each test session runs unprivileged (see "Running as a non-superuser" - below). -- `core/functions.sql` — a shared helper, `\i`'d by `sql/extension_tests.sql`. - Defines `ncs()` (discovers, live, which schema count_nulls is actually - installed in - never trusts a hardcoded/passed-in value) plus a battery of - `test__*` functions covering function definitions, immutability/ - strictness, and behavior across `anyarray`/`json`/`jsonb` and both - trigger functions. -- `../bin/compare_fresh_vs_update` — not part of the pgTAP suite itself: a - standalone script the `test` CI job's update leg runs after - `TEST_LOAD_SOURCE=update`, which installs a fresh copy and a - 0.9.6-then-updated copy of the extension into their own scratch - databases - each an unqualified `CREATE EXTENSION`, so both land in the - same default schema by construction, which is all the diff needs to - isolate real update-vs-fresh divergence rather than a spurious - schema-name difference - and diffs `pg_get_functiondef`/comments/ACLs for - every object the extension owns. Catches an update script leaving some - definition subtly different from a fresh install, even when the fixed - pgTAP suite above still passes (it only asserts the specific behaviors it - happens to check). The `pg-upgrade-test` CI job also reuses it (via its - optional `EXISTING_DB` argument) to compare a fresh install against the - real, already-populated databases a binary `pg_upgrade` just produced, - discovering and matching that database's own randomly generated schema - (from `helpers/create_test_schema.sql`, via `bin/test_existing - prepare-old`) instead of generating a new one. -- `sql/extension_tests.sql` — `\i`'s `core/functions.sql`, adds two more - `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. -- `helpers/test_user.sql` — drops the session to a non-superuser role (see - "Running as a non-superuser" below). `\i`'d by `deps.sql` and - `helpers/create_test_schema.sql`. - -## Running as a non-superuser - -Every session the suite runs in - the install session and each test session -- runs as an ordinary role, `Test user for count_nulls`. That's what makes -`superuser = false` in `count_nulls.control` a tested property rather than a -claim: count_nulls is pure SQL functions with nothing privileged in it, and -a suite running as a superuser could never notice that line going missing. - -`helpers/test_user.sql` owns the whole arrangement, and is `\i`'d from both -entry points that start such a session: `deps.sql` (every `test/sql/` file, -via pgxntool's `setup.sql`) and `helpers/create_test_schema.sql` (the -install session, and `bin/test_existing`'s `prepare-old`). - -The role's name is spelled exactly once, as a psql variable at the top of -that file. Like the generated schema name it can't be written without SQL -identifier quoting, and it's deliberately sentence-like so it won't collide -with a real role on whatever cluster someone points the suite at. - -**Every decision is server-side**, in a `pg_temp` function taking the role -name and *returning the role this session should run as*, because psql can't -branch before 10: `\if` is psql 10, and CI covers back to 9.4, where psql -reports it as an invalid command and then runs the branch it should have -skipped. (`\gset`, which feeds the returned name into the `SET ROLE`, is fine -- 9.3.) The function creates the role if it's missing, grants it what it -needs, and raises if the role is a superuser or a member of any managed-cloud -equivalent - `rds_superuser` (RDS/Aurora), `cloudsqlsuperuser` (Cloud SQL) or -`azure_pg_admin` (Azure Flexible Server), none of which carry `rolsuper`, so -they have to be named. The `SET ROLE` itself is a plain statement after that -call, not something the function does. - -**One case deliberately doesn't switch**: a count_nulls that's already -installed and owned by somebody else, which is what a real `pg_upgrade` -leaves behind (existing mode). PostgreSQL has no `ALTER EXTENSION ... OWNER -TO`, so pg_dump can't carry an extension's ownership across - `--binary- -upgrade` emits `binary_upgrade_create_empty_extension()`, which takes no -owner, and the extension ends up belonging to whoever ran the restore. Its -member functions keep their owner; only the extension object itself is out of -reach, which is precisely what `test__shutdown__drop_all` needs. Switching -there would just hand the suite a role that can't drop the extension, and -nothing is lost by staying put: existing mode installs nothing, so it was -never the leg proving the install works unprivileged. - -Don't expect a newer PostgreSQL to retire that branch. `extowner` has had no -matching `ALTER EXTENSION ... OWNER TO` since it was added in 2011, because -what that should do to the contained objects was never settled (handing a -non-superuser a C-language handler function is the awkward case). Reported as -BUG #18625 and acknowledged as a known shortcoming, still unfixed. -`pg_dump --use-set-session-authorization` does dodge it, but `pg_upgrade` -offers no way to ask for that. - -The file opens with `RESET ROLE` so a second `\i` in one session behaves -exactly like the first - which is not hypothetical, since on psql older than -10 `install/load.sql`'s mode branches all run and this file gets reached -twice. - -Only two privileges are granted: `CREATE` on the database (count_nulls' own -schema, and `_null_count_test`) and `USAGE` on schema `tap` (pgTAP is -harness, and `setup.sql` creates that schema as the connecting role, which -grants nobody else access). Anything beyond those turning out to be -necessary is a finding about count_nulls, not something to grant here. - -## Schema targeting - -`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, `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. - -`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 -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` -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 passes no matter -which randomly generated name count_nulls actually landed in. - -**`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. +- `install/load.sql` — installs count_nulls once, committed, before the + `test/sql/` schedule. 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/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/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 + install against an updated one (definitions, comments, ACLs). + +## Two things to know before changing anything + +**The suite runs as an ordinary role, not a superuser** (`Test user for +count_nulls`). That's what makes `superuser = false` in +`count_nulls.control` a tested property rather than a claim. See +`helpers/test_user.sql`, including why it deliberately does *not* switch +when a real `pg_upgrade` has left the extension owned by someone else. + +**count_nulls is installed into a randomly named schema**, never a fixed one +and never the default. That's what gives `core/functions.sql`'s +`%I`-qualified calls their meaning: the extension's schema can never +coincidentally land on `search_path`, so those calls only pass if they're +genuinely correct. `test__check_ncs` asserts it. Two consequences: + +- Nothing may assume the name — use `ncs()` from SQL, or + `helpers/find_test_schema.sql` from a separate session. +- Assertions pass an explicit, schema-free description to every pgTAP call, + overriding pgTAP's own (which embeds the schema). That's what keeps + `expected/extension_tests.out` a single file valid for every run. ## Regenerating expected output -Never hand-edit files under `expected/`. Regenerate via `make results` -(guarded by `make verify-results`, which refuses to copy while -`regression.diffs` shows real failures - use -`PGXNTOOL_ENABLE_VERIFY_RESULTS=no` to bypass that guard for a run you've -already reviewed and know is a legitimate, intentional change, not a way to -skip reviewing the diff). `make results` only ever writes the unsuffixed -default; alternates (`_1.out`, ...) have to be copied by hand from a real -`test/results/.out` for that scenario. +Never hand-edit `expected/`. Use `make results`, guarded by `make +verify-results` (which refuses while `regression.diffs` shows real failures; +`PGXNTOOL_ENABLE_VERIFY_RESULTS=no` bypasses it for a diff you've already +reviewed). It only writes the unsuffixed default — alternates (`_1.out`, …) +must be copied by hand from `test/results/.out`. diff --git a/test/deps.sql b/test/deps.sql index b04b8f2..42a9bc6 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -1,19 +1,6 @@ /* - * Per-test-session setup, run by pgxntool/test/pgxntool/setup.sql (vendored, - * never hand-edited) for every file under test/sql/. It does NOT install - * count_nulls - test/install/load.sql does that once, committed, before - * test/sql/ runs (see its header comment). - * - * The one thing it does do is drop the session's privileges, so the suite - * proper runs as a non-superuser too and not just the install does. This is - * the last hook that runs before the test file's own SQL, and it runs after - * setup.sql has already created the tap schema and pgtap as the connecting - * (superuser) role - pgTAP is test harness, not something count_nulls' - * privileges have anything to say about. - * - * Also can't be deleted regardless: setup.sql unconditionally `\i`s it, and - * it's one of only two files (.gitignore, test/deps.sql) that pgxntool's - * subtree-sync reconciliation tracks and 3-way-merges on every - * `git subtree pull`. + * Per-test-session setup: pgxntool's setup.sql `\i`s this for every file + * under test/sql/. See pgxntool/README.asc's "test/install" section for why + * installing count_nulls is not done here. */ \i test/helpers/test_user.sql diff --git a/test/helpers/test_user.sql b/test/helpers/test_user.sql index 34f683d..62a494a 100644 --- a/test/helpers/test_user.sql +++ b/test/helpers/test_user.sql @@ -89,19 +89,21 @@ BEGIN END IF; /* - * CREATE on the database is the one privilege the suite can't do - * without: create_test_schema.sql creates count_nulls' own schema, and - * core/functions.sql creates _null_count_test. USAGE on tap is pgTAP, - * which is harness rather than subject matter - pgxntool's setup.sql - * creates that schema as the connecting role, and a fresh schema grants - * nobody else access, so without this runtests() is simply invisible. - * Nothing else is granted: anything further turning out to be necessary - * is a finding about count_nulls, not something to paper over here. + * Can't fold into the CREATE ROLE above: the role is cluster-wide and + * outlives any one run, but this grant lives in the current database's + * ACL, so a run against a new database still has to issue it. + * + * These two grants are all the suite gets. Anything else turning out to + * be necessary is a finding about count_nulls, not something to grant. */ EXECUTE format( 'GRANT CREATE ON DATABASE %I TO %I', current_database(), p_test_user ); + /* + * setup.sql creates schema tap as the connecting role, which grants + * nobody else USAGE - without this, runtests() is invisible. + */ IF EXISTS(SELECT 1 FROM pg_namespace WHERE nspname = 'tap') THEN EXECUTE format('GRANT USAGE ON SCHEMA tap TO %I', p_test_user); END IF; @@ -124,8 +126,11 @@ BEGIN * pg_has_role() errors outright on one that doesn't, and SQL promises * no evaluation order between the two halves of an AND. * - * MEMBER, not USAGE: being able to SET ROLE to one of these is just as - * disqualifying as inheriting it outright. + * MEMBER, not USAGE, is deliberately over-strict: it also rejects a + * role that merely *could* SET ROLE to one of these without ever having + * done so, which isn't the same as holding the privileges. Simpler than + * reasoning about when it would actually matter, and nothing needs the + * looser check yet. */ IF EXISTS(SELECT 1 FROM pg_roles WHERE rolname = v_role) THEN IF pg_has_role(p_test_user, v_role, 'MEMBER') THEN From 8b32f152fd6a43fa7fb26b86d5c3ae4f0d9c1437 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 15:17:11 -0500 Subject: [PATCH 05/10] Drop two comments, and name the helper for what running it does test/helpers/test_user.sql becomes use_test_user.sql. The other helpers are named for the action running them performs (create_test_schema, find_test_schema); this one performed an action but read as a noun. Drops the comment above the SET ROLE, which restated what the two statements under it plainly do, and the one explaining why a nested RAISE message needs its own dollar-quote tag - that's general plpgsql knowledge, not something this site needs to teach. --- test/README.md | 4 ++-- test/deps.sql | 2 +- test/helpers/create_test_schema.sql | 4 ++-- test/helpers/{test_user.sql => use_test_user.sql} | 7 ------- test/install/load.sql | 5 ----- 5 files changed, 5 insertions(+), 17 deletions(-) rename test/helpers/{test_user.sql => use_test_user.sql} (95%) diff --git a/test/README.md b/test/README.md index 744ec08..08da8a3 100644 --- a/test/README.md +++ b/test/README.md @@ -12,7 +12,7 @@ Unusually for a pgTAP suite, assertions aren't written per test file: - `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/test_user.sql` — switches the session to the non-superuser role. +- `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/find_test_schema.sql` — finds that schema again, from a session @@ -25,7 +25,7 @@ Unusually for a pgTAP suite, assertions aren't written per test file: **The suite runs as an ordinary role, not a superuser** (`Test user for count_nulls`). That's what makes `superuser = false` in `count_nulls.control` a tested property rather than a claim. See -`helpers/test_user.sql`, including why it deliberately does *not* switch +`helpers/use_test_user.sql`, including why it deliberately does *not* switch when a real `pg_upgrade` has left the extension owned by someone else. **count_nulls is installed into a randomly named schema**, never a fixed one diff --git a/test/deps.sql b/test/deps.sql index 42a9bc6..c916f26 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -3,4 +3,4 @@ * under test/sql/. See pgxntool/README.asc's "test/install" section for why * installing count_nulls is not done here. */ -\i test/helpers/test_user.sql +\i test/helpers/use_test_user.sql diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 8eb6025..8946ecf 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -37,13 +37,13 @@ * generating this run's own name. See test/helpers/find_test_schema.sql * for how later, separate sessions rediscover the name this creates. * - * Everything below runs as a non-superuser (see test/helpers/test_user.sql), + * Everything below runs as a non-superuser (see test/helpers/use_test_user.sql), * so a successful install here is itself the proof that count_nulls doesn't * need superuser to install. The switch has to come before the cleanup loop, * not just before CREATE EXTENSION: a leftover schema is one this same role * created on a previous run, so it's the role that must be able to drop it. */ -\i test/helpers/test_user.sql +\i test/helpers/use_test_user.sql SET count_nulls.test_schema_version = :'version'; diff --git a/test/helpers/test_user.sql b/test/helpers/use_test_user.sql similarity index 95% rename from test/helpers/test_user.sql rename to test/helpers/use_test_user.sql index 62a494a..f76c2ae 100644 --- a/test/helpers/test_user.sql +++ b/test/helpers/use_test_user.sql @@ -144,13 +144,6 @@ BEGIN END $body$; -/* - * \gset, not \if, is what keeps the SET ROLE below both unconditional and - * correct: the function returns whichever role this session should run as, - * so the decision stays server-side while the switch itself stays an - * ordinary statement. It also keeps the function's result out of every - * test's expected output. - */ SELECT pg_temp.count_nulls_prepare_test_user(:'test_user') AS count_nulls_run_as \gset diff --git a/test/install/load.sql b/test/install/load.sql index 2f5e889..06a9f77 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -81,11 +81,6 @@ BEGIN SELECT default_version INTO v_default FROM pg_available_extensions WHERE name = 'count_nulls'; ELSE - /* - * A distinct tag. Reusing this DO block's own (untagged) delimiter - * would close its body right here, leaving the remainder to be parsed - * as bare SQL - which is also why this comment can't spell it out. - */ RAISE EXCEPTION $msg$count_nulls.test_existing_deploy must be 'filesystem' or 'pgtle', got '%'$msg$ , v_deploy From 6529b7272bc4ed67f3dfd20ab8b038db65f1216c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 15:39:03 -0500 Subject: [PATCH 06/10] Treat managed-cloud admin roles as equivalent to superuser The bootstrap that creates the test user and grants it what it needs was gated on is_superuser alone. On RDS, Cloud SQL and Azure Flexible Server the admin account never reports that, by design, so on those platforms the bootstrap could never run even though the account has the rights - the managed roles were only ever consulted afterwards, to disqualify the test user. Both checks now consult the same list. An admin that isn't a real superuser also can't SET ROLE to the role it just created: from PG16 a CREATEROLE creator gets ADMIN on that role, which isn't SET. It now grants the role to itself in that case, which a real superuser doesn't need. Stale-schema cleanup moves ahead of the role switch, so it runs as the connecting role. A leftover schema can belong to anyone - one created by a run predating this change belongs to a superuser - and only the connecting role is sure to be able to drop it. --- test/helpers/create_test_schema.sql | 14 +++--- test/helpers/use_test_user.sql | 69 ++++++++++++++++++----------- 2 files changed, 49 insertions(+), 34 deletions(-) diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 8946ecf..d0cf9ee 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -37,14 +37,12 @@ * generating this run's own name. See test/helpers/find_test_schema.sql * for how later, separate sessions rediscover the name this creates. * - * Everything below runs as a non-superuser (see test/helpers/use_test_user.sql), - * so a successful install here is itself the proof that count_nulls doesn't - * need superuser to install. The switch has to come before the cleanup loop, - * not just before CREATE EXTENSION: a leftover schema is one this same role - * created on a previous run, so it's the role that must be able to drop it. + * 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. */ -\i test/helpers/use_test_user.sql - SET count_nulls.test_schema_version = :'version'; DO $$ @@ -65,6 +63,8 @@ BEGIN END $$; +\i test/helpers/use_test_user.sql + SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema \gset diff --git a/test/helpers/use_test_user.sql b/test/helpers/use_test_user.sql index f76c2ae..8dee2f5 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -42,19 +42,34 @@ CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_user( ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE /* - * The managed-cloud analogues of superuser. None of them carry the - * rolsuper attribute - that's the whole point of them - so the rolsuper - * check below can't see them and they have to be named explicitly. AWS - * RDS and Aurora call it rds_superuser, Cloud SQL cloudsqlsuperuser, - * Azure Flexible Server azure_pg_admin. + * The managed-cloud analogues of superuser: AWS RDS and Aurora's + * rds_superuser, Cloud SQL's cloudsqlsuperuser, Azure Flexible Server's + * azure_pg_admin. None carries the rolsuper attribute - that's the whole + * point of them - so rolsuper and is_superuser can't see them and they + * have to be named. Both checks below treat them as equivalent to + * superuser: enough to set the test user up, and disqualifying for the + * test user itself. */ c_managed_superuser_roles CONSTANT name[] := '{rds_superuser,cloudsqlsuperuser,azure_pg_admin}' ; - v_role name; + + /* + * Joining pg_roles rather than naming the roles to pg_has_role() keeps the + * ones that don't exist on this cluster out of it entirely - it errors on + * a role that isn't there. MEMBER, not USAGE, is deliberately over-strict: + * it counts a role that merely *could* SET ROLE without having done so. + */ + c_admin CONSTANT boolean := current_setting('is_superuser') = 'on' OR EXISTS( + SELECT 1 FROM pg_roles + WHERE rolname = ANY(c_managed_superuser_roles) + AND pg_has_role(current_user, oid, 'MEMBER') + ); + c_extension_owner CONSTANT name := ( SELECT pg_get_userbyid(extowner) FROM pg_extension WHERE extname = 'count_nulls' ); + v_disqualifying name; BEGIN /* * An already-installed count_nulls belonging to somebody else is one a @@ -83,11 +98,20 @@ BEGIN RETURN current_user; END IF; - IF current_setting('is_superuser') = 'on' THEN + IF c_admin THEN IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN EXECUTE format('CREATE ROLE %I', p_test_user); END IF; + /* + * A real superuser can already SET ROLE to anything. A managed-cloud + * admin can't, even to a role it just created: from PG16 a CREATEROLE + * creator gets ADMIN on that role, which is not the same as SET. + */ + IF current_setting('is_superuser') = 'off' THEN + EXECUTE format('GRANT %I TO %I', p_test_user, current_user); + END IF; + /* * Can't fold into the CREATE ROLE above: the role is cluster-wide and * outlives any one run, but this grant lives in the current database's @@ -111,7 +135,7 @@ BEGIN IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN RAISE EXCEPTION - 'role "%" does not exist, and this session is not a superuser so it cannot be created' + 'role "%" does not exist, and this session cannot create it' , p_test_user ; END IF; @@ -120,25 +144,16 @@ BEGIN RAISE EXCEPTION 'test user "%" must not be a superuser', p_test_user; END IF; - FOREACH v_role IN ARRAY c_managed_superuser_roles LOOP - /* - * Whether the role exists has to be settled in its own statement: - * pg_has_role() errors outright on one that doesn't, and SQL promises - * no evaluation order between the two halves of an AND. - * - * MEMBER, not USAGE, is deliberately over-strict: it also rejects a - * role that merely *could* SET ROLE to one of these without ever having - * done so, which isn't the same as holding the privileges. Simpler than - * reasoning about when it would actually matter, and nothing needs the - * looser check yet. - */ - IF EXISTS(SELECT 1 FROM pg_roles WHERE rolname = v_role) THEN - IF pg_has_role(p_test_user, v_role, 'MEMBER') THEN - RAISE EXCEPTION - 'test user "%" must not be granted %', p_test_user, v_role; - END IF; - END IF; - END LOOP; + SELECT rolname INTO v_disqualifying + FROM pg_roles + WHERE rolname = ANY(c_managed_superuser_roles) + AND pg_has_role(p_test_user, oid, 'MEMBER') + ; + + IF v_disqualifying IS NOT NULL THEN + RAISE EXCEPTION + 'test user "%" must not be granted %', p_test_user, v_disqualifying; + END IF; RETURN p_test_user; END From 922c349c50ae5a899fb5d2d21feae1538c21a18e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 15:59:40 -0500 Subject: [PATCH 07/10] Use dollar quoting for error messages, and trim the schema-setup header Doubled single quotes ('' ... '') in RAISE EXCEPTION messages made two call sites hard to read; switch both to distinct-tag dollar quoting so the literal quotes read as themselves. create_test_schema.sql's header comment had grown to explain nearly everything the file does, most of it far from the code it was describing. Cut it to a few lines on what the file is and why it exists standalone (bin/test_existing's prepare-old runs it outside any psql session that also runs load.sql), and move each remaining rationale down next to the code it explains: the GUC-bridging and 'current' sentinel notes by the guard, the crash-recovery note by the cleanup loop, the identifier-quoting note by the name generation, and the WITH SCHEMA note by CREATE EXTENSION. --- test/helpers/create_test_schema.sql | 81 +++++++++++------------------ test/install/load.sql | 2 +- 2 files changed, 31 insertions(+), 52 deletions(-) diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index d0cf9ee..6be4d3b 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -1,58 +1,36 @@ /* * 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 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 - * 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. - * - * 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. + * 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. + * + * '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. + */ 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'; + RAISE EXCEPTION $msg$:version must be set explicitly, or 'current'$msg$; END IF; END $$; +/* + * 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. + */ DO $$ DECLARE r record; @@ -65,20 +43,21 @@ $$; \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"; /* - * 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. + * 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. */ SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause \gset diff --git a/test/install/load.sql b/test/install/load.sql index 06a9f77..694722f 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -33,7 +33,7 @@ DO $$ BEGIN IF current_setting('count_nulls.test_load_mode') NOT IN ('fresh', 'update', 'existing') THEN RAISE EXCEPTION - 'count_nulls.test_load_mode must be ''fresh'', ''update'' or ''existing'', got ''%''' + $msg$count_nulls.test_load_mode must be 'fresh', 'update' or 'existing', got '%'$msg$ , current_setting('count_nulls.test_load_mode') ; END IF; From b5659f4d86da289d1514098ceab449dc535f5e65 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 16:48:52 -0500 Subject: [PATCH 08/10] Scope the extension-owner short-circuit to existing mode only count_nulls_prepare_test_user() now takes the load mode as an explicit argument (the caller must supply it - the GUC it comes from isn't set for every invocation, e.g. bin/test_existing's prepare-old) and only skips the role switch for a foreign-owned extension when that mode is 'existing'. In fresh/update, a foreign-owned count_nulls is a cleanup miss, not a preserved pg_upgrade artifact, so it now raises instead of silently letting the suite run as the connecting role. --- test/deps.sql | 8 ++++++++ test/helpers/create_test_schema.sql | 5 +++++ test/helpers/use_test_user.sql | 24 ++++++++++++++++++++++-- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/test/deps.sql b/test/deps.sql index c916f26..01be477 100644 --- a/test/deps.sql +++ b/test/deps.sql @@ -2,5 +2,13 @@ * Per-test-session setup: pgxntool's setup.sql `\i`s this for every file * under test/sql/. See pgxntool/README.asc's "test/install" section for why * installing count_nulls is not done here. + * + * 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.) These are real + * pg_regress sessions, so the Makefile has exported it via PGOPTIONS. */ +SELECT current_setting('count_nulls.test_load_mode') AS count_nulls_load_mode +\gset + \i test/helpers/use_test_user.sql diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 6be4d3b..b79bdcd 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -41,6 +41,11 @@ BEGIN END $$; +/* + * Never reached in existing mode - see this file's own header - so a + * literal is enough; there's no GUC to lose by not reading it. + */ +\set count_nulls_load_mode 'fresh' \i test/helpers/use_test_user.sql /* diff --git a/test/helpers/use_test_user.sql b/test/helpers/use_test_user.sql index 8dee2f5..6ff220c 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -34,11 +34,18 @@ RESET ROLE; * where psql reports it as an invalid command and then carries straight on * into the branch it should have skipped. (\gset, below, is fine - 9.3.) * + * p_load_mode must come from the caller, not from reading + * count_nulls.test_load_mode in here: the Makefile only exports that GUC via + * PGOPTIONS for pg_regress sessions, and bin/test_existing's prepare-old + * invokes psql directly without it. Every includer sets the psql variable + * count_nulls_load_mode before \i-ing this file. + * * TODO: collapse this into a \gset + \if once 10 is the oldest version * supported - the plpgsql is only here to work around \if's absence. */ CREATE OR REPLACE FUNCTION pg_temp.count_nulls_prepare_test_user( p_test_user name + , p_load_mode text ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE /* @@ -93,9 +100,22 @@ BEGIN * * Nothing is lost by not switching here: existing mode installs nothing, * so it was never the leg proving the install works unprivileged. + * + * Scoped to existing mode only. In fresh/update, a foreign-owned + * count_nulls isn't a preserved pg_upgrade artifact - it's a leftover this + * run's cleanup failed to reach - and silently keeping the connecting + * role would run the whole suite as its (often superuser) privileges + * without ever exercising the switch this file exists to make. */ IF c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user THEN - RETURN current_user; + IF p_load_mode = 'existing' THEN + RETURN current_user; + END IF; + + RAISE EXCEPTION + 'count_nulls is owned by "%", not test user "%", and load mode "%" expects no pre-existing installation' + , c_extension_owner, p_test_user, p_load_mode + ; END IF; IF c_admin THEN @@ -159,7 +179,7 @@ BEGIN END $body$; -SELECT pg_temp.count_nulls_prepare_test_user(:'test_user') AS count_nulls_run_as +SELECT pg_temp.count_nulls_prepare_test_user(:'test_user', :'count_nulls_load_mode') AS count_nulls_run_as \gset SET ROLE :"count_nulls_run_as"; From af48b239c43dc429accdcf5f1beb33b4d24e0624 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 17:06:09 -0500 Subject: [PATCH 09/10] Address reviewer comments: reframe intent, relocate a misplaced comment, and trim an unenforceable ownership claim The load-mode literal comment asserted an unenforced fact about the rest of the system instead of stating the intent of this code path; restate it as intent, noting the loud-failure fallback if that intent ever breaks. The WITH SCHEMA comment had ended up above an unrelated CASE expression when the header was split up; move it back next to the CREATE EXTENSION statement it documents. The extension-owner short-circuit's comment assumed a foreign-owned extension always came from a real pg_upgrade restore, which the code doesn't and can't know; trim it to the two facts the code actually depends on plus the one non-obvious reason (no ALTER EXTENSION ... OWNER TO) existing mode can meet a foreign owner at all. --- test/helpers/create_test_schema.sql | 11 ++++----- test/helpers/use_test_user.sql | 35 ++++++++--------------------- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index b79bdcd..9a5a3b9 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -42,8 +42,9 @@ END $$; /* - * Never reached in existing mode - see this file's own header - so a - * literal is enough; there's no GUC to lose by not reading it. + * This file exists to install count_nulls, so a foreign-owned extension + * here is always an error - passing a non-'existing' mode says so. If + * that stops being true, use_test_user.sql raises loudly, not silently. */ \set count_nulls_load_mode 'fresh' \i test/helpers/use_test_user.sql @@ -58,13 +59,13 @@ SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schem CREATE SCHEMA :"schema"; +SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause +\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. */ -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/use_test_user.sql b/test/helpers/use_test_user.sql index 6ff220c..6bd01e5 100644 --- a/test/helpers/use_test_user.sql +++ b/test/helpers/use_test_user.sql @@ -79,33 +79,16 @@ DECLARE v_disqualifying name; BEGIN /* - * An already-installed count_nulls belonging to somebody else is one a - * real pg_upgrade restored, and it can only be managed by its owner - so - * stay as we are rather than switching to a role that can't drop it. + * In existing mode, an extension owned by someone else is tolerated - the + * test user can't manage it, so stay as the connecting role. Any other + * mode treats this as an error: silently staying would run the suite + * with the connecting role's (often superuser) privileges, proving + * nothing. * - * PostgreSQL has no ALTER EXTENSION ... OWNER TO, so pg_dump has no way - * to carry an extension's ownership across: --binary-upgrade emits - * binary_upgrade_create_empty_extension(), which takes no owner, and the - * extension ends up belonging to whoever ran the restore. Its member - * functions DO keep their owner, so only the extension object itself is - * out of reach - which is exactly what test__shutdown__drop_all needs. - * - * Don't expect a newer PostgreSQL to retire this branch. extowner has had - * no matching ALTER EXTENSION ... OWNER TO since it was added in 2011, - * because what that should do to the contained objects was never settled - * (handing a non-superuser a C-language handler function is the awkward - * case). Reported as BUG #18625 and acknowledged as a known shortcoming, - * still unfixed. pg_dump's --use-set-session-authorization does dodge it, - * but pg_upgrade offers no way to ask for that. - * - * Nothing is lost by not switching here: existing mode installs nothing, - * so it was never the leg proving the install works unprivileged. - * - * Scoped to existing mode only. In fresh/update, a foreign-owned - * count_nulls isn't a preserved pg_upgrade artifact - it's a leftover this - * run's cleanup failed to reach - and silently keeping the connecting - * role would run the whole suite as its (often superuser) privileges - * without ever exercising the switch this file exists to make. + * PostgreSQL has no ALTER EXTENSION ... OWNER TO, so pg_upgrade can't + * preserve extension ownership (BUG #18625) - which is why existing mode + * can meet a foreign owner at all. Member functions keep their owner; + * only the extension object doesn't. */ IF c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user THEN IF p_load_mode = 'existing' THEN From 06bce4ef5095f6eb2ff3e9d69b54bdc50148272e Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 31 Aug 2026 17:25:19 -0500 Subject: [PATCH 10/10] Fix stale control-file filename, and make the caller supply the test load mode count_nulls.control referenced test/helpers/test_user.sql, which was renamed to use_test_user.sql; the tree-wide grep for that rename had been scoped to subdirectories and missed it. create_test_schema.sql hardcoded \set count_nulls_load_mode 'fresh' right before including use_test_user.sql, which both invented a mode the caller never asked for and would silently override one a caller had legitimately set. It's already wrong in practice: test/install/load.sql's update leg runs through this file with the mode hardcoded to 'fresh' even though it's really 'update'. The value now comes from whichever caller genuinely knows it - test/install/load.sql (derived from count_nulls.test_load_mode, the same way test/deps.sql already does) or bin/test_existing's -v on the command line (always a genuine fresh install) - and create_test_schema.sql only consumes :count_nulls_load_mode, same as every other includer of use_test_user.sql. --- bin/test_existing | 7 +++++-- count_nulls.control | 2 +- test/helpers/create_test_schema.sql | 8 ++++---- test/install/load.sql | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/bin/test_existing b/bin/test_existing index 0a9dab5..995b7a9 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -184,10 +184,13 @@ 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). +# 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 +# install (prepare-old), never 'existing'. create_extension_in_schema() { local db=$1 version=$2 - psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" -f test/helpers/create_test_schema.sql + psql -d "$db" -v ON_ERROR_STOP=1 -v version="$version" \ + -v count_nulls_load_mode=fresh -f test/helpers/create_test_schema.sql } # --------------------------------------------------------------------------- diff --git a/count_nulls.control b/count_nulls.control index 998df78..5c938a7 100644 --- a/count_nulls.control +++ b/count_nulls.control @@ -4,6 +4,6 @@ default_version = 'stable' relocatable = false # Pure SQL functions, nothing privileged - anyone with CREATE on the target # schema can install it. Enforced by the suite running as a non-superuser -# (test/helpers/test_user.sql), which fails outright if this reverts to the +# (test/helpers/use_test_user.sql), which fails outright if this reverts to the # default. superuser = false diff --git a/test/helpers/create_test_schema.sql b/test/helpers/create_test_schema.sql index 9a5a3b9..28166ed 100644 --- a/test/helpers/create_test_schema.sql +++ b/test/helpers/create_test_schema.sql @@ -42,11 +42,11 @@ END $$; /* - * This file exists to install count_nulls, so a foreign-owned extension - * here is always an error - passing a non-'existing' mode says so. If - * that stops being true, use_test_user.sql raises loudly, not silently. + * :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'. */ -\set count_nulls_load_mode 'fresh' \i test/helpers/use_test_user.sql /* diff --git a/test/install/load.sql b/test/install/load.sql index 694722f..919867c 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -24,7 +24,7 @@ * 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 +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 \gset