[jnasbyupgrade]

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: the same state table, sentinel, COLLATE "C" order and lock handling. Its final enable() undoes it. _etg_capture and _etg_fix_identity keep their 0.1.0 bodies.
• 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 (35 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 and which error wins over a call still in effect, "C"-order processing, the lock helper for a role that can't row-lock the catalog (it warns, returns evtenabled and takes no advisory lock), 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 fresh mode only.
• test/build/expected/build.out gains three blank lines, one per new create_function call.

[coderabbitai]

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: 816c629d-bcbc-4118-b241-25520f8f1a7a

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

• X
• Mastodon
• Reddit
• LinkedIn

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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

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

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

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

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

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

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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

1. 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.
2. 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.
3. 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.
4. 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]

Written by Claude.

Replies to items 2-5 of the automated review.

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

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

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

5. 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]

Written by Claude.

Re: #45 (comment)

4. 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).

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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

1. 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.
2. Whitespace-only change. FOR r IN SELECT loses its trailing space in _etg_capture. It is harmless but unrelated to this PR.
3. 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]

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.

[jnasbyupgrade]

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.

[jnasbyupgrade]

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?

[jnasbyupgrade]

Lets raise a warning

[jnasbyupgrade]

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.

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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.

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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)

1. 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.
2. 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.
3. 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.

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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)

1. 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.
2. 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.
3. 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.
4. 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

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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)

1. 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.
2. Rejected empty array. L1627 now raises on '{}'. The earlier reply in the thread says '{}' is accepted, so that reply is stale.
3. Shared error text. The pre-lock and post-lock "does not exist" errors use the same message (L1654 and L1684). This is cosmetic.
4. CASE without ELSE in enable() (L1727). This was discussed earlier and left as is.
   · branch event-trigger-disable

[jnasbyupgrade]

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]

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]

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

---

Code review: PR #45 (event_trigger__disable()/__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)

1. 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).
2. 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.
3. CASE in enable() has no ELSE. This was discussed earlier and I agree with leaving it.
4. 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

[github-actions]

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

---

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

The /code-review skill failed to run again, so this is a manual read of the sql/object_reference.sql diff and the bootstrap block at the top of sql/object_reference--0.1.0--stable.sql. I didn't build or run the tests, and I didn't re-read the test files.

I found no blocking bugs.

• Validation order. disable() checks the array (NULL or empty, NULL element, duplicate), then unknown names, then a previous call in effect, then another session's sentinel. All of these run before the sentinel, the state table, any lock or any ALTER. enable() checks for a matching disable() first.
• Locking. _event_trigger__lock() falls back to an unlocked read only on insufficient_privilege. The EXCEPTION block's subtransaction undoes the failed FOR UPDATE, so the outer transaction stays usable. A trigger dropped in between returns NULL and is caught by the post-lock re-check.
• Order. disable() and enable() both use COLLATE "C". array_agg(evtname ORDER BY ...) and array_agg(evtenabled ORDER BY ...) use the same ordering, so names and states stay paired.
• Cleanup order. enable() drops the state table first, restores every trigger, then drops the sentinel trigger and its function. The sentinel therefore stays enabled until the last trigger is restored.
• Bootstrap. The hand-written block in the update script mirrors disable(): the same sentinel, state table, "C" order and lock handling. The earlier question about 0.1.0 triggers firing on the bootstrap temp objects is answered in the thread, because 0.1.0's capture skips pg_temp.

Notes (optional)

1. Sentinel left behind by a committed disable() with no enable(). The thread already established that PostgreSQL drops the sentinel when the creating session ends, because it depends on the pg_temp function. Until then it is a no-op. The pre-check that raises a clear error for another session's sentinel covers the window in which the creating session is still alive. If that session has already ended, the sentinel is gone and nothing is left to detect.
2. CASE in enable() has no ELSE. This was discussed earlier and I agree with leaving it.
3. The bootstrap block and disable() duplicate the state-saving logic. The PR description explains this, since 0.1.0 has no disable() to call. It means a later change to disable() must be mirrored by hand in the update script.
4. CI runs fresh mode only. The update-mode checks in test/install/load.sql run only under make test-update. A regression in the 0.1.0 → stable restore path wouldn't be caught automatically. The PR description says this, so I'm not asking for a change here.
5. Silent unlocked fallback. 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

[jnasbyupgrade]

Claude here. Summary of the automated-review items still open, with where each stands. Code change for item 1 is in 5d768a2.

