Add bin/update_lint: static check that the update script covers the object diff - #90
Add bin/update_lint: static check that the update script covers the object diff#90jnasbyupgrade wants to merge 2 commits into
Conversation
…bject diff A PR that changes an extension's SQL must also extend the update script from the last released version, so an install that got there via ALTER EXTENSION UPDATE reaches the same objects as a fresh install. Nothing enforced that until the runtime check ran, which needs a database, seven PostgreSQL majors and minutes of CI. bin/update_lint parses the two install scripts on either side of an update, diffs their object sets, and verifies the update script accounts for every added and removed object. It needs no database, no make and no pg_config -- it reads the tracked .sql.in sources directly -- so it runs in the existing cheap lint job alongside the style linter. It compares object IDENTITY, never definition, so it is an early-warning net rather than an authority; bin/structural_diff remains the check that a fresh install and an updated install are actually equivalent. Run against the frozen pre-0.2.2 update scripts it reports the five enum types that never got GRANT USAGE, the same defect structural_diff found at runtime, which is pinned in the test suite as a known-bad oracle. Only the update path into the current unreleased version is linted by default. Released version files are frozen once tagged, so a finding against a historical pair could never be fixed and would be permanently red; there is deliberately no baseline or suppression file, because the exclusion is structural. Historical pairs stay reachable by passing an explicit pair. The Makefile target stays unwired from `lint` in both directions: lint.mk's vendored include is guarded on $(wildcard .git), so `lint` does not exist as a target in a released tarball and `make lint` fails loudly there. Naming it as a prerequisite would define it with no recipe and turn that loud failure into a silent pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
Claude finished @jnasbyupgrade's task in 2m 32s —— View job Claude is reviewing this PR
|
CLAUDE.md had no mention of `make update-lint`/`bin/update_lint`, so the only way to discover it was a red CI check; add a short entry next to the SQL file conventions it enforces. Also add a `hint:` line to the linter's gap-found failure path so a first-time reader knows what to do next (extend the named update script) instead of just that something is wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @jnasbyupgrade's task in 8m 10s —— View job I'll analyze this and get back to you. |

A PR that changes an extension's SQL must also extend the update script from the last released version, so an install that got there via
ALTER EXTENSION UPDATEreaches the same objects as a fresh install. Nothing enforced that until the runtime check ran, which needs a database, seven PostgreSQL majors and minutes of CI.bin/update_lintparses the two install scripts on either side of an update, diffs their object sets, and verifies the update script accounts for every added and removed object. It needs no database, nomakeand nopg_config— it reads the tracked.sql.insources directly — so it runs in the existing cheap lint job alongside the style linter, in about a tenth of a second.It compares object identity, never definition, so it is an early-warning net rather than an authority:
bin/structural_diffremains the check that a fresh install and an updated install are actually equivalent. A clean run here means only that no object was added or removed without the update script accounting for it.It reproduces a real defect
Run against the frozen pre-0.2.2 update scripts, it reports the five enum types that never got
GRANT USAGE:That is the same defect
bin/structural_difffound at runtime in #55, fixed forward in #68. Both pairs are pinned in the suite as a known-bad oracle: the files are frozen, so the finding is stable and documents the history.Catching it statically depends on modelling
ALTER DEFAULT PRIVILEGESas persistentpg_default_aclstate seeded from the old install script, since the old database already ran that statement. Without the seeding,0.2.3→0.3.0false-positives on every new type; with per-file-only flags, the historical gap is invisible.Scope
Only the update path into the current unreleased version is linted by default. Released version files are frozen once tagged, so a finding against a historical pair could never be fixed and would be permanently red — the same reasoning that already scopes
LINT_TARGETSaway from them. There is deliberately no baseline or suppression file: the exclusion is structural, and historical pairs stay reachable by passing an explicit pair. The linted pair is derived fromdefault_versionand the highest tracked release rather than hardcoded, so it follows the release cycle with no edit. A missing update script is treated as empty rather than skipped, since skipping would pass silently on exactly the omission this check exists to catch.Parsing
Most of this extension's objects never appear in a
CREATEstatement —sql/cat_tools.sql.inbuilds 66 functions through__cat_tools.create_function()against 4 literalCREATE FUNCTIONs — so aCREATE-scanning lint would see a fraction of the extension and report all-clear. A quote- and comment-aware scanner splits statements, a dispatch table classifies each one, and dynamic SQL is followed through three gateways:create_function(),__cat_tools.exec(), andDOblocks. That distinction is load-bearing in both directions. Four views and a role exist only inside dollar quotes reached through a gateway, whilecreate_function's own body containsformat()templates (CREATE OR REPLACE FUNCTION %s() that must not become objects.Any statement the dispatch table does not recognize is a hard error rather than a silent skip, which is what keeps a green run meaningful. All 20 SQL sources in the tree parse with no unrecognized statement, so this lands with no suppression backlog.
Verification
Beyond the 145-test suite, the extracted object set was diffed against the real catalog after installing the extension into PostgreSQL 17: 192/192 exact match across schemas, relations, indexes, types, enum labels, functions, aggregates, casts and constraints, and no blind spots on ACLs or comments.
make update-lintruns the check andmake update-lint-testruns the suite; both are wired into the existing lint job. The target stays unwired fromlintin both directions, deliberately:lint.mk's vendored include is guarded on$(wildcard .git), solintdoes not exist as a target in a released tarball andmake lintfails loudly there. Naming it as a prerequisite would define it with no recipe and turn that loud failure into a silent pass.Known limitations
Documented in the script header, and the reason it is a net rather than an authority:
cat_tools.relation__kindand eight other names.REVOKE-only update script satisfies coverage for an added grant. That is the same class as the five-grant finding above.ALTER DEFAULT PRIVILEGESseeding assumes the same role runs the update as ran the install, sincepg_default_aclis per-role.-- SED:markers resolve to the newest-PG branch, so an object existing only on a pre-floor major is not tracked.