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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