1. #45 (comment), note 1: questioned in part.

every ddl_command_end in the database would then fail

That does not happen. The sentinel event trigger depends on its pg_temp function, so PostgreSQL drops the trigger together with the function when the creating session ends. I verified this on PG12 and PG17: the count of sentinel triggers went from 1 to 0 after the session ended.

The part that is true: while that session was still alive, another session's event_trigger__disable() failed with a plain event trigger ... already exists. It now raises object_not_in_prerequisite_state with a HINT (5d768a2), tested in test/sql/event_trigger_disable.sql.

2. Same review, note 2: no action needed (the review says so). Note 3: answered earlier, unchanged.

3. Same review, note 4 (keep the unlocked fallback silent): acknowledged. Whether that fallback should stay silent, emit a NOTICE or raise an error is the maintainer's choice, asked in #45 (comment).

4. #45 (comment), note 1 (the temp-table name usable as a flag; repeated in #45 (comment) and #45 (comment)): no longer applies. The to_regclass guards in _etg_capture and _etg_fix_identity were removed in 266688a. The table name now serves only disable()'s own nested-call check, so a role that creates it can only block its own session's disable().

5. #45 (comment), nit 3 (update-mode CI coverage): update-mode CI is the open #19; the PR description notes the gap.

[github-actions]

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

---

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

The /code-review skill failed to run again, so this is a manual read of the sql/object_reference.sql diff and the bootstrap block at the top of sql/object_reference--0.1.0--stable.sql. I didn't build or run the tests, and I didn't re-read the test files.

I found no blocking bugs.

• Validation order. disable() checks the array (NULL or empty, NULL element, duplicate), then unknown names, then a previous call in effect, then another session's sentinel. All of these run before the sentinel, the state table, any lock or any ALTER. enable() checks for a matching disable() first.
• Locking. _event_trigger__lock() falls back to an unlocked read only on insufficient_privilege. The EXCEPTION subtransaction undoes the failed FOR UPDATE. A trigger dropped in between returns NULL and is caught by the post-lock check.
• Order and pairing. disable() and enable() both use COLLATE "C". Both array_agg calls in enable() use the same ORDER BY, so names and states stay paired.
• Cleanup order. enable() drops the state table first, restores every trigger, then drops the sentinel trigger and its function. The sentinel stays enabled until the last trigger is restored.
• Bootstrap. The hand-written block mirrors disable(): same sentinel, state table, "C" order and lock handling. Its temp objects live in pg_temp, which 0.1.0's capture skips.

Notes (optional)

1. State-saving logic is duplicated. The bootstrap block and disable() hold the same logic, so a later change to disable() has to be mirrored by hand in the update script. The PR description already says why.
2. CASE in enable() has no ELSE. This was discussed earlier and I agree with leaving it.
3. CI covers fresh mode only. The update-mode restore checks in test/install/load.sql run only under make test-update. PR Add extension-update-test and pg-upgrade-stepwise CI jobs #19 tracks update-mode CI.
4. Unlocked fallback is silent. 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

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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, and I didn't re-read the update script or the test files.

I found no blocking bugs.

• Validation order. disable() checks the array (NULL or empty, NULL element, duplicate), unknown names, a previous call in effect, then another session's sentinel. All of this runs before the sentinel, the state table, any lock or any ALTER.
• Locking. _event_trigger__lock() falls back to an unlocked read only on insufficient_privilege. A trigger dropped in between returns NULL and is caught by the post-lock check.
• Order and pairing. Locking, disabling and restoring all use COLLATE "C". Both array_agg calls in enable() share the same ORDER BY, so names and states stay paired.
• Cleanup order. enable() drops the state table first and restores every trigger. It drops the sentinel trigger and its function last, so one trigger stays enabled throughout.

Notes (optional)

1. The state-restore CASE in enable() has no ELSE. This was discussed earlier and I agree with leaving it.
2. The bootstrap block and disable() duplicate the save logic. The PR description explains why. A later change to one has to be mirrored in the other.
3. CI runs fresh mode only. The update-mode checks run only under make test-update. PR Add extension-update-test and pg-upgrade-stepwise CI jobs #19 tracks update-mode CI.
4. The unlocked fallback is silent. I'd keep it silent, because a NOTICE would appear on every managed-service update and the user can't act on it.

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

[github-actions]

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

---

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

The /code-review skill failed to run again, so this is a manual read of the latest commit (5a5d2ef) and its surroundings. I didn't build or run the tests.

