diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e0d9a2..8973954 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -394,6 +394,10 @@ jobs: steps: - name: Check out the repo uses: actions/checkout@v7 + - name: Test bin/ + # First, before anything that uses bin/: a bug in the tooling should be + # reported as a bug in the tooling, not as a finding against the SQL. + run: make test-bin - name: Lint SQL # CRITICAL: call `make lint` directly, not some other path (a script, a # different target, etc). lint.mk's vendored include is guarded on @@ -404,6 +408,12 @@ jobs: # wrapper that swallows that exit code or calls a different entry point # would defeat this safety net. run: make lint + - name: Check the update script covers this cycle's install-script changes + # Static and database-free (see bin/update_lint_textfirst's header), + # which is why it rides this job rather than the pgxn-tools container. + # A step, not a job: all-checks-passed hard-fails when the workflow's + # job-key set differs from its `needs:`. + run: make update-lint # cancel-on-close.yml cancels in-flight CI/claude-review runs on PR close by # joining the SAME concurrency group strings this workflow and diff --git a/.vendor/linter b/.vendor/linter index b8632c2..e4fab69 160000 --- a/.vendor/linter +++ b/.vendor/linter @@ -1 +1 @@ -Subproject commit b8632c2a3d93de664a45f4622235871e8f19cf78 +Subproject commit e4fab69e7f6256682f5b6b80e7a8f2423a0c39a9 diff --git a/CLAUDE.md b/CLAUDE.md index bbe9480..0a37c70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,6 +55,13 @@ documented in `../ai/CLAUDE.md` or pgxntool's own docs. `.sql` directly (historical exception; see `sql/.gitignore`). - Never hand-edit a generated `.sql` file — edit the `.sql.in` (or, for the base file, `sql/cat_tools.sql.in`) and regenerate. +- **A change to `sql/cat_tools.sql.in` must also extend the update + script `sql/cat_tools----stable.sql.in`**, so an + existing install reaches the same objects. `make update-lint` checks + that statically (no database) and runs in CI's `lint` job; see + `bin/update_lint_textfirst`'s header for what it catches, what it + cannot, and the `-- update-lint: ok /REGEX/ reason` escape hatch for + deliberate divergence. See [`../ai/CLAUDE.md`](../ai/CLAUDE.md) for the general (pgxntool-level) rules this preprocessing sits on top of: why version-specific install and diff --git a/Makefile b/Makefile index 4c200bc..ad5f367 100644 --- a/Makefile +++ b/Makefile @@ -105,3 +105,25 @@ clean_old_version: # `.vendor/linter/sql/bin/sql-lint sql/cat_tools--0.3.0.sql.in`. LINT_TARGETS = sql/cat_tools.sql.in test/ include lint.mk + +# Static check that this cycle's update script accounts for every change to the +# install script -- no database, no SQL parser. See bin/update_lint_textfirst's +# header for what it does and does not catch. +# +# Deliberately NOT tied to `lint` in either direction. lint.mk's 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. +.PHONY: update-lint +update-lint: + bin/update_lint_textfirst + +# Everything in bin/ that the rest of the suite leans on -- bin/test_existing +# and bin/structural_diff drive the pgTAP runs, bin/update_lint_textfirst drives +# `update-lint` above -- so run this before anything that uses them: a broken +# tool otherwise reports as a broken extension. prove takes the directory, so a +# new bin/test/*.t needs no edit here. Test::Harness is core Perl; no setup. +.PHONY: test-bin +test-bin: + prove bin/test/ diff --git a/RELEASE.md b/RELEASE.md index 8a58948..1aed672 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -1,145 +1,24 @@ # Releasing cat_tools -cat_tools builds on pgxntool (https://github.com/Postgres-Extensions/pgxntool); -the release machinery (`make tag`, `make dist`) lives in `pgxntool/base.mk`. These -steps cut a new release. - -## 1. Safety check: verify committed version files haven't drifted - -Before anything else, confirm every committed versioned install script still matches -what that version actually shipped. - -- [ ] For each committed versioned install script — `sql/cat_tools--.sql.in` - (0.2.0 onward) or `sql/cat_tools--.sql` (the historical pre-0.2.0 files - `0.1.0`/`0.1.3`/`0.1.4`/`0.1.5`, which have no `.sql.in` source and are tracked - directly; see CLAUDE.md "SQL file conventions") — find its last-touching commit: - `git log -1 --format='%H %ad' -- sql/cat_tools--.sql.in` (or `.sql`). -- [ ] Confirm that commit is no later than when that version was actually tagged. - cat_tools tags every release, unprefixed (e.g. `0.2.2`, `0.2.3` — see step 6's - `make tag`), so compare directly: `git log -1 --format='%H %ad' `. (No - tags exist before `0.2.2` — `0.1.x`, `0.2.0`, and `0.2.1` predate cat_tools' - tagging convention — so this direct comparison is only available from `0.2.2` - onward.) -- [ ] A version file touched by a commit LATER than its own release's tag is a red - flag — it likely means `default_version` in `cat_tools.control` was left pointing - at that real (non-`stable`) version after release, and a later source edit to - `sql/cat_tools.sql.in` silently regenerated — and corrupted — the committed file - via `sql.mk`'s current-version rule (`$(EXTENSION__CURRENT_VERSION__FILES:.sql=.sql.in): - sql/cat_tools.sql.in cat_tools.control`, which `cp`s the base source over it). - Investigate before proceeding. -- [ ] **Known exception, not necessarily a corruption:** a version file whose - last-touching commit is much later than its version's real release can also mean - the file was legitimately backfilled or reformatted after the fact (e.g. once new - build tooling started requiring something that wasn't tracked before). A late - touch-date alone isn't suspicious — only worry about a file whose *content* - actually differs from what shipped. - -## 2. Pre-release checks -- [ ] Open issues/PRs for this release reviewed, merged or deferred. -- [ ] CI green on all supported PostgreSQL versions (the `all-checks-passed` job on master). -- [ ] Locally: `make verify-results` passes. It depends on `$(TEST_DEPS)` directly, not - on `test` itself (deliberately — see `base.mk`'s own comment on why: `test`'s early - exit on a regression would otherwise abort the chain before verify-results got to - inspect and report the diff), so it still runs the suite first, then gates on the - results with a stricter pgtap-aware check. `make test` itself also returns - non-zero on a regression as of pgxntool 2.3.0, but `verify-results` remains the - stricter, documented gate. - -## 3. Decide the version and what to track -- [ ] Pick the new version (semantic versioning). -- [ ] **Minor change? Consider NOT committing the generated versioned install script.** - If this release makes only fairly minor changes (unlikely to cross a PostgreSQL - supported-version boundary), decide whether to omit the generated - `sql/cat_tools--.sql.in` — it has little update-test-coverage value and - omitting it keeps the repo smaller (it is regenerated from `sql/cat_tools.sql.in` - at build time). The update script (`sql/cat_tools----.sql.in`) is - ALWAYS committed. See CLAUDE.md "SQL file conventions". - -## 4. Update version + changelog - -> **⚠️ CRITICAL — you are temporarily leaving the `stable` pseudo-version.** Master's -> `default_version` normally sits at the `stable` pseudo-version so that source edits -> regenerate `cat_tools--stable.sql` and never a frozen released file. Stamping a real -> version here points the generated current-version file at `cat_tools--`. The -> moment this release is merged you **MUST** flip `default_version` back to `stable` on -> master (step 7). If you forget, the next source edit on master will regenerate — and -> corrupt — the just-released version's install file. -> -> `sql/cat_tools--.sql.in` must ALWAYS be tracked — that's the entire point, -> keeping a real history of the extension across releases. `stable` is the ONE exception -> (its `.sql.in` is gitignored, not tracked) for the ONE reason that `stable` itself is -> never actually released, so there's no history to keep. - -- [ ] Bump `default_version` in `cat_tools.control` (bumped by hand). -- [ ] Bump the version in `META.in.json` — the source of truth is - `provides.cat_tools.version` (also update the top-level `version`); `META.json`, - `control.mk`, and `meta.mk` (which feeds `PGXNVERSION`) regenerate via `make`. -- [ ] Advance `release_status` in `META.in.json` as appropriate (unstable → testing → - stable). -- [ ] Add/finish the update script `sql/cat_tools----.sql.in`; confirm - `ALTER EXTENSION cat_tools UPDATE` from the previous version reaches the new one, - on multiple PG majors. -- [ ] Stamp `HISTORY.asc`: the top `STABLE` section accumulates user-facing changes as - PRs land; at release, rename that header to the new version number. - -## 5. Verify -- [ ] `make verify-results` green (it runs `test` first, then gates on the results). -- [ ] From a clean checkout (or `git archive` of the tag): `make && make install` - regenerates and installs cleanly and `CREATE EXTENSION cat_tools;` reports the new - version — confirms a PGXN consumer can build from the tracked sources alone. (This - mirrors what `make dist` ships, since it archives the tag: committed files only, so - any omitted generated install script is regenerated on the consumer's side.) - -## 6. Tag and distribute - -> **⚠️ Pass `PGXN_REMOTE=` to every target below if your clone's `origin` -> is a personal fork** (as it typically is for a maintainer working from a fork, -> with the canonical repo configured under some other remote name — check -> `git remote -v` rather than assuming it's called `upstream`). Without it, -> `tag`/`rmtag`/`forcetag`/`dist` push to `origin` by default — silently -> tagging the fork instead of `Postgres-Extensions/cat_tools`. This is exactly -> what happened when the 0.2.3 release tag was first cut and had to be -> re-pointed by hand; pgxntool 2.2.0 added `PGXN_REMOTE` specifically to fix -> this (https://github.com/Postgres-Extensions/pgxntool/issues/53). - -- [ ] Commit the release changes; working tree must be clean — `make tag` aborts with - "Untracked changes!" on a dirty tree. -- [ ] `make tag` — creates a git tag named exactly the version, UNPREFIXED (e.g. `0.2.3`, - matching the existing `0.2.2` tag; no `v` prefix), taken from `PGXNVERSION`, and - pushes it to `$(PGXN_REMOTE)` (default `origin`). It is idempotent when the tag - already points at HEAD, and errors if the tag exists on a different commit. To - move an existing tag use `make forcetag` (= `make rmtag` then `make tag`); - `make rmtag` deletes the tag locally and on `$(PGXN_REMOTE)`. -- [ ] `make dist` — depends on `tag` (and builds the HTML docs), then - `git archive`s the tag into `../cat_tools-.zip` (parent directory). - Because it archives the tag, only committed files are included. If a `.gitattributes` - exists it must be committed, or `dist` aborts (git archive only honors - `export-ignore` for committed files). `make forcedist` = `forcetag` + `dist`. -- [ ] Upload the `../cat_tools-.zip` to PGXN (manual). - -## 7. Return master to `stable` (CRITICAL — do not skip) -- [ ] As soon as the release is merged, flip `default_version` back to the `stable` - pseudo-version on master (`cat_tools.control` + `META.in.json`), open a new top - `STABLE` section in `HISTORY.asc`, and re-seed a fresh - `sql/cat_tools----stable.sql.in` update script for the next cycle. - Leaving master stamped at the real version means the next source edit regenerates - and corrupts the released version's install file. The just-released - `sql/cat_tools--.sql.in` stays tracked, permanently — nothing to clean up - or remove there; see step 4's note above. - -> The persistent `stable` pseudo-version (a permanent version literally named `stable`, -> with a live `sql/cat_tools----stable.sql.in` update script that every -> source fix targets) decouples fixes from version bumps. The machinery is built into -> `sql.mk`; it lands immediately after the 0.2.3 release, so 0.2.3 itself is the last -> release cut before the scheme exists — steps 4/7 above describe the flow from the -> next release onward. - -## Notes and caveats - -- **0.2.3 catalog-view repair / `pg_upgrade` caveat.** Databases updated from - 0.2.0/0.2.1 hold broken catalog views that fail binary `pg_upgrade` to PostgreSQL - 12+ until the extension is updated to 0.2.3 (the update rebuilds them, dropping and - recreating the public `pg_class_v`/`column`/`pg_class()` objects without `CASCADE`). - Surface this in the release notes when people may cross the PG 11 → 12+ boundary. See - README.asc "Updating the extension" and - https://github.com/Postgres-Extensions/cat_tools/pull/42. +See [`../ai/RELEASE.md`](../ai/RELEASE.md) for the actual release process. +What follows is repo-specific context that doesn't belong in that shared +doc. + +## Pre-0.2.2 releases predate tagging + +cat_tools' first git tag is `0.2.2`; `0.1.0`/`0.1.3`/`0.1.4`/`0.1.5`, `0.2.0`, +and `0.2.1` were never tagged. The shared doc's step 1 (verify committed +version files haven't drifted) calls for comparing a version file's +last-touching commit against that version's tag — there's no tag to compare +against for any of those versions. See CLAUDE.md "SQL file conventions" for +how those pre-0.2.0 versions are tracked (plain `.sql`, no `.sql.in`). + +## 0.2.3 catalog-view repair / `pg_upgrade` caveat + +Databases updated from 0.2.0/0.2.1 hold broken catalog views that fail +binary `pg_upgrade` to PostgreSQL 12+ until the extension is updated to +0.2.3 (the update rebuilds them, dropping and recreating the public +`pg_class_v`/`column`/`pg_class()` objects without `CASCADE`). Surface this +in the release notes when people may cross the PG 11 → 12+ boundary. See +README.asc "Updating the extension" and +https://github.com/Postgres-Extensions/cat_tools/pull/42. diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t new file mode 100644 index 0000000..ea40123 --- /dev/null +++ b/bin/test/textfirst.t @@ -0,0 +1,226 @@ +#!/usr/bin/env perl +# +# One case per rule bin/update_lint_textfirst implements, plus the three things +# only the real tree can prove: that the current development pair is clean, that +# the ALTER DEFAULT PRIVILEGES rule reproduces the historical bug it was written +# for, and that preprocessing erases sql.mk's " VERSIONED FILE!" tag. Kept +# deliberately small -- a checker whose test suite dwarfs it has stopped being +# the cheap option. Run from the repo root: +# +# prove bin/test/textfirst.t + +use strict; +use warnings; +use Test::More tests => 32; +use File::Temp qw(tempdir); + +my $PROG = 'bin/update_lint_textfirst'; +my $DIR = tempdir(CLEANUP => 1); + +# Write an OLD/NEW/UPDATE trio and run the linter over it. +sub run_trio { + my (%f) = @_; + my @p; + for my $k (qw(old new update)) { + my $p = "$DIR/$k.sql"; + open my $fh, '>', $p or die $!; + print $fh $f{$k} // ''; + close $fh; + push @p, $p; + } + my $out = qx{$^X $PROG @p 2>&1}; + return ($? >> 8, $out); +} + +# --- the splitter ----------------------------------------------------------- + +{ + # A `;` inside a string, a quoted identifier, a comment and a dollar-quoted + # body must not end a statement. If any did, NEW would hold extra + # statements OLD lacks and they would be reported as added. + my $sql = <<'SQL'; +SELECT 'a;b'; +SELECT "we;ird"; +SELECT 1; -- trailing ; comment +CREATE FUNCTION f() RETURNS int LANGUAGE plpgsql AS $body$ +BEGIN + /* nested /* block ; comment */ still in here ; */ + RETURN 1; +END +$body$; +SQL + my ($rc, $out) = run_trio(old => $sql, new => $sql, update => ''); + is $rc, 0, 'identical files are clean'; + like $out, qr/old \S+ \(4 statements\)/, 'four top-level statements found'; +} + +# --- rule 4: substring against the whole update file ------------------------ + +{ + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TABLE t (a int);\n", + # Wrapped in a DO block, so it is not a top-level statement over here. + update => "DO \$\$ BEGIN EXECUTE 'CREATE TABLE t (a int)'; END \$\$;\n", + ); + is $rc, 0, 'a copy buried in a DO block satisfies the added statement'; + like $out, qr/added 1 \(matched 1,/, '... and is counted as matched'; +} + +{ + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TABLE t (a int);\n", + update => "SELECT 2;\n", + ); + is $rc, 1, 'an added statement with no copy fails'; + like $out, qr/CREATE TABLE t \(a int\)/, '... and is named in the finding'; +} + +# --- enum labels, which are out of scope ------------------------------------ + +{ + # The pgTAP suite asserts enum contents against the catalog across fresh, + # updated and pg_upgraded databases, so this does not second-guess it. + my ($rc, $out) = run_trio( + old => "CREATE TYPE e AS ENUM ('a', 'b');\n", + new => "CREATE TYPE e AS ENUM ('a', 'b', 'c');\n", + update => "SELECT 1;\n", + ); + is $rc, 0, 'a changed enum on a pre-existing type is exempt'; + like $out, qr/enum 1,/, '... and the exemption is reported, not silent'; +} + +{ + # Only CHANGED enums are exempt. A brand-new one copies into the update + # script verbatim, so the ordinary rule has to keep applying to it. + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TYPE e AS ENUM ('a');\n", + update => "SELECT 2;\n", + ); + is $rc, 1, 'a brand-new enum type is still checked for a copy'; + like $out, qr/CREATE TYPE e AS ENUM/, '... and is named in the finding'; +} + +# --- scaffolding ------------------------------------------------------------ + +{ + # __cat_tools is created and dropped inside the install script, so its + # contents cannot differ between a fresh and an updated database. + my ($rc, $out) = run_trio( + old => "CREATE FUNCTION __cat_tools.helper() RETURNS void LANGUAGE sql AS 'SELECT';\n", + new => "CREATE FUNCTION __cat_tools.helper(a int) RETURNS void LANGUAGE sql AS 'SELECT';\n", + update => "SELECT 1;\n", + ); + is $rc, 0, 'a changed scaffolding definition is exempt'; + like $out, qr/scaffolding 1,/, '... and the exemption is reported, not silent'; +} + +{ + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nSELECT __cat_tools.create_function('cat_tools.f');\n", + update => "SELECT 2;\n", + ); + is $rc, 1, 'a CALL to scaffolding is still checked -- it creates a real object'; + like $out, qr/create_function\('cat_tools\.f'\)/, '... and is named in the finding'; +} + +# --- the escape hatch ------------------------------------------------------- + +{ + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TABLE t (a int);\n", + update => "-- update-lint: ok /CREATE TABLE t/ hand-rewritten below\n" + . "CREATE TABLE t (a int NOT NULL);\n", + ); + is $rc, 0, 'a waiver suppresses the finding'; + like $out, qr/waived .*hand-rewritten below/, '... and prints its reason'; +} + +{ + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\n", + update => "-- update-lint: ok /nothing matches this/ stale\n", + ); + is $rc, 1, 'a waiver that matches nothing fails'; + like $out, qr{stale waiver /nothing matches this/}, '... and says which one'; +} + +{ + # Whitespace closes the regex, not the first `/`. Closing at the first one + # would waive /a/ here -- far wider than written, and silently. + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nSELECT a/b;\n", + update => "-- update-lint: ok /a/b/ division is fine\n", + ); + is $rc, 0, 'a waiver regex may contain a slash'; + like $out, qr/waived .*division is fine/, '... and keeps the whole reason'; +} + +# A typo must not degrade into "no waiver at all". Both of these would otherwise +# leave the author staring at a finding they believe they already waived. +for my $bad ( + ['-- update-lint: ok /CREATE TABLE t/', 'a waiver with no reason'], + ['-- update-lint: okay /CREATE TABLE t/ mistyped', 'a mistyped waiver keyword'], +) { + my ($line, $desc) = @$bad; + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TABLE t (a int);\n", + update => "$line\n", + ); + is $rc, 2, "$desc is a usage error, not a silent no-op"; + like $out, qr/malformed waiver/, '... naming it as malformed'; +} + +# --- ALTER DEFAULT PRIVILEGES ----------------------------------------------- + +{ + # Only one ADP shape is understood. A different one must say so rather than + # pass as "nothing to see" -- a copy in the update script is not enough, + # since ADP does not reach objects that already exist. + my $adp = 'ALTER DEFAULT PRIVILEGES FOR ROLE r IN SCHEMA s GRANT USAGE ON TYPES TO u;'; + my ($rc, $out) = run_trio( + old => "SELECT 1;\n", + new => "SELECT 1;\n$adp\n", + update => "$adp\n", + ); + is $rc, 1, 'an ALTER DEFAULT PRIVILEGES form the rule cannot read fails'; + like $out, qr/unrecognized ALTER DEFAULT PRIVILEGES form/, '... saying so, not skipping it'; +} + +# --- against the real tree -------------------------------------------------- + +SKIP: { + skip 'run from the repo root', 6 unless -e 'sql/cat_tools--0.2.1.sql.in'; + + # No arguments at all: the pair every SQL-touching PR is judged on. It has + # to be clean, or the CI step this drives is useless from the day it lands. + my $out = qx{$^X $PROG 2>&1}; + is $? >> 8, 0, 'the current development pair is clean'; + like $out, qr{new sql/cat_tools\.sql\.in\b}, '... comparing against the base install script'; + like $out, qr{update sql/cat_tools--\S+--stable\.sql\.in}, '... via this cycle\'s update script'; + + # A released install script is a copy of the base file with sql.mk's + # " VERSIONED FILE!" tag added to every @generated@ marker. One of those + # markers sits inside a dollar-quoted function body where no comment strip + # can reach it, so the pair above is only clean if preprocessing erases the + # difference. + unlike $out, qr/create_function/, '... with no @generated@ tag false positive'; + + $out = qx{$^X $PROG --versions 0.2.1 0.2.2 2>&1}; + my @gaps = $out =~ /^\S+: TYPE (\S+) predates/mg; + is_deeply [sort @gaps], [sort qw( + cat_tools.constraint_type cat_tools.procedure_type cat_tools.relation_type + cat_tools.relation_relkind cat_tools.object_type + )], 'the five enum types that never got GRANT USAGE are flagged'; + + # ADP was already in force across this pair, so nothing predates it. + $out = qx{$^X $PROG --versions 0.2.2 0.2.3 2>&1}; + unlike $out, qr/predates/, 'an ADP present in both installs raises nothing'; +} diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst new file mode 100755 index 0000000..f686382 --- /dev/null +++ b/bin/update_lint_textfirst @@ -0,0 +1,566 @@ +#!/usr/bin/env perl +# +# update_lint_textfirst - catch an install-script change whose update script +# forgot to keep up, statically: no database, no SQL parser, seconds not +# minutes. +# +# A PR that changes an extension's SQL must also extend +# sql/----.sql.in, or an existing install reaches a different +# set of objects than a fresh one. Nothing fails when that is forgotten until a +# runtime check that needs a database and minutes of CI. +# +# USAGE +# +# bin/update_lint_textfirst # the current cycle +# bin/update_lint_textfirst --versions OLD NEW # any other pair +# bin/update_lint_textfirst OLD NEW UPDATE # explicit paths +# Options: --sql-dir DIR (default "sql"), --ext NAME (default "cat_tools"). +# Exit: 0 clean, 1 findings, 2 usage error. +# +# With no arguments it checks the only pair that is still editable: the current +# development cycle. OLD is the last released install script, NEW is the base +# sql/.sql.in, and the update script is the one every SQL-touching PR +# extends. Both version numbers are derived -- the current one from +# .control's default_version, the previous one from the single update +# script named after it -- so a release needs no edit here. Released pairs are +# frozen and are not in scope by default; running them by hand is a way to +# validate this script, not a gate. +# +# HOW IT WORKS +# +# Never build a model of what an object IS. Compare STATEMENT TEXT. +# +# 1. Split both install scripts into top-level statements (quote-, comment- +# and dollar-quote-aware, so a `;` inside any of those is not a boundary). +# 2. Normalize each statement: drop comments, collapse whitespace. +# 3. Multiset-diff the two normalized statement lists. In NEW but not OLD is +# "added"; in OLD but not NEW is "removed". +# 4. Every added statement must appear, normalized, SOMEWHERE in the +# normalized update script -- substring match against the whole file, not +# statement against statement, so a copy wrapped in a DO block, a format() +# or an IF still matches. +# 5. Anything left over is a finding; exit non-zero. +# +# That works because update scripts are overwhelmingly copy-paste from the +# install script. Aim it at the small, subtle diffs -- a changed function body, +# a changed GRANT, a tweaked view definition. A whole new object is hard to miss +# in a diff of the install script between two versions, so nothing here chases +# that case, and the deliberate cost of that choice is that this stays small. +# +# The interesting content is therefore not the matcher. It is the short list of +# cases where a copy is impossible, below, plus the escape hatch for the rest. +# +# ENUM LABELS -- out of scope, checked properly elsewhere +# +# A changed CREATE TYPE ... AS ENUM whose type already existed in OLD is +# exempt, and deliberately so: enum contents are verified at runtime by the +# pgTAP suite, which asserts them against the catalog. See the limitations +# below for why that is the right place and this is not. +# +# SCAFFOLDING +# +# Statements that create or drop an object in the __ schema are exempt. +# That schema is created at the top of the install script and dropped at the +# bottom, so nothing in it survives CREATE EXTENSION and no divergence there +# can leave an updated database short an object; the update script's own copy +# of a helper is free to be bound to whatever names existed when it was +# written. Only definitions are exempt, never calls: a +# `SELECT __cat_tools.create_function('cat_tools.foo', ...)` creates a real +# object and is still checked. +# +# ALTER DEFAULT PRIVILEGES +# +# ADP is not retroactive, so a copy of it in the update script is not enough. +# When NEW's install adds an ADP that OLD's install did not have, every object +# of that category that ALREADY EXISTED in OLD needs an explicit GRANT in the +# update script; only objects created after the ADP runs inherit it. That is a +# real bug in this tree's history and the acceptance test for this rule: +# --versions 0.2.1 0.2.2 flags the five enum types created in 0.2.0/0.2.1 that +# never got GRANT USAGE. +# +# Narrow on purpose: only `IN SCHEMA GRANT

