Pick the test/install load mode server-side, and turn on ON_ERROR_STOP - #63
Pick the test/install load mode server-side, and turn on ON_ERROR_STOP#63jnasbyupgrade wants to merge 11 commits into
Conversation
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.
|
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. |
…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. |
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
|
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.
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 |
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.
![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 |
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.
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.
| 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.
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.
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.
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.
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. |
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.
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. |
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.
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. |
…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.
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.
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 →
1 participant
test/install/load.sql'sRAISE 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_STOPis what makes the file fail the way its own header says it does.That couldn't just be switched on, because
\ifis 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-enteredcreate_test_schema.sql, whose stale-schema cleanup dropped that schemaCASCADE— taking the just-updated extension with it — and installed current fresh instead.make test TEST_LOAD_SOURCE=updateon 9.4/9.5/9.6 has therefore never once exercised the update path. UnderON_ERROR_STOPthe same\ifwould 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 whattest/helpers/test_user.sqlalready does, for the same reason. The schema creation and install it shares withbin/test_existing'sprepare-oldmoves totest/helpers/install_extension.sql, which only defines the function;test/helpers/create_test_schema.sqlis now just the call, and exists as a file purely becauseprepare-oldinvokes it as a standalonepsql -f.Drops the
client_min_messagessuppression that used to wrap theALTER EXTENSION ... UPDATE. Postgres already raises it to at leastWARNINGfor 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.Stacks on #62, so the commit list here shows those commits until it merges.