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), #45 (comment), #45 (comment)

Replaces the 0.1.0 -> stable update script's session_replication_role = replica bracket with a reusable internal pair, _object_reference.event_trigger__disable(name[]) and event_trigger__enable(). disable() records each named event trigger's exact prior evtenabled and disables it; enable() restores that state rather than assuming origin. The update script disables all three of the extension's event triggers for its whole body and restores them with one enable() at the end; no trigger tries to recognize the extension's own script.

Closes #40

Details

Sentinel. disable() first creates a no-op ddl_command_end event trigger, __object_reference__event_trigger_sentinel, on a pg_temp function, and enable() drops both after restoring every state. While every sql_drop/table_rewrite/ddl_command_end event trigger is disabled, Postgres collects the commands nested in an extension script onto the enclosing CREATE/ALTER EXTENSION and replays them to its ddl_command_end. There, pg_event_trigger_ddl_commands() fails on objects dropped since on PG12/13 (cache lookup failed for namespace), and on PG14+ hands capture the update's own objects. One enabled event trigger prevents that. The function lives in pg_temp because capture skips pg_temp (0.1.0's capture body too), so creating it is never captured, inside or outside an extension script.
Update-script bootstrap. 0.1.0 has no disable(), so the update script starts by doing by hand what disable() does for all three triggers: a state table, the sentinel, COLLATE "C" order and lock handling. Unlike disable(), it has no pre-check for an existing sentinel (a committed one fails with the plain "already exists" error) and its state table has no commit check (below), so the update path doesn't get that check; enable() skips the check cleanup when the trigger is absent. Its final enable() undoes it. _etg_capture and _etg_fix_identity keep their 0.1.0 bodies.
Commit check. disable() also puts a deferred constraint trigger (__object_reference__event_trigger_state_check, ENABLE ALWAYS) on the temp state table. If the transaction commits without enable() having run, the commit fails with SQLSTATE 55000 instead of leaving event triggers disabled; enable() empties the table, fires the check, and drops the table.
Transactional. ALTER EVENT TRIGGER is transactional: other sessions don't see a disable until commit, and a rolled-back caller undoes it without any cleanup logic.
Locking. _object_reference._event_trigger__lock(name) row-locks the trigger's pg_event_trigger row FOR UPDATE before its state is read, which blocks a concurrent raw ALTER EVENT TRIGGER of that trigger until commit. A role without UPDATE on the catalog (SQLSTATE 42501) can't take that lock; the helper then raises a WARNING naming the trigger and reads evtenabled unlocked, in _event_trigger__lock() and in the update script's bootstrap alike. The tests exercise only the helper's fallback; the bootstrap's is untested because the update runs as superuser there, and its WARNING text matches the helper's. The remaining race needs a concurrent raw ALTER of the same trigger during an administrator's own update. Names are locked and restored in COLLATE "C" order, so concurrent callers can't deadlock on lock order.
Validation. disable() checks its arguments before any side effect. A NULL or empty array, a NULL element or a duplicate name raises invalid_parameter_value; an unknown trigger raises undefined_object; a previous call still in effect, or a sentinel event trigger left by another session, raises object_not_in_prerequisite_state. enable() without a matching disable() raises object_not_in_prerequisite_state.
test/sql/event_trigger_disable.sql (40 assertions) covers prior-state preservation, the sentinel existing during a call and being gone (function included) after enable(), an existing sentinel rejected before anything changes, each argument error with its SQLSTATE, "C"-order processing, the lock helper for a role that can't row-lock the catalog (when the pg_event_trigger row lock isn't permitted, it falls back to reading evtenabled without the lock and emits a WARNING that the test silences), the commit check under the origin and replica session_replication_role (fails before enable(), passes after), and disabling zzz_object_reference_capture itself stopping capture until enable(). The file runs in a transaction that is never committed, so no other session sees its disables.
Update-mode checks (make test--update): test/build/update.sql runs four rolled-back scenarios (its output is compared, so a failed assertion fails the build step; it prints nothing in other modes); test/install/load.sql then does a plain committed update. Scenarios 1-3 run with a capture group active. (1) All three triggers at their default state; 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. (4) The triggers start as DISABLE, ENABLE REPLICA and ENABLE ALWAYS and must be restored to exactly those states.
A failed load.sql install or update fails make test--update. pgxntool ignores installcheck's exit status and fails make test only when regression.diffs exists, and load.sql's output is never compared (pg_regress 16+ prints not ok for it, PG12's prints ok). Instead test/load.sql raises in every later test file when update or existing mode left the extension uninstalled or at a version other than the one under test (in update mode test_update_to if set, else default_version; in existing mode default_version), so those files fail and make exits non-zero. test/dump/load_all.sql installs the extension itself and sets the mode to fresh first. CI runs make verify-results (fresh mode) and make test--update.
make test--update runs the update-path test (make test TEST_LOAD_SOURCE=update); test-update is a synonym.
test/install/load.sql sets client_min_messages = WARNING before installing the old version, so the install's NOTICEs don't appear in the test output.
test/build/expected/build.out gains four blank lines, one per new create_function call.

`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: a167ad54-7515-4f7e-8593-699ec2cb2e2b

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

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: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.

Claude here. Changed in 266688a: _event_trigger__lock() no longer takes an advisory lock. An advisory lock only serializes other callers of this function, not a raw ALTER EVENT TRIGGER, so it bought nothing.

It still tries SELECT ... FOR UPDATE on the trigger's pg_event_trigger row. On insufficient_privilege (42501) only, it now reads evtenabled without a lock and carries on, with no NOTICE or WARNING. That's my reading of handling environments where the FOR UPDATE lock isn't possible: run unlocked rather than fail. Without catalog UPDATE rights the row can't be locked, so the remaining race needs a concurrent raw ALTER EVENT TRIGGER of the same trigger during an administrator's own update. The update script's hand-written bootstrap (0.1.0 has no disable()) handles the lock the same way.

Would you rather this path raise a NOTICE, or fail with an error, instead of staying silent?

Lets raise a warning

Claude here. Commit 5a5d2ef: the 42501 (insufficient_privilege) fallback now raises a WARNING naming the event trigger, in both _event_trigger__lock() and the update script's bootstrap, then reads the state without a lock. The test run as an unprivileged role covers it via the expected output.

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

event_trigger__disable() now creates a no-op ddl_command_end event trigger on a pg_temp function before disabling anything, and event_trigger__enable() drops it after restoring every state. While every sql_drop/table_rewrite/ddl_command_end trigger is disabled, Postgres collects the commands nested in an extension script onto the enclosing ALTER EXTENSION and replays them to its ddl_command_end, where pg_event_trigger_ddl_commands() fails on dropped objects before PG14 and hands capture the update's own objects on PG14+. The 0.1.0 -> stable update script disables all three event triggers at its start (by hand, since 0.1.0 has no disable()) and restores them with one enable() at the end. _etg_capture and _etg_fix_identity no longer recognize the update script themselves, so their 0.1.0 bodies are kept unchanged. _event_trigger__lock() no longer falls back to an advisory lock: on insufficient_privilege it reads evtenabled without a lock. disable() and enable() raise undefined_object, invalid_parameter_value or object_not_in_prerequisite_state as appropriate, and the tests check the SQLSTATE.

event_trigger__disable() raised the same 'does not exist' text for a trigger missing at validation and for one dropped between validation and the lock. The latter now says it was dropped concurrently (same ERRCODE). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

github-actions · 2026-11-02T01:51:53Z

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

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

The /code-review skill failed to run again. This is a manual read of _event_trigger__lock(), event_trigger__disable() and event_trigger__enable() in sql/object_reference.sql (L1586-1742). I didn't build or run the tests, and I didn't read the update script or the test files in this pass.

I found no blocking bugs.

Validation order. disable() checks the array (NULL or empty, NULL element, duplicate) and then unknown names. It checks "previous call still in effect" last, before it creates anything. Nothing is created or disabled before those checks pass.
Locking. _event_trigger__lock() falls back to a plain read only on insufficient_privilege. The EXCEPTION block's subtransaction undoes the failed FOR UPDATE.
Order. disable() iterates ORDER BY n COLLATE "C" and enable() restores in the same order.
Cleanup order. enable() drops the state table first, restores each trigger, and drops the sentinel event trigger last.

Notes (optional)

Sentinel left behind if disable() is committed without enable(). disable() creates a persistent event trigger, __object_reference__event_trigger_sentinel, that points at a pg_temp function (L1667-1671). If the transaction commits and enable() never runs, the catalog keeps that event trigger. It also depends on a function that disappears when the session ends. Normal use, where the update script runs in one transaction, is fine. The to_regclass() "previous call" check can't see a leftover from a dead session. Consider documenting this, or checking for an existing sentinel trigger by name.
Rejected empty array. L1627 now raises on '{}'. The earlier reply in the thread says '{}' is accepted, so that reply is stale.
Shared error text. The pre-lock and post-lock "does not exist" errors use the same message (L1654 and L1684). This is cosmetic.
CASE without ELSE in enable() (L1727). This was discussed earlier and left as is.
· branch event-trigger-disable

jnasbyupgrade · 2026-11-02T19:04:39Z

Claude here. Correction to my earlier reply (#45 (comment)): it is out of date on two points.

(a) That reply quoted the review as saying:

'{}' is accepted as a no-op, which seems fine.

event_trigger__disable('{}') now raises invalid_parameter_value.

(b) That reply said of event_trigger__disable(NULL): "No change … an explicit NULL check would only restate the error PL/pgSQL already raises." event_trigger__disable(NULL), and a NULL element, now raise invalid_parameter_value from an explicit check.

Both were added while moving the caller sanity checks ahead of any side effect (#45 (comment)).

jnasbyupgrade · 2026-11-02T19:04:52Z

Claude here. Resolution of the three automated review bodies (#45 (comment), #45 (comment), #45 (comment)).

Sentinel left behind if disable() is committed without enable() (6567178996 note 1): questioned, no code change. The sentinel event trigger depends on its pg_temp function, so PostgreSQL drops it automatically when the creating session ends. Until then it is a no-op that fires harmlessly in other sessions. Verified on PG12 and PG17: the count went from 1 to 0 after the session ended. So it does not stay in the catalog. The real consequence of committing disable() without enable() is that the disabled triggers stay disabled and their saved prior states are lost with the session's temp table. That is the open item in Standard internal update-time disable mechanism, and refuse to self-track extension-owned objects #40 ("should get a clear failure if they do") and is not changed in this PR.
Empty array rejected (6567178996 note 2, 6534939150 note 1, 6534733484 note 1): intentional. The earlier reply saying '{}' is accepted was stale; corrected in Add event_trigger__disable()/__enable() for update scripts #45 (comment).
Shared "does not exist" text (6567178996 note 3, 6534939150 note 3, 6534733484 note 2): fixed in 4ce1661. The post-lock re-check now raises event trigger "%" was dropped concurrently, still undefined_object.
CASE without ELSE (6567178996 note 4, 6534939150 note 4): answered earlier, unchanged.
Advisory-lock notes (6534939150 note 2, 6534733484 note 3): no longer apply. The advisory fallback was removed in 266688a.

github-actions · 2026-11-02T19:07:13Z

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

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

The /code-review skill failed to run again, so this is a manual read of the sql/object_reference.sql diff against origin/master. I didn't build or run the tests. I didn't re-read the update script or the test files in this pass.

No blocking bugs found.

Validation order. disable() checks the array, unknown names and "previous call in effect" before it creates the sentinel, the state table or any lock. enable() checks for a matching disable() first.
Lock fallback. _event_trigger__lock() falls back to an unlocked read only on insufficient_privilege. The EXCEPTION subtransaction undoes the failed FOR UPDATE.
Order and cleanup. disable() and enable() both use COLLATE "C" order. enable() drops the state table first, restores every trigger, then drops the sentinel and its function.

Notes (optional)

Sentinel can outlive its session.
- The sentinel event trigger is persistent, but its function is in pg_temp.
- If a transaction commits after disable() without enable(), the trigger stays in the catalog and its function disappears when the session ends.
- Every ddl_command_end in the database would then fail, because the trigger function no longer exists.
- The to_regclass() check can't see this state from a new session. The next disable() fails with a plain event trigger ... already exists from CREATE EVENT TRIGGER.
- Callers are the extension's own scripts, which run in one transaction, so this is unlikely. A one-line comment in the header would help.
- A pre-check that raises a clearer error would also help (a sentinel row in pg_event_trigger with no live temp state table).
The "does not exist" message appears twice. The pre-lock and post-lock errors differ in wording, so the two cases can be told apart. This is fine as is.
CASE in enable() has no ELSE. This was discussed earlier and I agree with leaving it.
Silent unlocked fallback. On the NOTICE-or-error question in the 266688a thread: I'd keep it silent. A NOTICE would appear in every managed-service update with nothing the user can do about it.

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

…ger_disable.sql Both sections ended in a cleanup enable(); as one closing sequence that leaves a disable() in effect (the file's transaction is never committed) they need none.

github-actions · 2026-11-03T18:38:57Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (e9aa9d6). I did not build the extension or run the tests.

I found no blocking bugs. e9aa9d6 moves the stray-sentinel and nested-disable() tests to the end of test/sql/event_trigger_disable.sql, so the closing disable() needs no cleanup enable(). It changes tests only.

The plan adds up to 35 (7+4+1+1+1+3+4+3+5+4+2), and test/expected/event_trigger_disable.out starts with 1..35 and lists 35 ok lines.
The rejected stray-sentinel call, the successful disable() and the nested-call rejection still run in the order they depended on. The "Must stay last" comment says why the order matters.
The two trailing search_path assertions don't depend on trigger state, so the call left in effect doesn't affect them.

Notes

The PR description is stale. The test/sql/event_trigger_disable.sql bullet says "46 assertions" and "which error wins over a call still in effect". The file now has 35 assertions and no precedence tests.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

…e plan comment The plan comment for the closing block listed two of its four assertions. It now names each one: the stray sentinel is rejected, no state table is created, disable() succeeds once the stray is dropped, then a nested disable() is rejected.

github-actions · 2026-11-03T18:51:12Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (5e3959d). I did not build the extension or run the tests.

I found no blocking bugs. 5e3959d only edits a comment in the plan() call of test/sql/event_trigger_disable.sql. It now names all four closing-sequence assertions, and the count stays at +4. The expected output still starts with 1..35 and has 35 ok lines.

Notes

The PR description is stale. The test/sql/event_trigger_disable.sql bullet says "46 assertions" and mentions "which error wins over a call still in effect". The file now has 35 assertions and no precedence tests.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

… comment

github-actions · 2026-11-03T20:21:33Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (a05bc78). I did not build the extension or run any tests.

I found no blocking bugs. a05bc78 edits only the header comment of event_trigger__disable(), in both sql/object_reference.sql and sql/object_reference--0.1.0--stable.sql. It now says the change commits or rolls back with the caller's transaction, so an error needs no cleanup logic, but a caller that commits without calling enable() leaves the triggers disabled. That matches the code and the reply to the last review comment. There are no code changes.

Notes

The PR description is stale. Its test/sql/event_trigger_disable.sql bullet should match the file as it stands now (35 assertions, per the earlier reviews). I haven't re-checked the count in this pass.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

github-actions · 2026-11-03T20:42:32Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (9d77972). I did not build the extension or run the tests.

I found no blocking bugs. 9d77972 only rewords the header comment of event_trigger__disable(), in both sql/object_reference.sql and sql/object_reference--0.1.0--stable.sql. It now says an error in the same transaction needs no cleanup logic, and that a commit without enable() leaves the triggers disabled and loses the saved states when the session ends. That matches the code and the earlier thread. There are no code changes.

Notes

Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and CI covers fresh mode only (#19).

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

…le() A deferred constraint trigger on the session's event-trigger state table raises at commit while any state row remains. enable() empties the table and fires the pending check before dropping it, so a later disable() still works.

github-actions · 2026-11-03T20:51:21Z

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

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

I read the latest commit (b36dd9a) and the sql/object_reference.sql diff. I did not build the extension or run the tests. The /code-review skill was not run, so this is a manual review.

I found no blocking bugs. b36dd9a adds a deferred constraint trigger on the state table, so a commit without enable() now fails.

Check logic. _event_trigger__state_check() raises while any state row remains. disable() creates the constraint trigger only after the ALTER ... DISABLE loop, then inserts the rows. The trigger's own CREATE therefore doesn't fire a capture trigger the caller left enabled.
enable(). It deletes the rows and runs SET CONSTRAINTS ... IMMEDIATE. That fires the pending events against an empty table, so they pass. The table can then be dropped, and a later disable() in the same transaction still works.
Caller settings. ENABLE ALWAYS keeps session_replication_role from skipping the check. The explicit SET CONSTRAINTS ... DEFERRED stops a caller's earlier SET CONSTRAINTS ALL IMMEDIATE from firing it at insert time.
Rollback. A failed commit rolls back the disables, so the triggers keep their original state. The header comment now says this, and says a hand-run ALTER EVENT TRIGGER is not covered.
Duplicate code. The same change is mirrored in the update script's bootstrap block.

Notes (optional)

The PR description is stale. It doesn't mention the commit-time check. It also says test/sql/event_trigger_disable.sql has 35 assertions, and that count has probably changed. Please re-check the count.
A stale sentinel can still occur. Only a commit that bypassed the check, such as a dropped constraint trigger, can leave one behind. The error HINT now says "left behind", which fits.
The earlier points still stand as answered in the thread. These are the CASE without ELSE, the save logic duplicated in the bootstrap block, and CI covering fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

…ALWAYS event_trigger__enable() now drops the commit-check trigger's state only when the trigger exists: the update script's hand-written disable creates the state table without it, so enable() failed there. The replica-role test now sets session_replication_role before disable(), so ENABLE ALWAYS on the check trigger is what makes it fail. Document that an active capture group must be named in disable()'s arguments, and correct stale comments.

github-actions · 2026-11-03T21:03:06Z

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

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

The /code-review skill failed to run again, so this is a manual read of the latest commit (27c26ef) and the disable()/enable() bodies in sql/object_reference.sql (L1739-1822). I did not build the extension or run the tests.

I found no blocking bugs.

Commit check. The constraint trigger is created after the disable loop and is ENABLE ALWAYS. The explicit SET CONSTRAINTS ... DEFERRED stops an earlier SET CONSTRAINTS ALL IMMEDIATE from firing it at insert time.
enable() path. It empties the state table and sets the check IMMEDIATE. It then drops the table before restoring any trigger. The check trigger is AFTER INSERT, so the DELETE doesn't fire it, and DROP TABLE no longer fails on pending events.
Update-script path. The hand-written bootstrap creates the state table without the check trigger. The new EXISTS guard in enable() handles that.

Notes (optional)

The commit check doesn't cover the update script. Its bootstrap has no check trigger, so a commit without enable() isn't caught there. That is fine in practice, because enable() is the script's last statement.
The PR description is stale. It doesn't mention the commit-time check, and its assertion count should be re-checked.
Settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and CI runs fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

Test 36 now claims only that the check passes after enable(); the tolerance of a preceding SET CONSTRAINTS ALL IMMEDIATE moves to the description of the next disable(). A new test records that SET CONSTRAINTS ALL IMMEDIATE between disable() and enable() raises.

github-actions · 2026-11-03T21:08:05Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (57aa966). I did not build the extension or run the tests.

I found no blocking bugs. 57aa966 trims the check-trigger comment in sql/object_reference.sql and in sql/object_reference--0.1.0--stable.sql. It also splits the commit-check tests in test/sql/event_trigger_disable.sql and adds one.

Plan count. The commit-check block goes from +10 to +11, and test/expected/event_trigger_disable.out starts with 1..46. The listed ok lines run 1 to 46.
New test 39. SET CONSTRAINTS ALL IMMEDIATE between disable() and enable() must raise SQLSTATE 55000. This pins down that the check trigger starts out deferred. Test 38 covers the case where the caller already ran SET CONSTRAINTS ALL IMMEDIATE. throws_ok wraps the statement in a subtransaction, so the failed statement shouldn't affect the following enable(). I did not run it to confirm.
Comment trim. The removed sentences said the constraint trigger is created after the disable loop, and that creating it fires any capture trigger still enabled. The code still creates it after the loop. Nothing else in the comments now explains why it must come after the loop, so a future reorder wouldn't be warned off.

Notes (optional)

Ordering rationale. Consider keeping one short line saying the check trigger is created after the disable loop. Creating it earlier would fire any capture trigger the caller left enabled.
Stale PR description. The test/sql/event_trigger_disable.sql bullet still says 35 assertions and doesn't mention the commit-time check. The file now has 46 assertions.
Settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, the commit check doesn't cover the update script's bootstrap, and CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

jnasbyupgrade · 2026-11-03T23:04:36Z

+SELECT is(
+  (SELECT count(*)::int FROM pg_catalog.pg_locks WHERE locktype = 'advisory' AND pid = pg_backend_pid())
+  , 0
+  , 'helper took no advisory lock'


Uhm, why is this here???

Claude here. Removed in 8668c0a. It asserted that the helper takes no advisory lock, but the advisory lock was dropped in 266688a, so it only pinned the absence of removed behavior. (An earlier reply on this thread answered a different question, the pgTAP one.) If you wanted it kept for another reason, say so and it will be restored.

jnasbyupgrade · 2026-11-03T23:07:43Z

+ * unlocked, with a WARNING naming the trigger, instead of an error. The DO
+ * block runs as that role, and first proves the role really cannot FOR UPDATE
+ * the catalog. The WARNING is suppressed because whether it is raised depends
+ * on the environment's catalog privileges.


If this is using a non-superuser role then it definitely wouldn't be able to FOR UPDATE on the catalog, regardless of the environment. What we can't assume is that the regular role running the test is a true superuser (ie, it won't be on RDS).

Claude here. Fixed the comment (b37123e): it no longer says the WARNING depends on the environment's catalog privileges; the test role never has them, so the WARNING is always raised, and it is suppressed only to keep it out of the expected output.

Claude here. I read your second sentence as explaining why the original comment's reasoning was wrong, not as a request to change the suite's assumption that the runner is a superuser (the shared test conventions set that). The WARNING stays suppressed, as requested in 4457948113, so the expected output stays free of it. If you would rather assert it or leave it in the expected output, say so.

Claude here: Understood. No non-superuser mode will be exposed via make, and the existing role-based test (the helper called under SET LOCAL ROLE as a role that cannot row-lock the catalog) stays as the non-superuser sanity check. The WARNING suppression in that test also stays as it is, so no further change for this thread.

jnasbyupgrade · 2026-11-03T23:10:24Z

+    FROM _object_reference.object_group__object m
+      JOIN _object_reference.object o USING (object_id)
+    WHERE m.object_group_id = (object_reference.object_group__get('event_trigger_disable_test_group')).object_group_id)
+  , '{public.event_trigger_disable_test__after_enable}'::text[]


Uh, this is really looking like it should be results_eq or bag_eq, with a VALUES().

Claude here. Fixed (b37123e): the three array_agg/is() comparisons (processing order, restored states, captured objects) are now results_eq against a VALUES list.

jnasbyupgrade · 2026-11-03T23:11:54Z

+  $$SELECT _object_reference.event_trigger__disable('{event_trigger_disable_test__a}')$$
+  , 'disable() again after SET CONSTRAINTS ALL IMMEDIATE (the check trigger must start out deferred)'
+);
+SELECT throws_ok(


SIGH if you did this BEFORE the passing case above you wouldn't need to re-run a disable. You should always be looking at ways to reduce needless state manipulation by leveraging existing state in the test.

Make sure this is in ../ai/

Claude here. Fixed (b37123e): the SET CONSTRAINTS ALL IMMEDIATE failure case now runs right after the replica check, while disable() is still in effect, ahead of the passing case. The extra disable()/enable() cycle is gone; the final disable() in the closing sequence still covers that a disable() after ALL IMMEDIATE does not fail early (the check trigger starts out deferred).

Claude here. Recorded as a rule in test/CLAUDE.md in Postgres-Extensions/ai: Postgres-Extensions/ai#46. I read "this" as the "reduce needless state manipulation by leveraging existing state" lesson (the pgTap/results_eq point is already in that file). Say if a different rule was meant.

Claude here. 3356fa9 collapses one more cycle: the origin-role commit-check assertions (check fails, and SET CONSTRAINTS ALL IMMEDIATE fails, between disable() and enable()) now run inside the existing unsorted-names disable()/enable() pair instead of their own. The session_replication_role = replica cycle stays separate: the check trigger's event is queued when disable() inserts, so a replica-role check needs its own disable(), and without an origin-role disable() a trigger enabled only for replica would go unnoticed. Mutations (no SET CONSTRAINTS ... DEFERRED in disable(); no ENABLE ALWAYS; ENABLE REPLICA; neutered check function) all still fail the suite. The all-three-restored check is now a bag_eq against a VALUES list.

…to reuse state Replace to_regprocedure/to_regclass checks with has_function/hasnt_function/ hasnt_table, array_agg comparisons with results_eq, and run the SET CONSTRAINTS ALL IMMEDIATE failure case before enable() instead of re-running disable().

github-actions · 2026-11-03T23:18:18Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (b37123e). I did not build the extension or run the tests.

I found no blocking bugs. b37123e changes tests only. It does what the last review comments asked:

pgTAP assertions. to_regprocedure/to_regclass checks became has_function/hasnt_function and hasnt_table. The array_agg comparisons became results_eq against VALUES.
- The has_function and hasnt_function calls look up the schema through pg_my_temp_schema(). The temp schema exists at both call sites, because disable() has created the state table by then, so the schema argument is not NULL.
- hasnt_table('__object_reference__event_trigger_state') has no schema argument and relies on temp-schema visibility. A same-named table in a visible schema would give a false pass, which is unlikely.
State reuse. The SET CONSTRAINTS ALL IMMEDIATE failure case now runs before enable(), so the extra disable() and enable() cycle is gone. The "starts out deferred" check moved into the final disable() assertion's description, which still holds, because that call follows a SET CONSTRAINTS ALL IMMEDIATE.
Plan count. The commit-check block goes from +11 to +9, which matches the nine assertions now in it. The total is 44, and test/expected/event_trigger_disable.out starts with 1..44 and lists 44 ok lines.
results_eq queries. The "all three restored" and "only the table created after enable() was captured" queries have no ORDER BY. That is safe here, because every expected row is identical in the first and has one row in the second. The "C order" query orders by ctid and compares the name column against a typed VALUES.

Notes

The PR description is stale. The test/sql/event_trigger_disable.sql bullet says 35 assertions, and the file now has 44. The bullet doesn't mention the commit-time check, which fails a commit that follows disable() without enable(). The "Validation" bullet also doesn't list it.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, the commit check doesn't cover the update script's bootstrap, and CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

_event_trigger__lock() no longer takes an advisory lock, so the test only pinned the absence of removed behavior.

The origin-role commit-check assertions now run inside the unsorted-names pair instead of their own, leaving one dedicated pair for the session_replication_role = replica case (the check trigger's event is queued when disable() inserts, so replica needs its own disable()). Compare the restored states with bag_eq against a VALUES list.

…escription throws_ok rolls the failed disable() back in a subtransaction, so a state table created before the sentinel check would vanish before hasnt_table looked; the assertion could not fail.

github-actions · 2026-11-03T23:32:07Z

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

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

I read the latest commit (49b85cc) in full. I did not build the extension or run the tests. The /code-review skill was not run, so this is a manual check.

I found no blocking bugs. 49b85cc changes tests only. It removes the hasnt_table assertion after the rejected stray-sentinel disable().

The removal is justified. throws_ok runs the failing call in a subtransaction. A state table created before the sentinel check would be rolled back before hasnt_table looked, so the assertion could not fail.
Plan count. The plan adds up to 40 (7+4+1+1+1+3+4+2+2+5+5+3+2). The closing-sequence block went from +4 to +3. test/expected/event_trigger_disable.out starts with 1..40 and lists 40 ok lines.
Moved explanation. The "starts out deferred" claim left the test description and is now a comment above the lives_ok. The earlier SET CONSTRAINTS ALL IMMEDIATE is still what makes that call meaningful.

Notes

The PR description is stale. The test/sql/event_trigger_disable.sql bullet says 35 assertions and does not mention the commit-time check. The file now has 40. The "Validation" bullet also does not list the commit check.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, the commit check does not cover the update script's bootstrap, and CI runs fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

Add a CI step that runs the suite against a database installed at TEST_UPDATE_FROM and then ALTER EXTENSION UPDATEd. Describe test-update as a human shorthand for the TEST_LOAD_SOURCE form.

github-actions · 2026-11-03T23:55:06Z

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

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

I read the latest commit (093dba1) and its diff. I didn't build the extension or run the tests, and I didn't re-read the rest of the PR in this pass.

I found no blocking bugs. 093dba1 adds one CI step and rewords a Makefile comment.

CI step. make verify-results TEST_LOAD_SOURCE=update runs after the fresh-mode step. Its comment gives the right reason for not using make test-update: that target wraps make test, which doesn't fail the step on its own. In update mode a failed install or update reaches the exit code through the test/load.sql guard and test/build/update.sql's compared output, as the PR description says.
Makefile. The comment now calls test-update a human shorthand and points CI at the TEST_LOAD_SOURCE form. It matches the workflow.

Notes

Stale PR description. The description says CI runs fresh mode only, and the Update-mode bullet implies the same. CI now also runs the update path. Reviews on earlier commits said "CI covers fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19)", and that no longer holds. The assertion count in the test/sql/event_trigger_disable.sql bullet should be re-checked against the file too.
Update step depends on TEST_UPDATE_FROM. The step relies on the Makefile default for TEST_UPDATE_FROM. If that default doesn't name an installable 0.1.0 on every PG version in the matrix, this step will fail where the fresh step passes. I didn't run it to confirm.
Settled earlier in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and the WARNING text appears three times.

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

…as a synonym

github-actions · 2026-11-04T00:00:48Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (afa2fc6). I did not build the extension or run the tests.

I found no blocking bugs. afa2fc6 adds test--update as the preferred name for the update-path target and keeps test-update as a synonym. It also updates the CI comment and the test/build/update.sql header comment to match.

Makefile. test--update test-update: is a multi-target rule with one recipe, and both names are in .PHONY. The recipe still recurses with $(MAKE) test TEST_LOAD_SOURCE=update, so the parse-time conditional re-evaluates.
CI. The step still runs make verify-results TEST_LOAD_SOURCE=update. Only its comment changed.

Notes

Stale PR description. It still says make test TEST_LOAD_SOURCE=update and that CI runs fresh mode only. CI now also runs the update path, and the preferred local target is make test--update.
Assertion count. The test/sql/event_trigger_disable.sql bullet should be re-checked against the file.
Settled earlier in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and the WARNING text appears three times.

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

make test exits non-zero on a regression with the vendored pgxntool, so the update-path step needs no CI-specific form. Drop the comments claiming otherwise.

github-actions · 2026-11-04T00:07:06Z

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

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

The /code-review skill failed to run again, so this is a manual check of the latest commit (bd195e8). I did not build the extension or run the tests.

I found no blocking bugs. bd195e8 changes the CI update-path step from make verify-results TEST_LOAD_SOURCE=update to make test--update. It also trims the related comments in .github/workflows/ci.yml and the Makefile.

The new step can fail CI. pgxntool/base.mk:375-376 has test: exit 1 when regression.diffs exists. .IGNORE: installcheck (L314) only stops installcheck's own exit status from failing the build. So the commit message is right that make test exits non-zero on a regression, and the removed "never fails the step" comment was wrong.
The update step no longer parses pgTAP results. The fresh-mode step still runs verify-results, which parses the pgTAP output as well as checking regression.diffs. make test--update only checks regression.diffs. A pgTAP failure in the update run shows up only if it produces a diff against the expected output. The test/event_trigger_disable.sql plan count and ok lines are in the expected file, so a failed assertion there does diff. Whether that holds for every file that runs in update mode I did not check.

Notes

The PR description is stale. It still says "pgxntool ignores installcheck's exit status and fails make test only when regression.diffs exists". That is accurate. But it also still gives the assertion count for test/sql/event_trigger_disable.sql as 35, and the file has changed since.
Settled earlier in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and the WARNING text appears three times.

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

github-actions · 2026-11-04T00:19:42Z

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

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

The /code-review skill failed to run, so this is a manual check of the latest commit (11680f6). I did not build the extension or run the tests.

I found no blocking bugs. 11680f6 rewords one comment in .github/workflows/ci.yml. The new text says the fresh-mode step runs the test chain and then parses the pgTAP results, which make test alone does not. That matches the step's make verify-results command. There are no code changes.

Notes

The PR description is stale. It says CI runs fresh mode only. CI now also runs make test--update.
The assertion count in the description is stale too. The test/sql/event_trigger_disable.sql bullet should be re-checked against the file.
Earlier points are settled in the thread: the CASE in enable() has no ELSE, the bootstrap block duplicates the save logic, and the WARNING text appears three times.

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

jnasbyupgrade and others added 2 commits October 31, 2026 18:49

jnasbyupgrade mentioned this pull request Nov 2, 2026

test/install errors pass silently on PG 9.5-15: pg_regress ignores psql's exit status before PG16 Postgres-Extensions/pgxntool#148

Open

Move stray-sentinel and nested-disable tests to the end of event_trig…

e9aa9d6

…ger_disable.sql Both sections ended in a cleanup enable(); as one closing sequence that leaves a disable() in effect (the file's transaction is never committed) they need none.

jnasbyupgrade commented Nov 3, 2026

View reviewed changes

Comment thread sql/object_reference.sql Resolved

Clarify event_trigger__disable() transactional behavior in its header…

a05bc78

… comment

Clarify event_trigger__disable() error and commit-without-enable() cases

9d77972

jnasbyupgrade commented Nov 3, 2026

View reviewed changes

jnasbyupgrade added 3 commits November 3, 2026 17:26

Remove the no-advisory-lock test from event_trigger_disable

8668c0a

_event_trigger__lock() no longer takes an advisory lock, so the test only pinned the absence of removed behavior.

Drop the vacuous rejected-disable() state-table test; one claim per d…

49b85cc

…escription throws_ok rolls the failed disable() back in a subtransaction, so a state table created before the sentinel check would vanish before hasnt_table looked; the assertion could not fail.

CI: run the update path via make verify-results TEST_LOAD_SOURCE=update

093dba1

Add a CI step that runs the suite against a database installed at TEST_UPDATE_FROM and then ALTER EXTENSION UPDATEd. Describe test-update as a human shorthand for the TEST_LOAD_SOURCE form.

Make test--update the preferred update-path target; keep test-update …

afa2fc6

…as a synonym

CI: run the update path via make test--update

bd195e8

make test exits non-zero on a regression with the vendored pgxntool, so the update-path step needs no CI-specific form. Drop the comments claiming otherwise.

CI: reword the verify-results step comment

11680f6

Conversation

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

Uh oh!

Details

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

github-actions Bot commented Nov 2, 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())

Notes (optional)

Uh oh!

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

Uh oh!

Uh oh!

jnasbyupgrade commented Nov 2, 2026

Uh oh!

github-actions Bot commented Nov 2, 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())

Notes (optional)

Uh oh!

github-actions Bot commented Nov 3, 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())

Notes

Uh oh!

github-actions Bot commented Nov 3, 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())

Notes

Uh oh!

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

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()`)

github-actions Bot commented Nov 2, 2026 •

edited

Loading

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

jnasbyupgrade commented Nov 2, 2026 •

edited

Loading

github-actions Bot commented Nov 2, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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

github-actions Bot commented Nov 3, 2026 •

edited

Loading

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