Keep install and update scripts' settings changes from leaking to the caller by jnasbyupgrade · Pull Request #11 · Postgres-Extensions/trunklet

jnasbyupgrade · 2026-10-27T20:48:11Z

The install and update scripts change settings only with SET LOCAL, so once the transaction running CREATE EXTENSION trunklet or ALTER EXTENSION trunklet UPDATE ends, the caller's search_path and client_min_messages are unchanged. Within that transaction search_path is always unchanged, and so is client_min_messages except for a caller at error, who sees warning until the transaction ends when installing 0.2.0 or when updating from 0.2.0 or 0.2.1 (the 0.2.1--0.3.0 step): those two scripts have no set-back. The test install now fails the suite if either one leaves any user-settable setting changed. This is stacked on #10, whose update helper schema-qualifies everything after the update to work around the leak.

Why SET LOCAL is enough: before an extension script runs, the server already sets search_path, and raises client_min_messages to warning when the caller is below it, and it restores the caller's values when the script ends. A plain SET in the script overrides that restore and SET LOCAL does not, so the scripts' existing save-and-set-back (which saves the server's values, not the caller's) is kept unchanged apart from LOCAL. The server saves client_min_messages only when the caller is below warning; for a caller at error, a script's SET LOCAL of it lasts until the end of the enclosing transaction unless the script sets it back, which is the exception above.

Changes

SET becomes SET LOCAL, and nothing else in these files changes apart from a comment, in each already-released script, that it shipped with plain SET:
- sql/trunklet.sql: client_min_messages and the SET %I = %L set-back.
- sql/trunklet--0.2.0.sql: client_min_messages and search_path (this script has no set-back).
- sql/trunklet--0.2.1.sql: client_min_messages, search_path and the set-back.
- sql/trunklet--0.3.3.sql: client_min_messages and the set-back.
- sql/trunklet--0.2.0--0.2.1.sql: client_min_messages, search_path and the set-back. It is new since 0.3.3, so it has no comment.
- sql/trunklet--0.2.1--0.3.0.sql: client_min_messages and search_path (this script has no set-back).
- sql/trunklet--0.3.2--0.3.3.sql: client_min_messages and the set-back.
The released install scripts sql/trunklet--0.2.0.sql, sql/trunklet--0.2.1.sql and sql/trunklet--0.3.3.sql therefore no longer match their release branches exactly. RELEASE.md records that difference for its drift check, and notes that sql/trunklet--0.2.1--0.3.0.sql and sql/trunklet--0.3.2--0.3.3.sql also differ from what shipped.
test/helpers/settings_snapshot.sql is a new shared helper. pg_temp.settings_snapshot() records every pg_settings row with context user or superuser (the only ones SET can change in-session), and pg_temp.settings_check(action) raises an error naming each setting that has since changed, with its before and after value. Settings that first appear after the snapshot are ignored, because loading a library can define new ones.
test/install/load.sql sets client_min_messages to notice (below warning, so the server saves it and a plain SET would show; a caller at error or higher is not exercised), snapshots, and checks after CREATE EXTENSION. The CREATE and its check run in one transaction. An error in load.sql doesn't fail make by itself (pgxntool's .IGNORE: installcheck), so a failed check rolls back the install, and the downstream suite then fails on every PostgreSQL version.
test/helpers/update_with_data.sql drops the schema qualification and runs the same two functions around ALTER EXTENSION UPDATE. A failed check there stops load.sql before the helper drops its _trunklet_test schema, and the suite then fails on that schema already existing.
test/build/build.sql replaces its search_path assertion with the same helper: it snapshots before CREATE EXTENSION and asserts pg_temp.settings_check('CREATE EXTENSION') under pgTap lives_ok. Unlike load.sql, the build test's output is compared, so a not ok fails it. Adding SET work_mem = '7MB'; to sql/trunklet.sql turns ok 2 into not ok 2 - CREATE EXTENSION changed no setting.
test/sql/search_path.sql and test/expected/search_path.out are removed; load.sql covers the fresh CREATE EXTENSION.
HISTORY.asc notes the change.

Verification CI doesn't cover

Each of these mutations, adding SET work_mem = '7MB';, makes make exit non-zero with the check's error in load.out:

In sql/trunklet.sql: make test and make verify-results on PG17, and make test on PG12 (CREATE EXTENSION changed settings).
In sql/trunklet--0.3.2--0.3.3.sql: make test-update on PG17 (ALTER EXTENSION UPDATE changed settings).
In sql/trunklet--0.2.0.sql: make test-update on PG17 (CREATE EXTENSION changed settings).

With the BEGIN/COMMIT removed, the sql/trunklet.sql mutation still reports the error in load.out, but make test exits 0.

CREATE EXTENSION and ALTER EXTENSION UPDATE left the caller's client_min_messages at warning, and updating from 0.2.0 or 0.2.1 also changed search_path. Remove the settings save/restore and the SETs from sql/trunklet.sql and the 0.2.0--0.2.1, 0.2.1--0.3.0 and 0.3.2--0.3.3 update scripts. The suite now asserts both settings are unchanged after a fresh install and after an update, and the update helper no longer needs to schema-qualify names. test/sql/build.sql loads the raw script with \i, so it sets client_min_messages itself.

coderabbitai · 2026-10-27T20:48:20Z

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

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 404b020d-252c-413e-8164-cd663d739fb1

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

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Check client_min_messages around CREATE EXTENSION in search_path.sql rather than load.sql, and reword the HISTORY entry to match the observed settings after each update and install script.

Restores the released update scripts' settings save/restore, with each SET and the set-back made SET LOCAL so the caller's search_path and client_min_messages survive the script.

…nges

jnasbyupgrade · 2026-10-28T21:46:21Z

+ * client_min_messages first: TEST_UPDATE_FROM's own install script may have left
+ * it raised, which would hide an update script doing the same.
+ */
+SET client_min_messages = notice;


This is doing the same thing as what's in test/sql/search_path.sql; refactor them. And I'd say if we're going to do this we should actually validate all of pg_settings (uh, I guess can ignore things that would require server restart etc), not just search_path and client_min.

Claude here. Done in baccd69.

Shared helper: test/helpers/settings_snapshot.sql, used by both test/sql/search_path.sql and test/helpers/update_with_data.sql.

It compares every pg_settings row with context user or superuser, the only ones SET can change in-session, so restart and reload settings are skipped. Settings that first appear after the script are ignored, because loading a library can define new ones.

search_path.sql now has a single is_empty() assertion, so its output doesn't vary by PG version.

Mutations that confirmed it catches a setting outside the old two: adding SET work_mem = '7MB'; to sql/trunklet.sql fails search_path in fresh mode, and adding it to sql/trunklet--0.3.2--0.3.3.sql fails update-from-0.3.2 mode, on both PG17 and PG12.

test/helpers/settings_snapshot.sql snapshots every pg_settings row with context user or superuser and exposes a view of the ones that changed. search_path.sql and update_with_data.sql both use it, replacing their separate search_path/client_min_messages checks.

jnasbyupgrade

You seem to be using 2 different styles for the settings polution stuff; one with pgTap output and one without. It's confusing. I would prefer that we do all of it (including setup) with pgTap functions (ie, lives_ok()). I'm OK with some of it being raw SQL if that makes more sense, but I think the final check really should be pgTap.

Also, i'm not sure we actually need test/sql/search_path.sql anymore. Presumably we should just test this as part of load.sql (uh, which I guess if we're doing that it can't actually use pgTap). Given that we probably do want the UPDATE testing to verify this as well, and that that probably should use pgTap, maybe the right thing here is to have the helper define setup and then check functions in pg_temp. Or maybe 2 separete chunks of code that just refer to the view are OK. (Note that really nothing in the extension should be screwing with GUCs ever, so I think it's fine if we actually check the view some time after CREATE or ALTER).

jnasbyupgrade · 2026-10-28T22:50:44Z

+
 ALTER EXTENSION trunklet UPDATE;

+DO $body$


WTF you're STILL re-inventing wheels! Pick ONE way to validate settings are sane. And have the helper define a text-returning function that runs a pgTap test ensuring nothing changed (ie, maybe use bag_eq or results_eq). And I lean towards we don't really need to drop the temp objects.

Claude here. As of f562476 there is one check. test/helpers/settings_snapshot.sql defines pg_temp.settings_snapshot() and pg_temp.settings_check(action), and that pair runs around each CREATE EXTENSION in test/install/load.sql and around ALTER EXTENSION UPDATE here. It's plain SQL, not pgTap, because this file runs inside load.sql, before the suite loads pgTap. The temp objects are no longer dropped.

Claude here. The load-time check in load.sql and update_with_data.sql is plain SQL that raises an error rather than pgTap, because load.sql's output is never compared: a pgTap not ok there would fail nothing, and only an error that breaks the downstream tests fails the run. Where output is compared, in test/build/build.sql, the same helper now runs under pgTap lives_ok (91bb4f5), replacing its old search_path assertion, so there is one settings check.

jnasbyupgrade · 2026-10-28T22:52:43Z

-CREATE TEMP TABLE after AS
-  SELECT current_setting('search_path')
-;
+CREATE TEMP TABLE changed AS SELECT * FROM pg_temp.settings_changed;


Claude here. The copy froze the changed settings before this file reset search_path for pgTap, so that reset wouldn't show up as a change itself. As of f562476, test/sql/search_path.sql is gone, and the copy with it; test/install/load.sql checks the CREATE EXTENSION instead.

`test/install/load.sql` snapshots every user-settable setting and checks it after `CREATE EXTENSION`, and `test/helpers/update_with_data.sql` uses the same `pg_temp.settings_snapshot()`/`pg_temp.settings_check()` pair around `ALTER EXTENSION UPDATE`. `test/sql/search_path.sql` is removed. An error in `load.sql` fails the run only by breaking the downstream tests, so the `CREATE` and its check run in one transaction: a leak rolls back the install and the suite fails. The released 0.2.0, 0.2.1 and 0.3.3 install scripts now use `SET LOCAL`, including in the settings set-back, so installing them by version no longer leaks `search_path` or `client_min_messages`. Each released script changed from plain `SET` has a comment saying so.

jnasbyupgrade · 2026-10-29T20:46:50Z

Claude here, answering the review body above, as of f562476.

The check now lives in the load session, per your follow-up. test/install/load.sql checks the CREATE EXTENSION, and test/helpers/update_with_data.sql, which load.sql runs in update mode, checks the ALTER EXTENSION UPDATE. Both use the same two pg_temp functions from test/helpers/settings_snapshot.sql. They're plain SQL rather than pgTap because both run before the suite loads pgTap. test/sql/search_path.sql is removed.
The released install scripts (0.2.0, 0.2.1, 0.3.3) now use SET LOCAL, including the settings set-back in 0.2.1 and 0.3.3, and each has a comment saying it was released with plain SET. So CREATE EXTENSION ... VERSION at any of them passes the check too (make test-update TEST_UPDATE_FROM=<version>).
Of the shipped update scripts, 0.2.1--0.3.0 and 0.3.2--0.3.3 got the same comment. 0.2.0--0.2.1 is SET LOCAL too but has no comment: it's new since 0.3.3 and hasn't shipped.
An error in load.sql doesn't fail make by itself, because pgxntool's base.mk has .IGNORE: installcheck. It fails the run only by breaking the downstream tests. So the CREATE and its check run in one transaction: a leak rolls back the install, and the downstream test fails on every PG version. The ALTER check already failed the run that way: its error stops load.sql before update_with_data.sql drops its _trunklet_test schema.

The 0.2.0 install and the 0.2.1--0.3.0 update have no set-back, so a caller at error sees client_min_messages at warning until the transaction ends. State that in HISTORY.asc and the PR description, move the set-back "released as plain SET" comments above their statements, note the two shipped update scripts in RELEASE.md, and clarify two test comments.

test/build/build.sql now snapshots settings before CREATE EXTENSION and asserts via lives_ok that none changed, replacing the weaker search_path check, so the repo has one settings check.

…standard trunklet.sql and trunklet--0.2.0--0.2.1.sql no longer set client_min_messages. trunklet.sql now runs with search_path = pg_catalog, set back with SET LOCAL. The build test sets client_min_messages itself before \i-ing trunklet.sql.

jnasbyupgrade added 6 commits October 27, 2026 15:59

Merge update-testing into fix-script-guc-leak

728c099

Test client_min_messages across CREATE EXTENSION in search_path.sql

da7c42e

Check client_min_messages around CREATE EXTENSION in search_path.sql rather than load.sql, and reword the HISTORY entry to match the observed settings after each update and install script.

Merge update-testing (bafef65) into fix-script-guc-leak

7121f0d

Dedupe settings assertions; fix HISTORY wording

2414f09

Make update scripts' settings changes SET LOCAL

0eb911f

Restores the released update scripts' settings save/restore, with each SET and the set-back made SET LOCAL so the caller's search_path and client_min_messages survive the script.

Merge update-testing (da8dce5) into fix-script-guc-leak

f62a380

jnasbyupgrade changed the title ~~Stop install and update scripts changing caller settings~~ Keep update scripts' settings changes from leaking to the caller Oct 28, 2026

jnasbyupgrade added 2 commits October 28, 2026 13:21

Clarify HISTORY entry for the SET LOCAL change

4126ec2

Make HISTORY entry precise about which settings each install path cha…

e1a34af

…nges

jnasbyupgrade commented Oct 28, 2026

View reviewed changes

jnasbyupgrade added 2 commits October 28, 2026 16:47

Remove stale search_path comment from update test helper

54c25d1

jnasbyupgrade commented Oct 28, 2026

View reviewed changes

jnasbyupgrade changed the title ~~Keep update scripts' settings changes from leaking to the caller~~ Keep install and update scripts' settings changes from leaking to the caller Oct 29, 2026

jnasbyupgrade added 3 commits October 29, 2026 15:56

Check settings in the build test with the shared helper under pgTap

91bb4f5

test/build/build.sql now snapshots settings before CREATE EXTENSION and asserts via lives_ok that none changed, replacing the weaker search_path check, so the repo has one settings check.

Navigation Menu

Repository navigation

Keep install and update scripts' settings changes from leaking to the caller - #11

Keep install and update scripts' settings changes from leaking to the caller#11
jnasbyupgrade wants to merge 15 commits into
update-testingfrom
fix-script-guc-leak

jnasbyupgrade commented Oct 27, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 27, 2026 •

edited

Loading

Review skipped

Uh oh!

jnasbyupgrade Oct 28, 2026

Uh oh!

jnasbyupgrade Oct 28, 2026

Uh oh!

jnasbyupgrade left a comment

Uh oh!

jnasbyupgrade Oct 28, 2026

Uh oh!

jnasbyupgrade Oct 29, 2026

Uh oh!

jnasbyupgrade Oct 29, 2026

Uh oh!

jnasbyupgrade Oct 28, 2026

Uh oh!

jnasbyupgrade Oct 29, 2026

Uh oh!

jnasbyupgrade commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jnasbyupgrade commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Verification CI doesn't cover

Uh oh!

coderabbitai Bot commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

jnasbyupgrade Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade left a comment

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade commented Oct 29, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jnasbyupgrade commented Oct 27, 2026 •

edited

Loading

coderabbitai Bot commented Oct 27, 2026 •

edited

Loading