Fix CREATE EXTENSION on PG16+ for non-superuser installs (#14)

.github/workflows/ci.yml

@@ -5,22 +5,41 @@
 #
 #   test             -- FRESH install: CREATE EXTENSION at the current
 #                        version, on every supported PostgreSQL major. The
-#                        baseline a brand-new user gets.
+#                        install itself runs as a disposable non-superuser
+#                        role (test/install/load.sql's test_factory_installer
+#                        -- see its own comment), not as whatever superuser
+#                        CI happens to connect as: pg_has_role(current_user,
+#                        ...) is unconditionally true for a superuser, so
+#                        only a real non-superuser installer actually
+#                        exercises https://github.com/Postgres-Extensions/test_factory/issues/14's
+#                        fix. A separate superuser-install leg would add
+#                        nothing real -- superuser bypasses every check that
+#                        role is subject to, so if the non-superuser installer
+#                        can do it, a superuser certainly can too.
 #   pg-upgrade-test   -- BINARY pg_upgrade: install the current version on an
 #                        OLD major, binary-upgrade the cluster to a NEWER
 #                        major, then run the suite against the migrated
 #                        objects in "existing" mode (test/install/load.sql).
 #                        Proves objects created on an old server still work
 #                        read back on a new one -- fresh-install testing
-#                        never exercises this at all.
+#                        never exercises this at all. Runs as the ambient
+#                        superuser throughout (binary pg_upgrade itself needs
+#                        OS/cluster-level superuser access anyway), not the
+#                        non-superuser installer -- test/install/load.sql only
+#                        switches to it in the fresh/update branch.
 #
 # test_factory has shipped only one version (0.5.0), so there is no
 # in-place `ALTER EXTENSION UPDATE` path to test yet (no extension-update-test
 # job) and the pg_upgrade job needs no bridge step (it always installs the
 # CURRENT version on the old cluster -- there's no older, pg_upgrade-unsafe
-# version in the wild to carry forward). Both jobs derive their PostgreSQL
-# major list from the single source of truth computed in the `changes` job
-# below, so adding/dropping a supported major is a one-line edit there.
+# version in the wild to carry forward). The `test` job derives its
+# PostgreSQL major list from the single source of truth computed in the
+# `changes` job below, so adding/dropping a supported major is a one-line
+# edit there for that job. `pg-upgrade-test`'s matrix is NOT derived from
+# that same source -- its old_pg/new_pg pairs are hardcoded literals below,
+# deliberately chosen to straddle the PostgreSQL 16 boundary without ever
+# crossing it (see that job's own comment for why), so bumping NEWEST/FLOOR
+# does not automatically update it.
 name: CI
 on:
   # Post-merge CI only; pull_request already covers every PR commit. Without
@@ -190,13 +209,12 @@ jobs:
   # older, pg_upgrade-unsafe install to carry forward -- every leg installs
   # the current version directly on the old cluster.
   #
-  # Two legs, not an every-major stepwise climb (deliberately out of scope --
-  # see the PR description): the oldest-to-newest jump (widest catalog
-  # distance) and the newest-boundary jump (most likely to hit a *new*
-  # PostgreSQL major's catalog change first). test_factory has no views or
-  # functions touching catalog internals (no SELECT * over a system catalog),
-  # so the per-major-boundary risk a full stepwise climb protects against is
-  # low here.
+  # Two legs, deliberately chosen to each stay entirely on one side of the
+  # PostgreSQL 16 boundary (10->15 and 16->18), never crossing PG16 itself:
+  # binary pg_upgrade doesn't re-run the install script, so
+  # https://github.com/Postgres-Extensions/test_factory/issues/14's
+  # `GRANT ... WITH SET TRUE` fix can't reach a role grant that predates PG16
+  # (see README's "Known limitation" section).
   pg-upgrade-test:
     # Also needs `test`, not just `changes`: without this, a trivially-broken
     # PR (fails the cheap fresh-install matrix) still burns the full,
@@ -207,8 +225,8 @@ jobs:
       matrix:
         include:
           - old_pg: "10"
-            new_pg: "18"
-          - old_pg: "17"
+            new_pg: "15"
+          - old_pg: "16"
             new_pg: "18"
     name: 🔄 Binary pg_upgrade ${{ matrix.old_pg }} → ${{ matrix.new_pg }}
     runs-on: ubuntu-latest

README.md

@@ -89,6 +89,25 @@ so:
 
     PGOPTIONS=--search_path=extensions psql -d mydb -f test_factory.sql
 
+Known limitation: SET-enabled membership in test_factory__owner
+------------------------------------------------------------------
+
+`CREATE EXTENSION test_factory` doesn't require a superuser -- a role with
+CREATEROLE (to create `test_factory__owner`) and CREATE on the target
+database (for the schemas the install script creates) is enough.
+
+Installing this way grants the installing role membership in
+`test_factory__owner` -- SET-enabled on PostgreSQL 16+ (needed to `SET ROLE
+test_factory__owner`), plain membership pre-16 -- but only if the installer
+already has that membership, or has ADMIN OPTION on `test_factory__owner` to
+grant it themselves. If neither is true, `CREATE EXTENSION` fails
+immediately with an error naming exactly who needs to run what.
+
+If you ever do need to grant this membership manually (e.g. it was revoked,
+or a role was set up some other way):
+
+    GRANT test_factory__owner TO <role> WITH SET TRUE;
+
 Copyright and License
 ---------------------
 

sql/test_factory--0.5.0--stable.sql

@@ -1,15 +1,50 @@
 /*
- * Genuine no-op: nothing in this extension has changed since 0.5.0 yet.
- * This file exists purely so ALTER EXTENSION test_factory UPDATE has an
- * edge to follow at all -- Postgres's version-graph resolution requires
- * an actual sql/test_factory--<from>--<to>.sql file to exist for a
- * transition, regardless of whether its content would be a no-op
- * (confirmed directly: without this file, ALTER EXTENSION UPDATE fails
- * outright with "has no update path from version 0.5.0 to version
- * stable", even though nothing would actually need to change). See
- * ../ai/RELEASE.md's `stable` pseudo-version workflow: every subsequent
- * SQL-touching PR adds whatever ALTER .../CREATE OR REPLACE ... statements
- * are needed here to bring an install on 0.5.0 up to that change.
+ * https://github.com/Postgres-Extensions/test_factory/issues/14: whoever
+ * originally installed 0.5.0 never automatically got SET-enabled (PG16+)
+ * or even plain (pre-16) membership in test_factory__owner -- that's the
+ * bug, and it applies just as much to an already-existing 0.5.0 install
+ * as to a fresh one (see sql/test_factory.sql's own comment for the fresh
+ * case). This grants it retroactively, so whoever runs this update -- and
+ * anyone else who later needs to SET ROLE test_factory__owner -- has it.
+ *
+ * No object in this extension changed between 0.5.0 and here, so unlike
+ * the fresh-install script, there's nothing to create or alter AS
+ * test_factory__owner, and so no role to switch to or restore.
  */
+DO $body$
+BEGIN
+	IF current_setting('server_version_num')::int >= 160000 THEN
+		IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN
+			BEGIN
+				EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user);
+			EXCEPTION
+				WHEN insufficient_privilege THEN
+					RAISE EXCEPTION
+						'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself'
+						, current_user
+						USING ERRCODE = 'insufficient_privilege'
+						, HINT = format(
+							'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s'
+							, format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user)
+						);
+			END;
+		END IF;
+	ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN
+		BEGIN
+			EXECUTE format('GRANT test_factory__owner TO %I', current_user);
+		EXCEPTION
+			WHEN insufficient_privilege THEN
+				RAISE EXCEPTION
+					'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself'
+					, current_user
+					USING ERRCODE = 'insufficient_privilege'
+					, HINT = format(
+						'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s'
+						, format('GRANT test_factory__owner TO %I;', current_user)
+					);
+		END;
+	END IF;
+END
+$body$;
 
 -- vi: expandtab ts=2 sw=2

