Pick the test/install load mode server-side, and turn on ON_ERROR_STOP - #63
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 |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
1 similar comment
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
9734de4 to
40ec4ea
Compare
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude finished @jnasbyupgrade's task in 4m 22s —— View job Claude's reviewTasks
SummaryThis commit ( Finding: stale "see X" pointers left behind by the
|
40ec4ea to
37f8e18
Compare
|
Claude finished @jnasbyupgrade's task in 22m 11s —— View job Claude's reviewTasks
SummaryRan a code review of this PR (
Both findings went through an independent verification pass before posting. Separately fact-checked that removing the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| END LOOP; | ||
| END | ||
| $$; | ||
| \i test/helpers/test_user.sql |
There was a problem hiding this comment.
Correctness / test-fidelity regression: Adding \i test/helpers/test_user.sql here makes bin/test_existing's prepare-old install count_nulls as the Test user for count_nulls role instead of as the connecting (super)user.
For the pg-tle-test CI job's existing-mode leg — which, per bin/test_existing's own header comment, reuses prepare-old -> update -> run-suite with no real binary pg_upgrade in between — this means the extension's owner going into run-suite is now the test user itself. That silently makes test_user.sql's "extension already installed and owned by somebody else" branch (IF c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user THEN RETURN current_user) unreachable via this path: c_extension_owner at prepare-old time is NULL (nothing installed yet) so the switch-to-test-user proceeds and installs as that role; by run-suite time c_extension_owner already equals p_test_user, so the "stay put" branch that's supposed to model a real pg_upgrade's ownership loss never fires for the pg-tle-test job. No test fails — the existing-mode leg just silently stops exercising the ownership-mismatch scenario it's documented to model.
(For the real pg-upgrade-test job this is masked because a genuine binary pg_upgrade resets ownership to whoever performs the restore regardless of what prepare-old set it to — so the gap is specific to the pg_tle path.)
Verified: before this diff create_test_schema.sql did no role-switching at all (ran as the connecting role), and bin/test_existing's prepare-old/CI invocations pass no PGUSER/role override, so this is a genuine behavior change, not a false alarm.
There was a problem hiding this comment.
Claude here — this one's intended, not a regression, and applies to both PRs. prepare-old installing as the test user rather than superuser is the exact property this work exists to prove (count_nulls needs no superuser rights to install); the pg_upgrade leg still works because the owner short-circuit, now scoped to existing mode, leaves that later session running as the connecting role rather than switching it.
| SELECT pg_temp.count_nulls_load( | ||
| current_setting('count_nulls.test_load_mode') | ||
| , current_setting('count_nulls.test_existing_deploy') | ||
| ) AS count_nulls_loaded | ||
| \gset |
There was a problem hiding this comment.
Minor robustness regression: count_nulls.test_existing_deploy is now read unconditionally for every mode (as a plain argument to pg_temp.count_nulls_load), whereas before this refactor it was only read from inside the existing-mode branch (\if :count_nulls_existing_mode).
current_setting() is called here without missing_ok, so if this file is ever driven directly (e.g. psql -c "SET count_nulls.test_load_mode='fresh'" -f test/install/load.sql, bypassing this repo's Makefile) with only the load-mode GUC set, it now raises unrecognized configuration parameter "count_nulls.test_existing_deploy" even in fresh/update mode, where that GUC is semantically irrelevant and previously was never touched.
This is masked in the normal workflow because the Makefile unconditionally exports count_nulls.test_existing_deploy via PGOPTIONS (default 'filesystem') regardless of TEST_LOAD_SOURCE, so make test/make test-update never hits it — but it does broaden the "fail loudly on unpropagated GUC" behavior beyond what the surrounding comment describes, for any caller that invokes this file directly.
There was a problem hiding this comment.
Claude here — acknowledged, deferring; this one is #63-specific. #62 only reads count_nulls.test_existing_deploy inside its existing-mode branch, but #63's server-side mode dispatch (d1a820e) reads it unconditionally. The Makefile always exports it regardless of mode though, so every supported invocation still sets it - reading it unconditionally doesn't introduce a case where it's actually missing.
37f8e18 to
4acb9f3
Compare
|
Claude finished @jnasbyupgrade's task in 4m 36s —— View job Code review complete
SummaryI re-diffed Both previously-flagged findings remain accurate, so I'm not duplicating them, but noting current status:
I also checked No new correctness issues found in this PR's own diff. The |
4acb9f3 to
964e460
Compare
|
Claude finished @jnasbyupgrade's task in 10m 38s —— View job Code review complete
SummaryReviewed the current diff ( The refactor itself — moving Three findings posted as inline comments, all in the new
|
| RETURN current_user; | ||
| END IF; | ||
|
|
||
| IF current_setting('is_superuser') = 'on' THEN |
There was a problem hiding this comment.
Correctness: managed-cloud "superuser" roles can never bootstrap the test role.
The bootstrap block (create the role, GRANT CREATE ON DATABASE, GRANT USAGE ON SCHEMA tap) only runs IF current_setting('is_superuser') = 'on'. is_superuser reflects the actual rolsuper catalog bit of the current role — it is never on for AWS RDS/Aurora's rds_superuser, GCP Cloud SQL's cloudsqlsuperuser, or Azure Flexible Server's azure_pg_admin, none of which carry rolsuper (as this very function's own comment on c_managed_superuser_roles a few lines below explicitly notes).
Those roles are only ever consulted later, as a restriction (the test user must not be a member of one) — never as an alternate condition for allowing the bootstrap. So the very first run of this suite against a fresh managed-cloud database, connected as that platform's admin role, hits:
IF NOT EXISTS(...) THEN
RAISE EXCEPTION 'role "%" does not exist, and this session is not a superuser so it cannot be created', p_test_user;
even though the connecting role has ample privilege (it can create roles and grant CREATE ON DATABASE in practice) to do exactly that. Given the pg_tle CI job this suite ships (pg-tle-test) exists specifically because pg_tle is commonly deployed on exactly these managed platforms, this looks like a real gap rather than a merely theoretical one — worth gating the bootstrap on current_setting('is_superuser') = 'on' OR (SELECT bool_or(pg_has_role(current_user, r, 'MEMBER')) FROM unnest(c_managed_superuser_roles) r) (or similar) rather than is_superuser alone.
There was a problem hiding this comment.
|
|
||
| IF current_setting('is_superuser') = 'on' THEN | ||
| IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN | ||
| EXECUTE format('CREATE ROLE %I', p_test_user); |
There was a problem hiding this comment.
Correctness: check-then-create race on the test role.
IF NOT EXISTS(SELECT 1 FROM pg_roles WHERE rolname = p_test_user) THEN
EXECUTE format('CREATE ROLE %I', p_test_user);
END IF;
p_test_user is a fixed, non-randomized name ('Test user for count_nulls', unlike the randomized schema name), and this role is cluster-wide, not per-database. If two invocations of this suite run concurrently against the same Postgres cluster (different databases) — e.g. a developer running make test and make test-update locally at the same time, or any future CI layout that shares one instance across parallel jobs — both sessions can see "role does not exist" and both execute CREATE ROLE. The loser gets an unhandled duplicate_object error, and since this file runs under ON_ERROR_STOP, that whole install/load.sql (or deps.sql) session aborts.
Not exploitable in this repo's current CI (each job gets its own isolated pg-start cluster), but it's a real hazard for local/dev use and worth at least a comment, if not a CREATE ROLE IF NOT EXISTS / exception-swallowing guard.
There was a problem hiding this comment.
Claude here — acknowledged, not fixing now; applies to both PRs (shared code on #62). pgxntool's schedule-based install file completes before the parallel test/sql/ batch starts, so by the time any session could race on the check-then-create, the role already exists. Deferring rather than dismissing, and tracking it for follow-up since that ordering is an external invariant this file doesn't enforce itself.
There was a problem hiding this comment.
Claude here — the race is now documented in a comment at the site (a068c29). The actual hardening remains deferred and tracked separately rather than fixed here.
| IF EXISTS(SELECT 1 FROM pg_namespace WHERE nspname = 'tap') THEN | ||
| EXECUTE format('GRANT USAGE ON SCHEMA tap TO %I', p_test_user); | ||
| END IF; | ||
| END IF; |
There was a problem hiding this comment.
Efficiency (minor): the whole bootstrap block (role-existence check, GRANT CREATE ON DATABASE, tap-schema GRANT USAGE) re-runs on every per-test-file session, not just once — test/deps.sql \i's this file for every file under test/sql/ (currently just one file, but the mechanism re-executes per session regardless). Each of the GRANTs and EXISTS checks is idempotent so this is harmless, but it's a few avoidable catalog lookups/statements per session that only ever need to happen once, right after install/load.sql creates the role. Not blocking, just noting it as wasted work if this suite ever grows more test files.
There was a problem hiding this comment.
Claude here — acknowledged as real but non-blocking; applies to both PRs (shared code on #62). The bootstrap statements (role creation, grants) are idempotent, so re-running them every session is wasted work, not a correctness risk. Deferring that optimization and tracking it for follow-up.
964e460 to
0b817de
Compare
|
Claude finished @jnasbyupgrade's task in 7m 2s —— View job Code review complete
SummaryReviewed the current diff (
Both are genuine ordering/scoping regressions introduced by moving mode selection server-side, not issues with the |
| \i test/helpers/use_test_user.sql | ||
| \i test/helpers/extension_installer.sql |
There was a problem hiding this comment.
Correctness: stale-schema cleanup now runs after the switch to the test user, undoing the fix from 6529b72 ("Stale-schema cleanup moves ahead of the role switch, so it runs as the connecting role").
Here test/helpers/use_test_user.sql (the SET ROLE to the non-superuser test user) runs before test/helpers/extension_installer.sql is even loaded. count_nulls_load() — created and invoked later in this same file, i.e. after the switch — calls pg_temp.count_nulls_cleanup_test_schemas() internally (line 106), so cleanup executes as the test user, not the connecting role.
Compare with the sibling file test/helpers/create_test_schema.sql, which gets the order right: \i extension_installer.sql → SELECT pg_temp.count_nulls_cleanup_test_schemas() (still the connecting role) → \i use_test_user.sql (switch) → install.
extension_installer.sql's own header comment states the contract this file breaks: "every caller can run cleanup before switching to the test user … while installing after - a leftover schema can belong to any role, and only the connecting one is sure to be able to drop it."
Failure scenario: a prior fresh/update run crashes before its own teardown, leaving count_nulls test schema * behind owned by a superuser (or any role other than the test user) — exactly the scenario 6529b72 called out ("one created by a run predating this change belongs to a superuser"). The next run's cleanup now executes as the test user, which doesn't own that schema, so DROP SCHEMA %I CASCADE fails with a permission error instead of cleaning it up.
There was a problem hiding this comment.
Claude here — already fixed in d1a820e. Cleanup now runs before the role switch, mode-gated, so in existing mode it doesn't drop the schema the pg_upgrade run is being verified to have preserved.
| IF c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user THEN | ||
| RETURN current_user; |
There was a problem hiding this comment.
Correctness: this owner short-circuit isn't scoped to "existing" mode, so it can silently defeat the non-superuser guarantee in fresh/update mode too.
count_nulls_prepare_test_user() is called unconditionally at the top of test/install/load.sql, before the caller's count_nulls.test_load_mode is even read. If count_nulls already exists in the target database under some other owner (e.g. a leftover extension from an earlier interrupted/manual run, installed by a superuser), this check returns current_user without ever switching to the test user — regardless of whether the actual requested mode is fresh or update.
Failure scenario: a stale count_nulls extension (owned by, say, postgres) lingers in the test database when make test TEST_LOAD_SOURCE=fresh runs. count_nulls_prepare_test_user() sees c_extension_owner IS NOT NULL AND c_extension_owner <> p_test_user, returns current_user (the superuser), and the session never switches roles. count_nulls_load() then runs cleanup + fresh install entirely as the superuser, with no error — silently defeating the very property (superuser = false actually works) this test suite exists to prove.
There was a problem hiding this comment.
Claude here — fixed: count_nulls_prepare_test_user() now takes the load mode as an explicit argument, and the owner short-circuit only returns current_user when that mode is existing; otherwise it raises, naming both the owner and the test user. This applies to both PRs (the function lives on #62); the fix lands there and reaches #63 by rebase.
0b817de to
d1a820e
Compare
|
Claude finished @jnasbyupgrade's task in 3m 57s —— View job Code review complete
SummaryBranch has moved again since the last review (now Both previously-confirmed findings are now fixed:
Re-examined the one finding that looked like it might still be open — the "owner short-circuit isn't scoped to mode" point from the 20:55 review ( No new correctness issues found in No new inline comments posted; nothing new to flag. |
d1a820e to
3178cd9
Compare
|
Claude finished @jnasbyupgrade's task in 2m 21s —— View job Code review complete
SummaryBranch has moved again since the last review (now Both previously-open findings from the 20:55 review are now fixed:
Traced both fixes through all three call sites ( Not re-flagging (already covered in earlier rounds on this PR, unchanged by these two commits, and either minor or out of this PR's own diff):
Spot-checked No new correctness issues found; no inline comments needed this round. |
3178cd9 to
3b63cfb
Compare
|
Claude finished @jnasbyupgrade's task in 4m 8s —— View job Code review complete
SummaryThe branch has been rewritten again since the last review round (history restructured — commits reordered/rebased, current tip All previously-confirmed findings on this PR remain fixed in the current state:
I also re-checked Not re-flagging (deferred/accepted in earlier rounds on this PR, unchanged by the latest commits):
No new correctness issues found. No new inline comments needed this round. |
3b63cfb to
c7179df
Compare
|
Claude finished @jnasbyupgrade's task in 3m 45s —— View job Code review complete
SummaryBranch has moved since the last review round: two new commits,
All previously-confirmed findings on this PR (cleanup-order-before-role-switch, the owner short-circuit scoped to One new finding, posted inline: No other new correctness issues found. The |
| # CREATE EXTENSION count_nulls at VERSION, into a freshly, randomly | ||
| # generated schema - shared with test/install/load.sql's own fresh/update | ||
| # installs via test/helpers/create_test_schema.sql (see that file for the | ||
| # full rationale). | ||
| # full rationale). This invocation sets no count_nulls.test_load_mode GUC, | ||
| # so count_nulls_load_mode is supplied here instead: it's a genuine fresh | ||
| # install (prepare-old), never 'existing'. |
There was a problem hiding this comment.
Stale pointer, in a comment this PR's own commit (06bce4e) just edited: "shared with test/install/load.sql's own fresh/update installs via test/helpers/create_test_schema.sql (see that file for the full rationale)" is no longer true. test/install/load.sql doesn't touch create_test_schema.sql at all anymore — it \i's test/helpers/extension_installer.sql directly (test/install/load.sql:26), and that's also where the shared cleanup/install logic and its rationale actually live now. create_test_schema.sql's own header already reflects this correctly: "the work itself lives in test/helpers/extension_installer.sql."
This sentence predates this PR (it was flagged as stale in an earlier review round, when this file wasn't part of the diff yet), but 06bce4e added two new sentences immediately after it in this same comment block without correcting the one that was already wrong — so it's now shipping inaccurate documentation as part of this PR's own change, not just inheriting it unmodified.
Fix stale create_test_schema.sql pointer in bin/test_existing →
There was a problem hiding this comment.
Claude here — confirmed stale: test/install/load.sql \i's test/helpers/extension_installer.sql directly and doesn't reference create_test_schema.sql anywhere. The comment now points at test/helpers/extension_installer.sql as the shared install logic (and the rationale), and describes create_test_schema.sql as the thin wrapper that calls it, kept as a file only so this script can invoke it standalone; the two count_nulls_load_mode sentences are unchanged, since they were correct. Fixed in bd1a7b1.
test/install/load.sql's RAISE EXCEPTIONs were decorative. pg_regress resolves its expected and result paths to the same file, so the diff is self-identical no matter what, and a psql that reports an error, carries on and exits 0 leaves pg_regress nothing to notice. ON_ERROR_STOP is what makes the file fail the way its own header says it does. That couldn't just be switched on, because \if is psql 10 and CI covers back to 9.4, where psql reports it as an invalid command - and then, having skipped nothing, runs every branch. On those versions load.sql already behaved badly, silently: the existing-mode assertion fired and errored in every mode, and update mode installed 0.9.6, updated it, then re-entered create_test_schema.sql, whose stale-schema cleanup dropped that schema CASCADE - taking the just-updated extension with it - and installed current fresh instead. `make test TEST_LOAD_SOURCE=update` on 9.4/9.5/9.6 has therefore never once exercised the update path. Under ON_ERROR_STOP the same \if would have aborted those legs outright. So the mode selection moves server-side, into pg_temp.count_nulls_load(), leaving load.sql with no branching of its own - matching what test/helpers/test_user.sql already does, for the same reason. The schema creation and install it shares with bin/test_existing's prepare-old moves to test/helpers/install_extension.sql, which only defines the function; test/helpers/create_test_schema.sql is now just the call, and exists as a file purely because prepare-old invokes it as a standalone psql -f. Drops the client_min_messages suppression that used to wrap the ALTER EXTENSION UPDATE. Postgres already raises it to at least WARNING for the duration of an update script and restores the caller's setting afterwards, so it was redundant, and being unconditional it lowered the level for a caller who had set something stricter. Confirmed: update mode still emits no NOTICEs without it.
test/install/load.sql no longer goes through create_test_schema.sql; both it and this script reach the shared install logic via test/helpers/extension_installer.sql, which is also where the rationale now lives. create_test_schema.sql is just the wrapper that calls it, kept as a file so prepare-old can run it standalone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c7179df to
bd1a7b1
Compare
|
Claude finished @jnasbyupgrade's task in 2m 1s —— View job Code review complete
SummaryReviewed the current diff (2 commits on top of All previously-confirmed findings on this PR remain fixed:
Not re-flagging (acknowledged/deferred in earlier rounds on this PR, unchanged):
One very minor, non-blocking observation not worth an inline comment: in No new correctness issues found. No inline comments needed this round. |
Note in a comment that the CREATE ROLE guard in count_nulls_prepare_test_user() is not atomic, and why that's safe today: load.sql finishes before pg_regress starts the concurrent test/sql/ sessions, so nothing races to create the role.
|
Claude finished @jnasbyupgrade's task in 1m 3s —— View job Code review complete
SummaryThe branch gained exactly one new commit since the last review round: The comment documents the check-then-create race on All previously-confirmed findings on this PR remain fixed, unaffected by this commit:
Not re-flagging (acknowledged/deferred by the author in earlier rounds, now further justified by this commit's own comment):
No new correctness issues found. No inline comments needed this round. |
1 participant
test/install/load.sql'sRAISE EXCEPTIONs were decorative: pg_regress resolves its expected and result paths to the same file, so nothing ever failed on error, and turning onON_ERROR_STOPis what makes it actually fail as its header claims.That couldn't be turned on while the file used
\if, which is psql 10 while CI covers back to 9.4 — there psql reports it as an invalid command and runs every branch, soTEST_LOAD_SOURCE=updatehad never actually exercised the update path on 9.4/9.5/9.6. Mode selection now happens server-side instead, and the shared schema-creation/install logic moves intotest/helpers/extension_installer.sql.