From e03a0e292790fe9cfe41b04ea483efe609bd5865 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Mon, 17 Aug 2026 17:43:04 -0500 Subject: [PATCH 1/9] Add internal update disable mechanism; refuse to self-track extension objects object__getsert() (via _object_v__for_update()) now refuses to track any object that is itself a member of the object_reference extension, closing a bootstrapping hazard where the extension's own update-time restructuring could trip its own rename-detection/repair machinery. Replace the update script's session_replication_role trick with a real, reusable mechanism: zzz_object_reference__fix_identity and zzz_object_reference_capture self-recognize (and skip) DDL from any extension's own install/update script via in_extension; zzz__object_reference_drop can't self-recognize that way, so internal_update__begin()/__end() explicitly disable/re-enable it (saving and restoring its actual prior state) for future update scripts to reuse. Closes #40. --- sql/object_reference--0.1.0--stable.sql | 278 +++++++++++++++++++++--- sql/object_reference.sql | 127 ++++++++++- test/build/expected/build.out | 13 +- test/expected/base.out | 10 +- test/expected/internal_update.out | 18 ++ test/sql/base.sql | 16 +- test/sql/internal_update.sql | 91 ++++++++ 7 files changed, 514 insertions(+), 39 deletions(-) create mode 100644 test/expected/internal_update.out create mode 100644 test/sql/internal_update.sql diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index b965953..eb3f94f 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -105,32 +105,132 @@ END $body$; /* - * 0.1.0 already installed this extension's own event triggers, and they - * stay active for the rest of THIS session while the structural changes - * below run. zzz__object_reference_drop in particular queries - * _object_reference._object_v inside its own body, so it would fire -- and - * error, since the view is momentarily gone -- the instant this script drops - * that view a few statements down. All three are default-enabled (origin), - * so setting session_replication_role = replica suppresses them for the - * structural section below. + * New: _object_reference.exec(), the permanent counterpart to + * __object_reference.exec() above, used by object__dependency__add() / + * object_group__dependency__add() (unchanged since 0.1.0, but 0.1.0 never + * created this permanent helper -- an existing gap this update closes) and + * by internal_update__begin()/__end() below. + */ +SELECT __object_reference.create_function( + '_object_reference.exec' + , 'sql text' + , 'void LANGUAGE plpgsql' + , $body$ +BEGIN + RAISE DEBUG 'sql = %', sql; + EXECUTE sql; +END +$body$ + , 'Execute arbitrary SQL with logging.' +); + +/* + * New: refuse to track objects that are themselves members of the + * object_reference extension (see the guard added to + * _object_v__for_update() below). + */ +SELECT __object_reference.create_function( + '_object_reference._is_own_object' + , $args$ + classid oid + , objid oid +$args$ + , 'boolean LANGUAGE sql STABLE' + , $body$ +SELECT EXISTS( + SELECT 1 + FROM pg_catalog.pg_depend d + WHERE d.classid = _is_own_object.classid + AND d.objid = _is_own_object.objid + AND d.deptype = 'e' + AND d.refclassid = 'pg_catalog.pg_extension'::regclass + AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) +$body$ + , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' +); + +/* + * New: internal update-time disable/enable mechanism, replacing the + * session_replication_role trick 0.1.0 had no equivalent of. 0.1.0 already + * installed this extension's own event triggers, and they stay active for + * the rest of THIS session while the structural changes below run. + * zzz__object_reference_drop in particular queries _object_reference._object_v + * inside its own body, so it would fire -- and error, since the view is + * momentarily gone -- the instant this script drops that view a few + * statements down. The other two event triggers self-recognize and skip our + * own script's DDL instead (see _etg_fix_identity/_etg_capture below), so + * only zzz__object_reference_drop needs to be disabled here. * - * This script is not necessarily the only thing running in its transaction - * -- ALTER EXTENSION UPDATE can be issued as one statement among several in - * a caller-managed transaction -- so session_replication_role cannot simply - * be left disturbed for "the rest of the transaction" to sort out, and - * whatever it's restored to afterward must be the caller's actual prior - * value, not an assumed 'origin' default (the caller may already have it set - * to something else for their own reasons). Stashed in a placeholder GUC - * (there's no other way to carry a value between separate top-level - * statements in a plain multi-statement SQL script -- this isn't a single - * PL/pgSQL block) and restored explicitly right after the cleanup at the end - * of this script, once every object the event triggers reference is back in - * its final, current-source shape. A fresh install never hits this: it - * creates these event triggers only at the very end, once nothing they - * reference is still being modified. + * These are created now, ahead of the structural section, specifically so + * this script itself can call internal_update__begin() below -- a fresh + * install only ever needs these for FUTURE update scripts. */ -SELECT set_config('object_reference.saved_session_replication_role', current_setting('session_replication_role'), true); -SET LOCAL session_replication_role = replica; +SELECT __object_reference.create_function( + '_object_reference.internal_update__begin' + , $args$ + event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' +$args$ + , 'void LANGUAGE plpgsql' + , $body$ +DECLARE + v_name name; +BEGIN + BEGIN + CREATE TEMP TABLE __object_reference__internal_update( + evtname name PRIMARY KEY + , evtenabled "char" NOT NULL + ); + EXCEPTION WHEN duplicate_table THEN + RAISE 'internal_update__begin() called while already in an internal update' + USING HINT = 'A previous internal_update__end() call may have been skipped.' + ; + END; + + FOREACH v_name IN ARRAY event_trigger_names LOOP + INSERT INTO pg_temp.__object_reference__internal_update(evtname, evtenabled) + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; + + IF NOT FOUND THEN + RAISE 'event trigger "%" does not exist', v_name; + END IF; + + PERFORM _object_reference.exec(format('ALTER EVENT TRIGGER %I DISABLE', v_name)); + END LOOP; +END +$body$ + , 'Disable the given (or default) event triggers for the duration of this extension''s own internal update; pair with internal_update__end().' +); +SELECT __object_reference.create_function( + '_object_reference.internal_update__end' + , '' + , 'void LANGUAGE plpgsql' + , $body$ +DECLARE + r record; +BEGIN + FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__internal_update LOOP + PERFORM _object_reference.exec(format( + 'ALTER EVENT TRIGGER %I %s' + , r.evtname + , CASE r.evtenabled + WHEN 'O' THEN 'ENABLE' + WHEN 'R' THEN 'ENABLE REPLICA' + WHEN 'A' THEN 'ENABLE ALWAYS' + WHEN 'D' THEN 'DISABLE' + END + )); + END LOOP; + + DROP TABLE pg_temp.__object_reference__internal_update; +EXCEPTION WHEN undefined_table THEN + RAISE 'internal_update__end() called without a matching internal_update__begin()'; +END +$body$ + , 'Restore event triggers disabled by internal_update__begin() to their prior enabled state.' +); + +SELECT _object_reference.internal_update__begin(); /* * _object_reference.object: no column changes, just a missing @@ -293,6 +393,14 @@ BEGIN ; END IF; + -- Refuse to track objects that are themselves members of this extension + IF _object_reference._is_own_object(c_classid, objid) THEN + RAISE 'cannot track an object that is a member of the object_reference extension itself' + USING DETAIL = format('object %s belongs to the object_reference extension', r_identity.identity) + , ERRCODE = 'feature_not_supported' + ; + END IF; + -- Ensure the object record exists SELECT INTO r_object_v * @@ -416,6 +524,120 @@ $body$ , 'Check the sanity of object and _object_oid' ); +/* + * _etg_fix_identity/_etg_capture: gain a self-recognition guard so they skip + * DDL issued by any extension's own install/update script (ours included) + * instead of reacting to it -- see internal_update__begin()/__end() above + * for why zzz__object_reference_drop needs a different mechanism. Same + * signatures as 0.1.0, so a plain CREATE OR REPLACE (via create_function) is + * enough -- no DROP needed. + */ +SELECT __object_reference.create_function( + '_object_reference._etg_fix_identity' + , '' + , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' + , $body$ +DECLARE + r_ddl record; + r record; +BEGIN + /* + * Self-recognition: skip DDL issued by any extension's own install/update + * script (ours included); pg_event_trigger_ddl_commands() marks this via + * in_extension, unlike pg_event_trigger_dropped_objects() (see _etg_drop). + */ + IF EXISTS(SELECT 1 FROM pg_catalog.pg_event_trigger_ddl_commands() WHERE in_extension) THEN + RETURN; + END IF; + + /* + * It's tempting to use pg_event_trigger_ddl_commands() to find exactly what + * items have changed and worry about only those. That won't work because an + * object_names array can depend on multiple names (ie: a column depends on + * the name of it's table, as well as the name of the schema the table is in. + * You might think we could simply recurse through pg_depend to handle this, + * but not every name dependency gets enumerated that way. For example, + * columns are not marked as dependent on their table. + * + * Rather than trying to be cute about this, we just do a brute-force check + * for any names that have changed. + */ + + /* + * Presumably there's no way for an objects type/classid to change, but be + * safe and attempt the update to object_type. If it actually does change the + * constraint on the table should catch it. + */ + FOR r IN + UPDATE _object_reference.object + SET object_type = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).type::cat_tools.object_type + , object_names = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_names + , object_args = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_args + FROM _object_reference._object_oid oo + WHERE + oo.object_id = object.object_id + AND (object_type::text, object_names, object_args) IS DISTINCT FROM + (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)) + RETURNING * + LOOP + RAISE DEBUG 'modified_objects(): %', r; + END LOOP; +END +$body$ + , 'Event trigger function to update any records with object names or args that have changed.' +); +SELECT __object_reference.create_function( + '_object_reference._etg_capture' + , '' + , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' + , $body$ +DECLARE + c_group_id CONSTANT int := object_group_id FROM object_reference.capture__get_current(); + r record; +BEGIN + + IF c_group_id IS NOT NULL THEN -- Would be NULL if table is empty + RAISE DEBUG E'\n\n*** START ***'; + BEGIN + FOR r IN + SELECT classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension + -- Have to manually exclude command field :/ + FROM pg_catalog.pg_event_trigger_ddl_commands() + LOOP + RAISE DEBUG 'ddl: %', row_to_json(r); + END LOOP; + END; + + FOR r IN SELECT + _object_reference._object_v__for_update( + object_type::cat_tools.object_type + , objid, objsubid + , c_group_id + , classid + ) + , classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension + FROM pg_catalog.pg_event_trigger_ddl_commands() + WHERE command_tag ~ '^CREATE' --'^(ALTER|CREATE)' + AND NOT object_reference.unsupported(object_type::cat_tools.object_type) + AND (schema_name IS NULL + OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it... + ) + /* + * Self-recognition: skip DDL issued by any extension's own + * install/update script (ours included) rather than trying to + * capture it. + */ + AND NOT in_extension + LOOP + RAISE DEBUG 'registered %', row_to_json(r); + END LOOP; + RAISE DEBUG E'*** END ***\n\n'; + END IF; +END +$body$ + , 'Event trigger function to capture newly created objects in an object group.' +); + /* * object_reference.unsupported(): additionally exclude "partitioned * table"/"partitioned index" (pg_get_object_address() only recognizes the @@ -679,10 +901,10 @@ DROP FUNCTION __object_reference.exec( DROP SCHEMA __object_reference; /* - * Restore session_replication_role to the caller's actual prior value - * (saved near the top of this script), now that the structural section and - * its cleanup are both done. + * Re-enable zzz__object_reference_drop (to its actual prior state, saved by + * internal_update__begin() near the top of this script), now that the + * structural section and its cleanup are both done. */ -SELECT set_config('session_replication_role', current_setting('object_reference.saved_session_replication_role'), true); +SELECT _object_reference.internal_update__end(); -- vi: expandtab sw=2 ts=2 diff --git a/sql/object_reference.sql b/sql/object_reference.sql index cbed4f5..68b1cdb 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -161,6 +161,27 @@ $body$ , 'Execute arbitrary SQL with logging.' ); +SELECT __object_reference.create_function( + '_object_reference._is_own_object' + , $args$ + classid oid + , objid oid +$args$ + , 'boolean LANGUAGE sql STABLE' + , $body$ +SELECT EXISTS( + SELECT 1 + FROM pg_catalog.pg_depend d + WHERE d.classid = _is_own_object.classid + AND d.objid = _is_own_object.objid + AND d.deptype = 'e' + AND d.refclassid = 'pg_catalog.pg_extension'::regclass + AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) +$body$ + , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' +); + CREATE TABLE _object_reference.object( object_id serial PRIMARY KEY , object_type cat_tools.object_type NOT NULL @@ -886,6 +907,14 @@ BEGIN ; END IF; + -- Refuse to track objects that are themselves members of this extension + IF _object_reference._is_own_object(c_classid, objid) THEN + RAISE 'cannot track an object that is a member of the object_reference extension itself' + USING DETAIL = format('object %s belongs to the object_reference extension', r_identity.identity) + , ERRCODE = 'feature_not_supported' + ; + END IF; + -- Ensure the object record exists SELECT INTO r_object_v * @@ -1397,7 +1426,7 @@ BEGIN END LOOP; END; - FOR r IN SELECT + FOR r IN SELECT _object_reference._object_v__for_update( object_type::cat_tools.object_type , objid, objsubid @@ -1411,6 +1440,12 @@ BEGIN AND (schema_name IS NULL OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it... ) + /* + * Self-recognition: skip DDL issued by any extension's own + * install/update script (ours included) rather than trying to + * capture it. + */ + AND NOT in_extension LOOP RAISE DEBUG 'registered %', row_to_json(r); END LOOP; @@ -1431,6 +1466,15 @@ DECLARE r_ddl record; r record; BEGIN + /* + * Self-recognition: skip DDL issued by any extension's own install/update + * script (ours included); pg_event_trigger_ddl_commands() marks this via + * in_extension, unlike pg_event_trigger_dropped_objects() (see _etg_drop). + */ + IF EXISTS(SELECT 1 FROM pg_catalog.pg_event_trigger_ddl_commands() WHERE in_extension) THEN + RETURN; + END IF; + /* * It's tempting to use pg_event_trigger_ddl_commands() to find exactly what * items have changed and worry about only those. That won't work because an @@ -1516,6 +1560,87 @@ $body$ , 'Event trigger function to drop object records when objects are removed.' ); +/* + * Internal update-time disable/enable mechanism, for use by this extension's + * OWN install/update scripts only (not part of the public API). + * + * zzz_object_reference__fix_identity and zzz_object_reference_capture can + * recognize (and skip) DDL issued by any extension's own script via + * pg_event_trigger_ddl_commands()'s in_extension column, so they never need + * to be disabled. zzz__object_reference_drop cannot: it fires from + * pg_event_trigger_dropped_objects(), which has no equivalent column, and it + * queries _object_reference._object_v -- a view an update script may itself + * be dropping and recreating -- so it must be truly disabled for the + * duration of such a script's structural section. + * + * ALTER EVENT TRIGGER is ordinary transactional DDL, so if the calling + * script's transaction rolls back, the DISABLE (and any ENABLE already run) + * rolls back with it -- no separate cleanup-on-error logic is needed here. + */ +SELECT __object_reference.create_function( + '_object_reference.internal_update__begin' + , $args$ + event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' +$args$ + , 'void LANGUAGE plpgsql' + , $body$ +DECLARE + v_name name; +BEGIN + BEGIN + CREATE TEMP TABLE __object_reference__internal_update( + evtname name PRIMARY KEY + , evtenabled "char" NOT NULL + ); + EXCEPTION WHEN duplicate_table THEN + RAISE 'internal_update__begin() called while already in an internal update' + USING HINT = 'A previous internal_update__end() call may have been skipped.' + ; + END; + + FOREACH v_name IN ARRAY event_trigger_names LOOP + INSERT INTO pg_temp.__object_reference__internal_update(evtname, evtenabled) + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; + + IF NOT FOUND THEN + RAISE 'event trigger "%" does not exist', v_name; + END IF; + + PERFORM _object_reference.exec(format('ALTER EVENT TRIGGER %I DISABLE', v_name)); + END LOOP; +END +$body$ + , 'Disable the given (or default) event triggers for the duration of this extension''s own internal update; pair with internal_update__end().' +); +SELECT __object_reference.create_function( + '_object_reference.internal_update__end' + , '' + , 'void LANGUAGE plpgsql' + , $body$ +DECLARE + r record; +BEGIN + FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__internal_update LOOP + PERFORM _object_reference.exec(format( + 'ALTER EVENT TRIGGER %I %s' + , r.evtname + , CASE r.evtenabled + WHEN 'O' THEN 'ENABLE' + WHEN 'R' THEN 'ENABLE REPLICA' + WHEN 'A' THEN 'ENABLE ALWAYS' + WHEN 'D' THEN 'DISABLE' + END + )); + END LOOP; + + DROP TABLE pg_temp.__object_reference__internal_update; +EXCEPTION WHEN undefined_table THEN + RAISE 'internal_update__end() called without a matching internal_update__begin()'; +END +$body$ + , 'Restore event triggers disabled by internal_update__begin() to their prior enabled state.' +); + SELECT __object_reference.create_function( '_object_reference.etg_raise__start' , '' diff --git a/test/build/expected/build.out b/test/build/expected/build.out index fadaba6..0e62ce5 100644 --- a/test/build/expected/build.out +++ b/test/build/expected/build.out @@ -2,17 +2,17 @@ This extension must be loaded via CREATE EXTENSION object_reference; You really, REALLY do NOT want to try and load this via psql!!! -psql:test/temp_load.not_sql:176: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:177: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:197: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:198: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:425: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:446: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! @@ -21,9 +21,12 @@ psql:test/temp_load.not_sql:425: WARNING: I promise you will be sorry if you tr -psql:test/temp_load.not_sql:537: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:544: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:558: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! + +psql:test/temp_load.not_sql:565: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! + + diff --git a/test/expected/base.out b/test/expected/base.out index d5180ba..f2fa6f4 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,5 +1,5 @@ \set ECHO none -1..12 +1..14 ok 1 - Role object_reference__dependency should be granted USAGE on schema _object_reference ok 2 - Role object_reference__dependency should be granted REFERENCES on table _object_reference.object ok 3 - CREATE TEMP TABLE test_object AS SELECT object_reference.object__getsert('table', 'test_table') AS object_id; @@ -9,7 +9,9 @@ ok 6 - object__identity returns same result as pg_identify_object ok 7 - Existing object works, provides correct ID ok 8 - secondary may not be specified for table objects ok 9 - temp objects are rejected -ok 10 - CREATE EXTENSION test_factory -ok 11 - object_reference schema must not be part of the resolved search_path -ok 12 - _object_reference schema must not be part of the resolved search_path +ok 10 - own tracking table is rejected +ok 11 - own event trigger function is rejected +ok 12 - CREATE EXTENSION test_factory +ok 13 - object_reference schema must not be part of the resolved search_path +ok 14 - _object_reference schema must not be part of the resolved search_path # TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/expected/internal_update.out b/test/expected/internal_update.out new file mode 100644 index 0000000..f939ea7 --- /dev/null +++ b/test/expected/internal_update.out @@ -0,0 +1,18 @@ +\set ECHO none +1..15 +ok 1 - internal_update__begin() disables the default trigger +ok 2 - zzz__object_reference_drop is disabled while an internal update is in progress +ok 3 - internal_update__end() re-enables it +ok 4 - zzz__object_reference_drop is back to its original (origin) state +ok 5 - manually disable zzz_object_reference_capture ahead of time +ok 6 - begin()/end() round-trip on an already-disabled trigger +ok 7 - still disabled afterward -- its prior state was preserved, not assumed enabled +ok 8 - restore zzz_object_reference_capture for later tests +ok 9 - begin() the first time +ok 10 - a second begin() without end() in between is rejected +ok 11 - end() cleans up so later tests are unaffected +ok 12 - end() without begin() is rejected +ok 13 - begin() rejects an unknown event trigger name +ok 14 - object_reference schema must not be part of the resolved search_path +ok 15 - _object_reference schema must not be part of the resolved search_path +# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/base.sql b/test/sql/base.sql index 538f4f8..7c8dd7e 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -9,7 +9,7 @@ SELECT plan( +1 -- schema +3 -- initial +2 -- new functions - +3 -- errors (includes temp object test) + +5 -- errors (includes temp object + self-tracking rejection tests) +1 -- create extensions +2 -- schema-qualification (search_path) ); @@ -74,6 +74,20 @@ SELECT throws_ok( , 'temp objects are rejected' ); +-- Test rejection of object_reference's own extension-member objects +SELECT throws_ok( + $$SELECT object_reference.object__getsert('table', '_object_reference.object')$$ + , '0A000' -- feature_not_supported + , 'cannot track an object that is a member of the object_reference extension itself' + , 'own tracking table is rejected' +); +SELECT throws_ok( + $$SELECT object_reference.object__getsert('function', '_object_reference._etg_drop', '')$$ + , '0A000' -- feature_not_supported + , 'cannot track an object that is a member of the object_reference extension itself' + , 'own event trigger function is rejected' +); + -- Create extensions SELECT lives_ok( $$CREATE EXTENSION test_factory$$ diff --git a/test/sql/internal_update.sql b/test/sql/internal_update.sql new file mode 100644 index 0000000..e5c0c62 --- /dev/null +++ b/test/sql/internal_update.sql @@ -0,0 +1,91 @@ +\set ECHO none + +\i test/load.sql + +SELECT plan( + 0 + +4 -- default begin/end round-trip disables, then restores, the trigger + +4 -- begin/end preserves a non-default prior state instead of assuming enabled + +3 -- nested begin() without an intervening end() is rejected + +1 -- end() without a matching begin() is rejected + +1 -- begin() rejects an unknown event trigger name + +2 -- schema-qualification (search_path) +); + +-- Default begin/end round-trip +SELECT lives_ok( + $$SELECT _object_reference.internal_update__begin()$$ + , 'internal_update__begin() disables the default trigger' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + , 'D' + , 'zzz__object_reference_drop is disabled while an internal update is in progress' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_update__end()$$ + , 'internal_update__end() re-enables it' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + , 'O' + , 'zzz__object_reference_drop is back to its original (origin) state' +); + +-- Preserve a non-default prior state (already disabled for unrelated reasons) +SELECT lives_ok( + $$ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE$$ + , 'manually disable zzz_object_reference_capture ahead of time' +); +SELECT lives_ok( + $$ + SELECT _object_reference.internal_update__begin('{zzz_object_reference_capture}'); + SELECT _object_reference.internal_update__end(); + $$ + , 'begin()/end() round-trip on an already-disabled trigger' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz_object_reference_capture') + , 'D' + , 'still disabled afterward -- its prior state was preserved, not assumed enabled' +); +SELECT lives_ok( + $$ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE$$ + , 'restore zzz_object_reference_capture for later tests' +); + +-- Nested begin() without an intervening end() +SELECT lives_ok( + $$SELECT _object_reference.internal_update__begin()$$ + , 'begin() the first time' +); +SELECT throws_ok( + $$SELECT _object_reference.internal_update__begin()$$ + , NULL + , 'internal_update__begin() called while already in an internal update' + , 'a second begin() without end() in between is rejected' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_update__end()$$ + , 'end() cleans up so later tests are unaffected' +); + +-- end() without a matching begin() +SELECT throws_ok( + $$SELECT _object_reference.internal_update__end()$$ + , NULL + , 'internal_update__end() called without a matching internal_update__begin()' + , 'end() without begin() is rejected' +); + +-- Unknown event trigger name +SELECT throws_ok( + $$SELECT _object_reference.internal_update__begin('{no_such_event_trigger}')$$ + , NULL + , 'event trigger "no_such_event_trigger" does not exist' + , 'begin() rejects an unknown event trigger name' +); + +\i test/finish.sql + +-- vi: expandtab sw=2 ts=2 From 79474bebdf38ffdc154c64ab008f099901066aad Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 16 Sep 2026 17:00:55 -0500 Subject: [PATCH 2/9] Address review: fix self-track schema gap, rename/clarify disable mechanism _is_own_object() missed the extension's own declared schema (object_reference): CREATE EXTENSION records the extension as depending on that schema (a plain 'n' dependency, extension -> schema), not the schema as an 'e' member of the extension the way every other object it creates is, so the original pg_depend check never matched it. Added the missing special case, plus a regression test proving the schema itself is now rejected too. Renamed internal_update__begin()/__end() to event_trigger__disable()/ event_trigger__enable(): the mechanism is a general disable-with-restore primitive, not something conceptually tied to "being mid-update" (only zzz__object_reference_drop, of the three event triggers, ever needs it). The backing temp table is now built via CTAS off pg_catalog.pg_event_trigger itself so the connection to that catalog's own evtname/evtenabled columns is visible in the code, not just a hand-typed column list that happened to reuse its names. Also expands the _etg_fix_identity/_etg_capture update-script comment to explain why recreating them is needed, not just what changed. Co-Authored-By: Claude Sonnet 5 --- test/expected/event_trigger_disable.out | 18 +++++ test/expected/internal_update.out | 18 ----- test/sql/event_trigger_disable.sql | 91 +++++++++++++++++++++++++ test/sql/internal_update.sql | 91 ------------------------- 4 files changed, 109 insertions(+), 109 deletions(-) create mode 100644 test/expected/event_trigger_disable.out delete mode 100644 test/expected/internal_update.out create mode 100644 test/sql/event_trigger_disable.sql delete mode 100644 test/sql/internal_update.sql diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out new file mode 100644 index 0000000..8a770ad --- /dev/null +++ b/test/expected/event_trigger_disable.out @@ -0,0 +1,18 @@ +\set ECHO none +1..15 +ok 1 - event_trigger__disable() disables the default trigger +ok 2 - zzz__object_reference_drop is disabled while a call is in effect +ok 3 - event_trigger__enable() re-enables it +ok 4 - zzz__object_reference_drop is back to its original (origin) state +ok 5 - manually disable zzz_object_reference_capture ahead of time +ok 6 - disable()/enable() round-trip on an already-disabled trigger +ok 7 - still disabled afterward -- its prior state was preserved, not assumed enabled +ok 8 - restore zzz_object_reference_capture for later tests +ok 9 - disable() the first time +ok 10 - a second disable() without enable() in between is rejected +ok 11 - enable() cleans up so later tests are unaffected +ok 12 - enable() without disable() is rejected +ok 13 - disable() rejects an unknown event trigger name +ok 14 - object_reference schema must not be part of the resolved search_path +ok 15 - _object_reference schema must not be part of the resolved search_path +# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/expected/internal_update.out b/test/expected/internal_update.out deleted file mode 100644 index f939ea7..0000000 --- a/test/expected/internal_update.out +++ /dev/null @@ -1,18 +0,0 @@ -\set ECHO none -1..15 -ok 1 - internal_update__begin() disables the default trigger -ok 2 - zzz__object_reference_drop is disabled while an internal update is in progress -ok 3 - internal_update__end() re-enables it -ok 4 - zzz__object_reference_drop is back to its original (origin) state -ok 5 - manually disable zzz_object_reference_capture ahead of time -ok 6 - begin()/end() round-trip on an already-disabled trigger -ok 7 - still disabled afterward -- its prior state was preserved, not assumed enabled -ok 8 - restore zzz_object_reference_capture for later tests -ok 9 - begin() the first time -ok 10 - a second begin() without end() in between is rejected -ok 11 - end() cleans up so later tests are unaffected -ok 12 - end() without begin() is rejected -ok 13 - begin() rejects an unknown event trigger name -ok 14 - object_reference schema must not be part of the resolved search_path -ok 15 - _object_reference schema must not be part of the resolved search_path -# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql new file mode 100644 index 0000000..793c512 --- /dev/null +++ b/test/sql/event_trigger_disable.sql @@ -0,0 +1,91 @@ +\set ECHO none + +\i test/load.sql + +SELECT plan( + 0 + +4 -- default disable/enable round-trip disables, then restores, the trigger + +4 -- disable/enable preserves a non-default prior state instead of assuming enabled + +3 -- nested disable() without an intervening enable() is rejected + +1 -- enable() without a matching disable() is rejected + +1 -- disable() rejects an unknown event trigger name + +2 -- schema-qualification (search_path) +); + +-- Default disable/enable round-trip +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable()$$ + , 'event_trigger__disable() disables the default trigger' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + , 'D' + , 'zzz__object_reference_drop is disabled while a call is in effect' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'event_trigger__enable() re-enables it' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + , 'O' + , 'zzz__object_reference_drop is back to its original (origin) state' +); + +-- Preserve a non-default prior state (already disabled for unrelated reasons) +SELECT lives_ok( + $$ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE$$ + , 'manually disable zzz_object_reference_capture ahead of time' +); +SELECT lives_ok( + $$ + SELECT _object_reference.event_trigger__disable('{zzz_object_reference_capture}'); + SELECT _object_reference.event_trigger__enable(); + $$ + , 'disable()/enable() round-trip on an already-disabled trigger' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz_object_reference_capture') + , 'D' + , 'still disabled afterward -- its prior state was preserved, not assumed enabled' +); +SELECT lives_ok( + $$ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE$$ + , 'restore zzz_object_reference_capture for later tests' +); + +-- Nested disable() without an intervening enable() +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable()$$ + , 'disable() the first time' +); +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__disable()$$ + , NULL + , 'event_trigger__disable() called while a previous call is still in effect' + , 'a second disable() without enable() in between is rejected' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'enable() cleans up so later tests are unaffected' +); + +-- enable() without a matching disable() +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , NULL + , 'event_trigger__enable() called without a matching event_trigger__disable()' + , 'enable() without disable() is rejected' +); + +-- Unknown event trigger name +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__disable('{no_such_event_trigger}')$$ + , NULL + , 'event trigger "no_such_event_trigger" does not exist' + , 'disable() rejects an unknown event trigger name' +); + +\i test/finish.sql + +-- vi: expandtab sw=2 ts=2 diff --git a/test/sql/internal_update.sql b/test/sql/internal_update.sql deleted file mode 100644 index e5c0c62..0000000 --- a/test/sql/internal_update.sql +++ /dev/null @@ -1,91 +0,0 @@ -\set ECHO none - -\i test/load.sql - -SELECT plan( - 0 - +4 -- default begin/end round-trip disables, then restores, the trigger - +4 -- begin/end preserves a non-default prior state instead of assuming enabled - +3 -- nested begin() without an intervening end() is rejected - +1 -- end() without a matching begin() is rejected - +1 -- begin() rejects an unknown event trigger name - +2 -- schema-qualification (search_path) -); - --- Default begin/end round-trip -SELECT lives_ok( - $$SELECT _object_reference.internal_update__begin()$$ - , 'internal_update__begin() disables the default trigger' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') - , 'D' - , 'zzz__object_reference_drop is disabled while an internal update is in progress' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_update__end()$$ - , 'internal_update__end() re-enables it' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') - , 'O' - , 'zzz__object_reference_drop is back to its original (origin) state' -); - --- Preserve a non-default prior state (already disabled for unrelated reasons) -SELECT lives_ok( - $$ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE$$ - , 'manually disable zzz_object_reference_capture ahead of time' -); -SELECT lives_ok( - $$ - SELECT _object_reference.internal_update__begin('{zzz_object_reference_capture}'); - SELECT _object_reference.internal_update__end(); - $$ - , 'begin()/end() round-trip on an already-disabled trigger' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz_object_reference_capture') - , 'D' - , 'still disabled afterward -- its prior state was preserved, not assumed enabled' -); -SELECT lives_ok( - $$ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE$$ - , 'restore zzz_object_reference_capture for later tests' -); - --- Nested begin() without an intervening end() -SELECT lives_ok( - $$SELECT _object_reference.internal_update__begin()$$ - , 'begin() the first time' -); -SELECT throws_ok( - $$SELECT _object_reference.internal_update__begin()$$ - , NULL - , 'internal_update__begin() called while already in an internal update' - , 'a second begin() without end() in between is rejected' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_update__end()$$ - , 'end() cleans up so later tests are unaffected' -); - --- end() without a matching begin() -SELECT throws_ok( - $$SELECT _object_reference.internal_update__end()$$ - , NULL - , 'internal_update__end() called without a matching internal_update__begin()' - , 'end() without begin() is rejected' -); - --- Unknown event trigger name -SELECT throws_ok( - $$SELECT _object_reference.internal_update__begin('{no_such_event_trigger}')$$ - , NULL - , 'event trigger "no_such_event_trigger" does not exist' - , 'begin() rejects an unknown event trigger name' -); - -\i test/finish.sql - --- vi: expandtab sw=2 ts=2 From 7ba1a939bfc6db543db04ea313a60fe4812c386a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 16 Sep 2026 17:10:31 -0500 Subject: [PATCH 3/9] Fix CI regression: test event_trigger__disable()/__enable() in isolation ALTER EVENT TRIGGER is visible database-wide the instant it runs, unlike the session_replication_role trick it replaces -- so the previous test, which called these against the real zzz_* triggers, raced with every other test file running concurrently in the same pg_regress batch: whichever one happened to run a DROP on a tracked object while zzz__object_reference_drop was mid-disable never got its tracking row cleaned up. Reproduced locally by running the suite's test files concurrently against a fresh database outside pg_regress's own scheduling, matching the exact CI failure. Rewired the test around two dummy event triggers it creates and owns itself, so exercising the mechanism never touches shared state any other test file depends on; the default target (zzz__object_reference_drop) is confirmed via pg_get_functiondef() instead of actually being invoked. Documented the database-wide-visibility tradeoff in both event_trigger.sql locations so a reader (or someone running ALTER EXTENSION UPDATE) knows not to expect session-local isolation from it. Co-Authored-By: Claude Sonnet 5 --- sql/object_reference--0.1.0--stable.sql | 100 ++++++++++++++++-------- sql/object_reference.sql | 60 ++++++++++---- test/build/expected/build.out | 10 +-- test/expected/base.out | 9 ++- test/expected/event_trigger_disable.out | 16 ++-- test/sql/base.sql | 8 +- test/sql/event_trigger_disable.sql | 76 ++++++++++-------- 7 files changed, 179 insertions(+), 100 deletions(-) diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index eb3f94f..b6e38b2 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -109,7 +109,7 @@ $body$; * __object_reference.exec() above, used by object__dependency__add() / * object_group__dependency__add() (unchanged since 0.1.0, but 0.1.0 never * created this permanent helper -- an existing gap this update closes) and - * by internal_update__begin()/__end() below. + * by event_trigger__disable()/__enable() below. */ SELECT __object_reference.create_function( '_object_reference.exec' @@ -146,28 +146,45 @@ SELECT EXISTS( AND d.refclassid = 'pg_catalog.pg_extension'::regclass AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') ) +/* + * The extension's own declared schema (object_reference) is a special + * case: CREATE EXTENSION records the EXTENSION as depending on it (a plain + * DEPENDENCY_NORMAL row, extension -> schema), not the schema as an 'e' + * member of the extension the way every other object it creates is -- so + * it never matches the pg_depend check above. + */ +OR ( + _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass + AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); /* - * New: internal update-time disable/enable mechanism, replacing the - * session_replication_role trick 0.1.0 had no equivalent of. 0.1.0 already - * installed this extension's own event triggers, and they stay active for - * the rest of THIS session while the structural changes below run. - * zzz__object_reference_drop in particular queries _object_reference._object_v - * inside its own body, so it would fire -- and error, since the view is - * momentarily gone -- the instant this script drops that view a few - * statements down. The other two event triggers self-recognize and skip our - * own script's DDL instead (see _etg_fix_identity/_etg_capture below), so - * only zzz__object_reference_drop needs to be disabled here. + * New: general-purpose event-trigger disable/enable mechanism, replacing + * the session_replication_role trick 0.1.0 had no equivalent of. 0.1.0 + * already installed this extension's own event triggers, and they stay + * active for the rest of THIS session while the structural changes below + * run. zzz__object_reference_drop in particular queries + * _object_reference._object_v inside its own body, so it would fire -- + * and error, since the view is momentarily gone -- the instant this + * script drops that view a few statements down. The other two event + * triggers self-recognize and skip our own script's DDL instead (see + * _etg_fix_identity/_etg_capture below), so only zzz__object_reference_drop + * needs to be disabled here. * * These are created now, ahead of the structural section, specifically so - * this script itself can call internal_update__begin() below -- a fresh - * install only ever needs these for FUTURE update scripts. + * this script itself can call event_trigger__disable() below -- a fresh + * install only ever needs these for FUTURE update scripts, or anywhere + * else a future need to safely quiet an event trigger comes up (hence the + * mechanism-focused name, not one tied to "being mid-update"). Unlike + * session_replication_role, disabling this way is visible database-wide + * the instant it runs, not just to this script's own session -- run + * updates without concurrent DDL on tracked objects for this reason. */ SELECT __object_reference.create_function( - '_object_reference.internal_update__begin' + '_object_reference.event_trigger__disable' , $args$ event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' $args$ @@ -177,18 +194,23 @@ DECLARE v_name name; BEGIN BEGIN - CREATE TEMP TABLE __object_reference__internal_update( - evtname name PRIMARY KEY - , evtenabled "char" NOT NULL - ); + /* + * Mirrors pg_event_trigger's own evtname/evtenabled columns (via CTAS, + * so the connection to that catalog is visible in the code, not just a + * hand-typed column list that happens to reuse its names) -- this is + * exactly the row this extension needs to restore later. + */ + CREATE TEMP TABLE __object_reference__event_trigger_state AS + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false; + ALTER TABLE pg_temp.__object_reference__event_trigger_state ADD PRIMARY KEY (evtname); EXCEPTION WHEN duplicate_table THEN - RAISE 'internal_update__begin() called while already in an internal update' - USING HINT = 'A previous internal_update__end() call may have been skipped.' + RAISE 'event_trigger__disable() called while a previous call is still in effect' + USING HINT = 'A previous event_trigger__enable() call may have been skipped.' ; END; FOREACH v_name IN ARRAY event_trigger_names LOOP - INSERT INTO pg_temp.__object_reference__internal_update(evtname, evtenabled) + INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; IF NOT FOUND THEN @@ -199,17 +221,17 @@ BEGIN END LOOP; END $body$ - , 'Disable the given (or default) event triggers for the duration of this extension''s own internal update; pair with internal_update__end().' + , 'Disable the given (or default) event triggers, remembering their exact prior state; pair with event_trigger__enable().' ); SELECT __object_reference.create_function( - '_object_reference.internal_update__end' + '_object_reference.event_trigger__enable' , '' , 'void LANGUAGE plpgsql' , $body$ DECLARE r record; BEGIN - FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__internal_update LOOP + FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__event_trigger_state LOOP PERFORM _object_reference.exec(format( 'ALTER EVENT TRIGGER %I %s' , r.evtname @@ -222,15 +244,15 @@ BEGIN )); END LOOP; - DROP TABLE pg_temp.__object_reference__internal_update; + DROP TABLE pg_temp.__object_reference__event_trigger_state; EXCEPTION WHEN undefined_table THEN - RAISE 'internal_update__end() called without a matching internal_update__begin()'; + RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; END $body$ - , 'Restore event triggers disabled by internal_update__begin() to their prior enabled state.' + , 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.' ); -SELECT _object_reference.internal_update__begin(); +SELECT _object_reference.event_trigger__disable(); /* * _object_reference.object: no column changes, just a missing @@ -527,10 +549,20 @@ $body$ /* * _etg_fix_identity/_etg_capture: gain a self-recognition guard so they skip * DDL issued by any extension's own install/update script (ours included) - * instead of reacting to it -- see internal_update__begin()/__end() above - * for why zzz__object_reference_drop needs a different mechanism. Same - * signatures as 0.1.0, so a plain CREATE OR REPLACE (via create_function) is - * enough -- no DROP needed. + * instead of reacting to it. Without it, _etg_capture would try to call + * _object_reference._object_v__for_update() (the FUNCTION) to register any + * CREATE-tagged command in this very script -- including a moment where + * that function has been dropped and not yet recreated (see the structural + * section below), which would fail outright if a capture happened to be + * active during an extension update; _etg_fix_identity would otherwise run + * its blanket identity-recompute pass on every one of this script's many + * DDL statements for no reason, since nothing it touches is (or, after this + * update's self-tracking guard, ever legitimately can be) one of this + * extension's own tracked rows. zzz__object_reference_drop can't + * self-recognize the same way; see event_trigger__disable()/__enable() + * above for why it needs a different mechanism. Same signatures as 0.1.0, + * so a plain CREATE OR REPLACE (via create_function) is enough -- no DROP + * needed. */ SELECT __object_reference.create_function( '_object_reference._etg_fix_identity' @@ -902,9 +934,9 @@ DROP SCHEMA __object_reference; /* * Re-enable zzz__object_reference_drop (to its actual prior state, saved by - * internal_update__begin() near the top of this script), now that the + * event_trigger__disable() near the top of this script), now that the * structural section and its cleanup are both done. */ -SELECT _object_reference.internal_update__end(); +SELECT _object_reference.event_trigger__enable(); -- vi: expandtab sw=2 ts=2 diff --git a/sql/object_reference.sql b/sql/object_reference.sql index 68b1cdb..d9a4f7a 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -178,6 +178,17 @@ SELECT EXISTS( AND d.refclassid = 'pg_catalog.pg_extension'::regclass AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') ) +/* + * The extension's own declared schema (object_reference) is a special + * case: CREATE EXTENSION records the EXTENSION as depending on it (a plain + * DEPENDENCY_NORMAL row, extension -> schema), not the schema as an 'e' + * member of the extension the way every other object it creates is -- so + * it never matches the pg_depend check above. + */ +OR ( + _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass + AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); @@ -1561,13 +1572,16 @@ $body$ ); /* - * Internal update-time disable/enable mechanism, for use by this extension's - * OWN install/update scripts only (not part of the public API). + * General-purpose event-trigger disable/enable mechanism, for use by this + * extension's OWN install/update scripts only (not part of the public API). + * Not tied to "being mid-update" specifically -- it's a plain disable-with- + * restore primitive for any event trigger that can't self-recognize (via + * in_extension, see below) that it should stay quiet. * * zzz_object_reference__fix_identity and zzz_object_reference_capture can * recognize (and skip) DDL issued by any extension's own script via * pg_event_trigger_ddl_commands()'s in_extension column, so they never need - * to be disabled. zzz__object_reference_drop cannot: it fires from + * to be disabled this way. zzz__object_reference_drop cannot: it fires from * pg_event_trigger_dropped_objects(), which has no equivalent column, and it * queries _object_reference._object_v -- a view an update script may itself * be dropping and recreating -- so it must be truly disabled for the @@ -1576,9 +1590,16 @@ $body$ * ALTER EVENT TRIGGER is ordinary transactional DDL, so if the calling * script's transaction rolls back, the DISABLE (and any ENABLE already run) * rolls back with it -- no separate cleanup-on-error logic is needed here. + * + * Unlike the session_replication_role trick this replaces, disabling an + * event trigger this way is visible database-wide the instant it runs, not + * just to the calling session -- any other session's DDL on a tracked + * object during that window also won't fire the disabled trigger. Update + * scripts are expected to run without concurrent DDL on tracked objects for + * exactly this reason. */ SELECT __object_reference.create_function( - '_object_reference.internal_update__begin' + '_object_reference.event_trigger__disable' , $args$ event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' $args$ @@ -1588,18 +1609,23 @@ DECLARE v_name name; BEGIN BEGIN - CREATE TEMP TABLE __object_reference__internal_update( - evtname name PRIMARY KEY - , evtenabled "char" NOT NULL - ); + /* + * Mirrors pg_event_trigger's own evtname/evtenabled columns (via CTAS, + * so the connection to that catalog is visible in the code, not just a + * hand-typed column list that happens to reuse its names) -- this is + * exactly the row this extension needs to restore later. + */ + CREATE TEMP TABLE __object_reference__event_trigger_state AS + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false; + ALTER TABLE pg_temp.__object_reference__event_trigger_state ADD PRIMARY KEY (evtname); EXCEPTION WHEN duplicate_table THEN - RAISE 'internal_update__begin() called while already in an internal update' - USING HINT = 'A previous internal_update__end() call may have been skipped.' + RAISE 'event_trigger__disable() called while a previous call is still in effect' + USING HINT = 'A previous event_trigger__enable() call may have been skipped.' ; END; FOREACH v_name IN ARRAY event_trigger_names LOOP - INSERT INTO pg_temp.__object_reference__internal_update(evtname, evtenabled) + INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; IF NOT FOUND THEN @@ -1610,17 +1636,17 @@ BEGIN END LOOP; END $body$ - , 'Disable the given (or default) event triggers for the duration of this extension''s own internal update; pair with internal_update__end().' + , 'Disable the given (or default) event triggers, remembering their exact prior state; pair with event_trigger__enable().' ); SELECT __object_reference.create_function( - '_object_reference.internal_update__end' + '_object_reference.event_trigger__enable' , '' , 'void LANGUAGE plpgsql' , $body$ DECLARE r record; BEGIN - FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__internal_update LOOP + FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__event_trigger_state LOOP PERFORM _object_reference.exec(format( 'ALTER EVENT TRIGGER %I %s' , r.evtname @@ -1633,12 +1659,12 @@ BEGIN )); END LOOP; - DROP TABLE pg_temp.__object_reference__internal_update; + DROP TABLE pg_temp.__object_reference__event_trigger_state; EXCEPTION WHEN undefined_table THEN - RAISE 'internal_update__end() called without a matching internal_update__begin()'; + RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; END $body$ - , 'Restore event triggers disabled by internal_update__begin() to their prior enabled state.' + , 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.' ); SELECT __object_reference.create_function( diff --git a/test/build/expected/build.out b/test/build/expected/build.out index 0e62ce5..ac88a11 100644 --- a/test/build/expected/build.out +++ b/test/build/expected/build.out @@ -3,16 +3,16 @@ This extension must be loaded via CREATE EXTENSION object_reference; You really, REALLY do NOT want to try and load this via psql!!! -psql:test/temp_load.not_sql:197: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:208: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:198: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:209: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:446: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:457: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! @@ -22,9 +22,9 @@ psql:test/temp_load.not_sql:446: WARNING: I promise you will be sorry if you tr -psql:test/temp_load.not_sql:558: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:569: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:565: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:576: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! diff --git a/test/expected/base.out b/test/expected/base.out index f2fa6f4..6c73346 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,5 +1,5 @@ \set ECHO none -1..14 +1..15 ok 1 - Role object_reference__dependency should be granted USAGE on schema _object_reference ok 2 - Role object_reference__dependency should be granted REFERENCES on table _object_reference.object ok 3 - CREATE TEMP TABLE test_object AS SELECT object_reference.object__getsert('table', 'test_table') AS object_id; @@ -11,7 +11,8 @@ ok 8 - secondary may not be specified for table objects ok 9 - temp objects are rejected ok 10 - own tracking table is rejected ok 11 - own event trigger function is rejected -ok 12 - CREATE EXTENSION test_factory -ok 13 - object_reference schema must not be part of the resolved search_path -ok 14 - _object_reference schema must not be part of the resolved search_path +ok 12 - own declared schema is rejected (extension depends on it, not the other way around) +ok 13 - CREATE EXTENSION test_factory +ok 14 - object_reference schema must not be part of the resolved search_path +ok 15 - _object_reference schema must not be part of the resolved search_path # TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out index 8a770ad..c6c7793 100644 --- a/test/expected/event_trigger_disable.out +++ b/test/expected/event_trigger_disable.out @@ -1,13 +1,13 @@ \set ECHO none 1..15 -ok 1 - event_trigger__disable() disables the default trigger -ok 2 - zzz__object_reference_drop is disabled while a call is in effect -ok 3 - event_trigger__enable() re-enables it -ok 4 - zzz__object_reference_drop is back to its original (origin) state -ok 5 - manually disable zzz_object_reference_capture ahead of time -ok 6 - disable()/enable() round-trip on an already-disabled trigger -ok 7 - still disabled afterward -- its prior state was preserved, not assumed enabled -ok 8 - restore zzz_object_reference_capture for later tests +ok 1 - default event trigger to disable is zzz__object_reference_drop +ok 2 - manually disable test trigger b ahead of time +ok 3 - disable() both test triggers +ok 4 - test trigger a is disabled while a call is in effect +ok 5 - test trigger b is (still) disabled while a call is in effect +ok 6 - enable() restores both +ok 7 - test trigger a is back to its original (origin) state +ok 8 - test trigger b is still disabled -- its prior state was preserved, not assumed enabled ok 9 - disable() the first time ok 10 - a second disable() without enable() in between is rejected ok 11 - enable() cleans up so later tests are unaffected diff --git a/test/sql/base.sql b/test/sql/base.sql index 7c8dd7e..79ccd90 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -9,7 +9,7 @@ SELECT plan( +1 -- schema +3 -- initial +2 -- new functions - +5 -- errors (includes temp object + self-tracking rejection tests) + +6 -- errors (includes temp object + self-tracking rejection tests) +1 -- create extensions +2 -- schema-qualification (search_path) ); @@ -87,6 +87,12 @@ SELECT throws_ok( , 'cannot track an object that is a member of the object_reference extension itself' , 'own event trigger function is rejected' ); +SELECT throws_ok( + $$SELECT object_reference.object__getsert('schema', 'object_reference')$$ + , '0A000' -- feature_not_supported + , 'cannot track an object that is a member of the object_reference extension itself' + , 'own declared schema is rejected (extension depends on it, not the other way around)' +); -- Create extensions SELECT lives_ok( diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql index 793c512..63435ae 100644 --- a/test/sql/event_trigger_disable.sql +++ b/test/sql/event_trigger_disable.sql @@ -2,65 +2,79 @@ \i test/load.sql +/* + * event_trigger__disable()/__enable() are ALTER EVENT TRIGGER under the + * hood, which is a database-wide change visible to every session the + * instant it runs (unlike the session-local session_replication_role trick + * it replaces) -- so this test exercises the mechanism against its OWN + * dummy event triggers, never the real zzz_* ones other test files in this + * same parallel run depend on staying enabled. + */ +CREATE FUNCTION event_trigger_disable_test__noop() RETURNS event_trigger LANGUAGE plpgsql AS $$ +BEGIN +END +$$; +CREATE EVENT TRIGGER event_trigger_disable_test__a ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); +CREATE EVENT TRIGGER event_trigger_disable_test__b ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); + SELECT plan( 0 - +4 -- default disable/enable round-trip disables, then restores, the trigger - +4 -- disable/enable preserves a non-default prior state instead of assuming enabled + +1 -- default target is zzz__object_reference_drop + +7 -- multi-trigger disable/enable preserves each one's own prior state +3 -- nested disable() without an intervening enable() is rejected +1 -- enable() without a matching disable() is rejected +1 -- disable() rejects an unknown event trigger name +2 -- schema-qualification (search_path) ); --- Default disable/enable round-trip +-- Default target (checked via source, never invoked against a real trigger) +SELECT matches( + pg_catalog.pg_get_functiondef('_object_reference.event_trigger__disable(name[])'::regprocedure) + , 'zzz__object_reference_drop' + , 'default event trigger to disable is zzz__object_reference_drop' +); + +-- Multi-trigger disable/enable, preserving each trigger's own prior state +SELECT lives_ok( + $$ALTER EVENT TRIGGER event_trigger_disable_test__b DISABLE$$ + , 'manually disable test trigger b ahead of time' +); SELECT lives_ok( - $$SELECT _object_reference.event_trigger__disable()$$ - , 'event_trigger__disable() disables the default trigger' + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a,event_trigger_disable_test__b}')$$ + , 'disable() both test triggers' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') + , 'D' + , 'test trigger a is disabled while a call is in effect' ); SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') , 'D' - , 'zzz__object_reference_drop is disabled while a call is in effect' + , 'test trigger b is (still) disabled while a call is in effect' ); SELECT lives_ok( $$SELECT _object_reference.event_trigger__enable()$$ - , 'event_trigger__enable() re-enables it' + , 'enable() restores both' ); SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz__object_reference_drop') + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') , 'O' - , 'zzz__object_reference_drop is back to its original (origin) state' -); - --- Preserve a non-default prior state (already disabled for unrelated reasons) -SELECT lives_ok( - $$ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE$$ - , 'manually disable zzz_object_reference_capture ahead of time' -); -SELECT lives_ok( - $$ - SELECT _object_reference.event_trigger__disable('{zzz_object_reference_capture}'); - SELECT _object_reference.event_trigger__enable(); - $$ - , 'disable()/enable() round-trip on an already-disabled trigger' + , 'test trigger a is back to its original (origin) state' ); SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'zzz_object_reference_capture') + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') , 'D' - , 'still disabled afterward -- its prior state was preserved, not assumed enabled' -); -SELECT lives_ok( - $$ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE$$ - , 'restore zzz_object_reference_capture for later tests' + , 'test trigger b is still disabled -- its prior state was preserved, not assumed enabled' ); -- Nested disable() without an intervening enable() SELECT lives_ok( - $$SELECT _object_reference.event_trigger__disable()$$ + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ , 'disable() the first time' ); SELECT throws_ok( - $$SELECT _object_reference.event_trigger__disable()$$ + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ , NULL , 'event_trigger__disable() called while a previous call is still in effect' , 'a second disable() without enable() in between is rejected' From f0babc023a93e5b8ec9de09dd9a83c175732d78f Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 15:38:56 -0500 Subject: [PATCH 4/9] Address review: fix real races/bugs found via empirical testing + reviews Empirically tested (via a dispatched agent) whether ALTER EVENT TRIGGER DISABLE is visible to other sessions or blocks their DDL: neither is true -- it's ordinary transactional DDL, invisible to other sessions until commit, taking no lock on the trigger itself. RENAME TO is not usable as a locking primitive either; it errors immediately as a duplicate name. This corrects an inaccurate "visible database-wide" claim the previous round's comments made. Fixed the real race that remains (two sessions altering the SAME trigger concurrently) with a FOR UPDATE lock in event_trigger__disable() before reading each trigger's prior state. Replaced the in_extension self-recognition check (which matched DDL from ANY extension's script, not just this one -- a real behavior change bug a bot review caught) with a session-local check instead: zzz_object_reference__fix_identity and zzz_object_reference_capture now check whether an event_trigger__disable() call is currently in effect for their own session, precisely and without any cross-extension false positives. Fixed the resulting ordering requirement (their guarded bodies must be installed before event_trigger__disable() is first called, not after -- another real bug the same review caught) by moving their recreation earlier in the update script. Found and fixed two more crashes via manual testing beyond either review, reproducing the "capture active during ALTER EXTENSION UPDATE" scenario end-to-end: (1) event_trigger__enable()'s own cleanup DROP of its bookkeeping temp table fired zzz__object_reference_drop the instant it was re-enabled, cascading into a hard error over unrelated stale state -- fixed by dropping the table before re-enabling anything. (2) The update script's own bootstrap statements (creating its __object_reference scratch schema) run before any guard exists to protect them, so an active capture group would capture them; fixed with a raw ALTER EVENT TRIGGER DISABLE/ ENABLE bracket around just those few statements, plus excluding object_reference/_object_reference/__object_reference from what _etg_capture will ever register (a newly-created member of this extension's own update script isn't recorded as an 'e' pg_depend member yet at the moment it's created, so _is_own_object() can't catch it there either). Also: removed the DEFAULT from event_trigger__disable()'s signature (callers must now explicitly name what they're disabling); rewrote its test to exercise dummy event triggers it owns instead of the real zzz_* ones, verified via a genuine fresh vs. update-mode pg_regress run outside the suite's own parallel batch (isolating a real cross-test race the previous round's fix only partially addressed); added a warning at the top of event_trigger__disable()'s own body and the comment above it; added the missing _is_own_object() special case for the extension's own pg_extension row; added a heavy-weight-use warning to _object_v__for_update()'s comment. Co-Authored-By: Claude Sonnet 5 --- sql/object_reference--0.1.0--stable.sql | 431 +++++++++++++++--------- sql/object_reference.sql | 171 +++++++--- test/build/expected/build.out | 10 +- test/expected/event_trigger_disable.out | 36 +- test/sql/event_trigger_disable.sql | 54 ++- 5 files changed, 463 insertions(+), 239 deletions(-) diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index b6e38b2..26a2c6c 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -1,3 +1,20 @@ +/* + * Immediately disable zzz_object_reference_capture and + * zzz_object_reference__fix_identity (raw ALTER EVENT TRIGGER -- nothing + * else, not even the __object_reference bootstrap schema below, exists yet + * to route through) for the handful of bootstrap statements that follow: + * their OLD (0.1.0) bodies have no way to recognize "this is our own + * update script" until their guarded replacements are installed a few + * statements down, and CREATE SCHEMA __object_reference is exactly the + * kind of CREATE-tagged statement _etg_capture would otherwise try (and + * fail) to register into any capture group active during this update. + * Assumed 'O' (origin) on the restore below rather than captured and + * restored precisely: 0.1.0 always creates both this way, and nothing else + * in this extension ever changes it before an update runs. + */ +ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE; +ALTER EVENT TRIGGER zzz_object_reference__fix_identity DISABLE; + /* * Uses a private __object_reference schema, mirroring * sql/object_reference.sql's own bootstrap/teardown convention, so every @@ -157,51 +174,237 @@ OR ( _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') ) +/* + * The extension's own pg_extension row is also its own special case: it + * isn't a member of itself (no 'e' row with itself as both member and + * owner), so treat it as one explicitly. + */ +OR ( + _is_own_object.classid = 'pg_catalog.pg_extension'::regclass + AND _is_own_object.objid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); /* - * New: general-purpose event-trigger disable/enable mechanism, replacing - * the session_replication_role trick 0.1.0 had no equivalent of. 0.1.0 - * already installed this extension's own event triggers, and they stay - * active for the rest of THIS session while the structural changes below - * run. zzz__object_reference_drop in particular queries - * _object_reference._object_v inside its own body, so it would fire -- - * and error, since the view is momentarily gone -- the instant this - * script drops that view a few statements down. The other two event - * triggers self-recognize and skip our own script's DDL instead (see - * _etg_fix_identity/_etg_capture below), so only zzz__object_reference_drop - * needs to be disabled here. + * _etg_fix_identity/_etg_capture: gain a self-recognition guard so they skip + * work while this extension's own event_trigger__disable() call is in + * effect for this session (checked via to_regclass() on the temp table + * event_trigger__disable() creates below) -- i.e. this extension's own + * install/update script is doing delicate internal restructuring right + * now. Installed here, ahead of the structural section below, specifically + * so the guard is already active by the time event_trigger__disable() is + * first called a few statements down: recreating them any later would + * leave the OLD (0.1.0), unguarded bodies live for that whole window -- + * which mattered in practice for _etg_capture, which would otherwise try + * to call _object_reference._object_v__for_update() (the FUNCTION) for any + * CREATE-tagged command in this script if a capture happened to be active, + * including a moment where that function has been dropped and not yet + * recreated, which would fail outright. Same signatures as 0.1.0, so a + * plain CREATE OR REPLACE (via create_function) is enough -- no DROP + * needed. + */ +SELECT __object_reference.create_function( + '_object_reference._etg_capture' + , '' + , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' + , $body$ +DECLARE + c_group_id CONSTANT int := object_group_id FROM object_reference.capture__get_current(); + r record; +BEGIN + /* + * Self-recognition: skip while this extension's own event_trigger__disable() + * is in effect (see below) -- i.e. this extension's own install/update + * script is doing delicate internal restructuring right now. Checked via + * to_regclass() rather than a catalog lookup that would error if the temp + * table doesn't exist, which is the common case. + */ + IF to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL THEN + RETURN; + END IF; + + IF c_group_id IS NOT NULL THEN -- Would be NULL if table is empty + RAISE DEBUG E'\n\n*** START ***'; + BEGIN + FOR r IN + SELECT classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension + -- Have to manually exclude command field :/ + FROM pg_catalog.pg_event_trigger_ddl_commands() + LOOP + RAISE DEBUG 'ddl: %', row_to_json(r); + END LOOP; + END; + + FOR r IN SELECT + _object_reference._object_v__for_update( + object_type::cat_tools.object_type + , objid, objsubid + , c_group_id + , classid + ) + , classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension + FROM pg_catalog.pg_event_trigger_ddl_commands() + WHERE command_tag ~ '^CREATE' --'^(ALTER|CREATE)' + AND NOT object_reference.unsupported(object_type::cat_tools.object_type) + AND (schema_name IS NULL + OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it... + ) + /* + * __object_reference is this extension's own scratch install/update + * schema (created and dropped within a single script, never an + * extension member) -- self-recognition via the temp table above + * can't cover the handful of bootstrap statements that run before + * that table exists, so exclude it here too (object_identity + * carries the name for the CREATE SCHEMA statement itself, where + * schema_name is null). + * + * object_reference/_object_reference are excluded outright rather + * than relying on _object_v__for_update()'s own _is_own_object() + * guard: a brand-new object created by this extension's own + * update/install script isn't yet recorded as an 'e' member in + * pg_depend at the point its CREATE fires ddl_command_end (that + * happens once the surrounding CREATE/ALTER EXTENSION completes), + * so _is_own_object() can't see it as self-owned yet either -- + * confirmed by running into it: an active capture group during + * ALTER EXTENSION UPDATE otherwise ends up with this extension's + * own new functions as members. + */ + AND coalesce(schema_name, object_identity, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') + LOOP + RAISE DEBUG 'registered %', row_to_json(r); + END LOOP; + RAISE DEBUG E'*** END ***\n\n'; + END IF; +END +$body$ + , 'Event trigger function to capture newly created objects in an object group.' +); +SELECT __object_reference.create_function( + '_object_reference._etg_fix_identity' + , '' + , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' + , $body$ +DECLARE + r_ddl record; + r record; +BEGIN + /* + * Self-recognition: skip while this extension's own event_trigger__disable() + * is in effect (see below) -- i.e. this extension's own install/update + * script is doing delicate internal restructuring right now. Checked via + * to_regclass() rather than a catalog lookup that would error if the temp + * table doesn't exist, which is the common case. + */ + IF to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL THEN + RETURN; + END IF; + + /* + * It's tempting to use pg_event_trigger_ddl_commands() to find exactly what + * items have changed and worry about only those. That won't work because an + * object_names array can depend on multiple names (ie: a column depends on + * the name of it's table, as well as the name of the schema the table is in. + * You might think we could simply recurse through pg_depend to handle this, + * but not every name dependency gets enumerated that way. For example, + * columns are not marked as dependent on their table. + * + * Rather than trying to be cute about this, we just do a brute-force check + * for any names that have changed. + */ + + /* + * Presumably there's no way for an objects type/classid to change, but be + * safe and attempt the update to object_type. If it actually does change the + * constraint on the table should catch it. + */ + FOR r IN + UPDATE _object_reference.object + SET object_type = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).type::cat_tools.object_type + , object_names = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_names + , object_args = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_args + FROM _object_reference._object_oid oo + WHERE + oo.object_id = object.object_id + AND (object_type::text, object_names, object_args) IS DISTINCT FROM + (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)) + RETURNING * + LOOP + RAISE DEBUG 'modified_objects(): %', r; + END LOOP; +END +$body$ + , 'Event trigger function to update any records with object names or args that have changed.' +); + +/* + * Re-enable now that the guarded bodies above are live -- self-recognition + * (checking for event_trigger__disable()'s temp table, created below) takes + * over from here for the rest of this script. + */ +ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE; +ALTER EVENT TRIGGER zzz_object_reference__fix_identity ENABLE; + +/* + * WARNING: avoid disabling event triggers at all where any other option + * exists. ALTER EVENT TRIGGER is ordinary transactional DDL -- like any + * other catalog write, it's invisible to other sessions until commit (no + * special database-wide/immediate effect: verified empirically that a + * concurrent session's DDL neither blocks on, nor is otherwise affected by, + * another session's still-uncommitted DISABLE) and it takes no lock at all + * on the event trigger itself. The real risk is TWO SESSIONS both trying to + * alter the SAME event trigger concurrently: a second writer blocks on the + * first the way any two concurrent writes to the same catalog row would, + * and without care, the one that unblocks second can record and later + * restore a "prior state" that was never actually the trigger's state + * immediately before it acted (see the FOR UPDATE lock in + * event_trigger__disable()'s body below, which exists specifically to close + * that gap). Prefer a self-recognition check (a session-local flag, checked + * from inside the trigger's own body, as _etg_fix_identity/_etg_capture + * above do) over calling this at all; reach for it only when nothing else + * can make the trigger stay quiet, as is currently true for + * zzz__object_reference_drop. + * + * General-purpose event-trigger disable/enable mechanism, replacing the + * session_replication_role trick 0.1.0 had no equivalent of. 0.1.0 already + * installed this extension's own event triggers, and they stay active for + * the rest of THIS session while the structural changes below run. + * zzz__object_reference_drop in particular queries _object_reference._object_v + * inside its own body, so it would fire -- and error, since the view is + * momentarily gone -- the instant this script drops that view a few + * statements down. It can't self-recognize the way _etg_fix_identity/ + * _etg_capture above do without also touching that same view from inside + * its own body, so it must be truly disabled for the duration of this + * script's structural section. * * These are created now, ahead of the structural section, specifically so * this script itself can call event_trigger__disable() below -- a fresh * install only ever needs these for FUTURE update scripts, or anywhere * else a future need to safely quiet an event trigger comes up (hence the - * mechanism-focused name, not one tied to "being mid-update"). Unlike - * session_replication_role, disabling this way is visible database-wide - * the instant it runs, not just to this script's own session -- run - * updates without concurrent DDL on tracked objects for this reason. + * mechanism-focused name, not one tied to "being mid-update"). */ SELECT __object_reference.create_function( '_object_reference.event_trigger__disable' , $args$ - event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' + event_trigger_names name[] $args$ , 'void LANGUAGE plpgsql' , $body$ DECLARE v_name name; + v_enabled "char"; BEGIN + /* + * WARNING: avoid disabling event triggers at all where any other option + * exists -- this is a database-wide change with real race-condition risk + * against concurrent sessions' DDL. See the warning above this function. + */ BEGIN - /* - * Mirrors pg_event_trigger's own evtname/evtenabled columns (via CTAS, - * so the connection to that catalog is visible in the code, not just a - * hand-typed column list that happens to reuse its names) -- this is - * exactly the row this extension needs to restore later. - */ + -- Save old trigger state CREATE TEMP TABLE __object_reference__event_trigger_state AS - SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false; + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false + ; ALTER TABLE pg_temp.__object_reference__event_trigger_state ADD PRIMARY KEY (evtname); EXCEPTION WHEN duplicate_table THEN RAISE 'event_trigger__disable() called while a previous call is still in effect' @@ -210,18 +413,31 @@ BEGIN END; FOREACH v_name IN ARRAY event_trigger_names LOOP - INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) - SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; + /* + * FOR UPDATE locks the row before we read it, so no other session's own + * ALTER EVENT TRIGGER on the same trigger can land between our read and + * our DISABLE below -- without it, a concurrent change there would + * leave us recording (and later restoring) a state that was never + * actually the trigger's state immediately before we disabled it. + */ + SELECT evtenabled INTO v_enabled + FROM pg_catalog.pg_event_trigger + WHERE evtname = v_name + FOR UPDATE + ; IF NOT FOUND THEN RAISE 'event trigger "%" does not exist', v_name; END IF; + INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) + VALUES (v_name, v_enabled); + PERFORM _object_reference.exec(format('ALTER EVENT TRIGGER %I DISABLE', v_name)); END LOOP; END $body$ - , 'Disable the given (or default) event triggers, remembering their exact prior state; pair with event_trigger__enable().' + , 'Disable the given event triggers, remembering their exact prior state; pair with event_trigger__enable().' ); SELECT __object_reference.create_function( '_object_reference.event_trigger__enable' @@ -229,13 +445,36 @@ SELECT __object_reference.create_function( , 'void LANGUAGE plpgsql' , $body$ DECLARE - r record; + v_names name[]; + v_states "char"[]; + i int; BEGIN - FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__event_trigger_state LOOP + BEGIN + SELECT array_agg(evtname), array_agg(evtenabled) + INTO v_names, v_states + FROM pg_temp.__object_reference__event_trigger_state + ; + EXCEPTION WHEN undefined_table THEN + RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; + END; + + /* + * Drop our own bookkeeping table BEFORE re-enabling anything below: + * dropping it is itself DDL, and if zzz__object_reference_drop is one of + * the triggers being restored here, re-enabling it first would make this + * DROP immediately fire it -- reacting to our own internal cleanup, + * exactly the hazard this whole mechanism exists to avoid. (Confirmed by + * running into it: with the table dropped after, an active capture group + * elsewhere left a stale tracked row that this DROP's cascade into + * post_restore() then found and errored on.) + */ + DROP TABLE pg_temp.__object_reference__event_trigger_state; + + FOR i IN 1..coalesce(array_length(v_names, 1), 0) LOOP PERFORM _object_reference.exec(format( 'ALTER EVENT TRIGGER %I %s' - , r.evtname - , CASE r.evtenabled + , v_names[i] + , CASE v_states[i] WHEN 'O' THEN 'ENABLE' WHEN 'R' THEN 'ENABLE REPLICA' WHEN 'A' THEN 'ENABLE ALWAYS' @@ -243,16 +482,12 @@ BEGIN END )); END LOOP; - - DROP TABLE pg_temp.__object_reference__event_trigger_state; -EXCEPTION WHEN undefined_table THEN - RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; END $body$ , 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.' ); -SELECT _object_reference.event_trigger__disable(); +SELECT _object_reference.event_trigger__disable('{zzz__object_reference_drop}'); /* * _object_reference.object: no column changes, just a missing @@ -495,7 +730,7 @@ BEGIN RETURN r_object_v; END $body$ - , 'Return details of a object record, creating a new record if one does not exist.' + , 'Return details of a object record, creating a new record if one does not exist. Heavy-weight compared to a plain read of _object_reference._object_v -- use that instead when an existing record is all that''s needed.' ); SELECT __object_reference.create_function( @@ -546,130 +781,6 @@ $body$ , 'Check the sanity of object and _object_oid' ); -/* - * _etg_fix_identity/_etg_capture: gain a self-recognition guard so they skip - * DDL issued by any extension's own install/update script (ours included) - * instead of reacting to it. Without it, _etg_capture would try to call - * _object_reference._object_v__for_update() (the FUNCTION) to register any - * CREATE-tagged command in this very script -- including a moment where - * that function has been dropped and not yet recreated (see the structural - * section below), which would fail outright if a capture happened to be - * active during an extension update; _etg_fix_identity would otherwise run - * its blanket identity-recompute pass on every one of this script's many - * DDL statements for no reason, since nothing it touches is (or, after this - * update's self-tracking guard, ever legitimately can be) one of this - * extension's own tracked rows. zzz__object_reference_drop can't - * self-recognize the same way; see event_trigger__disable()/__enable() - * above for why it needs a different mechanism. Same signatures as 0.1.0, - * so a plain CREATE OR REPLACE (via create_function) is enough -- no DROP - * needed. - */ -SELECT __object_reference.create_function( - '_object_reference._etg_fix_identity' - , '' - , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' - , $body$ -DECLARE - r_ddl record; - r record; -BEGIN - /* - * Self-recognition: skip DDL issued by any extension's own install/update - * script (ours included); pg_event_trigger_ddl_commands() marks this via - * in_extension, unlike pg_event_trigger_dropped_objects() (see _etg_drop). - */ - IF EXISTS(SELECT 1 FROM pg_catalog.pg_event_trigger_ddl_commands() WHERE in_extension) THEN - RETURN; - END IF; - - /* - * It's tempting to use pg_event_trigger_ddl_commands() to find exactly what - * items have changed and worry about only those. That won't work because an - * object_names array can depend on multiple names (ie: a column depends on - * the name of it's table, as well as the name of the schema the table is in. - * You might think we could simply recurse through pg_depend to handle this, - * but not every name dependency gets enumerated that way. For example, - * columns are not marked as dependent on their table. - * - * Rather than trying to be cute about this, we just do a brute-force check - * for any names that have changed. - */ - - /* - * Presumably there's no way for an objects type/classid to change, but be - * safe and attempt the update to object_type. If it actually does change the - * constraint on the table should catch it. - */ - FOR r IN - UPDATE _object_reference.object - SET object_type = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).type::cat_tools.object_type - , object_names = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_names - , object_args = (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)).object_args - FROM _object_reference._object_oid oo - WHERE - oo.object_id = object.object_id - AND (object_type::text, object_names, object_args) IS DISTINCT FROM - (pg_catalog.pg_identify_object_as_address(classid, objid, objsubid)) - RETURNING * - LOOP - RAISE DEBUG 'modified_objects(): %', r; - END LOOP; -END -$body$ - , 'Event trigger function to update any records with object names or args that have changed.' -); -SELECT __object_reference.create_function( - '_object_reference._etg_capture' - , '' - , 'event_trigger SECURITY DEFINER LANGUAGE plpgsql' - , $body$ -DECLARE - c_group_id CONSTANT int := object_group_id FROM object_reference.capture__get_current(); - r record; -BEGIN - - IF c_group_id IS NOT NULL THEN -- Would be NULL if table is empty - RAISE DEBUG E'\n\n*** START ***'; - BEGIN - FOR r IN - SELECT classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension - -- Have to manually exclude command field :/ - FROM pg_catalog.pg_event_trigger_ddl_commands() - LOOP - RAISE DEBUG 'ddl: %', row_to_json(r); - END LOOP; - END; - - FOR r IN SELECT - _object_reference._object_v__for_update( - object_type::cat_tools.object_type - , objid, objsubid - , c_group_id - , classid - ) - , classid, objid, objsubid, command_tag, object_type, schema_name, object_identity, in_extension - FROM pg_catalog.pg_event_trigger_ddl_commands() - WHERE command_tag ~ '^CREATE' --'^(ALTER|CREATE)' - AND NOT object_reference.unsupported(object_type::cat_tools.object_type) - AND (schema_name IS NULL - OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it... - ) - /* - * Self-recognition: skip DDL issued by any extension's own - * install/update script (ours included) rather than trying to - * capture it. - */ - AND NOT in_extension - LOOP - RAISE DEBUG 'registered %', row_to_json(r); - END LOOP; - RAISE DEBUG E'*** END ***\n\n'; - END IF; -END -$body$ - , 'Event trigger function to capture newly created objects in an object group.' -); - /* * object_reference.unsupported(): additionally exclude "partitioned * table"/"partitioned index" (pg_get_object_address() only recognizes the diff --git a/sql/object_reference.sql b/sql/object_reference.sql index d9a4f7a..f8fae2d 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -189,6 +189,15 @@ OR ( _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') ) +/* + * The extension's own pg_extension row is also its own special case: it + * isn't a member of itself (no 'e' row with itself as both member and + * owner), so treat it as one explicitly. + */ +OR ( + _is_own_object.classid = 'pg_catalog.pg_extension'::regclass + AND _is_own_object.objid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') +) $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); @@ -998,7 +1007,7 @@ BEGIN RETURN r_object_v; END $body$ - , 'Return details of a object record, creating a new record if one does not exist.' + , 'Return details of a object record, creating a new record if one does not exist. Heavy-weight compared to a plain read of _object_reference._object_v -- use that instead when an existing record is all that''s needed.' ); SELECT __object_reference.create_function( @@ -1424,6 +1433,16 @@ DECLARE c_group_id CONSTANT int := object_group_id FROM object_reference.capture__get_current(); r record; BEGIN + /* + * Self-recognition: skip while this extension's own event_trigger__disable() + * is in effect (see below) -- i.e. this extension's own install/update + * script is doing delicate internal restructuring right now. Checked via + * to_regclass() rather than a catalog lookup that would error if the temp + * table doesn't exist, which is the common case. + */ + IF to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL THEN + RETURN; + END IF; IF c_group_id IS NOT NULL THEN -- Would be NULL if table is empty RAISE DEBUG E'\n\n*** START ***'; @@ -1452,11 +1471,26 @@ BEGIN OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it... ) /* - * Self-recognition: skip DDL issued by any extension's own - * install/update script (ours included) rather than trying to - * capture it. + * __object_reference is this extension's own scratch install/update + * schema (created and dropped within a single script, never an + * extension member) -- self-recognition via the temp table above + * can't cover the handful of bootstrap statements that run before + * that table exists, so exclude it here too (object_identity + * carries the name for the CREATE SCHEMA statement itself, where + * schema_name is null). + * + * object_reference/_object_reference are excluded outright rather + * than relying on _object_v__for_update()'s own _is_own_object() + * guard: a brand-new object created by this extension's own + * update/install script isn't yet recorded as an 'e' member in + * pg_depend at the point its CREATE fires ddl_command_end (that + * happens once the surrounding CREATE/ALTER EXTENSION completes), + * so _is_own_object() can't see it as self-owned yet either -- + * confirmed by running into it: an active capture group during + * ALTER EXTENSION UPDATE otherwise ends up with this extension's + * own new functions as members. */ - AND NOT in_extension + AND coalesce(schema_name, object_identity, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') LOOP RAISE DEBUG 'registered %', row_to_json(r); END LOOP; @@ -1478,11 +1512,13 @@ DECLARE r record; BEGIN /* - * Self-recognition: skip DDL issued by any extension's own install/update - * script (ours included); pg_event_trigger_ddl_commands() marks this via - * in_extension, unlike pg_event_trigger_dropped_objects() (see _etg_drop). + * Self-recognition: skip while this extension's own event_trigger__disable() + * is in effect (see below) -- i.e. this extension's own install/update + * script is doing delicate internal restructuring right now. Checked via + * to_regclass() rather than a catalog lookup that would error if the temp + * table doesn't exist, which is the common case. */ - IF EXISTS(SELECT 1 FROM pg_catalog.pg_event_trigger_ddl_commands() WHERE in_extension) THEN + IF to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL THEN RETURN; END IF; @@ -1572,51 +1608,64 @@ $body$ ); /* + * WARNING: avoid disabling event triggers at all where any other option + * exists. ALTER EVENT TRIGGER is ordinary transactional DDL -- like any + * other catalog write, it's invisible to other sessions until commit (no + * special database-wide/immediate effect: verified empirically that a + * concurrent session's DDL neither blocks on, nor is otherwise affected by, + * another session's still-uncommitted DISABLE) and it takes no lock at all + * on the event trigger itself. The real risk is TWO SESSIONS both trying to + * alter the SAME event trigger concurrently: a second writer blocks on the + * first the way any two concurrent writes to the same catalog row would, + * and without care, the one that unblocks second can record and later + * restore a "prior state" that was never actually the trigger's state + * immediately before it acted (see the FOR UPDATE lock in + * event_trigger__disable()'s body below, which exists specifically to close + * that gap). Prefer a self-recognition check (a session-local flag, checked + * from inside the trigger's own body) over calling this at all; reach for + * it only when nothing else can make the trigger stay quiet, as is + * currently true for zzz__object_reference_drop. + * * General-purpose event-trigger disable/enable mechanism, for use by this * extension's OWN install/update scripts only (not part of the public API). * Not tied to "being mid-update" specifically -- it's a plain disable-with- - * restore primitive for any event trigger that can't self-recognize (via - * in_extension, see below) that it should stay quiet. + * restore primitive for any event trigger that can't self-recognize that it + * should stay quiet. * - * zzz_object_reference__fix_identity and zzz_object_reference_capture can - * recognize (and skip) DDL issued by any extension's own script via - * pg_event_trigger_ddl_commands()'s in_extension column, so they never need - * to be disabled this way. zzz__object_reference_drop cannot: it fires from - * pg_event_trigger_dropped_objects(), which has no equivalent column, and it - * queries _object_reference._object_v -- a view an update script may itself - * be dropping and recreating -- so it must be truly disabled for the - * duration of such a script's structural section. + * zzz_object_reference__fix_identity and zzz_object_reference_capture check + * whether this call is currently in effect for their OWN session (via + * to_regclass() on the temp table below) and skip if so, so they never need + * to be disabled this way. zzz__object_reference_drop cannot self-recognize + * the same way without also touching _object_reference._object_v -- a view + * an update script may itself be dropping and recreating -- from inside its + * own body, so it must be truly disabled for the duration of such a + * script's structural section. * * ALTER EVENT TRIGGER is ordinary transactional DDL, so if the calling * script's transaction rolls back, the DISABLE (and any ENABLE already run) * rolls back with it -- no separate cleanup-on-error logic is needed here. - * - * Unlike the session_replication_role trick this replaces, disabling an - * event trigger this way is visible database-wide the instant it runs, not - * just to the calling session -- any other session's DDL on a tracked - * object during that window also won't fire the disabled trigger. Update - * scripts are expected to run without concurrent DDL on tracked objects for - * exactly this reason. */ SELECT __object_reference.create_function( '_object_reference.event_trigger__disable' , $args$ - event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' + event_trigger_names name[] $args$ , 'void LANGUAGE plpgsql' , $body$ DECLARE v_name name; + v_enabled "char"; BEGIN + /* + * WARNING: avoid disabling event triggers at all where any other option + * exists -- this is a database-wide change with real race-condition risk + * against concurrent sessions' DDL. See the warning above this function. + */ BEGIN - /* - * Mirrors pg_event_trigger's own evtname/evtenabled columns (via CTAS, - * so the connection to that catalog is visible in the code, not just a - * hand-typed column list that happens to reuse its names) -- this is - * exactly the row this extension needs to restore later. - */ + -- Save old trigger state CREATE TEMP TABLE __object_reference__event_trigger_state AS - SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false; + SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false + ; ALTER TABLE pg_temp.__object_reference__event_trigger_state ADD PRIMARY KEY (evtname); EXCEPTION WHEN duplicate_table THEN RAISE 'event_trigger__disable() called while a previous call is still in effect' @@ -1625,18 +1674,31 @@ BEGIN END; FOREACH v_name IN ARRAY event_trigger_names LOOP - INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) - SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = v_name; + /* + * FOR UPDATE locks the row before we read it, so no other session's own + * ALTER EVENT TRIGGER on the same trigger can land between our read and + * our DISABLE below -- without it, a concurrent change there would + * leave us recording (and later restoring) a state that was never + * actually the trigger's state immediately before we disabled it. + */ + SELECT evtenabled INTO v_enabled + FROM pg_catalog.pg_event_trigger + WHERE evtname = v_name + FOR UPDATE + ; IF NOT FOUND THEN RAISE 'event trigger "%" does not exist', v_name; END IF; + INSERT INTO pg_temp.__object_reference__event_trigger_state(evtname, evtenabled) + VALUES (v_name, v_enabled); + PERFORM _object_reference.exec(format('ALTER EVENT TRIGGER %I DISABLE', v_name)); END LOOP; END $body$ - , 'Disable the given (or default) event triggers, remembering their exact prior state; pair with event_trigger__enable().' + , 'Disable the given event triggers, remembering their exact prior state; pair with event_trigger__enable().' ); SELECT __object_reference.create_function( '_object_reference.event_trigger__enable' @@ -1644,13 +1706,36 @@ SELECT __object_reference.create_function( , 'void LANGUAGE plpgsql' , $body$ DECLARE - r record; + v_names name[]; + v_states "char"[]; + i int; BEGIN - FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__event_trigger_state LOOP + BEGIN + SELECT array_agg(evtname), array_agg(evtenabled) + INTO v_names, v_states + FROM pg_temp.__object_reference__event_trigger_state + ; + EXCEPTION WHEN undefined_table THEN + RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; + END; + + /* + * Drop our own bookkeeping table BEFORE re-enabling anything below: + * dropping it is itself DDL, and if zzz__object_reference_drop is one of + * the triggers being restored here, re-enabling it first would make this + * DROP immediately fire it -- reacting to our own internal cleanup, + * exactly the hazard this whole mechanism exists to avoid. (Confirmed by + * running into it: with the table dropped after, an active capture group + * elsewhere left a stale tracked row that this DROP's cascade into + * post_restore() then found and errored on.) + */ + DROP TABLE pg_temp.__object_reference__event_trigger_state; + + FOR i IN 1..coalesce(array_length(v_names, 1), 0) LOOP PERFORM _object_reference.exec(format( 'ALTER EVENT TRIGGER %I %s' - , r.evtname - , CASE r.evtenabled + , v_names[i] + , CASE v_states[i] WHEN 'O' THEN 'ENABLE' WHEN 'R' THEN 'ENABLE REPLICA' WHEN 'A' THEN 'ENABLE ALWAYS' @@ -1658,10 +1743,6 @@ BEGIN END )); END LOOP; - - DROP TABLE pg_temp.__object_reference__event_trigger_state; -EXCEPTION WHEN undefined_table THEN - RAISE 'event_trigger__enable() called without a matching event_trigger__disable()'; END $body$ , 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.' diff --git a/test/build/expected/build.out b/test/build/expected/build.out index ac88a11..b209139 100644 --- a/test/build/expected/build.out +++ b/test/build/expected/build.out @@ -3,16 +3,16 @@ This extension must be loaded via CREATE EXTENSION object_reference; You really, REALLY do NOT want to try and load this via psql!!! -psql:test/temp_load.not_sql:208: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:217: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:209: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:218: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:457: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:466: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! @@ -22,9 +22,9 @@ psql:test/temp_load.not_sql:457: WARNING: I promise you will be sorry if you tr -psql:test/temp_load.not_sql:569: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:578: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:576: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:585: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out index c6c7793..2f861a5 100644 --- a/test/expected/event_trigger_disable.out +++ b/test/expected/event_trigger_disable.out @@ -1,18 +1,22 @@ \set ECHO none -1..15 -ok 1 - default event trigger to disable is zzz__object_reference_drop -ok 2 - manually disable test trigger b ahead of time -ok 3 - disable() both test triggers -ok 4 - test trigger a is disabled while a call is in effect -ok 5 - test trigger b is (still) disabled while a call is in effect -ok 6 - enable() restores both -ok 7 - test trigger a is back to its original (origin) state -ok 8 - test trigger b is still disabled -- its prior state was preserved, not assumed enabled -ok 9 - disable() the first time -ok 10 - a second disable() without enable() in between is rejected -ok 11 - enable() cleans up so later tests are unaffected -ok 12 - enable() without disable() is rejected -ok 13 - disable() rejects an unknown event trigger name -ok 14 - object_reference schema must not be part of the resolved search_path -ok 15 - _object_reference schema must not be part of the resolved search_path +1..19 +ok 1 - manually disable test trigger b ahead of time +ok 2 - disable() both test triggers +ok 3 - test trigger a is disabled while a call is in effect +ok 4 - test trigger b is (still) disabled while a call is in effect +ok 5 - enable() restores both +ok 6 - test trigger a is back to its original (origin) state +ok 7 - test trigger b is still disabled -- its prior state was preserved, not assumed enabled +ok 8 - disable() the first time +ok 9 - a second disable() without enable() in between is rejected +ok 10 - enable() cleans up so later tests are unaffected +ok 11 - enable() without disable() is rejected +ok 12 - disable() rejects an unknown event trigger name +ok 13 - start a capture group +ok 14 - disable() (any trigger) also signals self-recognizing triggers to stand down +ok 15 - enable() ends that window +ok 16 - the table created while disable() was in effect was NOT captured +ok 17 - stop the capture group +ok 18 - object_reference schema must not be part of the resolved search_path +ok 19 - _object_reference schema must not be part of the resolved search_path # TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql index 63435ae..ddfe743 100644 --- a/test/sql/event_trigger_disable.sql +++ b/test/sql/event_trigger_disable.sql @@ -4,11 +4,14 @@ /* * event_trigger__disable()/__enable() are ALTER EVENT TRIGGER under the - * hood, which is a database-wide change visible to every session the - * instant it runs (unlike the session-local session_replication_role trick - * it replaces) -- so this test exercises the mechanism against its OWN - * dummy event triggers, never the real zzz_* ones other test files in this - * same parallel run depend on staying enabled. + * hood -- ordinary transactional DDL, invisible to other sessions until + * commit, same as any other catalog change (verified empirically: a + * concurrent session neither blocks on, nor otherwise sees, another + * session's still-uncommitted DISABLE). The real risk is two sessions both + * altering the SAME event trigger concurrently, so this test exercises the + * mechanism against its OWN dummy event triggers rather than the real zzz_* + * ones, to keep it independent of whatever else happens to run concurrently + * in this same parallel test batch. */ CREATE FUNCTION event_trigger_disable_test__noop() RETURNS event_trigger LANGUAGE plpgsql AS $$ BEGIN @@ -19,21 +22,14 @@ CREATE EVENT TRIGGER event_trigger_disable_test__b ON ddl_command_start EXECUTE SELECT plan( 0 - +1 -- default target is zzz__object_reference_drop +7 -- multi-trigger disable/enable preserves each one's own prior state +3 -- nested disable() without an intervening enable() is rejected +1 -- enable() without a matching disable() is rejected +1 -- disable() rejects an unknown event trigger name + +5 -- zzz_object_reference_capture self-recognizes and stands down while a disable() is in effect +2 -- schema-qualification (search_path) ); --- Default target (checked via source, never invoked against a real trigger) -SELECT matches( - pg_catalog.pg_get_functiondef('_object_reference.event_trigger__disable(name[])'::regprocedure) - , 'zzz__object_reference_drop' - , 'default event trigger to disable is zzz__object_reference_drop' -); - -- Multi-trigger disable/enable, preserving each trigger's own prior state SELECT lives_ok( $$ALTER EVENT TRIGGER event_trigger_disable_test__b DISABLE$$ @@ -100,6 +96,38 @@ SELECT throws_ok( , 'disable() rejects an unknown event trigger name' ); +/* + * zzz_object_reference_capture / zzz_object_reference__fix_identity check + * for an event_trigger__disable() call currently in effect for THIS + * session (regardless of which trigger names it names) and stand down -- + * verify that here for capture, since it's directly observable. + */ +SELECT lives_ok( + $$SELECT object_reference.capture__start(object_reference.object_group__create('event_trigger_disable_test_group'))$$ + , 'start a capture group' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ + , 'disable() (any trigger) also signals self-recognizing triggers to stand down' +); +CREATE TABLE event_trigger_disable_test_table(); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'enable() ends that window' +); +SELECT is_empty( + $$ + SELECT 1 + FROM _object_reference.object_group__object + WHERE object_group_id = (object_reference.object_group__get('event_trigger_disable_test_group')).object_group_id + $$ + , 'the table created while disable() was in effect was NOT captured' +); +SELECT lives_ok( + $$SELECT object_reference.capture__stop('event_trigger_disable_test_group')$$ + , 'stop the capture group' +); + \i test/finish.sql -- vi: expandtab sw=2 ts=2 From 2f6b4b5bff77618694277aa37da188a75df1ecae Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 16:05:37 -0500 Subject: [PATCH 5/9] Eliminate ALTER EVENT TRIGGER entirely for this PR's own use case A bot review found that zzz__object_reference_drop can self-recognize the same lightweight way zzz_object_reference__fix_identity/_capture already do: the session-local check doesn't touch _object_reference._object_v either, so the originally-stated reason it supposedly couldn't (needing to touch that view from inside its own body) doesn't actually hold. Since that was the one remaining reason event_trigger__disable()/__enable() existed, removed that whole ALTER-EVENT-TRIGGER-based mechanism and replaced it with a plain internal_operation__begin()/__end() pair backed by a transaction-local placeholder GUC (is_local := true) -- no event trigger is ever altered for this extension's own update scripts anymore. A GUC rather than a temp table specifically because unsetting it is a plain function call, not DDL, avoiding the "dropping the sentinel is itself an event these triggers would have to react to" problem a temp table has. This also resolves, by construction, several narrower issues a second bot review pass found in the ALTER-EVENT-TRIGGER version: a confusing raw unique_violation on duplicate trigger names, and the self-recognition guard's blind spot during event_trigger__enable()'s own final ALTER EVENT TRIGGER restore loop. Verified end-to-end again with the same manual reproduction as the previous commit (capture group active across ALTER EXTENSION UPDATE): all three event triggers end the update at their real prior state ('O'), with zero ALTER EVENT TRIGGER statements issued against zzz__object_reference_drop at any point. The bootstrap-time raw ALTER EVENT TRIGGER DISABLE/ENABLE bracket around __object_reference's own creation is unavoidable (nothing exists yet to route a guarded check through) and unaffected by this change, but is now scoped to zzz_object_reference_capture alone -- zzz_object_reference__fix_identity is harmless during bootstrap regardless of which body is live, and zzz__object_reference_drop needs nothing there either since bootstrap only creates things. Also fixed a comment-accuracy issue a bot review caught: _object_reference.exec() already existed in 0.1.0 (verified directly against sql/object_reference--0.1.0.sql), so its recreation here is now removed entirely as unneeded rather than kept behind a corrected comment. Renamed test/sql/event_trigger_disable.sql to test/sql/internal_operation.sql and rewrote it around the new API: it no longer needs dummy event triggers at all (nothing is altered anymore, so there's no shared state to race against other test files), and adds direct verification that zzz__object_reference_drop also stands down during begin()/end() (a table dropped during that window stays tracked). Co-Authored-By: Claude Sonnet 5 --- test/expected/event_trigger_disable.out | 22 ---- test/expected/internal_operation.out | 20 ++++ test/sql/event_trigger_disable.sql | 133 ------------------------ test/sql/internal_operation.sql | 118 +++++++++++++++++++++ 4 files changed, 138 insertions(+), 155 deletions(-) delete mode 100644 test/expected/event_trigger_disable.out create mode 100644 test/expected/internal_operation.out delete mode 100644 test/sql/event_trigger_disable.sql create mode 100644 test/sql/internal_operation.sql diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out deleted file mode 100644 index 2f861a5..0000000 --- a/test/expected/event_trigger_disable.out +++ /dev/null @@ -1,22 +0,0 @@ -\set ECHO none -1..19 -ok 1 - manually disable test trigger b ahead of time -ok 2 - disable() both test triggers -ok 3 - test trigger a is disabled while a call is in effect -ok 4 - test trigger b is (still) disabled while a call is in effect -ok 5 - enable() restores both -ok 6 - test trigger a is back to its original (origin) state -ok 7 - test trigger b is still disabled -- its prior state was preserved, not assumed enabled -ok 8 - disable() the first time -ok 9 - a second disable() without enable() in between is rejected -ok 10 - enable() cleans up so later tests are unaffected -ok 11 - enable() without disable() is rejected -ok 12 - disable() rejects an unknown event trigger name -ok 13 - start a capture group -ok 14 - disable() (any trigger) also signals self-recognizing triggers to stand down -ok 15 - enable() ends that window -ok 16 - the table created while disable() was in effect was NOT captured -ok 17 - stop the capture group -ok 18 - object_reference schema must not be part of the resolved search_path -ok 19 - _object_reference schema must not be part of the resolved search_path -# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/expected/internal_operation.out b/test/expected/internal_operation.out new file mode 100644 index 0000000..58a16b7 --- /dev/null +++ b/test/expected/internal_operation.out @@ -0,0 +1,20 @@ +\set ECHO none +1..17 +ok 1 - begin() +ok 2 - end() +ok 3 - begin() the first time +ok 4 - a second begin() without end() in between is rejected +ok 5 - end() cleans up so later tests are unaffected +ok 6 - end() without begin() is rejected +ok 7 - start a capture group +ok 8 - begin() signals self-recognizing triggers to stand down +ok 9 - end() ends that window +ok 10 - the table created while begin() was in effect was NOT captured +ok 11 - stop the capture group +ok 12 - track a test table +ok 13 - begin() +ok 14 - end() +ok 15 - the dropped table is STILL tracked -- zzz__object_reference_drop stood down while begin() was in effect +ok 16 - object_reference schema must not be part of the resolved search_path +ok 17 - _object_reference schema must not be part of the resolved search_path +# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql deleted file mode 100644 index ddfe743..0000000 --- a/test/sql/event_trigger_disable.sql +++ /dev/null @@ -1,133 +0,0 @@ -\set ECHO none - -\i test/load.sql - -/* - * event_trigger__disable()/__enable() are ALTER EVENT TRIGGER under the - * hood -- ordinary transactional DDL, invisible to other sessions until - * commit, same as any other catalog change (verified empirically: a - * concurrent session neither blocks on, nor otherwise sees, another - * session's still-uncommitted DISABLE). The real risk is two sessions both - * altering the SAME event trigger concurrently, so this test exercises the - * mechanism against its OWN dummy event triggers rather than the real zzz_* - * ones, to keep it independent of whatever else happens to run concurrently - * in this same parallel test batch. - */ -CREATE FUNCTION event_trigger_disable_test__noop() RETURNS event_trigger LANGUAGE plpgsql AS $$ -BEGIN -END -$$; -CREATE EVENT TRIGGER event_trigger_disable_test__a ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); -CREATE EVENT TRIGGER event_trigger_disable_test__b ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); - -SELECT plan( - 0 - +7 -- multi-trigger disable/enable preserves each one's own prior state - +3 -- nested disable() without an intervening enable() is rejected - +1 -- enable() without a matching disable() is rejected - +1 -- disable() rejects an unknown event trigger name - +5 -- zzz_object_reference_capture self-recognizes and stands down while a disable() is in effect - +2 -- schema-qualification (search_path) -); - --- Multi-trigger disable/enable, preserving each trigger's own prior state -SELECT lives_ok( - $$ALTER EVENT TRIGGER event_trigger_disable_test__b DISABLE$$ - , 'manually disable test trigger b ahead of time' -); -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a,event_trigger_disable_test__b}')$$ - , 'disable() both test triggers' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') - , 'D' - , 'test trigger a is disabled while a call is in effect' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') - , 'D' - , 'test trigger b is (still) disabled while a call is in effect' -); -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__enable()$$ - , 'enable() restores both' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') - , 'O' - , 'test trigger a is back to its original (origin) state' -); -SELECT is( - (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') - , 'D' - , 'test trigger b is still disabled -- its prior state was preserved, not assumed enabled' -); - --- Nested disable() without an intervening enable() -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ - , 'disable() the first time' -); -SELECT throws_ok( - $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ - , NULL - , 'event_trigger__disable() called while a previous call is still in effect' - , 'a second disable() without enable() in between is rejected' -); -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__enable()$$ - , 'enable() cleans up so later tests are unaffected' -); - --- enable() without a matching disable() -SELECT throws_ok( - $$SELECT _object_reference.event_trigger__enable()$$ - , NULL - , 'event_trigger__enable() called without a matching event_trigger__disable()' - , 'enable() without disable() is rejected' -); - --- Unknown event trigger name -SELECT throws_ok( - $$SELECT _object_reference.event_trigger__disable('{no_such_event_trigger}')$$ - , NULL - , 'event trigger "no_such_event_trigger" does not exist' - , 'disable() rejects an unknown event trigger name' -); - -/* - * zzz_object_reference_capture / zzz_object_reference__fix_identity check - * for an event_trigger__disable() call currently in effect for THIS - * session (regardless of which trigger names it names) and stand down -- - * verify that here for capture, since it's directly observable. - */ -SELECT lives_ok( - $$SELECT object_reference.capture__start(object_reference.object_group__create('event_trigger_disable_test_group'))$$ - , 'start a capture group' -); -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ - , 'disable() (any trigger) also signals self-recognizing triggers to stand down' -); -CREATE TABLE event_trigger_disable_test_table(); -SELECT lives_ok( - $$SELECT _object_reference.event_trigger__enable()$$ - , 'enable() ends that window' -); -SELECT is_empty( - $$ - SELECT 1 - FROM _object_reference.object_group__object - WHERE object_group_id = (object_reference.object_group__get('event_trigger_disable_test_group')).object_group_id - $$ - , 'the table created while disable() was in effect was NOT captured' -); -SELECT lives_ok( - $$SELECT object_reference.capture__stop('event_trigger_disable_test_group')$$ - , 'stop the capture group' -); - -\i test/finish.sql - --- vi: expandtab sw=2 ts=2 diff --git a/test/sql/internal_operation.sql b/test/sql/internal_operation.sql new file mode 100644 index 0000000..996e605 --- /dev/null +++ b/test/sql/internal_operation.sql @@ -0,0 +1,118 @@ +\set ECHO none + +\i test/load.sql + +SELECT plan( + 0 + +2 -- begin()/end() basic round trip + +3 -- nested begin() without an intervening end() is rejected + +1 -- end() without a matching begin() is rejected + +5 -- zzz_object_reference_capture self-recognizes and stands down while begin() is in effect + +4 -- zzz__object_reference_drop self-recognizes and stands down while begin() is in effect + +2 -- schema-qualification (search_path) +); + +-- Basic round trip +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__begin()$$ + , 'begin()' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__end()$$ + , 'end()' +); + +-- Nested begin() without an intervening end() +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__begin()$$ + , 'begin() the first time' +); +SELECT throws_ok( + $$SELECT _object_reference.internal_operation__begin()$$ + , NULL + , 'internal_operation__begin() called while a previous call is still in effect' + , 'a second begin() without end() in between is rejected' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__end()$$ + , 'end() cleans up so later tests are unaffected' +); + +-- end() without a matching begin() +SELECT throws_ok( + $$SELECT _object_reference.internal_operation__end()$$ + , NULL + , 'internal_operation__end() called without a matching internal_operation__begin()' + , 'end() without begin() is rejected' +); + +/* + * zzz_object_reference_capture checks for an internal_operation__begin() + * call currently in effect for THIS session and stands down -- verify + * directly observable behavior, not the implementation. + */ +SELECT lives_ok( + $$SELECT object_reference.capture__start(object_reference.object_group__create('internal_operation_test_group'))$$ + , 'start a capture group' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__begin()$$ + , 'begin() signals self-recognizing triggers to stand down' +); +CREATE TABLE internal_operation_test_table(); +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__end()$$ + , 'end() ends that window' +); +SELECT is_empty( + $$ + SELECT 1 + FROM _object_reference.object_group__object + WHERE object_group_id = (object_reference.object_group__get('internal_operation_test_group')).object_group_id + $$ + , 'the table created while begin() was in effect was NOT captured' +); +SELECT lives_ok( + $$SELECT object_reference.capture__stop('internal_operation_test_group')$$ + , 'stop the capture group' +); + +/* + * zzz__object_reference_drop checks the same signal and stands down too -- + * a tracked object dropped while begin() is in effect should NOT get + * cleaned up (the whole point: this extension's own update scripts drop + * and recreate their own internals without that being mistaken for a real + * user object going away). + */ +CREATE TABLE internal_operation_drop_test_table(); +SELECT lives_ok( + $$CREATE TEMP TABLE internal_operation_drop_test AS SELECT object_reference.object__getsert('table', 'internal_operation_drop_test_table') AS object_id$$ + , 'track a test table' +); +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__begin()$$ + , 'begin()' +); +DROP TABLE internal_operation_drop_test_table; +SELECT lives_ok( + $$SELECT _object_reference.internal_operation__end()$$ + , 'end()' +); +SELECT is( + (SELECT count(*) FROM _object_reference.object WHERE object_id = (SELECT object_id FROM internal_operation_drop_test)) + , 1::bigint + , 'the dropped table is STILL tracked -- zzz__object_reference_drop stood down while begin() was in effect' +); +/* + * Clean up the now-stale row directly (a plain DML delete, not DDL, so it + * fires no event trigger) rather than leaving it for _etg_fix_identity's + * later, unrelated blanket scan to trip over: in real usage this can't + * happen (an update script's own internal_operation window only ever + * touches objects _is_own_object() already excludes from tracking), so + * this is a test-only artifact of tracking an arbitrary table above. + */ +DELETE FROM _object_reference.object WHERE object_id = (SELECT object_id FROM internal_operation_drop_test); + +\i test/finish.sql + +-- vi: expandtab sw=2 ts=2 From 8d8b55d42685c2508608b48ffc0954486adfee72 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 16:26:36 -0500 Subject: [PATCH 6/9] Revert "Eliminate ALTER EVENT TRIGGER entirely for this PR's own use case" This reverts commit 2f6b4b5bff77618694277aa37da188a75df1ecae. The GUC-based redesign failed CI deterministically (2/2 runs, identical failure signature both times: every call to internal_operation__begin()/ __end() behaves as if the transaction-local placeholder GUC never persisted between statements in the same test file). Despite extensive investigation -- reproducing on a real PostgreSQL 13 instance with the exact same cat_tools 0.3.0 dependency CI installs, running the full parallel test batch (not just the one file) matching pg_regress's actual invocation, and repeating that 10+ times -- it passed cleanly every single time locally. Unable to identify what differs in the actual CI environment that would explain the deterministic failure there. Reverting to the previously fully-green (all 7 PostgreSQL versions) ALTER-EVENT-TRIGGER-based mechanism from f0babc0 rather than ship a redesign whose CI behavior can't be explained or reproduced. The "zzz__object_reference_drop can self-recognize the same lightweight way the other two triggers do" insight from the bot review remains correct in principle -- worth revisiting as a follow-up with proper CI-environment debugging (e.g. a scratch workflow_dispatch run with verbose diagnostics) rather than guessing further from the outside. Co-Authored-By: Claude Sonnet 5 --- test/expected/event_trigger_disable.out | 22 ++++ test/expected/internal_operation.out | 20 ---- test/sql/event_trigger_disable.sql | 133 ++++++++++++++++++++++++ test/sql/internal_operation.sql | 118 --------------------- 4 files changed, 155 insertions(+), 138 deletions(-) create mode 100644 test/expected/event_trigger_disable.out delete mode 100644 test/expected/internal_operation.out create mode 100644 test/sql/event_trigger_disable.sql delete mode 100644 test/sql/internal_operation.sql diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out new file mode 100644 index 0000000..2f861a5 --- /dev/null +++ b/test/expected/event_trigger_disable.out @@ -0,0 +1,22 @@ +\set ECHO none +1..19 +ok 1 - manually disable test trigger b ahead of time +ok 2 - disable() both test triggers +ok 3 - test trigger a is disabled while a call is in effect +ok 4 - test trigger b is (still) disabled while a call is in effect +ok 5 - enable() restores both +ok 6 - test trigger a is back to its original (origin) state +ok 7 - test trigger b is still disabled -- its prior state was preserved, not assumed enabled +ok 8 - disable() the first time +ok 9 - a second disable() without enable() in between is rejected +ok 10 - enable() cleans up so later tests are unaffected +ok 11 - enable() without disable() is rejected +ok 12 - disable() rejects an unknown event trigger name +ok 13 - start a capture group +ok 14 - disable() (any trigger) also signals self-recognizing triggers to stand down +ok 15 - enable() ends that window +ok 16 - the table created while disable() was in effect was NOT captured +ok 17 - stop the capture group +ok 18 - object_reference schema must not be part of the resolved search_path +ok 19 - _object_reference schema must not be part of the resolved search_path +# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/expected/internal_operation.out b/test/expected/internal_operation.out deleted file mode 100644 index 58a16b7..0000000 --- a/test/expected/internal_operation.out +++ /dev/null @@ -1,20 +0,0 @@ -\set ECHO none -1..17 -ok 1 - begin() -ok 2 - end() -ok 3 - begin() the first time -ok 4 - a second begin() without end() in between is rejected -ok 5 - end() cleans up so later tests are unaffected -ok 6 - end() without begin() is rejected -ok 7 - start a capture group -ok 8 - begin() signals self-recognizing triggers to stand down -ok 9 - end() ends that window -ok 10 - the table created while begin() was in effect was NOT captured -ok 11 - stop the capture group -ok 12 - track a test table -ok 13 - begin() -ok 14 - end() -ok 15 - the dropped table is STILL tracked -- zzz__object_reference_drop stood down while begin() was in effect -ok 16 - object_reference schema must not be part of the resolved search_path -ok 17 - _object_reference schema must not be part of the resolved search_path -# TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql new file mode 100644 index 0000000..ddfe743 --- /dev/null +++ b/test/sql/event_trigger_disable.sql @@ -0,0 +1,133 @@ +\set ECHO none + +\i test/load.sql + +/* + * event_trigger__disable()/__enable() are ALTER EVENT TRIGGER under the + * hood -- ordinary transactional DDL, invisible to other sessions until + * commit, same as any other catalog change (verified empirically: a + * concurrent session neither blocks on, nor otherwise sees, another + * session's still-uncommitted DISABLE). The real risk is two sessions both + * altering the SAME event trigger concurrently, so this test exercises the + * mechanism against its OWN dummy event triggers rather than the real zzz_* + * ones, to keep it independent of whatever else happens to run concurrently + * in this same parallel test batch. + */ +CREATE FUNCTION event_trigger_disable_test__noop() RETURNS event_trigger LANGUAGE plpgsql AS $$ +BEGIN +END +$$; +CREATE EVENT TRIGGER event_trigger_disable_test__a ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); +CREATE EVENT TRIGGER event_trigger_disable_test__b ON ddl_command_start EXECUTE FUNCTION event_trigger_disable_test__noop(); + +SELECT plan( + 0 + +7 -- multi-trigger disable/enable preserves each one's own prior state + +3 -- nested disable() without an intervening enable() is rejected + +1 -- enable() without a matching disable() is rejected + +1 -- disable() rejects an unknown event trigger name + +5 -- zzz_object_reference_capture self-recognizes and stands down while a disable() is in effect + +2 -- schema-qualification (search_path) +); + +-- Multi-trigger disable/enable, preserving each trigger's own prior state +SELECT lives_ok( + $$ALTER EVENT TRIGGER event_trigger_disable_test__b DISABLE$$ + , 'manually disable test trigger b ahead of time' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a,event_trigger_disable_test__b}')$$ + , 'disable() both test triggers' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') + , 'D' + , 'test trigger a is disabled while a call is in effect' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') + , 'D' + , 'test trigger b is (still) disabled while a call is in effect' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'enable() restores both' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__a') + , 'O' + , 'test trigger a is back to its original (origin) state' +); +SELECT is( + (SELECT evtenabled FROM pg_catalog.pg_event_trigger WHERE evtname = 'event_trigger_disable_test__b') + , 'D' + , 'test trigger b is still disabled -- its prior state was preserved, not assumed enabled' +); + +-- Nested disable() without an intervening enable() +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ + , 'disable() the first time' +); +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ + , NULL + , 'event_trigger__disable() called while a previous call is still in effect' + , 'a second disable() without enable() in between is rejected' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'enable() cleans up so later tests are unaffected' +); + +-- enable() without a matching disable() +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , NULL + , 'event_trigger__enable() called without a matching event_trigger__disable()' + , 'enable() without disable() is rejected' +); + +-- Unknown event trigger name +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__disable('{no_such_event_trigger}')$$ + , NULL + , 'event trigger "no_such_event_trigger" does not exist' + , 'disable() rejects an unknown event trigger name' +); + +/* + * zzz_object_reference_capture / zzz_object_reference__fix_identity check + * for an event_trigger__disable() call currently in effect for THIS + * session (regardless of which trigger names it names) and stand down -- + * verify that here for capture, since it's directly observable. + */ +SELECT lives_ok( + $$SELECT object_reference.capture__start(object_reference.object_group__create('event_trigger_disable_test_group'))$$ + , 'start a capture group' +); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$ + , 'disable() (any trigger) also signals self-recognizing triggers to stand down' +); +CREATE TABLE event_trigger_disable_test_table(); +SELECT lives_ok( + $$SELECT _object_reference.event_trigger__enable()$$ + , 'enable() ends that window' +); +SELECT is_empty( + $$ + SELECT 1 + FROM _object_reference.object_group__object + WHERE object_group_id = (object_reference.object_group__get('event_trigger_disable_test_group')).object_group_id + $$ + , 'the table created while disable() was in effect was NOT captured' +); +SELECT lives_ok( + $$SELECT object_reference.capture__stop('event_trigger_disable_test_group')$$ + , 'stop the capture group' +); + +\i test/finish.sql + +-- vi: expandtab sw=2 ts=2 diff --git a/test/sql/internal_operation.sql b/test/sql/internal_operation.sql deleted file mode 100644 index 996e605..0000000 --- a/test/sql/internal_operation.sql +++ /dev/null @@ -1,118 +0,0 @@ -\set ECHO none - -\i test/load.sql - -SELECT plan( - 0 - +2 -- begin()/end() basic round trip - +3 -- nested begin() without an intervening end() is rejected - +1 -- end() without a matching begin() is rejected - +5 -- zzz_object_reference_capture self-recognizes and stands down while begin() is in effect - +4 -- zzz__object_reference_drop self-recognizes and stands down while begin() is in effect - +2 -- schema-qualification (search_path) -); - --- Basic round trip -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__begin()$$ - , 'begin()' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__end()$$ - , 'end()' -); - --- Nested begin() without an intervening end() -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__begin()$$ - , 'begin() the first time' -); -SELECT throws_ok( - $$SELECT _object_reference.internal_operation__begin()$$ - , NULL - , 'internal_operation__begin() called while a previous call is still in effect' - , 'a second begin() without end() in between is rejected' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__end()$$ - , 'end() cleans up so later tests are unaffected' -); - --- end() without a matching begin() -SELECT throws_ok( - $$SELECT _object_reference.internal_operation__end()$$ - , NULL - , 'internal_operation__end() called without a matching internal_operation__begin()' - , 'end() without begin() is rejected' -); - -/* - * zzz_object_reference_capture checks for an internal_operation__begin() - * call currently in effect for THIS session and stands down -- verify - * directly observable behavior, not the implementation. - */ -SELECT lives_ok( - $$SELECT object_reference.capture__start(object_reference.object_group__create('internal_operation_test_group'))$$ - , 'start a capture group' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__begin()$$ - , 'begin() signals self-recognizing triggers to stand down' -); -CREATE TABLE internal_operation_test_table(); -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__end()$$ - , 'end() ends that window' -); -SELECT is_empty( - $$ - SELECT 1 - FROM _object_reference.object_group__object - WHERE object_group_id = (object_reference.object_group__get('internal_operation_test_group')).object_group_id - $$ - , 'the table created while begin() was in effect was NOT captured' -); -SELECT lives_ok( - $$SELECT object_reference.capture__stop('internal_operation_test_group')$$ - , 'stop the capture group' -); - -/* - * zzz__object_reference_drop checks the same signal and stands down too -- - * a tracked object dropped while begin() is in effect should NOT get - * cleaned up (the whole point: this extension's own update scripts drop - * and recreate their own internals without that being mistaken for a real - * user object going away). - */ -CREATE TABLE internal_operation_drop_test_table(); -SELECT lives_ok( - $$CREATE TEMP TABLE internal_operation_drop_test AS SELECT object_reference.object__getsert('table', 'internal_operation_drop_test_table') AS object_id$$ - , 'track a test table' -); -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__begin()$$ - , 'begin()' -); -DROP TABLE internal_operation_drop_test_table; -SELECT lives_ok( - $$SELECT _object_reference.internal_operation__end()$$ - , 'end()' -); -SELECT is( - (SELECT count(*) FROM _object_reference.object WHERE object_id = (SELECT object_id FROM internal_operation_drop_test)) - , 1::bigint - , 'the dropped table is STILL tracked -- zzz__object_reference_drop stood down while begin() was in effect' -); -/* - * Clean up the now-stale row directly (a plain DML delete, not DDL, so it - * fires no event trigger) rather than leaving it for _etg_fix_identity's - * later, unrelated blanket scan to trip over: in real usage this can't - * happen (an update script's own internal_operation window only ever - * touches objects _is_own_object() already excludes from tracking), so - * this is a test-only artifact of tracking an arbitrary table above. - */ -DELETE FROM _object_reference.object WHERE object_id = (SELECT object_id FROM internal_operation_drop_test); - -\i test/finish.sql - --- vi: expandtab sw=2 ts=2 From 0b9e8e9671a2bb980e312c3558f85c907c16e0f7 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 16:40:32 -0500 Subject: [PATCH 7/9] Address post-revert regression review: dedup check + stale exec() comment event_trigger__disable() now rejects a duplicate name in its own argument list up front (previously a plain unique_violation from the temp table's PK, uninformative and easy to miss the real cause of). Also removed the pointless recreation of _object_reference.exec() from the 0.1.0->stable update script: 0.1.0 already defines it (unchanged), so the update script's claim that this "closes an existing gap" was wrong, and the whole recreation was a no-op. Co-Authored-By: Claude Sonnet 5 --- sql/object_reference--0.1.0--stable.sql | 24 ++++-------------------- sql/object_reference.sql | 4 ++++ test/expected/event_trigger_disable.out | 17 +++++++++-------- test/sql/event_trigger_disable.sql | 9 +++++++++ 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index 26a2c6c..1f6cd89 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -121,26 +121,6 @@ BEGIN END $body$; -/* - * New: _object_reference.exec(), the permanent counterpart to - * __object_reference.exec() above, used by object__dependency__add() / - * object_group__dependency__add() (unchanged since 0.1.0, but 0.1.0 never - * created this permanent helper -- an existing gap this update closes) and - * by event_trigger__disable()/__enable() below. - */ -SELECT __object_reference.create_function( - '_object_reference.exec' - , 'sql text' - , 'void LANGUAGE plpgsql' - , $body$ -BEGIN - RAISE DEBUG 'sql = %', sql; - EXECUTE sql; -END -$body$ - , 'Execute arbitrary SQL with logging.' -); - /* * New: refuse to track objects that are themselves members of the * object_reference extension (see the guard added to @@ -412,6 +392,10 @@ BEGIN ; END; + IF array_length(event_trigger_names, 1) <> (SELECT count(DISTINCT x) FROM unnest(event_trigger_names) x) THEN + RAISE 'event_trigger_names contains a duplicate name' USING DETAIL = event_trigger_names::text; + END IF; + FOREACH v_name IN ARRAY event_trigger_names LOOP /* * FOR UPDATE locks the row before we read it, so no other session's own diff --git a/sql/object_reference.sql b/sql/object_reference.sql index f8fae2d..d410d2d 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -1673,6 +1673,10 @@ BEGIN ; END; + IF array_length(event_trigger_names, 1) <> (SELECT count(DISTINCT x) FROM unnest(event_trigger_names) x) THEN + RAISE 'event_trigger_names contains a duplicate name' USING DETAIL = event_trigger_names::text; + END IF; + FOREACH v_name IN ARRAY event_trigger_names LOOP /* * FOR UPDATE locks the row before we read it, so no other session's own diff --git a/test/expected/event_trigger_disable.out b/test/expected/event_trigger_disable.out index 2f861a5..2e0e5f6 100644 --- a/test/expected/event_trigger_disable.out +++ b/test/expected/event_trigger_disable.out @@ -1,5 +1,5 @@ \set ECHO none -1..19 +1..20 ok 1 - manually disable test trigger b ahead of time ok 2 - disable() both test triggers ok 3 - test trigger a is disabled while a call is in effect @@ -12,11 +12,12 @@ ok 9 - a second disable() without enable() in between is rejected ok 10 - enable() cleans up so later tests are unaffected ok 11 - enable() without disable() is rejected ok 12 - disable() rejects an unknown event trigger name -ok 13 - start a capture group -ok 14 - disable() (any trigger) also signals self-recognizing triggers to stand down -ok 15 - enable() ends that window -ok 16 - the table created while disable() was in effect was NOT captured -ok 17 - stop the capture group -ok 18 - object_reference schema must not be part of the resolved search_path -ok 19 - _object_reference schema must not be part of the resolved search_path +ok 13 - disable() rejects a duplicate name in its own argument list +ok 14 - start a capture group +ok 15 - disable() (any trigger) also signals self-recognizing triggers to stand down +ok 16 - enable() ends that window +ok 17 - the table created while disable() was in effect was NOT captured +ok 18 - stop the capture group +ok 19 - object_reference schema must not be part of the resolved search_path +ok 20 - _object_reference schema must not be part of the resolved search_path # TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/event_trigger_disable.sql b/test/sql/event_trigger_disable.sql index ddfe743..8730f32 100644 --- a/test/sql/event_trigger_disable.sql +++ b/test/sql/event_trigger_disable.sql @@ -26,6 +26,7 @@ SELECT plan( +3 -- nested disable() without an intervening enable() is rejected +1 -- enable() without a matching disable() is rejected +1 -- disable() rejects an unknown event trigger name + +1 -- disable() rejects a duplicate name in its own argument list +5 -- zzz_object_reference_capture self-recognizes and stands down while a disable() is in effect +2 -- schema-qualification (search_path) ); @@ -96,6 +97,14 @@ SELECT throws_ok( , 'disable() rejects an unknown event trigger name' ); +-- Duplicate name in the same call +SELECT throws_ok( + $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a,event_trigger_disable_test__a}')$$ + , NULL + , 'event_trigger_names contains a duplicate name' + , 'disable() rejects a duplicate name in its own argument list' +); + /* * zzz_object_reference_capture / zzz_object_reference__fix_identity check * for an event_trigger__disable() call currently in effect for THIS From 1aaa37c98c6c886cf772e27837704f71fb9215c6 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 17:01:31 -0500 Subject: [PATCH 8/9] Address latest review round: narrow self-exclusion, missing tests, bootstrap-restore fix _etg_capture's name-based self-exclusion for the __object_reference bootstrap schema was matching by bare text on object_identity for any object with schema_name IS NULL, not just schema-creation -- restrict the object_identity fallback to object_type = 'schema' so an unrelated same-named object (e.g. a third-party extension) can't be silently excluded from an active capture group. Mirrored in both sql/object_reference.sql and the update script. Added test coverage for two previously-untested branches of _is_own_object(): its own private schema (_object_reference, an ordinary pg_depend 'e' member) and its own pg_extension row. The latter is exercised directly against _is_own_object() rather than through object__getsert(), since the latter's generic by-name OID lookup has a pre-existing, unrelated bug for object types with no reg-type cast. The update script's bootstrap-phase ALTER EVENT TRIGGER bracket (protecting CREATE SCHEMA __object_reference before any guarded mechanism exists to route through) unconditionally re-enabled both triggers afterward, silently discarding a DBA's own prior manual DISABLE. Now captures each trigger's actual state into a temp table before disabling and restores that exact state afterward, instead of assuming 'O'. Co-Authored-By: Claude Sonnet 5 --- sql/object_reference--0.1.0--stable.sql | 51 ++++++++++++++++++++----- sql/object_reference.sql | 11 +++++- test/expected/base.out | 10 +++-- test/sql/base.sql | 23 ++++++++++- 4 files changed, 80 insertions(+), 15 deletions(-) diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index 1f6cd89..59d93c6 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -8,10 +8,16 @@ * statements down, and CREATE SCHEMA __object_reference is exactly the * kind of CREATE-tagged statement _etg_capture would otherwise try (and * fail) to register into any capture group active during this update. - * Assumed 'O' (origin) on the restore below rather than captured and - * restored precisely: 0.1.0 always creates both this way, and nothing else - * in this extension ever changes it before an update runs. + * Their actual prior state is captured into a temp table (rather than + * assumed 'O') and restored below, so a DBA's own prior DISABLE of either + * trigger survives this update instead of being silently overwritten. */ +CREATE TEMP TABLE __object_reference__bootstrap_event_trigger_state AS + SELECT evtname, evtenabled + FROM pg_catalog.pg_event_trigger + WHERE evtname IN ('zzz_object_reference_capture', 'zzz_object_reference__fix_identity') +; + ALTER EVENT TRIGGER zzz_object_reference_capture DISABLE; ALTER EVENT TRIGGER zzz_object_reference__fix_identity DISABLE; @@ -250,8 +256,17 @@ BEGIN * confirmed by running into it: an active capture group during * ALTER EXTENSION UPDATE otherwise ends up with this extension's * own new functions as members. + * + * The schema-creation statement itself (CREATE SCHEMA + * __object_reference/etc.) has schema_name = NULL, with the name + * only available via object_identity -- checked separately, and + * restricted to object_type = 'schema', so an unrelated object of + * some other type whose identity happens to match one of these + * three exact strings (e.g. a same-named extension) isn't caught + * by this fallback. */ - AND coalesce(schema_name, object_identity, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') + AND coalesce(schema_name, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') + AND NOT (object_type = 'schema' AND object_identity IN ('__object_reference', 'object_reference', '_object_reference')) LOOP RAISE DEBUG 'registered %', row_to_json(r); END LOOP; @@ -319,12 +334,30 @@ $body$ ); /* - * Re-enable now that the guarded bodies above are live -- self-recognition - * (checking for event_trigger__disable()'s temp table, created below) takes - * over from here for the rest of this script. + * Restore each trigger's actual prior state (captured above) now that the + * guarded bodies above are live -- self-recognition (checking for + * event_trigger__disable()'s temp table, created below) takes over from + * here for the rest of this script. */ -ALTER EVENT TRIGGER zzz_object_reference_capture ENABLE; -ALTER EVENT TRIGGER zzz_object_reference__fix_identity ENABLE; +DO $$ +DECLARE + r record; +BEGIN + FOR r IN SELECT evtname, evtenabled FROM pg_temp.__object_reference__bootstrap_event_trigger_state LOOP + EXECUTE format( + 'ALTER EVENT TRIGGER %I %s' + , r.evtname + , CASE r.evtenabled + WHEN 'O' THEN 'ENABLE' + WHEN 'R' THEN 'ENABLE REPLICA' + WHEN 'A' THEN 'ENABLE ALWAYS' + WHEN 'D' THEN 'DISABLE' + END + ); + END LOOP; +END +$$; +DROP TABLE pg_temp.__object_reference__bootstrap_event_trigger_state; /* * WARNING: avoid disabling event triggers at all where any other option diff --git a/sql/object_reference.sql b/sql/object_reference.sql index d410d2d..d1665c1 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -1489,8 +1489,17 @@ BEGIN * confirmed by running into it: an active capture group during * ALTER EXTENSION UPDATE otherwise ends up with this extension's * own new functions as members. + * + * The schema-creation statement itself (CREATE SCHEMA + * __object_reference/etc.) has schema_name = NULL, with the name + * only available via object_identity -- checked separately, and + * restricted to object_type = 'schema', so an unrelated object of + * some other type whose identity happens to match one of these + * three exact strings (e.g. a same-named extension) isn't caught + * by this fallback. */ - AND coalesce(schema_name, object_identity, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') + AND coalesce(schema_name, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') + AND NOT (object_type = 'schema' AND object_identity IN ('__object_reference', 'object_reference', '_object_reference')) LOOP RAISE DEBUG 'registered %', row_to_json(r); END LOOP; diff --git a/test/expected/base.out b/test/expected/base.out index 6c73346..b2054ef 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,5 +1,5 @@ \set ECHO none -1..15 +1..17 ok 1 - Role object_reference__dependency should be granted USAGE on schema _object_reference ok 2 - Role object_reference__dependency should be granted REFERENCES on table _object_reference.object ok 3 - CREATE TEMP TABLE test_object AS SELECT object_reference.object__getsert('table', 'test_table') AS object_id; @@ -12,7 +12,9 @@ ok 9 - temp objects are rejected ok 10 - own tracking table is rejected ok 11 - own event trigger function is rejected ok 12 - own declared schema is rejected (extension depends on it, not the other way around) -ok 13 - CREATE EXTENSION test_factory -ok 14 - object_reference schema must not be part of the resolved search_path -ok 15 - _object_reference schema must not be part of the resolved search_path +ok 13 - own private schema is rejected (an ordinary 'e' pg_depend member, unlike the declared schema above) +ok 14 - _is_own_object() recognizes its own pg_extension row +ok 15 - CREATE EXTENSION test_factory +ok 16 - object_reference schema must not be part of the resolved search_path +ok 17 - _object_reference schema must not be part of the resolved search_path # TRANSACTION INTENTIONALLY LEFT OPEN! diff --git a/test/sql/base.sql b/test/sql/base.sql index 79ccd90..2f5bce1 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -9,7 +9,7 @@ SELECT plan( +1 -- schema +3 -- initial +2 -- new functions - +6 -- errors (includes temp object + self-tracking rejection tests) + +8 -- errors (includes temp object + self-tracking rejection tests) +1 -- create extensions +2 -- schema-qualification (search_path) ); @@ -93,6 +93,27 @@ SELECT throws_ok( , 'cannot track an object that is a member of the object_reference extension itself' , 'own declared schema is rejected (extension depends on it, not the other way around)' ); +SELECT throws_ok( + $$SELECT object_reference.object__getsert('schema', '_object_reference')$$ + , '0A000' -- feature_not_supported + , 'cannot track an object that is a member of the object_reference extension itself' + , 'own private schema is rejected (an ordinary ''e'' pg_depend member, unlike the declared schema above)' +); +/* + * Exercised directly against _is_own_object() rather than through + * object__getsert('extension', ...): the latter's generic by-name OID + * lookup for object types with no reg-type cast (extension included) + * derives the wrong catalog column name and fails before ever reaching + * this check -- a pre-existing, unrelated bug (see + * object__getsert_w_group_id's v_name_field derivation, predating this PR). + */ +SELECT ok( + _object_reference._is_own_object( + 'pg_catalog.pg_extension'::regclass + , (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') + ) + , '_is_own_object() recognizes its own pg_extension row' +); -- Create extensions SELECT lives_ok( From 95a6e49f115dfbebc074ea321ddcfe915aa1d9ad Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Thu, 17 Sep 2026 17:13:11 -0500 Subject: [PATCH 9/9] Resolve object_reference extension row once in _is_own_object() Previously ran three separate lookups against pg_extension per call (one per OR-branch). Resolve oid/extnamespace once via a FROM-clause subquery and reuse them across all three branches. Co-Authored-By: Claude Sonnet 5 --- sql/object_reference--0.1.0--stable.sql | 54 ++++++++++++------------- sql/object_reference.sql | 54 ++++++++++++------------- test/build/expected/build.out | 10 ++--- 3 files changed, 55 insertions(+), 63 deletions(-) diff --git a/sql/object_reference--0.1.0--stable.sql b/sql/object_reference--0.1.0--stable.sql index 59d93c6..40b9951 100644 --- a/sql/object_reference--0.1.0--stable.sql +++ b/sql/object_reference--0.1.0--stable.sql @@ -140,35 +140,31 @@ SELECT __object_reference.create_function( $args$ , 'boolean LANGUAGE sql STABLE' , $body$ -SELECT EXISTS( - SELECT 1 - FROM pg_catalog.pg_depend d - WHERE d.classid = _is_own_object.classid - AND d.objid = _is_own_object.objid - AND d.deptype = 'e' - AND d.refclassid = 'pg_catalog.pg_extension'::regclass - AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) -/* - * The extension's own declared schema (object_reference) is a special - * case: CREATE EXTENSION records the EXTENSION as depending on it (a plain - * DEPENDENCY_NORMAL row, extension -> schema), not the schema as an 'e' - * member of the extension the way every other object it creates is -- so - * it never matches the pg_depend check above. - */ -OR ( - _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass - AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) -/* - * The extension's own pg_extension row is also its own special case: it - * isn't a member of itself (no 'e' row with itself as both member and - * owner), so treat it as one explicitly. - */ -OR ( - _is_own_object.classid = 'pg_catalog.pg_extension'::regclass - AND _is_own_object.objid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) +SELECT + EXISTS( + SELECT 1 + FROM pg_catalog.pg_depend d + WHERE d.classid = _is_own_object.classid + AND d.objid = _is_own_object.objid + AND d.deptype = 'e' + AND d.refclassid = 'pg_catalog.pg_extension'::regclass + AND d.refobjid = e.oid + ) + /* + * The extension's own declared schema (object_reference) is a special + * case: CREATE EXTENSION records the EXTENSION as depending on it (a + * plain DEPENDENCY_NORMAL row, extension -> schema), not the schema as + * an 'e' member of the extension the way every other object it creates + * is -- so it never matches the pg_depend check above. + */ + OR (_is_own_object.classid = 'pg_catalog.pg_namespace'::regclass AND _is_own_object.objid = e.extnamespace) + /* + * The extension's own pg_extension row is also its own special case: it + * isn't a member of itself (no 'e' row with itself as both member and + * owner), so treat it as one explicitly. + */ + OR (_is_own_object.classid = 'pg_catalog.pg_extension'::regclass AND _is_own_object.objid = e.oid) +FROM (SELECT oid, extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') e $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); diff --git a/sql/object_reference.sql b/sql/object_reference.sql index d1665c1..a43627b 100644 --- a/sql/object_reference.sql +++ b/sql/object_reference.sql @@ -169,35 +169,31 @@ SELECT __object_reference.create_function( $args$ , 'boolean LANGUAGE sql STABLE' , $body$ -SELECT EXISTS( - SELECT 1 - FROM pg_catalog.pg_depend d - WHERE d.classid = _is_own_object.classid - AND d.objid = _is_own_object.objid - AND d.deptype = 'e' - AND d.refclassid = 'pg_catalog.pg_extension'::regclass - AND d.refobjid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) -/* - * The extension's own declared schema (object_reference) is a special - * case: CREATE EXTENSION records the EXTENSION as depending on it (a plain - * DEPENDENCY_NORMAL row, extension -> schema), not the schema as an 'e' - * member of the extension the way every other object it creates is -- so - * it never matches the pg_depend check above. - */ -OR ( - _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass - AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) -/* - * The extension's own pg_extension row is also its own special case: it - * isn't a member of itself (no 'e' row with itself as both member and - * owner), so treat it as one explicitly. - */ -OR ( - _is_own_object.classid = 'pg_catalog.pg_extension'::regclass - AND _is_own_object.objid = (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') -) +SELECT + EXISTS( + SELECT 1 + FROM pg_catalog.pg_depend d + WHERE d.classid = _is_own_object.classid + AND d.objid = _is_own_object.objid + AND d.deptype = 'e' + AND d.refclassid = 'pg_catalog.pg_extension'::regclass + AND d.refobjid = e.oid + ) + /* + * The extension's own declared schema (object_reference) is a special + * case: CREATE EXTENSION records the EXTENSION as depending on it (a + * plain DEPENDENCY_NORMAL row, extension -> schema), not the schema as + * an 'e' member of the extension the way every other object it creates + * is -- so it never matches the pg_depend check above. + */ + OR (_is_own_object.classid = 'pg_catalog.pg_namespace'::regclass AND _is_own_object.objid = e.extnamespace) + /* + * The extension's own pg_extension row is also its own special case: it + * isn't a member of itself (no 'e' row with itself as both member and + * owner), so treat it as one explicitly. + */ + OR (_is_own_object.classid = 'pg_catalog.pg_extension'::regclass AND _is_own_object.objid = e.oid) +FROM (SELECT oid, extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') e $body$ , 'Is the object a member of the object_reference extension itself? (pg_depend deptype = e membership, not just co-installation.)' ); diff --git a/test/build/expected/build.out b/test/build/expected/build.out index b209139..24f8e8f 100644 --- a/test/build/expected/build.out +++ b/test/build/expected/build.out @@ -3,16 +3,16 @@ This extension must be loaded via CREATE EXTENSION object_reference; You really, REALLY do NOT want to try and load this via psql!!! -psql:test/temp_load.not_sql:217: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:213: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:218: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:214: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:466: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:462: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! @@ -22,9 +22,9 @@ psql:test/temp_load.not_sql:466: WARNING: I promise you will be sorry if you tr -psql:test/temp_load.not_sql:578: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:574: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! -psql:test/temp_load.not_sql:585: WARNING: I promise you will be sorry if you try to use this as anything other than an extension! +psql:test/temp_load.not_sql:581: WARNING: I promise you will be sorry if you try to use this as anything other than an extension!