Pick the test/install load mode server-side, and turn on ON_ERROR_STOP

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
 -----

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:
 

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
 }
 
 # ---------------------------------------------------------------------------

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/use_test_user.sql), which fails outright if this reverts to the
+# default.
+superuser = false

test/README.md

@@ -1,124 +1,61 @@
 # 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`). 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.
-- `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.
-
-## 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.
+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, 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/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
+  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/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
+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.
+
+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 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/<test>.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/<test>.out`.

test/deps.sql

@@ -1,16 +1,14 @@
 /*
- * 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: 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.
  *
- * 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`.
- *
- * 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.
+ * 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

test/helpers/create_test_schema.sql

@@ -1,78 +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,
- * `\set version` then `\i` this file) AND bin/test_existing's prepare-old
- * (a SEPARATE invocation - `-v version=<INSTALL_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.
+ * 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=<VERSION> -f); the work itself lives in
+ * test/helpers/extension_installer.sql.
  *
- * :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.
  */
-SET count_nulls.test_schema_version = :'version';
-
-DO $$
-BEGIN
-  IF current_setting('count_nulls.test_schema_version') = '' THEN
-    RAISE EXCEPTION ':version must be set explicitly - use ''current'' to install whatever the current default is, never an empty string, so an accidentally-unpropagated value fails loudly instead of silently installing ''current'' when something else was actually intended';
-  END IF;
-END
-$$;
-
-DO $$
-DECLARE
-  r record;
-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
-$$;
+\i test/helpers/extension_installer.sql
 
-SELECT 'count_nulls test schema ' || substr(md5(random()::text), 1, 12) AS schema
-\gset
-
-CREATE SCHEMA :"schema";
+/*
+ * Always a fresh install here, never 'existing' - see the mode's no-op note
+ * on count_nulls_cleanup_test_schemas() itself.
+ */
+SELECT pg_temp.count_nulls_cleanup_test_schemas('fresh');
 
 /*
- * 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.
+ * :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'.
  */
-SELECT CASE WHEN :'version' = 'current' THEN '' ELSE format(' VERSION %L', :'version') END AS version_clause
+\i test/helpers/use_test_user.sql
+
+SELECT pg_temp.count_nulls_install_extension(:'version') AS count_nulls_test_schema
 \gset
 
-CREATE EXTENSION count_nulls WITH SCHEMA :"schema":version_clause;
+-- vi: expandtab sw=2 ts=2