sql/test_factory.sql

@@ -5,6 +5,7 @@
  * it's much lighter weight than creating a table.
  */
 SELECT pg_catalog.set_config('test_factory.original_role', current_user, true);
+
 DO $body$
 BEGIN
 	CREATE ROLE test_factory__owner;
@@ -14,6 +15,86 @@ EXCEPTION
 END
 $body$;
 
+/*
+ * CREATE ROLE alone never grants the creator any relationship to the role
+ * it just created -- on ANY version, confirmed directly against a genuine
+ * non-superuser CREATEROLE installer with zero prior grants: SET ROLE
+ * test_factory__owner below fails outright ("permission denied to set
+ * role") without an explicit GRANT first, even immediately after this
+ * same session created it. A real superuser bypasses the SET ROLE check
+ * entirely regardless of any of this, which is why this was easy to miss
+ * without testing against a genuine non-superuser role.
+ *
+ * As of PG16, plain membership isn't enough either -- SET ROLE requires
+ * the SET option specifically, which needs GRANT ... WITH SET TRUE.
+ * Before PG16, plain membership (no WITH SET syntax, which doesn't exist
+ * yet) already confers the ability to SET ROLE. pg_has_role's 'SET'
+ * privilege type is PG16+ only, but as a plain function argument (not a
+ * catalog column reference) it's safe to call inside this same
+ * version-gated branch, unlike a static reference to
+ * pg_auth_members.set_option, which would fail to parse on older servers
+ * even inside the gate.
+ *
+ * Either way, skip the GRANT when it's not actually needed: an installer
+ * might already have the membership it needs some other way -- e.g. a DBA
+ * pre-provisioned the role and granted it directly, deliberately
+ * withholding ADMIN OPTION as a least-privilege measure -- and GRANT ROLE
+ * requires ADMIN OPTION on the target role (or superuser) to run AT ALL,
+ * regardless of whether it would end up a no-op; forcing it unconditionally
+ * broke that already-working case (confirmed against a live non-superuser
+ * role with SET but not ADMIN OPTION: "ERROR: permission denied to grant
+ * role ... Only roles with the ADMIN option ... may grant this role").
+ *
+ * If the installer has neither the membership it needs nor ADMIN OPTION to
+ * grant it themselves, installation genuinely cannot proceed -- nobody
+ * else can do it on their behalf from inside this script. Catch that
+ * specific case and say so plainly instead of surfacing Postgres's generic
+ * permission error.
+ */
+DO $body$
+BEGIN
+	IF current_setting('server_version_num')::int >= 160000 THEN
+		IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN
+			BEGIN
+				EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user);
+			EXCEPTION
+				WHEN insufficient_privilege THEN
+					/*
+					 * RAISE's own %-substitution is plain string
+					 * interpolation, not format()'s %I/%L -- build the
+					 * copy-pastable suggested command with format() first
+					 * (so the role name is properly identifier-quoted),
+					 * then substitute the whole result in with a single,
+					 * ordinary %.
+					 */
+					RAISE EXCEPTION
+						'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself'
+						, current_user -- unquoted; the "" above is the only quoting
+						USING ERRCODE = 'insufficient_privilege'
+						, HINT = format(
+							'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s'
+							, format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user)
+						);
+			END;
+		END IF;
+	ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN
+		BEGIN
+			EXECUTE format('GRANT test_factory__owner TO %I', current_user);
+		EXCEPTION
+			WHEN insufficient_privilege THEN
+				RAISE EXCEPTION
+					'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself'
+					, current_user -- unquoted; the "" above is the only quoting
+					USING ERRCODE = 'insufficient_privilege'
+					, HINT = format(
+						'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s'
+						, format('GRANT test_factory__owner TO %I;', current_user)
+					);
+		END;
+	END IF;
+END
+$body$;
+
 CREATE SCHEMA tf AUTHORIZATION test_factory__owner;
 COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$;
 GRANT USAGE ON SCHEMA tf TO public;