ON TYPES TO ` is +# understood, because TABLES/FUNCTIONS/SEQUENCES would each need their own +# "what creates one of these" pattern. Any other ADP form -- FOR ROLE, a +# schema-less global default, a category other than TYPES -- is REPORTED as +# unrecognized rather than skipped, so widening the rule stays a decision +# someone makes deliberately instead of a gap nobody notices. This reproduces +# a known bug shape; it is not a general ADP analysis, and widening it into +# one is the kind of growth the limitations section argues against. +# +# ESCAPE HATCH +# +# A waiver comment in the UPDATE script: +# +# -- update-lint: ok /REGEX/ reason +# +# Any finding whose text matches REGEX is suppressed and the reason printed +# instead -- that text is the statement itself for a missing copy, and the +# message for every other rule, so /CREATE TYPE cat_tools.object_type/ reaches +# either. The reason is mandatory. +# +# Two rules keep a waiver from outliving its reason, which is how a check like +# this rots. A waiver that matches nothing FAILS rather than being quietly +# ignored -- either the divergence is gone, or the statement changed underneath +# it into something nobody has reviewed. And a line that opens with +# `-- update-lint` but does not parse is a hard error naming the line, never a +# no-op: a typo that silently drops the waiver hands the author a finding they +# believe they already waived, with nothing pointing at why. +# +# Waivers live in the update script because that is the file being reviewed +# and the file the exception is a property of. The intent is a handful per +# release on genuine exceptions, never one per statement. +# +# LIMITATIONS +# +# Each case below is declined, not missing. The rule that decides what belongs +# here is: ask what the right way to verify THIS case is. Where text comparison +# is genuinely easy it is used; where it is awkward, that awkwardness is evidence +# the case belongs to a different mechanism -- a runtime catalog comparison, or +# an explicit test -- and not to a cleverer parser. Growing this script to reach +# one of them would cost far more than it bought. +# +# - THIS COMPARES TEXT, SO A DIVERGENCE THAT IS INVISIBLE IN THE SQL BUT REAL +# IN THE CATALOG IS INVISIBLE HERE. Demonstrated, not hypothetical: defect +# D1 in https://github.com/Postgres-Extensions/cat_tools/issues/93 had a +# fresh and an updated install agreeing on every ACL and every +# has_type_privilege answer while differing in five pg_init_privs rows, +# because ALTER DEFAULT PRIVILEGES grants implicitly and an implicit grant +# is never snapshotted -- so the two produced different pg_dump output. +# bin/structural_diff called them identical too. That class needs a +# pg_init_privs or pg_dump comparison; nothing text-level can reach it. +# - ENUM LABEL CHANGES ARE OUT OF SCOPE, because a better mechanism already +# covers them. The pgTAP suite runs the SAME expected output against a fresh +# install, an updated database and a pg_upgraded one (see CLAUDE.md's +# TEST_LOAD_SOURCE), so an install and an update script that disagree about +# an enum's labels make one of those runs fail against the real catalog -- +# stronger than anything this could assert about text. A changed CREATE TYPE +# ... AS ENUM is therefore exempt here rather than waived. Only a type OLD +# already had is exempt; a brand-new enum copies into the update script +# verbatim like any other new object, so the ordinary rule covers it. +# - Removals are counted and listed but never failed on. A DROP does not +# appear in an install script, so a removed statement can only be matched by +# inference, never by copy. +# - Waivers are read line by line, not through the scanner, so the marker is +# live even inside a string or a dollar-quoted body. Writing `-- update-lint` +# anywhere is taken to mean it; waivers are file-scoped, so a directive found +# in an odd place still behaves as written. The sharp edge: such a line that +# does not parse is a hard error naming a line that is not a comment at all. +# Known and left alone -- a second scanner costs more than the case is worth. +# - Any semantics-preserving edit that changes text is a false positive: +# reindenting a statement, reordering two GRANTs, hand-rewriting a helper +# call as the literal CREATE OR REPLACE FUNCTION it expands to, adding an +# overload. The escape hatch is the answer; there is no cheaper one. +# - Substring-against-the-whole-file is loose in the other direction. A +# statement copied into a comment, into a format() template that is never +# executed, or into an `IF false` branch counts as matched. +# - A statement that is present in both installs but whose runtime effect +# changed because something it calls changed is not seen at all. + +use strict; +use warnings; + +my $PROG = 'bin/update_lint_textfirst'; + +# Set from @ARGV below, but declared here so the subs close over them. +my ($sql_dir, $ext) = ('sql', 'cat_tools'); + +sub usage_error { print STDERR "$PROG: $_[0]\n"; exit 2 } + +sub abbrev { my $s = shift; length($s) > 140 ? substr($s, 0, 137) . '...' : $s } + +# --------------------------------------------------------------------------- +# Input +# --------------------------------------------------------------------------- + +# DUPLICATED AT sql.mk -- the sql/%.sql pattern rule's @generated@ sed and the +# _apply_version_seds define. Both sides must stay in step. +# +# sql.mk resolves the @generated@ and `-- SED:` markers on the way to the +# generated .sql; doing the identical substitutions here is what makes them +# inert without special-casing them in the scanner. As in the released files, +# PRIOR TO branches are commented out: every marker in this tree names a +# PostgreSQL below the support floor, so the REQUIRES branch is the one that +# installs everywhere. +# +# The " VERSIONED FILE!" tag sql.mk appends when it copies the base file to a +# versioned one must collapse to the same text, because one @generated@ sits +# inside __cat_tools.create_function()'s dollar-quoted body, where a comment is +# body text the splitter cannot strip. Left distinct, that one marker makes +# every release's copy of that function differ from the base file's for a +# reason that has nothing to do with the extension's contents. +sub preprocess { + my ($text) = @_; + $text =~ s/\@generated\@(?:\s+VERSIONED\s+FILE!)?/-- GENERATED FILE! DO NOT EDIT!/g; + $text =~ s/^(.*)-- SED: PRIOR TO ([^!\n]*)!/-- Not used prior to $2: $1/gm; + return $text; +} + +sub read_file { + my ($path) = @_; + open my $fh, '<', $path or usage_error("cannot read $path: $!"); + usage_error("$path is not a plain file") unless -f $fh || -c $fh; + local $/; + my $t = <$fh>; + close $fh; + return preprocess(defined $t ? $t : ''); +} + +# --------------------------------------------------------------------------- +# Splitter +# +# One left-to-right pass. Comment bodies are replaced by a single space as they +# are consumed, which is both the comment strip and the whitespace collapse +# seed; everything else is copied through verbatim so that quoted text is never +# reinterpreted. A `;` only ends a statement when the pass is not inside a +# quote, a dollar-quote or a comment, which is the entire reason this is a +# scanner and not a split /;/. +# --------------------------------------------------------------------------- + +sub split_statements { + my ($text) = @_; + my (@stmts, $cur); + my ($i, $n, $line, $stmt_line) = (0, length($text), 1, 1); + $cur = ''; + + my $flush = sub { + my $norm = $cur; + $norm =~ s/\s+/ /g; + $norm =~ s/\A | \z//g; + push @stmts, { norm => $norm, line => $stmt_line } if $norm =~ /\S/; + $cur = ''; + }; + + while ($i < $n) { + my $two = substr($text, $i, 2); + + if ($two eq '--') { + my $j = index($text, "\n", $i); + $j = $n if $j < 0; + $i = $j; + $cur .= ' '; + next; + } + if ($two eq '/*') { # PostgreSQL block comments nest + my ($depth, $j) = (1, $i + 2); + while ($j < $n && $depth) { + my $t = substr($text, $j, 2); + if ($t eq '/*') { $depth++; $j += 2 } + elsif ($t eq '*/') { $depth--; $j += 2 } + else { $line++ if substr($text, $j, 1) eq "\n"; $j++ } + } + $i = $j; + $cur .= ' '; + next; + } + + my $c = substr($text, $i, 1); + + if ($c eq q{'} || $c eq q{"}) { + my $j = $i + 1; + while ($j < $n) { + if (substr($text, $j, 1) eq $c) { + last unless substr($text, $j + 1, 1) eq $c; # '' escapes ' + $j += 2; + next; + } + $line++ if substr($text, $j, 1) eq "\n"; + $j++; + } + $cur .= substr($text, $i, $j - $i + 1); + $i = $j + 1; + next; + } + + if ($c eq '$' && substr($text, $i) =~ /\A(\$(?:[A-Za-z_]\w*)?\$)/) { + my $tag = $1; + my $j = index($text, $tag, $i + length $tag); + $j = $n - length($tag) if $j < 0; + my $chunk = substr($text, $i, $j - $i + length $tag); + $line += ($chunk =~ tr/\n//); + $cur .= $chunk; + $i = $j + length $tag; + next; + } + + if ($c eq ';') { + $flush->(); + $i++; + $stmt_line = $line; + next; + } + + $line++ if $c eq "\n"; + $stmt_line = $line if $cur !~ /\S/; # first real char sets the line + $cur .= $c; + $i++; + } + $flush->(); + return @stmts; +} + +# The haystack for rule 4: the whole update file as one normalized string, so a +# statement copied into a DO block, a format() template or an IF branch still +# matches even though it is not a top-level statement over there. +sub normalized_blob { + my ($text) = @_; + return join ' ; ', map { $_->{norm} } split_statements($text); +} + +# --------------------------------------------------------------------------- +# Scaffolding (see header) +# --------------------------------------------------------------------------- + +sub is_scaffolding { + my ($norm) = @_; + return $norm =~ m{\A + (?: CREATE | DROP ) \s+ (?: OR \s+ REPLACE \s+ )? + (?: SCHEMA | FUNCTION | PROCEDURE | TYPE | DOMAIN | TABLE | VIEW ) \s+ + (?: IF \s+ (?: NOT \s+ )? EXISTS \s+ )? + __\Q$ext\E \b + }xi; +} + +# --------------------------------------------------------------------------- +# Enum labels (see header) +# --------------------------------------------------------------------------- + +sub enum_type_name { + my ($norm) = @_; + return $norm =~ /\ACREATE\s+TYPE\s+(\S+)\s+AS\s+ENUM\b/i ? lc $1 : undef; +} + +# --------------------------------------------------------------------------- +# Waivers (see header) +# --------------------------------------------------------------------------- + +sub parse_waivers { + my ($text, $path) = @_; + my @w; + my $lineno = 0; + for my $line (split /\n/, $text) { + $lineno++; + next unless $line =~ m{--\s*update-lint\b(.*)\z}; + my $rest = $1; + # Opening with the marker is enough to mean it: anything past that which + # does not parse is an error, never a no-op (see header). + # + # Whitespace, not the first `/`, is what closes the regex -- otherwise a + # regex containing a `/` (an operator, a character class, a path) would + # truncate at it and silently waive something narrower than written. + usage_error("$path:$lineno: malformed waiver, expected " + . "`-- update-lint: ok /REGEX/ reason`:\n $line") + unless $rest =~ m{\A:\s*ok\s+/(.+?)/\s+(\S.*?)\s*\z}; + my ($re, $why) = ($1, $2); + my $qr = eval { qr/$re/i }; + usage_error("$path:$lineno: bad regex in waiver: /$re/") unless $qr; + push @w, { re => $qr, src => $re, why => $why, used => 0 }; + } + return @w; +} + +sub waive { + my ($waivers, $subject) = @_; + for my $w (@$waivers) { + next unless $subject =~ $w->{re}; + $w->{used}++; + return $w; + } + return undef; +} + +# --------------------------------------------------------------------------- +# ALTER DEFAULT PRIVILEGES (see header) +# +# Only the TYPES category is written. TABLES/FUNCTIONS/SEQUENCES/SCHEMAS would +# each need their own "what creates one of these" pattern, so any other form is +# reported as unrecognized instead. +# --------------------------------------------------------------------------- + +sub adp_gaps { + my ($old, $new, $update_blob) = @_; + my %in_old = map { $_->{norm} => 1 } @$old; + my @gaps; + + for my $st (@$new) { + next if $in_old{ $st->{norm} }; # already in force before OLD's objects were made + next unless $st->{norm} =~ /\AALTER\s+DEFAULT\s+PRIVILEGES\b/i; + + # /x ignores the layout whitespace below, so nothing here is a literal + # space; every gap between keywords is an explicit \s+, matching the + # single spaces split_statements already collapsed the statement to. + unless ($st->{norm} =~ /\A + ALTER \s+ DEFAULT \s+ PRIVILEGES \s+ + IN \s+ SCHEMA \s+ (\S+) \s+ + GRANT \s+ (.+?) \s+ ON \s+ TYPES \s+ TO \s+ (\S+) + /xi) { + # Report rather than skip. An ADP shape nobody taught this rule is + # exactly the case the rule exists for, so passing it silently is + # the failure it was written to prevent; waive it if it is fine. + push @gaps, "unrecognized ALTER DEFAULT PRIVILEGES form -- only " + . "`IN SCHEMA GRANT

