Reject self-tracked objects; add reusable update-disable mechanism - #41
jnasbyupgrade wants to merge 3 commits into
Conversation
… objects object__getsert() (via _object_v__for_update()) now refuses to track any object that is itself a member of the object_reference extension, closing a bootstrapping hazard where the extension's own update-time restructuring could trip its own rename-detection/repair machinery. Replace the update script's session_replication_role trick with a real, reusable mechanism: zzz_object_reference__fix_identity and zzz_object_reference_capture self-recognize (and skip) DDL from any extension's own install/update script via in_extension; zzz__object_reference_drop can't self-recognize that way, so internal_update__begin()/__end() explicitly disable/re-enable it (saving and restoring its actual prior state) for future update scripts to reuse. Closes Postgres-Extensions#40.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Claude finished @jnasbyupgrade's task in 2m 54s —— View job Claude's ReviewTodo list
Overall this is a clean, well-explained PR. The One design point worth a second look: Scope of the
|
| BEGIN | ||
| BEGIN | ||
| CREATE TEMP TABLE __object_reference__internal_update( | ||
| evtname name PRIMARY KEY |
There was a problem hiding this comment.
Abbreviations suck. They should never be used without explicit approval. Add that rule to ../ai/. s/evt/event_/
There was a problem hiding this comment.
Bleh, just realized where you got the names from. Use either CTAS or CREATE TABLE LIKE to make it clear we're just mirroring pg_event_trigger.
There was a problem hiding this comment.
Fixed — the temp table is now built via CREATE TEMP TABLE ... AS SELECT evtname, evtenabled FROM pg_catalog.pg_event_trigger WHERE false (plus a separate ADD PRIMARY KEY, since CTAS doesn't carry constraints), so the mirroring is visible in the code itself rather than a coincidental name match. Also proposed the general rule in Postgres-Extensions/ai#25.
| ); | ||
|
|
||
| /* | ||
| * New: internal update-time disable/enable mechanism, replacing the |
There was a problem hiding this comment.
After this release these functions shouldn't normally be needed though, correct? If so then we should change the name to be more specific (event_trigger__disable()) and update the comment.
There was a problem hiding this comment.
Good question — I'd guess it's plausible but not certain future updates need this again (whatever prompts a future need to drop/recreate _object_reference._object_v would hit the same issue), but agreed either way: the actual mechanism is a general disable-with-restore primitive for an event trigger, not something conceptually tied to "being mid-update" — the other two triggers never use it at all. Renamed to event_trigger__disable()/event_trigger__enable() and updated the comment to say so.
| ); | ||
|
|
||
| /* | ||
| * _etg_fix_identity/_etg_capture: gain a self-recognition guard so they skip |
There was a problem hiding this comment.
Needs to also explain why this is needed.
There was a problem hiding this comment.
Expanded: without the guard, _etg_capture would try to call _object_reference._object_v__for_update() (the FUNCTION) for any CREATE-tagged command in this very script if a capture happened to be active during an extension update — including a moment where that function has been dropped and not yet recreated, which would fail outright; _etg_fix_identity would otherwise run its blanket identity-recompute pass on every one of this script's DDL statements for no reason, since nothing it touches is (or, after this update's self-tracking guard, ever legitimately can be) one of this extension's own tracked rows.
| $args$ | ||
| , 'boolean LANGUAGE sql STABLE' | ||
| , $body$ | ||
| SELECT EXISTS( |
There was a problem hiding this comment.
Pretty sure we need a special case here for the object_reference schema itself (ext depends on it instead of the other way 'round)
There was a problem hiding this comment.
Confirmed and fixed — CREATE EXTENSION records the extension as depending on its declared schema (a plain 'n' DEPENDENCY_NORMAL row, extension -> schema; verified against ExecCreateExtension in extension.c), not the schema as an 'e' member of the extension the way everything else it creates is, so the original check never matched it. Added the special case plus a regression test (object__getsert('schema', 'object_reference') is now rejected too).
…hanism _is_own_object() missed the extension's own declared schema (object_reference): CREATE EXTENSION records the extension as depending on that schema (a plain 'n' dependency, extension -> schema), not the schema as an 'e' member of the extension the way every other object it creates is, so the original pg_depend check never matched it. Added the missing special case, plus a regression test proving the schema itself is now rejected too. Renamed internal_update__begin()/__end() to event_trigger__disable()/ event_trigger__enable(): the mechanism is a general disable-with-restore primitive, not something conceptually tied to "being mid-update" (only zzz__object_reference_drop, of the three event triggers, ever needs it). The backing temp table is now built via CTAS off pg_catalog.pg_event_trigger itself so the connection to that catalog's own evtname/evtenabled columns is visible in the code, not just a hand-typed column list that happened to reuse its names. Also expands the _etg_fix_identity/_etg_capture update-script comment to explain why recreating them is needed, not just what changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ALTER EVENT TRIGGER is visible database-wide the instant it runs, unlike the session_replication_role trick it replaces -- so the previous test, which called these against the real zzz_* triggers, raced with every other test file running concurrently in the same pg_regress batch: whichever one happened to run a DROP on a tracked object while zzz__object_reference_drop was mid-disable never got its tracking row cleaned up. Reproduced locally by running the suite's test files concurrently against a fresh database outside pg_regress's own scheduling, matching the exact CI failure. Rewired the test around two dummy event triggers it creates and owns itself, so exercising the mechanism never touches shared state any other test file depends on; the default target (zzz__object_reference_drop) is confirmed via pg_get_functiondef() instead of actually being invoked. Documented the database-wide-visibility tradeoff in both event_trigger.sql locations so a reader (or someone running ALTER EXTENSION UPDATE) knows not to expect session-local isolation from it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 12m 14s —— View job I'll analyze this and get back to you. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| , 'Restore event triggers disabled by event_trigger__disable() to their exact prior state.' | ||
| ); | ||
|
|
||
| SELECT _object_reference.event_trigger__disable(); |
There was a problem hiding this comment.
Self-recognition guard is installed too late in this script.
event_trigger__disable() here only disables zzz__object_reference_drop. The comment above it claims the other two event triggers (zzz_object_reference__fix_identity / zzz_object_reference_capture) "self-recognize and skip our own script's DDL instead" via their new in_extension guard — but that guard isn't actually installed until _etg_fix_identity and _etg_capture are recreated later in this same script (around the _object_reference._etg_fix_identity/_etg_capture create_function calls, well after the structural DROP VIEW/CREATE VIEW/CREATE FUNCTION section for _object_v/_object_v__for_update/_object_oid__add).
Between this line and that recreation, the old (0.1.0) unguarded bodies of those two triggers are still live. Previously session_replication_role = replica silenced all three triggers for this whole window; now only one is silenced.
Failure scenario: a session calls object_reference.capture__start() and then runs ALTER EXTENSION object_reference UPDATE in the same transaction (the very scenario the deleted comments anticipated). The CREATE VIEW _object_reference._object_v statement a few lines down fires the old _etg_capture, which calls _object_reference._object_v__for_update(...) (the FUNCTION) — but that function was just DROPped and hasn't been recreated yet, so the update aborts with "function does not exist". No test in test/sql/event_trigger_disable.sql or test/sql/base.sql exercises an update-in-progress capture, so this isn't caught by CI.
| * install/update script (ours included) rather than trying to | ||
| * capture it. | ||
| */ | ||
| AND NOT in_extension |
There was a problem hiding this comment.
in_extension excludes DDL from any extension, not just object_reference's own.
pg_event_trigger_ddl_commands().in_extension is true whenever the command runs as part of any extension's install/update script — it can't distinguish object_reference's own script from, say, CREATE EXTENSION postgis. This filter silently changes what capture__start()/object groups can capture: previously, objects created by installing any extension while a capture was active would be captured (ordinary tables/functions with CREATE-tagged DDL); after this change they never will be, for any extension, forever — not just during object_reference's own updates.
Failure scenario: SELECT object_reference.capture__start(g); CREATE EXTENSION hstore; SELECT object_reference.capture__stop(); — hstore's tables/functions/operators are silently no longer added to group g, with no error and no test coverage of this case.
| * script (ours included); pg_event_trigger_ddl_commands() marks this via | ||
| * in_extension, unlike pg_event_trigger_dropped_objects() (see _etg_drop). | ||
| */ | ||
| IF EXISTS(SELECT 1 FROM pg_catalog.pg_event_trigger_ddl_commands() WHERE in_extension) THEN |
There was a problem hiding this comment.
Same broad-in_extension issue as _etg_capture: this RETURNs (skipping the entire identity-repair scan across all tracked objects) if any row from pg_event_trigger_ddl_commands() for this event has in_extension = true — i.e. whenever the triggering DDL happened to run inside any extension's install/update script, not just object_reference's own.
Failure scenario: some unrelated extension's update script renames/moves an object that is unrelated to that extension but happens to already be tracked by object_reference (e.g. a table ALTERed as a side effect of another extension's migration) — the blanket early return means this event trigger invocation does no identity-repair work at all for that DDL batch, so a rename that should have updated _object_reference.object.object_names silently doesn't.
| */ | ||
| OR ( | ||
| _is_own_object.classid = 'pg_catalog.pg_namespace'::regclass | ||
| AND _is_own_object.objid = (SELECT extnamespace FROM pg_catalog.pg_extension WHERE extname = 'object_reference') |
There was a problem hiding this comment.
_is_own_object() handles the extension's declared schema as a special case (since CREATE EXTENSION links it via a plain dependency, not deptype = 'e'), but doesn't handle the extension's own pg_extension row itself (classid = 'pg_catalog.pg_extension'::regclass, objid = its own oid). That row isn't an 'e' member of itself, and isn't covered by the namespace special case either.
Failure scenario: if extension is a supported cat_tools.object_type for this system, object_reference.object__getsert('extension', 'object_reference') bypasses this guard entirely and successfully tracks the extension as an object of itself — contradicting the "refuse to track objects that are themselves members of this extension" intent this PR adds.
| END IF; | ||
|
|
||
| -- Refuse to track objects that are themselves members of this extension | ||
| IF _object_reference._is_own_object(c_classid, objid) THEN |
There was a problem hiding this comment.
Efficiency: _is_own_object() now runs on every call to _object_v__for_update() — i.e. on every object__getsert() for every tracked object, forever, not just during extension updates — re-resolving (SELECT oid FROM pg_catalog.pg_extension WHERE extname = 'object_reference') (and the analogous extnamespace lookup) by name on each invocation instead of once. Minor, but it's added catalog work on what's otherwise a fairly hot path; hoisting the extension oid lookup (e.g. into a WITH once per call, or caching it) would avoid repeating the same lookup twice within a single _is_own_object() call (once per branch of the OR).
| DECLARE | ||
| v_name name; | ||
| BEGIN | ||
| BEGIN |
There was a problem hiding this comment.
Reuse/simplification: this reentrancy guard (CREATE TEMP TABLE + catch duplicate_table to detect "a previous disable() is still in effect") is a new bookkeeping idiom. The codebase already has an established convention for exactly this kind of "session temp-table state, create lazily" problem — capture__start()'s own pg_temp.__object_reference__ddl_capture table, created lazily in an EXCEPTION WHEN undefined_table handler rather than pre-emptively. Two different idioms now exist for the same "is there already session state from a paired call" check, making the pattern harder to recognize/reuse next time.
| SELECT __object_reference.create_function( | ||
| '_object_reference.event_trigger__disable' | ||
| , $args$ | ||
| event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' |
There was a problem hiding this comment.
Altitude: this is framed as a "general-purpose event-trigger disable/enable mechanism" (per the comment above), but the default event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}' is its only actual caller anywhere in the codebase or tests — it's really a single hardcoded workaround for one trigger that can't self-recognize, wearing a general-purpose signature. That's fine as far as it goes, but nothing here has exercised or needs the "multiple/arbitrary trigger names" generality it advertises, so that generality is unproven and a future caller passing something other than the default is on its own.
1 participant
Two related fixes needed for #38's OID-repair design, but useful independent of it: object__getsert() (via _object_v__for_update()) now refuses to track any object that is a member of the object_reference extension itself, since letting the tracking system observe its own extension-member objects turns the extension's own DDL -- including its own update scripts restructuring itself -- into things the tracking/repair machinery would react to.
The update script's session_replication_role trick is replaced by a real, reusable mechanism. zzz_object_reference__fix_identity and zzz_object_reference_capture now self-recognize DDL from any extension's own install/update script via pg_event_trigger_ddl_commands()'s in_extension column and skip it; zzz__object_reference_drop can't self-recognize that way (pg_event_trigger_dropped_objects() has no equivalent column), so
_object_reference.internal_update__begin()/__end()explicitly disable and re-enable it -- saving and restoring its actual prior enabled state rather than assuming 'origin' -- for this and future update scripts to call.Details
_object_reference._is_own_object(classid, objid)checks extension membership viapg_depend(deptype = 'e',refobjid= object_reference's own extension oid); wired into_object_v__for_update()right after the existing temp-object rejection.internal_update__begin(event_trigger_names name[] DEFAULT '{zzz__object_reference_drop}')/internal_update__end()disable/re-enable the given event triggers, recording each one's priorevtenabledvalue in a temp table soend()restores it exactly.ALTER EVENT TRIGGERis ordinary transactional DDL, so a rolled-back update script undoes any DISABLE automatically -- no separate cleanup-on-error logic needed.sql/object_reference--0.1.0--stable.sqlcreates all three new functions early (0.1.0 has none of them) and callsinternal_update__begin()/__end()around its structural section instead of the oldsession_replication_rolesave/restore. It also now creates_object_reference.exec(), a permanent helper 0.1.0 never got thatobject__dependency__add()/object_group__dependency__add()(both unchanged since 0.1.0) already depended on, closing a pre-existing gap in the update path.test/sql/internal_update.sqlcovers the begin/end round-trip, restoring a non-default prior state, and the nested-call/no-matching-call guards.test/sql/base.sqlgains two rejection tests (tracking_object_reference.objectitself, and one of the event trigger functions).Verified with both
make testandmake test-update(0.1.0 → stable).Closes #40.