diff --git a/HISTORY.asc b/HISTORY.asc index 28d9851..23a761b 100644 --- a/HISTORY.asc +++ b/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()` diff --git a/RELEASE.md b/RELEASE.md index 7e0a937..1710c47 100644 --- a/RELEASE.md +++ b/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. diff --git a/sql/trunklet--0.2.0--0.2.1.sql b/sql/trunklet--0.2.0--0.2.1.sql index 9de514e..04d0cf8 100644 --- a/sql/trunklet--0.2.0--0.2.1.sql +++ b/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; diff --git a/sql/trunklet--0.2.0.sql b/sql/trunklet--0.2.0.sql index 3f23ef4..3da29c2 100644 --- a/sql/trunklet--0.2.0.sql +++ b/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 diff --git a/sql/trunklet--0.2.1--0.3.0.sql b/sql/trunklet--0.2.1--0.3.0.sql index 90628a5..8123085 100644 --- a/sql/trunklet--0.2.1--0.3.0.sql +++ b/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 diff --git a/sql/trunklet--0.2.1.sql b/sql/trunklet--0.2.1.sql index 6a32c74..1d03b6f 100644 --- a/sql/trunklet--0.2.1.sql +++ b/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 ) diff --git a/sql/trunklet--0.3.2--0.3.3.sql b/sql/trunklet--0.3.2--0.3.3.sql index c4843f4..bc66832 100644 --- a/sql/trunklet--0.3.2--0.3.3.sql +++ b/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 ) diff --git a/sql/trunklet--0.3.3.sql b/sql/trunklet--0.3.3.sql index 85fc93b..529e987 100644 --- a/sql/trunklet--0.3.3.sql +++ b/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 ) diff --git a/sql/trunklet.sql b/sql/trunklet.sql index 0342576..a56988a 100644 --- a/sql/trunklet.sql +++ b/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; diff --git a/test/build/build.sql b/test/build/build.sql index 5b592e3..2a4041d 100644 --- a/test/build/build.sql +++ b/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'); diff --git a/test/build/expected/build.out b/test/build/expected/build.out index 2ae39bb..0541b68 100644 --- a/test/build/expected/build.out +++ b/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; diff --git a/test/expected/search_path.out b/test/expected/search_path.out deleted file mode 100644 index 52f2a59..0000000 --- a/test/expected/search_path.out +++ /dev/null @@ -1,5 +0,0 @@ -\set ECHO none -1..1 -tap, public -ok 1 - verify search_path has not changed -# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/helpers/settings_snapshot.sql b/test/helpers/settings_snapshot.sql new file mode 100644 index 0000000..c2de743 --- /dev/null +++ b/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 diff --git a/test/helpers/update_with_data.sql b/test/helpers/update_with_data.sql index 6bca2fd..f8b5ef1 100644 --- a/test/helpers/update_with_data.sql +++ b/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 diff --git a/test/install/load.sql b/test/install/load.sql index 6e50390..d7dfe75 100644 --- a/test/install/load.sql +++ b/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. diff --git a/test/sql/search_path.sql b/test/sql/search_path.sql deleted file mode 100644 index 83d56cb..0000000 --- a/test/sql/search_path.sql +++ /dev/null @@ -1,36 +0,0 @@ -\set ECHO none -\i test/pgxntool/setup.sql - -SELECT plan( - 0 - - +1 -- verify search_path -); - --- Drop test/install/load.sql's committed install; this test's rollback restores it -DROP EXTENSION trunklet; - -CREATE TEMP TABLE before AS - SELECT current_setting('search_path') -; - -SHOW search_path; - -CREATE EXTENSION trunklet;-- CASCADE; - -CREATE TEMP TABLE after AS - SELECT current_setting('search_path') -; - --- Forcibly reset it so we know tap will work -SET search_path=public,tap; - -SELECT is( - (SELECT current_setting FROM after) - , (SELECT current_setting FROM before) - , 'verify search_path has not changed' -); - -\i test/pgxntool/finish.sql - --- vi: expandtab ts=2 sw=2