ON TYPES TO ` is checked " + . "(see header):\n " . abbrev($st->{norm}); + next; + } + my ($schema, $privs, $role) = ($1, $2, $3); + + for my $o (@$old) { + next unless $o->{norm} =~ /\ACREATE\s+(TYPE|DOMAIN)\s+(\S+)/i; + my ($what, $name) = (uc $1, $2); + next unless $name =~ /\A\Q$schema\E\./; + next if $update_blob =~ /GRANT\s+\Q$privs\E\s+ON\s+(?:$what\s+)?\Q$name\E\s+TO\s+\Q$role\E/i; + push @gaps, "$what $name predates the new ALTER DEFAULT PRIVILEGES; " + . "update script needs an explicit " + . "GRANT $privs ON $what $name TO $role"; + } + } + return @gaps; +} + +# --------------------------------------------------------------------------- +# Which files to compare +# --------------------------------------------------------------------------- + +# The version sql/.sql.in currently builds. Read rather than hardcoded so a +# release does not have to remember this file. +sub current_version { + my $path = "$ext.control"; + open my $fh, '<', $path or usage_error("cannot read $path: $!"); + local $/; + my ($v) = <$fh> =~ /^\s*default_version\s*=\s*'([^']+)'/m; + usage_error("no default_version in $path") unless defined $v; + return $v; +} + +# A missing file is EMPTY, not skipped: skipping would pass silently on exactly +# the omission this check exists to catch. +sub first_existing { for (@_) { return $_ if -e } return '/dev/null' } + +sub install_script { + my ($v) = @_; + # The current version's own install script is a gitignored copy of the base + # file (see sql/.gitignore), so the base file stands in for it. + return "$sql_dir/$ext.sql.in" if $v eq current_version(); + return first_existing("$sql_dir/$ext--$v.sql.in", "$sql_dir/$ext--$v.sql"); +} + +# The current cycle's pair, derived from the one update script targeting the +# current version. OLD is a glob rather than a second lookup because a version +# can be reachable from several others -- this tree ships both 0.2.0--0.2.2 and +# 0.2.1--0.2.2 -- so the target version alone does not name a file. Exactly one +# is still expected here: a development cycle has a single update script, and +# two would mean the pair being checked is ambiguous. +sub current_pair { + my $cur = current_version(); + # .sql.in first and only: a built tree also holds the .sql generated from + # it, and matching both would make this ambiguous exactly when someone has + # run make. + my @acc = glob "$sql_dir/$ext--*--$cur.sql.in"; + @acc = glob "$sql_dir/$ext--*--$cur.sql" unless @acc; + usage_error("expected exactly one $sql_dir/$ext--*--$cur.sql.in, found " . scalar @acc) + unless @acc == 1 && $acc[0] =~ m{\Q$ext\E--(.+)--\Q$cur\E\.sql(?:\.in)?\z}; + return ($1, $cur); +} + +# --------------------------------------------------------------------------- +# Main +# --------------------------------------------------------------------------- + +my (@versions, @paths); +while (@ARGV) { + my $a = shift @ARGV; + if ($a eq '--sql-dir') { $sql_dir = shift @ARGV // usage_error('--sql-dir needs a value') } + elsif ($a eq '--ext') { $ext = shift @ARGV // usage_error('--ext needs a value') } + elsif ($a eq '--versions') { @versions = splice @ARGV, 0, 2 } + elsif ($a =~ /\A-/) { usage_error("unknown option $a") } + else { push @paths, $a } +} + +@versions = current_pair() unless @versions || @paths; + +if (@versions) { + usage_error('--versions needs OLD and NEW') unless @versions == 2; + usage_error('--versions and positional paths are exclusive') if @paths; + my ($o, $n) = @versions; + @paths = ( + install_script($o), + install_script($n), + first_existing("$sql_dir/$ext--$o--$n.sql.in", "$sql_dir/$ext--$o--$n.sql"), + ); +} +usage_error('need three paths, or --versions OLD NEW') unless @paths == 3; + +my ($old_text, $new_text, $upd_text) = map { read_file($_) } @paths; +my @old = split_statements($old_text); +my @new = split_statements($new_text); +my $blob = normalized_blob($upd_text); +my @waivers = parse_waivers($upd_text, $paths[2]); + +my %old_enum; +for my $st (@old) { + my $name = enum_type_name($st->{norm}); + $old_enum{$name} = 1 if defined $name; +} + +my %old_count; +$old_count{ $_->{norm} }++ for @old; +my %new_count; +$new_count{ $_->{norm} }++ for @new; + +# Multiset, not set: two identical statements in NEW against one in OLD is one +# addition. The budget hash is what makes the second copy stop counting. +my (@added, @removed, %budget); +%budget = map { $_ => $new_count{$_} - ($old_count{$_} // 0) } keys %new_count; +push @added, $_ for grep { $budget{ $_->{norm} }-- > 0 } @new; +%budget = map { $_ => $old_count{$_} - ($new_count{$_} // 0) } keys %old_count; +push @removed, $_ for grep { $budget{ $_->{norm} }-- > 0 } @old; + +my (@fail, @waived); +my ($matched, $scaffolding, $enum) = (0, 0, 0); + +# $subject is what a waiver regex sees, $detail is what gets printed. For a +# missing copy the subject is the bare statement, so a waiver can name the +# object rather than this script's prose around it. +my $finding = sub { + my ($where, $subject, $detail) = @_; + if (my $w = waive(\@waivers, $subject)) { push @waived, "$where: $w->{why}"; return } + push @fail, "$where: $detail"; +}; + +for my $st (@added) { + my $where = "$paths[1]:$st->{line}"; + + if (index($blob, $st->{norm}) >= 0) { $matched++; next } + if (is_scaffolding($st->{norm})) { $scaffolding++; next } + + my $enum_name = enum_type_name($st->{norm}); + if (defined $enum_name && $old_enum{$enum_name}) { $enum++; next } + + $finding->($where, $st->{norm}, + "added statement has no copy in $paths[2]\n " . abbrev($st->{norm})); +} + +$finding->($paths[2], $_, $_) for adp_gaps(\@old, \@new, $blob); + +# A waiver that matches nothing is a finding in its own right: either the +# divergence it was written for is gone, or the statement changed underneath it +# into something that has not been reviewed. +push @fail, "$paths[2]: stale waiver /$_->{src}/ matches nothing -- $_->{why}" + for grep { !$_->{used} } @waivers; + +printf "%s\n old %s (%d statements)\n new %s (%d statements)\n update %s\n", + $PROG, $paths[0], scalar @old, $paths[1], scalar @new, $paths[2]; +printf " added %d (matched %d, scaffolding %d, enum %d, unmatched %d)\n", + scalar @added, $matched, $scaffolding, $enum, + scalar @added - $matched - $scaffolding - $enum; +printf " removed %d (not checked -- see header)\n", scalar @removed; +print " waived $_\n" for @waived; + +if (@fail) { + print "\n$_\n" for @fail; + printf STDERR "\n%s: FAIL, %d finding(s)\n", $PROG, scalar @fail; + exit 1; +} +print " OK\n"; +exit 0; + +# vi: expandtab ts=4 sw=4 diff --git a/sql.mk b/sql.mk index 17e8b99..38bc185 100644 --- a/sql.mk +++ b/sql.mk @@ -180,6 +180,9 @@ else _sql_sed_93 = pgxntool/safesed $@.tmp -E -e 's/(.*)-- SED: PRIOR TO 9\.3!/-- Not used prior to 9.3: \1/' endif +# DUPLICATED AT bin/update_lint_textfirst's preprocess(), which reads .sql.in +# directly and has to reach the same text this produces. +# # Apply all version-conditional SED markers to $@.tmp: 9.x via the safesed vars # above; 10+ generically via awk (REQUIRES N -> commented if MAJORVER < N*10; # PRIOR TO N -> commented if MAJORVER >= N*10). POSIX awk only (no gawk @@ -222,6 +225,9 @@ endef # TODO: refactor the version handling into a function. # ---------------------------------------------------------------------------- +# DUPLICATED AT bin/update_lint_textfirst's preprocess() (this recipe's +# @generated@ sed, and the " VERSIONED FILE!" tag added by the rule below it). +# # @generated@ becomes the "-- GENERATED FILE! DO NOT EDIT!" marker below via a # plain, unanchored substring match -- it also fires on a handful of # coincidental @generated@ occurrences inside real-code comments in diff --git a/sql/cat_tools.sql.in b/sql/cat_tools.sql.in index 339ff00..7eaa88a 100644 --- a/sql/cat_tools.sql.in +++ b/sql/cat_tools.sql.in @@ -775,8 +775,10 @@ CREATE TYPE cat_tools.object_type AS ENUM( , 'materialized view column' , 'composite type column' , 'foreign table column' - -- pg_constraint - -- NOTE: a domain itself is considered to be a type + /* + * pg_constraint + * NOTE: a domain itself is considered to be a type + */ , 'domain constraint', 'table constraint' -- pg_proc , 'aggregate', 'function' @@ -1281,8 +1283,10 @@ $$ @generated@ --- Borrowed from newsysviews: http://pgfoundry.org/projects/newsysviews/ --- sql-lint:disable-block all: copied verbatim from newsysviews, do not reformat +/* + * Borrowed from newsysviews: http://pgfoundry.org/projects/newsysviews/ + * sql-lint:disable-block all: copied verbatim from newsysviews, do not reformat + */ CREATE OR REPLACE VIEW cat_tools.pg_all_foreign_keys AS SELECT n1.nspname AS fk_schema_name,