Remove redundant client_min_messages handling from install script

[jnasbyupgrade]

CREATE EXTENSION already raises client_min_messages to WARNING for the install script (only-raising, so a stricter caller is respected) and restores it afterward — so the script's own setting was redundant, and being blind it actually lowered a stricter caller's level during install. Remove it.

[jnasbyupgrade]

@claude please review this PR.

[claude]

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

---

Review: Don't clobber caller's client_min_messages on install

• Gather context (read diff, repo files, CLAUDE.md)
• Understand the request (code review only)
• Review sql/count_nulls.sql changes for correctness/security/style
• Post review feedback

Overall the approach is sound: save the caller's client_min_messages into a transaction-local custom GUC, conditionally raise it to WARNING only if the caller's level is more verbose, and explicitly restore it at the end so the change doesn't leak past CREATE EXTENSION — including when it's run inside a larger explicit transaction (which a simple SET LOCAL wouldn't handle, since that only reverts at transaction end, not statement end). One issue should block merge, plus a couple of minor notes.

🔴 Breaks the extension's own declared/tested PostgreSQL 9.4 support

sql/count_nulls.sql:14-22 uses pg_catalog.array_position() to rank severity levels:

c_levels CONSTANT text[] := ARRAY[
  'debug5', 'debug4', 'debug3', 'debug2', 'debug1'
  , 'log', 'notice', 'warning', 'error'
];
...
IF pg_catalog.array_position(c_levels, pg_catalog.lower(v_current))
   < pg_catalog.array_position(c_levels, 'warning')

array_position() was only added in PostgreSQL 9.5. This repo's META.json declares "PostgreSQL": "9.4.0" as the minimum required version, and .travis.yml explicitly runs the test matrix against PGVERSION=9.4. On 9.4, CREATE EXTENSION count_nulls; would now fail with something like function pg_catalog.array_position(text[], unknown) does not exist, breaking install entirely on a version this project claims to (and tests to) support.

A 9.4-compatible replacement avoids needing ordering/array lookups at all, e.g.:

IF pg_catalog.lower(v_current) IN
   ('debug5','debug4','debug3','debug2','debug1','log','notice')
THEN
  SET client_min_messages = warning;
END IF;

Fix this →

🟡 Minor: c_levels omits fatal/panic

client_min_messages also legally accepts fatal and panic (rarely used, but valid). For those values, array_position returns NULL, so NULL < array_position(...,'warning') is NULL and the IF is skipped — which happens to be the correct behavior (don't lower an already-stricter setting), but only by accident of NULL-comparison semantics rather than by design. Worth adding them explicitly (or switching to the IN-list form above, which sidesteps the issue) so the correctness doesn't depend on that subtlety.

🟡 Minor: same clobbering bug remains in the upgrade scripts

sql/count_nulls--0.9.0--0.9.2.sql:1, sql/count_nulls--0.9.2--0.9.5.sql:1, and sql/count_nulls--0.9.5--0.9.6.sql:1 all still have the original unguarded SET client_min_messages = WARNING;. That means ALTER EXTENSION count_nulls UPDATE; still clobbers the caller's session setting the same way CREATE EXTENSION used to. The PR title says "on install," so this may be intentionally out of scope, but it's the same bug via a different path and worth a follow-up if not addressed here.

🟢 Nit: no regression test added

The 34-line change has no accompanying test (e.g., asserting that client_min_messages set to error before CREATE EXTENSION is still error afterward). Not blocking, but would help guard this fix against regressions.

[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: 6b95b9fa-42cc-4680-8c0d-a30dbe496b6d

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[jnasbyupgrade]

@claude re-review pls

[claude]

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

---

Re-review: Remove redundant client_min_messages handling from install

• Gather context (diff, prior review, PG extension-loading behavior)
• Check whether the previously-flagged blocker still applies
• Re-review current state of sql/count_nulls.sql
• Post updated review feedback

Correction to my prior review: that review discussed a DO block using pg_catalog.array_position() at sql/count_nulls.sql:14-22 and flagged it as breaking PG 9.4 support. That code does not exist anywhere in this repo (verified via git show 06cc753 and a full-repo grep for array_position/client_min_messages). The actual diff for this PR is exactly what the title says — a 2-line deletion:

-SET client_min_messages = WARNING;
-
 CREATE OR REPLACE FUNCTION null_count(

That prior finding does not apply here; disregard it.

Review of the actual change

The claim in the PR body is correct: PostgreSQL's execute_extension_script() (backend/commands/extension.c, present since extensions were introduced in 9.1) raises client_min_messages to WARNING only when the current value is lower (more verbose), and restores the exact prior value via AtEOXact_GUC() at a dedicated GUC nesting level when the script finishes — not just at transaction end, so this is safe even inside an explicit multi-statement transaction. Given that, the script's own unconditional SET client_min_messages = WARNING; was:

• redundant when the caller already had a stricter (quieter) setting, and
• actively harmful when the caller had set something stricter than WARNING (e.g. ERROR) before running CREATE EXTENSION, since the unconditional SET would lower it for the remainder of the install script.

Removing it is correct and strictly simplifies the script by relying on core behavior instead of duplicating it.

Remaining minor notes (non-blocking, carried over from prior review)

• 🟡 sql/count_nulls--0.9.0--0.9.2.sql:1, sql/count_nulls--0.9.2--0.9.5.sql:1, and sql/count_nulls--0.9.5--0.9.6.sql:1 still have the same unconditional SET client_min_messages = WARNING;, so ALTER EXTENSION count_nulls UPDATE; still clobbers a caller's stricter setting. Same bug, different path — likely fine to leave as a follow-up since this PR is scoped to "on install," but worth a separate PR if you want it fully closed out.
• 🟢 No regression test was added (e.g., asserting client_min_messages set to error before CREATE EXTENSION remains error afterward). Not blocking given this now relies on core PG behavior rather than custom logic, but would still guard against a future accidental re-add of the redundant SET.

No blocking issues. This change looks correct and safe to merge as-is.