[jnasbyupgrade]

extension_drop's event trigger now fires for DROP SCHEMA and DROP OWNED as well as DROP EXTENSION, so an extension dropped by DROP SCHEMA ... CASCADE or DROP OWNED BY runs its registered SQL and has its registration removed instead of leaving a stale row in extension_drop__commands (which makes every later DROP EXTENSION raise XD001).

These three are the only commands that can drop an extension. An extension has ordinary pg_depend rows only on its schema and on any extension it requires, so it goes away via DROP EXTENSION (including cascading to dependents), DROP SCHEMA ... CASCADE, or DROP OWNED, which walks the owner dependencies in shdepend. This doesn't depend on the PostgreSQL version. Corroboration on PG12 and PG17 with an unfiltered sql_drop trigger: only those three tags dropped an extension, and DROP ROLE, DROP TYPE/DROP FUNCTION ... CASCADE on a member, and DROP SCHEMA without CASCADE all error out, while REASSIGN OWNED drops nothing.

Since the trigger now runs for every DROP SCHEMA and DROP OWNED, extension_drop__event_trigger() returns immediately unless an extension is among the dropped objects. Unrelated drops therefore don't read extension_drop__commands or run the sanity assert, so they work for non-superusers and are unaffected by a stale row.

Behavior change: dropping an unregistered extension (by any of the three commands) needs only USAGE on the extension's schema, to resolve a new SECURITY DEFINER function, extension_drop__registered(name[]), executable by PUBLIC, which the trigger asks which of the dropped extensions are registered; it needs no privileges on extension_drop__commands. Dropping a registered extension additionally needs SELECT and DELETE on that table. A role without the access gets SQLSTATE 42501 with the primary message permission denied for table "<schema>"."extension_drop__commands" (owned by role "<owner>"), which extension_drop needs to drop extension(s) <name>, the underlying error as DETAIL, and a HINT giving the grants that fix it, instead of permission denied for table extension_drop__commands (or type "extension_drop__commands" does not exist without USAGE on the schema). The registered SQL runs as the caller and may need further privileges.

