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

HISTORY.asc

@@ -6,6 +6,10 @@ STABLE
 `ALTER EXTENSION trunklet UPDATE` now works from 0.2.0. Previously there was no update path from 0.2.0.
 Updated installs now also match a fresh install: the `language__get()` debug message that a fresh 0.3.3 install has is added by `0.3.3--stable` and was retroactively added to the already-released `0.3.2--0.3.3` update script.
 
+=== Stop install and update scripts from leaking `search_path` and `client_min_messages`
+After the transaction running `CREATE EXTENSION trunklet` or `ALTER EXTENSION trunklet UPDATE` ends, the caller's `search_path` and `client_min_messages` are unchanged. Within it, `search_path` is always unchanged; `client_min_messages` is too, except that a caller at `error` 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).
+`trunklet.sql` and `0.2.0--0.2.1` do not set `client_min_messages`, and run with `search_path = pg_catalog, pg_temp` set with `SET LOCAL`.
+
 0.3.3
 -----
 === Add `ignore_missing_functions` agument to `template_language__remove()`

RELEASE.md

@@ -6,4 +6,4 @@ Follow [`../ai/RELEASE.md`](https://github.com/Postgres-Extensions/ai/blob/main/
 
 Each of those releases is a branch on `origin` named exactly the version; the repo has no tags before the first `make tag` release. Where step 1 says to compare against the tag, compare against that version's branch tip instead.
 
-`sql/trunklet--0.2.0.sql`, `sql/trunklet--0.2.1.sql` and `sql/trunklet--0.3.3.sql` were committed after those versions shipped, so step 1 will flag their late last-touch dates. That is the backfill exception it describes, not drift. Each matches `sql/trunklet.sql` on its version branch, except that the 0.3.3 copy adds pgxntool's `/* DO NOT EDIT - AUTO-GENERATED FILE */` header line.
+`sql/trunklet--0.2.0.sql`, `sql/trunklet--0.2.1.sql` and `sql/trunklet--0.3.3.sql` were committed after those versions shipped, so step 1 will flag their late last-touch dates. That is the backfill exception it describes, not drift. Each matches `sql/trunklet.sql` on its version branch except that its script-level `SET` commands, including any settings set-back, are now `SET LOCAL` and commented as released with plain `SET`, and the 0.3.3 copy also adds pgxntool's `/* DO NOT EDIT - AUTO-GENERATED FILE */` header line. `sql/trunklet--0.2.1--0.3.0.sql` and `sql/trunklet--0.3.2--0.3.3.sql` were also edited after release, so step 1 will flag them too, and their content intentionally differs from what shipped: both have their `SET` commands (and `0.3.2--0.3.3`'s set-back) changed to `SET LOCAL` with a comment, and `0.3.2--0.3.3` additionally gains the `language__get()` debug-message function that its release branch lacks.

sql/trunklet--0.2.0--0.2.1.sql

@@ -7,10 +7,9 @@ CREATE SCHEMA __trunklet;
 CREATE TABLE __trunklet.old_settings AS
   SELECT name, setting
     FROM pg_catalog.pg_settings
-    WHERE name IN ('client_min_messages', 'search_path')
+    WHERE name IN ('search_path')
 ;
