From 39b14a0aa32c7f1cbc21f6927d1b1bab55ed5212 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 15:54:04 -0500 Subject: [PATCH 1/5] Restore sql/test_factory--0.5.0.sql to its true historical content test_factory 0.5.0 has been the released, real-world-installed version since 2017 (confirmed: no tag for it in this repo, but the "Bump version to 0.5.0" commit is nine years old, and issue #14 itself describes a live production failure against it). Every edit to sql/test_factory.sql on fix/issue-14-clean got silently baked into this file too, via pgxntool's own auto-regeneration (it copies the base file to sql/{ext}--{version}.sql whenever default_version matches) -- retroactively rewriting what "version 0.5.0" contains without ever bumping the version number, which is exactly the anti-pattern pgxntool's own docs warn a released version's file should never be subject to. Restored to match upstream/master (verified byte-for-byte identical), i.e. what 0.5.0 has always actually contained. The issue #14 fix itself stays intact in the base sql/test_factory.sql file, ready for whichever new version number eventually ships it -- that decision is deliberately not made here. release_status in META.in.json/META.json is already "stable" in both files; no change needed there. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0.sql | 81 ------------------------------------- 1 file changed, 81 deletions(-) diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 38db882..37e742b 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -6,7 +6,6 @@ * it's much lighter weight than creating a table. */ SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); - DO $body$ BEGIN CREATE ROLE test_factory__owner; @@ -16,86 +15,6 @@ EXCEPTION END $body$; -/* - * CREATE ROLE alone never grants the creator any relationship to the role - * it just created -- on ANY version, confirmed directly against a genuine - * non-superuser CREATEROLE installer with zero prior grants: SET ROLE - * test_factory__owner below fails outright ("permission denied to set - * role") without an explicit GRANT first, even immediately after this - * same session created it. A real superuser bypasses the SET ROLE check - * entirely regardless of any of this, which is why this was easy to miss - * without testing against a genuine non-superuser role. - * - * As of PG16, plain membership isn't enough either -- SET ROLE requires - * the SET option specifically, which needs GRANT ... WITH SET TRUE. - * Before PG16, plain membership (no WITH SET syntax, which doesn't exist - * yet) already confers the ability to SET ROLE. pg_has_role's 'SET' - * privilege type is PG16+ only, but as a plain function argument (not a - * catalog column reference) it's safe to call inside this same - * version-gated branch, unlike a static reference to - * pg_auth_members.set_option, which would fail to parse on older servers - * even inside the gate. - * - * Either way, skip the GRANT when it's not actually needed: an installer - * might already have the membership it needs some other way -- e.g. a DBA - * pre-provisioned the role and granted it directly, deliberately - * withholding ADMIN OPTION as a least-privilege measure -- and GRANT ROLE - * requires ADMIN OPTION on the target role (or superuser) to run AT ALL, - * regardless of whether it would end up a no-op; forcing it unconditionally - * broke that already-working case (confirmed against a live non-superuser - * role with SET but not ADMIN OPTION: "ERROR: permission denied to grant - * role ... Only roles with the ADMIN option ... may grant this role"). - * - * If the installer has neither the membership it needs nor ADMIN OPTION to - * grant it themselves, installation genuinely cannot proceed -- nobody - * else can do it on their behalf from inside this script. Catch that - * specific case and say so plainly instead of surfacing Postgres's generic - * permission error. - */ -DO $body$ -BEGIN - IF current_setting('server_version_num')::int >= 160000 THEN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - /* - * RAISE's own %-substitution is plain string - * interpolation, not format()'s %I/%L -- build the - * copy-pastable suggested command with format() first - * (so the role name is properly identifier-quoted), - * then substitute the whole result in with a single, - * ordinary %. - */ - RAISE EXCEPTION - 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) - ); - END; - END IF; - ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - RAISE EXCEPTION - 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I;', current_user) - ); - END; - END IF; -END -$body$; - CREATE SCHEMA tf AUTHORIZATION test_factory__owner; COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; GRANT USAGE ON SCHEMA tf TO public; From 3f3fa4a72211f6ca17f40646fdae409df2b5a4d8 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 17:18:43 -0500 Subject: [PATCH 2/5] Fix the root cause of the 0.5.0 mess: sit at the `stable` pseudo-version The real problem wasn't just that sql/test_factory--0.5.0.sql had drifted -- it's that test_factory.control's default_version was pinned at '0.5.0' (a real, already-released version) the whole time. Per ../ai/RELEASE.md's `stable` pseudo-version convention: between releases, default_version should sit at the literal string 'stable', so ordinary source edits regenerate sql/--stable.sql (a disposable scratch file) and never touch a frozen, already-shipped version's file. Left at a real version number, every edit to sql/test_factory.sql kept silently corrupting sql/test_factory--0.5.0.sql -- exactly what happened all session, and what would happen again on the next source edit if left as-is. Set default_version = 'stable' in both test_factory.control and test_factory_pgtap.control (test_factory_pgtap.control's own `superuser = false` addition has the identical problem, one version number earlier). Regenerated sql/test_factory--stable.sql and sql/test_factory_pgtap--stable.sql from the current source (confirmed sql/test_factory--0.5.0.sql and sql/test_factory_pgtap--0.1.0.sql are untouched by this regeneration). Added sql/test_factory--0.5.0--stable.sql, the update script from the last real release to `stable`, containing the same GRANT-if-missing logic as the fresh-install fix -- required for `ALTER EXTENSION test_factory UPDATE` to have anywhere to go at all. test/install/load.sql's update-mode branch needed a real restructuring, not just a version bump: CREATE EXTENSION test_factory VERSION '0.5.0' (the FROM version, genuinely unfixed) cannot succeed as a non-superuser -- that's the original bug, unrelated to versioning -- and extension ownership can't be transferred (`ALTER EXTENSION ... OWNER TO` isn't valid syntax, confirmed), so only the role that ran the original install can ever run ALTER EXTENSION UPDATE on it. There's no reachable "different non-superuser applies this later" scenario to test. Update mode now runs entirely as the ambient role; only fresh mode switches to test_factory_installer (which already fully covers the non-superuser install path) -- moved the issue #14 regression check into the fresh branch specifically, since it was checking the wrong thing under update mode's ambient (typically superuser) role. Verified locally on PG12 and PG17: fresh and update modes both pass, and default_version = 'stable' means `make` no longer touches sql/test_factory--0.5.0.sql or sql/test_factory_pgtap--0.1.0.sql on any future source edit. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0--stable.sql | 49 ++++ sql/test_factory--stable.sql | 337 ++++++++++++++++++++++++++++ sql/test_factory_pgtap--stable.sql | 41 ++++ test/install/load.sql | 162 +++++++------ test_factory.control | 2 +- test_factory_pgtap.control | 2 +- 6 files changed, 520 insertions(+), 73 deletions(-) create mode 100644 sql/test_factory--0.5.0--stable.sql create mode 100644 sql/test_factory--stable.sql create mode 100644 sql/test_factory_pgtap--stable.sql diff --git a/sql/test_factory--0.5.0--stable.sql b/sql/test_factory--0.5.0--stable.sql new file mode 100644 index 0000000..de2a979 --- /dev/null +++ b/sql/test_factory--0.5.0--stable.sql @@ -0,0 +1,49 @@ +/* + * Same issue as a fresh install (see sql/test_factory.sql's own comment): + * CREATE ROLE never granted the ORIGINAL installer any relationship to + * test_factory__owner either, on any PostgreSQL version. An existing + * 0.5.0 install already has test_factory__owner and doesn't need it + * recreated -- it only needs this same GRANT, so that whoever runs this + * update (and anyone else who later needs to SET ROLE + * test_factory__owner) has it too. No object in this extension changed + * between 0.5.0 and here, so nothing needs to run AS test_factory__owner + * -- unlike the fresh-install script, there's no role to switch to or + * restore. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING ERRCODE = 'insufficient_privilege' + , HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) + ); + END; + END IF; + ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING ERRCODE = 'insufficient_privilege' + , HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I;', current_user) + ); + END; + END IF; +END +$body$; + +-- vi: expandtab ts=2 sw=2 diff --git a/sql/test_factory--stable.sql b/sql/test_factory--stable.sql new file mode 100644 index 0000000..23c9360 --- /dev/null +++ b/sql/test_factory--stable.sql @@ -0,0 +1,337 @@ +/* DO NOT EDIT - AUTO-GENERATED FILE */ +/* + * Save the caller's role so we can restore it at the end (we SET LOCAL ROLE + * below to own our objects). A GUC is used instead of a temp table not to + * avoid CREATE EXTENSION breakage (trivial to avoid either way) but because + * it's much lighter weight than creating a table. + */ +SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); + +DO $body$ +BEGIN + CREATE ROLE test_factory__owner; +EXCEPTION + WHEN duplicate_object THEN + NULL; +END +$body$; + +/* + * CREATE ROLE alone never grants the creator any relationship to the role + * it just created -- on ANY version, confirmed directly against a genuine + * non-superuser CREATEROLE installer with zero prior grants: SET ROLE + * test_factory__owner below fails outright ("permission denied to set + * role") without an explicit GRANT first, even immediately after this + * same session created it. A real superuser bypasses the SET ROLE check + * entirely regardless of any of this, which is why this was easy to miss + * without testing against a genuine non-superuser role. + * + * As of PG16, plain membership isn't enough either -- SET ROLE requires + * the SET option specifically, which needs GRANT ... WITH SET TRUE. + * Before PG16, plain membership (no WITH SET syntax, which doesn't exist + * yet) already confers the ability to SET ROLE. pg_has_role's 'SET' + * privilege type is PG16+ only, but as a plain function argument (not a + * catalog column reference) it's safe to call inside this same + * version-gated branch, unlike a static reference to + * pg_auth_members.set_option, which would fail to parse on older servers + * even inside the gate. + * + * Either way, skip the GRANT when it's not actually needed: an installer + * might already have the membership it needs some other way -- e.g. a DBA + * pre-provisioned the role and granted it directly, deliberately + * withholding ADMIN OPTION as a least-privilege measure -- and GRANT ROLE + * requires ADMIN OPTION on the target role (or superuser) to run AT ALL, + * regardless of whether it would end up a no-op; forcing it unconditionally + * broke that already-working case (confirmed against a live non-superuser + * role with SET but not ADMIN OPTION: "ERROR: permission denied to grant + * role ... Only roles with the ADMIN option ... may grant this role"). + * + * If the installer has neither the membership it needs nor ADMIN OPTION to + * grant it themselves, installation genuinely cannot proceed -- nobody + * else can do it on their behalf from inside this script. Catch that + * specific case and say so plainly instead of surfacing Postgres's generic + * permission error. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + /* + * RAISE's own %-substitution is plain string + * interpolation, not format()'s %I/%L -- build the + * copy-pastable suggested command with format() first + * (so the role name is properly identifier-quoted), + * then substitute the whole result in with a single, + * ordinary %. + */ + RAISE EXCEPTION + 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) + ); + END; + END IF; + ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + BEGIN + EXECUTE format('GRANT test_factory__owner TO %I', current_user); + EXCEPTION + WHEN insufficient_privilege THEN + RAISE EXCEPTION + 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' + , current_user + USING HINT = format( + 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' + , format('GRANT test_factory__owner TO %I;', current_user) + ); + END; + END IF; +END +$body$; + +CREATE SCHEMA tf AUTHORIZATION test_factory__owner; +COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; +GRANT USAGE ON SCHEMA tf TO public; + +CREATE SCHEMA _tf AUTHORIZATION test_factory__owner; +-- Sucks that we have to do this. Need community to separate visibility and usage. +GRANT USAGE ON SCHEMA _tf TO public; + +CREATE SCHEMA _test_factory_test_data AUTHORIZATION test_factory__owner; + +-- Need to be SU +CREATE OR REPLACE FUNCTION _tf.schema__getsert( +) RETURNS name SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ +BEGIN + RETURN '_test_factory_test_data'; +END +$body$; + +SET LOCAL ROLE test_factory__owner; + +CREATE TYPE tf.test_set AS ( + set_name text + , insert_sql text +); + +CREATE TABLE _tf._test_factory( + factory_id SERIAL NOT NULL PRIMARY KEY + , table_oid regclass NOT NULL -- Can't do a FK to a catalog + , set_name text NOT NULL + , insert_sql text NOT NULL + , UNIQUE( table_oid, set_name ) +); +SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); +SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); + + +CREATE OR REPLACE FUNCTION _tf.data_table_name( + table_name text -- Sanitized by tf.test_factory__get() + , set_name _tf._test_factory.set_name%TYPE +) RETURNS name LANGUAGE plpgsql AS $body$ +DECLARE + v_factory_id_text text; + v_table_name name; + + v_name name; +BEGIN + SELECT + -- Get a fixed-width representation of ID. btrim shouldn't be necessary but it is + '_' || btrim( to_char( + factory_id + -- Get a string of 0's long enough to hold a max-sized int + , repeat( '0', length( (2^31-1)::int::text ) ) + ) ) + , c.relname + INTO v_factory_id_text, v_table_name + FROM tf.test_factory__get( table_name, set_name ) f + JOIN pg_class c ON c.oid = f.table_oid + JOIN pg_namespace n ON n.oid = c.relnamespace + ; + + v_name := v_table_name || v_factory_id_text; + + -- Was the name truncated? + IF v_name <> (v_table_name || v_factory_id_text) THEN + v_name := substring( v_table_name, length(v_name) - length(v_factory_id_text ) ) + || v_factory_id_text + ; + END IF; + + RETURN v_name; +END +$body$; + + +CREATE OR REPLACE FUNCTION _tf.test_factory__get( + table_name text -- Sanitized by tf.test_factory__get() + , set_name _tf._test_factory.set_name%TYPE + , table_oid oid -- Must be passed in because of forced search_path +) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ +DECLARE + v_test_factory _tf._test_factory; +BEGIN + SELECT * INTO STRICT v_test_factory + FROM _tf._test_factory tf + WHERE tf.table_oid = test_factory__get.table_oid + AND tf.set_name = test_factory__get.set_name + ; + + RETURN v_test_factory; +EXCEPTION + WHEN no_data_found THEN + RAISE 'No factory found for table "%", set name "%"', table_name, set_name; +END +$body$; +CREATE OR REPLACE FUNCTION tf.test_factory__get( + table_name text + , set_name _tf._test_factory.set_name%TYPE +) RETURNS _tf._test_factory LANGUAGE sql AS $body$ +SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) +$body$; + + +CREATE OR REPLACE FUNCTION _tf.test_factory__set( + table_oid regclass + , set_name text + , insert_sql text +) RETURNS void SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ +BEGIN + UPDATE _tf._test_factory + SET insert_sql = test_factory__set.insert_sql + WHERE _test_factory.table_oid = test_factory__set.table_oid + AND _test_factory.set_name = test_factory__set.set_name + ; + /* + * There shouldn't be concurrency conflicts here. If there are I think it's + * better to error than UPSERT. + */ + IF NOT FOUND THEN + INSERT INTO _tf._test_factory( table_oid, set_name, insert_sql ) + VALUES( table_oid, set_name, insert_sql ) + ; + END IF; +END +$body$; + + +CREATE OR REPLACE FUNCTION tf.register( + table_name text + , test_sets tf.test_set[] +) RETURNS void LANGUAGE plpgsql AS $body$ +DECLARE + c_table_oid CONSTANT regclass := table_name; + v_set tf.test_set; +BEGIN + FOREACH v_set IN ARRAY test_sets LOOP + PERFORM _tf.test_factory__set( + c_table_oid + , v_set.set_name + , v_set.insert_sql + ); + END LOOP; +END +$body$; + + +CREATE OR REPLACE FUNCTION _tf.table_create( + table_name text +) RETURNS void SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ +DECLARE + c_td_schema CONSTANT name := _tf.schema__getsert(); + sql text; +BEGIN + sql := format( + $sql$ +CREATE TABLE %I.%I AS SELECT * FROM pg_temp.%2$I; + $sql$ + , c_td_schema + , table_name + ); + RAISE DEBUG 'sql = %', sql; + EXECUTE sql; +END +$body$; + +CREATE OR REPLACE FUNCTION tf.get( + table_type anyelement + , set_name text +) RETURNS SETOF anyelement LANGUAGE plpgsql AS $body$ +DECLARE + c_table_name CONSTANT text := pg_typeof(table_type); + c_data_table_name CONSTANT name := _tf.data_table_name( c_table_name, set_name ); +BEGIN + -- SEE BELOW AS WELL + RETURN QUERY SELECT * FROM _tf.get(table_type, set_name, c_data_table_name); +EXCEPTION + WHEN undefined_table THEN + DECLARE + create_sql text; + BEGIN + -- TODO: Create temp table with caller security then create permanent table as test_factory__owner + SELECT format( + $$ +CREATE TEMP TABLE %I ON COMMIT DROP AS +WITH i AS ( + %s + ) + SELECT * + FROM i +; +GRANT SELECT ON pg_temp.%1$I TO test_factory__owner; +$$ + , c_data_table_name + , factory.insert_sql + ) + INTO create_sql + FROM tf.test_factory__get( c_table_name, set_name ) factory + ; + RAISE DEBUG 'sql = %', create_sql; + EXECUTE create_sql; + PERFORM _tf.table_create( c_data_table_name ); + + -- SEE ABOVE AS WELL + RETURN QUERY SELECT * FROM _tf.get(table_type, set_name, c_data_table_name); + + -- Can't do this in the secdef function because it doesn't own it. + EXECUTE format( 'DROP TABLE pg_temp.%I', c_data_table_name ); + END; +END +$body$; + +CREATE OR REPLACE FUNCTION _tf.get( + table_type anyelement -- Sanitized by tf.test_factory__get() + , set_name text + , data_table_name name +) RETURNS SETOF anyelement SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ +DECLARE + c_table_name CONSTANT text := pg_typeof(table_type); + c_td_schema CONSTANT name := _tf.schema__getsert(); + + sql text; +BEGIN + sql := format( + 'SELECT * FROM %I.%I AS t' + , c_td_schema + , data_table_name + ); + RAISE DEBUG 'sql = %', sql; + + RETURN QUERY EXECUTE sql; +END +$body$; + +--select (tf.get('moo','moo')::moo).*; +-- Restore the caller's role saved at the top of this script. +DO $body$ +BEGIN + EXECUTE 'SET ROLE ' || pg_catalog.quote_ident(pg_catalog.current_setting('test_factory.original_role')); +END +$body$; + +-- vi: expandtab ts=2 sw=2 diff --git a/sql/test_factory_pgtap--stable.sql b/sql/test_factory_pgtap--stable.sql new file mode 100644 index 0000000..9eea098 --- /dev/null +++ b/sql/test_factory_pgtap--stable.sql @@ -0,0 +1,41 @@ +/* DO NOT EDIT - AUTO-GENERATED FILE */ +/* + * Save the caller's role so we can restore it at the end (we SET LOCAL ROLE + * below to own our objects). A GUC is used instead of a temp table not to + * avoid CREATE EXTENSION breakage (trivial to avoid either way) but because + * it's much lighter weight than creating a table. + */ +SELECT pg_catalog.set_config('test_factory_pgtap.original_role', current_user, true); + +SET LOCAL ROLE test_factory__owner; + +CREATE OR REPLACE FUNCTION tf.tap( + table_name text + , set_name text DEFAULT 'base' +) RETURNS SETOF text LANGUAGE plpgsql AS $body$ +DECLARE + c_table CONSTANT regclass := table_name; +BEGIN + RETURN NEXT isnt_empty( + format( + $$SELECT tf.get( NULL::%s, %L )$$ -- We assume regclass::text gives us valid output + , c_table + , set_name + ) + , format( + 'Get test data set "%s" for table %s' + , set_name + , c_table + ) + ); +END +$body$; + +-- Set role back to original value (saved at the top of this script). +DO $body$ +BEGIN + EXECUTE 'SET ROLE ' || pg_catalog.quote_ident(pg_catalog.current_setting('test_factory_pgtap.original_role')); +END +$body$; + +-- vi: expandtab ts=2 sw=2 diff --git a/test/install/load.sql b/test/install/load.sql index 5c084f2..9f6ad7f 100644 --- a/test/install/load.sql +++ b/test/install/load.sql @@ -246,39 +246,30 @@ SELECT :'load_mode' = 'existing' AS is_existing DROP EXTENSION IF EXISTS test_factory_pgtap CASCADE; DROP EXTENSION IF EXISTS test_factory CASCADE; - -- Everything from here on -- the actual install this mode exists to - -- test -- runs as the installer instead. - SET SESSION AUTHORIZATION :installer_role; - - /* - * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap - * installs, not after. test_factory_pgtap.control declares pgtap as a - * requirement too, so the CREATE EXTENSION ... CASCADE (or plain CREATE - * EXTENSION, in update mode) below would otherwise cascade-install pgtap - * itself into whatever schema this session's ambient search_path - * resolves to (public, by default) -- and since that satisfies "pgtap is - * already installed", the main suite's own tap_setup.sql (which does - * CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap) then skips creating it - * in "tap" at all, leaving every pgTAP-based test file failing with - * "function no_plan() does not exist" (hit this for real writing this - * file). IF NOT EXISTS on both statements: harmless no-op on a rerun - * where a previous pass already did this and the drop-first reset above - * didn't touch it (cascading test_factory_pgtap's drop doesn't remove - * pgtap -- pgtap is its dependency, not the other way around). - */ - CREATE SCHEMA IF NOT EXISTS tap; - CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; - - -- Captured before either branch below runs CREATE EXTENSION, so the - -- role-restore proof after \endif covers whichever one actually ran. - SELECT current_user AS role_before_install - \gset - SELECT :'load_mode' = 'update' AS is_update \gset -- update-vs-fresh install \if :is_update + /* + * update mode runs entirely as the ambient role, not + * test_factory_installer: extension ownership can't be transferred + * (`ALTER EXTENSION ... OWNER TO` isn't valid syntax -- confirmed), + * so only the role that ran the ORIGINAL CREATE EXTENSION can ever + * run ALTER EXTENSION UPDATE on it. There's no reachable "a + * different non-superuser applies this later" scenario to test here + * -- whether the original install itself could have been done + * non-superuser is exactly what the fresh branch below already + * proves. + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + CREATE EXTENSION test_factory VERSION :'update_from'; SET client_min_messages = ERROR; -- suppress update-script deprecation NOTICEs \if :has_update_to @@ -297,12 +288,36 @@ SELECT :'load_mode' = 'existing' AS is_existing -- update-vs-fresh install \else + -- Everything from here on -- the actual install this mode exists to + -- test -- runs as the installer instead. + SET SESSION AUTHORIZATION :installer_role; + + /* + * pgtap must land in a dedicated "tap" schema BEFORE test_factory_pgtap + * installs, not after. test_factory_pgtap.control declares pgtap as a + * requirement too, so the CREATE EXTENSION ... CASCADE below would + * otherwise cascade-install pgtap itself into whatever schema this + * session's ambient search_path resolves to (public, by default) -- + * and since that satisfies "pgtap is already installed", the main + * suite's own tap_setup.sql (which does CREATE EXTENSION IF NOT + * EXISTS pgtap SCHEMA tap) then skips creating it in "tap" at all, + * leaving every pgTAP-based test file failing with "function + * no_plan() does not exist" (hit this for real writing this file). + * IF NOT EXISTS on both statements: harmless no-op on a rerun where a + * previous pass already did this and the drop-first reset above + * didn't touch it (cascading test_factory_pgtap's drop doesn't + * remove pgtap -- pgtap is its dependency, not the other way + * around). + */ + CREATE SCHEMA IF NOT EXISTS tap; + CREATE EXTENSION IF NOT EXISTS pgtap SCHEMA tap; + + -- Captured before CREATE EXTENSION, so the role-restore proof below + -- covers it too. + SELECT current_user AS role_before_install + \gset + /* - * fresh: install for real, right here, uniformly with update/existing -- - * so test/sql/base.sql and test/sql/pgtap.sql can assume both - * extensions are already present in every mode, instead of each mode - * needing its own install-or-skip dance (what test/helpers/ - * create_extension.sql used to do; deleted along with this change). * CASCADE proves test_factory_pgtap.control's "requires = 'pgtap, * test_factory'" line actually pulls test_factory in -- * test/sql/pgtap.sql separately proves the dependency is *enforced* @@ -310,6 +325,49 @@ SELECT :'load_mode' = 'existing' AS is_existing */ CREATE EXTENSION test_factory_pgtap CASCADE; + /* + * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, + * checked from THIS session (still test_factory_installer, per the + * SET SESSION AUTHORIZATION above) rather than from + * test/sql/base.sql's own separate connection, which would only ever + * see whatever role CI or a developer happens to connect as -- + * pg_has_role(current_user, ...) is unconditionally true for a + * superuser regardless of whether the fix's GRANT logic in + * sql/test_factory.sql ever ran. test_factory_installer is freshly + * created every run with zero prior relationships, so it can only + * have this membership if that GRANT actually fired -- there's no + * other way it could already be there. Only checked here, not in + * update mode above: update mode never switches to + * test_factory_installer (see its own comment), so current_user + * there is whatever ambient role connected -- typically a superuser, + * for whom this would be checking nothing. + * + * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as + * sql/test_factory.sql's own comment); pre-16, plain membership is + * the closest equivalent ('SET' isn't a meaningful distinction yet -- + * membership itself already confers the ability to SET ROLE). + */ + SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus + \gset + -- pg16+ SET-enabled membership check + \if :pg16plus + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \else + DO $$ + BEGIN + IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN + RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; + END IF; + END $$; + -- pg16+ SET-enabled membership check + \endif + -- update-vs-fresh install \endif @@ -333,44 +391,6 @@ SELECT :'load_mode' = 'existing' AS is_existing DO $$ BEGIN RAISE EXCEPTION 'CREATE EXTENSION did not restore the calling role'; END $$; \endif - /* - * Regression test for https://github.com/Postgres-Extensions/test_factory/issues/14, - * checked from THIS session (still test_factory_installer, per the SET - * SESSION AUTHORIZATION above) rather than from test/sql/base.sql's own - * separate connection, which would only ever see whatever role CI or a - * developer happens to connect as -- pg_has_role(current_user, ...) is - * unconditionally true for a superuser regardless of whether the fix's - * GRANT logic in sql/test_factory.sql ever ran. test_factory_installer is - * freshly created every run with zero prior relationships, so it can only - * have this membership if that GRANT actually fired -- there's no other - * way it could already be there. - * - * pg_has_role's 'SET' privilege type is PG16+ only (same reasoning as - * sql/test_factory.sql's own comment); pre-16, plain membership is the - * closest equivalent ('SET' isn't a meaningful distinction yet -- - * membership itself already confers the ability to SET ROLE). - */ - SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus - \gset - -- pg16+ SET-enabled membership check - \if :pg16plus - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks SET-enabled membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \else - DO $$ - BEGIN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - RAISE EXCEPTION 'issue #14 regression: % lacks membership in test_factory__owner after install', current_user; - END IF; - END $$; - -- pg16+ SET-enabled membership check - \endif - -- existing-vs-fresh/update \endif diff --git a/test_factory.control b/test_factory.control index b05ee89..446c18a 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,5 +1,5 @@ comment = 'A framework for managing test data' -default_version = '0.5.0' +default_version = 'stable' relocatable = false # Not a security boundary weakening: test_factory__owner is a locked-down, # dedicated owner role, and every privileged function is SECURITY DEFINER diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index 0711698..f19f82f 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -1,5 +1,5 @@ comment = 'A framework for managing test data' -default_version = '0.1.0' +default_version = 'stable' relocatable = false requires = 'pgtap, test_factory' # See test_factory.control -- same rationale. From fd839b3e0771cbaca95d0c54c654a8a654fc94f2 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Tue, 11 Aug 2026 17:30:11 -0500 Subject: [PATCH 3/5] bin/test_existing: fix version assertion for the stable pseudo-version assert_version() compared installed extversion against `make -s print-PGXNVERSION`, which pulls META.json's *distribution* version (still the real semver, e.g. 0.5.0) -- unaffected by test_factory.control's default_version, which this PR pins to the literal 'stable' pseudo-version between releases. A bare CREATE EXTENSION now installs at 'stable', so the two were guaranteed to mismatch: confirmed via the pg-upgrade-test CI job failing with "test_factory in '...' is 'stable', expected '0.5.0'". Switched to `make -s print-EXTENSION_test_factory_VERSION` (from pgxntool's generated control.mk, tracks default_version directly), verified locally end-to-end: a fresh CREATE EXTENSION installs at 'stable', and bin/test_existing run-suite's own version check now reports installed='stable' expected='stable' and passes. Also picked up the ERRCODE fix in sql/test_factory--stable.sql (regenerated after rebasing onto the latest fix/issue-14-clean). Co-Authored-By: Claude Sonnet 5 --- bin/test_existing | 21 ++++++++++++++------- sql/test_factory--stable.sql | 6 ++++-- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/bin/test_existing b/bin/test_existing index a4f8a74..d92e4be 100755 --- a/bin/test_existing +++ b/bin/test_existing @@ -26,12 +26,12 @@ # # run-suite DB # Assert the installed version matches the current build (never -# hardcoded -- derived from `make -s print-PGXNVERSION`, so a broken -# extraction can't silently compare "" to ""), then run the suite in -# existing mode via --use-existing (pg_regress must not drop/recreate -# DB) and gate on verify-results, not a bare `make test` (pgxntool -# marks installcheck .IGNORE, so a plain test run exits 0 even when -# regression.diffs is nonempty). +# hardcoded -- derived from `make -s print-EXTENSION_test_factory_VERSION`, +# so a broken extraction can't silently compare "" to ""), then run +# the suite in existing mode via --use-existing (pg_regress must not +# drop/recreate DB) and gate on verify-results, not a bare `make test` +# (pgxntool marks installcheck .IGNORE, so a plain test run exits 0 +# even when regression.diffs is nonempty). # # No separate guard-planting subcommand: unlike cat_tools, test_factory's # dependency guard (a view in schema test_factory_drop_guard depending on @@ -57,8 +57,15 @@ psql_do() { psql -d "$db" -v ON_ERROR_STOP=1 "$@" } +# EXTENSION_test_factory_VERSION, not PGXNVERSION: the latter is the +# *distribution* version from META.json, which no longer tracks what +# CREATE EXTENSION actually installs now that test_factory.control's +# default_version sits at the literal 'stable' pseudo-version between +# releases (see ../ai/RELEASE.md) -- a bare CREATE EXTENSION always +# resolves to whatever default_version says, not the distribution's +# semver. current_version() { - make -s print-PGXNVERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p' + make -s print-EXTENSION_test_factory_VERSION 2>/dev/null | sed -n 's/.*set to "\(.*\)"$/\1/p' } installed_version() { diff --git a/sql/test_factory--stable.sql b/sql/test_factory--stable.sql index 23c9360..38db882 100644 --- a/sql/test_factory--stable.sql +++ b/sql/test_factory--stable.sql @@ -71,7 +71,8 @@ BEGIN RAISE EXCEPTION 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + USING ERRCODE = 'insufficient_privilege' + , HINT = format( 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) ); @@ -85,7 +86,8 @@ BEGIN RAISE EXCEPTION 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' , current_user - USING HINT = format( + USING ERRCODE = 'insufficient_privilege' + , HINT = format( 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' , format('GRANT test_factory__owner TO %I;', current_user) ); From 08eae2e0934805490460ce16693f2fedb61f390c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 12 Aug 2026 14:52:21 -0500 Subject: [PATCH 4/5] Gitignore the stable pseudo-version's generated SQL files sql/test_factory--stable.sql and sql/test_factory_pgtap--stable.sql are permanently current, not a frozen release -- per ../ai/CLAUDE.md's "Version-specific SQL files" section (referenced directly from ../ai/RELEASE.md), they'd be regenerated and re-diffed on every single source edit for zero test-coverage value if tracked. Removed from git, added sql/*--stable.sql to .gitignore. sql/test_factory--0.5.0--stable.sql (the real update script) is unaffected and stays tracked -- confirmed via git check-ignore. Verified: rm'd both files, ran `make clean && make test` (fresh and update modes) from that state -- both regenerate automatically and the full suite still passes. Co-Authored-By: Claude Sonnet 5 --- .gitignore | 7 + sql/test_factory--stable.sql | 339 ----------------------------- sql/test_factory_pgtap--stable.sql | 41 ---- 3 files changed, 7 insertions(+), 380 deletions(-) delete mode 100644 sql/test_factory--stable.sql delete mode 100644 sql/test_factory_pgtap--stable.sql diff --git a/.gitignore b/.gitignore index 941a061..8122bbe 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,13 @@ control.mk # built targets # Note: Version-specific files (sql/*--*.sql) are now tracked in git and should be committed +# Exception: the `stable` pseudo-version (default_version between releases, +# see ../ai/RELEASE.md and ../ai/CLAUDE.md's "Version-specific SQL files") +# is permanently current, not a frozen release -- it would be regenerated +# and re-diffed on every source edit for zero test-coverage value if +# tracked. The update script *to* stable (sql/*----stable.sql) +# is not affected by this and must stay committed. +sql/*--stable.sql # Generated by asciidoctor from doc/*.asc; only the .asc source is tracked doc/*.html diff --git a/sql/test_factory--stable.sql b/sql/test_factory--stable.sql deleted file mode 100644 index 38db882..0000000 --- a/sql/test_factory--stable.sql +++ /dev/null @@ -1,339 +0,0 @@ -/* DO NOT EDIT - AUTO-GENERATED FILE */ -/* - * Save the caller's role so we can restore it at the end (we SET LOCAL ROLE - * below to own our objects). A GUC is used instead of a temp table not to - * avoid CREATE EXTENSION breakage (trivial to avoid either way) but because - * it's much lighter weight than creating a table. - */ -SELECT pg_catalog.set_config('test_factory.original_role', current_user, true); - -DO $body$ -BEGIN - CREATE ROLE test_factory__owner; -EXCEPTION - WHEN duplicate_object THEN - NULL; -END -$body$; - -/* - * CREATE ROLE alone never grants the creator any relationship to the role - * it just created -- on ANY version, confirmed directly against a genuine - * non-superuser CREATEROLE installer with zero prior grants: SET ROLE - * test_factory__owner below fails outright ("permission denied to set - * role") without an explicit GRANT first, even immediately after this - * same session created it. A real superuser bypasses the SET ROLE check - * entirely regardless of any of this, which is why this was easy to miss - * without testing against a genuine non-superuser role. - * - * As of PG16, plain membership isn't enough either -- SET ROLE requires - * the SET option specifically, which needs GRANT ... WITH SET TRUE. - * Before PG16, plain membership (no WITH SET syntax, which doesn't exist - * yet) already confers the ability to SET ROLE. pg_has_role's 'SET' - * privilege type is PG16+ only, but as a plain function argument (not a - * catalog column reference) it's safe to call inside this same - * version-gated branch, unlike a static reference to - * pg_auth_members.set_option, which would fail to parse on older servers - * even inside the gate. - * - * Either way, skip the GRANT when it's not actually needed: an installer - * might already have the membership it needs some other way -- e.g. a DBA - * pre-provisioned the role and granted it directly, deliberately - * withholding ADMIN OPTION as a least-privilege measure -- and GRANT ROLE - * requires ADMIN OPTION on the target role (or superuser) to run AT ALL, - * regardless of whether it would end up a no-op; forcing it unconditionally - * broke that already-working case (confirmed against a live non-superuser - * role with SET but not ADMIN OPTION: "ERROR: permission denied to grant - * role ... Only roles with the ADMIN option ... may grant this role"). - * - * If the installer has neither the membership it needs nor ADMIN OPTION to - * grant it themselves, installation genuinely cannot proceed -- nobody - * else can do it on their behalf from inside this script. Catch that - * specific case and say so plainly instead of surfacing Postgres's generic - * permission error. - */ -DO $body$ -BEGIN - IF current_setting('server_version_num')::int >= 160000 THEN - IF NOT pg_has_role(current_user, 'test_factory__owner', 'SET') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - /* - * RAISE's own %-substitution is plain string - * interpolation, not format()'s %I/%L -- build the - * copy-pastable suggested command with format() first - * (so the role name is properly identifier-quoted), - * then substitute the whole result in with a single, - * ordinary %. - */ - RAISE EXCEPTION - 'role "%" lacks SET-enabled membership in "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I WITH SET TRUE;', current_user) - ); - END; - END IF; - ELSIF NOT pg_has_role(current_user, 'test_factory__owner', 'MEMBER') THEN - BEGIN - EXECUTE format('GRANT test_factory__owner TO %I', current_user); - EXCEPTION - WHEN insufficient_privilege THEN - RAISE EXCEPTION - 'role "%" is not a member of "test_factory__owner", and lacks ADMIN OPTION to grant it to itself' - , current_user - USING ERRCODE = 'insufficient_privilege' - , HINT = format( - 'Ask a superuser, or a role with ADMIN OPTION on "test_factory__owner", to run: %s' - , format('GRANT test_factory__owner TO %I;', current_user) - ); - END; - END IF; -END -$body$; - -CREATE SCHEMA tf AUTHORIZATION test_factory__owner; -COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; -GRANT USAGE ON SCHEMA tf TO public; - -CREATE SCHEMA _tf AUTHORIZATION test_factory__owner; --- Sucks that we have to do this. Need community to separate visibility and usage. -GRANT USAGE ON SCHEMA _tf TO public; - -CREATE SCHEMA _test_factory_test_data AUTHORIZATION test_factory__owner; - --- Need to be SU -CREATE OR REPLACE FUNCTION _tf.schema__getsert( -) RETURNS name SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ -BEGIN - RETURN '_test_factory_test_data'; -END -$body$; - -SET LOCAL ROLE test_factory__owner; - -CREATE TYPE tf.test_set AS ( - set_name text - , insert_sql text -); - -CREATE TABLE _tf._test_factory( - factory_id SERIAL NOT NULL PRIMARY KEY - , table_oid regclass NOT NULL -- Can't do a FK to a catalog - , set_name text NOT NULL - , insert_sql text NOT NULL - , UNIQUE( table_oid, set_name ) -); -SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory', ''); -SELECT pg_catalog.pg_extension_config_dump('_tf._test_factory_factory_id_seq', ''); - - -CREATE OR REPLACE FUNCTION _tf.data_table_name( - table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE -) RETURNS name LANGUAGE plpgsql AS $body$ -DECLARE - v_factory_id_text text; - v_table_name name; - - v_name name; -BEGIN - SELECT - -- Get a fixed-width representation of ID. btrim shouldn't be necessary but it is - '_' || btrim( to_char( - factory_id - -- Get a string of 0's long enough to hold a max-sized int - , repeat( '0', length( (2^31-1)::int::text ) ) - ) ) - , c.relname - INTO v_factory_id_text, v_table_name - FROM tf.test_factory__get( table_name, set_name ) f - JOIN pg_class c ON c.oid = f.table_oid - JOIN pg_namespace n ON n.oid = c.relnamespace - ; - - v_name := v_table_name || v_factory_id_text; - - -- Was the name truncated? - IF v_name <> (v_table_name || v_factory_id_text) THEN - v_name := substring( v_table_name, length(v_name) - length(v_factory_id_text ) ) - || v_factory_id_text - ; - END IF; - - RETURN v_name; -END -$body$; - - -CREATE OR REPLACE FUNCTION _tf.test_factory__get( - table_name text -- Sanitized by tf.test_factory__get() - , set_name _tf._test_factory.set_name%TYPE - , table_oid oid -- Must be passed in because of forced search_path -) RETURNS _tf._test_factory SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ -DECLARE - v_test_factory _tf._test_factory; -BEGIN - SELECT * INTO STRICT v_test_factory - FROM _tf._test_factory tf - WHERE tf.table_oid = test_factory__get.table_oid - AND tf.set_name = test_factory__get.set_name - ; - - RETURN v_test_factory; -EXCEPTION - WHEN no_data_found THEN - RAISE 'No factory found for table "%", set name "%"', table_name, set_name; -END -$body$; -CREATE OR REPLACE FUNCTION tf.test_factory__get( - table_name text - , set_name _tf._test_factory.set_name%TYPE -) RETURNS _tf._test_factory LANGUAGE sql AS $body$ -SELECT * FROM _tf.test_factory__get(table_name, set_name, table_name::regclass) -$body$; - - -CREATE OR REPLACE FUNCTION _tf.test_factory__set( - table_oid regclass - , set_name text - , insert_sql text -) RETURNS void SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ -BEGIN - UPDATE _tf._test_factory - SET insert_sql = test_factory__set.insert_sql - WHERE _test_factory.table_oid = test_factory__set.table_oid - AND _test_factory.set_name = test_factory__set.set_name - ; - /* - * There shouldn't be concurrency conflicts here. If there are I think it's - * better to error than UPSERT. - */ - IF NOT FOUND THEN - INSERT INTO _tf._test_factory( table_oid, set_name, insert_sql ) - VALUES( table_oid, set_name, insert_sql ) - ; - END IF; -END -$body$; - - -CREATE OR REPLACE FUNCTION tf.register( - table_name text - , test_sets tf.test_set[] -) RETURNS void LANGUAGE plpgsql AS $body$ -DECLARE - c_table_oid CONSTANT regclass := table_name; - v_set tf.test_set; -BEGIN - FOREACH v_set IN ARRAY test_sets LOOP - PERFORM _tf.test_factory__set( - c_table_oid - , v_set.set_name - , v_set.insert_sql - ); - END LOOP; -END -$body$; - - -CREATE OR REPLACE FUNCTION _tf.table_create( - table_name text -) RETURNS void SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ -DECLARE - c_td_schema CONSTANT name := _tf.schema__getsert(); - sql text; -BEGIN - sql := format( - $sql$ -CREATE TABLE %I.%I AS SELECT * FROM pg_temp.%2$I; - $sql$ - , c_td_schema - , table_name - ); - RAISE DEBUG 'sql = %', sql; - EXECUTE sql; -END -$body$; - -CREATE OR REPLACE FUNCTION tf.get( - table_type anyelement - , set_name text -) RETURNS SETOF anyelement LANGUAGE plpgsql AS $body$ -DECLARE - c_table_name CONSTANT text := pg_typeof(table_type); - c_data_table_name CONSTANT name := _tf.data_table_name( c_table_name, set_name ); -BEGIN - -- SEE BELOW AS WELL - RETURN QUERY SELECT * FROM _tf.get(table_type, set_name, c_data_table_name); -EXCEPTION - WHEN undefined_table THEN - DECLARE - create_sql text; - BEGIN - -- TODO: Create temp table with caller security then create permanent table as test_factory__owner - SELECT format( - $$ -CREATE TEMP TABLE %I ON COMMIT DROP AS -WITH i AS ( - %s - ) - SELECT * - FROM i -; -GRANT SELECT ON pg_temp.%1$I TO test_factory__owner; -$$ - , c_data_table_name - , factory.insert_sql - ) - INTO create_sql - FROM tf.test_factory__get( c_table_name, set_name ) factory - ; - RAISE DEBUG 'sql = %', create_sql; - EXECUTE create_sql; - PERFORM _tf.table_create( c_data_table_name ); - - -- SEE ABOVE AS WELL - RETURN QUERY SELECT * FROM _tf.get(table_type, set_name, c_data_table_name); - - -- Can't do this in the secdef function because it doesn't own it. - EXECUTE format( 'DROP TABLE pg_temp.%I', c_data_table_name ); - END; -END -$body$; - -CREATE OR REPLACE FUNCTION _tf.get( - table_type anyelement -- Sanitized by tf.test_factory__get() - , set_name text - , data_table_name name -) RETURNS SETOF anyelement SECURITY DEFINER SET search_path = pg_catalog LANGUAGE plpgsql AS $body$ -DECLARE - c_table_name CONSTANT text := pg_typeof(table_type); - c_td_schema CONSTANT name := _tf.schema__getsert(); - - sql text; -BEGIN - sql := format( - 'SELECT * FROM %I.%I AS t' - , c_td_schema - , data_table_name - ); - RAISE DEBUG 'sql = %', sql; - - RETURN QUERY EXECUTE sql; -END -$body$; - ---select (tf.get('moo','moo')::moo).*; --- Restore the caller's role saved at the top of this script. -DO $body$ -BEGIN - EXECUTE 'SET ROLE ' || pg_catalog.quote_ident(pg_catalog.current_setting('test_factory.original_role')); -END -$body$; - --- vi: expandtab ts=2 sw=2 diff --git a/sql/test_factory_pgtap--stable.sql b/sql/test_factory_pgtap--stable.sql deleted file mode 100644 index 9eea098..0000000 --- a/sql/test_factory_pgtap--stable.sql +++ /dev/null @@ -1,41 +0,0 @@ -/* DO NOT EDIT - AUTO-GENERATED FILE */ -/* - * Save the caller's role so we can restore it at the end (we SET LOCAL ROLE - * below to own our objects). A GUC is used instead of a temp table not to - * avoid CREATE EXTENSION breakage (trivial to avoid either way) but because - * it's much lighter weight than creating a table. - */ -SELECT pg_catalog.set_config('test_factory_pgtap.original_role', current_user, true); - -SET LOCAL ROLE test_factory__owner; - -CREATE OR REPLACE FUNCTION tf.tap( - table_name text - , set_name text DEFAULT 'base' -) RETURNS SETOF text LANGUAGE plpgsql AS $body$ -DECLARE - c_table CONSTANT regclass := table_name; -BEGIN - RETURN NEXT isnt_empty( - format( - $$SELECT tf.get( NULL::%s, %L )$$ -- We assume regclass::text gives us valid output - , c_table - , set_name - ) - , format( - 'Get test data set "%s" for table %s' - , set_name - , c_table - ) - ); -END -$body$; - --- Set role back to original value (saved at the top of this script). -DO $body$ -BEGIN - EXECUTE 'SET ROLE ' || pg_catalog.quote_ident(pg_catalog.current_setting('test_factory_pgtap.original_role')); -END -$body$; - --- vi: expandtab ts=2 sw=2 From 9204e5acbd78c48190477cb5f0cd0b54fe8e3c97 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 12 Aug 2026 15:38:52 -0500 Subject: [PATCH 5/5] sql/test_factory--0.5.0--stable.sql: fix a comment lifted from the wrong context Opened with "CREATE ROLE never granted..." -- language carried over from sql/test_factory.sql's own comment, where a CREATE ROLE statement exists right above it. This file has no CREATE ROLE at all (test_factory__owner already exists on any real 0.5.0 install), so that framing described an event that never happens here, confusing about why role creation was even being discussed. Rewrote it to stand on its own: the actual issue #14 bug (never getting SET/plain membership automatically) applies to an existing install just as much as a fresh one, which is why this update script exists at all. Co-Authored-By: Claude Sonnet 5 --- sql/test_factory--0.5.0--stable.sql | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/sql/test_factory--0.5.0--stable.sql b/sql/test_factory--0.5.0--stable.sql index de2a979..7ba91f7 100644 --- a/sql/test_factory--0.5.0--stable.sql +++ b/sql/test_factory--0.5.0--stable.sql @@ -1,14 +1,15 @@ /* - * Same issue as a fresh install (see sql/test_factory.sql's own comment): - * CREATE ROLE never granted the ORIGINAL installer any relationship to - * test_factory__owner either, on any PostgreSQL version. An existing - * 0.5.0 install already has test_factory__owner and doesn't need it - * recreated -- it only needs this same GRANT, so that whoever runs this - * update (and anyone else who later needs to SET ROLE - * test_factory__owner) has it too. No object in this extension changed - * between 0.5.0 and here, so nothing needs to run AS test_factory__owner - * -- unlike the fresh-install script, there's no role to switch to or - * restore. + * https://github.com/Postgres-Extensions/test_factory/issues/14: whoever + * originally installed 0.5.0 never automatically got SET-enabled (PG16+) + * or even plain (pre-16) membership in test_factory__owner -- that's the + * bug, and it applies just as much to an already-existing 0.5.0 install + * as to a fresh one (see sql/test_factory.sql's own comment for the fresh + * case). This grants it retroactively, so whoever runs this update -- and + * anyone else who later needs to SET ROLE test_factory__owner -- has it. + * + * No object in this extension changed between 0.5.0 and here, so unlike + * the fresh-install script, there's nothing to create or alter AS + * test_factory__owner, and so no role to switch to or restore. */ DO $body$ BEGIN