Skip to content

CI: Document current OID-repair/self-healing behavior - #37

Open
jnasbyupgrade wants to merge 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:docs/oid-repair-current-behavior
Open

CI: Document current OID-repair/self-healing behavior#37
jnasbyupgrade wants to merge 2 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:docs/oid-repair-current-behavior

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

Ground-truth reference for how object_reference's object-identity tracking and OID-repair/self-healing logic behaves today on master: the core model (object/_object_oid tables, _object_v/_object_v__for_update views, _sanity()), every write/repair path in full (object__getsert, _object_oid__add, fix_refs, post_restore, _repair/_sentry_mv, the three event triggers), exhaustive config-dump marking, and three end-to-end scenarios traced with real reproductions in scratch databases: logical pg_dump/pg_restore, a binary pg_upgrade (PG12→PG17), and a stale _object_oid row followed by ALTER ... RENAME.

Also independently confirms, with reproduction, the three known-bug claims referenced in #24: _object_oid__add()'s missing ON CONFLICT, fix_refs()'s r_object/r_object_v typo, and _etg_fix_identity()'s lack of any staleness guard.

Documentation only, describing current behavior — no design proposals. The design question of how/whether to auto-detect and repair after a pg_upgrade remains tracked separately in #24/#25.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

Ground-truth reference for how object_reference's object-identity
tracking and OID-repair/self-healing logic behaves today on master,
covering the core model, every write/repair path, config-dump
marking, three end-to-end scenarios (dump/restore, binary pg_upgrade,
stale-row + rename), and independent confirmation of three known bug
claims (_object_oid__add's missing ON CONFLICT, fix_refs's r_object
typo, _etg_fix_identity's lack of a staleness guard) — all backed by
direct code citations and real reproductions in scratch databases.

Documentation only, describing current behavior; no design proposals
(tracked separately in issues Postgres-Extensions#24/Postgres-Extensions#25).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 5m 38s —— View job


Claude finished @jnasbyupgrade's task

Task list

  • Gather context (PR diff, CLAUDE.md, repo structure)
  • Read sql/object_reference.sql (full source, 1610 lines) and the new doc/oid-repair-current-behavior.md (2803 lines)
  • Cross-check every doc section against source (core model, _sanity/views, _object_v__for_update, object__getsert*, fix_refs, post_restore, _repair/_sentry_mv, all three event triggers, config-dump marking, the three end-to-end scenarios)
  • Verify the three confirmed-bug claims against source
  • Post review feedback

CI is green (31650695158).

Summary

This is a documentation-only PR (single new file, no code changes), and it holds up very well under scrutiny. I verified every "verbatim" SQL quote in the document character-by-character against sql/object_reference.sql, and they all match exactly. The three headline bug claims are all real and correctly diagnosed:

  • fix_refs()'s r_object/r_object_v typo — confirmed at sql/object_reference.sql:386 and :393: RAISE WARNING 'extraneous ID information for object_id %', r_object.object_id references an undeclared variable (only r_object_v is declared), so this branch always fails with a plpgsql "record ... is not assigned yet" / undefined-column error instead of producing its intended diagnostic.
  • _object_oid__add()/_repair() missing ON CONFLICT — confirmed: the INSERT INTO _object_reference._object_oid(...) at sql/object_reference.sql:296-297 has no conflict clause, so re-running against an already-populated row raises a duplicate-key error. Correctly not idempotent.
  • _etg_fix_identity()'s lack of a staleness guard — confirmed: the UPDATE ... FROM _object_reference._object_oid oo at sql/object_reference.sql:1452-1465 blindly trusts the stored (classid, objid, objsubid) and re-derives names from whatever it currently resolves to (or throws an uncaught error if it resolves to nothing), on every ddl_command_end regardless of relevance to the triggering statement.

Also confirmed: the dangling "zzz_object_reference_end" trigger name referenced in the HINT at line 952 doesn't correspond to any of the three actual event triggers (zzz__object_reference_drop, zzz_object_reference__fix_identity, zzz_object_reference_capture) — the doc calls this out correctly as a stale/leftover reference rather than a real missing trigger.

Minor nits (citation-only, don't affect correctness of conclusions)

  • _etg_drop() is cited as starting at line 1471; the create_function(...) call actually starts at 1470 (1471 is just the function-name argument).
  • A "WHEN NOT r_object_v.ids_ok" branch is mentioned in the _object_oid__add() call-site discussion — no such arm exists in _object_v__for_update's CASE (the real arms are WHEN r_object_v.ids_ok / WHEN NOT r_object_v.ids_exist / WHEN r_object_v.ids_exist). The cited line range (926-940) does point at the correct (real) branch, so this is a wording slip, not a wrong pointer.
  • The object__getsert signature citation near the end gives 1168-1174 for a quote that includes loose boolean DEFAULT false, which is actually on line 1175 (off by one).
  • The "Files/paths referenced" summary list near the end truncates a couple of line ranges by a few lines relative to the earlier, more careful citations in the body (e.g. giving 1571-1585 for the event-trigger registrations, when the third one runs through 1588).
  • One dangling forward-reference ("see the full 53-value enum list captured below") near the end of the document doesn't point at anything in the remaining ~45 lines — likely means "above" instead, referring to content earlier in the doc.

None of these affect the document's substantive conclusions — they're citation-precision slips in an otherwise very carefully sourced and empirically-verified document. No changes are required before merging; the nits above are optional polish.

@coderabbitai

coderabbitai Bot commented Aug 12, 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: a836d8d7-e034-47f1-bec3-e7777eddeca6

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.

Corrects five line-number/wording slips flagged by CI review: the
_etg_drop create_function start line, a fictitious WHEN NOT
r_object_v.ids_ok CASE arm, the object__getsert signature range, the
event-trigger registration range, and a "below" that should say
"above". None affect the document's substantive conclusions.
@jnasbyupgrade

Copy link
Copy Markdown
Contributor Author

Claude here — fixed all five citation nits from the review above in dabe793:

  • _etg_drop()'s create_function call now cited as starting at line 1470 (was 1471).
  • Removed the fictitious WHEN NOT r_object_v.ids_ok CASE arm from the branch description — only WHEN NOT r_object_v.ids_exist is real for that call site.
  • object__getsert() signature range corrected to 1168-1175 (was 1168-1174), to include the loose boolean DEFAULT false line.
  • Event-trigger registration range in the "Files/paths referenced" summary corrected to 1571-1588 (was 1571-1585), covering all three CREATE EVENT TRIGGER statements.
  • The dangling "captured below" forward-reference to the 53-value enum list corrected to "captured above" (the list appears earlier in the document).

All verified directly against sql/object_reference.sql before fixing, per the review's own note that these are citation-precision slips only — no substantive conclusions changed.

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

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


Claude finished reviewing this PR

Task list

  • Gather context (PR diff, CLAUDE.md, repo structure)
  • Read the full diff (doc/oid-repair-current-behavior.md)
  • Cross-check documented SQL/behavior claims against sql/object_reference.sql
  • Post review feedback

Summary

This PR is unchanged in substance since the last review — the only new commit (dabe793) fixes the five citation nits flagged previously. I re-verified each fix directly against sql/object_reference.sql:

Nit Fix Verified
_etg_drop() cited as starting at 1471 Changed to 1470 create_function( call for _etg_drop is at line 1470; '_object_reference._etg_drop' (the name arg) is at 1471
Non-existent WHEN NOT r_object_v.ids_ok branch mentioned Removed, left only the real WHEN NOT r_object_v.ids_exist ✅ confirmed the CASE at lines 923/926/942 has no ids_ok-negated arm
object__getsert signature cited as 1168-1174 Changed to 1168-1175 loose boolean DEFAULT false is on line 1175
"Files/paths referenced" summary gave 1571-1585 for event trigger registrations Changed to 1571-1588 ✅ the third CREATE EVENT TRIGGER ... zzz_object_reference_capture block runs through line 1588
Dangling "captured below" forward-reference Changed to "captured above" ✅ matches actual document structure

All five corrections are accurate and no new issues were introduced by this commit — it's a pure precision fix with no changes to the document's substantive content or conclusions.

As covered in the prior review round, the document's core claims (the fix_refs() r_object/r_object_v typo at lines 386/393, the missing ON CONFLICT in _object_oid__add()/_repair(), and _etg_fix_identity()'s lack of a staleness guard) all still check out against current sql/object_reference.sql, and every verbatim SQL quote I spot-checked matches exactly.

No further changes needed — this is ready to merge as far as documentation accuracy goes.

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.

1 participant