Mechanism: after the early return, the trigger calls extension_drop__registered(), which returns the registered subset of the dropped extensions, runs the sanity check (XD001) when it is empty, and writes nothing; its search_path is the extension's schema followed by pg_temp, so a caller's temp table can't shadow the real one. If the result is empty the trigger returns. Otherwise (or if the helper can't be resolved) it deletes the registrations in a single DELETE ... USING pg_event_trigger_dropped_objects() ... RETURNING statement as the caller, wrapped in an exception block for insufficient_privilege and undefined_table (a caller without USAGE on the schema gets "does not exist", since name lookup skips a schema it can't use). The registered SQL runs afterward, outside the block, so its own privilege errors surface unchanged. The handler looks up the table's owner and schema through pg_extension and pg_class for the HINT, since name lookup can't be used there either. The trigger function stays SECURITY INVOKER, since SECURITY DEFINER would run registered SQL as the owner; only the read-only helper is a definer.

ALTER EVENT TRIGGER can't change WHEN, so extension_drop--0.1.1--stable.sql drops and recreates the trigger and replaces the function. Function properties (proacl, proconfig, prosecdef, source hash) and pg_event_trigger.evttags are identical after a fresh install and after updating from 0.1.1, on PG12 and PG17.

Tests: drop_schema_cascade and drop_owned fail without the wider WHEN list. unrelated_drops guards the widening: it fails if the trigger fires for these commands without the early return. drop_denied covers the error (code, primary message, DETAIL, HINT) for a registered extension under DROP SCHEMA ... CASCADE with and without USAGE on the schema, DROP OWNED BY, and DROP EXTENSION by a demoted owner; SELECT without DELETE and the HINT's grants (directly and through a group role) letting the drop succeed; a privilege error from registered SQL surfacing unchanged; and, for an unregistered extension, drops with no grants, with USAGE only and with SELECT only, plus the no-USAGE error. It also covers a temp table named extension_drop__commands not hiding the registration from the helper, a stale registration still raising XD001 on an unregistered drop, and the helper's EXECUTE revoked from PUBLIC (the trigger falls through to the caller's DELETE and raises the same error). Mutations that make drop_denied fail: neutering the handler's exception clause; removing undefined_table from it; removing insufficient_privilege from the helper-resolution trap (the revoked-EXECUTE test); pg_temp first in the helper's search_path (the temp-table test); dropping the helper's sanity assert (the stale-registration tests).

[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: a470ac79-3937-410b-af83-8bc01a5204ca

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

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

[github-actions]

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

---

Review of PR #55: fire the event trigger for DROP SCHEMA and DROP OWNED

I read the diff and the trigger and sanity-check code. I did not build the extension or run the test suite. The code-review skill failed to run, so this is a manual review.

The change is small and consistent. The trigger WHEN list, the 0.1.1→stable update script, the README and HISTORY entries, and the new tests all agree with each other.

Concern: the trigger now fires for ordinary users (not verified by running it)

DROP EXTENSION is normally run by superusers. DROP SCHEMA and DROP OWNED BY are routine for non-superusers on their own objects. The trigger function is not SECURITY DEFINER. It reads extension_drop__commands and calls extension_drop__sanity_assert(). The install script creates these with '' grants, which revokes all access (sql/extension_drop.sql:113 and :255).

Event triggers run with the privileges of the user who issued the command. So a non-superuser running DROP SCHEMA myschema or DROP OWNED BY me may now fail with permission denied for function extension_drop__event_trigger. That would break DDL that has nothing to do with extensions.

There is a second, related effect. Any stale row in extension_drop__commands now makes every DROP SCHEMA and DROP OWNED raise XD001, not only DROP EXTENSION. The blast radius of a bad state is larger.

Suggestions:

• Add a test that runs DROP SCHEMA and DROP OWNED BY as a non-superuser and confirms they still succeed. If they fail, make the trigger function SECURITY DEFINER with a pinned search_path (it already sets search_path FROM CURRENT).
• Alternatively, skip the work when pg_event_trigger_dropped_objects() contains no object_type = 'extension' row. Do this before sanity_assert, so unrelated drops never hit the assertion. The PR says the trigger function is unchanged, so this would be a deliberate change from that.

Minor

• The tests hide the drop cascades to notice with client_min_messages = WARNING in drop_schema_cascade.sql. drop_owned.sql has no such notice, so this is fine.
• drop_owned.sql creates a SUPERUSER role. That is fine only because the test transaction is left open and rolled back, so make sure that stays true.
• I found no bugs in the update script. Dropping and recreating the trigger is the right approach, since ALTER EVENT TRIGGER can't change WHEN.

[jnasbyupgrade]

Claude here. Addressing the review's non-superuser concern (confirmed by running it: a plain role's DROP SCHEMA and DROP OWNED BY failed with a permission error, and a stale row made unrelated drops raise XD001): extension_drop__event_trigger() now returns immediately unless pg_event_trigger_dropped_objects() contains an extension, before touching extension_drop__commands or the sanity assert. It also uses a record variable so it compiles for a role with no access to the extension's schema. New test unrelated_drops covers a non-superuser DROP SCHEMA ... CASCADE and DROP OWNED BY, and a superuser DROP SCHEMA while a stale row exists; it fails on the previous commit and passes now. SECURITY DEFINER was not used. The SUPERUSER role in drop_owned.sql is created inside the test transaction, which is rolled back.

[github-actions]

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

---

Review of PR #55: fire the event trigger for DROP SCHEMA and DROP OWNED

I read the diff, including the trigger SQL, the update script and test/sql/unrelated_drops.sql. I did not build the extension or run the tests.

I found no bugs. The earlier review's two concerns are both handled in the current head:

• Non-superuser drops. The trigger function now returns early unless pg_event_trigger_dropped_objects() contains an extension row (sql/extension_drop.sql:270). Ordinary DROP SCHEMA and DROP OWNED BY calls therefore never read extension_drop__commands, which has no grants. unrelated_drops.sql:36-55 tests both commands as a non-superuser.
• Stale rows. Because of the early return, a stale registration no longer makes unrelated drops raise XD001. unrelated_drops.sql:57-69 covers this by dropping a schema with a stale row present.

The fresh-install and 0.1.1→stable paths are consistent:

• Both create the same function body and the same WHEN tag IN ('DROP EXTENSION', 'DROP SCHEMA', 'DROP OWNED').
• The update script drops and recreates the trigger, which is needed because ALTER EVENT TRIGGER can't change WHEN.
• The README and HISTORY text match the behavior.

Minor notes (no action required)

• The PR description says "The trigger function is unchanged", but the diff adds the early return and changes r from a table-row type to record. The description is out of date, and the code is fine.
• unrelated_drops.sql leaves the role extension_drop_test_plain and the GRANT CREATE ON DATABASE in place. This is fine only if test/finish.sql rolls back the transaction, so keep it that way. I didn't check that.
• drop_owned.sql creates a SUPERUSER role. This has the same rollback caveat.

[jnasbyupgrade]

Claude here. Decisions needed from the maintainer:

1. Document known limits that predate this PR? A stale row or skipped cleanup can still occur when the event trigger is disabled (ALTER EVENT TRIGGER ... DISABLE), under session_replication_role = replica, and when the extension or schema holding extension_drop itself is dropped (other extensions' registered cleanup is silently skipped).
2. Warn about existing stale rows on update? ALTER EXTENSION ... UPDATE does not repair rows left by earlier DROP SCHEMA / DROP OWNED; HISTORY.asc and the XD001 hint point to extension_drop__repair(). Options: leave as is, or RAISE WARNING in the update script when extension_drop__sanity_check() is non-empty. Auto-repair is not recommended: it could discard cleanup SQL that never ran.
3. Structure for the follow-up PR. A follow-up (non-superuser DROP EXTENSION fix using a SECURITY DEFINER helper) depends on this PR. Per ai/PR.md, a dependent PR needs a decision on structure: (a) push when-tag and the follow-up branch to this repo and open a real stack, (b) open the follow-up as a draft against master with this PR's commits in its diff, or (c) wait for this PR to merge.

[github-actions]

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

---

Review of PR #55: fire the event trigger for DROP SCHEMA and DROP OWNED

I read the full diff: the install and update SQL, README/HISTORY, and the three new tests plus the helper. I did not build the extension or run the tests. The code-review skill failed to run, so this is a manual review.

I found no bugs.

• Install and update paths match. Both use the same function body and the same WHEN tag IN ('DROP EXTENSION', 'DROP SCHEMA', 'DROP OWNED'). The update script drops and recreates the trigger, which is needed because ALTER EVENT TRIGGER can't change WHEN.
• Early return. The function returns before it reads extension_drop__commands or runs the sanity assert, unless pg_event_trigger_dropped_objects() contains an extension row. Non-superuser and stale-row cases are covered by unrelated_drops.sql.
• record variable. It keeps the function from needing the table type before the early return.
• Docs. extension_drop__repair() in HISTORY exists (sql/extension_drop.sql:159). The README and HISTORY text match the behavior.

Minor notes (no action required)

• The PR description is accurate now. The earlier "function unchanged" wording is gone.
• The tests create a SUPERUSER role and a plain role, and they grant CREATE ON DATABASE. These are safe only if the test transaction is rolled back, so I'd keep it that way. I didn't check test/finish.sql.
• DROP OWNED BY for a role that owns other objects still relies on the registered SQL running while those objects exist. That is the same ordering as DROP EXTENSION, so it is fine.

[github-actions]

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

---

Review of PR #55: fire the event trigger for DROP SCHEMA and DROP OWNED

I read the diff of the install SQL, the 0.1.1→stable update script, and unrelated_drops.sql with the test helper. I did not build the extension or run the tests. The code-review skill failed to run, so this is a manual review.

I found no bugs.

• Install and update paths agree. Both define the same function body and the same WHEN tag IN ('DROP EXTENSION', 'DROP SCHEMA', 'DROP OWNED'). The update script drops and recreates the trigger, which it has to do because ALTER EVENT TRIGGER can't change WHEN.
• Early return. The function exits before it reads extension_drop__commands or runs the sanity assert, unless an extension row is among the dropped objects. Because r is now a record, the function body doesn't need the table type, so it compiles for a role with no access to that table.
• Tests. unrelated_drops.sql covers a non-superuser DROP SCHEMA ... CASCADE and DROP OWNED BY. It also covers a superuser DROP SCHEMA while a stale row exists. The test helper checks the table is empty and that extension_drop__sanity_check() returns nothing.

Minor notes (no action required)

• The tests create a plain role, a SUPERUSER role, and a GRANT CREATE ON DATABASE. They leave these behind, so they are safe only if the test transaction rolls back. I didn't check test/finish.sql.
• On the open decisions in the earlier comment:
  1. I'd document the known limits: a disabled event trigger, session_replication_role = replica, and dropping the schema or extension that holds extension_drop itself.
  2. A RAISE WARNING on update when extension_drop__sanity_check() is non-empty is cheap and doesn't discard cleanup SQL. I'd skip auto-repair, as you suggested.
  3. For structure, (c) waiting for this PR to merge avoids duplicated diffs. Choose (a) if the follow-up needs to proceed in parallel.

[jnasbyupgrade]

Need to actually explain why that matters.

[jnasbyupgrade]

Claude here. Fixed in 0cae837: the comment now says why. A typed variable is resolved when plpgsql compiles the function on first call, before the early return, so a role that cannot see the extension's schema fails every unrelated DROP with type "extension_drop__commands" does not exist. Reverting to the typed variable makes the unrelated_drops non-superuser DROP SCHEMA CASCADE and DROP OWNED BY tests fail; with record they pass. Same wording applied to sql/extension_drop--0.1.1--stable.sql.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check. I only looked at the current head of sql/extension_drop.sql. I did not build the extension or run the tests.

No new bugs found.

jnasbyupgrade's review comment said the r record note needed to explain why it matters. The head already has that explanation (sql/extension_drop.sql:268-271). The comment says a typed variable is resolved when the function compiles, before the early return. A role without access to the extension's schema would then fail every unrelated DROP. That is the reason the comment needed to give, so I made no further change.

The early return, the record variable and the FOR r IN SELECT c.* loop are consistent with each other.

[jnasbyupgrade]

Lets consolidate these some... we definitely don't need a +1 for each line. Apply to the rest of the new test files as well.

[jnasbyupgrade]

Claude here. Consolidated in 823be14: unrelated_drops.sql plan() is now 4 lines (was 8), and drop_owned.sql and drop_schema_cascade.sql are 5 lines each, grouped by purpose. The totals in those two are unchanged.

[jnasbyupgrade]

Claude here. 50bf9be consolidates the plan() terms in drop_owned, drop_schema_cascade and unrelated_drops into one labelled line per group, and each helper's count now has its own line labelled with the helper's path; totals are unchanged.

[jnasbyupgrade]

Uh... can't you just do this when creating the role?

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head (823be14). I read sql/extension_drop.sql and test/sql/unrelated_drops.sql. I did not build the extension or run the tests. I did not read the other new test files, so I can't confirm the plan consolidation was applied to them.

I found no bugs.

• Early return. The function returns before it touches extension_drop__commands or runs the sanity assert, unless an extension row is among the dropped objects. The r record comment now explains why a typed variable would break unrelated drops.
• unrelated_drops.sql after the latest review comments. The plan is consolidated into three groups. The schemas are created AUTHORIZATION extension_drop_test_plain by the superuser, so the database CREATE grant is gone. This addresses both comments on that file.

Minor notes (no action required)

• If the DO block in the non-superuser tests fails midway, it leaves the role without resetting it. The test already fails in that case, so this doesn't matter.
• The tests create a role and a schema without cleaning them up, so they are safe only if test/finish.sql rolls back the transaction. I didn't check that.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head (50bf9be). I read sql/extension_drop.sql and test/sql/unrelated_drops.sql. I did not build the extension or run the tests. origin/master shares no merge base with this checkout, so I could not diff against it. I did not read the other test files.

I found no bugs.

• Early return. The function returns before it touches extension_drop__commands or runs the sanity assert, unless an extension row is among the dropped objects. The r record block comment explains why a typed variable would break unrelated drops.
• unrelated_drops.sql plan count. The plan is 3 (non-superuser drops) + 3 (stale registration) + 1 (test/finish.sql) = 7. That matches the seven assertions in the file.

Minor notes (no action required)

• The tests create a role and schemas without cleaning them up. They are safe only if test/finish.sql rolls back the transaction. I didn't check that.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head. I read the install and update SQL and test/sql/drop_owned.sql with its helper. I did not build the extension or run the tests.

I found no bugs.

• Install and update paths match. Both define the same function body and the same WHEN tag IN ('DROP EXTENSION', 'DROP SCHEMA', 'DROP OWNED'). The update script drops and recreates the trigger, which it must do because ALTER EVENT TRIGGER can't change WHEN.
• Early return. The function returns before it reads extension_drop__commands or runs the sanity assert, unless an extension row is among the dropped objects. The r record comment explains why a typed variable would break unrelated drops.
• drop_owned.sql plan. The terms add up to 1 + 4 + 3 + 4 + 1 = 13.

Minor notes (no action required)

• The tests create roles, including a SUPERUSER role, and don't drop them. They are safe only if test/finish.sql rolls back the transaction. I didn't check that.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head. I read the test/sql/drop_denied.sql header, plan and first half, and the test_ext__verify_dropped.sql helper. I did not build the extension or run the tests. I did not re-read the other files.

I found no bugs.

• drop_denied.sql plan. The terms add up to 20 (2+1+3+2+2+1+2+4+2+1). The first-half assertions I read match the labelled groups, so I found no mismatch there. I did not read the second half.
• DETAIL/HINT checks. The test captures the stacked diagnostics in GUCs and compares them against the expected text built from extension_drop_schema, so it doesn't hard-code the random schema name.
• Helper. test_ext__verify_dropped.sql checks the extension is gone, the registration table is empty and extension_drop__sanity_check() returns nothing.

Minor notes (no action required)

• The tests create roles, including one that was a superuser, and don't drop them. They are safe only if test/finish.sql rolls back the transaction. I didn't check that.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head (fc01223). I read the trigger function in sql/extension_drop.sql and all of test/sql/drop_denied.sql. I compared the plan count against test/expected/drop_denied.out. I did not build the extension or run the tests.

I found no bugs.

• Early return. The function returns before it touches extension_drop__commands or runs the sanity assert, unless an extension row is among the dropped objects. The record comment explains why a typed variable would break unrelated drops.
• Exception block. The block wraps only the DELETE ... RETURNING statement, so errors from the registered SQL surface unchanged. The HINT lookup goes through pg_extension and pg_class by OID rather than by name. The bare RAISE when that lookup finds nothing re-raises the original error.
• drop_denied.sql plan. The terms add up to 34 (2+1+3+2+2+2+1+2+1+2+2+3+1+3+4+2+1). The expected output has 34 ok lines, so the plan and the assertions agree.
• Second half of drop_denied.sql. The cases are SELECT without DELETE, a privilege error from registered SQL passing through, the HINT's grants letting the drop succeed, a group role holding the grants, and a demoted owner. Each one tests what its label says.

Minor notes (no action required)

• The tests create roles, including one that was a superuser, and don't drop them. They are safe only if the test transaction rolls back. The expected output ends with TRANSACTION INTENTIONALLY LEFT OPEN!, which suggests it does, but I didn't read test/finish.sql.

[github-actions]

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

---

Review of PR #55

The code-review skill failed to run, so this is a manual check of the current head (d5906b7). I read the trigger function and the new extension_drop__registered() helper in sql/extension_drop.sql. I did not build the extension or run the tests. I did not re-read the update script or the test files for this commit.

I found no bugs in the code I read.

• Early return. The function still returns before it touches extension_drop__commands, unless an extension row is among the dropped objects.
• New helper. extension_drop__registered() is SECURITY DEFINER and read-only. It pins its path with pg_temp last, so a caller's temp table can't shadow the real table. It never runs registered SQL, so the registered SQL still runs as the caller.
• Error handling around it. The trigger catches undefined_function and insufficient_privilege, so a caller without USAGE on the schema falls through to the DELETE, which raises the actionable 42501 error. An empty result returns early, so unregistered extensions drop for callers without table access.

Notes

• PR description is out of date. It says the function "stays SECURITY INVOKER" and that every drop needs table access "whether or not the extension is registered". Since d5906b7 an unregistered extension needs no table access, and the new SECURITY DEFINER helper is not mentioned. Please update the description, and the README and HISTORY if they say the same.
• Helper is granted to public. Any role can call extension_drop__registered(names) and learn which extensions are registered. That is probably harmless, but the README could say so.
• Sanity assert from the helper. When nothing is registered, the helper runs extension_drop__sanity_assert(). A stale row therefore still makes an unrelated extension drop raise XD001. This is the same as before for DROP EXTENSION, and it's intended, but it is worth a line in the docs.

[github-actions]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.