From a630263d017f2a0bc9b3a8fe95cea32b6a9a4693 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 14 Jul 2026 17:25:48 -0500 Subject: [PATCH 01/17] Grant owner role WITH SET so CREATE EXTENSION works on PG16+ non-superuser As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled membership in the new role, so the install's `SET ROLE test_factory__owner` fails unless the current role is a superuser (which bypasses the check). This surfaces only on non-superuser installs (e.g. RDS/Aurora). Grant the role back to the installing role WITH SET, gated on PG16+ (pre-16 GRANT already permits SET ROLE). Unconditional, so it also covers a pre-existing role where CREATE ROLE was a no-op. Fixes #14 Co-Authored-By: Claude Opus 4.8 (1M context) --- sql/test_factory--0.5.0.sql | 17 +++++++++++++++++ sql/test_factory.sql | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 37e742b..c38b0df 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -15,6 +15,23 @@ EXCEPTION END $body$; +/* + * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled + * membership in the new role, so SET ROLE test_factory__owner below fails + * unless the current role is a superuser (which bypasses the check). Grant it + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO + * already confers the ability to SET ROLE. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + 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; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 38e2698..0cce207 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -14,6 +14,23 @@ EXCEPTION END $body$; +/* + * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled + * membership in the new role, so SET ROLE test_factory__owner below fails + * unless the current role is a superuser (which bypasses the check). Grant it + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO + * already confers the ability to SET ROLE. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + 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; From 1f372364eea5692a5528d6c24f197c50a181ecdd Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 14 Jul 2026 17:25:49 -0500 Subject: [PATCH 02/17] test: assert installer gets SET-enabled membership in owner role (issue #14) The failure can't be reproduced under pg_regress, which runs as a superuser that bypasses the SET ROLE check. Instead assert the state the fix establishes: after install the installing role holds a SET-enabled membership in test_factory__owner (PG16+; skipped with identical output pre-16). Co-Authored-By: Claude Opus 4.8 (1M context) --- test/sql/base.sql | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/sql/base.sql b/test/sql/base.sql index f41978c..07d6327 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -3,6 +3,39 @@ -- test/install/load.sql already installed the extension, in every mode. +/* + * Regression test for issue #14. On PostgreSQL 16+, CREATE ROLE no longer + * grants the creating role a SET-enabled membership in the new role, so the + * install must GRANT test_factory__owner ... WITH SET TRUE or the SET ROLE + * performed during install fails for non-superuser installs (RDS/Aurora). A + * real superuser bypasses the SET ROLE check, so a plain install here cannot + * reproduce the failure; instead assert the SET-enabled membership the fix + * establishes. pg_auth_members.set_option only exists on PG16+, so the check is + * skipped (with identical TAP output) on older versions, where a plain + * GRANT ... TO already confers the ability to SET ROLE. + */ +SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus \gset +-- pg16+ SET-enabled membership check +\if :pg16plus +SELECT ok( + EXISTS( + SELECT 1 + FROM pg_auth_members + WHERE roleid = 'test_factory__owner'::regrole + AND member = current_user::regrole + AND set_option + ) + , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' +); +-- pg16+ SET-enabled membership check +\else +SELECT ok( + true + , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' +); +-- pg16+ SET-enabled membership check +\endif + -- NOTE: This runs some tests itself \i test/helpers/create.sql From 9582bbdb7d27cbca8e88087129f2d1a31a03fa29 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 14:22:48 -0500 Subject: [PATCH 03/17] Rebase issue-14 fix onto master's test/build refactor master gained test/build/syntax.sql since fix/issue-14-clean branched, which re-runs sql/test_factory.sql bare (via \i) right after test/install/load.sql already installed the extension in the same database. That surfaced two problems the original fix never hit: - The unconditional re-GRANT emitted a NOTICE naming the installing role on this second run, making test/build/expected/syntax.out's content depend on which role runs the install (confirmed by reproducing locally as both "root" and "postgres" -- CI runs as "postgres", this container's shell runs as "root"). Guard the GRANT on an existing pg_auth_members SET-enabled membership check so it's a true no-op (no NOTICE) the second time. - That existence check referenced pg_auth_members.set_option directly, which doesn't exist before PG16. PL/pgSQL parses a query's text as soon as it reaches the statement, regardless of whether the surrounding IF branch ends up running, so this broke test-build on PG12 even though the check was already gated on server_version_num. Moved the check into the same EXECUTE-a-format()'d-string pattern already used for the GRANT, deferring the parse to runtime, only inside the PG16+ branch. Verified via `make lint` and `make test` on both PG17 and PG12 (the PG12 run confirms the pre-16 branch produces identical passing output), plus PG17 under TEST_LOAD_SOURCE=update and connected as role "postgres" (matching CI's PGUSER) to confirm the syntax.out fix isn't role-name-dependent. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0.sql | 31 ++++++++++++++++++++---- sql/test_factory.sql | 31 ++++++++++++++++++++---- test/build/expected/syntax.out | 10 ++++---- test/expected/base.out | 43 +++++++++++++++++----------------- 4 files changed, 79 insertions(+), 36 deletions(-) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index c38b0df..afe9400 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -19,15 +19,36 @@ $body$; * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs - * unconditionally, even when the role already existed and CREATE ROLE was a - * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO - * already confers the ability to SET ROLE. + * explicitly WITH SET so a non-superuser install works too. Runs even when + * the role already existed and CREATE ROLE was a no-op, but only when the + * membership isn't already SET-enabled -- an unconditional re-GRANT is a + * harmless no-op, but still emits a NOTICE naming the installing role, which + * would make test/build/syntax.sql's raw re-run of this file (after + * test/install/load.sql already installed it once) produce output that + * differs by whichever role happens to be running the install. The + * existence check itself must go through EXECUTE too, not just the GRANT -- + * pg_auth_members.set_option doesn't exist before PG16, so a plain (static) + * reference to it would fail to parse on older servers even inside this + * same version-gated IF, since PL/pgSQL parses a query's text as soon as it + * reaches that statement, before evaluating whether the branch actually + * runs. Building the query as a string and only EXECUTEing it once already + * inside the PG16+ branch defers that parsing until it's safe. Pre-16 + * GRANT ... TO already confers the ability to SET ROLE, so none of this + * runs there. */ DO $body$ +DECLARE + already_set_enabled boolean; BEGIN IF current_setting('server_version_num')::int >= 160000 THEN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + EXECUTE format( + 'SELECT EXISTS (SELECT 1 FROM pg_auth_members WHERE roleid = %L::regrole AND member = %L::regrole AND set_option)' + , 'test_factory__owner' + , current_user + ) INTO already_set_enabled; + IF NOT already_set_enabled THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + END IF; END IF; END $body$; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 0cce207..21dca18 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -18,15 +18,36 @@ $body$; * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs - * unconditionally, even when the role already existed and CREATE ROLE was a - * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO - * already confers the ability to SET ROLE. + * explicitly WITH SET so a non-superuser install works too. Runs even when + * the role already existed and CREATE ROLE was a no-op, but only when the + * membership isn't already SET-enabled -- an unconditional re-GRANT is a + * harmless no-op, but still emits a NOTICE naming the installing role, which + * would make test/build/syntax.sql's raw re-run of this file (after + * test/install/load.sql already installed it once) produce output that + * differs by whichever role happens to be running the install. The + * existence check itself must go through EXECUTE too, not just the GRANT -- + * pg_auth_members.set_option doesn't exist before PG16, so a plain (static) + * reference to it would fail to parse on older servers even inside this + * same version-gated IF, since PL/pgSQL parses a query's text as soon as it + * reaches that statement, before evaluating whether the branch actually + * runs. Building the query as a string and only EXECUTEing it once already + * inside the PG16+ branch defers that parsing until it's safe. Pre-16 + * GRANT ... TO already confers the ability to SET ROLE, so none of this + * runs there. */ DO $body$ +DECLARE + already_set_enabled boolean; BEGIN IF current_setting('server_version_num')::int >= 160000 THEN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + EXECUTE format( + 'SELECT EXISTS (SELECT 1 FROM pg_auth_members WHERE roleid = %L::regrole AND member = %L::regrole AND set_option)' + , 'test_factory__owner' + , current_user + ) INTO already_set_enabled; + IF NOT already_set_enabled THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + END IF; END IF; END $body$; diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index b1d0fc9..ed15c52 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,6 +1,6 @@ \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:87: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:88: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:126: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory.sql:148: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text +psql:sql/test_factory.sql:154: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text diff --git a/test/expected/base.out b/test/expected/base.out index 7af44d8..fbb4956 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,22 +1,23 @@ \set ECHO none -ok 1 - Register test customers -ok 2 - Create function customer__add -ok 3 - Register test invoices -ok 4 - Ensure original_role temp table was dropped -ok 5 - Security definer function _tf.get has search_path=pg_catalog -ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 7 - Security definer function _tf.table_create has search_path=pg_catalog -ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 10 - customer table is empty -ok 11 - invoice table is empty -ok 12 - invoice factory output -ok 13 - invoice table content -ok 14 - customer table content -ok 15 - invoice factory second call -ok 16 - invoice table content stayed constant -ok 17 - customer table content stayed constant -ok 18 - Test function factory -ok 19 - customer table has new row -ok 20 - truncate invoice -ok 21 - invoice factory get remains the same after truncate +ok 1 - Installing role has SET-enabled membership in test_factory__owner (issue #14) +ok 2 - Register test customers +ok 3 - Create function customer__add +ok 4 - Register test invoices +ok 5 - Ensure original_role temp table was dropped +ok 6 - Security definer function _tf.get has search_path=pg_catalog +ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 8 - Security definer function _tf.table_create has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 10 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 11 - customer table is empty +ok 12 - invoice table is empty +ok 13 - invoice factory output +ok 14 - invoice table content +ok 15 - customer table content +ok 16 - invoice factory second call +ok 17 - invoice table content stayed constant +ok 18 - customer table content stayed constant +ok 19 - Test function factory +ok 20 - customer table has new row +ok 21 - truncate invoice +ok 22 - invoice factory get remains the same after truncate From 56448d350f4cb38222eba550c2f0f6feb39819fc Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 15:32:35 -0500 Subject: [PATCH 04/17] ci: restructure pg-upgrade-test matrix to avoid crossing the PG16 boundary Binary pg_upgrade does not re-run an extension's install script -- it reconstructs catalog objects directly to preserve OIDs/relfilenodes for the physical file copy. That means the WITH SET role-membership fix for issue #14 (GRANT test_factory__owner TO WITH SET TRUE, added in this branch) only has a chance to run once, at original install time; a pg_upgrade that crosses PG16 (where set_option/WITH SET was introduced) can't apply or repair it after the fact. CI's pg-upgrade-test job previously had a leg (17 -> 18) that stayed on one side of PG16 and a leg (10 -> 18) that crossed it, and that crossing leg was intermittently failing the new regression test for reasons outside test_factory's own SQL. Since nothing in this extension can fix that pg_upgrade limitation, replace the two legs with 10 -> 15 (entirely pre-16) and 16 -> 18 (entirely post-16), so CI deliberately never exercises the one upgrade path that's known not to work, instead of flaking on it. Document the new rationale in the job's comment and add a matching "Known limitation" section to README.md with the manual GRANT ... WITH SET TRUE workaround for anyone who hits this after a real pre-16 -> 16+ pg_upgrade. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 38 ++++++++++++++++++++++++++------------ README.md | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 507bf1a..cff835b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,9 +18,14 @@ # 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 +195,22 @@ 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. + # This is not an oversight: binary pg_upgrade does not re-run an + # extension's install script -- it recreates catalog/member objects + # directly to preserve OIDs/relfilenodes for the physical file copy -- so + # whatever SET-enabled role-membership state existed on the OLD cluster at + # original-install time is all pg_upgrade's globals-restore step has to + # carry forward, and it does not preserve it as SET-enabled when crossing + # the pre-16->16+ boundary specifically (PG16 introduced the `WITH SET` + # grant option and pg_auth_members.set_option; see issue #14's `GRANT ... + # WITH SET TRUE` fix). Nothing in this extension's own SQL can fix that, + # because the install script simply isn't invoked again during a binary + # upgrade. See the README's "Known limitation" note (pg_upgrade across the + # PG16 boundary) for the user-facing workaround -- this matrix shape is why + # CI cannot cover that specific path, by design, and a manual regression + # there would go undetected. 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 +221,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 diff --git a/README.md b/README.md index fafece8..3906938 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,39 @@ so: PGOPTIONS=--search_path=extensions psql -d mydb -f test_factory.sql +Known limitation: binary pg_upgrade across PostgreSQL 16 +--------------------------------------------------------- + +On PostgreSQL 16 and later, `CREATE EXTENSION test_factory` grants the +installing role SET-enabled membership in `test_factory__owner` (`GRANT +test_factory__owner TO WITH SET TRUE`), which is what lets a +non-superuser installer later `SET ROLE test_factory__owner` as needed. + +If test_factory was installed on a cluster running PostgreSQL 15 or earlier, +and that cluster later underwent a **binary** `pg_upgrade` to PostgreSQL 16 +or later, the installing role's membership in `test_factory__owner` may come +through the upgrade *without* the SET option, even though the same upgrade +performed today (fresh-install on 16+) would have it. This happens because +binary `pg_upgrade` does not re-run an extension's install script -- it +reconstructs catalog objects directly to preserve OIDs/relfilenodes for the +physical file copy -- so there is no opportunity for the fix above to run +again during the upgrade itself. This is a `pg_upgrade` limitation, not a bug +in test_factory's install script, and nothing in that script can work around +it after the fact. + +If you hit a `must be able to SET ROLE "test_factory__owner"` error after +such an upgrade, the workaround is to have a superuser run, once, against the +affected database: + + GRANT test_factory__owner TO WITH SET TRUE; + +(substituting the actual role that owns/installed test_factory). CI does not +and cannot exercise this specific pg_upgrade-across-PG16 path (see +`.github/workflows/ci.yml`'s `pg-upgrade-test` job comment), so a future +regression here would not be caught automatically -- if in doubt after an +upgrade, just run the `GRANT` above; it's a no-op if the membership is +already SET-enabled. + Copyright and License --------------------- From 1466e5c79d517cbdb13750f6bf50445ab02dc65c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 16:34:05 -0500 Subject: [PATCH 05/17] Fix root cause of a real NOTICE, not just suppress it; tighten CI comment and README sql/test_factory.sql declared three function parameters as _tf._test_factory.set_name%TYPE, which Postgres can't preserve exactly in a function's formal parameter list -- it silently resolves to the underlying type (text, confirmed against the table definition) and emits a NOTICE every time the function is created. This has always been present (confirmed on master before any of this session's changes) and was being masked in every real install by test/install/load.sql's own `SET client_min_messages = WARNING`, which only exists for the test harness's own signal-to-noise, not as a substitute for fixing the extension itself -- a real CREATE EXTENSION at psql's default message level shows this notice today. Since the type is already known and %TYPE buys nothing for a function parameter (resolved once at CREATE FUNCTION time either way), declaring it as plain text removes the notice at its source instead of relying on a caller to suppress it. Regenerated test/build/expected/syntax.out, which now shows only the two already-known pg_extension_config_dump() errors, no NOTICE lines. Verified locally on PG12 and PG17, fresh and update modes. Also: shortened the pg-upgrade-test job's comment in ci.yml (the reasoning for the two-legs-avoiding-PG16 split doesn't need restating at length -- "pg_upgrade doesn't re-run the script" is the whole point), and tightened the README's "Known limitation" section -- stated what actually happens instead of hedging with "may", and dropped the CI-can't-test-this framing entirely (not something a user-facing README needs, and "can't" overstated it anyway -- it's a choice not to invest in covering this path, not an inherent limitation). --- .github/workflows/ci.yml | 19 ++++-------------- README.md | 35 ++++++++-------------------------- sql/test_factory--0.5.0.sql | 6 +++--- sql/test_factory.sql | 6 +++--- test/build/expected/syntax.out | 3 --- 5 files changed, 18 insertions(+), 51 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cff835b..5e887f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -196,21 +196,10 @@ jobs: # the current version directly on the old cluster. # # 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. - # This is not an oversight: binary pg_upgrade does not re-run an - # extension's install script -- it recreates catalog/member objects - # directly to preserve OIDs/relfilenodes for the physical file copy -- so - # whatever SET-enabled role-membership state existed on the OLD cluster at - # original-install time is all pg_upgrade's globals-restore step has to - # carry forward, and it does not preserve it as SET-enabled when crossing - # the pre-16->16+ boundary specifically (PG16 introduced the `WITH SET` - # grant option and pg_auth_members.set_option; see issue #14's `GRANT ... - # WITH SET TRUE` fix). Nothing in this extension's own SQL can fix that, - # because the install script simply isn't invoked again during a binary - # upgrade. See the README's "Known limitation" note (pg_upgrade across the - # PG16 boundary) for the user-facing workaround -- this matrix shape is why - # CI cannot cover that specific path, by design, and a manual regression - # there would go undetected. + # PostgreSQL 16 boundary (10->15 and 16->18), never crossing PG16 itself: + # binary pg_upgrade doesn't re-run the install script, so issue #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, diff --git a/README.md b/README.md index 3906938..73047f3 100644 --- a/README.md +++ b/README.md @@ -92,35 +92,16 @@ so: Known limitation: binary pg_upgrade across PostgreSQL 16 --------------------------------------------------------- -On PostgreSQL 16 and later, `CREATE EXTENSION test_factory` grants the -installing role SET-enabled membership in `test_factory__owner` (`GRANT -test_factory__owner TO WITH SET TRUE`), which is what lets a -non-superuser installer later `SET ROLE test_factory__owner` as needed. - -If test_factory was installed on a cluster running PostgreSQL 15 or earlier, -and that cluster later underwent a **binary** `pg_upgrade` to PostgreSQL 16 -or later, the installing role's membership in `test_factory__owner` may come -through the upgrade *without* the SET option, even though the same upgrade -performed today (fresh-install on 16+) would have it. This happens because -binary `pg_upgrade` does not re-run an extension's install script -- it -reconstructs catalog objects directly to preserve OIDs/relfilenodes for the -physical file copy -- so there is no opportunity for the fix above to run -again during the upgrade itself. This is a `pg_upgrade` limitation, not a bug -in test_factory's install script, and nothing in that script can work around -it after the fact. - -If you hit a `must be able to SET ROLE "test_factory__owner"` error after -such an upgrade, the workaround is to have a superuser run, once, against the -affected database: +On PostgreSQL 16+, `CREATE EXTENSION test_factory` grants the installing +role SET-enabled membership in `test_factory__owner`, which is what lets a +non-superuser installer `SET ROLE test_factory__owner` as needed. Binary +`pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 install +across the PostgreSQL 16 boundary loses that SET option. - GRANT test_factory__owner TO WITH SET TRUE; +If you hit `must be able to SET ROLE "test_factory__owner"` after such an +upgrade, fix it once as a superuser: -(substituting the actual role that owns/installed test_factory). CI does not -and cannot exercise this specific pg_upgrade-across-PG16 path (see -`.github/workflows/ci.yml`'s `pg-upgrade-test` job comment), so a future -regression here would not be caught automatically -- if in doubt after an -upgrade, just run the `GRANT` above; it's a no-op if the membership is -already SET-enabled. + GRANT test_factory__owner TO WITH SET TRUE; Copyright and License --------------------- diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index afe9400..cc003a5 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -91,7 +91,7 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ' CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE + , set_name text ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE v_factory_id_text text; @@ -129,7 +129,7 @@ $body$; CREATE OR REPLACE FUNCTION _tf.test_factory__get( table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE + , set_name text , table_oid oid -- Must be passed in because of forced search_path ) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ DECLARE @@ -149,7 +149,7 @@ END $body$; CREATE OR REPLACE FUNCTION tf.test_factory__get( table_name text - , set_name _tf._test_factory.set_name%TYPE + , set_name text ) RETURNS _tf._test_factory LANGUAGE sql AS $body$ SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) $body$; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 21dca18..4b8d6ce 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -90,7 +90,7 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ' CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE + , set_name text ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE v_factory_id_text text; @@ -128,7 +128,7 @@ $body$; CREATE OR REPLACE FUNCTION _tf.test_factory__get( table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE + , set_name text , table_oid oid -- Must be passed in because of forced search_path ) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ DECLARE @@ -148,7 +148,7 @@ END $body$; CREATE OR REPLACE FUNCTION tf.test_factory__get( table_name text - , set_name _tf._test_factory.set_name%TYPE + , set_name text ) RETURNS _tf._test_factory LANGUAGE sql AS $body$ SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) $body$; diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index ed15c52..fb8b1cf 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,6 +1,3 @@ \set ECHO none psql:sql/test_factory.sql:87: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION psql:sql/test_factory.sql:88: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:126: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text -psql:sql/test_factory.sql:148: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text -psql:sql/test_factory.sql:154: NOTICE: type reference _tf._test_factory.set_name%TYPE converted to text From 98cff1cb194cb29c5617a1935872bd5e39e5c4e4 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 16:46:45 -0500 Subject: [PATCH 06/17] Revert %TYPE removal (intentional); suppress both known NOTICEs in the script itself %TYPE on set_name parameters ties them to _test_factory.set_name's real column type, so a future column-type change can't silently drift out of sync with a hardcoded `text` -- restoring it, with a comment explaining why it's there despite the NOTICE it prints. Both that NOTICE and the GRANT-already-granted NOTICE from the previous commit are genuinely harmless install-time noise with no cleaner structural fix available (%TYPE can't be preserved exactly in a function's parameter list; re-granting an already-current membership is an intentional no-op, not a bug). The right place to quiet them is the install script itself, once, via SET LOCAL client_min_messages = WARNING right after saving the caller's original role -- SET LOCAL reverts automatically at the end of the install's own transaction, so it never leaks into the calling session, unlike relying on a caller (or, previously, test/install/load.sql) to set client_min_messages themselves. This also means the GRANT no longer needs its own existence-check guard against the notice -- back to a plain unconditional EXECUTE. Verified directly against a live database, not just the test suite: a genuinely fresh CREATE EXTENSION at psql's default message level, and a DROP/CREATE EXTENSION re-install with the role and grant already in place, both produce zero NOTICE output. Regenerated test/build/expected/syntax.out (now shows only the two known pg_extension_config_dump() errors, nothing else). Verified locally on PG12 and PG17, fresh and update modes. --- sql/test_factory--0.5.0.sql | 63 ++++++++++++++++++---------------- sql/test_factory.sql | 63 ++++++++++++++++++---------------- test/build/expected/syntax.out | 4 +-- 3 files changed, 70 insertions(+), 60 deletions(-) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index cc003a5..59caeac 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -6,6 +6,19 @@ * it's much lighter weight than creating a table. */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); + +/* + * SET LOCAL, not SET: reverts automatically at the end of this install's + * transaction, so it's scoped entirely to this script and never leaks into + * the calling session afterward -- nobody installing this extension needs + * to touch client_min_messages themselves, before or after. This covers a + * couple of harmless NOTICEs later in this file that Postgres emits for + * things this script does on purpose (see the %TYPE and GRANT comments + * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet + * without asking every caller to do it themselves. + */ +SET LOCAL client_min_messages = WARNING; + DO $body$ BEGIN CREATE ROLE test_factory__owner; @@ -19,36 +32,18 @@ $body$; * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs even when - * the role already existed and CREATE ROLE was a no-op, but only when the - * membership isn't already SET-enabled -- an unconditional re-GRANT is a - * harmless no-op, but still emits a NOTICE naming the installing role, which - * would make test/build/syntax.sql's raw re-run of this file (after - * test/install/load.sql already installed it once) produce output that - * differs by whichever role happens to be running the install. The - * existence check itself must go through EXECUTE too, not just the GRANT -- - * pg_auth_members.set_option doesn't exist before PG16, so a plain (static) - * reference to it would fail to parse on older servers even inside this - * same version-gated IF, since PL/pgSQL parses a query's text as soon as it - * reaches that statement, before evaluating whether the branch actually - * runs. Building the query as a string and only EXECUTEing it once already - * inside the PG16+ branch defers that parsing until it's safe. Pre-16 - * GRANT ... TO already confers the ability to SET ROLE, so none of this - * runs there. + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op and the membership is already SET-enabled -- that's a harmless + * no-op GRANT (the NOTICE it would otherwise print is exactly what the + * client_min_messages setting above is for). Gated on PG16+, where the + * WITH SET syntax exists; pre-16 GRANT ... TO already confers the ability + * to SET ROLE. */ DO $body$ -DECLARE - already_set_enabled boolean; BEGIN IF current_setting('server_version_num')::int >= 160000 THEN - EXECUTE format( - 'SELECT EXISTS (SELECT 1 FROM pg_auth_members WHERE roleid = %L::regrole AND member = %L::regrole AND set_option)' - , 'test_factory__owner' - , current_user - ) INTO already_set_enabled; - IF NOT already_set_enabled THEN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); - END IF; + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); END IF; END $body$; @@ -89,9 +84,19 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); +/* + * %TYPE here (and on the other two set_name parameters below) is + * intentional, not an oversight: it keeps this parameter's type tied to + * _test_factory.set_name's actual column type, so a future change to that + * column doesn't silently create a mismatch here that a hardcoded `text` + * would miss. Postgres can't preserve a %TYPE reference exactly in a + * function's parameter list -- it resolves it once at CREATE FUNCTION + * time and prints a NOTICE saying so every time. Harmless, and already + * suppressed for the whole install by client_min_messages above. + */ CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() - , set_name text + , set_name _tf._test_factory.set_name%TYPE ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE v_factory_id_text text; @@ -129,7 +134,7 @@ $body$; CREATE OR REPLACE FUNCTION _tf.test_factory__get( table_name text -- Sanitized by tf.test_factory__get() - , set_name text + , set_name _tf._test_factory.set_name%TYPE , table_oid oid -- Must be passed in because of forced search_path ) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ DECLARE @@ -149,7 +154,7 @@ END $body$; CREATE OR REPLACE FUNCTION tf.test_factory__get( table_name text - , set_name text + , set_name _tf._test_factory.set_name%TYPE ) RETURNS _tf._test_factory LANGUAGE sql AS $body$ SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) $body$; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 4b8d6ce..c0f1701 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -5,6 +5,19 @@ * it's much lighter weight than creating a table. */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); + +/* + * SET LOCAL, not SET: reverts automatically at the end of this install's + * transaction, so it's scoped entirely to this script and never leaks into + * the calling session afterward -- nobody installing this extension needs + * to touch client_min_messages themselves, before or after. This covers a + * couple of harmless NOTICEs later in this file that Postgres emits for + * things this script does on purpose (see the %TYPE and GRANT comments + * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet + * without asking every caller to do it themselves. + */ +SET LOCAL client_min_messages = WARNING; + DO $body$ BEGIN CREATE ROLE test_factory__owner; @@ -18,36 +31,18 @@ $body$; * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs even when - * the role already existed and CREATE ROLE was a no-op, but only when the - * membership isn't already SET-enabled -- an unconditional re-GRANT is a - * harmless no-op, but still emits a NOTICE naming the installing role, which - * would make test/build/syntax.sql's raw re-run of this file (after - * test/install/load.sql already installed it once) produce output that - * differs by whichever role happens to be running the install. The - * existence check itself must go through EXECUTE too, not just the GRANT -- - * pg_auth_members.set_option doesn't exist before PG16, so a plain (static) - * reference to it would fail to parse on older servers even inside this - * same version-gated IF, since PL/pgSQL parses a query's text as soon as it - * reaches that statement, before evaluating whether the branch actually - * runs. Building the query as a string and only EXECUTEing it once already - * inside the PG16+ branch defers that parsing until it's safe. Pre-16 - * GRANT ... TO already confers the ability to SET ROLE, so none of this - * runs there. + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op and the membership is already SET-enabled -- that's a harmless + * no-op GRANT (the NOTICE it would otherwise print is exactly what the + * client_min_messages setting above is for). Gated on PG16+, where the + * WITH SET syntax exists; pre-16 GRANT ... TO already confers the ability + * to SET ROLE. */ DO $body$ -DECLARE - already_set_enabled boolean; BEGIN IF current_setting('server_version_num')::int >= 160000 THEN - EXECUTE format( - 'SELECT EXISTS (SELECT 1 FROM pg_auth_members WHERE roleid = %L::regrole AND member = %L::regrole AND set_option)' - , 'test_factory__owner' - , current_user - ) INTO already_set_enabled; - IF NOT already_set_enabled THEN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); - END IF; + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); END IF; END $body$; @@ -88,9 +83,19 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); +/* + * %TYPE here (and on the other two set_name parameters below) is + * intentional, not an oversight: it keeps this parameter's type tied to + * _test_factory.set_name's actual column type, so a future change to that + * column doesn't silently create a mismatch here that a hardcoded `text` + * would miss. Postgres can't preserve a %TYPE reference exactly in a + * function's parameter list -- it resolves it once at CREATE FUNCTION + * time and prints a NOTICE saying so every time. Harmless, and already + * suppressed for the whole install by client_min_messages above. + */ CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() - , set_name text + , set_name _tf._test_factory.set_name%TYPE ) RETURNS name LANGUAGE plpgsql AS $body$ DECLARE v_factory_id_text text; @@ -128,7 +133,7 @@ $body$; CREATE OR REPLACE FUNCTION _tf.test_factory__get( table_name text -- Sanitized by tf.test_factory__get() - , set_name text + , set_name _tf._test_factory.set_name%TYPE , table_oid oid -- Must be passed in because of forced search_path ) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ DECLARE @@ -148,7 +153,7 @@ END $body$; CREATE OR REPLACE FUNCTION tf.test_factory__get( table_name text - , set_name text + , set_name _tf._test_factory.set_name%TYPE ) RETURNS _tf._test_factory LANGUAGE sql AS $body$ SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) $body$; diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index fb8b1cf..03ec399 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:87: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:88: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:82: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:83: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION From a3a078fb3c154b313832afb77ee1e95f691516e0 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 16:59:41 -0500 Subject: [PATCH 07/17] Drop the %TYPE explanatory comment -- it's a common idiom, not an oversight Doesn't need justifying inline every time it's used. --- sql/test_factory--0.5.0.sql | 13 +------------ sql/test_factory.sql | 13 +------------ test/build/expected/syntax.out | 2 +- 3 files changed, 3 insertions(+), 25 deletions(-) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 59caeac..a0f7ea5 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -12,8 +12,7 @@ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); * transaction, so it's scoped entirely to this script and never leaks into * the calling session afterward -- nobody installing this extension needs * to touch client_min_messages themselves, before or after. This covers a - * couple of harmless NOTICEs later in this file that Postgres emits for - * things this script does on purpose (see the %TYPE and GRANT comments + * couple of harmless NOTICEs later in this file (see the GRANT comment * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet * without asking every caller to do it themselves. */ @@ -84,16 +83,6 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); -/* - * %TYPE here (and on the other two set_name parameters below) is - * intentional, not an oversight: it keeps this parameter's type tied to - * _test_factory.set_name's actual column type, so a future change to that - * column doesn't silently create a mismatch here that a hardcoded `text` - * would miss. Postgres can't preserve a %TYPE reference exactly in a - * function's parameter list -- it resolves it once at CREATE FUNCTION - * time and prints a NOTICE saying so every time. Harmless, and already - * suppressed for the whole install by client_min_messages above. - */ CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() , set_name _tf._test_factory.set_name%TYPE diff --git a/sql/test_factory.sql b/sql/test_factory.sql index c0f1701..94369b1 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -11,8 +11,7 @@ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); * transaction, so it's scoped entirely to this script and never leaks into * the calling session afterward -- nobody installing this extension needs * to touch client_min_messages themselves, before or after. This covers a - * couple of harmless NOTICEs later in this file that Postgres emits for - * things this script does on purpose (see the %TYPE and GRANT comments + * couple of harmless NOTICEs later in this file (see the GRANT comment * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet * without asking every caller to do it themselves. */ @@ -83,16 +82,6 @@ SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); -/* - * %TYPE here (and on the other two set_name parameters below) is - * intentional, not an oversight: it keeps this parameter's type tied to - * _test_factory.set_name's actual column type, so a future change to that - * column doesn't silently create a mismatch here that a hardcoded `text` - * would miss. Postgres can't preserve a %TYPE reference exactly in a - * function's parameter list -- it resolves it once at CREATE FUNCTION - * time and prints a NOTICE saying so every time. Harmless, and already - * suppressed for the whole install by client_min_messages above. - */ CREATE OR REPLACE FUNCTION _tf.data_table_name( table_name text -- Sanitized by tf.test_factory__get() , set_name _tf._test_factory.set_name%TYPE diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index 03ec399..e80bf58 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none +psql:sql/test_factory.sql:81: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION psql:sql/test_factory.sql:82: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:83: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION From 49b03861abfcafbe0c80dd18c9df72c3ebef4cbd Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 17:45:35 -0500 Subject: [PATCH 08/17] Fix a real regression: GRANT requires ADMIN OPTION even for a no-op re-grant Caught in review (PR #18 discussion): GRANT TO requires the grantor to hold ADMIN OPTION on the target role (or be superuser) to run AT ALL -- regardless of whether the grant would end up a no-op. Running it unconditionally broke an already-working setup: a role with pre-existing SET-enabled membership on test_factory__owner (however it got there -- e.g. a DBA pre-provisioned the role and granted SET directly, deliberately withholding ADMIN OPTION as a least-privilege measure) would previously install fine, but this fix's own GRANT attempt now fails with "permission denied to grant role ... Only roles with the ADMIN option ... may grant this role" -- a regression this PR introduced, confirmed against a live non-superuser role in exactly that configuration. Fixed by gating the GRANT on pg_has_role(current_user, 'test_factory__owner', 'SET') -- skip it entirely when already true. 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 the same version-gated branch without needing EXECUTE to defer parsing, unlike the earlier pg_auth_members.set_option approach -- confirmed this doesn't break PG12. If the installer has neither SET-enabled membership nor ADMIN OPTION to grant it themselves, installation genuinely cannot proceed -- nothing in this script can grant a privilege on someone else's behalf. Catch that specific permission failure and raise a clear, actionable error naming exactly who needs to run what, instead of letting Postgres's generic "permission denied to grant role" surface. Verified against a live non-superuser role with neither SET nor ADMIN OPTION. (Hit and fixed a real bug in the fix itself while verifying live: RAISE's %-substitution is plain string interpolation, not format()'s %I/%L -- using %I directly in a RAISE message string produced "roleI" instead of a quoted identifier. Built the suggested command with format() first, then substituted the whole result in with an ordinary %.) README: broadened the "Known limitation" section -- the SET-enabled membership requirement isn't only a pg_upgrade concern; a fresh install by a role lacking both SET-enabled membership and ADMIN OPTION hits the same underlying requirement, just detected immediately now instead of failing later. Kept the pg_upgrade case separately, since that one genuinely can't be caught up front (no install script runs during a binary upgrade) and still needs the same manual GRANT afterward. Verified locally on PG12 and PG17 (fresh and update modes), plus directly against a live database: an installer with pre-existing SET-enabled membership but no ADMIN OPTION now installs cleanly (no failed GRANT attempt), and an installer with neither gets the new, clear error message. --- README.md | 23 ++++++++------ sql/test_factory--0.5.0.sql | 57 ++++++++++++++++++++++++++-------- sql/test_factory.sql | 57 ++++++++++++++++++++++++++-------- test/build/expected/syntax.out | 4 +-- 4 files changed, 104 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 73047f3..fa8c3e9 100644 --- a/README.md +++ b/README.md @@ -89,17 +89,22 @@ so: PGOPTIONS=--search_path=extensions psql -d mydb -f test_factory.sql -Known limitation: binary pg_upgrade across PostgreSQL 16 ---------------------------------------------------------- +Known limitation: SET-enabled membership in test_factory__owner +------------------------------------------------------------------ On PostgreSQL 16+, `CREATE EXTENSION test_factory` grants the installing -role SET-enabled membership in `test_factory__owner`, which is what lets a -non-superuser installer `SET ROLE test_factory__owner` as needed. Binary -`pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 install -across the PostgreSQL 16 boundary loses that SET option. - -If you hit `must be able to SET ROLE "test_factory__owner"` after such an -upgrade, fix it once as a superuser: +role SET-enabled membership in `test_factory__owner`, needed to `SET ROLE +test_factory__owner` as a non-superuser -- 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. + +Binary `pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 +install across the PostgreSQL 16 boundary can leave a database without the +SET-enabled grant a fresh install would have set up. Since no install +script runs during the upgrade, this case isn't caught up front -- it +surfaces later, as `must be able to SET ROLE "test_factory__owner"` from +ordinary use (e.g. `tf.register()`). Fix it once as a superuser: GRANT test_factory__owner TO WITH SET TRUE; diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index a0f7ea5..7ab8aa1 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -12,9 +12,10 @@ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); * transaction, so it's scoped entirely to this script and never leaks into * the calling session afterward -- nobody installing this extension needs * to touch client_min_messages themselves, before or after. This covers a - * couple of harmless NOTICEs later in this file (see the GRANT comment - * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet - * without asking every caller to do it themselves. + * harmless NOTICE later in this file, from a %TYPE reference Postgres + * can't preserve exactly in a function's parameter list; suppressing it + * here, once, keeps a real CREATE EXTENSION quiet without asking every + * caller to do it themselves. */ SET LOCAL client_min_messages = WARNING; @@ -30,19 +31,49 @@ $body$; /* * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails - * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs - * unconditionally, even when the role already existed and CREATE ROLE was a - * no-op and the membership is already SET-enabled -- that's a harmless - * no-op GRANT (the NOTICE it would otherwise print is exactly what the - * client_min_messages setting above is for). Gated on PG16+, where the - * WITH SET syntax exists; pre-16 GRANT ... TO already confers the ability - * to SET ROLE. + * unless the current role is a superuser (which bypasses the check). Grant + * it explicitly WITH SET so a non-superuser install works too -- but only + * when it's actually missing (pg_has_role's 'SET' privilege type, PG16+ + * only, but safe to call as a plain function argument inside this same + * version-gated branch -- unlike a static reference to + * pg_auth_members.set_option, which would fail on older servers even + * inside the gate). An installer might already have SET-enabled membership + * 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 SET-enabled membership 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 - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + IF current_setting('server_version_num')::int >= 160000 + AND 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + , current_user + , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + END; END IF; END $body$; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 94369b1..e53c362 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -11,9 +11,10 @@ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); * transaction, so it's scoped entirely to this script and never leaks into * the calling session afterward -- nobody installing this extension needs * to touch client_min_messages themselves, before or after. This covers a - * couple of harmless NOTICEs later in this file (see the GRANT comment - * below); suppressing them here, once, keeps a real CREATE EXTENSION quiet - * without asking every caller to do it themselves. + * harmless NOTICE later in this file, from a %TYPE reference Postgres + * can't preserve exactly in a function's parameter list; suppressing it + * here, once, keeps a real CREATE EXTENSION quiet without asking every + * caller to do it themselves. */ SET LOCAL client_min_messages = WARNING; @@ -29,19 +30,49 @@ $body$; /* * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled * membership in the new role, so SET ROLE test_factory__owner below fails - * unless the current role is a superuser (which bypasses the check). Grant it - * explicitly WITH SET so a non-superuser install works too. Runs - * unconditionally, even when the role already existed and CREATE ROLE was a - * no-op and the membership is already SET-enabled -- that's a harmless - * no-op GRANT (the NOTICE it would otherwise print is exactly what the - * client_min_messages setting above is for). Gated on PG16+, where the - * WITH SET syntax exists; pre-16 GRANT ... TO already confers the ability - * to SET ROLE. + * unless the current role is a superuser (which bypasses the check). Grant + * it explicitly WITH SET so a non-superuser install works too -- but only + * when it's actually missing (pg_has_role's 'SET' privilege type, PG16+ + * only, but safe to call as a plain function argument inside this same + * version-gated branch -- unlike a static reference to + * pg_auth_members.set_option, which would fail on older servers even + * inside the gate). An installer might already have SET-enabled membership + * 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 SET-enabled membership 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 - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + IF current_setting('server_version_num')::int >= 160000 + AND 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + , current_user + , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + END; END IF; END $body$; diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index e80bf58..118f54f 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:81: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:82: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:112: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:113: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION From 30ef622c7d455dd81c716106de4176f741e0da85 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 17:50:43 -0500 Subject: [PATCH 09/17] Fix regression test to check pg_has_role, not a raw pg_auth_members row CI caught this immediately (all PG10-18 jobs failed identically): the previous version of this test queried pg_auth_members directly for a literal grant row naming the installing role. That's not what the fix actually guarantees, and it stopped being true the moment the GRANT became conditional (previous commit) -- CI's installer is a real superuser (`postgres`), and a superuser always has effective SET privilege on every role via bypass, without needing (or, now, receiving) an explicit grant. The now-conditional GRANT correctly skips granting a superuser something they don't need, so the literal catalog row this test checked for was never created, and the assertion failed even though the underlying property (can this role SET ROLE test_factory__owner) was never actually false. Fixed by checking pg_has_role(current_user, 'test_factory__owner', 'SET') instead -- the same idiom the fix's own gating logic already uses, and the actual property in question. Correctly returns true for a superuser via bypass (no grant needed) and true for a non-superuser with an explicit grant (however they got it), matching what "SET-enabled membership" is actually supposed to mean here. My own local verification runs had been passing throughout today's earlier commits by accident: leftover GRANT ... WITH SET TRUE state from manual testing earlier in this session had polluted the shared local cluster, making the old catalog-row check pass locally against contaminated state that CI's genuinely fresh cluster never had. Re-verified this fix against a manually-cleaned local state (REVOKE test_factory__owner FROM root, postgres) on both PG17 and PG12, fresh and update modes -- confirmed the assertion now passes correctly, and confirmed no catalog grant gets created for a superuser install either way. --- test/sql/base.sql | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/test/sql/base.sql b/test/sql/base.sql index 07d6327..0d1cc1d 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -6,25 +6,31 @@ /* * Regression test for issue #14. On PostgreSQL 16+, CREATE ROLE no longer * grants the creating role a SET-enabled membership in the new role, so the - * install must GRANT test_factory__owner ... WITH SET TRUE or the SET ROLE - * performed during install fails for non-superuser installs (RDS/Aurora). A - * real superuser bypasses the SET ROLE check, so a plain install here cannot - * reproduce the failure; instead assert the SET-enabled membership the fix - * establishes. pg_auth_members.set_option only exists on PG16+, so the check is - * skipped (with identical TAP output) on older versions, where a plain - * GRANT ... TO already confers the ability to SET ROLE. + * install must GRANT test_factory__owner ... WITH SET TRUE (when needed -- + * see sql/test_factory.sql's own comment) or the SET ROLE performed during + * install fails for non-superuser installs (RDS/Aurora). + * + * pg_has_role(), not a raw pg_auth_members query: a real superuser always + * has effective SET privilege on every role (bypasses the check entirely, + * no explicit grant needed), which pg_has_role correctly reports as true -- + * a literal catalog-row check would not, since CI's installer here IS a + * real superuser and the fix's own gating logic (also pg_has_role-based) + * correctly skips granting a superuser something they don't need. This is + * what the fix actually guarantees -- "can this role SET ROLE + * test_factory__owner", not "does a specific catalog row exist" -- and + * checking it the same way the fix does is what makes this a real + * regression test rather than an assertion about an implementation detail. + * + * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as + * sql/test_factory.sql), so the check is skipped (identical TAP output) on + * older versions, where a plain GRANT ... TO already confers the ability + * to SET ROLE. */ SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus \gset -- pg16+ SET-enabled membership check \if :pg16plus SELECT ok( - EXISTS( - SELECT 1 - FROM pg_auth_members - WHERE roleid = 'test_factory__owner'::regrole - AND member = current_user::regrole - AND set_option - ) + pg_has_role(current_user, 'test_factory__owner', 'SET') , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' ); -- pg16+ SET-enabled membership check From 93bbc3ff48e14238a2003d3629b76366a35747e7 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 10 Aug 2026 18:15:42 -0500 Subject: [PATCH 10/17] Don't SET client_min_messages in the install script; that's test/build's job Per ../ai/CODE_STYLE.md (already-merged, authoritative policy that names this exact repo/PR as one of several where this mistake was reintroduced): CREATE EXTENSION/ALTER EXTENSION UPDATE already forces client_min_messages up to at least WARNING for the duration of an install script, restoring the caller's original setting the moment the script finishes -- confirmed directly against execute_extension_script() in Postgres's own source, and verified empirically here: a real CREATE EXTENSION never showed the %TYPE NOTICE in the first place, with or without my own SET LOCAL, because Postgres was already suppressing it. Adding it in the script itself was redundant at best, and worse than doing nothing in general: Postgres's mechanism only ever *raises* the level, but an unconditional SET LOCAL unconditionally *lowers* a caller who set something stricter (e.g. ERROR). Moved the suppression to test/build/syntax.sql instead, immediately before the \i -- that's the one place that actually needs it, since running the script bare via \i gets none of CREATE EXTENSION's built-in handling. Also fixed a bare `issue #14` reference in ci.yml to a full URL, per ../ai/CODE_STYLE.md's rule for references to a still-relevant known limitation (as opposed to purely historical context, where a bare number is fine). Verified locally on PG12 and PG17, fresh and update modes, against a manually-cleaned (non-polluted) local role state. --- .github/workflows/ci.yml | 3 ++- sql/test_factory--0.5.0.sql | 12 ------------ sql/test_factory.sql | 12 ------------ test/build/expected/syntax.out | 4 ++-- test/build/syntax.sql | 11 +++++++++++ 5 files changed, 15 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e887f3..7125107 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -197,7 +197,8 @@ jobs: # # 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 issue #14's + # 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: diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 7ab8aa1..7ef5cbc 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -7,18 +7,6 @@ */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); -/* - * SET LOCAL, not SET: reverts automatically at the end of this install's - * transaction, so it's scoped entirely to this script and never leaks into - * the calling session afterward -- nobody installing this extension needs - * to touch client_min_messages themselves, before or after. This covers a - * harmless NOTICE later in this file, from a %TYPE reference Postgres - * can't preserve exactly in a function's parameter list; suppressing it - * here, once, keeps a real CREATE EXTENSION quiet without asking every - * caller to do it themselves. - */ -SET LOCAL client_min_messages = WARNING; - DO $body$ BEGIN CREATE ROLE test_factory__owner; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index e53c362..93f123e 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -6,18 +6,6 @@ */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); -/* - * SET LOCAL, not SET: reverts automatically at the end of this install's - * transaction, so it's scoped entirely to this script and never leaks into - * the calling session afterward -- nobody installing this extension needs - * to touch client_min_messages themselves, before or after. This covers a - * harmless NOTICE later in this file, from a %TYPE reference Postgres - * can't preserve exactly in a function's parameter list; suppressing it - * here, once, keeps a real CREATE EXTENSION quiet without asking every - * caller to do it themselves. - */ -SET LOCAL client_min_messages = WARNING; - DO $body$ BEGIN CREATE ROLE test_factory__owner; diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index 118f54f..7499796 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:112: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:113: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:100: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:101: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION diff --git a/test/build/syntax.sql b/test/build/syntax.sql index 3c002b8..056cb9b 100644 --- a/test/build/syntax.sql +++ b/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 From c2c255b9b19552914825af324f3683f27276c0e3 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 13:02:47 -0500 Subject: [PATCH 11/17] Fix a deeper pre-16 bug, allow genuine non-superuser installs, add real coverage CREATE ROLE never grants the creator any relationship to the role it just created, on ANY PostgreSQL version -- not just PG16+ as the original fix assumed. This was only caught by actually running the suite as a genuine non-superuser: pre-16, `SET ROLE test_factory__owner` failed outright with zero prior grant, even though the original fix's comment claimed plain membership was already conferred automatically. Added the same GRANT-if-missing logic (gated on pg_has_role's 'MEMBER' privtype instead of 'SET') to both sql/test_factory.sql's test_factory__owner grant and test/install/load.sql's own test_role bootstrap, which has the identical issue. Added `superuser = false` to both control files -- previously neither set it, so `CREATE EXTENSION` was only reachable by an actual superuser on stock PostgreSQL regardless of role grants, making the whole fix unreachable via the extension's own documented install path (per PR #18 review). Added bin/test_nonsuperuser and a new `test-nonsuperuser` CI job (matrixed across every supported major, mirroring `test`) that runs the full suite through a disposable, real non-superuser installer role. Without this, `pg_has_role(current_user, ...)` is unconditionally true for the superuser CI otherwise runs as, so neither the fix's own gating logic nor test/sql/base.sql's regression assertion could ever fail regardless of whether the underlying GRANT logic worked (per PR #18 review). Also tightened base.sql's pre-16 branch, previously a literal `ok(true, ...)` with zero coverage, to the same pg_has_role-based check as PG16+. Updated README's "Known limitation" section to reflect that CREATE EXTENSION now genuinely reaches this scenario for a suitably-privileged non-superuser, covering both the PG16+ and pre-16 cases, and documented the new CI mechanism in test/CLAUDE.md. Verified locally on PG12 and PG17: fresh and update modes as superuser (no regressions), plus fresh mode as a genuine disposable non-superuser role, all passing. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 73 +++++++++++++++++++++-------- README.md | 14 ++++-- bin/test_nonsuperuser | 51 +++++++++++++++++++++ sql/test_factory--0.5.0.sql | 84 +++++++++++++++++++++------------- sql/test_factory.sql | 84 +++++++++++++++++++++------------- test/CLAUDE.md | 18 ++++++++ test/build/expected/syntax.out | 4 +- test/install/load.sql | 35 ++++++++++++++ test/sql/base.sql | 26 +++++++---- test_factory.control | 8 ++++ test_factory_pgtap.control | 2 + 11 files changed, 301 insertions(+), 98 deletions(-) create mode 100755 bin/test_nonsuperuser diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7125107..317e114 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,29 +3,36 @@ # A test_factory install can be arrived at two ways, each of which can break # differently, so each gets its own job below: # -# test -- FRESH install: CREATE EXTENSION at the current -# version, on every supported PostgreSQL major. The -# baseline a brand-new user gets. -# 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. +# test -- FRESH install: CREATE EXTENSION at the current +# version, on every supported PostgreSQL major, as +# the superuser CI itself runs as. The baseline a +# brand-new superuser install gets. +# test-nonsuperuser -- FRESH install, same as `test`, but as a disposable +# non-superuser installer role (bin/test_nonsuperuser) +# on every supported major. `test`'s superuser +# bypasses every role-membership check the fix for +# https://github.com/Postgres-Extensions/test_factory/issues/14 +# added, so only this job actually exercises it. +# 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. # # 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). 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. +# version in the wild to carry forward). Both `test` and `test-nonsuperuser` +# 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 for both jobs. `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 @@ -184,6 +191,34 @@ jobs: - name: Test on PostgreSQL ${{ matrix.pg }} run: make verify-results + # Proves test_factory_pgtap/test_factory are actually installable by a + # non-superuser (https://github.com/Postgres-Extensions/test_factory/issues/14), + # by running the whole suite as one -- not by asserting a property + # (pg_has_role(current_user, ...)) that's trivially true whenever the + # suite happens to run as the `test` job's own superuser regardless of + # whether the fix does anything (see PR #18's review comments). Matrixed + # across every supported major, not just one on either side of PG16: the + # fix itself branches there (pg_has_role's SET privtype is PG16+ only; + # pre-16 needs plain membership instead), so testing only one side would + # leave the other branch with the exact same always-green coverage gap + # this job exists to close. + test-nonsuperuser: + needs: [changes] + if: needs.changes.outputs.docs_only != 'true' + strategy: + matrix: + pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} + name: 🔒 PostgreSQL ${{ matrix.pg }} (non-superuser installer) + runs-on: ubuntu-latest + container: pgxn/pgxn-tools + steps: + - name: Start PostgreSQL ${{ matrix.pg }} + run: pg-start ${{ matrix.pg }} + - name: Check out the repo + uses: actions/checkout@v7 + - name: Run the suite as a disposable non-superuser installer role + run: bin/test_nonsuperuser run test_factory_nonsuperuser tf_installer + # Proves test_factory survives a BINARY pg_upgrade (in-place catalog # migration to a newer PostgreSQL major), not just a fresh CREATE EXTENSION. # Each leg: install the CURRENT version on an old cluster, binary-pg_upgrade @@ -283,7 +318,7 @@ jobs: # jobs gated off by the `changes` job on a docs-only push), and fails if any # failed or were cancelled. all-checks-passed: - needs: [changes, lint, test, pg-upgrade-test] + needs: [changes, lint, test, test-nonsuperuser, pg-upgrade-test] if: always() runs-on: ubuntu-latest steps: diff --git a/README.md b/README.md index fa8c3e9..7ad9350 100644 --- a/README.md +++ b/README.md @@ -92,11 +92,15 @@ so: Known limitation: SET-enabled membership in test_factory__owner ------------------------------------------------------------------ -On PostgreSQL 16+, `CREATE EXTENSION test_factory` grants the installing -role SET-enabled membership in `test_factory__owner`, needed to `SET ROLE -test_factory__owner` as a non-superuser -- 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 +`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. Binary `pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 diff --git a/bin/test_nonsuperuser b/bin/test_nonsuperuser new file mode 100755 index 0000000..6fb2ede --- /dev/null +++ b/bin/test_nonsuperuser @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# +# Exercise the test suite through a genuine, disposable NON-SUPERUSER +# installer role, to actually prove +# https://github.com/Postgres-Extensions/test_factory/issues/14's fix works +# -- rather than asserting a property (pg_has_role(current_user, ...)) that +# is trivially true whenever the suite happens to run as CI's own +# superuser, regardless of whether the fix does anything at all. Modeled on +# bin/test_existing. +# +# USAGE: bin/test_nonsuperuser run DB ROLE +# +# run DB ROLE +# Create DB and ROLE (LOGIN, NOSUPERUSER, CREATEROLE -- the exact +# privilege floor CREATE EXTENSION test_factory_pgtap needs: CREATEROLE +# for CREATE ROLE test_factory__owner, CREATE ON DATABASE for the +# schemas the install script creates), grant ROLE that CREATE +# privilege, then run the suite's fresh-install mode AS that role via +# PGUSER. --use-existing keeps pg_regress from dropping/recreating DB, +# which would throw away the GRANT just made. +set -euo pipefail + +cd "$(dirname "$(readlink -f "$0")")/.." + +run() { + local db=$1 role=$2 + createdb "$db" + createuser "$role" --no-superuser --no-createdb --createrole + psql -d "$db" -v ON_ERROR_STOP=1 -c "GRANT CREATE ON DATABASE \"$db\" TO \"$role\";" + PGUSER="$role" make verify-results \ + TEST_LOAD_SOURCE=fresh \ + CONTRIB_TESTDB="$db" \ + EXTRA_REGRESS_OPTS=--use-existing \ + PGXNTOOL_ENABLE_TEST_BUILD=no +} + +usage() { + echo "usage: bin/test_nonsuperuser run DB ROLE" >&2 + exit 2 +} + +main() { + local cmd=${1:-} + shift || true + case "$cmd" in + run) run "$@" ;; + *) usage ;; + esac +} + +main "$@" diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 7ef5cbc..db11d5f 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -17,25 +17,36 @@ END $body$; /* - * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled - * membership in the new role, so SET ROLE test_factory__owner below fails - * unless the current role is a superuser (which bypasses the check). Grant - * it explicitly WITH SET so a non-superuser install works too -- but only - * when it's actually missing (pg_has_role's 'SET' privilege type, PG16+ - * only, but safe to call as a plain function argument inside this same - * version-gated branch -- unlike a static reference to - * pg_auth_members.set_option, which would fail on older servers even - * inside the gate). An installer might already have SET-enabled membership - * 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"). + * 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. * - * If the installer has neither SET-enabled membership nor ADMIN OPTION to + * 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 @@ -43,24 +54,35 @@ $body$; */ DO $body$ BEGIN - IF current_setting('server_version_num')::int >= 160000 - AND NOT pg_has_role(current_user, 'test_factory__owner', 'SET') - THEN + 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + , current_user + , 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 WITH SET TRUE', current_user); + EXECUTE format('GRANT test_factory__owner TO %I', 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'Role "%" is not a member of "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' , current_user - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + , format('GRANT test_factory__owner TO %I;', current_user); END; END IF; END diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 93f123e..7746dff 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -16,25 +16,36 @@ END $body$; /* - * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled - * membership in the new role, so SET ROLE test_factory__owner below fails - * unless the current role is a superuser (which bypasses the check). Grant - * it explicitly WITH SET so a non-superuser install works too -- but only - * when it's actually missing (pg_has_role's 'SET' privilege type, PG16+ - * only, but safe to call as a plain function argument inside this same - * version-gated branch -- unlike a static reference to - * pg_auth_members.set_option, which would fail on older servers even - * inside the gate). An installer might already have SET-enabled membership - * 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"). + * 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. * - * If the installer has neither SET-enabled membership nor ADMIN OPTION to + * 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 @@ -42,24 +53,35 @@ $body$; */ DO $body$ BEGIN - IF current_setting('server_version_num')::int >= 160000 - AND NOT pg_has_role(current_user, 'test_factory__owner', 'SET') - THEN + 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + , current_user + , 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 WITH SET TRUE', current_user); + EXECUTE format('GRANT test_factory__owner TO %I', 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 "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'Role "%" is not a member of "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' , current_user - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + , format('GRANT test_factory__owner TO %I;', current_user); END; END IF; END diff --git a/test/CLAUDE.md b/test/CLAUDE.md index b536df9..c37a539 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -70,6 +70,24 @@ 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 (`bin/test_nonsuperuser`) + +Every mode above runs as whatever role CI or the developer happens to be +connected as -- normally a superuser, which 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` proves +nothing about whether the fix works; it's true either way. + +`bin/test_nonsuperuser run ` creates a disposable role (`LOGIN`, +`NOSUPERUSER`, `CREATEROLE`) and database, grants the role `CREATE` on that +database, then runs the suite's **fresh** mode with `PGUSER` set to that +role via `EXTRA_REGRESS_OPTS=--use-existing` (so `pg_regress` doesn't +drop/recreate the database out from under the grant just made). This is +the CI `test-nonsuperuser` job's own entry point -- see `.github/workflows/ +ci.yml`'s top-of-file comment. + ### Dependency Guard Planted only in `existing` mode (see `load.sql`): a view in schema diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index 7499796..b707c8b 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:100: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:101: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:122: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:123: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION diff --git a/test/install/load.sql b/test/install/load.sql index 1ad130c..f1a195d 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -58,6 +58,41 @@ SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'test_role') AS need_ CREATE ROLE :test_role; \endif +/* + * Same issue as test_factory__owner (see sql/test_factory.sql's comment): + * CREATE ROLE alone never grants the creator any relationship to the role + * it just created, on ANY version -- confirmed directly, this session's + * connecting role gets "permission denied to set role" on the very role + * it just created above, with zero prior grant. test/helpers/create.sql's + * later SET ROLE = test_role needs SET-enabled membership on PG16+, or + * plain membership pre-16 (no WITH SET syntax there yet). Either branch + * skips the GRANT when it's not actually needed, since GRANT ROLE needs + * ADMIN OPTION to run at all, even for a no-op re-grant. + * + * Plain SQL + \if, not a DO block: :test_role is a psql variable, and psql + * never substitutes :variables inside a dollar-quoted body (confirmed + * directly -- it sends the literal text ":test_role" to the server, + * producing a syntax error there instead of anywhere client-side that + * would point at the real problem). GRANT ... TO CURRENT_USER sidesteps + * needing the *installer's* name dynamically at all -- CURRENT_USER is a + * real keyword, not a value needing substitution. + */ +SELECT (current_setting('server_version_num')::int >= 160000) AS test_role_pg16plus +\gset +\if :test_role_pg16plus +SELECT NOT pg_has_role(current_user, :'test_role', 'SET') AS test_role_needs_grant +\gset +\if :test_role_needs_grant +GRANT :test_role TO CURRENT_USER WITH SET TRUE; +\endif +\else +SELECT NOT pg_has_role(current_user, :'test_role', 'MEMBER') AS test_role_needs_grant +\gset +\if :test_role_needs_grant +GRANT :test_role TO CURRENT_USER; +\endif +\endif + /* * psql's \if only accepts a plain boolean token, not a comparison * expression -- compute it via SQL first (\if :load_mode = 'existing' would diff --git a/test/sql/base.sql b/test/sql/base.sql index 0d1cc1d..d3ba6dd 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -4,14 +4,15 @@ -- test/install/load.sql already installed the extension, in every mode. /* - * Regression test for issue #14. On PostgreSQL 16+, CREATE ROLE no longer - * grants the creating role a SET-enabled membership in the new role, so the - * install must GRANT test_factory__owner ... WITH SET TRUE (when needed -- - * see sql/test_factory.sql's own comment) or the SET ROLE performed during - * install fails for non-superuser installs (RDS/Aurora). + * Regression test for issue #14. CREATE ROLE never grants the creating role + * any relationship to the role it just created, on any version, so the + * install must GRANT test_factory__owner explicitly (WITH SET TRUE on + * PostgreSQL 16+, plain on older versions -- see sql/test_factory.sql's own + * comment) or the SET ROLE performed during install fails for + * non-superuser installs (RDS/Aurora). * * pg_has_role(), not a raw pg_auth_members query: a real superuser always - * has effective SET privilege on every role (bypasses the check entirely, + * has effective SET/membership on every role (bypasses the check entirely, * no explicit grant needed), which pg_has_role correctly reports as true -- * a literal catalog-row check would not, since CI's installer here IS a * real superuser and the fix's own gating logic (also pg_has_role-based) @@ -21,10 +22,15 @@ * checking it the same way the fix does is what makes this a real * regression test rather than an assertion about an implementation detail. * + * Both branches only actually catch a regression when the suite runs as a + * genuine non-superuser (bin/test_nonsuperuser, the CI test-nonsuperuser + * job) -- pg_has_role is unconditionally true for the superuser this file + * otherwise runs as, same as it is for the fix's own gating logic. + * * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as - * sql/test_factory.sql), so the check is skipped (identical TAP output) on - * older versions, where a plain GRANT ... TO already confers the ability - * to SET ROLE. + * sql/test_factory.sql); pre-16, plain membership is the closest equivalent + * ('SET' isn't a meaningful distinction yet -- membership itself already + * confers the ability to SET ROLE). */ SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus \gset -- pg16+ SET-enabled membership check @@ -36,7 +42,7 @@ SELECT ok( -- pg16+ SET-enabled membership check \else SELECT ok( - true + pg_has_role(current_user, 'test_factory__owner', 'MEMBER') , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' ); -- pg16+ SET-enabled membership check diff --git a/test_factory.control b/test_factory.control index a18e438..b05ee89 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,3 +1,11 @@ comment = 'A framework for managing test data' default_version = '0.5.0' relocatable = false +# Not a security boundary weakening: test_factory__owner is a locked-down, +# dedicated owner role, and every privileged function is SECURITY DEFINER +# with search_path=pg_catalog specifically so this is safe to install +# without a real superuser (e.g. RDS/Aurora's non-superuser master user). +# Installing still requires CREATEROLE (for test_factory__owner) plus +# CREATE on the target database -- an ordinary role gets neither by +# default. +superuser = false diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index 8e4a75f..0711698 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -2,3 +2,5 @@ comment = 'A framework for managing test data' default_version = '0.1.0' relocatable = false requires = 'pgtap, test_factory' +# See test_factory.control -- same rationale. +superuser = false From cc0fc959d5fe489b05079aaf027a2e78041da870 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 13:37:08 -0500 Subject: [PATCH 12/17] README: fix wrong symptom for the pg_upgrade SET-enabled-grant limitation tf.register()/tf.get() never run SET ROLE -- they go through SECURITY DEFINER functions (_tf.test_factory__set, _tf.get, etc, all owned by test_factory__owner), which transparently run as the owner without any role-membership check. SET ROLE only appears once, inside the install script itself (sql/test_factory.sql:108). A missing SET-enabled grant can only bite the next install/reinstall action that re-runs the script, not ordinary use of the public API. Per PR #18 review. Co-Authored-By: Claude Sonnet 5 --- README.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7ad9350..18e571f 100644 --- a/README.md +++ b/README.md @@ -107,8 +107,10 @@ Binary `pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 install across the PostgreSQL 16 boundary can leave a database without the SET-enabled grant a fresh install would have set up. Since no install script runs during the upgrade, this case isn't caught up front -- it -surfaces later, as `must be able to SET ROLE "test_factory__owner"` from -ordinary use (e.g. `tf.register()`). Fix it once as a superuser: +surfaces later, the next time an install/reinstall action runs `SET ROLE` +(e.g. re-running `CREATE EXTENSION`) -- not from ordinary use of +`tf.register()` or `tf.get()`, neither of which ever run `SET ROLE`. Fix it +once as a superuser: GRANT test_factory__owner TO WITH SET TRUE; From 102a50853a116c52850bb75eb6b6b5c9fdc681c5 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 14:47:30 -0500 Subject: [PATCH 13/17] Fold non-superuser install testing into the normal suite, drop the extra CI leg Per feedback: running the whole suite twice per PostgreSQL major (once as CI's superuser, once as a separate disposable non-superuser role via bin/test_nonsuperuser + a parallel test-nonsuperuser CI job) was overkill. A superuser bypasses every role-membership check involved in issue #14's fix, so if a properly-privileged non-superuser can install successfully, a superuser certainly can too -- there's no distinct superuser-only code path worth testing separately. test/install/load.sql's fresh/update branch now creates a non-login test_factory_installer role (CREATEROLE only, no superuser bypass) and switches to it via SET SESSION AUTHORIZATION before running CREATE EXTENSION/ALTER EXTENSION UPDATE -- no separate connection, password, or CI job needed, since the switch only affects load.sql's own already-authenticated session. Every mode this suite runs in (CI's superuser, a developer's local superuser) now exercises the same non-superuser install path automatically. Removed bin/test_nonsuperuser and the parallel test-nonsuperuser CI job entirely. Moved the issue #14 regression check out of test/sql/base.sql (which runs in a SEPARATE connection from load.sql, so it was checking pg_has_role for the wrong role -- whatever ambient superuser connects, not the installer that actually ran CREATE EXTENSION) into load.sql itself, right after install, still running as test_factory_installer. Verified locally on PG12 and PG17, fresh and update modes, as the ambient superuser only -- no PGUSER override, no separate role provisioning step. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 87 +++++++++++++++------------------------- bin/test_nonsuperuser | 51 ----------------------- test/CLAUDE.md | 32 ++++++++------- test/expected/base.out | 43 ++++++++++---------- test/install/load.sql | 78 +++++++++++++++++++++++++++++++++++ test/roles.sql | 3 ++ test/sql/base.sql | 48 +--------------------- 7 files changed, 155 insertions(+), 187 deletions(-) delete mode 100755 bin/test_nonsuperuser diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 317e114..e73a52b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,36 +3,43 @@ # A test_factory install can be arrived at two ways, each of which can break # differently, so each gets its own job below: # -# test -- FRESH install: CREATE EXTENSION at the current -# version, on every supported PostgreSQL major, as -# the superuser CI itself runs as. The baseline a -# brand-new superuser install gets. -# test-nonsuperuser -- FRESH install, same as `test`, but as a disposable -# non-superuser installer role (bin/test_nonsuperuser) -# on every supported major. `test`'s superuser -# bypasses every role-membership check the fix for -# https://github.com/Postgres-Extensions/test_factory/issues/14 -# added, so only this job actually exercises it. -# 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. +# test -- FRESH install: CREATE EXTENSION at the current +# version, on every supported PostgreSQL major. The +# 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. 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 `test` and `test-nonsuperuser` -# 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 for both jobs. `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. +# 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 @@ -191,34 +198,6 @@ jobs: - name: Test on PostgreSQL ${{ matrix.pg }} run: make verify-results - # Proves test_factory_pgtap/test_factory are actually installable by a - # non-superuser (https://github.com/Postgres-Extensions/test_factory/issues/14), - # by running the whole suite as one -- not by asserting a property - # (pg_has_role(current_user, ...)) that's trivially true whenever the - # suite happens to run as the `test` job's own superuser regardless of - # whether the fix does anything (see PR #18's review comments). Matrixed - # across every supported major, not just one on either side of PG16: the - # fix itself branches there (pg_has_role's SET privtype is PG16+ only; - # pre-16 needs plain membership instead), so testing only one side would - # leave the other branch with the exact same always-green coverage gap - # this job exists to close. - test-nonsuperuser: - needs: [changes] - if: needs.changes.outputs.docs_only != 'true' - strategy: - matrix: - pg: ${{ fromJSON(needs.changes.outputs.supported_pg) }} - name: 🔒 PostgreSQL ${{ matrix.pg }} (non-superuser installer) - runs-on: ubuntu-latest - container: pgxn/pgxn-tools - steps: - - name: Start PostgreSQL ${{ matrix.pg }} - run: pg-start ${{ matrix.pg }} - - name: Check out the repo - uses: actions/checkout@v7 - - name: Run the suite as a disposable non-superuser installer role - run: bin/test_nonsuperuser run test_factory_nonsuperuser tf_installer - # Proves test_factory survives a BINARY pg_upgrade (in-place catalog # migration to a newer PostgreSQL major), not just a fresh CREATE EXTENSION. # Each leg: install the CURRENT version on an old cluster, binary-pg_upgrade @@ -318,7 +297,7 @@ jobs: # jobs gated off by the `changes` job on a docs-only push), and fails if any # failed or were cancelled. all-checks-passed: - needs: [changes, lint, test, test-nonsuperuser, pg-upgrade-test] + needs: [changes, lint, test, pg-upgrade-test] if: always() runs-on: ubuntu-latest steps: diff --git a/bin/test_nonsuperuser b/bin/test_nonsuperuser deleted file mode 100755 index 6fb2ede..0000000 --- a/bin/test_nonsuperuser +++ /dev/null @@ -1,51 +0,0 @@ -#!/usr/bin/env bash -# -# Exercise the test suite through a genuine, disposable NON-SUPERUSER -# installer role, to actually prove -# https://github.com/Postgres-Extensions/test_factory/issues/14's fix works -# -- rather than asserting a property (pg_has_role(current_user, ...)) that -# is trivially true whenever the suite happens to run as CI's own -# superuser, regardless of whether the fix does anything at all. Modeled on -# bin/test_existing. -# -# USAGE: bin/test_nonsuperuser run DB ROLE -# -# run DB ROLE -# Create DB and ROLE (LOGIN, NOSUPERUSER, CREATEROLE -- the exact -# privilege floor CREATE EXTENSION test_factory_pgtap needs: CREATEROLE -# for CREATE ROLE test_factory__owner, CREATE ON DATABASE for the -# schemas the install script creates), grant ROLE that CREATE -# privilege, then run the suite's fresh-install mode AS that role via -# PGUSER. --use-existing keeps pg_regress from dropping/recreating DB, -# which would throw away the GRANT just made. -set -euo pipefail - -cd "$(dirname "$(readlink -f "$0")")/.." - -run() { - local db=$1 role=$2 - createdb "$db" - createuser "$role" --no-superuser --no-createdb --createrole - psql -d "$db" -v ON_ERROR_STOP=1 -c "GRANT CREATE ON DATABASE \"$db\" TO \"$role\";" - PGUSER="$role" make verify-results \ - TEST_LOAD_SOURCE=fresh \ - CONTRIB_TESTDB="$db" \ - EXTRA_REGRESS_OPTS=--use-existing \ - PGXNTOOL_ENABLE_TEST_BUILD=no -} - -usage() { - echo "usage: bin/test_nonsuperuser run DB ROLE" >&2 - exit 2 -} - -main() { - local cmd=${1:-} - shift || true - case "$cmd" in - run) run "$@" ;; - *) usage ;; - esac -} - -main "$@" diff --git a/test/CLAUDE.md b/test/CLAUDE.md index c37a539..55410e1 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -70,23 +70,27 @@ 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 (`bin/test_nonsuperuser`) +### Non-superuser installer (`test_factory_installer`) -Every mode above runs as whatever role CI or the developer happens to be -connected as -- normally a superuser, which bypasses every role-membership -check involved in +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` proves -nothing about whether the fix works; it's true either way. - -`bin/test_nonsuperuser run ` creates a disposable role (`LOGIN`, -`NOSUPERUSER`, `CREATEROLE`) and database, grants the role `CREATE` on that -database, then runs the suite's **fresh** mode with `PGUSER` set to that -role via `EXTRA_REGRESS_OPTS=--use-existing` (so `pg_regress` doesn't -drop/recreate the database out from under the grant just made). This is -the CI `test-nonsuperuser` job's own entry point -- see `.github/workflows/ -ci.yml`'s top-of-file comment. +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 diff --git a/test/expected/base.out b/test/expected/base.out index fbb4956..7af44d8 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,23 +1,22 @@ \set ECHO none -ok 1 - Installing role has SET-enabled membership in test_factory__owner (issue #14) -ok 2 - Register test customers -ok 3 - Create function customer__add -ok 4 - Register test invoices -ok 5 - Ensure original_role temp table was dropped -ok 6 - Security definer function _tf.get has search_path=pg_catalog -ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 8 - Security definer function _tf.table_create has search_path=pg_catalog -ok 9 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 10 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 11 - customer table is empty -ok 12 - invoice table is empty -ok 13 - invoice factory output -ok 14 - invoice table content -ok 15 - customer table content -ok 16 - invoice factory second call -ok 17 - invoice table content stayed constant -ok 18 - customer table content stayed constant -ok 19 - Test function factory -ok 20 - customer table has new row -ok 21 - truncate invoice -ok 22 - invoice factory get remains the same after truncate +ok 1 - Register test customers +ok 2 - Create function customer__add +ok 3 - Register test invoices +ok 4 - Ensure original_role temp table was dropped +ok 5 - Security definer function _tf.get has search_path=pg_catalog +ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 7 - Security definer function _tf.table_create has search_path=pg_catalog +ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 10 - customer table is empty +ok 11 - invoice table is empty +ok 12 - invoice factory output +ok 13 - invoice table content +ok 14 - customer table content +ok 15 - invoice factory second call +ok 16 - invoice table content stayed constant +ok 17 - customer table content stayed constant +ok 18 - Test function factory +ok 19 - customer table has new row +ok 20 - truncate invoice +ok 21 - invoice factory get remains the same after truncate diff --git a/test/install/load.sql b/test/install/load.sql index f1a195d..a3dc36c 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -93,6 +93,35 @@ GRANT :test_role TO CURRENT_USER; \endif \endif +/* + * test_factory_installer (see test/roles.sql): the fresh/update branch + * below runs CREATE EXTENSION as this role, not as whatever ambient role + * connected (CI's superuser, a developer's own local superuser) -- if a + * non-superuser with the exact privilege floor CREATE EXTENSION needs + * (CREATEROLE, CREATE on this database) can install it, a superuser + * certainly can too, since superuser bypasses every check that role is + * deliberately still subject to. This is what actually exercises + * https://github.com/Postgres-Extensions/test_factory/issues/14's fix -- + * running the *same* suite twice, once as a superuser and once as a + * separate non-superuser role, would just waste CI time proving the + * (already-guaranteed) superuser case a second time. + * + * No LOGIN attribute: unlike test_role, nothing ever connects AS this role + * directly -- it's only ever reached via SET SESSION AUTHORIZATION within + * this same already-authenticated session, further below. That sidesteps + * needing a password or relying on whatever auth method (peer, trust, + * scram) the connecting user happens to have configured for it. + */ +SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'installer_role') AS need_installer +\gset +\if :need_installer +CREATE ROLE :installer_role CREATEROLE; +\endif + +SELECT format('GRANT CREATE ON DATABASE %I TO %I', current_database(), :'installer_role') AS installer_db_grant +\gset +:installer_db_grant; + /* * psql's \if only accepts a plain boolean token, not a comparison * expression -- compute it via SQL first (\if :load_mode = 'existing' would @@ -215,6 +244,17 @@ SELECT :'load_mode' = 'existing' AS is_existing DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; + /* + * Drop-first reset above still runs as the ambient connecting role, not + * test_factory_installer: on a persistent local dev DB, pre-existing + * objects from before this role existed (or owned by a different role) + * need whatever privilege the ambient role already has (typically a + * developer's own superuser) to drop, not the installer's deliberately + * narrow privilege floor. Everything from here on -- the actual install + * this mode exists to test -- runs as the installer instead. + */ + SET SESSION AUTHORIZATION :installer_role; + /* * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap * installs, not after. test_factory_pgtap.control declares pgtap as a @@ -298,6 +338,44 @@ SELECT :'load_mode' = 'existing' AS is_existing DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; \endif + /* + * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, + * checked from THIS session (still test_factory_installer, per the SET + * SESSION AUTHORIZATION above) rather than from test/sql/base.sql's own + * separate connection, which would only ever see whatever role CI or a + * developer happens to connect as -- pg_has_role(current_user, ...) is + * unconditionally true for a superuser regardless of whether the fix's + * GRANT logic in sql/test_factory.sql ever ran. test_factory_installer is + * freshly created every run with zero prior relationships, so it can only + * have this membership if that GRANT actually fired -- there's no other + * way it could already be there. + * + * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as + * sql/test_factory.sql's own comment); pre-16, plain membership is the + * closest equivalent ('SET' isn't a meaningful distinction yet -- + * membership itself already confers the ability to SET ROLE). + */ + SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus + \gset + -- pg16+ SET-enabled membership check + \if :pg16plus + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \else + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \endif + -- existing-vs-fresh/update \endif diff --git a/test/roles.sql b/test/roles.sql index a48f56f..4ab7010 100644 --- a/test/roles.sql +++ b/test/roles.sql @@ -7,4 +7,7 @@ */ \set test_role test_role +-- test/install/load.sql only -- see its own comment. +\set installer_role test_factory_installer + -- vi: expandtab ts=2 sw=2 diff --git a/test/sql/base.sql b/test/sql/base.sql index d3ba6dd..35091cc 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -1,52 +1,8 @@ \set ECHO none \i test/helpers/setup.sql --- test/install/load.sql already installed the extension, in every mode. - -/* - * Regression test for issue #14. CREATE ROLE never grants the creating role - * any relationship to the role it just created, on any version, so the - * install must GRANT test_factory__owner explicitly (WITH SET TRUE on - * PostgreSQL 16+, plain on older versions -- see sql/test_factory.sql's own - * comment) or the SET ROLE performed during install fails for - * non-superuser installs (RDS/Aurora). - * - * pg_has_role(), not a raw pg_auth_members query: a real superuser always - * has effective SET/membership on every role (bypasses the check entirely, - * no explicit grant needed), which pg_has_role correctly reports as true -- - * a literal catalog-row check would not, since CI's installer here IS a - * real superuser and the fix's own gating logic (also pg_has_role-based) - * correctly skips granting a superuser something they don't need. This is - * what the fix actually guarantees -- "can this role SET ROLE - * test_factory__owner", not "does a specific catalog row exist" -- and - * checking it the same way the fix does is what makes this a real - * regression test rather than an assertion about an implementation detail. - * - * Both branches only actually catch a regression when the suite runs as a - * genuine non-superuser (bin/test_nonsuperuser, the CI test-nonsuperuser - * job) -- pg_has_role is unconditionally true for the superuser this file - * otherwise runs as, same as it is for the fix's own gating logic. - * - * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as - * sql/test_factory.sql); pre-16, plain membership is the closest equivalent - * ('SET' isn't a meaningful distinction yet -- membership itself already - * confers the ability to SET ROLE). - */ -SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus \gset --- pg16+ SET-enabled membership check -\if :pg16plus -SELECT ok( - pg_has_role(current_user, 'test_factory__owner', 'SET') - , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' -); --- pg16+ SET-enabled membership check -\else -SELECT ok( - pg_has_role(current_user, 'test_factory__owner', 'MEMBER') - , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' -); --- pg16+ SET-enabled membership check -\endif +-- test/install/load.sql already installed the extension, in every mode +-- (including the regression test for issue #14 -- see its own comment). -- NOTE: This runs some tests itself \i test/helpers/create.sql From b2339fc58158686ac8e37e789004f973e6008609 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 15:51:51 -0500 Subject: [PATCH 14/17] Address review: error message style, \gexec, comment sizing; correct README claim sql/test_factory.sql: split the RAISE EXCEPTION messages into a primary message (lowercase, factual, no embedded suggestion) plus a HINT carrying the suggested GRANT command, per Postgres's own error style guide instead of cramming both into one message. test/install/load.sql: use \gexec instead of \gset-then-substitute for the dynamic GRANT CREATE ON DATABASE statement -- simpler, and removes the class of bug where a substituted bare variable is missing its trailing semicolon (confirmed CREATE ROLE has no IF NOT EXISTS variant in Postgres, so the existence-check dance for test_factory_installer is necessary, not an oversight -- same reason test_role above it needs the same pattern). Also moved the "why run as superuser" rationale for the drop-first reset into a small note directly above the DROP statements themselves, and shrunk the SET SESSION AUTHORIZATION comment to one line now that it no longer needs to carry that explanation too. README: the "Known limitation" paragraph claiming binary pg_upgrade could leave a database without the SET-enabled grant doesn't hold up -- verified directly that pg_dumpall (which pg_upgrade uses internally to carry role/membership state to the new cluster) emits a bare `GRANT role TO member;` for a pre-16 plain membership, and a bare GRANT like that defaults to SET TRUE on PostgreSQL 16+. Replaced the speculative limitation with a short manual-fix note for the unrelated case of a grant being revoked or a role being set up some other way. Detailed investigation belongs on the issue tracker, not the README. Co-Authored-By: Claude Sonnet 5 --- README.md | 10 ++-------- sql/test_factory--0.5.0.sql | 14 ++++++++++---- sql/test_factory.sql | 14 ++++++++++---- test/build/expected/syntax.out | 4 ++-- test/install/load.sql | 21 ++++++++------------- 5 files changed, 32 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 18e571f..60bb071 100644 --- a/README.md +++ b/README.md @@ -103,14 +103,8 @@ 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. -Binary `pg_upgrade` doesn't re-run install scripts, so upgrading a pre-16 -install across the PostgreSQL 16 boundary can leave a database without the -SET-enabled grant a fresh install would have set up. Since no install -script runs during the upgrade, this case isn't caught up front -- it -surfaces later, the next time an install/reinstall action runs `SET ROLE` -(e.g. re-running `CREATE EXTENSION`) -- not from ordinary use of -`tf.register()` or `tf.get()`, neither of which ever run `SET ROLE`. Fix it -once as a superuser: +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 WITH SET TRUE; diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index db11d5f..23c9360 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -69,9 +69,12 @@ BEGIN * ordinary %. */ RAISE EXCEPTION - 'Role "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + USING 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 @@ -80,9 +83,12 @@ BEGIN 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. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - , format('GRANT test_factory__owner TO %I;', current_user); + USING 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 diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 7746dff..0b3975e 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -68,9 +68,12 @@ BEGIN * ordinary %. */ RAISE EXCEPTION - 'Role "%" does not have SET-enabled membership in "test_factory__owner" and lacks ADMIN OPTION to grant it to itself. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user); + USING 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 @@ -79,9 +82,12 @@ BEGIN 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. Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %' + 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - , format('GRANT test_factory__owner TO %I;', current_user); + USING 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 diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index b707c8b..202997e 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:122: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:123: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:128: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +psql:sql/test_factory.sql:129: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION diff --git a/test/install/load.sql b/test/install/load.sql index a3dc36c..5c084f2 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -118,9 +118,8 @@ SELECT NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = :'installer_role') AS CREATE ROLE :installer_role CREATEROLE; \endif -SELECT format('GRANT CREATE ON DATABASE %I TO %I', current_database(), :'installer_role') AS installer_db_grant -\gset -:installer_db_grant; +SELECT format('GRANT CREATE ON DATABASE %I TO %I', current_database(), :'installer_role') +\gexec /* * psql's \if only accepts a plain boolean token, not a comparison @@ -239,20 +238,16 @@ SELECT :'load_mode' = 'existing' AS is_existing * bootstrapping (CREATE ROLE test_factory__owner, guarded by WHEN * duplicate_object in sql/test_factory.sql) already tolerates being * re-run, so unlike pgxntool's own drop-first example there's no - * separate role-drop step needed here. + * separate role-drop step needed here. Runs as the ambient role, not + * test_factory_installer: that's plausibly how the extension got here + * in the first place (a superuser), which the installer's narrower + * privileges might not be able to drop. */ DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; - /* - * Drop-first reset above still runs as the ambient connecting role, not - * test_factory_installer: on a persistent local dev DB, pre-existing - * objects from before this role existed (or owned by a different role) - * need whatever privilege the ambient role already has (typically a - * developer's own superuser) to drop, not the installer's deliberately - * narrow privilege floor. Everything from here on -- the actual install - * this mode exists to test -- runs as the installer instead. - */ + -- Everything from here on -- the actual install this mode exists to + -- test -- runs as the installer instead. SET SESSION AUTHORIZATION :installer_role; /* From 4a48e6582e0a91800b80da3f5d0d993684e91e33 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 16:26:36 -0500 Subject: [PATCH 15/17] Use a real ERRCODE; fix a comment that dodged the linter's letter, not its spirit sql/test_factory.sql: assign ERRCODE = 'insufficient_privilege' (SQLSTATE 42501) to both RAISE EXCEPTION calls -- an existing Postgres code that fits exactly (this IS a re-raise of the same insufficient_privilege condition just caught, with a clearer message), instead of leaving them at the generic default. Per ../ai/CLAUDE.md's newly-added RAISE style-guide section. test/sql/base.sql: the 2-line "--" comment I'd left here passed the linter's own comment-stacked-dashes check (which only flags 3+ consecutive lines), but that check is a narrow mechanical heuristic, not the actual style rule -- this comment is doing real explanatory work (a fact plus a pointer to detail elsewhere), which calls for a block comment regardless of its raw line count. Converted to /* */. Also pulled 3 newly-merged PRs into ../ai/ (was tracking a stale fork remote instead of upstream) -- confirmed sql/test_factory.sql's existing role-restore logic (plain SET ROLE at the end, not relying on SET LOCAL's transaction-boundary revert) already matches its newly-documented "session state in create/update scripts must be reverted explicitly" convention, so no change needed there. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0.sql | 6 ++++-- sql/test_factory.sql | 6 ++++-- test/build/expected/syntax.out | 4 ++-- test/sql/base.sql | 6 ++++-- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 23c9360..38db882 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -71,7 +71,8 @@ BEGIN RAISE EXCEPTION 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + 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) ); @@ -85,7 +86,8 @@ BEGIN RAISE EXCEPTION 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + 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) ); diff --git a/sql/test_factory.sql b/sql/test_factory.sql index 0b3975e..47c9cee 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -70,7 +70,8 @@ BEGIN RAISE EXCEPTION 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + 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) ); @@ -84,7 +85,8 @@ BEGIN RAISE EXCEPTION 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + 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) ); diff --git a/test/build/expected/syntax.out b/test/build/expected/syntax.out index 202997e..f014617 100644 --- a/test/build/expected/syntax.out +++ b/test/build/expected/syntax.out @@ -1,3 +1,3 @@ \set ECHO none -psql:sql/test_factory.sql:128: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION -psql:sql/test_factory.sql:129: ERROR: pg_extension_config_dump() can only be called from an SQL script executed by CREATE EXTENSION +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 diff --git a/test/sql/base.sql b/test/sql/base.sql index 35091cc..84db8d5 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -1,8 +1,10 @@ \set ECHO none \i test/helpers/setup.sql --- test/install/load.sql already installed the extension, in every mode --- (including the regression test for issue #14 -- see its own comment). +/* + * test/install/load.sql already installed the extension, in every mode + * (including the regression test for issue #14 -- see its own comment). + */ -- NOTE: This runs some tests itself \i test/helpers/create.sql From 10bc55a1a36259e7c271d9a7c15676ebd2972e21 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 12 Aug 2026 16:43:47 -0500 Subject: [PATCH 16/17] Fold in the stable-pseudo-version fixes now that #42 merged to master Merged upstream/master (which now has #42's stable-pseudo-version work) into this branch. The merge itself resolved cleanly for the control files (default_version='stable' from master combined with superuser=false from this branch, on non-overlapping lines), but silently kept this branch's own corrupted sql/test_factory--0.5.0.sql instead of master's correct one -- git's merge doesn't know that file is supposed to be immutable, so a clean textual merge isn't the same as a correct one. Restored it to match master exactly. Replaced #42's no-op sql/test_factory--0.5.0--stable.sql placeholder with the real GRANT-if-missing content -- this branch is "the first SQL-touching PR since the last release" per ../ai/RELEASE.md, so it's responsible for populating this file for real, which is also the signal the new multi-extension release procedure uses to decide test_factory needs a version bump next release (test_factory_pgtap doesn't, since its own placeholder stays a genuine no-op). Replaced test/install/load.sql with the version that splits update-mode role handling from fresh-mode: installing the genuinely-unfixed real 0.5.0 (test_factory VERSION '0.5.0') cannot succeed as a non-superuser -- that's the original bug, not a versioning artifact -- so update mode stays on the ambient/superuser role throughout, while only fresh mode switches to the disposable non-superuser installer (which already fully covers the non-superuser install path). Moved the issue #14 regression check into the fresh-mode branch specifically, since it was checking the wrong thing under update mode's ambient role. Verified locally on PG12 and PG17: fresh and update modes both pass, sql/test_factory--0.5.0.sql and sql/test_factory_pgtap--0.1.0.sql remain byte-identical to what shipped, and `make` no longer touches either. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0--stable.sql | 57 ++++++++-- sql/test_factory--0.5.0.sql | 81 -------------- test/install/load.sql | 162 ++++++++++++++++------------ 3 files changed, 137 insertions(+), 163 deletions(-) diff --git a/sql/test_factory--0.5.0--stable.sql b/sql/test_factory--0.5.0--stable.sql index e7e34b8..7ba91f7 100644 --- a/sql/test_factory--0.5.0--stable.sql +++ b/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----.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 diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 38db882..37e742b 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -6,7 +6,6 @@ * 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; @@ -16,86 +15,6 @@ 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 - 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$; - 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; diff --git a/test/install/load.sql b/test/install/load.sql index 5c084f2..9f6ad7f 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -246,39 +246,30 @@ SELECT :'load_mode' = 'existing' AS is_existing DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; - -- Everything from here on -- the actual install this mode exists to - -- test -- runs as the installer instead. - SET SESSION AUTHORIZATION :installer_role; - - /* - * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap - * installs, not after. test_factory_pgtap.control declares pgtap as a - * requirement too, so the CREATE EXTENSION ... CASCADE (or plain CREATE - * EXTENSION, in update mode) below would otherwise cascade-install pgtap - * itself into whatever schema this session's ambient search_path - * resolves to (public, by default) -- and since that satisfies "pgtap is - * already installed", the main suite's own tap_setup.sql (which does - * CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap) then skips creating it - * in "tap" at all, leaving every pgTAP-based test file failing with - * "function no_plan() does not exist" (hit this for real writing this - * file). IF NOT EXISTS on both statements: harmless no-op on a rerun - * where a previous pass already did this and the drop-first reset above - * didn't touch it (cascading test_factory_pgtap's drop doesn't remove - * pgtap -- pgtap is its dependency, not the other way around). - */ - CREATE SCHEMA IF NOT EXISTS tap; - CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; - - -- Captured before either branch below runs CREATE EXTENSION, so the - -- role-restore proof after \endif covers whichever one actually ran. - SELECT current_user AS role_before_install - \gset - SELECT :'load_mode' = 'update' AS is_update \gset -- update-vs-fresh install \if :is_update + /* + * update mode runs entirely as the ambient role, not + * test_factory_installer: extension ownership can't be transferred + * (`ALTER EXTENSION ... OWNER TO` isn't valid syntax -- confirmed), + * so only the role that ran the ORIGINAL CREATE EXTENSION can ever + * run ALTER EXTENSION UPDATE on it. There's no reachable "a + * different non-superuser applies this later" scenario to test here + * -- whether the original install itself could have been done + * non-superuser is exactly what the fresh branch below already + * proves. + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + CREATE EXTENSION test_factory VERSION :'update_from'; SET client_min_messages = ERROR; -- suppress update-script deprecation NOTICEs \if :has_update_to @@ -297,12 +288,36 @@ SELECT :'load_mode' = 'existing' AS is_existing -- update-vs-fresh install \else + -- Everything from here on -- the actual install this mode exists to + -- test -- runs as the installer instead. + SET SESSION AUTHORIZATION :installer_role; + + /* + * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap + * installs, not after. test_factory_pgtap.control declares pgtap as a + * requirement too, so the CREATE EXTENSION ... CASCADE below would + * otherwise cascade-install pgtap itself into whatever schema this + * session's ambient search_path resolves to (public, by default) -- + * and since that satisfies "pgtap is already installed", the main + * suite's own tap_setup.sql (which does CREATE EXTENSION IF NOT + * EXISTS pgtap SCHEMA tap) then skips creating it in "tap" at all, + * leaving every pgTAP-based test file failing with "function + * no_plan() does not exist" (hit this for real writing this file). + * IF NOT EXISTS on both statements: harmless no-op on a rerun where a + * previous pass already did this and the drop-first reset above + * didn't touch it (cascading test_factory_pgtap's drop doesn't + * remove pgtap -- pgtap is its dependency, not the other way + * around). + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + /* - * fresh: install for real, right here, uniformly with update/existing -- - * so test/sql/base.sql and test/sql/pgtap.sql can assume both - * extensions are already present in every mode, instead of each mode - * needing its own install-or-skip dance (what test/helpers/ - * create_extension.sql used to do; deleted along with this change). * CASCADE proves test_factory_pgtap.control's "requires = 'pgtap, * test_factory'" line actually pulls test_factory in -- * test/sql/pgtap.sql separately proves the dependency is *enforced* @@ -310,6 +325,49 @@ SELECT :'load_mode' = 'existing' AS is_existing */ CREATE EXTENSION test_factory_pgtap CASCADE; + /* + * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, + * checked from THIS session (still test_factory_installer, per the + * SET SESSION AUTHORIZATION above) rather than from + * test/sql/base.sql's own separate connection, which would only ever + * see whatever role CI or a developer happens to connect as -- + * pg_has_role(current_user, ...) is unconditionally true for a + * superuser regardless of whether the fix's GRANT logic in + * sql/test_factory.sql ever ran. test_factory_installer is freshly + * created every run with zero prior relationships, so it can only + * have this membership if that GRANT actually fired -- there's no + * other way it could already be there. Only checked here, not in + * update mode above: update mode never switches to + * test_factory_installer (see its own comment), so current_user + * there is whatever ambient role connected -- typically a superuser, + * for whom this would be checking nothing. + * + * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as + * sql/test_factory.sql's own comment); pre-16, plain membership is + * the closest equivalent ('SET' isn't a meaningful distinction yet -- + * membership itself already confers the ability to SET ROLE). + */ + SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus + \gset + -- pg16+ SET-enabled membership check + \if :pg16plus + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \else + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \endif + -- update-vs-fresh install \endif @@ -333,44 +391,6 @@ SELECT :'load_mode' = 'existing' AS is_existing DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; \endif - /* - * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, - * checked from THIS session (still test_factory_installer, per the SET - * SESSION AUTHORIZATION above) rather than from test/sql/base.sql's own - * separate connection, which would only ever see whatever role CI or a - * developer happens to connect as -- pg_has_role(current_user, ...) is - * unconditionally true for a superuser regardless of whether the fix's - * GRANT logic in sql/test_factory.sql ever ran. test_factory_installer is - * freshly created every run with zero prior relationships, so it can only - * have this membership if that GRANT actually fired -- there's no other - * way it could already be there. - * - * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as - * sql/test_factory.sql's own comment); pre-16, plain membership is the - * closest equivalent ('SET' isn't a meaningful distinction yet -- - * membership itself already confers the ability to SET ROLE). - */ - SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus - \gset - -- pg16+ SET-enabled membership check - \if :pg16plus - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \else - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \endif - -- existing-vs-fresh/update \endif From da2c48eb1c95d684a9de4928e838ccb5796595fb Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 12 Aug 2026 18:13:24 -0500 Subject: [PATCH 17/17] sql/test_factory.sql: clarify current_user is unquoted in the RAISE messages Per review: confirmed directly (SELECT current_user, and against a role deliberately named with an embedded literal double-quote) that current_user always returns the raw, unquoted role name -- so the "" in the message text on the line above is the only source of quoting, not something current_user already provides. Added a trailing comment on both occurrences (the mirrored pre-16 branch has the identical pattern) so this doesn't need re-deriving on the next read. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sql/test_factory.sql b/sql/test_factory.sql index cbc4470..49b53f9 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -69,7 +69,7 @@ BEGIN */ RAISE EXCEPTION 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user + , 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' @@ -84,7 +84,7 @@ BEGIN 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 + , 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'