-SET client_min_messages = warning;
-SET search_path = pg_catalog;
+SET LOCAL search_path = pg_catalog, pg_temp;
 
 CREATE OR REPLACE FUNCTION _trunklet.template__get(
   template_id _trunklet.template.template_id%TYPE
@@ -263,13 +262,8 @@ $body$;
 
 
 
-SELECT _trunklet.exec(
-    format(
-      'SET %I = %L'
-      , name
-      , setting
-    )
-  )
+-- Use set_config(), not SET LOCAL %I = %L: the latter turns a list setting into one quoted name
+SELECT pg_catalog.set_config(name, setting, true)
   FROM __trunklet.old_settings
 ;
 DROP TABLE __trunklet.old_settings;

sql/trunklet--0.2.0.sql

@@ -4,10 +4,11 @@
  *
  */
 
-SET client_min_messages = warning;
+-- Both released as plain SET, which leaks the setting to the caller
+SET LOCAL client_min_messages = warning;
 
 -- Set a safe search_path
-SET search_path = pg_catalog;
+SET LOCAL search_path = pg_catalog;
 
 DO $do$
 BEGIN

sql/trunklet--0.2.1--0.3.0.sql

@@ -9,8 +9,9 @@ CREATE TABLE __trunklet.old_settings AS
     FROM pg_catalog.pg_settings
     WHERE name IN ('client_min_messages', 'search_path')
 ;
-SET client_min_messages = warning;
-SET search_path = pg_catalog;
+-- Released as plain SET, which leaks both settings to the caller
+SET LOCAL client_min_messages = warning;
+SET LOCAL search_path = pg_catalog;
 
 CREATE OR REPLACE FUNCTION _trunklet.language__get_loose(
   language_id _trunklet.language.language_id%TYPE

sql/trunklet--0.2.1.sql

@@ -15,8 +15,9 @@ CREATE TABLE __trunklet.old_settings AS
     FROM pg_catalog.pg_settings
     WHERE name IN ('client_min_messages', 'search_path')
 ;
-SET client_min_messages = warning;
-SET search_path = pg_catalog;
+-- Released as plain SET, which leaks the setting to the caller
+SET LOCAL client_min_messages = warning;
+SET LOCAL search_path = pg_catalog;
 
 DO $do$
 BEGIN
@@ -805,9 +806,10 @@ $body$;
 
 
 
+-- Released as plain SET, which leaks the setting to the caller
 SELECT _trunklet.exec(
     format(
-      'SET %I = %L'
+      'SET LOCAL %I = %L'
       , name
       , setting
     )

sql/trunklet--0.3.2--0.3.3.sql

@@ -9,7 +9,8 @@ CREATE TABLE __trunklet.old_settings AS
     FROM pg_catalog.pg_settings
     WHERE name IN ('client_min_messages')-- This doesn't actually work as expected :( , 'search_path')
 ;
-SET client_min_messages = warning;
+-- Released as plain SET, which leaks the setting to the caller
+SET LOCAL client_min_messages = warning;
 
 CREATE OR REPLACE FUNCTION _trunklet.language__get(
   language_name _trunklet.language.language_name%TYPE
@@ -183,9 +184,10 @@ REVOKE ALL ON FUNCTION trunklet.template_language__remove(
 
 
 
+-- Released as plain SET, which leaks the setting to the caller
 SELECT _trunklet.exec(
     format(
-      'SET %I = %L'
+      'SET LOCAL %I = %L'
       , name
       , setting
     )

sql/trunklet--0.3.3.sql

@@ -16,7 +16,8 @@ CREATE TABLE __trunklet.old_settings AS
     FROM pg_catalog.pg_settings
     WHERE name IN ('client_min_messages')-- This doesn't actually work as expected :( , 'search_path')
 ;
-SET client_min_messages = warning;
+-- Released as plain SET, which leaks the setting to the caller
+SET LOCAL client_min_messages = warning;
 -- PG apparently truncates the path to a single element, so our reset doesn't work :( SET LOCAL search_path = pg_catalog;
 
 DO $do$
@@ -959,9 +960,10 @@ $body$;
 */
 
 
+-- Released as plain SET, which leaks the setting to the caller
 SELECT _trunklet.exec(
     format(
-      'SET %I = %L'
+      'SET LOCAL %I = %L'
       , name
       , setting
     )

sql/trunklet.sql

@@ -13,10 +13,9 @@ CREATE SCHEMA __trunklet;
 CREATE TABLE __trunklet.old_settings AS
   SELECT name, setting
     FROM pg_catalog.pg_settings
-    WHERE name IN ('client_min_messages')-- This doesn't actually work as expected :( , 'search_path')
+    WHERE name IN ('search_path')
 ;
-SET client_min_messages = warning;
--- PG apparently truncates the path to a single element, so our reset doesn't work :( SET LOCAL search_path = pg_catalog;
+SET LOCAL search_path = pg_catalog, pg_temp;
 
 DO $do$
 BEGIN
@@ -958,13 +957,8 @@ $body$;
 */
 
 
-SELECT _trunklet.exec(
-    format(
-      'SET %I = %L'
-      , name
-      , setting
-    )
-  )
+-- Use set_config(), not SET LOCAL %I = %L: the latter turns a list setting into one quoted name
+SELECT pg_catalog.set_config(name, setting, true)
   FROM __trunklet.old_settings
 ;
 DROP TABLE __trunklet.old_settings;

test/build/build.sql

@@ -4,23 +4,36 @@
 \i test/helpers/psql.sql
 
 BEGIN;
+SET LOCAL client_min_messages = warning;
+SET LOCAL search_path = public, pg_temp;
+-- Discards the set-back's set_config() result row, which depends on the server's default search_path
+\o /dev/null
 \i sql/trunklet.sql
+\o
+-- A bare \i is the only place the set-back is visible; it must restore the list, not one quoted name
+\t on
+SELECT current_setting('search_path');
+\t off
 ROLLBACK;
 
 BEGIN;
 \i test/helpers/tap_setup.sql
+\i test/helpers/settings_snapshot.sql
+SELECT plan(5);
 
-SELECT plan(4);
+SELECT lives_ok(
+    $$SELECT pg_temp.settings_snapshot()$$
+    , 'settings_snapshot() saved the caller''s settings'
+);
 
 SELECT lives_ok(
     'CREATE EXTENSION trunklet;'
     , 'CREATE EXTENSION trunklet;'
 );
 
-SELECT isnt(
-    current_setting('search_path')
-    , 'pg_catalog'
-    , 'Verify search_path is back to something sane after extension creation.'
+SELECT lives_ok(
+    $$SELECT pg_temp.settings_check('CREATE EXTENSION')$$
+    , 'CREATE EXTENSION changed no setting'
 );
 SELECT hasnt_schema('__trunklet');
 

test/build/expected/build.out

@@ -1,11 +1,9 @@
 \set ECHO none
- exec 
-------
-
-(1 row)
+ public, pg_temp
 
-1..4
-ok 1 - CREATE EXTENSION trunklet;
-ok 2 - Verify search_path is back to something sane after extension creation.
-ok 3 - Schema __trunklet should not exist
-ok 4 - DROP EXTENSION trunklet;
+1..5
+ok 1 - settings_snapshot() saved the caller's settings
+ok 2 - CREATE EXTENSION trunklet;
+ok 3 - CREATE EXTENSION changed no setting
+ok 4 - Schema __trunklet should not exist
+ok 5 - DROP EXTENSION trunklet;

test/helpers/settings_snapshot.sql

@@ -0,0 +1,35 @@
+/*
+ * pg_temp.settings_snapshot() records every user-settable setting (pg_settings
+ * context 'user' or 'superuser', the only ones SET can change in-session),
+ * replacing any earlier snapshot. pg_temp.settings_check(action) raises an
+ * error naming action if any snapshotted setting has since changed or no longer
+ * exists. Settings that first appear later are ignored, since loading a library
+ * can define new ones.
+ */
+CREATE TEMP TABLE settings_before(name text PRIMARY KEY, setting text);
+
+CREATE FUNCTION pg_temp.settings_snapshot() RETURNS void LANGUAGE sql AS $body$
+DELETE FROM pg_temp.settings_before;
+INSERT INTO pg_temp.settings_before
+  SELECT name, setting FROM pg_catalog.pg_settings WHERE context IN ('user', 'superuser')
+;
+$body$;
+
+CREATE FUNCTION pg_temp.settings_check(action text) RETURNS void LANGUAGE plpgsql AS $body$
+DECLARE
+  c_changed CONSTANT text := (
+    SELECT string_agg(format('%s: %L -> %L', b.name, b.setting, s.setting), ', ' ORDER BY b.name)
+      FROM pg_temp.settings_before b
+        LEFT JOIN pg_catalog.pg_settings s USING (name)
+      WHERE s.setting IS DISTINCT FROM b.setting
+  );
+BEGIN
+  IF c_changed IS NOT NULL THEN
+    RAISE EXCEPTION '% changed settings', action
+      USING DETAIL = c_changed
+    ;
+  END IF;
+END
+$body$;
+
+-- vi: expandtab ts=2 sw=2

test/helpers/update_with_data.sql

@@ -1,10 +1,12 @@
 /*
  * Seeds a test language and a test template under TEST_UPDATE_FROM, runs ALTER
- * EXTENSION UPDATE, and checks the version and that the template still
- * processes. It then removes the language and checks that nothing is left
- * behind, so the suite's expected output matches fresh mode.
+ * EXTENSION UPDATE, and checks that the update changed no setting, that the
+ * extension is at the expected version, and that the template still processes.
+ * It then removes the language and checks that nothing is left behind, so the
+ * suite's expected output matches fresh mode.
  *
- * test/install/load.sql runs it in update mode.
+ * test/install/load.sql runs it in update mode, after loading
+ * test/helpers/settings_snapshot.sql.
  */
 CREATE SCHEMA _trunklet_test;
 SET search_path = _trunklet_test;
@@ -13,7 +15,12 @@ SET search_path = _trunklet_test;
 -- Committed before the update, as stored data would be in production
 CREATE TABLE seeded_template_ids AS SELECT get_test_templates() AS ids;
 
+SELECT pg_temp.settings_snapshot();
+-- One transaction, so SET LOCAL leaks from the script are caught (as in load.sql's CREATE)
+BEGIN;
 ALTER EXTENSION trunklet UPDATE;
+SELECT pg_temp.settings_check('ALTER EXTENSION UPDATE');
+COMMIT;
 
 DO $body$
 DECLARE
@@ -30,11 +37,9 @@ BEGIN
 END
 $body$;
 
--- Updating from before 0.3.0 changes search_path, so qualify from here on
-
 DO $body$
 DECLARE
-  c_ids CONSTANT int[] := (SELECT ids FROM _trunklet_test.seeded_template_ids);
+  c_ids CONSTANT int[] := (SELECT ids FROM seeded_template_ids);
   c_parameters CONSTANT text[] := '{}';
   r record;
 BEGIN
@@ -75,7 +80,7 @@ END
 $body$;
 
 -- The language's functions were created by TEST_UPDATE_FROM's code
-SELECT trunklet.template_language__remove(_trunklet_test.get_test_language_name(), cascade => true);
+SELECT trunklet.template_language__remove(get_test_language_name(), cascade => true);
 
 DO $body$
 DECLARE

test/install/load.sql

@@ -4,12 +4,23 @@
  * and "Update & Upgrade (U&U) Testing" sections). The mode comes from the
  * Makefile's TEST_LOAD_SOURCE / TEST_UPDATE_FROM via PGOPTIONS.
  *
- * This file's output is never compared against anything. Its \set ON_ERROR_STOP
- * makes psql exit non-zero on an error, which fails the run only on PG16+. On
- * PG 9.5-15 a failure surfaces only through the downstream tests.
+ * This file's output is never compared against anything, and an error here
+ * fails the run only by breaking the downstream tests. \set ON_ERROR_STOP makes
+ * psql stop at the first error.
  */
 \set ON_ERROR_STOP on
 
+\i test/helpers/settings_snapshot.sql
+
+-- Below warning, so a plain SET in a script would show (error or higher is not exercised)
+SET client_min_messages = notice;
+SELECT pg_temp.settings_snapshot();
+
+/*
+ * One transaction, so a failed settings check rolls back the install and the
+ * downstream tests fail
+ */
+BEGIN;
 -- Branches server-side because \if needs psql 10+
 DO $body$
 DECLARE
@@ -28,6 +39,8 @@ BEGIN
   END IF;
 END
 $body$;
+SELECT pg_temp.settings_check('CREATE EXTENSION');
+COMMIT;
 
 -- Update mode only: update_with_data.sql does the ALTER EXTENSION UPDATE. Branches
 -- server-side for the same reason as above.