Design: automated OID-repair for pg_dump/pg_upgrade — consolidated plan + review findings

[jnasbyupgrade]

Summary

Design for automated OID-repair (follow-up to #24, #25). Fully resolved — no remaining top-level decision blocks implementation; see Next steps for the sub-issue breakdown (now tracked as GitHub sub-issues of this one). See #39 for a deliberately-deferred follow-up (caching the sanity-check status), and #43 for a deliberately out-of-scope follow-up (opt-in tracking of other extensions' objects).

(The design/review history — multiple rounds of adversarial review, empirical reproductions, and the discussion that led here — has been trimmed from this issue for clarity and kept locally; ask if any of it needs to come back.)

Background

Self-healing today (_object_reference._object_v__for_update()) only covers "_object_oid row missing" — not "row present but stale," which pg_upgrade produces unconditionally for tracked functions/triggers/constraints/casts/default-values (pg_upgrade doesn't preserve real catalog OIDs for those kinds, only for tables/indexes/sequences/views/columns/types). A stale-but-present row followed by an ALTER ... RENAME on that object can silently misattribute the tracked identity to a different real object, or fork it into a duplicate object_id — real, reproduced data corruption, not a hypothetical.

Three ways a stale-but-present row currently arises: a logical restore's DDL running before this extension's own event triggers are recreated (CREATE EVENT TRIGGER lands in pg_dump's POST_DATA/POST_ACL), a binary pg_upgrade (physical file copy, same underlying cause), or deliberate session_replication_role suppression. In normal operation, in between those events, the existing event triggers keep everything correct in real time.

Operational context worth keeping in mind throughout: pg_upgrade in practice almost never runs against a database still serving live application traffic — normal practice is to stop the application, upgrade, then resume. Every mechanism below must still be correct under the assumption that a client could connect and start issuing calls the instant the upgraded cluster accepts connections (that's the only safe assumption), but scenarios framed around "the database might still be serving traffic during the upgrade bracket" describe a safety margin the design must hold up under, not the expected common case. (Recorded as a general org convention in Postgres-Extensions/ai#17.)

Plan A: automatic detection + proactive repair

1. Marker table, deliberately NOT pg_extension_config_dump()-marked, holding the PostgreSQL major version active at install time, plus (see Plan B) the pg_upgrade bracket-open flag — one unified row, not two markers. Empty at check time → just restored from a logical dump (non-config tables are wiped/recreated fresh by any restore). Present but version-mismatched → a pg_upgrade just happened (ordinary tables survive it via physical file copy, untouched). Updated to the current major version after a successful repair.

2. A new ddl_command_start event trigger — fires before any DDL takes effect (unlike the existing ddl_command_end-based name-sync trigger), specifically so a rename can't act on stale data before repair happens. Checks the marker table; if dirty, does a full repair sweep of every tracked object, re-resolving each by name. Needs no suppression handling of its own (see item 6) — its own dirty/clean check already makes it inert during ordinary ALTER EXTENSION UPDATE execution. The sweep must have the same tolerant exception handling _sanity() already has for an object that doesn't resolve yet (can happen mid-restore, before every tracked object has been recreated) — this is sufficient regardless of restore/upgrade timing, since pg_upgrade's physical file-transfer step discards anything written during its schema-restore phase regardless of correctness, and _sentry_mv's unconditional _repair() call guarantees final correctness for logical restore regardless of what the sweep did or didn't finish mid-restore. Wrap any full sweep in pg_advisory_xact_lock() (transaction-scoped, not the session-scoped pg_advisory_lock()) so concurrent sessions serialize instead of racing.

3. A new public function exposing the same check/repair manually.

4. Every existing public API function gets the same check-and-repair call — covers sessions that never issue DDL. Implementation: one shared internal function holds the logic; every public function has a direct, visible call to it at its top (a hybrid of "single source of truth" and "mechanically testable" — see Testability). A CI/pgTAP test enumerating pg_proc/pg_namespace (pg_proc.prosrc holds the exact literal function body) against an explicit allowlist can check every public function has the call — as a fast lint only; the real gate is a behavioral test (dirty the state, call the function, assert the repair/warn/error side effect actually happened), since a naive text/regex match alone is defeatable (e.g. by a commented-out call).

5. On finding an inconsistency, severity is decided by what was actually found, not by any caller-selectable mode (see Reporting modes):

   • Expected — the marker definitively confirms a known, legitimate cause (a pg_upgrade major-version change, or a fresh/restored marker row) → repair silently, no WARNING.
   • Uncertain but handleable — the marker doesn't confirm a cause, but the finding itself is a shape the repair logic knows how to fix mechanically (e.g. _object_oid empty while _object_reference.object is non-empty) → repair, but WARNING.
   • Unexpected — a finding outside the repair logic's known-safe vocabulary (e.g. the object's current identity disagrees with the stored one, with no rename expected) → do not attempt to auto-recover; hard ERROR, with a clear explanation of what's wrong and, where possible, the exact command to correct it by hand.

   In every repair-performing case (the first two tiers), a factual NOTICE is always emitted summarizing how many objects were fixed out of how many total. If the transaction is read-only and something needs fixing, raise a proactive, clear custom error rather than surfacing Postgres's generic one, regardless of tier. This branching is fixed/automatic for the ddl_command_start trigger and the instrumented API functions — see Reporting modes for how manual invocation composes with it. Two specific findings, both described under Plan B, are deliberate, documented exceptions to the "expected → silent" rule and always get a WARNING even though their cause is fully confirmed — both exist specifically to serve Plan B's awareness-raising goal, not because the underlying repair is any less certain: a pg_dump taken while the pg_upgrade bracket was open, and (a related but distinct case) the bracket having stayed open longer than it should have.

   (Note: examples above illustrate the three tiers, not an exhaustive catalog of every scenario — mapping out the full set of "unexpected" cases and their recommended manual-recovery commands is implementation work for the relevant sub-issue, not a top-level design question.)

6. All event triggers (existing three, plus the new ddl_command_start one) become ENABLE ALWAYS, closing the gap where any unrelated session/process (e.g. a logical replication apply worker) setting session_replication_role = replica for its own reasons would silently suppress this extension's real-time tracking.

   Suppressing the triggers during object_reference's own internal operations (update scripts' structural sections, and pg_upgrade__pre()/pg_upgrade__post() — see Plan B) is a separate concern from ENABLE ALWAYS, and does not need a differentiated per-trigger approach. _etg_drop's sql_drop has no in_extension-equivalent self-recognition signal (pg_event_trigger_dropped_objects() doesn't expose one, and by the time it fires the dropped object's own extension-membership record is already gone from pg_depend), so it needs a literal ALTER EVENT TRIGGER ... DISABLE/ENABLE bracket regardless — and once that mechanism/privilege is needed for one trigger, extending it uniformly to all of them costs nothing further and removes an entire second mechanism (in_extension self-recognition) to reason about, test, and get subtly wrong. Resolved: all internal-update-type operations (ordinary extension update scripts, and pg_upgrade__pre()/pg_upgrade__post()) uniformly disable all event triggers at the start and re-enable at the end, via the standardized internal disable/enable mechanism Standard internal update-time disable mechanism, and refuse to self-track extension-owned objects #40 is building — not a one-off inline pattern, and not in_extension-based self-recognition for any trigger.

7. Independent repair-logic bugs, needed regardless of the above: _object_oid__add() needs INSERT ... ON CONFLICT (object_id) DO UPDATE (bare INSERT today); _object_oid's other unique constraint (classid, objid, objsubid) needs DEFERRABLE INITIALLY DEFERRED, since a bulk sweep can trip it when two different stale rows' real OIDs cross over post-upgrade; fix_refs()'s "extraneous ID information" branch references an undeclared variable (r_object should be r_object_v) and, separately, never actually repairs the present-but-stale case even in warning_only mode; _etg_fix_identity() needs a guard so it never trusts an already-stale OID to derive/overwrite a name.

8. _object_oid stays a separate, non-config-dumped table (merging its columns into _object_reference.object was evaluated and rejected — it would make the OID columns survive every logical restore in a stale state, hitting the _object_oid__add() duplicate-key crash on every restore instead of only after pg_upgrade; a separate table also lets concurrent writes to object and _object_oid for the same tracked object proceed without blocking each other). Add a code comment above its CREATE TABLE explaining it's deliberately not config-dumped and why. Additionally, mark it UNLOGGED — this is required, not optional: _object_oid gets swept into any CREATE PUBLICATION FOR ALL TABLES today, and a second database independently running object_reference subscribed to it would have the first database's OIDs (meaningless there) continuously, silently overwrite its own correct cache via ordinary replication, with nothing in this design ever able to detect it (it's DML via the apply worker, not DDL or an API call). UNLOGGED closes this: Postgres refuses UNLOGGED tables in any publication outright, and generates no WAL for them, closing physical replication too. The durability trade-off (truncated on crash) maps onto the "row missing" case this extension already self-heals correctly. To resolve the crash-vs-legitimate-emptiness ambiguity: also check whether _object_reference.object itself is empty — if so, nothing has ever been tracked and there's genuinely nothing to repair; if object is non-empty while _object_oid is empty, that's unambiguously a real anomaly needing repair, regardless of whether the restart was clean or a crash.

9. _sentry_mv's current form (a pg_extension_config_dump()-marked materialized view) is removed as a pg_upgrade mechanism — that's what crashes it — but see Plan B: it's kept, unmodified, for logical restore, and gains a second, deliberate role there.

Operational requirement, confirmed under physical (streaming) replication: event triggers never fire on a standby (confirmed empirically with a real primary+standby pair), so a standby can only ever hit the read-only/custom-error branch and never self-repair — a stale entry replicates identically to every standby (physical replication cannot produce OID divergence; a standby's state is always a byte-for-byte replay of the primary's WAL) and only a writable transaction on the primary ever fixes it. The manual repair function (item 3) should be a required step in any pg_upgrade/restore runbook, and the marker-table update plus the full repair sweep must be a single atomic transaction (otherwise a standby rebuilt via a fresh pg_basebackup taken between separate commits could inherit a false "clean" marker over an incompletely-repaired cache). See #42 for dedicated test coverage of this and related replication scenarios.

Other implementation gotchas to carry into the sub-issues: several target functions for "add the check everywhere" (object__getsert_w_group_id, object__identity, object__describe, object__cleanup, _object_v__for_update) are not SECURITY DEFINER today, and there are no INSERT/UPDATE grants on the tracking tables for object_reference__usage — bolting a repair-write onto these as-is fails with a permission error; the codebase has an existing, unaddressed -- TODO: Force search_path if options ~* 'definer' that shouldn't be expanded on without also closing it. (Confirmed empirically: nested SECURITY DEFINER calls are not restricted — a SECURITY DEFINER function calling another with a different owner works fine, each running as its own owner — so making the shared check-and-repair function itself SECURITY DEFINER is a safe, direct option, no non-SECDEF wrapper needed.) STABLE-marked functions (e.g. object_group__get()) can have repeat calls cached/skipped by the planner within one query, silently no-op'ing a repair rather than erroring — these need to become VOLATILE. capture__get_current() is already called from the existing _etg_capture trigger on every DDL statement — instrumenting it as a fifth "public function with the check" would double-fire the sweep. The marker row's creation should be an explicit, unconditional upsert in the install/update script (not lazily tied to iterating tracked objects, which would never populate it on a fresh install with zero tracked objects yet). The marker table itself needs some access-control story (nothing currently stops an unrelated process from resetting or spoofing it).

Plan B: bracket pg_upgrade with explicit pre/post steps — additive to Plan A, not an alternative

This is in addition to Plan A, specifically as a forcing function to make operators aware pg_upgrade needs special handling for this extension — not a lighter-weight substitute. Being additive does not reduce Plan A's scope, cost, or risk in any way.

_sentry_mv stays in place, unmodified, for logical restore (already correct there). For pg_upgrade specifically, two new functions (named per this codebase's noun__verb convention):

• object_reference.pg_upgrade__pre(): drops _sentry_mv (ALTER EXTENSION object_reference DROP MATERIALIZED VIEW _sentry_mv; DROP MATERIALIZED VIEW _sentry_mv;) so there's nothing for pg_upgrade's restore phase to crash on, and records an explicit "upgrade bracket deliberately opened" marker (a distinct boolean/timestamp field in the unified marker row — not just the current major version, which doesn't change yet).
• object_reference.pg_upgrade__post(): recreates _sentry_mv (CREATE MATERIALIZED VIEW _sentry_mv AS SELECT _repair(); ALTER EXTENSION object_reference ADD MATERIALIZED VIEW _sentry_mv; SELECT pg_catalog.pg_extension_config_dump('_sentry_mv', ''); — a fresh object gets a fresh OID, so config-dump marking must be reapplied) and clears the bracket marker. Recreating it both restores the guard and performs a repair as an inherent side effect of the CREATE. Must be safe to call redundantly (bracket already closed, nothing to do) — a clear NOTICE, not a cryptic duplicate-object error.

Today's crash (pg_class heap OID value not set when in binary upgrade mode) is the forcing function: an operator who skips pg_upgrade__pre() hits it immediately and loudly, with the upgrade cleanly aborted, not corrupted.

That crash also gets a clearer, dedicated error, not just a hard requirement to remember pg_upgrade__pre(). Verified empirically (a real PostgreSQL instance started in genuine binary-upgrade mode, matching pg_upgrade's own invocation): the crash happens inside heap_create_with_catalog(), strictly before _sentry_mv's own defining query is ever evaluated — so neither _sentry_mv nor _repair() can intercept it from within their own bodies. A ddl_command_start event trigger can fire early enough, though: current_query() (unlike pg_event_trigger_ddl_commands(), which isn't available until ddl_command_end) gives the raw SQL text, sufficient to recognize _sentry_mv's deterministic REFRESH MATERIALIZED VIEW statement; and calling any pg_catalog.binary_upgrade_set_next_*_oid() function is a reliable, engine-level, non-spoofable signal for "genuinely running under pg_upgrade's -b flag" — it raises SQLSTATE 55P02 when not in binary-upgrade mode and succeeds silently when it is (branch on the SQLSTATE, not the translated message text — the error message itself is localized). Add a dedicated ddl_command_start trigger (ENABLE ALWAYS, consistent with the others), scoped to REFRESH MATERIALIZED VIEW statements naming _sentry_mv, that probes this signal and raises a clear, object_reference-specific error with a HINT to call pg_upgrade__pre() first, instead of letting the generic crash happen. (Checked and ruled out separately: pg_upgrade --check has no extension point at all — its checks are a fixed, hardcoded sequence in pg_upgrade's own source, nothing a third-party extension can register into.) As cheap defense-in-depth, also document a pre-pg_upgrade runbook check (SELECT to_regclass('_object_reference._sentry_mv') IS NULL) an operator can run before ever invoking pg_upgrade, independent of any of the above.

pg_upgrade__pre()/pg_upgrade__post() are, conceptually, doing exactly what an extension update script's structural section does: mucking with object_reference's own member objects. They use the same standardized internal disable/enable mechanism #40 is building — not a bespoke mechanism of their own, and not the pg_event_trigger_dropped_objects()'s original-column self-recognition trick considered earlier (unnecessary once _etg_drop is simply disabled for the duration, the same as for any other internal-update-triggered drop). (Confirmed separately, and still relevant regardless of this: a DROP EXTENSION object_reference CASCADE — where _sentry_mv is dropped alongside everything else, not alone — is a non-issue, since PostgreSQL doesn't fire sql_drop when the trigger's own function is itself among the objects being dropped in that same cascade.)

A pg_dump taken while the bracket is open loses the general logical-restore safety net, not just the pg_upgrade-specific one. Confirmed from _sentry_mv's own definition (CREATE MATERIALIZED VIEW _object_reference._sentry_mv AS SELECT _object_reference._repair();) — creating this matview, by any path, evaluates _repair() immediately as a side effect of populating it. Under ordinary operation this happens twice: once, uselessly, when CREATE EXTENSION re-executes the install script's literal CREATE MATERIALIZED VIEW statement during schema restore (pg_dump's PRE-DATA phase, before other tracked objects have their final OIDs or _object_oid's own dumped rows have loaded), and once for real when pg_extension_config_dump()'s forced REFRESH runs in pg_dump's POST-DATA phase, after everything else is in place — it's this second, POST-DATA invocation that actually matters. If _sentry_mv doesn't exist at dump time (because pg_upgrade__pre() dropped it), there's nothing marked in pg_extension_config_dump() for pg_dump to schedule into POST-DATA — the restore only gets the useless PRE-DATA invocation, and the real, load-bearing repair silently never happens. A pg_dump taken between pg_upgrade__pre() and pg_upgrade__post() — for any reason, not necessarily connected to an actual pg_upgrade run — produces a dump that, once restored, has no forced repair at all.

This is automatically detectable, and automatically repairable, without depending on a pg_upgrade major-version change: under correct operation, the unified marker row's bracket-open flag and _sentry_mv's existence are always in lockstep (pg_upgrade__pre() sets bracket-open and drops the guard together; pg_upgrade__post() recreates the guard and clears bracket-open together) — so "bracket marked open, but the guard already exists" is a combination that can only arise from a restore of a dump taken mid-bracket (CREATE EXTENSION always recreates the guard from the install script, independent of what the dumped marker row says, since the marker's row content is data, not schema). Add this combination as its own always-checked signal, alongside the major-version check, in the same automatic entry points (the ddl_command_start trigger, the instrumented API functions): on detecting it, run the full repair sweep and clear the bracket-open flag, with a WARNING — the first of the two "confirmed cause, but still WARNING" exceptions noted in Plan A item 5, specifically because the situation itself (a pg_dump taken inside the bracket) is exactly what Plan B exists to make operators aware they should avoid.

Forgotten pg_upgrade__post() call: since Plan A's own version-marker-driven repair (item 5) fixes _object_oid regardless of whether pg_upgrade__post() ever gets called, a forgotten call is not a correctness risk to the currently-running database — the actual, ongoing risk is that the general dump/restore safety net (_sentry_mv) stays missing indefinitely, and that risk is exactly what the bracket-open-plus-guard-present detection above already catches automatically, the moment (if ever) someone actually dumps and restores this database while the bracket is still open. Given that, this design does not repeatedly WARNING in the logs purely because the bracket happens to be open (that's log noise the underlying risk doesn't justify, and doesn't serve Plan B's awareness-raising goal any better than the alternative below) and does not silently auto-recreate the guard on its own initiative (that would make pg_upgrade__post() purely decorative). Instead: make the open bracket visible on demand — the manual check/repair function (item 3) reports the bracket's state (open, and since when) as part of its normal summary output whenever it's run, so an operator checking on the database's health sees it without having to know to look; and pg_upgrade__post() itself must be safe and informative to call redundantly, as already noted above. The forcing/teaching moment for an operator who never revisits the primary at all is the automatic detection at restore time, described above — that is where a WARNING actually belongs, since that's the point where the mistake has concrete, immediate consequences worth flagging.

Simplification, agreed: unify the bracket-open marker into Plan A's existing major-version marker table (one table, a couple of columns) rather than tracking it separately — no conflict found (independent semantics: Plan A's automatic sweep only ever touches major_version, Plan B's detection only ever touches the bracket flag + guard-existence, so sharing a row doesn't create a collision), and both need the same underlying properties (ordinary durable table, not config-dumped). The UNLOGGED-crash "startup time" signal originally proposed as a third piece of state turns out to be unnecessary — the _object_reference.object-emptiness check above (item 8) is already fully sufficient on its own to resolve the ambiguity, so there's nothing left to unify there.

Reporting modes

No caller-selectable severity mode is exposed anywhere — every finding's severity comes from the 3-tier classification in Plan A item 5 (what was actually found), never from an argument the caller picks. The only variable across entry points is a single internal signal: whether the shared check-and-repair logic is being invoked automatically (the ddl_command_start trigger, the instrumented API functions) or as part of a deliberate, explicit cleanup the operator asked for (the Plan A item 3 manual function, and pg_upgrade__post()). In the deliberate-invocation case, the operator already knows and expects to be cleaning up — so the uncertain-but-handleable tier's WARNING is suppressed down to the same factual summary NOTICE the expected tier already gets; the unexpected tier is never suppressed, in either case — an anomaly the repair logic doesn't know how to safely fix on its own is always a hard ERROR, regardless of who or what triggered the check.

Next steps

Before any implementation starts: #24 and #42 (both now GitHub sub-issues of this one) must stay fully current through implementation, not just accurate as of when they were opened or last reviewed.

Per the repo owner's implementation guidance: this should land as multiple separate, narrowly-scoped PRs/sub-issues, each focused on one feature plus its own full test coverage, not one large PR. Candidate sub-issues:

• The underlying repair-logic bug fixes and the object row's INSERT ... ON CONFLICT modernization (object_reference cannot survive binary pg_upgrade: _sentry_mv crashes it, and repair logic can't fix stale OIDs anyway #24).
• The _object_oid UNLOGGED change, the fresh-install/object-emptiness check, and the code comment explaining why it's not config-dumped.
• The marker table (unified, single record) + ddl_command_start trigger (with tolerant exception handling) + instrumented API functions (Plan A core), including the pg_advisory_xact_lock()-guarded sweep.
• pg_upgrade__pre()/pg_upgrade__post(), the bracket-open-plus-guard-present detection, and the forgotten-call visibility/idempotency handling (Plan B).
• The shared check-and-repair internal function, its explicit-vs-automatic invocation signal, and its behavioral test.
• sql/object_reference--0.1.0--stable.sql's structural section switched from session_replication_role suppression to the standardized uniform event-trigger disable/enable mechanism (Standard internal update-time disable mechanism, and refuse to self-track extension-owned objects #40).
• Streaming/physical replication test coverage (Dedicated test coverage: streaming/physical replication (promotion, pg_upgrade-of-a-standby, permanently-empty _object_oid) #42).