Skip to content

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

Merged
jnasbyupgrade merged 19 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/issue-14-clean
Aug 12, 2026
Merged

Fix CREATE EXTENSION on PG16+ for non-superuser installs (#14)#18
jnasbyupgrade merged 19 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/issue-14-clean

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

CREATE EXTENSION test_factory fails on PostgreSQL 16+ when installed by a non-superuser role:

ERROR:  must be able to SET ROLE "test_factory__owner"

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 is rejected. A superuser bypasses the check, so this is a core PG16 behavior change, not anything platform-specific -- it just happens to be much easier to hit on RDS/Aurora, since those don't grant a true superuser role, but it affects any non-superuser install on PG16+ regardless of platform.

Fix: after creating the role, grant it back to the installing role WITH SET, gated on PG16+ (pre-16 GRANT ... TO already permits SET ROLE). The grant is unconditional so it also covers the case where the role already existed.

The failure can't be reproduced under pg_regress, which runs as a superuser, so the added test instead asserts the SET-enabled membership the fix establishes.

Fixes #14

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6b9137d0-42fd-463b-b63d-e9cdd2d83bc4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

jnasbyupgrade added a commit to jnasbyupgrade/test_factory that referenced this pull request Jul 15, 2026
@jnasbyupgrade
Squashed 'pgxntool/' changes from bed3604..eb84bc6
f73920c 
eb84bc6 Stamp 2.1.0
9fbe6f4 Fix results ordering, control file whitespace, ENABLE_* override, debug levels (Postgres-Extensions#31)
88bb4f2 Add Claude Code GitHub Actions workflows (Postgres-Extensions#41)
af5bbbb ci: pass repo owners to run-tests.yml for fork-account matching (Postgres-Extensions#40)
c7928af Fix repo-root guard to work inside a git worktree (Postgres-Extensions#39)
b062fca ci: point reusable test workflow at @master
b6cdbfd Add CI workflows and multi-session PR guard (Postgres-Extensions#33)
1ba0987 Stamp 2.0.3
1931cbe Fix pgxntool-sync remote and make it runnable without make (Postgres-Extensions#37)
8176304 Stamp 2.0.2
3e142ab Fix parse_control_file: remove comments before stripping quotes (Postgres-Extensions#27)
cacc301 Stamp 2.0.1
bf1db6b Fix bash 3.2 / Linux compatibility issues (Postgres-Extensions#26)
62d0fcb Fix broken ifeq for --load-language=plpgsql on PG < 13 (Postgres-Extensions#24)
121f0b3 Stamp 2.0.0
ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18)
c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23)
abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22)
08c1879 Stamp 1.1.2
6e0dad2 Fix double --dbname bug that defeated unique test database names
639756c Stamp 1.1.1
6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15)
3b8cb2a Stamp 1.1.0
550a901 Remove commit.md (maintained in pgxntool-test)
d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13)
9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (Postgres-Extensions#12)
ab7f6e2 Stamp 1.0.0
3a571ba Add pg_tle support and modernize test infrastructure (Postgres-Extensions#11)
b96ea6d Add support for Claude code; build and doc improvements (Postgres-Extensions#9)
e9c24de Fix pg_regress on versions > 12 (Postgres-Extensions#5)
REVERT: bed3604 Fix pg_regress on versions > 12 (Postgres-Extensions#5) (Postgres-Extensions#6)

git-subtree-dir: pgxntool
git-subtree-split: eb84bc6e87e21f2ced11ff1b8ddb4028b7c67c8f
jnasbyupgrade added a commit to jnasbyupgrade/test_factory that referenced this pull request Jul 16, 2026
@jnasbyupgrade
Squashed 'pgxntool/' changes from bed3604..eb84bc6
21b2ee5 
eb84bc6 Stamp 2.1.0
9fbe6f4 Fix results ordering, control file whitespace, ENABLE_* override, debug levels (Postgres-Extensions#31)
88bb4f2 Add Claude Code GitHub Actions workflows (Postgres-Extensions#41)
af5bbbb ci: pass repo owners to run-tests.yml for fork-account matching (Postgres-Extensions#40)
c7928af Fix repo-root guard to work inside a git worktree (Postgres-Extensions#39)
b062fca ci: point reusable test workflow at @master
b6cdbfd Add CI workflows and multi-session PR guard (Postgres-Extensions#33)
1ba0987 Stamp 2.0.3
1931cbe Fix pgxntool-sync remote and make it runnable without make (Postgres-Extensions#37)
8176304 Stamp 2.0.2
3e142ab Fix parse_control_file: remove comments before stripping quotes (Postgres-Extensions#27)
cacc301 Stamp 2.0.1
bf1db6b Fix bash 3.2 / Linux compatibility issues (Postgres-Extensions#26)
62d0fcb Fix broken ifeq for --load-language=plpgsql on PG < 13 (Postgres-Extensions#24)
121f0b3 Stamp 2.0.0
ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18)
c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23)
abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22)
08c1879 Stamp 1.1.2
6e0dad2 Fix double --dbname bug that defeated unique test database names
639756c Stamp 1.1.1
6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15)
3b8cb2a Stamp 1.1.0
550a901 Remove commit.md (maintained in pgxntool-test)
d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13)
9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (Postgres-Extensions#12)
ab7f6e2 Stamp 1.0.0
3a571ba Add pg_tle support and modernize test infrastructure (Postgres-Extensions#11)
b96ea6d Add support for Claude code; build and doc improvements (Postgres-Extensions#9)
e9c24de Fix pg_regress on versions > 12 (Postgres-Extensions#5)
REVERT: bed3604 Fix pg_regress on versions > 12 (Postgres-Extensions#5) (Postgres-Extensions#6)

git-subtree-dir: pgxntool
git-subtree-split: eb84bc6e87e21f2ced11ff1b8ddb4028b7c67c8f
@jnasbyupgrade jnasbyupgrade changed the title Fix CREATE EXTENSION on PG16+ without a superuser (RDS/Aurora) (#14) Fix CREATE EXTENSION on PG16+ for non-superuser installs (#14) Jul 17, 2026
@jnasbyupgrade jnasbyupgrade mentioned this pull request Jul 18, 2026
Closed
3 tasks
jnasbyupgrade added a commit to jnasbyupgrade/test_factory that referenced this pull request Jul 29, 2026
@jnasbyupgrade
Squashed 'pgxntool/' changes from bed3604..b67064b
3f798c5 
b67064b Release 2.2.0 (#77)
d949d6d Fix stale README/CLAUDE.md docs; rename internal-only test shim (#76)
6d7822c ci: grant actions:write so bun cache save succeeds (#75)
2738f59 README.asc: fix stale docs, document undocumented API surface (#74)
d1d3bee Add make pgxntool-version to print the embedded pgxntool version (#73)
493a0f8 Fix parallel-build SQL corruption, remote hardcoding, and add safety checks (#61)
3b0e3de Fix make pgtle PGTLE_VERSION=X being silently ignored (#66)
21b35ef ci: skip paired-test-PR requirement for doc-only PRs (#71)
55efd03 Rename EXTENSION_VERSION_FILES to EXTENSION__CURRENT_VERSION__FILES (#69)
9d0a856 Fix pgtle.sh rejecting non-numeric version aliases like 'stable' (#57) (#63)
9c2f75f Docs: versioned SQL file tracking tradeoffs and update-testing pattern (#60)
c38cf2a ci: never check out fork PR head in claude-code-review pull_request_target job (#70)
aab8f2b pgtle_versions.md: fix wrong uninstall_extension() version claim (#59)
e2c9174 README.asc: make tag creates a git tag, not a branch (#58)
eb84bc6 Stamp 2.1.0
9fbe6f4 Fix results ordering, control file whitespace, ENABLE_* override, debug levels (Postgres-Extensions#31)
88bb4f2 Add Claude Code GitHub Actions workflows (Postgres-Extensions#41)
af5bbbb ci: pass repo owners to run-tests.yml for fork-account matching (Postgres-Extensions#40)
c7928af Fix repo-root guard to work inside a git worktree (Postgres-Extensions#39)
b062fca ci: point reusable test workflow at @master
b6cdbfd Add CI workflows and multi-session PR guard (Postgres-Extensions#33)
1ba0987 Stamp 2.0.3
1931cbe Fix pgxntool-sync remote and make it runnable without make (Postgres-Extensions#37)
8176304 Stamp 2.0.2
3e142ab Fix parse_control_file: remove comments before stripping quotes (Postgres-Extensions#27)
cacc301 Stamp 2.0.1
bf1db6b Fix bash 3.2 / Linux compatibility issues (Postgres-Extensions#26)
62d0fcb Fix broken ifeq for --load-language=plpgsql on PG < 13 (Postgres-Extensions#24)
121f0b3 Stamp 2.0.0
ad3ca7e Remove .source support; add test/install, test/build, and verify-results (Postgres-Extensions#18)
c010cf8 Fix bash 3.2 compatibility (Postgres-Extensions#23)
abeb9d3 Remove .source file support from pg_regress integration (Postgres-Extensions#22)
08c1879 Stamp 1.1.2
6e0dad2 Fix double --dbname bug that defeated unique test database names
639756c Stamp 1.1.1
6ba3176 Fix pg_tle exception handler and empty upgrade files (Postgres-Extensions#15)
3b8cb2a Stamp 1.1.0
550a901 Remove commit.md (maintained in pgxntool-test)
d73ca93 Add unique test database names to prevent conflicts (Postgres-Extensions#13)
9b344be Add update-setup-files.sh for 3-way merging after pgxntool-sync (Postgres-Extensions#12)
ab7f6e2 Stamp 1.0.0
3a571ba Add pg_tle support and modernize test infrastructure (Postgres-Extensions#11)
b96ea6d Add support for Claude code; build and doc improvements (Postgres-Extensions#9)
e9c24de Fix pg_regress on versions > 12 (Postgres-Extensions#5)
REVERT: bed3604 Fix pg_regress on versions > 12 (Postgres-Extensions#5) (Postgres-Extensions#6)

git-subtree-dir: pgxntool
git-subtree-split: b67064b16ca2eb30761c078511dc8c974938480f
jnasbyupgrade added a commit to jnasbyupgrade/test_factory that referenced this pull request Jul 30, 2026
@jnasbyupgrade
Actually install as a non-superuser role, closing the Postgres-Extens…
8c17d27 
…ions#18 test gap

Mark both control files `superuser = false` (the pre-PG13 mechanism,
not `trusted` -- `trusted` is an unrecognized control-file key on PG10-12,
which this project's CI still tests, and errors out entirely there, not
just for non-superuser attempts). Verified empirically against this
container's PG12 and PG17 clusters.

test/sql/install.sql now creates a disposable NOSUPERUSER + CREATEROLE
role (mirroring what a real RDS/Aurora master user has) and installs
through it via SET SESSION AUTHORIZATION, replacing the indirect
pg_auth_members proxy check from Postgres-Extensions#18 with a genuine end-to-end repro:
before the Postgres-Extensions#18 fix this fails with "must be able to SET ROLE
test_factory__owner"; after the fix it succeeds.

Two extra grants were needed beyond CREATEROLE, found by actually running
this rather than reasoning about it: USAGE on the tap schema (a pgtap
test-harness necessity, unrelated to what's under test) and CREATE on the
current database (never granted to PUBLIC by default -- only CONNECT/TEMP
are -- unlike what I'd assumed).

Also fixed a real local-iteration flakiness this surfaced: test_factory__owner
is deliberately left behind by DROP EXTENSION so a real install/uninstall
cycle by the same installer keeps working, but this test creates a fresh
disposable installer role every run, so an orphaned owner role from a
previous run of this file belongs to an installer that no longer exists,
breaking the GRANT ... WITH SET. install.sql now drops both roles at
start and end. Verified stable across many repeated `make test` runs
against the same cluster.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@jnasbyupgrade jnasbyupgrade mentioned this pull request Aug 4, 2026
Open
3 tasks
jnasbyupgrade and others added 3 commits August 10, 2026 14:10
@jnasbyupgrade @claude
Grant owner role WITH SET so CREATE EXTENSION works on PG16+ non-supe…
a630263 
…ruser

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 Postgres-Extensions#14

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jnasbyupgrade @claude
test: assert installer gets SET-enabled membership in owner role (issue
1f37236 
Postgres-Extensions#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) <noreply@anthropic.com>
@jnasbyupgrade @claude
Rebase issue-14 fix onto master's test/build refactor
9582bbd 
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 <noreply@anthropic.com>
jnasbyupgrade and others added 4 commits August 10, 2026 15:32
@jnasbyupgrade @claude
ci: restructure pg-upgrade-test matrix to avoid crossing the PG16 bou…
56448d3 
…ndary

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 Postgres-Extensions#14 (GRANT test_factory__owner TO <role> 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 <noreply@anthropic.com>
@jnasbyupgrade
Fix root cause of a real NOTICE, not just suppress it; tighten CI com…
1466e5c 
…ment 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).
@jnasbyupgrade
Revert %TYPE removal (intentional); suppress both known NOTICEs in th…
98cff1c 
…e 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.
@jnasbyupgrade
Drop the %TYPE explanatory comment -- it's a common idiom, not an ove…
a3a078f 
…rsight

Doesn't need justifying inline every time it's used.
github-actions[bot]
github-actions Bot reviewed Aug 10, 2026
View reviewed changes
Comment thread sql/test_factory.sql
@jnasbyupgrade
Fix a real regression: GRANT requires ADMIN OPTION even for a no-op r…
49b0386 
…e-grant

Caught in review (PR Postgres-Extensions#18 discussion): GRANT <role> TO <user> 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.
@jnasbyupgrade
Fix regression test to check pg_has_role, not a raw pg_auth_members row
30ef622 
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.
@jnasbyupgrade jnasbyupgrade mentioned this pull request Aug 10, 2026
Merged
@jnasbyupgrade
Don't SET client_min_messages in the install script; that's test/buil…
93bbc3f 
…d'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 Postgres-Extensions#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-actions[bot]
github-actions Bot reviewed Aug 10, 2026
View reviewed changes
Comment thread test/sql/base.sql Outdated
github-actions[bot]
github-actions Bot reviewed Aug 10, 2026
View reviewed changes
Comment thread README.md Outdated
@jnasbyupgrade @claude
Fix a deeper pre-16 bug, allow genuine non-superuser installs, add re…
c2c255b 
…al 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 Postgres-Extensions#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 Postgres-Extensions#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 <noreply@anthropic.com>
github-actions[bot]
github-actions Bot reviewed Aug 11, 2026
View reviewed changes
Comment thread README.md Outdated
jnasbyupgrade and others added 2 commits August 11, 2026 13:37
@jnasbyupgrade @claude
README: fix wrong symptom for the pg_upgrade SET-enabled-grant limita…
cc0fc95 
…tion

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 Postgres-Extensions#18 review.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jnasbyupgrade @claude
Fold non-superuser install testing into the normal suite, drop the ex…
102a508 
…tra 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 Postgres-Extensions#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 Postgres-Extensions#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 <noreply@anthropic.com>
jnasbyupgrade and others added 2 commits August 11, 2026 15:51
@jnasbyupgrade @claude
Address review: error message style, \gexec, comment sizing; correct …
b2339fc 
…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 <noreply@anthropic.com>
@jnasbyupgrade @claude
Use a real ERRCODE; fix a comment that dodged the linter's letter, no…
4a48e65 
…t 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 <noreply@anthropic.com>
This was referenced Aug 11, 2026
Closed
Merged
jnasbyupgrade added a commit that referenced this pull request Aug 12, 2026
@jnasbyupgrade
Adopt the stable pseudo-version convention (#42)
6c09a22 
- Adopts pgxntool's `stable` pseudo-version convention (per
`../ai/RELEASE.md`): `default_version` in both `test_factory.control`
and `test_factory_pgtap.control` now sits at the literal string
`'stable'` between releases, instead of a real, already-released version
number.
- Without this, an ordinary source edit to `sql/test_factory.sql` would
silently regenerate -- and corrupt -- the frozen, already-shipped
`sql/test_factory--0.5.0.sql` (exactly what happened while developing
#18, before this fix existed).
jnasbyupgrade and others added 4 commits August 12, 2026 16:39
@jnasbyupgrade
Merge remote-tracking branch 'upstream/master' into fix/issue-14-clean
6d9510d
@jnasbyupgrade @claude
Fold in the stable-pseudo-version fixes now that Postgres-Extensions#42…
10bc55a 
… merged to master

Merged upstream/master (which now has Postgres-Extensions#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 Postgres-Extensions#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 Postgres-Extensions#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 <noreply@anthropic.com>
@jnasbyupgrade
Merge upstream/master (linter tightened to flag 2+ stacked dashes)
5fc603a 
Master's own Postgres-Extensions#43 further tightened the vendored linter's
comment-stacked-dashes rule (2+ consecutive "--" lines now must be a
block comment, down from 3+), which itself was needed to fix two
pre-existing 2-line comments elsewhere. That change conflicted with this
branch's own restructured test/install/load.sql, which had duplicated a
similar 2-line comment into two places (fresh mode's own "Captured
before..." note, and its "Everything from here on..." note) -- both now
also need to be block comments under the new threshold. Fixed all three
occurrences the linter flagged after resolving.

Also caught and fixed my own mistake while resolving this: initially
checked out the *previous* .vendor/linter submodule commit instead of
the new one master's Postgres-Extensions#43 actually points at (misread the diff
direction) -- confirmed against the real upstream linter repo's history
before fixing it, which is what actually surfaced these findings in the
first place.

Verified locally on PG12 and PG17: fresh and update modes both pass,
make lint is clean, and sql/test_factory--0.5.0.sql remains untouched.
@jnasbyupgrade @claude
sql/test_factory.sql: clarify current_user is unquoted in the RAISE m…
da2c48e 
…essages

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 <noreply@anthropic.com>
@jnasbyupgrade
jnasbyupgrade merged commit bc774a6 into Postgres-Extensions:master Aug 12, 2026
16 checks passed
@jnasbyupgrade
jnasbyupgrade deleted the fix/issue-14-clean branch August 12, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CREATE EXTENSION fails on PG16+ without a superuser (RDS/Aurora): must be able to SET ROLE "test_factory__owner"

1 participant

@jnasbyupgrade