Skip to content

Strengthen ALTER EXTENSION UPDATE safety (tracking) #93

Description

@jnasbyupgrade

Tracking issue for one question: does ALTER EXTENSION UPDATE reliably produce the same database as a fresh CREATE EXTENSION?

All work carries the update-safety label. Sub-issues are linked below.

Scope: this is forward-looking

Released update scripts are locked and will not be touched barring something serious. The pair that matters is the current development cyclesql/cat_tools--<last-released>.sql.insql/cat_tools.sql.in, with sql/cat_tools--<last-released>--stable.sql.in as the accumulator every SQL-touching PR extends. That accumulator exists and is tracked. Checks are run against historical pairs to validate the checks themselves, which is useful, but protecting those pairs is not the goal and findings against them are not blockers.

Corollary for tooling: false positives are cheap. Future update scripts can be required to satisfy whatever rule we adopt, so a check that occasionally needs a one-line waiver is fine. Silent failure — reporting green while having understood nothing — is not.

Separation of concerns

Changes that add or alter cat_tools features get their own PRs, separate from work that only improves CI and checking. Where a check would be more useful as a user-facing function than as test scaffolding, prefer the function — but weigh ongoing maintenance and testing cost before committing to that.

Choosing the right mechanism per case

The governing principle for everything below: think hard about the right way to verify each individual case. Use a parser where parsing is genuinely easy. Where it is not, the answer is a different mechanism — not a cleverer parser. A case that is awkward to check by text comparison is positive evidence that it belongs somewhere else, and declining to handle it in a static checker is the correct outcome rather than a coverage gap to apologize for.

Worked example: enum contents. The install script states a full label list in CREATE TYPE … AS ENUM (…); an update script says ALTER TYPE … ADD VALUE, one label at a time. Teaching a static checker that those two statement forms mean the same change is exactly the kind of special-casing that makes such a tool grow without bound. A test asserting each enum's contents is both simpler and stronger — the suite already runs identical expected output against fresh, updated and pg_upgraded databases, so a disagreement fails on its own, in whichever mode diverges. Enum support was therefore dropped from #92 and replaced by #97.

Status

Known limitation to document

A fresh install and an updated install can have identical runtime behaviour while differing in catalog state that pg_dump cares about. D1 was exactly this: typacl byte-identical and has_type_privilege true on both sides, yet five pg_init_privs rows differed, because ALTER DEFAULT PRIVILEGES grants implicitly and implicit grants are not snapshotted.

That means our checks are imperfect in a specific, nameable way, and that should be written down rather than discovered again. bin/structural_diff compares per-object ACL columns and reported these databases identical. A text-level check cannot see it at all. Only a pg_init_privs comparison or a pg_dump diff catches it.

Plan

Privilege correctness

ALTER DEFAULT PRIVILEGES stays. It is not merely documentation — it actively enforces the default, and removing it would break restore for already-updated databases whose dumps carry no explicit GRANT. Explicit REVOKE/GRANT normalization (landed in #96) is the second half of a belt-and-suspenders arrangement, not a replacement: ADP enforces the default, normalization guarantees the explicit end state.

Remaining: detect privilege drift directly (#95), so normalization is defence-in-depth rather than the only thing standing between us and drift. Deferred for now — the immediate priority was getting the two paths in sync, which #96 did. A pg_init_privs comparison must land after #96 or it fires on the divergence #96 fixed.

The ADP invariant check — for every pg_default_acl entry, assert every existing object of that class/schema/owner already carries the privilege — is worth building as a real cat_tools feature rather than test scaffolding, since it is useful against any database including production, and needs no superuser. Feature PR, separate from CI work. Weigh maintenance cost first.

Detection gaps

Release-time gate

The only moment this truly matters is a release, since that is when a version's contents are locked. A heavier, partly interpretive check is acceptable there and need not be cheap enough for every PR: pg_dump comparison of fresh vs updated across every supported update origin, pg_init_privs included, plus review of the update script against the inter-release diff. A dump comparison alone would have caught D1 with no new tooling.

Static early-warning check

#92, kept deliberately simple. Its value is catching small, subtle diffs — a changed function body, a changed grant, a tweaked view definition. Whole new objects are comparatively easy to spot unaided in a diff of the install script, so the check does not need to chase them and should not grow machinery to do so. Enum labels are explicitly out of scope; see the principle above and #97.

Related work, tracked separately

#88 (non-superuser install, superuser = off) constrains the permission model — see the comment below for the constraints adopted here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    update-safetyALTER EXTENSION UPDATE correctness: fresh-vs-updated equivalence

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions