[jnasbyupgrade]

Install and update scripts now change settings only with SET LOCAL, so the caller's search_path and client_min_messages are unchanged once the transaction running CREATE EXTENSION trunklet or ALTER EXTENSION trunklet UPDATE ends. The test install fails the suite if a script leaves any user-settable setting changed. Stacked on #10, whose update helper schema-qualifies everything after the update to work around the leak.

Within the 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.

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, with LOCAL added (the two unreleased scripts use set_config(..., true), below). 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 the released scripts changes apart from a comment in each that it shipped with plain SET. The two unreleased scripts also drop client_min_messages:
  • sql/trunklet.sql: no longer sets client_min_messages. It now runs with SET LOCAL search_path = pg_catalog, pg_temp (without pg_temp, it would still be searched first for relations and types), saves only search_path, and sets it back with set_config(name, setting, true), which is SET LOCAL but reads the saved value as a list: SET LOCAL %I = %L would turn public, pg_temp into one quoted name. The member objects and schemas of a fresh CREATE EXTENSION are unchanged, and no reference needed qualifying.
  • 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: no longer sets client_min_messages; search_path and the set-back are changed the same way as in sql/trunklet.sql. 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 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, which runs in a BEGIN/COMMIT like load.sql's CREATE, so a SET LOCAL left by an update script is caught too. 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 runs the snapshot, CREATE EXTENSION, pg_temp.settings_check('CREATE EXTENSION') and DROP EXTENSION all under pgTap lives_ok. Unlike load.sql, the build test's output is compared, so a not ok fails it. It sets client_min_messages to warning immediately before \i sql/trunklet.sql, which runs outside CREATE EXTENSION. Adding SET work_mem = '7MB'; to sql/trunklet.sql turns ok 3 into not ok 3 - CREATE EXTENSION changed no setting. The \i sql/trunklet.sql outside CREATE EXTENSION writes its output to /dev/null, because the set-back's set_config() result row depends on the server's default search_path. Its first block also sets search_path = public, pg_temp and checks it is restored after the \i: the set-back is observable only under a bare \i, since CREATE/ALTER EXTENSION restore search_path afterwards, and the old SET LOCAL %I = %L set-back leaves "public, pg_temp" there (make test exits 2 with it).
• 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, including the unreleased scripts' settings handling.

Verification CI doesn't cover

Re-run at the current head, with SET work_mem = '7MB'; (or SET LOCAL work_mem = '7MB'; where noted) appended to a script, each makes the command exit non-zero:

• sql/trunklet.sql: make test on PG17 (SET and SET LOCAL; the build test reports not ok 3 - CREATE EXTENSION changed no setting) and on PG12 (SET).
• sql/trunklet--0.3.2--0.3.3.sql: make test-update on PG17 (SET and SET LOCAL; ALTER EXTENSION UPDATE changed settings) and on PG12 (SET).
• sql/trunklet--0.2.0.sql: make test-update on PG17 (SET).

make test, make test-update, and make test-update TEST_UPDATE_FROM=0.2.1 and =0.3.3 pass on PG17 (port 5417) and PG12 (port 5412). Installing the old and new sql/trunklet.sql into scratch databases gives identical pg_dump -s output on PG12 and on PG17 (apart from the random \restrict token). From a transaction with search_path = public, pg_temp, \i sql/trunklet.sql leaves public, pg_temp.

[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

• 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: 9dee137b-de33-4d3d-aa6a-5ac993d6499f

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[jnasbyupgrade]

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.

[jnasbyupgrade]

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.

[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]

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.

[jnasbyupgrade]

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.

[jnasbyupgrade]

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]

WTF? Why??

[jnasbyupgrade]

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.

[jnasbyupgrade]

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.