Skip to content

Remove redundant client_min_messages handling from install script#26

Open
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/client-min-messages
Open

Remove redundant client_min_messages handling from install script#26
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix/client-min-messages

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

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: a2250ea6-5c19-499a-a58d-dcae5594dfdd

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

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

@jnasbyupgrade jnasbyupgrade force-pushed the fix/client-min-messages branch from 1071470 to 7686117 Compare July 13, 2026 19:29
@jnasbyupgrade

Copy link
Copy Markdown
Contributor Author

@claude please review this PR.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


Claude finished the review

  • Gather context (PR diff, CLAUDE.md conventions, Makefile generation rules)
  • Understand the request (code review of sql/cat_tools.sql.in changes)
  • Review the guard logic and its interaction with CREATE EXTENSION/transaction semantics
  • Check repo conventions (generated .sql.in tracking rules)
  • Post review feedback

Review of sql/cat_tools.sql.in

The overall approach (save the caller's client_min_messages, only raise it to WARNING when the caller's setting is more verbose, restore afterward) is the right idea. Two issues found, one of which is significant:

1. Generated per-version file not regenerated (blocking)

Per this repo's own convention (CLAUDE.md, "SQL file conventions" rule 4), sql/cat_tools--0.2.2.sql.in is generated by make from sql/cat_tools.sql.in but must still be tracked. This PR only touches sql/cat_tools.sql.insql/cat_tools--0.2.2.sql.in still contains the old, unguarded SET LOCAL client_min_messages = WARNING; (verified by reading the file on this branch). Since PGXN packaging installs from the tracked --0.2.2.sql.in rather than running make first, the fix won't actually ship until this file is regenerated and committed.

Fix this →

2. The save/restore mechanism can still leak past install (the bug the PR is trying to fix)

sql/cat_tools.sql.in lines ~1-27 stash the caller's client_min_messages into a custom GUC (cat_tools.client_min_messages) via SET LOCAL (transaction-scoped, correct), then raise via a plain SET client_min_messages = warning; (non-local). At the end of the file (~line 1566), the restore does:

SELECT pg_catalog.set_config('client_min_messages', pg_catalog.current_setting('cat_tools.client_min_messages'), false);

false means non-local (session-level) SET.

Per Postgres GUC-stacking rules, when a plain (non-local) SET happens after a SET LOCAL within the same transaction level, the non-local SET wins and the value propagates past COMMIT. Concretely:

BEGIN;
SET LOCAL client_min_messages = error;   -- caller wants this only for this transaction
CREATE EXTENSION cat_tools;              -- our script sees 'error', skips raising (already >= warning)
                                          -- final restore does a *non-local* SET of 'error'
COMMIT;
-- client_min_messages is now permanently 'error' for the rest of the session,
-- instead of reverting to whatever it was before BEGIN.

This reproduces exactly the "leaking the change past install" problem the PR's own comment says it's avoiding — just for a caller-supplied SET LOCAL instead of our own raise.

Suggested simplification: since a DO block without an EXCEPTION clause doesn't open a subtransaction, a guarded SET LOCAL by itself is sufficient and needs no save/restore at all:

DO $$
DECLARE
  c_levels CONSTANT text[] := ARRAY['debug5','debug4','debug3','debug2','debug1','log','notice','warning','error'];
  v_current CONSTANT text := pg_catalog.current_setting('client_min_messages');
BEGIN
  IF pg_catalog.array_position(c_levels, pg_catalog.lower(v_current))
     < pg_catalog.array_position(c_levels, 'warning')
  THEN
    SET LOCAL client_min_messages = warning;
  END IF;
END
$$;

SET LOCAL is inherently transaction-scoped, so it automatically reverts to whatever was in effect (session-level or an ambient caller's SET LOCAL) once that transaction ends — correctly handling both the plain top-level CREATE EXTENSION case and the "runs inside a larger transaction" case this PR is explicitly trying to support, with no custom GUC, no explicit restore statement, and less code.

Fix this →

Minor

  • No test coverage was added for this behavior (e.g. a TAP test that runs CREATE EXTENSION inside an explicit transaction with a stricter client_min_messages and asserts it isn't clobbered, or is restored correctly after commit).
  • The rest of the logic (level ordering array, array_position comparison, LOG sorting between DEBUG1/NOTICE) is correct and matches documented client_min_messages semantics.

@jnasbyupgrade jnasbyupgrade force-pushed the fix/client-min-messages branch from 7686117 to 98b58e5 Compare July 14, 2026 20:28
@jnasbyupgrade jnasbyupgrade changed the title Don't clobber caller's client_min_messages on install Remove redundant client_min_messages handling from install script Jul 14, 2026
CREATE EXTENSION already forces client_min_messages to at least WARNING for
the install script (only raising it, so a stricter caller is respected) and
restores the caller's value afterward -- it has done so since extensions were
introduced in 9.1. The script's own setting was therefore redundant, and being
blind it lowered a stricter caller's level during install. Drop it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jnasbyupgrade jnasbyupgrade force-pushed the fix/client-min-messages branch from 98b58e5 to 8e5a7d7 Compare July 14, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant