Add fresh/update/existing-mode extension test infrastructure

[jnasbyupgrade]

Adds the local test/Makefile machinery for fresh/update/existing-mode testing: TEST_LOAD_SOURCE=fresh|update|existing plus TEST_UPDATE_FROM/TEST_UPDATE_TO make vars, a single committed installer covering all three modes, a hand-authored 0.1.0 → stable update script, and one permanent pgTAP assertion proving the extension's own SQL never relies on an ambient search_path. CI wiring that actually drives update/existing mode in an automated job is a separate, later PR — CI here just keeps running the normal fresh-mode suite.

• test/install/load.sql: single, committed-once installer covering all three load modes — a drop-first reset (including a pg_temp.drop_role() helper for the extension's own global roles) for fresh/update, and a presence+version assertion for existing.
• sql/object_reference--0.1.0--stable.sql: hand-authored update script. Every function/view it recreates goes through the same private-helper-schema bootstrap/teardown convention the fresh install itself uses, rather than hand-written DROP/CREATE/REVOKE/GRANT/COMMENT, so the update path can't silently diverge from what a fresh install actually produces. The view drop order matters: _object_v/_object_v__for_update are dropped before the _object_oid table's column changes, since pg_regress won't allow dropping a column a view still selects. The extension's own event triggers (already active from the 0.1.0 base) are suppressed for the structural portion of the script via session_replication_role = replica — zzz__object_reference_drop's body queries _object_reference._object_v, which would error the instant the script's own DROP VIEW ran against it otherwise — and the caller's actual prior value (not an assumed default) is restored afterward, since ALTER EXTENSION UPDATE isn't guaranteed to be the only statement in its transaction.
• test/finish.sql: the one permanent assertion that object_reference/_object_reference are absent from the resolved search_path, checked at the end of every test file — proof that nothing in the suite accidentally relies on an ambient search path to resolve the extension's own objects.
• test/build/build.sql (moved from test/sql/, and dropped its historical zzz_ ordering prefix): the raw-source-load sanity check now runs under pgxntool's test-build feature, in its own separate database — it collides with the real extension now installed via test/install/load.sql in the shared main-suite database otherwise.
• Dependency-guard anchor: _object_reference.object's own row type, referenced from a guard view. Confirmed to block a non-CASCADE DROP EXTENSION (with CASCADE correctly dropping both) on a fresh install, and confirmed to survive the update path unchanged and still block a non-CASCADE drop afterward.

Verified locally (CI doesn't yet exercise update/existing mode, per above): make test TEST_LOAD_SOURCE=update and make test TEST_LOAD_SOURCE=existing against both a correctly-updated and a deliberately-stale database pass/fail as expected, and a structural comparison of every recreated function/view (bodies via pg_get_functiondef, table/view columns, comments, ACLs) between the update path and a fresh install is byte-for-byte identical.

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

[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

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4a75529c-a5e9-48a4-9e50-46787786d438

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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

---

Claude's Review of PR #18

Tasks

• Gather context (diff vs origin/reconcile-object-functions, read changed files)
• Review Makefile / CI wiring changes
• Review test/install/load.sql (three-mode loader)
• Review sql/object_reference--0.1.0--stable.sql (update script)
• Review test/finish.sql and per-test-file plan bumps
• Review moved test/build/zzz_build.sql
• Post final review

---

Reviewed this diff against origin/reconcile-object-functions as instructed (not master). CI is green (both PG matrix legs passing).

Summary

This is a careful, well-scoped PR. I cross-checked the highest-risk piece — sql/object_reference--0.1.0--stable.sql — line-by-line against both sql/object_reference--0.1.0.sql (to verify every dropped constraint/index/trigger/function signature actually matches what 0.1.0 created) and sql/object_reference.sql (to verify every recreated function/view is byte-for-byte identical to the fresh-install source). Specifically verified:

• All 7 reg*_classid CHECK constraints, the null_count trigger, and the 6 _object_oid__u_reg* indexes dropped in the script exist under those exact names in sql/object_reference--0.1.0.sql (correctly no regnamespace index, since 0.1.0 never created one).
• The backfill (UPDATE ... SET object_oid = objid WHERE object_oid IS NULL) is correct given 0.1.0's objid_must_match invariant (objid always mirrored whichever single reg* column was set).
• _object_oid__add/_object_v__for_update's DROP FUNCTION signatures match 0.1.0's actual pre-update parameter types (classid regclass, not yet oid).
• Every function/view body recreated in the update script (_object_v/_object_v__for_update views, _object_oid__add, unsupported, the new object__describe/object__identity/object__cleanup, the group-cleanup trigger, _tg_capture_safety, both etg_raise__* functions) is textually identical to the current source in sql/object_reference.sql — confirms the "byte-for-byte structural match" claim in the PR description. The one cosmetic difference found (a trailing space after SELECT in the fresh-install view definitions vs. none in the update script) is immaterial — Postgres doesn't store view source text, only the parsed query tree.
• The _object_oid__add a.subobjid → a.objsubid bug fix described in the PR body is real: 0.1.0's script does reference the nonexistent a.subobjid column in a branch that (per the PR) was never actually hit, and the update script's recreated version correctly uses a.objsubid.
• View-before-column-drop and function-before-view drop ordering is correct (functions RETURNS _object_reference._object_v — a formal pg_depend edge — so they must go before the views, which must go before the table alterations that remove the columns they SELECT).
• Event-trigger disable/enable bracketing is correctly scoped around the exact structural section that could otherwise trip zzz__object_reference_drop's self-query.

test/install/load.sql's three-mode \if structure is sound (mode validated via a placeholder GUC read without missing_ok, so a broken make → PGOPTIONS → env → psql chain fails loudly rather than silently defaulting). The Makefile's TEST_LOAD_SOURCE parse-time validation, the conditional install: count_nulls prerequisite (correctly gated to update-mode only, not folded into the unconditional cat_tools dependency), and the test/install/.gitignore self-comparing-output setup all match the description and cat_tools' precedent.

test/finish.sql's assertion and the plan-count bumps line up exactly across all 6 test files (_object_v.sql 1→2, all.sql 73→74, base.sql 10→11, capture.sql 69→70, event_trigger.sql 42→43, object_group.sql 29→30) and the committed .out files reflect those counts correctly. Note all.out/capture.out show object_identity_temp_test_schema still on search_path at end-of-file — that's pre-existing test behavior (unrelated to this PR's diff) and doesn't trip the new assertion since it only checks for object_reference/_object_reference specifically, which is exactly the scope test-fixes.md's item 9 called for.

The moved test/build/zzz_build.sql (now under pgxntool's test-build feature, separate throwaway DB) is an unmodified pure rename, correctly resolving the collision with the now-persistently-installed extension in the main suite DB. The added apt-get install -y rsync CI step (needed for test-build's run-test-build.sh) is confirmed working — CI is green on both PG versions.

Findings

None. No correctness, security, or maintainability issues found in this diff. The scope is honestly and precisely bounded to what the PR description claims (local test/Makefile machinery only, CI wiring for update/existing modes explicitly deferred to the next stack link), and the "Not fully verified" section accurately reflects what wasn't covered (binary pg_upgrade).

[github-actions]

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

---

Claude's Review of PR #18 (re-run after synchronize)

Tasks

• Gather context (diff vs origin/reconcile-object-functions, identify new commits since last review)
• Review the 3 new commits (f0795fd, 777c777, 204b288)
• Cross-check against fresh-install source (sql/object_reference.sql) and 0.1.0 source
• Check CI status
• Post final review

---

Reviewed against origin/reconcile-object-functions. This synchronize only added three commits on top of the version already reviewed in full (see the earlier review above for the base-level audit of sql/object_reference--0.1.0--stable.sql, test/install/load.sql, test/finish.sql, etc.) — I focused this pass on what changed since then:

• f0795fd — Removes the redundant capture__start(object_group_id int) re-declaration from the update script. Verified: the function body in sql/object_reference--0.1.0.sql and the current sql/object_reference.sql are byte-for-byte identical (diffed both directly), so the CREATE OR REPLACE really was a no-op and dropping it from the update script is correct — nothing is lost.
• 777c777 — Regenerates test/expected/object_group.out's header from 1..29 to 1..30 and appends the new ok 30 - object_reference schema(s) must not be part of the resolved search_path line. Matches test/sql/object_group.sql's plan(30) and the test/finish.sql assertion added earlier in the stack.
• 204b288 — Drops _object_oid.object_oid from the update script (mirroring reconcile-object-functions's fresh-install change) instead of backfilling it, and switches SET client_min_messages to SET LOCAL. Checked both:
  • The backfill UPDATE ... SET object_oid = objid, the objid_must_match CHECK, and the object_oid passthrough in both view definitions and the _object_oid__add insert are all correctly removed in lockstep — grepped the full update script afterward and every remaining object_oid reference is either the table name _object_oid itself or an unrelated comment; no dangling references to the dropped column.
  • SET LOCAL client_min_messages = WARNING with the added comment is a verbatim adaptation of sql/object_reference.sql's own comment at the top of the fresh-install script (same reasoning: implicit-transaction rollback of the setting, just swapping "CREATE EXTENSION" for "ALTER EXTENSION UPDATE").

No issues found in this delta. CI is green on both PostgreSQL matrix legs for the current head commit.

[github-actions]

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

---

Claude's Review of PR #18 (re-run after synchronize)

Tasks

• Gather context (diff vs origin/reconcile-object-functions, identify new commit since last review)
• Review the 1 new commit (b01fa75)
• Check CI status
• Post final review

---

Reviewed against origin/reconcile-object-functions. This synchronize added exactly one commit on top of the version already fully reviewed above (see the two earlier reviews in this thread for the base-level audit of sql/object_reference--0.1.0--stable.sql, test/install/load.sql, test/finish.sql, etc.) — this pass covers only that delta:

• b01fa75 — Purely cosmetic: converts a multi-line comment in sql/object_reference--0.1.0--stable.sql (the SET LOCAL explanation) from consecutive -- lines into a /* ... */ block comment, matching the identical CODE_STYLE.md fix already applied to sql/object_reference.sql on the base branch (origin/reconcile-object-functions at 3c06f4d, same commit message/pattern). Diffed it directly — no wording changed, only the comment delimiter style. No functional impact.

CI is green on both PostgreSQL matrix legs for the current head commit. No new issues found.

Summary

No outstanding findings across the full stack of reviews on this PR. The diff remains a careful, well-scoped implementation of advanced-extension-testing.md checklist items 1-6, correctly deferring CI wiring for update/existing modes to the next stack link as stated in the PR description.

[github-actions]

Claude finished @jnasbyupgrade's task in 3m 19s —— View job

---

Claude's Code Review of PR #18

Ran the /code-review workflow against origin/reconcile-object-functions (per this repo's convention for this stacked PR, not master).

Tasks

• Check PR isn't closed/draft/trivial
• Identify relevant CLAUDE.md files (root, pgxntool/, .claude-pr/ — none impose rules relevant to this diff's SQL/test-infra content beyond what earlier passes in this thread already confirmed)
• Summarize the diff vs origin/reconcile-object-functions
• Deep dive on the one new commit since the last review in this thread
• Cross-check for other missed divergences of the same class
• Check CI status
• Post findings

Scope of this pass

Three prior reviews already exist in this thread covering everything through commit b01fa75 (cosmetic comment-style fix). The only change since then is one new commit:

• 4a37b00 — "Re-add capture__start(object_group_id) to the update script." An earlier commit in this same PR (7b060ce/f0795fd) had removed this function's re-declaration from sql/object_reference--0.1.0--stable.sql, on the assumption that 0.1.0's body and current source were byte-identical. This commit found that assumption was wrong and reverts course: 0.1.0's body still contains a dead, commented-out CREATE TEMP TABLE ... AS block inside the EXCEPTION handler that current source dropped. pg_get_functiondef() returns comments verbatim, so a comment-only difference is still a real structural divergence for an update-vs-fresh install comparison — exactly the class of bug bin/test_existing's structural diff exists to catch.

Verified by hand:

• 0.1.0's source (sql/object_reference--0.1.0.sql:1256-1261) does contain the dead comment block; current source (sql/object_reference.sql) does not — confirms the commit message's premise.
• The recreated body in sql/object_reference--0.1.0--stable.sql:660-706 is byte-for-byte identical to current source's capture__start(int) definition.
• Placement is correct: after the event triggers are recreated but before they're re-enabled (ALTER EVENT TRIGGER ... ENABLE at lines 710-712), consistent with the rest of the structural section's disable/enable bracket.
• To check for other instances of this same bug class, diffed sql/object_reference--0.1.0.sql against sql/object_reference.sql in full (not just the one function) and cross-referenced every structural delta against what the update script recreates: the _object_oid reg*-column removal, _object_oid__add's insert rewrite, unsupported()'s partitioned-table/index addition, the new _object_group__object__cleanup_trigger/trigger, the new object__describe/object__identity/object__cleanup functions, object__getsert's temp-schema rejection, _tg_capture_safety's RETURN NULL, and both etg_raise__* functions are all present and correctly recreated in the update script. No other comment-only or otherwise missed divergence found.
• SELECT __object_reference.safe_dump('_object_reference.object_object_id_seq') (new in current source, for the sequence) is also present in the update script.
• CI is green on both PostgreSQL matrix legs for the current head commit (4a37b00).

Findings

None. This commit closes a real gap the PR's own tooling was built to catch, is scoped correctly, and the full 0.1.0-vs-current diff cross-check turned up no other missed cases of the same bug class.
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/reconcile-object-functions.

Tasks

• Gather context (diff vs origin/reconcile-object-functions, identify new commit since last review)
• Review the 1 new commit (82818b4)
• Cross-check against the analogous change on origin/reconcile-object-functions (fad854d)
• Check CI status
• Post final review

Scope of this pass

Four prior reviews in this thread already cover everything through commit 4a37b00/abcbbb1 (re-add capture__start). This synchronize added exactly one new commit:

• 82818b4 — "Remove client_min_messages suppression from the update script." Deletes the SET LOCAL client_min_messages = WARNING (plus its explanatory comment) that a prior commit in this PR had added near the top of sql/object_reference--0.1.0--stable.sql.

The commit message claims this mirrors reconcile-object-functions' own identical change to sql/object_reference.sql (commit fad854d, "Move client_min_messages suppression from the extension script to the test"). Verified this by hand:

• sql/object_reference.sql on origin/reconcile-object-functions indeed no longer sets client_min_messages anywhere — confirms the fresh-install script was fully stripped of it, not just moved elsewhere within the same file.
• Unlike the fresh-install case, though, fad854d relocated the suppression into test/build/zzz_build.sql (the one raw \i-load test path that actually surfaces the %TYPE resolution NOTICEs). 82818b4 doesn't add an equivalent relocation anywhere for the update script — at first glance this looked like it could leave a gap for TEST_LOAD_SOURCE=update runs.
• Checked test/install/load.sql (unchanged by this commit) and found the gap doesn't actually exist: lines 180-186 already independently bracket the ALTER EXTENSION object_reference UPDATE ... call with SET client_min_messages = ERROR / SET client_min_messages = WARNING at the caller level, pre-existing from an earlier commit in this same PR ("matching cat_tools' own install/load.sql", per its comment). So NOTICE suppression for the update path was always coming from the test harness around the ALTER EXTENSION UPDATE call, not from the script itself — the in-script SET LOCAL this commit removes was genuinely redundant with that outer bracket, exactly as the commit message claims ("verified no test depends on the quieter output").
• Diffed the removal itself: clean 7-line deletion (the SET LOCAL statement plus its 4-line explanatory comment block and a blank line), no orphaned comment fragments or dangling references in the surrounding file header.
• This also correctly generalizes the "a shipped script shouldn't unilaterally change a session setting on its caller's behalf" principle from fad854d to the update script: real end users running ALTER EXTENSION object_reference UPDATE outside of this test suite entirely will no longer have their client_min_messages silently overridden either.

CI is green on both PostgreSQL matrix legs for the current head commit (82818b4).

Findings

None. This commit is a correct, well-verified generalization of the same session-setting-ownership principle already applied to the fresh-install script on the base branch, and the apparent gap (no relocated suppression for the update path) turns out to already be covered by pre-existing code in test/install/load.sql.
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Tasks

• Gather context (diff vs base, identify new commit(s) since last review)
• Review the new commit(s)
• Check CI status
• Post final review

Scope of this pass

This synchronize is a rebase, not new content. PR #17 (reconcile-object-functions) merged into master as c04df7d, so this branch's base moved from origin/reconcile-object-functions to origin/master — that's why GitHub reports new commit SHAs (e7b9f69…27d1c18) even though every commit message matches one already reviewed in this thread.

Verified this is a pure rebase, not a content change:

• git diff origin/master...HEAD --stat shows exactly 21 files changed, 1072 insertions / 14 deletions — matching this PR's stated totals precisely, and matching the cumulative diff already reviewed across the five prior passes in this thread.
• Spot-checked sql/object_reference--0.1.0--stable.sql (the highest-risk file, previously reviewed line-by-line in the first pass and re-verified after each subsequent commit) — header comment and content are unchanged from what was already audited.
• The five prior reviews in this thread already cover, in full: the update script (sql/object_reference--0.1.0--stable.sql), test/install/load.sql's three-mode loader, test/finish.sql and the per-file plan bumps, the test/build/zzz_build.sql move, the capture__start re-add/removal history, and the client_min_messages suppression removal. No commit exists on this branch that wasn't already reviewed.

CI is green: the CI workflow run for the current head completed successfully.

Findings

None. No new content to review — this synchronize only reflects master absorbing PR #17, which this stacked branch was already built on top of.
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/master.

Tasks

• Gather context (diff vs origin/master, identify new commit since last review)
• Review the new commit (501ad88)
• Check CI status
• Post final review

Scope of this pass

Six prior reviews in this thread already cover everything through commit 27d1c18 (removing the redundant client_min_messages suppression, and the earlier rebase onto master after PR #17 merged). This synchronize added exactly one new commit:

• 501ad88 — "test/finish.sql: don't report the actual search_path value." Drops the format('... -- got %s', current_schemas(false)) interpolation from the test/finish.sql assertion added earlier in this stack, leaving a static description string.

Verified by hand:

• test/finish.sql:16-19 now reads a plain ok(..., 'object_reference schema(s) must not be part of the resolved search_path') — no more format()/%s, matching the commit message.
• The rationale holds up: pgTAP's own failure diagnostics (# Failed test ...) already show the actual vs. expected values on failure, so echoing current_schemas(false) in the description was redundant, and dropping it removes a source of test fragility (.out files no longer bake in the exact resolved search_path, which could otherwise vary — e.g. all.out/capture.out previously hardcoded {object_identity_temp_test_schema,test_support,tap,public} — across environments or future test additions to those files).
• All 6 affected .out files (_object_v.out, all.out, base.out, capture.out, event_trigger.out, object_group.out) were regenerated correctly: each just drops the trailing -- got {...} suffix from its final ok N - ... line, with plan counts (1..2, 1..74, 1..11, 1..70, 1..43, 1..30) and every other line unchanged. Diffed each one directly — no stray changes.
• No other files touched by this commit.

CI is green on the current head commit (501ad88) — CI run.

Findings

None. This is a small, well-justified robustness fix (removing environment-dependent output from a committed .out file) with correctly regenerated expected output across all affected test files.
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/master.

Tasks

• Gather context (diff vs origin/master, identify new commit since last review)
• Review the new commit (090e5c1)
• Check CI status
• Post final review

Scope of this pass

Seven prior reviews in this thread already cover everything through commit 501ad88 (dropping the interpolated search_path value from test/finish.sql's assertion). This synchronize added exactly one new commit:

• 090e5c1 — "test/build: rename zzz_build to build." Renames test/build/zzz_build.sql → test/build/build.sql and test/build/expected/zzz_build.out → test/build/expected/build.out, plus updates the one comment in .github/workflows/ci.yml that referenced the old filename.

Verified by hand:

• The commit message's premise — that the zzz_ prefix only existed to force alphabetical-last ordering, and that pgxntool's test-build feature makes that irrelevant — holds up against pgxntool/base.mk:109-110: TEST_BUILD_SQL_FILES = $(wildcard $(TESTDIR)/build/*.sql) globs the whole test/build/ directory with no name-based ordering dependency, so the rename is purely cosmetic.
• pgxntool/README.asc:96 documents the canonical example filename as test/build/build.sql — this rename brings the file in line with pgxntool's own documented convention rather than diverging from it.
• Confirmed via grep that no other file (Makefile, CI workflow, pgxntool scripts) references the old zzz_build name anywhere after this commit — the only textual reference was the one comment line in ci.yml that this commit also updated.
• Both renames are pure git mv-equivalents (100% similarity, 0 content changes per git show --stat), and the .out/.sql pairing is preserved.

CI is green: the CI workflow run for the current head commit (090e5c1) completed successfully.

Findings

None. This is a pure, well-justified rename with no functional or behavioral change, correctly aligned with pgxntool's own naming convention for this feature.
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/master.

Tasks

• Gather context (diff vs origin/master, identify new commit since last review)
• Review the new commit (dbd3150)
• Check CI status
• Post final review

Scope of this pass

Eight prior reviews in this thread already cover everything through commit 090e5c1 (renaming zzz_build.sql to build.sql). This synchronize added exactly one new commit:

• dbd3150 — "test/finish.sql: use set_hasnt() instead of a hand-rolled ok()." Replaces the single ok(NOT (... OR ...)) assertion in test/finish.sql with two separate pgTAP set_hasnt() calls, one per schema (object_reference, _object_reference).

Verified by hand:

• test/finish.sql:16-26 — each set_hasnt($$ SELECT unnest(current_schemas(false)) $$, $$ VALUES ('<schema>') $$, '<description>') correctly asserts the resolved-search_path set contains none of the given single-element set, matching set_hasnt's documented semantics (fails if the first set has any element from the second). Splitting into two calls means a failure now names the specific leaked schema instead of a generic combined failure, exactly as the commit message claims.
• All 6 affected test/sql/*.sql files bump their plan() count by exactly 1 (one new assertion each), and the comment label was updated from -- schema-qualification (search_path) value +1 to +2 consistently across _object_v.sql, all.sql, base.sql, capture.sql, event_trigger.sql, object_group.sql.
• All 6 corresponding .out files were regenerated correctly: plan header bumped (e.g. 1..2→1..3, 1..74→1..75), the old combined ok N - object_reference schema(s) must not be part of the resolved search_path line is now split into ok N - object_reference schema must not be part of the resolved search_path followed by ok N+1 - _object_reference schema must not be part of the resolved search_path. Diffed each file directly — no stray or mismatched lines.
• The doc comment above the assertions was also lightly reworded ("own schema(s)" → "own schemas") for consistency with the now-plural two-assertion structure; no semantic change.

CI is green: the CI workflow run for the current head commit (dbd3150) completed successfully (run).

Findings

None. This is a small, well-executed test-quality improvement (better failure diagnostics via pgTAP's dedicated set-comparison function) with correctly regenerated plan counts and expected output across all six affected test files.
· branch: u-and-u-testing

[github-actions]

Claude finished @jnasbyupgrade's task in 4m 10s —— View job

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/master.

Tasks

• Gather context (diff vs origin/master, identify new commit since last review)
• Review the new commit (1612605)
• Verify the session_replication_role claim against Postgres event-trigger semantics
• Check CI status
• Post final review

Scope of this pass

Nine prior reviews in this thread already cover everything through commit dbd3150 (splitting the search_path assertion via set_hasnt()). This synchronize added exactly one new commit:

• 1612605 — "trim header, use session_replication_role." Two changes to sql/object_reference--0.1.0--stable.sql: (1) cuts the file's header comment down from a full change-history recap to a one-line pointer at the __object_reference helper-schema convention (purely cosmetic, no concern), and (2) replaces the three explicit ALTER EVENT TRIGGER ... DISABLE / ... ENABLE statements bracketing the structural section with a single SET LOCAL session_replication_role = replica; and no matching re-enable, relying on transaction end to revert it.

The technical premise checks out: event triggers do respect session_replication_role exactly like row/statement triggers (evtenabled supports the same O/D/R/A states, default O = fires unless role is replica), so this correctly suppresses all three of this extension's origin-enabled event triggers for the DDL between line 121 and end-of-file.

Finding

session_replication_role = replica has a much larger blast radius than the three event triggers it's meant to silence, and — unlike the three explicit ALTER EVENT TRIGGER statements it replaces — its scope depends on the caller's transaction, not just this script.

SET LOCAL reverts "at the end of the current transaction" (per Postgres docs), not at the end of this script. The old code disabled exactly the three named event triggers, unconditionally, and explicitly re-enabled them a few statements later — safe regardless of how the caller invoked ALTER EXTENSION ... UPDATE. The new code instead flips the session to replica mode for every table and trigger in the database (including foreign-key/RI check triggers and any other extension's event triggers), and that only "reverts automatically" if the ALTER EXTENSION UPDATE statement is the last thing in its transaction — true for test/install/load.sql's autocommit invocation (sql/object_reference--0.1.0--stable.sql:121), but not guaranteed for a real caller.

Concretely: a DBA running BEGIN; ALTER EXTENSION object_reference UPDATE; ALTER EXTENSION other_ext UPDATE; ...more DML...; COMMIT; (a normal pattern for coordinating several changes atomically) would have other_ext's event triggers, and every ordinary trigger on every table touched by "...more DML...", silently not fire for the rest of that transaction — with no error, no NOTICE, nothing to indicate why.

This directly contradicts the principle this same PR just established in commit 82818b4 ("Remove client_min_messages suppression from the update script"), whose own rationale was: "a shipped script shouldn't unilaterally change a session setting on its caller's behalf... real end users running ALTER EXTENSION object_reference UPDATE outside of this test suite entirely will no longer have their client_min_messages silently overridden either." session_replication_role is a session setting with far more consequential side effects than client_min_messages, and this commit reintroduces exactly the pattern the earlier one removed — just with a bigger footprint (all triggers/event-triggers session-wide, not just this extension's NOTICE verbosity).

The old three-statement DISABLE/ENABLE bracket didn't have this problem, since it targeted only the three event triggers by name and unconditionally re-enabled them within the same script — its safety didn't depend on the caller's transaction shape at all. Suggest reverting to that form here, even though it's more verbose, since the update script is meant to be safely embeddable in an arbitrary caller transaction the same way ALTER EXTENSION UPDATE normally is.

Fix this →

CI is green: the CI workflow run for the current head commit (1612605) completed successfully (run).
· branch: u-and-u-testing

[github-actions]

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

---

Claude's Code Review of PR #18 (re-run after synchronize)

Reviewed against origin/master.

Tasks

• Gather context (diff vs origin/master, identify new commit since last review)
• Review the new commit (93a1e9f), including the outstanding session_replication_role finding from the previous review
• Check CI status
• Post final review

Scope of this pass

Ten prior reviews in this thread already cover everything through commit 1612605, the last of which raised a finding: SET LOCAL session_replication_role = replica in sql/object_reference--0.1.0--stable.sql reverts at the end of the caller's transaction, not the end of the script, and unconditionally assumed the prior value was 'origin' rather than restoring whatever the caller actually had set. This synchronize added exactly one new commit that addresses that finding directly:

• 93a1e9f — "save/restore session_replication_role explicitly." Before disabling, stashes the actual current value of session_replication_role into a placeholder GUC (object_reference.saved_session_replication_role) via set_config(..., true); after the structural section and its __object_reference helper-schema teardown are both done, explicitly restores session_replication_role to that saved value (also via set_config(..., true)) rather than leaving it to the transaction to revert.

Verified by hand:

• sql/object_reference--0.1.0--stable.sql:121 now reads SELECT set_config('object_reference.saved_session_replication_role', current_setting('session_replication_role'), true); immediately before the existing SET LOCAL session_replication_role = replica; — captures the caller's real prior value (not an assumed 'origin') before disabling.
• The restore at the end of the file (SELECT set_config('session_replication_role', current_setting('object_reference.saved_session_replication_role'), true);) is placed after the __object_reference helper schema's DROP SCHEMA, i.e. strictly after every event-trigger-referenced object is back in its final shape — correct placement relative to the structural section it brackets.
• Both set_config calls use is_local = true, consistent with the existing precedent in this same repo for placeholder/session GUCs (sql/object_reference--0.1.0.sql:11's search_path save/restore, and test/install/load.sql's object_reference.test_load_mode/test_update_from/test_update_to placeholder GUCs) — this correctly scopes both the disable and the explicit restore to the current transaction, so behavior is unaffected whether ALTER EXTENSION UPDATE is the only statement in its transaction or one of several.
• This fully resolves the concern from the prior review: the script no longer assumes a value to revert to, and no longer depends on the caller's transaction ending immediately after ALTER EXTENSION UPDATE to restore normal trigger/event-trigger behavior — a caller running further DML or other extensions' DDL later in the same transaction will see its own actual prior session_replication_role restored well before that point.
• The updated header comment above the save statement accurately reflects the new mechanism (why a placeholder GUC is needed to carry a value between top-level statements in a plain SQL script, and why the restore can't be left implicit).
• No other statements in the file reference session_replication_role or the new placeholder GUC outside these two points — clean, self-contained fix.

CI is green: the CI workflow run for the current head commit (93a1e9f) completed successfully (run).

Findings

None. This commit correctly and completely resolves the session-scoping issue flagged in the previous review pass, following the same placeholder-GUC convention already established elsewhere in this codebase.
· branch: u-and-u-testing