test/CLAUDE.md

@@ -70,6 +70,28 @@ extensions installed, `pgtap` in schema `tap`), so `base.sql`/`pgtap.sql`
 and their expected output are shared across all of them -- no
 per-mode alternate expected files.
 
+### Non-superuser installer (`test_factory_installer`)
+
+Connecting as a superuser (CI's default, and most developers' local setup)
+bypasses every role-membership check involved in
+https://github.com/Postgres-Extensions/test_factory/issues/14's fix
+(`pg_has_role(current_user, 'test_factory__owner', ...)` is unconditionally
+true for a superuser). Asserting that property from `base.sql` while
+staying connected as a superuser would prove nothing about whether the fix
+works -- it's true either way.
+
+Rather than running the whole suite a second time as a separate disposable
+role (which would only prove what's already implied: a superuser can do
+anything a properly-privileged non-superuser can), `load.sql`'s own
+fresh/update branch creates a non-login `test_factory_installer` role
+(`CREATEROLE` only -- the exact privilege floor `CREATE EXTENSION` needs,
+paired with `CREATE` granted on the test database) and switches to it with
+`SET SESSION AUTHORIZATION` before running `CREATE EXTENSION`/`ALTER
+EXTENSION UPDATE`. No separate login, password, or CI job needed: the
+switch only affects `load.sql`'s own already-authenticated session, and
+every mode this suite runs in (superuser CI, a developer's local
+superuser) ends up exercising the exact same non-superuser install path.
+
 ### Dependency Guard
 
 Planted only in `existing` mode (see `load.sql`): a view in schema

test/build/expected/syntax.out

@@ -1,6 +1,3 @@
 \set ECHO none
-psql:sql/test_factory.sql:49: ERROR:  pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION
-psql:sql/test_factory.sql:50: ERROR:  pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION
-psql:sql/test_factory.sql:88: NOTICE:  type reference _tf._test_factory.set_name%TYPE converted to text
-psql:sql/test_factory.sql:110: NOTICE:  type reference _tf._test_factory.set_name%TYPE converted to text
-psql:sql/test_factory.sql:116: NOTICE:  type reference _tf._test_factory.set_name%TYPE converted to text
+psql:sql/test_factory.sql:130: ERROR:  pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION
+psql:sql/test_factory.sql:131: ERROR:  pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION

test/build/syntax.sql

@@ -34,6 +34,17 @@
 
 BEGIN;
 
+/*
+ * CREATE EXTENSION already forces client_min_messages to at least WARNING
+ * for the duration of an install script, restoring the caller's setting
+ * once it finishes -- so a real CREATE EXTENSION never shows the %TYPE
+ * NOTICE below on its own. Running the script bare via \i here gets none
+ * of that, so this harness sets it itself, immediately before the \i, to
+ * match what a real install actually looks like. ERROR is unaffected, so
+ * the two expected pg_extension_config_dump() errors below still show.
+ */
+SET LOCAL client_min_messages = WARNING;
+
 -- test_factory first: test_factory_pgtap's file needs its "tf" schema/role.
 \i sql/test_factory.sql
 \i sql/test_factory_pgtap.sql