Install count_nulls without superuser, and prove it in the suite - #62
Install count_nulls without superuser, and prove it in the suite#62jnasbyupgrade wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
91b89a6 to
0110c07
Compare
count_nulls is nothing but SQL functions, so requiring a superuser to CREATE EXTENSION it was never anything more than the control file's default. Set superuser = false. For that to stay true, the suite now runs as an ordinary role throughout - both the install session and every test session. test/helpers/test_user.sql creates "Test user for count_nulls" if it isn't there, grants it exactly two privileges (CREATE on the database, and USAGE on schema tap, since pgTAP is harness and pgxntool's setup.sql creates that schema as the connecting role, which grants nobody else access), and raises if the role is a superuser or a member of any managed-cloud equivalent - rds_superuser, cloudsqlsuperuser or azure_pg_admin, none of which carry rolsuper. Reverting superuser = false now fails the suite outright, with "permission denied to create extension". Every decision is made server-side, by a pg_temp function taking the role name and returning the role the session should run as, because psql can't branch before 10: \if is psql 10 and CI covers back to 9.4, where psql reports it as an invalid command and then runs the branch it should have skipped. \gset feeds that returned name into a plain SET ROLE. The file opens with RESET ROLE so a second \i in one session behaves like the first - not hypothetical, since on psql older than 10 install/load.sql's mode branches all run and this file gets reached twice. One case deliberately doesn't switch: a count_nulls already installed and owned by somebody else, which is what a real pg_upgrade leaves behind. PostgreSQL has no ALTER EXTENSION ... OWNER TO, so pg_dump can't carry an extension's ownership across - --binary-upgrade emits binary_upgrade_create_empty_extension(), which takes no owner, and the extension ends up belonging to whoever ran the restore. Member functions keep their owner; only the extension object is out of reach, which is exactly what test__shutdown__drop_all needs. Nothing is lost by staying put: existing mode installs nothing, so it was never the leg proving the install works unprivileged. Also fixes a live bug in install/load.sql's existing-mode assertion, whose nested $$-quoted RAISE message closed its own DO block's body, leaving the rest to be parsed as bare SQL - the whole assertion was a no-op. Fixed with a distinct tag. It stays silent for now because that file has no ON_ERROR_STOP; turning that on needs its \if branching to go server-side first, which is a separate change.
0110c07 to
a730479
Compare
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
…rtcoming The carve-off for an extension owned by somebody else reads like something that ought to be fixable, or like something a newer PostgreSQL would make unnecessary. It isn't either: extowner has had no matching ALTER EXTENSION ... OWNER TO since it was added in 2011, because what that should do to the contained objects was never settled, and it's been reported (BUG #18625) and acknowledged as a known shortcoming without being fixed.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
It's declared CONSTANT, so it takes the constant prefix, matching c_managed_superuser_roles right above it. v_role stays as it is - that one really is a loop variable.
|
Claude finished @jnasbyupgrade's task in 5m 30s —— View job Claude finished the code reviewTodo list
SummaryThis is a well-executed, thoroughly-documented change. I traced the core logic in
One documentation gap worth considering (non-blocking)
This is likely intentional/out of scope (the PR is about proving |
test/README.md had grown into a paraphrase of the comments in the files it describes, so it's now a layout list plus the two cross-cutting facts a reader needs before touching anything - the non-superuser role and the randomly named schema - each pointing at the file that explains itself. test/deps.sql said it was intentionally empty, which stopped being true when it took over switching to the test user, and spent a paragraph restating pgxntool's own docs. It now points at them instead. Records why the database grant can't move into the CREATE ROLE beside it: the role is cluster-wide and outlives a run, but the grant lives in the current database's ACL, so a run against a new database has to reissue it. Corrects the MEMBER-vs-USAGE note, which claimed being able to SET ROLE to a role is equivalent to holding its privileges. It isn't; the check is deliberately over-strict, and stays that way because nothing needs looser.
|
Claude finished @jnasbyupgrade's task in 11m 10s —— View job Code review postedRan a code review (
I also checked and ruled out several other candidates (transaction/rollback interaction with |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| RETURN current_user; | ||
| END IF; | ||
|
|
||
| IF current_setting('is_superuser') = 'on' THEN |
There was a problem hiding this comment.
Correctness/design gap: bootstrap never runs on the managed-cloud platforms this code explicitly targets.
The CREATE ROLE / GRANT bootstrap is gated only by current_setting('is_superuser') = 'on'. But c_managed_superuser_roles (lines 51-53) exists specifically because AWS RDS/Aurora, GCP Cloud SQL, and Azure Flexible Server admin accounts are members of rds_superuser/cloudsqlsuperuser/azure_pg_admin while deliberately never carrying rolsuper — which is exactly what makes is_superuser report off for them.
That means on a fresh database on any of these three platforms (which the comments show were deliberately considered), the bootstrap block can never execute, and the very first run fails at:
IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN
RAISE EXCEPTION
'role "%" does not exist, and this session is not a superuser so it cannot be created'
, p_test_user
;
END IF;...even when the connecting admin genuinely holds enough privilege (via rds_superuser membership, etc.) to create the role and issue the grants. The managed-role list is only consulted later (lines 123-141) as a negative check ("test user must not be a member of these"), never as a positive check to widen the bootstrap gate — e.g. current_setting('is_superuser') = 'on' OR pg_has_role(current_user, ANY(c_managed_superuser_roles), 'MEMBER').
There was a problem hiding this comment.
Claude here — fixed. The bootstrap gate now treats the managed-cloud admin roles as equivalent to superuser instead of consulting is_superuser alone, so the same role list drives both the bootstrap and the later disqualification check. Verifying this surfaced a second half of the same problem: an admin that isn't a real superuser can't SET ROLE to the role it just created, because from PG16 a CREATEROLE creator gets ADMIN on that role rather than SET — so it now grants the role to itself in that case, which a real superuser doesn't need. Verified by simulating a NOSUPERUSER role that is a member of rds_superuser. Landed in commit 6529b72.
| * not just before CREATE EXTENSION: a leftover schema is one this same role | ||
| * created on a previous run, so it's the role that must be able to drop it. | ||
| */ | ||
| \i test/helpers/test_user.sql |
There was a problem hiding this comment.
Correctness: stale-schema cleanup a few lines below can no longer drop schemas it doesn't own.
This \i test/helpers/test_user.sql now switches the session to the non-superuser test role before the cleanup loop further down in this file runs, per the added header comment above ("the switch has to come before the cleanup loop ... it's the role that must be able to drop it"). That reasoning only holds if every leftover count_nulls test schema % was created by that same role.
If a schema matching the prefix was left behind by a run predating this change (when the whole file still ran as a superuser), or by any session that isn't the test user (e.g. a manual superuser session, or a differently-named test role after a future rename), the cleanup loop's EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname) will now fail with "must be owner of schema ..." instead of self-healing — aborting the entire test run instead of quietly cleaning up, exactly the crash-resilience case that cleanup was written to handle.
Failure scenario: a developer with a persistent local Postgres instance who ran the suite before this branch (leaving a schema owned by the superuser), then checks out this branch and reruns make test — the very first run fails on that DROP instead of proceeding.
There was a problem hiding this comment.
Claude here — fixed. The stale-schema cleanup now runs before the role switch, so it executes as the connecting role rather than the test user. A leftover schema can belong to anyone (e.g. a superuser, if it's left over from a run predating this change), and only the connecting role is sure to be able to drop it. Landed in commit 6529b72.
test/helpers/test_user.sql becomes use_test_user.sql. The other helpers are named for the action running them performs (create_test_schema, find_test_schema); this one performed an action but read as a noun. Drops the comment above the SET ROLE, which restated what the two statements under it plainly do, and the one explaining why a nested RAISE message needs its own dollar-quote tag - that's general plpgsql knowledge, not something this site needs to teach.
|
Claude finished @jnasbyupgrade's task in 3m 14s —— View job Working on it
|
The bootstrap that creates the test user and grants it what it needs was gated on is_superuser alone. On RDS, Cloud SQL and Azure Flexible Server the admin account never reports that, by design, so on those platforms the bootstrap could never run even though the account has the rights - the managed roles were only ever consulted afterwards, to disqualify the test user. Both checks now consult the same list. An admin that isn't a real superuser also can't SET ROLE to the role it just created: from PG16 a CREATEROLE creator gets ADMIN on that role, which isn't SET. It now grants the role to itself in that case, which a real superuser doesn't need. Stale-schema cleanup moves ahead of the role switch, so it runs as the connecting role. A leftover schema can belong to anyone - one created by a run predating this change belongs to a superuser - and only the connecting role is sure to be able to drop it.
|
Claude finished @jnasbyupgrade's task in 8m 39s —— View job Working on it
|
Doubled single quotes ('' ... '') in RAISE EXCEPTION messages made two
call sites hard to read; switch both to distinct-tag dollar quoting so
the literal quotes read as themselves.
create_test_schema.sql's header comment had grown to explain nearly
everything the file does, most of it far from the code it was
describing. Cut it to a few lines on what the file is and why it
exists standalone (bin/test_existing's prepare-old runs it outside any
psql session that also runs load.sql), and move each remaining
rationale down next to the code it explains: the GUC-bridging and
'current' sentinel notes by the guard, the crash-recovery note by the
cleanup loop, the identifier-quoting note by the name generation, and
the WITH SCHEMA note by CREATE EXTENSION.
|
Claude finished @jnasbyupgrade's task in 2m 34s —— View job Code review postedTodo list
I re-reviewed the full diff against
New finding
but the file was renamed to No other issues found. Docs ( |
count_nulls_prepare_test_user() now takes the load mode as an explicit argument (the caller must supply it - the GUC it comes from isn't set for every invocation, e.g. bin/test_existing's prepare-old) and only skips the role switch for a foreign-owned extension when that mode is 'existing'. In fresh/update, a foreign-owned count_nulls is a cleanup miss, not a preserved pg_upgrade artifact, so it now raises instead of silently letting the suite run as the connecting role.
|
Claude finished @jnasbyupgrade's task in 3m 25s —— View job Code review postedTodo list
This synchronize added one new commit since the last review round: I traced every call site of
New findings
No other issues found. The role-switch and mode-scoping logic is sound, and CI's |
| relocatable = false | ||
| # Pure SQL functions, nothing privileged - anyone with CREATE on the target | ||
| # schema can install it. Enforced by the suite running as a non-superuser | ||
| # (test/helpers/test_user.sql), which fails outright if this reverts to the |
There was a problem hiding this comment.
Still stale: this comment references test/helpers/test_user.sql, but the file was renamed to test/helpers/use_test_user.sql in commit 8b32f15. Flagged in an earlier review round on this PR but not yet fixed — it's the only remaining reference to the old name anywhere in the tree.
| * Never reached in existing mode - see this file's own header - so a | ||
| * literal is enough; there's no GUC to lose by not reading it. | ||
| */ | ||
| \set count_nulls_load_mode 'fresh' |
There was a problem hiding this comment.
Nit: this literal is also used for the 'update' mode invocation (test/install/load.sql's \if :count_nulls_update_mode branch, which sets :version '0.9.6' and then \is this file) — not just 'fresh'. It's harmless for the actual role-switch logic (use_test_user.sql only distinguishes 'existing' from everything else), but if the new foreign-owned-extension RAISE EXCEPTION in use_test_user.sql:115-118 ever fires during an actual update run, the error text will claim load mode "fresh" expects no pre-existing installation even though TEST_LOAD_SOURCE=update was what ran — mildly misleading for anyone debugging that failure. Might be worth a value like 'not-existing' (or similar) to avoid implying a specific mode that isn't necessarily the one that ran.
comment, and trim an unenforceable ownership claim The load-mode literal comment asserted an unenforced fact about the rest of the system instead of stating the intent of this code path; restate it as intent, noting the loud-failure fallback if that intent ever breaks. The WITH SCHEMA comment had ended up above an unrelated CASE expression when the header was split up; move it back next to the CREATE EXTENSION statement it documents. The extension-owner short-circuit's comment assumed a foreign-owned extension always came from a real pg_upgrade restore, which the code doesn't and can't know; trim it to the two facts the code actually depends on plus the one non-obvious reason (no ALTER EXTENSION ... OWNER TO) existing mode can meet a foreign owner at all.
|
Claude finished @jnasbyupgrade's task in 1m 46s —— View job Code review postedTodo list
This synchronize added one new commit since the last review round: I re-traced the changes against their call sites:
Finding (posted inline)
No other issues found. The role-switch, extension-ownership, and load-mode logic all check out against every call site. |
| relocatable = false | ||
| # Pure SQL functions, nothing privileged - anyone with CREATE on the target | ||
| # schema can install it. Enforced by the suite running as a non-superuser | ||
| # (test/helpers/test_user.sql), which fails outright if this reverts to the |
There was a problem hiding this comment.
Still stale: this comment references test/helpers/test_user.sql, but the file was renamed to test/helpers/use_test_user.sql back in commit 8b32f15. This is the third review round flagging it — it's the only remaining reference to the old filename anywhere in the tree.
| # (test/helpers/test_user.sql), which fails outright if this reverts to the | |
| # (test/helpers/use_test_user.sql), which fails outright if this reverts to the |
…load mode count_nulls.control referenced test/helpers/test_user.sql, which was renamed to use_test_user.sql; the tree-wide grep for that rename had been scoped to subdirectories and missed it. create_test_schema.sql hardcoded \set count_nulls_load_mode 'fresh' right before including use_test_user.sql, which both invented a mode the caller never asked for and would silently override one a caller had legitimately set. It's already wrong in practice: test/install/load.sql's update leg runs through this file with the mode hardcoded to 'fresh' even though it's really 'update'. The value now comes from whichever caller genuinely knows it - test/install/load.sql (derived from count_nulls.test_load_mode, the same way test/deps.sql already does) or bin/test_existing's -v on the command line (always a genuine fresh install) - and create_test_schema.sql only consumes :count_nulls_load_mode, same as every other includer of use_test_user.sql.
|
Claude finished @jnasbyupgrade's task in 1m 17s —— View job Code review postedTodo list
This synchronize added one new commit since the last review round:
I traced every caller of No new issues found. All findings from the previous review rounds — stale filename, managed-cloud bootstrap gate, stale-schema cleanup ordering, and now the load-mode threading nit — are resolved. The one remaining open item (test role holding database-level |
1 participant
count_nulls is nothing but SQL functions, so requiring a superuser to
CREATE EXTENSIONit was never anything more than the control file's default. Setsuperuser = false.For that to stay true, the suite now runs as an ordinary role throughout — both the install session and every test session.
test/helpers/test_user.sqlcreates "Test user for count_nulls" if it isn't there, grants it exactly two privileges (CREATEon the database, andUSAGEon schematap, since pgTAP is harness and pgxntool'ssetup.sqlcreates that schema as the connecting role, which grants nobody else access), and raises if the role is a superuser or a member of any managed-cloud equivalent —rds_superuser,cloudsqlsuperuserorazure_pg_admin, none of which carryrolsuper. Revertingsuperuser = falsenow fails the suite outright, with "permission denied to create extension".Every decision is made server-side, by a
pg_tempfunction taking the role name and returning the role the session should run as, because psql can't branch before 10:\ifis psql 10 and CI covers back to 9.4, where psql reports it as an invalid command and then runs the branch it should have skipped.\gsetfeeds that returned name into a plainSET ROLE. The file opens withRESET ROLE, so what it does depends only on how the session connected.One case deliberately doesn't switch: a count_nulls already installed and owned by somebody else, which is what a real
pg_upgradeleaves behind. PostgreSQL has noALTER EXTENSION ... OWNER TO, so pg_dump can't carry an extension's ownership across —--binary-upgradeemitsbinary_upgrade_create_empty_extension(), which takes no owner, and the extension ends up belonging to whoever ran the restore. Its member functions keep their owner; only the extension object itself is out of reach, which is exactly whattest__shutdown__drop_allneeds. That's an acknowledged upstream shortcoming (BUG #18625), not something a newer PostgreSQL will retire, so the branch is commented as permanent. Nothing is lost by staying put: existing mode installs nothing, so it was never the leg proving the install works unprivileged.Also fixes a live bug in
test/install/load.sql's existing-mode assertion, whose nested$$-quoted RAISE message closed its ownDOblock's body, leaving the remainder to be parsed as bare SQL — the whole assertion was a no-op. Fixed with a distinct tag. It stays silent for now because that file has noON_ERROR_STOP; turning that on needs its\ifbranching to go server-side first, which is #63.