Add event_trigger__disable()/__enable() for update scripts by jnasbyupgrade · Pull Request #45 · Postgres-Extensions/object_reference

jnasbyupgrade · 2026-10-30T06:52:23Z

Questioned automated-review findings: #45 (comment), #45 (comment)

Replaces the 0.1.0 -> stable update script's session_replication_role = replica bracket with a reusable pair, _object_reference.event_trigger__disable(name[]) and event_trigger__enable(), for this and future update scripts. disable() records each named trigger's exact prior evtenabled value before disabling it, and enable() restores that state rather than assuming origin. Concurrent protection depends on privilege: with UPDATE on pg_event_trigger, disable() row-locks each trigger so another session's ALTER EVENT TRIGGER on it waits; without it (as on managed services such as RDS) it takes an advisory lock, which serializes only other callers of disable(), not a raw ALTER EVENT TRIGGER in another session.

Closes #40

Details

zzz_object_reference_capture and zzz_object_reference__fix_identity return immediately while disable()'s temp table exists in the current session (checked with to_regclass()), so they never need disabling themselves. zzz__object_reference_drop can't recognize the extension's own update script that way, so the update script disables it with event_trigger__disable('{zzz__object_reference_drop}') around its structural section.
ALTER EVENT TRIGGER is transactional, so a rolled-back caller undoes the DISABLE without separate cleanup.
disable() validates its arguments before any side effect, in this order: NULL or empty array, a NULL element, a duplicate name, an unknown trigger name, and finally a previous call still in effect. enable() rejects a call with no matching disable() before doing anything.
disable() sorts the names under COLLATE "C" and locks and disables in that order; enable() restores in the same order, so concurrent callers can't deadlock on lock order.
Locking goes through _object_reference._event_trigger__lock(name), which tries SELECT ... FOR UPDATE on the trigger's pg_event_trigger row and, only on insufficient_privilege (42501), takes pg_advisory_xact_lock keyed on the catalog and trigger oids. It returns the trigger's evtenabled, or NULL if it no longer exists.
0.1.0's own _etg_capture/_etg_fix_identity bodies have no such check, so the update script first disables both with raw ALTER EVENT TRIGGER, installs the guarded bodies, then restores each one's saved prior state before creating disable()/enable().
New test/sql/event_trigger_disable.sql (38 assertions) exercises the mechanism against its own dummy event triggers: prior-state preservation across two triggers, rejection of a nested call, an unknown name, a duplicate name, a NULL array, an empty array and a NULL element, which error wins over a call still in effect, "C"-order processing of unsorted names, the advisory fallback for a role that can't row-lock the catalog, and zzz_object_reference_capture standing down (a table created while a disable() is in effect isn't captured). A raised error rolls the call back, so the tests can't observe whether validation ran before a side effect; they check which error is reported.
Under make test-update, test/install/load.sql checks the 0.1.0 -> stable update, with a capture group active, in three rolled-back scenarios: (1) all three event triggers at their default state, then a table created after the update is captured; (2) zzz__object_reference_drop already disabled, and still disabled after; (3) a table captured before the update is renamed after it, and the group shows the new name. It then runs the update once more with zzz_object_reference_capture, zzz_object_reference__fix_identity and zzz__object_reference_drop in DISABLE, ENABLE REPLICA and ENABLE ALWAYS, and fails unless each is restored to exactly that state, which covers both the bootstrap restore and enable(). A failed check exits psql nonzero (ON_ERROR_STOP), but that does not fail the make run on any PostgreSQL version (pg_regress 16+ prints not ok, PG12's prints ok, and make exits 0 either way). CI runs fresh mode only.
test/build/expected/build.out changes only by line-number shifts.

Known open item

On PostgreSQL 12 (and probably 13), updating from 0.1.0 fails when a capture group is active and zzz__object_reference_drop was already disabled, with cache lookup failed for namespace from _etg_capture. The cause is a pg_event_trigger_ddl_commands() bug fixed upstream only in PG14+; the fix here is pending. PG12's pg_regress reports the resulting psql failure as ok, so make test-update does not flag it there.

`object__getsert()` (via `_object_v__for_update()`) now raises feature_not_supported for any object belonging to the object_reference extension. Letting the tracking machinery observe its own members means the extension's own DDL -- including its update scripts restructuring the extension -- becomes something that machinery reacts to. `_object_reference._is_own_object()` checks `pg_depend` membership (`deptype = 'e'`), plus the two objects that are never 'e' members: the extension's declared schema and its own `pg_extension` row. `_etg_capture` also skips objects in the object_reference, _object_reference and __object_reference schemas: an object created by the extension's own install/update script isn't recorded as an 'e' member until the surrounding CREATE/ALTER EXTENSION finishes, so `_is_own_object()` can't see it yet when ddl_command_end fires. The 0.1.0 -> stable update script creates `_is_own_object()` and the updated `_etg_capture` inside its existing session_replication_role section. Part of #40

…le() `_object_reference.event_trigger__disable(name[])` disables the given event triggers and records each one's prior `evtenabled` value in a temp table; `event_trigger__enable()` restores exactly that state. ALTER EVENT TRIGGER is transactional, so a rolled-back caller undoes the DISABLE without separate cleanup. `_etg_capture` and `_etg_fix_identity` return early while that temp table exists in the current session, so they never need disabling. `_etg_drop` can't recognize the extension's own update script that way, so the 0.1.0 -> stable update script disables it with `event_trigger__disable()` around its structural section, replacing the `session_replication_role = replica` bracket. 0.1.0's `_etg_capture`/`_etg_fix_identity` have no such guard, so the update script disables both with raw ALTER EVENT TRIGGER for the bootstrap statements that run before their guarded replacements are installed, then restores each one's saved prior state. Closes #40

coderabbitai · 2026-10-30T06:52:31Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 6d52a099-c925-4502-865c-1453b1d82886

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-10-30T06:54:02Z

Claude finished @jnasbyupgrade's task in 30s —— View job

Code review: PR #45 (event_triggerdisable()/enable())

I read the diff against origin/refuse-self-tracking. I didn't run the build or the test suite. The /code-review skill failed to execute, so this is a manual review.

No blocking bugs found. The save/restore logic holds up:

array_agg(evtname) and array_agg(evtenabled) come from one query, so the pairing in enable() is consistent.
enable() drops the bookkeeping table before restoring any trigger, which avoids firing zzz__object_reference_drop on its own cleanup.
FOR UPDATE on the pg_event_trigger row is a reasonable way to avoid recording a stale prior state.
Both failure paths (temp table created, then an unknown or duplicate name) abort the statement. The temp table rolls back with it, so nothing is left behind.

Observations

The self-recognition flag can be set by any user (sql/object_reference.sql ~L1432 and ~L1519; the same guard is in the update script).
- _etg_capture and _etg_fix_identity skip when to_regclass('pg_temp.__object_reference__event_trigger_state') is non-null.
- Any role with the default TEMP privilege can run CREATE TEMP TABLE __object_reference__event_trigger_state(x int) and then skip capture and identity fixing for the rest of its own session.
- That is probably acceptable, because it only affects that user's own DDL tracking. It is also a side effect of using a table name as the flag. If you want to harden it, check that the temp table has the expected columns, or use a session-local GUC or a SECURITY DEFINER-only marker instead.
- It's also worth a line in the comments, since the "never need disabling" claim depends on nobody else creating that name.
event_trigger__enable() fails partway if a saved trigger was dropped in between.
- The ALTER EVENT TRIGGER for a missing trigger raises an error, and the later triggers in the loop are not restored.
- The temp table is already dropped by then, so the state is lost.
- Within a transaction this rolls back cleanly, so the risk is low. Skipping missing names, or raising a clearer error, would be more robust.
The state restore CASE has no ELSE. It is duplicated in the bootstrap DO block and in enable(). An unexpected evtenabled value would produce ALTER EVENT TRIGGER x NULL, which is a syntax error. The values are limited to O/R/A/D, so this is only a nit. A shared helper, or ELSE RAISE, would also remove the duplication.
The bootstrap temp table is created while the old 0.1.0 triggers are still active (top of sql/object_reference--0.1.0--stable.sql).
- CREATE TEMP TABLE AS fires the unguarded 0.1.0 _etg_capture and _etg_fix_identity before the ALTER EVENT TRIGGER ... DISABLE lines run.
- This only matters if a capture group is active during ALTER EXTENSION UPDATE. In that case the 0.1.0 capture could try to register a pg_temp table.
- Swapping the order is not possible, because the state has to be read first. You could read the state into a DO-block variable or a GUC instead of a table.
- If CI already covers the active-capture update path, ignore this.
Tests. test/sql/event_trigger_disable.sql covers the prior-state, rejection and capture-standdown cases well. No test covers the 0.1.0 → stable update with one of the triggers starting out disabled (D) or set to ENABLE REPLICA. The PR description presents that case as the point of the change, so one test for it would be worthwhile.

In update mode, test/install/load.sql now starts `zzz_object_reference_capture`, `zzz_object_reference__fix_identity` and `zzz__object_reference_drop` in distinct non-default states (DISABLE, ENABLE REPLICA, ENABLE ALWAYS), runs the update, and raises unless each one is back in exactly that state. That covers both restore paths: the bootstrap block for the first two and `event_trigger__enable()` for the third. load.sql's own output is never compared against anything, so the update and the check share a transaction: a failed check also undoes the update, and the suite then fails against the old version. All three triggers are reset to ENABLE afterwards so the rest of the suite runs unchanged.

github-actions · 2026-10-30T18:03:36Z

Claude finished @jnasbyupgrade's task in 24s —— View job

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

I read the diff against origin/refuse-self-tracking. I didn't run the build or the test suite. The /code-review skill failed to run, so this is a manual review.

No blocking bugs found.

The save/restore logic holds up:

enable() drops the bookkeeping table before it restores any trigger, so a restored zzz__object_reference_drop can't fire on that cleanup.
Both failure paths in disable() (unknown name, duplicate name) abort the statement. The temp table rolls back with it.
The FOR UPDATE on the pg_event_trigger row is a reasonable way to avoid recording a stale prior state.
The bootstrap CREATE TEMP TABLE AS in the update script runs while the 0.1.0 triggers are still live. That is harmless, because 0.1.0's _etg_capture already skips pg_temp% schemas.
test/install/load.sql starts the three triggers in D, R and A and fails the update if any isn't restored. That covers the non-default-state path.

Nits and optional hardening

The self-recognition flag is a plain temp-table name.
- Any role with TEMP privilege can create __object_reference__event_trigger_state and make _etg_capture and _etg_fix_identity skip for the rest of its own session.
- Only that session's own DDL tracking is affected. If you want to harden it, check the table's columns, or mention the assumption in the comment.
enable() can fail partway.
- If a saved trigger was dropped in between, ALTER EVENT TRIGGER raises and the later triggers aren't restored. The bookkeeping table is already gone by then.
- Inside a transaction this rolls back cleanly, so the risk is low.
CASE without ELSE.
- The state-to-ALTER mapping is duplicated in enable() and in the update script's bootstrap DO block. An unexpected evtenabled value would produce ALTER EVENT TRIGGER x NULL, which is a syntax error.
- Valid values are only O/R/A/D, so a shared helper or an ELSE RAISE is optional.
event_trigger__disable(NULL) gives a poor error.
- array_length(NULL, 1) <> ... evaluates to NULL, so the duplicate check is skipped. FOREACH over NULL then raises a generic error.
- An explicit IF event_trigger_names IS NULL THEN RAISE would be clearer. '{}' is accepted as a no-op, which seems fine.

jnasbyupgrade · 2026-10-30T18:11:30Z

Written by Claude.

Replies to items 2-5 of the automated review.

event_trigger__enable() fails partway if a saved trigger was dropped in between.

The ALTER EVENT TRIGGER for a missing trigger raises an error, and the later triggers in the loop are not restored.

The temp table is already dropped by then, so the state is lost.

Within a transaction this rolls back cleanly, so the risk is low. Skipping missing names, or raising a clearer error, would be more robust.

No change. The error is the intended behavior, and the state is not lost. enable() runs as a single statement, so its error rolls back everything it did, including its own DROP TABLE of the state table, along with the statement or the enclosing transaction. I checked this with two dummy event triggers. I set or45_keep to ENABLE REPLICA and left or45_gone at ENABLE, ran event_trigger__disable('{or45_keep,or45_gone}'), then DROP EVENT TRIGGER or45_gone. After that, event_trigger__enable() fails with event trigger "or45_gone" does not exist. to_regclass('pg_temp.__object_reference__event_trigger_state') is still non-null and still holds both saved rows (O and R), and or45_keep is still D. The result is the same when enable() runs inside an explicit transaction that is then rolled back. That error already names the missing trigger. Skipping missing names would hide that a trigger the caller asked to restore no longer exists.

The state restore CASE has no ELSE. It is duplicated in the bootstrap DO block and in enable(). An unexpected evtenabled value would produce ALTER EVENT TRIGGER x NULL, which is a syntax error. The values are limited to O/R/A/D, so this is only a nit. A shared helper, or ELSE RAISE, would also remove the duplication.

No change. pg_event_trigger.evtenabled only takes O, R, A and D, and an unexpected value already aborts the statement, which is what an ELSE RAISE would do. The CASE yields NULL, format('%s') renders that as an empty string, and ALTER EVENT TRIGGER x fails with syntax error at end of input. The bootstrap copy is a one-shot block in the 0.1.0 → stable update script, so it is not worth an extra shared function.

The bootstrap temp table is created while the old 0.1.0 triggers are still active (top of sql/object_reference--0.1.0--stable.sql).

CREATE TEMP TABLE AS fires the unguarded 0.1.0 _etg_capture and _etg_fix_identity before the ALTER EVENT TRIGGER ... DISABLE lines run.

This only matters if a capture group is active during ALTER EXTENSION UPDATE. In that case the 0.1.0 capture could try to register a pg_temp table.

Swapping the order is not possible, because the state has to be read first. You could read the state into a DO-block variable or a GUC instead of a table.

If CI already covers the active-capture update path, ignore this.

No change. The bootstrap temp table can't be registered. 0.1.0's _etg_capture already skips temp objects (schema_name NOT LIKE 'pg_temp%'), and pg_event_trigger_ddl_commands() reports a temp table's schema_name as pg_temp. 0.1.0's _etg_fix_identity only rewrites tracked objects whose names changed, and creating a temp table changes none. I checked this on a database at 0.1.0. Inside one transaction I started a capture group and created public.or45_ctrl, which was captured (1 tracked object, 1 group member). I then ran ALTER EXTENSION object_reference UPDATE, called capture__stop() and committed. The update succeeded, both counts stayed at 1, and no tracked object has a pg_temp name.

Tests. test/sql/event_trigger_disable.sql covers the prior-state, rejection and capture-standdown cases well. No test covers the 0.1.0 → stable update with one of the triggers starting out disabled (D) or set to ENABLE REPLICA. The PR description presents that case as the point of the change, so one test for it would be worthwhile.

Fixed in 8429c9d. In update mode, test/install/load.sql now puts each trigger in a different state before the update: zzz_object_reference_capture in DISABLE, zzz_object_reference__fix_identity in ENABLE REPLICA and zzz__object_reference_drop in ENABLE ALWAYS. It then raises unless each one is back in exactly that state. That covers both restore paths: the bootstrap block restores the first two and event_trigger__enable() restores the third. I checked the test against two deliberately broken update scripts, one per restore path, each of which always restores ENABLE. Each one fails make verify-results TEST_LOAD_SOURCE=update, and the error names the trigger that was restored wrongly. The check runs under make test-update. CI currently runs fresh mode only.

jnasbyupgrade · 2026-10-30T21:11:13Z

Written by Claude.

Re: #45 (comment)

event_trigger__disable(NULL) gives a poor error.

array_length(NULL, 1) <> ... evaluates to NULL, so the duplicate check is skipped. FOREACH over NULL then raises a generic error.

An explicit IF event_trigger_names IS NULL THEN RAISE would be clearer. '{}' is accepted as a no-op, which seems fine.

No change. Passing NULL does not produce a generic error: PL/pgSQL raises FOREACH expression must not be null, which names the problem, and the statement aborts. The temp table the function created rolls back with it. I checked this on PG12 and PG17 with a throwaway DO block that runs FOREACH over NULL::name[]; both raise that same message.

event_trigger__disable() is internal to the extension's own install/update scripts and is not part of the public API. Its callers pass literal arrays, so an explicit NULL check would only restate the error PL/pgSQL already raises.

Items 2 and 3 of this review repeat items 2 and 3 of the earlier review, answered in #45 (comment).

`object__getsert()` (via `_object_v__for_update()`) now refuses, with `feature_not_supported`, to track any object that belongs to the object_reference extension itself. The extension's own members must never become tracked objects. Part of #40. #45, stacked on this PR, completes #40.

github-actions · 2026-10-30T21:25:49Z

Claude finished @jnasbyupgrade's task in 14s —— View job

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

I read the diff against origin/master. I didn't run the build or the test suite. The /code-review skill failed to execute, so this is a manual review, with a close read of sql/object_reference.sql.

No blocking bugs found.

The save/restore pairing is consistent. array_agg(evtname) and array_agg(evtenabled) come from one query.
enable() drops the bookkeeping table before restoring anything. A restored zzz__object_reference_drop therefore can't fire on that cleanup.
Failure paths in disable() (nested call, duplicate name, unknown name) abort the statement, and the temp table rolls back with it.
The FOR UPDATE on the pg_event_trigger row is a sound way to avoid recording a stale prior state.
The to_regclass() guard in _etg_capture and _etg_fix_identity runs before any other work, so it adds no cost in the normal case.
The extra __object_reference/object_reference/_object_reference exclusions in _etg_capture are limited to those exact names. The schema-creation fallback is restricted to object_type = 'schema'.

The open items from earlier reviews (a partial enable() failure, the CASE without ELSE, disable(NULL), the bootstrap temp table) have been answered in the thread. I agree with those replies and have nothing new to add on them.

Remaining optional nits

Temp-table name as the flag.
- Any role with TEMP privilege can create __object_reference__event_trigger_state and silence its own session's capture and identity fixing.
- The effect is limited to that session. A one-line comment noting the assumption would be enough.
Whitespace-only change. FOR r IN SELECT loses its trailing space in _etg_capture. It is harmless but unrelated to this PR.
CI coverage. The update-mode restore check in test/install/load.sql runs only under make test-update, and CI runs fresh mode only. A regression in the 0.1.0 → stable restore path wouldn't be caught automatically. Adding an update-mode CI job is worth considering, but not as part of this PR.

jnasbyupgrade · 2026-10-30T21:43:27Z

+    ;
+  END;
+
+  IF array_length(event_trigger_names, 1) <> (SELECT count(DISTINCT x) FROM unnest(event_trigger_names) x) THEN


Caller sanity checks like this should be done before we take meaningful action (unless there's a GOOD reason to wait).

Claude here. Done in 7a44f99. event_trigger__disable() now runs every argument check before any side effect (temp table creation, lock, ALTER): NULL/empty array, NULL elements, duplicate names, every named trigger exists, and finally the "previous call still in effect" check via to_regclass() instead of catching duplicate_table. event_trigger__enable() likewise checks for a matching disable() up front. The only check left after the lock is a defensive re-check that a trigger was not dropped by another session in between. Because a raised error rolls the call back, the tests assert error precedence (argument errors are reported even while a previous call is in effect) rather than leftover state.

jnasbyupgrade · 2026-10-30T21:46:39Z

+    SELECT evtenabled INTO v_enabled
+      FROM pg_catalog.pg_event_trigger
+      WHERE evtname = v_name
+      FOR UPDATE


This is not going to work on something like RDS that doesn't give true superuser access. I'd say we should use an advisory lock as a backup mechanism (ie, detect the error we'd get for not being able to modify the catalog).

Also, we should force event_trigger_names to be sorted (in a fixed local); otherwise there's a deadlock potential.

Claude here. Done in 7a44f99.

Lock fallback: per-trigger locking moved into _object_reference._event_trigger__lock(name). It tries SELECT ... FOR UPDATE on pg_event_trigger and, on insufficient_privilege (SQLSTATE 42501) only, takes pg_advisory_xact_lock(<pg_event_trigger regclass oid>::int, <trigger oid>::int). I confirmed 42501 ("permission denied for table pg_event_trigger") by running that SELECT as a plain non-superuser role on PG12 and PG17. Oid-to-int casts wrap (4294967295::oid::int is -1) rather than error, so large oids are safe as keys. The advisory lock only serializes against other callers of this function. A test calls the helper under SET ROLE as a role that cannot row-lock and checks pg_locks.

Sorting: I read "force event_trigger_names to be sorted (in a fixed local)" as the function sorting the names itself under COLLATE "C", not rejecting unsorted input. disable() locks and disables in that order and enable() restores in the same order. A test passes unsorted names and checks they are processed in "C" order.

In update mode, test/install/load.sql now runs three update scenarios before the committed update, each installing 0.1.0 in its own transaction and rolling back: 1. All three event triggers at ENABLE: the update leaves the capture group empty, restores every trigger, leaves no state table behind, and a table created afterwards is captured. 2. zzz__object_reference_drop already disabled: it stays disabled. 3. A table captured before the update and renamed after it shows its new name in the capture group. Each scenario asserts the trigger state it starts the update from. Also restore the trailing space after WHERE in the update script's copy of _etg_fix_identity, so its body matches sql/object_reference.sql, and correct load.sql's description of how a failed check fails the run.

… check

github-actions · 2026-10-30T22:05:05Z

Claude finished @jnasbyupgrade's task in 21s —— View job

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

The /code-review skill failed to run, so this is a manual review of event_trigger__disable() and event_trigger__enable() in sql/object_reference.sql. I didn't build or run the tests. The three findings below correspond to jnasbyupgrade's two inline review comments. I agree with both and add one more.

1. Do the argument checks before any side effects (sql/object_reference.sql:1668-1682)

disable() creates the temp table and adds its primary key first. It then checks for duplicate names, and only after that looks up unknown names.
The checks can go first: reject a NULL array (a clear RAISE instead of PL/pgSQL's FOREACH expression must not be null) and reject duplicates. The nested-call check is already effectively "create the table", so it can stay where it is.
The unknown-name check depends on the catalog, so it can't move above the table creation unless you do an up-front SELECT for missing names. That would be worth it, so a bad call never creates the table at all.
Nothing is left behind today, because the statement rolls back. Moving the checks up still makes the function easier to reason about and removes the dependence on that rollback.

2. FOR UPDATE on pg_event_trigger will fail on managed Postgres such as RDS (:1692-1696)

SELECT ... FOR UPDATE needs UPDATE privilege on the catalog table. Non-superuser admin roles such as rds_superuser don't have it, so disable() would raise permission denied for table pg_event_trigger and the update script would abort.
Suggested fallback: wrap the locking SELECT in EXCEPTION WHEN insufficient_privilege, then take pg_advisory_xact_lock on a key derived from the trigger name (for example hashtext('object_reference.event_trigger.' || v_name)) and read the state without FOR UPDATE.
The advisory lock only serializes sessions that go through this function. Other sessions' raw ALTER EVENT TRIGGER calls would not be covered, so the comment should say so.
Caveat on the fallback: a subtransaction around the lock attempt keeps the outer transaction usable, but the PL/pgSQL EXCEPTION block costs a subtransaction per trigger. That is fine here.

3. Sort the names to avoid deadlocks (:1684)

Two sessions calling disable() with the same names in different orders can each lock one pg_event_trigger row and then wait on the other.
Iterate over unnest(event_trigger_names) ORDER BY 1 in a fixed collation, for example ORDER BY x COLLATE "C". The same order applies to advisory-lock keys if you add them.
With a fixed order, the PK on the temp table already prevents the duplicate-lock case.

Nits (not blocking)

A test for the new validation order: disable('{x,x}') and an unknown name should leave no temp table behind. This is covered implicitly today but isn't asserted.
The earlier open threads (temp-table name as flag, CASE without ELSE, partial enable() failure) were answered in the discussion. I have nothing to add on them.

I made no code changes.

…fects disable() now rejects a NULL/empty array, NULL elements, duplicates, and unknown names, and a call still in effect, before creating its state table or locking anything. enable() checks for a matching disable() up front. Names are processed in "C" collation order (and restored in the same order) so concurrent callers lock in a consistent order. Per-trigger locking moves to _event_trigger__lock(), which falls back to a transaction-scoped advisory lock when the role cannot row-lock pg_event_trigger (insufficient_privilege). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

github-actions · 2026-10-30T22:21:40Z

Claude finished @jnasbyupgrade's task in 17s —— View job

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

The /code-review skill failed to run, so this is a manual read of the current event_trigger__disable(), event_trigger__enable() and _event_trigger__lock() in sql/object_reference.sql. I didn't build or run the tests.

No blocking bugs found. The earlier review requests are addressed:

Validation before side effects: every argument check (NULL or empty array, NULL elements, duplicates, unknown names, and "previous call still in effect" via to_regclass()) runs before the temp table is created, a lock is taken or anything is altered.
Locking fallback: _event_trigger__lock() tries FOR UPDATE and falls back to pg_advisory_xact_lock only on insufficient_privilege. The advisory key is built from two int4 casts of oids, which wrap rather than error. The code comment says the fallback only serializes callers of this function.
Deadlock avoidance: disable() locks in COLLATE "C" order and enable() restores in the same order, so concurrent callers can't take locks in opposite orders.
Cleanup order: enable() still drops its bookkeeping table before restoring any trigger.

Minor notes (optional)

Empty array is now rejected. event_trigger__disable('{}') used to be a no-op and now raises. That is fine for callers passing literal arrays, but it contradicts the earlier "'{}' is accepted" reply in this thread. It's worth making sure nothing relies on the old behavior.
The unknown-name check runs before the lock is taken. The defensive re-check after the lock covers a trigger dropped in between, so this is sound. The post-lock error carries the same message as the pre-lock one. That's fine, but you can't tell from the error which check fired.
The advisory-lock path is only as safe as the catalog read. After taking the lock it re-reads evtenabled without FOR UPDATE. A raw ALTER EVENT TRIGGER from another session can still slip in between that read and the DISABLE. This is the limitation the comment already states, and I don't see a way around it without catalog privileges.

I made no code changes.

…rage - Move _event_trigger__lock() ahead of the WARNING block so the block documents event_trigger__disable()/__enable() again; state the advisory fallback's actual scope (other callers of disable() only). - Use pg_event_trigger column %TYPE declarations in the functions this PR adds. - Drop the "nothing was changed" assertions (a rolled-back call can't fail them) and add error-precedence tests for an empty array and a NULL element. - Correct the load.sql comment: a failed check does not fail the make run on any PostgreSQL version.

github-actions · 2026-10-30T22:35:18Z

Claude finished @jnasbyupgrade's task in 19s —— View job

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

The /code-review skill failed to run, so this is a manual read of _event_trigger__lock(), event_trigger__disable() and event_trigger__enable() in sql/object_reference.sql (L1614-1817). I didn't build or run the tests.

No blocking bugs found.

The two review requests from 21:49 are addressed in the current code:

Validation before side effects. disable() checks the following before it creates the temp table, takes a lock or runs any ALTER:
- NULL or empty array
- NULL elements
- duplicate names
- unknown names
- a previous call still in effect (to_regclass())
The one check left after the lock is the defensive "dropped in between" re-check (L1757). enable() checks for a matching disable() first (L1780).
Lock fallback and ordering.
- _event_trigger__lock() falls back to pg_advisory_xact_lock only on insufficient_privilege. The EXCEPTION block's subtransaction rolls back the failed FOR UPDATE, so the outer transaction stays usable.
- disable() iterates ORDER BY n COLLATE "C" (L1746) and enable() restores in the same order (L1785).
- The comment block says the advisory fallback doesn't cover a raw ALTER EVENT TRIGGER elsewhere.

Minor notes (optional)

The empty array is now rejected. The earlier reply in this thread says '{}' is accepted as a no-op, and L1711 now raises on it. Nothing in the repo appears to depend on the old behavior, but that reply is now stale.
The advisory fallback re-reads evtenabled after taking the lock but doesn't block a raw ALTER EVENT TRIGGER (L1636-1640). The comment already states this limitation, so there is nothing to fix.
Both "does not exist" errors share one message (L1730 and L1758). Whether the pre-lock check or the post-lock re-check fired isn't visible from the error. This is cosmetic.
CASE in enable() has no ELSE (L1806). This was discussed earlier and I agree with leaving it.

I made no code changes.
· branch event-trigger-disable

jnasbyupgrade added 2 commits October 30, 2026 01:48

jnasbyupgrade added this pull request to stack #46 October 30, 2026 06:52

jnasbyupgrade mentioned this pull request Oct 30, 2026

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

Closed

jnasbyupgrade mentioned this pull request Oct 30, 2026

Refuse to track object_reference's own extension members #44

Merged

Base automatically changed from refuse-self-tracking to master October 30, 2026 21:17

Merge master into event-trigger-disable

aea7f4d

jnasbyupgrade commented Oct 30, 2026

View reviewed changes

jnasbyupgrade added 2 commits October 30, 2026 16:53

Correct load.sql's note on which pg_regress versions fail on a failed…

f2deadb

… check

Conversation

jnasbyupgrade commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Known open item

Uh oh!

coderabbitai Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Observations

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Nits and optional hardening

Uh oh!

jnasbyupgrade commented Oct 30, 2026

Uh oh!

jnasbyupgrade commented Oct 30, 2026

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Remaining optional nits

Uh oh!

jnasbyupgrade Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

jnasbyupgrade Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Minor notes (optional)

Uh oh!

github-actions Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review: PR #45 (event_trigger__disable()/__enable())

Minor notes (optional)

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jnasbyupgrade commented Oct 30, 2026 •

edited

Loading

coderabbitai Bot commented Oct 30, 2026 •

edited

Loading

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (event_triggerdisable()/enable())

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

jnasbyupgrade Oct 30, 2026 •

edited

Loading

jnasbyupgrade Oct 30, 2026 •

edited

Loading

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (`event_triggerdisable()`/`enable()`)

github-actions Bot commented Oct 30, 2026 •

edited

Loading

Code review: PR #45 (`event_triggerdisable()`/`enable()`)