Make ag_catalog ownership and built-in resolution explicit

[jrgemignani]

AGE places all of its objects in the ag_catalog schema. Make the assumptions around that schema explicit so installs and upgrades behave predictably regardless of how a database is provisioned:

• Ownership-checked install: CREATE EXTENSION age installs into ag_catalog only when that schema does not already exist under a different owner, keeping ownership of AGE's catalog well-defined.
• Deterministic name resolution: the pg_upgrade helper functions resolve built-ins from pg_catalog first and schema-qualify their format()/hashtext() calls, so their behavior does not depend on what else is defined in ag_catalog.
• README note describing ag_catalog ownership and the install-time check.

The upgrade script applies the same helper changes so existing installations get them on ALTER EXTENSION UPDATE. Adds an extension_security regression test covering the ownership check and the qualified-call / search_path properties.

Assisted-by: GitHub Copilot (Claude Opus 4.8)

modified: Makefile
modified: README.md
modified: age--1.7.0--y.y.y.sql
new file: regress/expected/extension_security.out
new file: regress/sql/extension_security.sql
modified: sql/age_main.sql
modified: sql/age_pg_upgrade.sql

[Copilot]

Pull request overview

This PR makes Apache AGE’s assumptions about the ag_catalog schema explicit to improve install/upgrade predictability and harden behavior against search-path object shadowing.

Changes:

• Add an install-time guard to refuse CREATE EXTENSION age when a pre-existing ag_catalog schema is owned by a different role.
• Make pg_upgrade helper functions resolve built-ins deterministically by preferring pg_catalog and schema-qualifying format()/hashtext() calls.
• Add regression coverage for the helper search_path/qualification properties and the ownership-detection logic, plus a README note documenting the new install behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sql/age_main.sql	Adds an install-time ownership guard for ag_catalog.
sql/age_pg_upgrade.sql	Prefers pg_catalog in helper search_path and qualifies key built-in calls.
age--1.7.0--y.y.y.sql	Applies the same pg_upgrade helper hardening in the extension upgrade script.
regress/sql/extension_security.sql	Adds a regression test validating helper proconfig and built-in qualification patterns.
regress/expected/extension_security.out	Expected output for the new regression test.
Makefile	Registers the new extension_security regression test.
README.md	Documents the ag_catalog ownership expectation and install-time refusal behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gregfelice]

Reviewed the full diff and traced the resolution/ownership changes against the actual function bodies and the control file. This is a clean, well-scoped hardening change — I'd merge it. (Read carefully; did not rebuild locally — the 38/38 installcheck is the author's.)

Verified

