Skip to content

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

Open
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:wip/issue-40
Open

Reject self-tracked objects; add reusable update-disable mechanism#41
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:wip/issue-40

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

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

  • New _object_reference._is_own_object(classid, objid) checks extension membership via pg_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 prior evtenabled value in a temp table so end() restores it exactly. ALTER EVENT TRIGGER is 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.sql creates all three new functions early (0.1.0 has none of them) and calls internal_update__begin()/__end() around its structural section instead of the old session_replication_role save/restore. It also now creates _object_reference.exec(), a permanent helper 0.1.0 never got that object__dependency__add()/object_group__dependency__add() (both unchanged since 0.1.0) already depended on, closing a pre-existing gap in the update path.
  • New test/sql/internal_update.sql covers the begin/end round-trip, restoring a non-default prior state, and the nested-call/no-matching-call guards. test/sql/base.sql gains two rejection tests (tracking _object_reference.object itself, and one of the event trigger functions).

Verified with both make test and make test-update (0.1.0 → stable).

Closes #40.

… 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.
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

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

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e39ebb2-1d89-4768-afea-1d72c8f1bdb8

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

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

❤️ Share

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

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 2m 54s —— View job


Claude's Review

Todo list

  • Gather context (diff, CLAUDE.md, changed files)
  • Review _is_own_object / _object_v__for_update() change
  • Review internal_update__begin/__end() mechanism
  • Review update script (--0.1.0--stable.sql) changes
  • Review test coverage
  • Post final review

Overall this is a clean, well-explained PR. The internal_update__begin/__end() mechanism is a solid improvement over the old session_replication_role trick (transactional rollback safety via ALTER EVENT TRIGGER, actual-prior-state restoration instead of assuming origin, and explicit nested-call/no-call guards backed by tests). Test coverage for the new mechanism (test/sql/internal_update.sql) is thorough — round-trip, non-default prior state, nesting guard, missing-begin guard, unknown-trigger-name guard.

One design point worth a second look:

Scope of the in_extension self-recognition guard may be broader than intended

sql/object_reference.sql:1448 (_etg_capture, AND NOT in_extension) and sql/object_reference.sql:1474 (_etg_fix_identity, early RETURN when in_extension) skip DDL for any extension's install/update script, not just object_reference's own — pg_event_trigger_ddl_commands().in_extension is true whenever the reported command runs as part of any CREATE EXTENSION/ALTER EXTENSION UPDATE script, with no extname/self-check filter.

The PR description confirms this is deliberate ("self-recognize DDL from any extension's own install/update script ... and skip it"), and it's a reasonable read of the goal — but it's a materially different (and broader) scope than the table-level guard added in the same PR, _is_own_object() (sql/object_reference.sql:165-181, used at sql/object_reference.sql:911), which only refuses tracking of objects that are members of object_reference itself.

Net effect: while an object_reference capture group is active, CREATE EXTENSION some_other_extension (or updating it) will now silently stop having its objects captured — a behavior change for a core feature (object_reference.capture__get_current()/object_group__dependency__add()), not just a self-protection fix. test/sql/base.sql:92-95 creates test_factory via CREATE EXTENSION, but only asserts lives_ok, not capture behavior, so this broader scope isn't exercised by the new tests either way.

Worth confirming this breadth is intentional (vs. narrowing the guard to just object_reference's own script, mirroring _is_own_object()'s scope) and, if intentional, calling it out explicitly in a comment/CHANGELOG note since it's easy to read the current comments ("skip DDL issued by any extension's own install/update script (ours included)") as self-protection when it's actually a general opt-out for every extension.

Minor

  • sql/object_reference.sql:913 / sql/object_reference--0.1.0--stable.sql:398: the DETAIL message uses r_identity.identity, computed by the pre-existing temp-object check a few lines above rather than being recomputed for this new guard — correct, just noting it relies on that ordering (guard is placed right after the temp-object check, so r_identity is already populated as intended).

No blocking issues found; CI is green (CI run succeeded). The scope question above is a design confirmation, not something that necessarily needs to block merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standard internal update-time disable mechanism, and refuse to self-track extension-owned objects

1 participant