You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "jrgemignani (via GitHub)" <gi...@apache.org> on 2026/06/17 23:04:23 UTC
[PR] Make ag_catalog ownership and built-in resolution explicit [age]
jrgemignani opened a new pull request, #2440:
URL: https://github.com/apache/age/pull/2440
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make ag_catalog ownership and built-in resolution explicit [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2440:
URL: https://github.com/apache/age/pull/2440#discussion_r3431986555
##########
sql/age_main.sql:
##########
@@ -20,6 +20,33 @@
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION age" to load this file. \quit
+--
+-- Ensure ag_catalog is created and owned by the installing role.
+--
Review Comment:
The header comment says this block “ensures ag_catalog is created and owned”, but the DO block only enforces ownership when ag_catalog already exists (schema creation is handled elsewhere by CREATE EXTENSION / the control file). Rewording avoids implying the script creates the schema itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make ag_catalog ownership and built-in resolution explicit [age]
Posted by "muhammadshoaib (via GitHub)" <gi...@apache.org>.
muhammadshoaib merged PR #2440:
URL: https://github.com/apache/age/pull/2440
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make ag_catalog ownership and built-in resolution explicit [age]
Posted by "gregfelice (via GitHub)" <gi...@apache.org>.
gregfelice commented on PR #2440:
URL: https://github.com/apache/age/pull/2440#issuecomment-4763281212
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make ag_catalog ownership and built-in resolution explicit [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #2440:
URL: https://github.com/apache/age/pull/2440#issuecomment-4763369767
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org