Install extension_drop into a fresh, randomly named schema on every test run

[jnasbyupgrade]

Summary

test/install/load.sql installs extension_drop into a freshly created schema with a constant, quote-forcing name prefix plus a random suffix, on every single fresh/update-mode test run.

• extension_drop's control file has no schema= line, so it's genuinely install-schema-flexible -- nothing in its own SQL may assume where it landed.
• test/install/load.sql (fresh/update modes only): cleans up any leftover schema from a crashed prior run, generates a new random name, CREATE SCHEMAs it, and targets a SINGLE CREATE EXTENSION extension_drop:suffix statement at it via a combined schema+version+cascade suffix built from chained CASE expressions -- never by mutating search_path first, which would mask exactly the qualification bug this exists to catch. existing mode is untouched.
• test/deps.sql rediscovers that schema per test-file session (pg_namespace joined to pg_extension, not extnamespace::regnamespace, which would reinterpret its quoted display form's literal quote characters as part of the name) -- and deliberately does NOT add it to search_path. test/sql/simple.sql and test/sql/dependency_guard.sql qualify their calls into extension_drop's own functions explicitly via :'extension_drop_schema' instead.
• test/finish.sql: this suite has no shared teardown-function library to hook a check into (plain pgTAP files, not runtests()), so every test file calls this instead of test/pgxntool/finish.sql directly -- it asserts extension_drop's schema was never reachable via search_path, then delegates to the real finish.sql.
• test/sql/schema.sql (the original dedicated schema-targeting test) is removed: its job -- proving the extension works installed outside search_path, with an explicit second schema and qualified cross-schema calls -- is now redundant. Every fresh/update run already installs into a random, never-on-search_path schema, and every remaining test file already qualifies its own calls. Traced each of its assertions against the rest of the suite individually before removing it; every one has a real equivalent elsewhere.
• No new Makefile machinery: no GUC, no variable, no extra target. It's unconditional.

Reference, not a port

Inspired by https://github.com/Postgres-Extensions/pg_count_nulls/pull/55's test/helpers/create_test_schema.sql, which implements the same idea for that (structurally simpler) extension. Deliberately NOT ported wholesale:

1. This repo has no bin/ script that independently needs to rediscover the schema name outside load.sql's own session -- only test/deps.sql does, so no find_test_schema.sql-style helper file was added.
2. This suite's test files are plain, independent pgTAP files using lives_ok/bag_eq/plan() directly, not count_nulls' shared runtests()/ncs() library architecture.
3. extension_drop depends on cat_tools (CASCADE-installed, with pre-PG10 special-casing); count_nulls has no extension dependencies. That logic in load.sql is untouched -- schema-targeting was added on top of it.
4. extension_drop_test (a helper extension this suite installs to exercise extension_drop's commands) is NOT randomized here -- deliberately deferred, tracked as https://github.com/Postgres-Extensions/extension_tools/issues/37.
5. TEST_LOAD_SOURCE's fresh/update/existing modes are untouched and unrelated to this change.

Verification

• Real, repeated make test runs (install-check + build groups both "All N tests passed") and make lint (zero findings) -- not dry runs, and re-verified from a fresh fetch of the pushed branch.
• Queried pg_extension/pg_namespace directly against the resulting test database across separate runs: confirmed a schema matching the constant prefix (never public), and confirmed different random suffixes across runs (not computed once and cached).
• Confirmed cleanup-before-create: only one matching schema exists at a time across repeated runs.
• Confirmed extension_drop's own functions are all created via a shared helper that attaches SET search_path FROM CURRENT, so internal cross-references are structurally protected by the schema captured at install time regardless of caller context -- this is what makes it safe for test/deps.sql to never add the schema to search_path while still relying on it structurally.

[coderabbitai]

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: 54da49ec-0992-4cf2-8caa-971e9febf9b2

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

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

Claude finished @jnasbyupgrade's task in 4m 4s —— View job

---

Claude finished the review

• Gather context (diff, CLAUDE.md, changed files)
• Understand the request
• Review test/install/load.sql changes
• Review test/deps.sql changes
• Review test/sql/schema.sql changes
• Post final review

Summary

This implements the schema-independence testing scheme described in the repo's CLAUDE.md: every fresh/update-mode test run now installs extension_drop into a freshly created, randomly named (and quoting-forcing) schema via CREATE EXTENSION ... SCHEMA, and test/deps.sql rediscovers that schema per-session from the catalog and folds it into search_path so the rest of the suite keeps working unqualified. existing mode is correctly left untouched, and test/sql/schema.sql's stricter "excluded from search_path" property is preserved. The overall design matches the CLAUDE.md guidance closely (targets via SCHEMA :"..." rather than mutating search_path pre-install, avoids the extnamespace::regnamespace quoting trap, no new Makefile machinery).

One real correctness/safety issue and a couple of minor observations below.

Findings

1. Unescaped _ in the cleanup LIKE pattern makes it broader than intended before a destructive DROP SCHEMA ... CASCADE — test/install/load.sql:153

FOR r IN SELECT nspname FROM pg_namespace WHERE nspname LIKE 'extension_drop test schema %' LOOP
  EXECUTE format('DROP SCHEMA %I CASCADE', r.nspname);
END LOOP;

In SQL LIKE, _ is a single-character wildcard, and the literal prefix extension_drop contains one (between "extension" and "drop"). So this pattern doesn't actually match only the literal string extension_drop test schema — it also matches extensionXdrop test schema ... for any single character X (e.g. extensionZdrop test schema 12345678 would also match and get dropped). The comment above it explicitly says the intent is "matching on the constant prefix," but the pattern as written is looser than that constant prefix.

Given this drives an unattended DROP SCHEMA ... CASCADE, precision matters even though collision is unlikely in these ephemeral per-run test databases. Fix by escaping the underscore or avoiding LIKE entirely, e.g.:

WHERE nspname LIKE 'extension\_drop test schema %' ESCAPE '\'
-- or
WHERE left(nspname, 28) = 'extension_drop test schema '

Fix this →

2. (Minor) search_path order changes relative to tap_setup.sql — test/deps.sql:33

test/pgxntool/tap_setup.sql sets search_path = tap, public before deps.sql runs and overrides it to :"extension_drop_schema", "$user", public, tap. tap moves from first to last. This is presumably harmless — pgTAP's function names are unlikely to collide with anything in public — but worth a conscious check that no pgTAP identifier gets shadowed by a same-named object in public now that public resolves first.

3. (Observation, not a bug) extension_drop_test implicitly rides along into the random schema

The PR description states extension_drop_test is deliberately not randomized (deferred to #37). Its control file also has no schema= line, though, and test/sql/simple.sql's unqualified CREATE EXTENSION extension_drop_test will now resolve to whatever schema is first in search_path — which, thanks to deps.sql, is the random extension_drop schema, not public. So in practice it already lands in the random schema for simple.sql's runs, just not deliberately/explicitly. Not a functional problem, just worth confirming the PR description's framing still matches actual behavior.

Nice touches worth calling out

• Rediscovering the schema via pg_namespace join instead of extnamespace::regnamespace avoids the exact quoting trap called out in CLAUDE.md.
• Targeting CREATE EXTENSION ... SCHEMA directly rather than pre-setting search_path correctly avoids masking qualification bugs.
• The constant prefix (extension_drop test schema , with trailing space) forces identifier quoting unconditionally, rather than relying on the random suffix coincidentally needing it.
• existing mode and test/sql/schema.sql's stricter exclusion test are both left untouched, matching their distinct purposes.

[github-actions]

Claude finished @jnasbyupgrade's task in 3m 3s —— View job

---

Claude finished the re-review

• Gather context (diff vs origin/master, new commits since last review)
• Confirm the previously-flagged LIKE/_ cleanup bug was fixed
• Re-review test/install/load.sql
• Re-review test/deps.sql and test/sql/schema.sql (unchanged since last review)
• Post updated review

Summary

This is a re-review after two new commits (a5acd94, 7ba46cc) that address the correctness issue from the previous review pass. The design still matches the CLAUDE.md guidance: test/install/load.sql installs extension_drop into a freshly created, randomly named, quoting-forcing schema via CREATE EXTENSION ... SCHEMA on every fresh/update run; test/deps.sql rediscovers it per-session from pg_namespace/pg_extension (avoiding the extnamespace::regnamespace quoting trap) and folds it into search_path; existing mode and test/sql/schema.sql's stricter "excluded from search_path" test are both left untouched.

Previously-flagged issue: fixed ✅

Unescaped _ in cleanup pattern (test/install/load.sql:153) — the cleanup DO block now matches via nspname ~ '^extension_drop test schema ' (POSIX regex) instead of LIKE. Regex treats _ as a literal character (unlike LIKE, where it's a single-char wildcard), so this now correctly matches only the intended constant prefix before the DROP SCHEMA ... CASCADE. Good fix.

Remaining observations (unchanged from prior review, non-blocking)

1. search_path order moves tap from first to last — test/deps.sql:33

test/pgxntool/tap_setup.sql sets search_path = tap, public; deps.sql now overrides it to :"extension_drop_schema", "$user", public, tap, so public now resolves before tap. Almost certainly harmless (pgTAP identifiers are unlikely to collide with anything in public), but worth a conscious check that no pgTAP function gets shadowed.

2. extension_drop_test implicitly rides along into the random schema

The PR description says extension_drop_test's randomization is deliberately deferred (#37). It also has no schema= line in its control file, though (test/extension/extension_drop_test.control), and test/sql/simple.sql's unqualified CREATE EXTENSION extension_drop_test resolves via search_path — which, thanks to deps.sql, now puts the random extension_drop schema first, not public. So in practice it already lands in the random schema for simple.sql's runs, just not deliberately. Not a functional problem, just worth confirming this framing still matches the intent of #37.

Nice touches

• Switching from LIKE to ~ for the prefix match is the right fix — no need for ESCAPE clauses or left()/length() workarounds, and it reads clearly.
• Cleanup-before-create ordering (drop leftover extension → drop leftover schemas by prefix → generate new name → create) avoids collisions across runs on a persistent cluster.
• Comments throughout clearly explain why (schema= vs relocatable=, the regnamespace quoting trap, why SCHEMA targeting beats pre-setting search_path), matching the rationale in this repo's CLAUDE.md.

I wasn't able to actually execute make test/make lint in this sandbox (no live Postgres cluster reachable), so this review is based on static reading of the SQL rather than a live run — the author's own test plan in the PR description already covers that.

[github-actions]

Claude is reviewing this PR

• Gather context (diff vs origin/master, new commits since last review)
• Review changed files
• Post review

View job run

[github-actions]

Claude finished @jnasbyupgrade's task in 3m 31s —— View job

---

Claude finished the re-review

• Gather context (diff vs origin/master, new commits since last review)
• Review changes since last review pass (7c7857b..HEAD: 4 new commits)
• Post review

Summary

Four new commits since the last review substantially reshape the PR to match its current description: test/sql/schema.sql (and test/expected/schema.out) are removed entirely, test/install/load.sql's CREATE EXTENSION calls are consolidated into one statement via a combined suffix, the random schema is now never added to search_path (previously it was — this also resolves my prior comment about search_path ordering by removing the line instead of reordering it), and a new test/finish.sql asserts at the end of every test file that the schema stayed unreachable via search_path (skipped in existing mode, via _1.out alternate expected files). test/sql/simple.sql and test/sql/dependency_guard.sql now qualify their calls into extension_drop's own objects via :'extension_drop_schema' instead of relying on search_path. I traced the removed schema.sql's assertions against the rest of the suite and they check out as the PR description claims — the off-search_path property, the quoting-forced schema targeting, and the qualified cross-schema calls are now all covered elsewhere.

Previously-flagged issues are both resolved:

• The unescaped _ in the cleanup LIKE pattern — fixed (regex ~) in an earlier pass, confirmed still correct.
• The search_path ordering concern — moot now that the schema is never added to search_path at all.

One new issue found in this pass:

Findings

1. bin/test_existing still references the now-deleted test/sql/schema.sql — bin/test_existing:59-76,97-103,244

EXISTING_MODE_EXCLUDE="schema"

test/sql/schema.sql was removed in this PR (commit 008bbc3), but bin/test_existing — used by the CI pg-upgrade-test job — still has EXISTING_MODE_EXCLUDE="schema" plus ~20 lines of comments (lines 59–76 and 97–102) explaining in detail why schema.sql must be excluded from existing-mode runs (its DROP EXTENSION/recreate-in-several-schemas behavior being incompatible with the persistent dependency guard). There's also a comment at line 244 referencing "the schema.sql conflict above".

Functionally this is harmless today: existing_mode_regress() (line 211) derives its list from test/sql/*.sql on disk and only skips a file if its basename matches EXISTING_MODE_EXCLUDE; since no schema.sql exists anymore, the exclude simply matches nothing and both remaining files (simple, dependency_guard) run in existing mode as expected. But it's dead configuration with now-inaccurate rationale in a script that other contributors will read to understand why existing mode excludes certain tests — exactly the kind of stale reference the other follow-through commit (7cb765f) already cleaned up in CLAUDE.md/load.sql/finish.sql. Worth removing EXISTING_MODE_EXCLUDE (and the comments justifying it) in the same cleanup pass, unless another test file is expected to need this exclusion mechanism soon.

Fix this →

Nice touches (new this pass)

• test/finish.sql's existing-mode skip is clean: reuses :'extension_drop_schema' captured once by deps.sql rather than rediscovering it, and correctly explains why existing mode can't make the same assertion (placement is outside the suite's control).
• The consolidated CREATE EXTENSION extension_drop:extension_drop_create_suffix in load.sql correctly folds all four fresh/update × pre/post-PG10 combinations into one statement via a single built suffix, replacing what was previously two duplicated \if/\else branches.
• dependency_guard.sql's CREATE VIEW ... \gexec correctly qualifies extension_drop__commands via %I against :'extension_drop_schema' now that the schema isn't on search_path.
• Removing test/sql/schema.sql outright (rather than keeping it around half-redundant) is the right call — its unique properties (off-search_path proof, quoting-forced targeting, qualified cross-schema calls) are now exercised by every fresh/update test run plus the remaining files' explicit qualification, so keeping it would have been pure duplication.

I wasn't able to execute make test/make lint in this sandbox (no live Postgres cluster reachable), so this review is based on static reading of the diff and the expected-output files, not a live run.

[github-actions]

Claude finished @jnasbyupgrade's task in 2m 27s —— View job

---

Claude finished the re-review

• Gather context (diff vs origin/master, commits since last review)
• Check whether the previously-flagged bin/test_existing issue was addressed
• Re-check test/install/load.sql, test/deps.sql, test/finish.sql, test/sql/*.sql, CLAUDE.md, expected outputs
• Post updated review

Summary

Since the last review pass, two new commits landed (9175bbf, then a merge of master: 05beaf1/a5e529a). 9175bbf (the existing-mode skip in test/finish.sql) was already covered in the previous review round. The master-merge commits are unrelated catch-up (CI scaffolding, doc-path fix) and don't touch this PR's actual diff — git diff origin/master...HEAD is still the same 12 files / +213/-146 described in the PR body, and I re-read every one of them plus CLAUDE.md's updated section.

The design is unchanged and still matches the CLAUDE.md guidance: fresh/update installs target a freshly created, randomly named, quoting-forcing schema via CREATE EXTENSION ... SCHEMA; the schema is deliberately never added to search_path; test/deps.sql rediscovers it per-session via the pg_namespace/pg_extension join (avoiding the regnamespace quoting trap); test/finish.sql asserts non-reachability via search_path at the end of every file (skipped in existing mode via _1.out alternates); test/sql/simple.sql/dependency_guard.sql qualify their own calls explicitly. test/sql/schema.sql removal is justified — its assertions are all covered elsewhere now.

Both previously-flagged correctness issues (unescaped _ in the cleanup LIKE pattern; search_path ordering) are resolved, as noted in the prior pass.

Outstanding from the last review (still unresolved)

bin/test_existing still references the deleted test/sql/schema.sql — bin/test_existing:59-76,97-103,244

This PR's diff doesn't touch bin/test_existing at all, so the EXISTING_MODE_EXCLUDE="schema" variable and ~20 lines of comments explaining why schema.sql needs excluding from existing-mode runs are still present, even though test/sql/schema.sql no longer exists (removed in 008bbc3). There's also a stale comment at line 244 referencing "the schema.sql conflict above". Functionally harmless — existing_mode_regress() derives its file list from disk, so the exclude just matches nothing now — but it's dead configuration with inaccurate rationale, the same kind of stale reference 7cb765f already cleaned up in CLAUDE.md/load.sql/finish.sql.

Fix this →

Other observations (unchanged, non-blocking)

• extension_drop_test still implicitly rides along into the random schema via simple.sql's unqualified CREATE EXTENSION extension_drop_test, even though the PR description frames its randomization as deferred to Randomize the test extension's (extension_drop_test) name and install schema #37. Not a functional problem — just worth double-checking that framing still matches intent.

Nice touches

• CLAUDE.md's rewritten section accurately reflects the final implementation (random-schema-on-every-run, never on search_path, test/finish.sql's assertion) rather than the earlier TEST_SCHEMA-switch design it originally described.
• test/expected/simple_1.out / dependency_guard_1.out correctly model the existing-mode SKIP variant produced by test/finish.sql's conditional.
• Nothing in the new master-merge commits conflicts with or duplicates this PR's own schema/random-install logic.

I wasn't able to execute make test/make lint in this sandbox (no live Postgres cluster reachable), so this is based on static reading of the diff, matching the author's own repeated make test/make lint verification described in the PR body.

[github-actions]

Claude finished @jnasbyupgrade's task in 1m 34s —— View job

---

Claude finished the re-review

• Gather context (diff vs origin/master, commits since last review)
• Check whether the previously-flagged bin/test_existing issue was addressed
• Review any other changes since last review pass
• Post updated review

Summary

Only one new commit since the last review pass: dd13716 ("bin/test_existing: remove dead EXISTING_MODE_EXCLUDE and stale schema.sql references"). This is exactly the fix for the one outstanding issue from the previous review round.

Previously-flagged issue: fixed ✅

bin/test_existing referencing the deleted test/sql/schema.sql — dd13716 removes EXISTING_MODE_EXCLUDE="schema", the ~20 lines of comments justifying it, and the stale "schema.sql conflict above" reference in run_suite()'s comment block. existing_mode_regress() is simplified to unconditionally emit every test/sql/*.sql basename, since there's nothing left to exclude. Verified there are no remaining references to schema.sql or EXISTING_MODE_EXCLUDE anywhere in the repo (grep -r across the tree found none), and read through the full updated bin/test_existing — the trimmed comments in plant_guard's docblock and the GUARD_SCHEMA declaration read cleanly on their own without the removed cross-references. Commit message states this was verified against the real prepare/run-suite flow against a live database.

No new issues found in this pass. With this fix, all issues raised across the review history of this PR are resolved:

• Unescaped _ in the cleanup LIKE pattern (fixed via ~ regex).
• search_path ordering concern (moot — schema is now never added to search_path).
• bin/test_existing stale schema.sql references (fixed in dd13716).

The one remaining item is an observation, not a bug: extension_drop_test still implicitly rides along into the random schema via simple.sql's unqualified CREATE EXTENSION extension_drop_test, even though the PR description frames that extension's randomization as deliberately deferred to #37. Worth a final sanity check that this framing still matches intent, but not something blocking this PR.

I wasn't able to execute make test/make lint in this sandbox (no live Postgres cluster reachable), so this is based on static reading of the diff, matching the author's own repeated make test/make lint verification described in the PR body.