Always install count_nulls into a fresh, randomly named schema for tests

[jnasbyupgrade]

Replace the two-value TEST_SCHEMA axis (fixed "" and "Quoted" legs, run via make test-schema-all's in-Makefile loop) with a single install per run into a freshly, randomly generated schema whose constant prefix (a literal trailing space) always requires SQL identifier quoting. This exercises %I-qualification on every run instead of only on a dedicated quoting leg, and removes the Makefile/GUC-propagation infrastructure that existed solely to support the two-value axis.

Cleanup-before-create matches on the constant prefix (count_nulls test schema %) to find and drop any schema left behind by a run that crashed before its own teardown, so stale schemas don't accumulate run over run. test/helpers/find_test_schema.sql lets separate sessions (bin/test_existing's per-step psql -f ... invocations, each a fresh connection) rediscover the randomly generated name without having created it themselves.

bin/test_existing's prepare-old/plant_guard/run-suite no longer take a schema argument, since every install always targets its own randomly generated schema now; create_extension_in_schema() moved to a proper -f script (bin/test_existing.sql/create_extension.sql) since it needs multiple statements including a \gset.

This supersedes PR #32's cross-schema approach, which is left open only for reference.

[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: c856bbc8-31bd-4819-b8b2-b78bffcac6e5

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 2m 7s —— View job

---

Claude finished reviewing this PR

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review of PR Always install count_nulls into a fresh, randomly named schema for tests #55)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

This is a solid, well-documented simplification: replacing the two-value TEST_SCHEMA axis with a single always-randomized, always-quoting-required schema per install, plus the cross-session rediscovery helper (test/helpers/find_test_schema.sql) needed once nothing propagates the schema name via a GUC anymore. I traced the \if/\elif/\endif nesting in test/install/load.sql (now \if existing … \else … \if update … \else … \endif \endif) and it's balanced and correct. The cleanup-before-create pattern (matching pg_namespace on the count_nulls test schema % prefix) is duplicated identically between test/install/load.sql and the new bin/test_existing.sql/create_extension.sql, which is fine since schemas are per-database and the two call sites never touch the same database concurrently.

Finding

Stale TEST_SCHEMA references left in test/core/functions.sql — this PR removes the TEST_SCHEMA GUC/Makefile concept everywhere else (Makefile, CI, README, extension_tests.sql), but three comments in test/core/functions.sql still refer to it as if it exists:

• Line 28: "At this phase there's no TEST_SCHEMA concept yet"
• Lines 79-80: "this suite may run against count_nulls installed in ANY schema (see TEST_SCHEMA in the Makefile)"
• Line 143: "Keeps this suite's output identical no matter which schema count_nulls is installed in (see TEST_SCHEMA)"

These now point at a Makefile variable that no longer exists, which will confuse future readers following the "see TEST_SCHEMA" pointers. Worth a follow-up comment update (e.g. "see 'Schema targeting' in test/README.md" or similar) to match the terminology the rest of the PR standardized on.

Fix this →

I did not find any other dangling references to TEST_SCHEMA/test-schema-all in the repo, and the Makefile/CI/README changes are internally consistent with each other.

[github-actions]

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

---

Reviewing PR #55

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review via /code-review)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

This is a clean simplification: a single, always-randomized, always-quoting-required install schema replaces the two-value TEST_SCHEMA axis, plus test/helpers/find_test_schema.sql for cross-session rediscovery now that nothing propagates the schema name via a GUC. I re-checked the whole diff against origin/master (not just the files touched by the latest commit):