I found no blocking bugs. Commit 5a5d2ef does what the maintainer asked in the last inline thread ("Lets raise a warning").

• Warning added in all three places. _event_trigger__lock() (sql/object_reference.sql:1604) and both copies of the bootstrap lock handling in sql/object_reference--0.1.0--stable.sql (L29, L225) raise a WARNING naming the trigger. They then fall back to the unlocked read. The EXCEPTION block catches only insufficient_privilege, so other errors still propagate.
• Test. test/sql/event_trigger_disable.sql switches to \set VERBOSITY terse around the unprivileged-role assertion so the expected output has no source-location line. It restores verbose afterwards, which matches test/pgxntool/psql.sql:5. The WARNING line in test/expected/event_trigger_disable.out sits before assertion 37, which fits how pg_regress interleaves stderr and stdout. It could be fragile if output buffering ever changed.

Notes (optional)

1. The PR description is now stale. The Locking bullet still says the fallback reads evtenabled unlocked "with no NOTICE or WARNING". It now emits a WARNING.
2. The WARNING text is duplicated three times. The update script's bootstrap can't call the helper, so this is expected. A later edit to one message needs to be mirrored in the other two.
3. Earlier notes still stand as answered in the thread. These are the CASE without ELSE, the duplicated save logic, and CI covering only fresh mode (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).

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

[github-actions]

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

---

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

The /code-review skill failed to run again. I checked the latest commit (373e263) and grepped the sql/object_reference.sql diff for its error and warning paths. I didn't build or run the tests.

I found no blocking bugs. 373e263 changes comments and message wording only, as its message says. The validation, locking and restore paths are unchanged from what earlier reviews covered.

Notes (optional)

1. The PR description is current on the warning. The Locking bullet now says the fallback raises a WARNING.
2. The CASE in enable() has no ELSE, and the save logic is duplicated in the bootstrap block. Both were answered earlier in the thread.
3. CI runs fresh mode only. PR Add extension-update-test and pg-upgrade-stepwise CI jobs #19 tracks update-mode CI.

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

[jnasbyupgrade]

It'd be clearer to say something like races with ALTER EVEnT TRIGGER are possible

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: the warning now reads "...reading its state without a lock, so races with ALTER EVENT TRIGGER are possible" (also in the inline copy in the update script).

[jnasbyupgrade]

comment no longer matches structure of code

[jnasbyupgrade]

Claude here: Restructured in 8f48f5f. _event_trigger__lock() has its own short comment directly above it. The overview, the sentinel explanation and the locking note now sit directly above event_trigger__disable(), and every "above"/"below" cross-reference points at the right place or is gone. If you meant a different mismatch, tell me which and I will fix it.

[jnasbyupgrade]

No need for comment; code is obvious.

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: comment removed.

[jnasbyupgrade]

Worth calling out the ORDER BY is to ensure our ordering is always consistent so we can't get deadlocks (at least with ourselves)

[jnasbyupgrade]