• The search_path flip is safe. Changing ag_catalog, pg_catalog -> pg_catalog, ag_catalog in the four pg_upgrade helpers only matters if some unqualified ag_catalog object reference relied on ag_catalog being first. I checked all four bodies: every ag_catalog table is already schema-qualified (ag_catalog.ag_graph, ag_catalog.ag_label, ...), there are no unqualified AGE function/operator calls, and _graphid_mapping is a TEMP table (resolves via the implicit pg_temp entry regardless of the explicit path). So the flip can only change built-in resolution, which is the intent.
• The install guard is reachable and effective. With schema = 'ag_catalog' in age.control and no explicit CREATE SCHEMA in age_main.sql, PostgreSQL creates ag_catalog (owned by the installer) when it does not pre-exist, so the guard is a no-op on the common path and fires only for a pre-existing, foreign-owned ag_catalog — exactly the case it is meant to catch. Placing it at the top of age_main.sql, before any object creation, is correct.
• Guard is correctly absent from the upgrade script. It lives only in the fresh-install path; the upgrade template gets only the qualification/search_path changes, so an ALTER EXTENSION ... UPDATE on an existing (possibly foreign-owned) ag_catalog won't be broken by it. Good call.
• Ownership compared by OID, not role membership — correct for the superuser-is-implicitly-a-member-of-every-role case, and the comment documents the intent.
• Nice touch: the regress test asserts via prosrc ~ '...' that no unqualified format(/hashtext( calls survive.

Suggestions (non-blocking)

1. The guard's actual firing is untested end-to-end. The regress test exercises the detection predicate (the EXISTS/nspowner <> query) in isolation, but never drives a real CREATE EXTENSION age against a foreign-owned ag_catalog, so the RAISE EXCEPTION path and its HINT are not covered. Understandable given the regress harness already has the extension installed — but a TAP test (or a scripted install into a fresh DB owned by a second role) would close the loop on the one behavior this PR actually adds.
2. Minor: the [^.]\mformat\s*\( regex requires a non-dot character before the keyword, so a format( at the very start of prosrc would slip through; in practice there is always leading whitespace, so this is negligible.

Solid work — the OID-based ownership comparison and the upgrade-path exclusion show good attention to the edge cases.

[gregfelice]

Approving. Clean, well-scoped hardening. I verified the two things that mattered: the search_path flip is safe (all ag_catalog tables in the four pg_upgrade helpers are schema-qualified, no unqualified AGE function/operator calls, temp tables resolve via pg_temp regardless), and the install guard is reachable and effective given schema = 'ag_catalog' in the control file — firing only for a pre-existing, foreign-owned ag_catalog, and correctly absent from the upgrade path.

Verified the cassert PG18 gate locally (CI-mirror: REL_18_STABLE --enable-cassert -O0, -Werror clean build, EXTRA_TESTS="pgvector fuzzystrmatch pg_trgm"): All 42 tests passed, including the new extension_security test, no regression.diffs.

Non-blocking: the guard's actual RAISE-on-install path is only exercised via the detection predicate, not end-to-end — a TAP/fresh-DB test would close that loop as a follow-up.

[muhammadshoaib]

Looks good

[jrgemignani]

Reviewed the full diff and traced the resolution/ownership changes against the actual function bodies and the control file. This is a clean, well-scoped hardening change — I'd merge it. (Read carefully; did not rebuild locally — the 38/38 installcheck is the author's.)

Verified

• The search_path flip is safe. Changing ag_catalog, pg_catalog -> pg_catalog, ag_catalog in the four pg_upgrade helpers only matters if some unqualified ag_catalog object reference relied on ag_catalog being first. I checked all four bodies: every ag_catalog table is already schema-qualified (ag_catalog.ag_graph, ag_catalog.ag_label, ...), there are no unqualified AGE function/operator calls, and _graphid_mapping is a TEMP table (resolves via the implicit pg_temp entry regardless of the explicit path). So the flip can only change built-in resolution, which is the intent.
• The install guard is reachable and effective. With schema = 'ag_catalog' in age.control and no explicit CREATE SCHEMA in age_main.sql, PostgreSQL creates ag_catalog (owned by the installer) when it does not pre-exist, so the guard is a no-op on the common path and fires only for a pre-existing, foreign-owned ag_catalog — exactly the case it is meant to catch. Placing it at the top of age_main.sql, before any object creation, is correct.
• Guard is correctly absent from the upgrade script. It lives only in the fresh-install path; the upgrade template gets only the qualification/search_path changes, so an ALTER EXTENSION ... UPDATE on an existing (possibly foreign-owned) ag_catalog won't be broken by it. Good call.
• Ownership compared by OID, not role membership — correct for the superuser-is-implicitly-a-member-of-every-role case, and the comment documents the intent.
• Nice touch: the regress test asserts via prosrc ~ '...' that no unqualified format(/hashtext( calls survive.

Suggestions (non-blocking)

1. The guard's actual firing is untested end-to-end. The regress test exercises the detection predicate (the EXISTS/nspowner <> query) in isolation, but never drives a real CREATE EXTENSION age against a foreign-owned ag_catalog, so the RAISE EXCEPTION path and its HINT are not covered. Understandable given the regress harness already has the extension installed — but a TAP test (or a scripted install into a fresh DB owned by a second role) would close the loop on the one behavior this PR actually adds.
2. Minor: the [^.]\mformat\s*\( regex requires a non-dot character before the keyword, so a format( at the very start of prosrc would slip through; in practice there is always leading whitespace, so this is negligible.

Solid work — the OID-based ownership comparison and the upgrade-path exclusion show good attention to the edge cases.

@gregfelice ty for the feedback! Unfortunately, with our current regression tests (and maybe we should consider changing them) that functionality (1) can't be tested due to how the regression tests work. This was tested locally to verify that it worked, though.