• test/install/load.sql's \if/\else/\endif nesting (existing-mode check wrapping the fresh/update split) is balanced and correct.
• The cleanup-before-create block (pg_namespace matched on the count_nulls test schema % prefix) is duplicated identically in test/install/load.sql and bin/test_existing.sql/create_extension.sql — fine, since schemas are per-database and the two call sites never share a database concurrently.
• bin/test_existing's subcommands (prepare-old/update/run-suite) and ci.yml's calls into them are consistent with each other now that the schema argument is gone (bin/test_existing prepare-old upgrade_oldest_first 0.9.6, etc.).
• Makefile, test/README.md, and ci.yml's own comments are internally consistent with the new "Schema targeting" terminology.
• Repo-wide grep for TEST_SCHEMA/test-schema-all turns up nothing left dangling except one deliberate holdout in test/core/functions.sql:28 ("At this phase there's no TEST_SCHEMA concept yet"), which the latest commit (e5d3ec9) explains is accurate historical scoping for that specific teardown__ function's own reasoning, not a claim about the current codebase — a defensible call, distinct from the two pointer-comments it did fix (which pointed readers at a Makefile variable that no longer exists).

No new issues found. The prior review's finding (stale TEST_SCHEMA comment pointers) was addressed by e5d3ec9.

[github-actions]

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

---

Reviewing PR #55

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review via /code-review)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

Re-reviewed the full diff against origin/master (all 5 commits, not just the latest). This is a clean, well-reasoned simplification: replacing the two-value TEST_SCHEMA axis with a single always-randomized, always-quoting-required install schema, plus test/helpers/find_test_schema.sql for cross-session rediscovery now that nothing propagates the schema name via a GUC.

Specific things I traced end-to-end and confirmed correct:

• test/install/load.sql's \if existing … \else … \if update … \else … \endif \endif nesting is balanced, and both branches correctly \set version before \i test/helpers/create_test_schema.sql.
• test/helpers/create_test_schema.sql's cleanup-before-create (pg_namespace matched on count_nulls test schema %) and its :"schema":version_clause interpolation into CREATE EXTENSION ... WITH SCHEMA both produce correct SQL for empty and non-empty :version.
• test/helpers/find_test_schema.sql's SELECT ... INTO STRICT uniqueness check followed by a separate plain SELECT ... \gset is a deliberate, correct workaround for \gset not working after a DO block.
• test/sql/extension_tests.sql's test__shutdown__drop_all now captures s CONSTANT name = ncs() before DROP EXTENSION — correctly avoiding the bug of calling ncs() (which queries pg_extension) after the extension is already gone.
• bin/test_existing's prepare-old/update/run-suite signatures, create_extension_in_schema(), and ci.yml's calls into them (bin/test_existing prepare-old upgrade_oldest_first 0.9.6, etc.) are all consistent with the schema argument being dropped.
• Repo-wide grep for TEST_SCHEMA/test_schema/test-schema-all turns up nothing dangling except the deliberate holdout below — everything else is the new create_test_schema.sql/find_test_schema.sql/:test_schema naming.

Minor nit (not blocking)

test/core/functions.sql:28's comment — "At this phase there's no TEST_SCHEMA concept yet" — is the one remaining reference to the removed TEST_SCHEMA term anywhere in the repo. Earlier review passes on this same PR debated this exact line and concluded it's deliberate historical scoping for why this particular teardown__ function can use a static literal instead of capturing search_path at runtime, distinct from the two pointer-comments that were fixed (which pointed at a Makefile variable). I don't disagree with that reasoning, but since TEST_SCHEMA is no longer defined anywhere else in the codebase, a reader landing on this comment cold has no anchor for the term. Purely optional — e.g. rewording to "count_nulls' own install schema isn't relevant yet at this phase" would sidestep the issue without needing to reference a removed concept at all. Given this has already been discussed twice, I'm flagging it as an FYI rather than re-requesting a change — happy to leave as-is if that's still the call.

No other issues found. Nice use of ncs()-before-DROP EXTENSION ordering and the INTO STRICT pattern.

[github-actions]

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

---

Reviewing PR #55

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review via /code-review)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