Wait, we do NOT need an ORDER BY here! Remove it, and add a comment below where it actually matters.

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: ORDER BY removed from the existence check; the sorted-order comment now sits on the lock loop and says why it matters (same lock order for every caller, so callers can't deadlock on each other). This supersedes the earlier ORDER BY comment.

[jnasbyupgrade]

Eh, given that this is where a race exists, do it before the insert

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: the ALTER EVENT TRIGGER ... DISABLE now runs before the INSERT of the recorded state.

[jnasbyupgrade]

Shouldn't it be opposite order?

[jnasbyupgrade]

Claude here: Keeping the order, and 8f48f5f rewords the comment to say why. The sorted COLLATE "C" order is the lock-acquisition order that prevents deadlock between concurrent callers; release order is irrelevant because every lock is released at commit. When enable() runs in its own transaction after a committed disable(), its ALTER EVENT TRIGGER statements take the row locks again, so it needs the same sorted order as disable(). Reversing it would bring back the deadlock risk against a concurrent caller. Tell me if you still want it reversed.

[jnasbyupgrade]

Need to exclude the WARNING, since we might or might not get it depending on the environment.

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: the test now sets client_min_messages = error around that call, so the WARNING no longer appears in the expected output.

[jnasbyupgrade]

This is too much for a script that's supposedly just loading. Break it into a test_update.sql or similar. Also, it's completely unclear why we're doing this as part of test/install instead of test/build (which at least is intended to rollback). Off-hand it seems it'd be simpler to make it part of build?

[jnasbyupgrade]

Claude here: Done in 8f48f5f. The three rolled-back scenarios and their helpers moved to test/build/update.sql (with test/build/expected/update.out); test/install/load.sql keeps only the committed update, and the state assertion both files use is in test/helpers/event_trigger_state.sql. On install vs build: there is no reason for install. Its output is self-comparing and never asserted, so a failure there could not fail make by itself. test-build runs before the main suite in a fresh database where the extension is not installed, its output is compared, and everything rolls back, which is what the scenarios need. The file prints nothing in fresh/existing mode and runs only when TEST_LOAD_SOURCE=update. Flipping one scenario assertion makes make test-update exit non-zero.

[jnasbyupgrade]

Claude here: Correction to my reply above: test/build/update.sql has four rolled-back scenarios (the fourth checks the D/R/A state across the update), test/install/load.sql holds only the committed install/update step plus existing-mode presence and version checks, with no event-trigger state assertion, and test/helpers/event_trigger_state.sql is used only by update.sql.

[jnasbyupgrade]

Eh... need to be careful about depending on that, since someone running by hand via psql could accidentally commit. Not saying it's worth a bunch of extra crap to deal with that possibility, but we should at least word the comment as we're assuming that this won't commit (uh, and should probably \echo a warning at the end instead-of or after finish.sql).

[jnasbyupgrade]

Claude here: Fixed in 12c5b07: the comment now says the file assumes the transaction is never committed, and an \echo after finish.sql warns a by-hand psql run to ROLLBACK.

[github-actions]

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

---

Code review: PR #45 (event_trigger__disable()/__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, mainly the latest commit (12c5b07). I didn't build or run the tests.

I found no blocking bugs.

• Validation order. disable() checks the array, unknown names, a previous call in effect and another session's sentinel before it creates anything.
• Lock order. Locking and disabling go through ORDER BY n COLLATE "C". The comment now sits on that loop, where it matters. enable() restores in the same order.
• Disable before record. ALTER ... DISABLE now runs before the INSERT into the state table. The FOR UPDATE lock is already held at that point, so the saved state is still the pre-disable state. If the ALTER fails, the statement aborts and the state table rolls back with it.
• Cleanup order. enable() drops the state table, restores each trigger, then drops the sentinel trigger and its function. One trigger stays enabled throughout.
• Lock fallback. _event_trigger__lock() catches only insufficient_privilege, warns, and reads the state unlocked. The reworded warning ("races with ALTER EVENT TRIGGER are possible") matches the review request.

Notes (optional)

1. Warning text appears three times. It's in the helper and in both bootstrap copies in the update script. The update script can't call the helper, so a later edit must be mirrored by hand.
2. CASE in enable() has no ELSE. This was discussed earlier and left as is.
3. CI runs fresh mode only. The update-mode checks in test/install/load.sql run only under make test-update. PR Add extension-update-test and pg-upgrade-stepwise CI jobs #19 tracks update-mode CI.

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

[github-actions]

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

---

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

I read only the latest commit (8f48f5f) in full: its sql/ changes and the start of test/build/update.sql. I did not build the extension or run any tests. I did not run the /code-review skill. I did not re-read the rest of test/build/update.sql, the helper, test/load.sql or test/install/load.sql.

I found no blocking bugs in that commit.

• Disable before record. In the update script's bootstrap, ALTER EVENT TRIGGER ... DISABLE now runs before the INSERT into the state table. This matches disable(). The row lock is already held at that point, so the recorded state is still the prior one. If the ALTER fails, the statement aborts and the state table rolls back with it.
• Comments.
  • The overview, sentinel and locking notes now sit directly above event_trigger__disable(). _event_trigger__lock() has its own short comment.
  • The enable() comment says why restore uses the sorted order.
  • The two cross-references ("see the sentinel note above") still point at the right places.
• Test move. The rolled-back update scenarios moved to test/build/update.sql. It emits no output in any mode, so a failed assertion shows up as a diff against expected/update.out. That is the behavior the earlier review asked for.

Notes (optional)

1. Sentinel wording. The enable() comment ends "(see the sentinel note above event_trigger__disable())". That reads a little oddly, since the note is now only a few lines away. It is cosmetic.
2. Settled points. These were answered earlier in the thread, and I have nothing new on them:
   • the CASE in enable() has no ELSE
   • the save logic is duplicated in the update script's bootstrap
   • CI covers fresh mode only (PR Add extension-update-test and pg-upgrade-stepwise CI jobs #19 tracks update-mode CI)

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

[github-actions]

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

---

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

The /code-review skill failed to run again, so this is a manual check. I read the latest commit (54c95e7) in full. I did not build the extension or run the tests, and I did not re-read the rest of the diff.

I found no blocking bugs. 54c95e7 rewords the enable() lock-order comment in sql/object_reference.sql and in sql/object_reference--0.1.0--stable.sql. It makes no code changes. The new wording ("if run in a later transaction, these ALTERs re-take the row locks, and one order for every caller prevents deadlock") matches the code. Both array_agg calls use the same ORDER BY evtname COLLATE "C".

Notes (optional)

1. The earlier open items are settled in the thread: 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

[github-actions]

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

---

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

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

I found no blocking bugs. 13fb35d moves the D/R/A restore check from test/install/load.sql into test/build/update.sql as scenario 4, and load.sql now only updates. Scenario 4 runs in its own BEGIN/ROLLBACK. It sets DISABLE, ENABLE REPLICA and ENABLE ALWAYS, asserts those states before the update, runs ALTER EXTENSION ... UPDATE, and asserts the same states afterwards. That is the same check as before, now in the file whose output is compared and which rolls back. Dropping the ENABLE reset lines from load.sql is correct, because the committed update no longer changes any trigger state.

Notes (optional)

1. The PR description is stale. The update-mode bullet says "The committed update starts from DISABLE, ENABLE REPLICA and ENABLE ALWAYS and must restore exactly those states." That check is now scenario 4 and is rolled back. The committed update in load.sql starts from the default states. Suggested wording: "(4) The three triggers start as DISABLE, ENABLE REPLICA and ENABLE ALWAYS and must be restored to exactly those states."
2. The test/load.sql guard has no restore check of its own. A restore regression is now caught only through test/build/update.sql's compared output. That is fine under make test-update, and CI still runs fresh mode only (Add extension-update-test and pg-upgrade-stepwise CI jobs #19).
3. 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

[github-actions]

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

---

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

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

I found no blocking bugs. b2af0d0 only edits the header comment of test/build/update.sql. It now says scenarios 1-3 start a capture group, which matches scenario 4 (the D/R/A restore check, which doesn't start one). 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

[jnasbyupgrade]

Uh... or move this chunk to the end so we don't have to worry about __enable...

[jnasbyupgrade]

Claude here: Done in e9aa9d6. I read "this chunk" as the nested-disable section: it and the stray-sentinel section both ended in a cleanup enable(), so they now form one closing sequence that leaves a disable() in effect (this file's transaction is never committed), and both cleanup calls are gone. Say if you meant a different chunk.

[jnasbyupgrade]

Eh... I don't think any of this is worth it. There's nothing all that magical about what order we're checking starting conditions/inputs in, certainly nothing critical. Given that it certainly doesn't deserve dedicated testing.

[jnasbyupgrade]

Claude here: Fixed in ddfe844: removed the tests that checked which argument error wins when several apply, along with the lead-in comment reference to them.

[jnasbyupgrade]

I'm pretty sure best practice is to release locks in REVERSE order of how they were taken, no?

[jnasbyupgrade]

Claude here: Reverse-order release is the rule for locks you release explicitly (mutexes, LWLocks). These row locks, taken by disable()'s FOR UPDATE and by the ALTERs in enable(), are held until the transaction ends and all drop together at commit or rollback, so there is no release order to choose. What matters is acquisition order: when enable() runs in a later transaction than disable(), it takes the same row locks again, and if it took them in reverse, a concurrent disable() (ascending) and enable() (descending) could each hold one lock and wait for the other, which is a deadlock. One ascending order for every caller rules that out, so I'm keeping the loop as is.

[github-actions]

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

---

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

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

I found no blocking bugs. ddfe844 deletes the eight error-precedence assertions and their setup from test/sql/event_trigger_disable.sql, as the maintainer asked. The plan count is now 38 (7+4+3+4+1+1+1+3+4+3+5+2), and test/expected/event_trigger_disable.out starts with 1..38, so the two agree. The remaining comment says the argument-error tests can't observe validation order, which still holds.

Notes

1. 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". It is now 38 assertions, and the precedence tests are gone.
2. 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]

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

---

Code review: PR #45 (event_trigger__disable()/__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

1. 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.
2. 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