Skip to content

Reject self-tracked objects; add reusable update-disable mechanism #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jnasbyupgrade wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Reject self-tracked objects; add reusable update-disable mechanism #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e03a0e2
Add internal update disable mechanism; refuse to self-track extension…
jnasbyupgrade Aug 17, 2026
79474be
Address review: fix self-track schema gap, rename/clarify disable mec…
jnasbyupgrade Sep 16, 2026
7ba1a93
Fix CI regression: test event_trigger__disable()/__enable() in isolation
jnasbyupgrade Sep 16, 2026
f0babc0
Address review: fix real races/bugs found via empirical testing + rev…
jnasbyupgrade Sep 17, 2026
2f6b4b5
Eliminate ALTER EVENT TRIGGER entirely for this PR's own use case
jnasbyupgrade Sep 17, 2026
8d8b55d
Revert "Eliminate ALTER EVENT TRIGGER entirely for this PR's own use …
jnasbyupgrade Sep 17, 2026
0b9e8e9
Address post-revert regression review: dedup check + stale exec() com…
jnasbyupgrade Sep 17, 2026
1aaa37c
Address latest review round: narrow self-exclusion, missing tests, bo…
jnasbyupgrade Sep 17, 2026
95a6e49
Resolve object_reference extension row once in _is_own_object()
jnasbyupgrade Sep 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
436 changes: 407 additions & 29 deletions sql/object_reference--0.1.0--stable.sql
View file
Open in desktop

Large diffs are not rendered by default.

245 changes: 243 additions & 2 deletions sql/object_reference.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,43 @@ $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 = 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.)'
);

CREATE TABLE _object_reference.object(
object_id serial PRIMARY KEY
, object_type cat_tools.object_type NOT NULL
Expand Down Expand Up @@ -886,6 +923,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

@github-actions github-actions Bot Sep 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Efficiency: _is_own_object() now runs on every call to _object_v__for_update() — i.e. on every object__getsert() for every tracked object, forever, not just during extension updates — re-resolving (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') (and the analogous extnamespace lookup) by name on each invocation instead of once. Minor, but it's added catalog work on what's otherwise a fairly hot path; hoisting the extension oid lookup (e.g. into a WITH once per call, or caching it) would avoid repeating the same lookup twice within a single _is_own_object() call (once per branch of the OR).

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a fair caution... if _object_v__for_update() actually was a hot path. The WHOLE reason it's separated is because it's MUCH heavier weight than a simple read. So it should ONLY get used when we actually need to modify existing data (and that should only be during DDL, and if someone's doing that much DDL they're probably gonna run into other problems...)

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HOWEVER, do make sure there's a warning in the comment for this function that it's heavy-weight and should only be used when the read-only version won't suffice.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the maintainer's reply above, no change needed here -- _object_v__for_update() isn't a hot path by design. Added the requested heavy-weight-use warning to its comment instead.

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
*
Expand Down Expand Up @@ -958,7 +1003,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(
Expand Down Expand Up @@ -1384,6 +1429,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 ***';
Expand All @@ -1397,7 +1452,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
Expand All @@ -1411,6 +1466,36 @@ BEGIN
AND (schema_name IS NULL
OR schema_name NOT LIKE 'pg_temp%' -- pg_my_temp_schema() doesn't seem worth it...
)

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name-based self-exclusion can hide unrelated objects from an active capture group.

coalesce(schema_name, object_identity, '') NOT IN ('__object_reference', 'object_reference', '_object_reference') matches on bare text regardless of object type, not just on the schema-creation case it was written for. Any supported object type with schema_name IS NULL whose object_identity happens to equal one of these three exact strings (e.g. CREATE EXTENSION "_object_reference" if some unrelated third-party extension happened to be packaged under that name) would be silently excluded from whatever capture group is currently active — not because it belongs to this extension, but purely by name collision.

This is a narrow scenario, but it's a real gap in an otherwise principled check: the exclusion is a text match on the tuple's identity rather than a check tied to the object actually being this extension's own (which is what _is_own_object() does elsewhere, just not usable here yet for the reason explained in the surrounding comment).

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1aaa37c — split the check into a schema-name-based filter (unchanged) and a separate object_type = 'schema' AND object_identity IN (...) check for the schema-creation-itself case, so an unrelated object of some other type whose identity happens to match one of these three exact strings can no longer be caught by the fallback. Mirrored in both files.

/*
* __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.
*
* 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, '') 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;
Expand All @@ -1431,6 +1516,17 @@ 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
Expand Down Expand Up @@ -1516,6 +1612,151 @@ $body$
, 'Event trigger function to drop object records when objects are removed.'
);

/*

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Altitude: the whole event_trigger__disable()/__enable() mechanism — which this comment itself calls "a database-wide change with real race-condition risk against concurrent sessions' DDL" — may not be necessary at all.

The stated reason zzz__object_reference_drop needs true ALTER EVENT TRIGGER DISABLE instead of self-recognition (like _etg_fix_identity/_etg_capture use) is that it "cannot self-recognize... without also touching _object_reference._object_v... from inside its own body." But the self-recognition check used by the other two triggers (IF to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL THEN RETURN; END IF;) doesn't touch the view either — it could be added as the very first lines of _etg_drop's body (before its second FOR r_object_v IN ... JOIN _object_reference._object_v loop), letting it skip itself the same lightweight way, for the same reason the 0.1.0 session_replication_role trick was able to blanket-suppress all three event triggers together with no special-casing.

If that works, none of the new event_trigger__disable/event_trigger__enable functions (with their FOR UPDATE locking, temp-table bookkeeping, and the concurrency risk their own comments warn about at length) would be needed for this PR's actual use case — a plain early-return guard, matching the pattern already established for the other two triggers, would be the simpler and safer fix at the root. (The one caveat: self-recognition is coarser — it goes quiet for any event_trigger__disable() call, not just ones naming zzz__object_reference_drop — but since only this extension's own scripts call it, and only ever to quiet its own triggers, that coarseness doesn't appear to cost anything in practice.)

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and you're right — implemented exactly this. Added the same session-local check to _etg_drop's own body and removed the whole event_trigger__disable()/event_trigger__enable() mechanism entirely, replacing it with a plain internal_operation__begin()/__end() pair backed by a transaction-local placeholder GUC (not even a temp table, since unsetting a GUC is a plain function call rather than DDL that would itself need to be guarded against). Zero ALTER EVENT TRIGGER statements against zzz__object_reference_drop anywhere now; verified end-to-end with the same manual repro as before (capture active across ALTER EXTENSION UPDATE) -- all three triggers end at their real prior 'O' state.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to my reply above: this was implemented (2f6b4b5) but then reverted (8d8b55d) along with the rest of that round's redesign, because the mechanism it depended on -- internal_operation__begin()/__end(), backed by a transaction-local placeholder GUC as the session marker -- failed deterministically in CI twice, in a way I could not reproduce locally (single-file runs, the full parallel batch, 10+ repeats, both PG17 and a real PG13 container matching CI's exact dependency versions) or explain after real investigation (ruled out a missing-submodule theory and a dependency version mismatch). Given I don't understand why it failed, I reverted rather than ship something with an unexplained failure mode.

The self-recognition idea itself still looks sound -- the problem was specifically the GUC as the marker, not the concept of _etg_drop checking a session-local signal before it ever touches _object_reference._object_v. A version backed by a real temp table (the same to_regclass() check _etg_capture/_etg_fix_identity already use, and which has been reliably green across every round) instead of set_config() might sidestep whatever the GUC-based version hit. I'm deferring that follow-up for now rather than risk another swing on this PR while the root cause is still unknown -- can pick it up as a follow-up PR/issue if that's useful.

* 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 that it
* should stay quiet.
*
* 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.
*/
SELECT __object_reference.create_function(
'_object_reference.event_trigger__disable'
, $args$
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
Comment thread
jnasbyupgrade marked this conversation as resolved.
-- Save old trigger state
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 '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;

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
* 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)

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness (robustness): duplicate trigger names in event_trigger__disable() raise a confusing raw error instead of a clear one.

If event_trigger_names contains the same name twice (e.g. '{a,a}'), the first iteration inserts a into pg_temp.__object_reference__event_trigger_state and disables it; the second iteration re-reads a (now 'D') and tries to INSERT it again, which violates the table's PRIMARY KEY (evtname) and raises a raw unique_violation instead of a purpose-built error message (unlike the existing duplicate_table/NOT FOUND handling elsewhere in this same function).

This function is documented as "general-purpose ... for use by this extension's OWN install/update scripts" (not just the one hardcoded call site), so a future caller passing an accidental duplicate gets a confusing low-level catalog error rather than a clear diagnostic. Worth an explicit dedup/validation check (e.g. SELECT array_agg(DISTINCT x) ... or an early RAISE on array_length(event_trigger_names,1) <> (SELECT count(DISTINCT x) FROM unnest(event_trigger_names) x)).

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot — the whole event_trigger_names array / multi-trigger-disable mechanism this applied to is gone (see the reply on the altitude comment below); internal_operation__begin() takes no arguments at all now.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came back with the 8d8b55d revert and was re-fixed the same way — see the reply on the newer "regression from the revert" copy of this comment below.

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression from the revert: duplicate trigger names raise a raw unique_violation instead of a clear diagnostic.

This is the same bug flagged in the previous review round (2026-09-16T22:24 / addressed in the now-reverted 2f6b4b5): if event_trigger_names contains a duplicate (e.g. '{a,a}'), the second iteration's INSERT into pg_temp.__object_reference__event_trigger_state violates PRIMARY KEY (evtname) and raises a confusing catalog error instead of a purpose-built one, unlike the existing duplicate_table/NOT FOUND handling elsewhere in this same function.

Since 8d8b55d reverted the GUC-based redesign (2f6b4b5) back to this ALTER EVENT TRIGGER-based mechanism (due to an unreproducible, deterministic CI failure — see the revert's commit message), this issue is back in the current diff. Worth an explicit dedup check before the loop, e.g. comparing array_length(event_trigger_names, 1) against a SELECT count(DISTINCT x) FROM unnest(event_trigger_names) x.

Fix this →

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (again) in 0b9e8e9 — added the same duplicate-name check ahead of the FOREACH loop in both sql/object_reference.sql and sql/object_reference--0.1.0--stable.sql, plus a pgTAP test (test/sql/event_trigger_disable.sql, "disable() rejects a duplicate name in its own argument list").

VALUES (v_name, v_enabled);

PERFORM _object_reference.exec(format('ALTER EVENT TRIGGER %I DISABLE', v_name));
END LOOP;
END
$body$
, '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'
, ''
, 'void LANGUAGE plpgsql'
, $body$
DECLARE
v_names name[];
v_states "char"[];
i int;
BEGIN
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;

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness/efficiency: the self-recognition guard's "on" window doesn't cover the tail of event_trigger__enable() itself.

_etg_fix_identity/_etg_capture skip their work only while to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL (checked at the top of each, e.g. lines 1443 and 1521). But event_trigger__enable() DROP TABLEs that very table (line 1732) before looping over ALTER EVENT TRIGGER ... ENABLE/DISABLE to restore the other triggers (lines 1734-1745). Since ddl_command_end fires after each DDL statement completes, by the time it fires for the DROP TABLE itself and for every subsequent ALTER EVENT TRIGGER restore statement in the loop, the guard is already off (the table is gone) — so _etg_fix_identity's unconditional, unfiltered UPDATE _object_reference.object ... (and _etg_capture's scan) run un-guarded for those statements, when the whole point of this mechanism was to keep them quiet for the entire disable/enable window.

Today this is only wasted work (currently no CREATE-tagged commands occur in that tail, and the _etg_fix_identity UPDATE is idempotent), but it's a real gap in the guard's coverage that a future change to this function (or to what runs between disable() and enable()) could turn into a real correctness issue. Consider moving the DROP TABLE to after the restore loop (it doesn't need to happen first now that zzz__object_reference_drop's re-enable is what the ordering comment above is actually protecting against, not this pair).

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot for the same reason — no more temp table, no more restore loop, no more DROP TABLE to worry about ordering against. See the reply on the altitude comment below.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as my reply below on your follow-up correction — this gap came back with the 8d8b55d revert too; see the reply on the newer "regression from the revert" copy of this comment for the current (deliberate-deferral) decision.

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to my comment above: moving DROP TABLE after the restore loop would be wrong — it would reintroduce exactly the hazard the existing comment above this line describes (re-enabling zzz__object_reference_drop before dropping the bookkeeping table would make this very DROP TABLE fire it). The ordering here is correct as-is.

The gap I'm flagging is narrower than my suggested fix implied: only that _etg_fix_identity/_etg_capture go unguarded for the DROP TABLE statement and the subsequent ALTER EVENT TRIGGER restore loop, since the guard is tied to the same table's existence. A safer fix would be something that doesn't reorder these two hazards against each other — e.g. have the guard check a session-local flag (a GUC, or an already-disabled/nonexistent marker distinct from "table exists") that gets cleared only after the restore loop completes, so the two triggers stay quiet through the whole function without perturbing the zzz__object_reference_drop ordering. As noted, this is currently harmless (idempotent, no CREATE-tagged commands in that tail) — flagging as a latent gap, not an active bug.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moot — see the reply on the altitude comment below; this whole mechanism (and the ordering question your correction was about) no longer exists.

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gap came back with the 8d8b55d revert too — see the reply on the newer "regression from the revert" copy of this comment below for the current (deliberate-deferral) decision.

@github-actions github-actions Bot Sep 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression from the revert: self-recognition guard's "on" window still doesn't cover this DROP TABLE or the restore loop below it.

Same gap flagged previously (2026-09-16T22:24, discussed 2026-09-17T20:53-20:54): _etg_fix_identity/_etg_capture only skip work while to_regclass('pg_temp.__object_reference__event_trigger_state') IS NOT NULL. This DROP TABLE removes that table before the ALTER EVENT TRIGGER restore loop a few lines down runs, so both this statement and every restore statement in the loop execute with the guard already off. Currently harmless (idempotent ops, no CREATE-tagged commands in that tail), but it's a real coverage gap that a future change here could turn into a correctness issue.

This was left as an acknowledged, narrow gap in the prior review thread (a same-session GUC/flag distinct from "table exists," cleared only after the restore loop, was suggested as a non-reordering fix) — flagging again since the revert (8d8b55d, undoing 2f6b4b5) restored this exact code path, including this unresolved gap.

Fix this →

@jnasbyupgrade jnasbyupgrade Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberately left as the same accepted, narrow, currently-harmless gap discussed on the original comment above -- introducing a new session-local marker to close it is exactly the kind of change that caused the unexplained, deterministic CI failure in 2f6b4b5/8d8b55d. Not reintroducing that risk until the root cause is actually understood; will revisit once there's a real explanation for that failure (or a debugging setup that can reproduce it outside CI).


FOR i IN 1..coalesce(array_length(v_names, 1), 0) LOOP
PERFORM _object_reference.exec(format(
'ALTER EVENT TRIGGER %I %s'
, v_names[i]
, CASE v_states[i]
WHEN 'O' THEN 'ENABLE'
WHEN 'R' THEN 'ENABLE REPLICA'
WHEN 'A' THEN 'ENABLE ALWAYS'
WHEN 'D' THEN 'DISABLE'
END
));
END LOOP;
END
$body$
, 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.'
);

SELECT __object_reference.create_function(
'_object_reference.etg_raise__start'
, ''
Expand Down
13 changes: 8 additions & 5 deletions test/build/expected/build.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: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: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: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:462: WARNING: I promise you will be sorry if you try to use this as anything other than an extension!



Expand All @@ -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: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:581: WARNING: I promise you will be sorry if you try to use this as anything other than an extension!





Expand Down
13 changes: 9 additions & 4 deletions test/expected/base.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
\set ECHO none
1..12
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;
Expand All @@ -9,7 +9,12 @@ 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 - own declared schema is rejected (extension depends on it, not the other way around)
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!
23 changes: 23 additions & 0 deletions test/expected/event_trigger_disable.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
\set ECHO none
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
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 - 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!
Loading
Loading