Focused this pass on the four commits pushed since the last review (923cd2f, 1fcc42c, 2789f70, bada5ba), which factor the shared schema-creation logic out of test/install/load.sql into the new test/helpers/create_test_schema.sql (used by both test/install/load.sql and bin/test_existing's prepare-old), and tighten :version handling to require an explicit 'current' sentinel instead of treating empty as a valid signal.

Things I traced and confirmed correct:

• test/helpers/create_test_schema.sql's SET count_nulls.test_schema_version = :'version' + current_setting(...) = '' guard correctly catches both an explicitly-empty :version and a completely unset one (psql substitutes an unset :'var' with an empty string + a stderr warning), so the "fails loudly instead of silently installing current" goal in the commit message actually holds for the case it's meant to guard against.
• test/install/load.sql's fresh leg now passes 'current' and its update leg still passes '0.9.6', both handled correctly by the new version_clause CASE.
• test/helpers/find_test_schema.sql's revert back to hand-counted RAISE EXCEPTION (undoing the INTO STRICT refactor from 2789f70) is justified by the same commit's own reasoning: a DO block can't populate a psql variable, so the separate SELECT ... \gset is unavoidable either way, and INTO STRICT didn't actually remove a statement.
• bin/test_existing's create_extension_in_schema() now points -f at test/helpers/create_test_schema.sql directly, consistent with bin/test_existing.sql/create_extension.sql being deleted as fully superseded.

Finding: stale test/install/load.sql pointers left by the create_test_schema.sql extraction

1fcc42c moved the cleanup-before-create + random-name + install logic out of test/install/load.sql into the new shared test/helpers/create_test_schema.sql, but a few comments that used to correctly point at load.sql (back when it owned this logic) weren't updated, and now point readers at the wrong file for call sites that never touch load.sql at all:

• bin/test_existing:38 — the prepare-old usage doc says the random schema is created "see test/install/load.sql", but prepare-old → create_extension_in_schema() calls test/helpers/create_test_schema.sql directly and never invokes load.sql.
• bin/test_existing.sql/plant_guard.sql:12 — same issue: "count_nulls always installs into its own randomly generated schema (see test/install/load.sql)", but plant_guard.sql runs right after prepare-old's direct create_test_schema.sql call, again never going through load.sql.
• test/README.md's "Layout" section (lines 9-33) lists helpers/find_test_schema.sql but has no entry for the new helpers/create_test_schema.sql, even though it's now a substantial (78-line), independently-documented file shared by two call sites. The "Schema targeting" section (lines 35-53) still attributes the random-name generation and cleanup-before-create behavior directly to install/load.sql ("install/load.sql always installs...", "before generating a new name, install/load.sql finds and drops..."), which is only true for the load.sql-mediated path now, not for bin/test_existing`'s.

(By contrast, the test/install/load.sql-pointers in test/core/functions.sql:294, test/sql/extension_tests.sql:10,64, and ci.yml:251 are all still accurate — those describe the pgTAP suite's own execution context, which genuinely is only ever reached via load.sql's fresh/update flow.)

Not a functional bug, but the same class of dangling-pointer issue flagged (and fixed) earlier in this PR for the TEST_SCHEMA comments - worth a follow-up pass now that create_test_schema.sql is the actual owner of this logic for two independent callers.

Fix this →

No other issues found in the new commits - ci.yml's 923cd2f rewording is accurate as a general "during testing" statement, and the bada5ba :version tightening is sound.
(branch: random-test-schema)

[github-actions]

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

---

Reviewing PR #55

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review via /code-review)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

This pass focuses on the one new commit since the last review, 11847e5 ("drop the redundant schema cleanup in test__shutdown__drop_all"), plus a recheck of whether prior findings were addressed.

11847e5 is correct as far as the test/sql/extension_tests.sql change itself goes: test/helpers/create_test_schema.sql unconditionally drops any leftover count_nulls test schema % schema before creating a fresh one (confirmed at test/helpers/create_test_schema.sql:55), so the explicit DROP SCHEMA %I this function also did was genuinely redundant, and test/expected/extension_tests.out:84 still expects exactly one ok row for this function (an EXECUTE'd DROP SCHEMA never produced its own TAP row, so removing it doesn't change expected output).

Finding: test/README.md's "shutdown drop is plain cleanup" section is now stale

test/README.md:81-87 still describes test__shutdown__drop_all as doing its own schema drop:

test__shutdown__drop_all's schema drop is plain cleanup, not a TAP assertion. Dropping the schema count_nulls was installed into isn't something this suite is testing, just tearing down what install/load.sql created - so it's plain EXECUTE'd SQL with no lives_ok()/skip() branch. ...

11847e5 removed that DROP SCHEMA entirely — the function now only asserts DROP EXTENSION count_nulls succeeds (test/sql/extension_tests.sql:58-64) and no longer touches the schema or calls ncs(). This whole paragraph should be deleted or rewritten to explain the current no-op-cleanup rationale (which is actually documented well in the new comment block directly above the function at test/sql/extension_tests.sql:47-57 — the README section could just be trimmed to a shorter pointer at that comment instead of duplicating stale detail).

Fix this →

Still open from the previous review pass

The stale "see test/install/load.sql" pointers flagged in the prior review (for logic that 1fcc42c moved into the shared test/helpers/create_test_schema.sql) are still present and unaddressed by this round of commits:

• bin/test_existing:38 — prepare-old's usage doc.
• bin/test_existing.sql/plant_guard.sql:12.
• test/README.md's Layout section (no entry for helpers/create_test_schema.sql) and Schema targeting section (lines 37, 47, 52 still attribute the random-name/cleanup-before-create logic to install/load.sql instead of create_test_schema.sql).

Repeating the same fix pointer from before in case it's still queued: Fix this →

The already-discussed test/core/functions.sql:28 TEST_SCHEMA holdout is unchanged and still stands as the deliberate, previously-accepted call from earlier passes — not re-flagging it again.

No other issues found in 11847e5.
(branch: random-test-schema)

[github-actions]

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

---

Reviewing PR #55

• Gather context (PR diff, description, CLAUDE.md)
• Understand the request (code review via /code-review)
• Review changed files for correctness, security, maintainability
• Post final review feedback

Summary

This pass focuses on the one new commit since the last review, f176c8c ("Point stale test/install/load.sql comments at test/helpers/create_test_schema.sql"), and re-checks whether the previously open finding was resolved.

It is. All three stale pointers flagged in the prior review round are fixed:

• bin/test_existing:35-38 — prepare-old's usage doc now points at test/helpers/create_test_schema.sql instead of test/install/load.sql, correctly reflecting that prepare-old → create_extension_in_schema() never touches load.sql.
• bin/test_existing.sql/plant_guard.sql:9-14 — same fix, same reasoning.
• test/README.md — the Layout section now has an entry for helpers/create_test_schema.sql (lines 31-34), and the Schema targeting section (lines 41-61) correctly attributes random-name generation and cleanup-before-create to create_test_schema.sql, explicitly noting it's shared by both install/load.sql and bin/test_existing. The test__shutdown__drop_all paragraph (lines 89-95) is also updated to match 11847e5's removal of the explicit per-run schema drop, correctly describing it as now relying entirely on create_test_schema.sql's own cleanup-before-create.

I also re-verified this commit didn't touch (and didn't need to touch) the other test/install/load.sql references repo-wide — test/core/functions.sql:28,294, test/sql/extension_tests.sql:10,30, bin/test_existing:6,146, test/README.md:11-18 — these all still correctly describe the pgTAP suite's own execution context, which is genuinely only ever reached via load.sql's fresh/update flow, or (for bin/test_existing:146) correctly describe the shared implementation while still crediting load.sql as a co-user.

No new issues found. All findings from earlier review passes on this PR (the TEST_SCHEMA comment pointers, the load.sql pointers, and the README test__shutdown__drop_all staleness) are now resolved. The one previously-flagged FYI (test/core/functions.sql:28's TEST_SCHEMA wording) remains an accepted, deliberate call from earlier discussion and isn't being re-raised.