From 63ade8df4962086e713fb26729fa2bf27cdcf11c Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Wed, 9 Sep 2026 17:32:21 -0500 Subject: [PATCH 1/7] Prototype: bin/update_lint_textfirst, a text-first update-script checker INCOMPLETE PROTOTYPE opened for comparison against https://github.com/Postgres-Extensions/cat_tools/pull/90, not a merge candidate. Nothing is wired into the Makefile or CI. Instead of parsing install scripts into semantic object keys, split both sides into top-level statements, normalize (strip comments, collapse whitespace), multiset-diff them, and require every added statement to appear as a substring of the normalized update script. Update scripts are overwhelmingly copy-paste from the install script, so most of the work the object model does is not needed to catch the omission. Implements one gap for real -- ALTER DEFAULT PRIVILEGES is not retroactive, so an ADP newly added in NEW leaves every same-category object that already existed in OLD needing an explicit GRANT. That flags the five enum types this tree never granted USAGE on. Enum values, new columns, changed view column lists and removals are stubbed and documented as such, and the header lists the false-positive sources. The escape hatch is a `-- update-lint: ok /REGEX/ reason` comment in the update script; unused waivers are reported so they cannot rot. Co-Authored-By: Claude Opus 5 (1M context) --- bin/test/textfirst.t | 114 ++++++++++++ bin/update_lint_textfirst | 367 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 481 insertions(+) create mode 100644 bin/test/textfirst.t create mode 100755 bin/update_lint_textfirst diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t new file mode 100644 index 0000000..272a12b --- /dev/null +++ b/bin/test/textfirst.t @@ -0,0 +1,114 @@ +#!/usr/bin/env perl +# +# Prototype-sized test set for bin/update_lint_textfirst: one case per idea the +# sketch is trying to demonstrate, not coverage. Run from the repo root: +# +# prove bin/test/textfirst.t + +use strict; +use warnings; +use Test::More tests => 12; +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, unmatched 0\)/, '... 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'; +} + +# --- 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, 0, 'an unused waiver does not fail'; + like $out, qr{UNUSED WAIVER /nothing matches this/}, '... but is reported so it cannot rot'; +} + +# --- ALTER DEFAULT PRIVILEGES, on the real historical bug ------------------- + +SKIP: { + skip 'run from the repo root', 2 unless -e 'sql/cat_tools--0.2.1.sql.in'; + + my $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..899dd3c --- /dev/null +++ b/bin/update_lint_textfirst @@ -0,0 +1,367 @@ +#!/usr/bin/env perl +# +# update_lint_textfirst - INCOMPLETE PROTOTYPE. Do not merge, do not wire into +# CI. It exists only to show, concretely, what a text-first update-script +# checker looks like next to the object-model one in +# https://github.com/Postgres-Extensions/cat_tools/pull/90. Several real cases +# are stubbed, and it has known false positives (both listed below). +# +# The problem is the same one bin/update_lint states: a PR that changes an +# extension's SQL must also extend sql/----.sql.in so an +# existing install reaches the same objects, and nothing fails when that is +# forgotten until a runtime check that needs a database and minutes of CI. +# +# The principle here is different, and it is the whole point of the sketch: +# 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/removed 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 unmatched is reported; exit non-zero. +# +# That works because update scripts are overwhelmingly copy-paste from the +# install script: measured on this tree's own history, 85% of added statements +# match verbatim. The interesting content of the tool is therefore not the +# matcher, it is the list of cases where a copy is impossible -- and the escape +# hatch for the rest. +# +# USAGE: +# bin/update_lint_textfirst --versions OLD NEW +# bin/update_lint_textfirst OLD_INSTALL NEW_INSTALL UPDATE_SCRIPT +# Options: --sql-dir DIR (default "sql"), --ext NAME (default "cat_tools"). +# Exit: 0 clean, 1 gaps found, 2 usage error. +# +# ESCAPE HATCH (implemented; it is central to the design) +# +# A waiver comment in the UPDATE script: +# +# -- update-lint: ok /REGEX/ reason +# +# Any finding whose text matches REGEX is suppressed, and the reason is +# printed instead. Reason is mandatory. Waivers that match nothing are +# reported, so they cannot rot silently. The design intent is that a release +# carries a handful of these on genuine exceptions -- not one per statement. +# They live in the update script because that is the file being reviewed and +# the file the exception is a property of. +# +# IMPLEMENTED GAP: 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. +# This is a real bug in this tree's history, which is the acceptance test: +# --versions 0.2.1 0.2.2 flags five enum types created in 0.2.0/0.2.1 that +# never got GRANT USAGE. Only the TYPES category is implemented. +# +# STUBBED -- these are the other cases where the update script legitimately +# cannot hold a copy of the install statement, and this prototype does not +# handle any of them (each is a false positive today, waivable by hand): +# +# - Enum values. Install says CREATE TYPE ... AS ENUM ('a','b','c'); the +# update must say ALTER TYPE ... ADD VALUE 'c'. Needs a label-set diff. +# - New columns. Install states the final CREATE TABLE column list; the +# update needs ALTER TABLE ... ADD COLUMN. +# - Changed view column list. Install uses CREATE OR REPLACE VIEW; the update +# must DROP VIEW + CREATE VIEW, because REPLACE cannot drop a column. +# - Removals. A DROP never appears in an install script, so a removed +# statement can only ever be matched by inference, never by copy. Removed +# statements are counted and listed but NOT failed on. +# +# KNOWN FALSE POSITIVE SOURCES +# +# - Scaffolding bound to old names. The update script's private copy of +# __cat_tools.create_function() calls a differently-named helper than the +# install's copy, so the text legitimately differs. This fires on the +# current dev pair today. +# - Any hand-reformatting. Two helper calls rewritten as literal CREATE OR +# REPLACE FUNCTION make 0.2.2->0.2.3 report 2 of 2 added unmatched, even +# though the update is correct. Text-first cannot tell that from a real +# omission; the escape hatch is the only answer. +# - Function overloads, argument reordering, and any semantics-preserving +# edit are all invisible to text matching in the same way. + +use strict; +use warnings; + +my $PROG = 'bin/update_lint_textfirst'; + +# --------------------------------------------------------------------------- +# Input +# --------------------------------------------------------------------------- + +# sql.mk rewrites the bare @generated@ marker into a -- comment on its way to +# the generated .sql, and resolves the version-conditional `-- SED:` markers. +# Doing the identical substitutions here is what makes them inert without +# special-casing them in the scanner. As in bin/update_lint, 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. +sub preprocess { + my ($text) = @_; + $text =~ s/\@generated\@/-- 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); +} + +# --------------------------------------------------------------------------- +# Waivers +# --------------------------------------------------------------------------- + +sub parse_waivers { + my ($text) = @_; + my @w; + for my $line (split /\n/, $text) { + next unless $line =~ m{--\s*update-lint:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z}; + my ($re, $why) = ($1, $2); + my $qr = eval { qr/$re/i }; + usage_error("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 +# +# The one gap this prototype implements. See the header for why a copy of the +# ADP statement in the update script does not cover objects that predate it. +# Only the TYPES category is handled; TABLES/FUNCTIONS/SEQUENCES/SCHEMAS would +# each need their own "what creates one of these" pattern and are not written. +# --------------------------------------------------------------------------- + +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} =~ /\A + ALTER \s+ DEFAULT \s+ PRIVILEGES \s+ + IN \s+ SCHEMA \s+ (\S+) \s+ + GRANT \s+ (.+?) \s+ ON \s+ TYPES \s+ TO \s+ (\S+) + /xi; + 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; +} + +# --------------------------------------------------------------------------- +# Main +# --------------------------------------------------------------------------- + +sub usage_error { print STDERR "$PROG: $_[0]\n"; exit 2 } + +my ($sql_dir, $ext, @versions, @paths) = ('sql', 'cat_tools'); +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 } +} + +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; + # A missing update script is EMPTY, not skipped: skipping would pass + # silently on exactly the omission this check exists to catch. + for my $stem ("$ext--$o", "$ext--$n", "$ext--$o--$n") { + my $p = "$sql_dir/$stem.sql.in"; + $p = "$sql_dir/$stem.sql" unless -e $p; + push @paths, -e $p ? $p : '/dev/null'; + } +} +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); + +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 = 0; +for my $st (@added) { + if (index($blob, $st->{norm}) >= 0) { $matched++; next } + if (my $w = waive(\@waivers, $st->{norm})) { + push @waived, "$paths[1]:$st->{line}: $w->{why}"; + next; + } + push @fail, "$paths[1]:$st->{line}: added statement has no copy in $paths[2]\n" + . ' ' . abbrev($st->{norm}); +} + +for my $g (adp_gaps(\@old, \@new, $blob)) { + if (my $w = waive(\@waivers, $g)) { push @waived, "ADP: $w->{why}"; next } + push @fail, "$paths[2]: $g"; +} + +sub abbrev { my $s = shift; length($s) > 140 ? substr($s, 0, 137) . '...' : $s } + +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, unmatched %d), removed %d (not checked -- see header)\n", + scalar @added, $matched, scalar @added - $matched, scalar @removed; +print " waived $_\n" for @waived; +print " UNUSED WAIVER /$_->{src}/ -- $_->{why}\n" for grep { !$_->{used} } @waivers; + +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; From d9a7f01cdf4d491662c73838c7f151b2c5a35f91 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 11 Sep 2026 17:01:20 -0500 Subject: [PATCH 2/7] Finish the text-first update-script checker and wire it into CI bin/update_lint_textfirst now runs with no arguments against the only pair that is still editable -- the last released install script against sql/cat_tools.sql.in, with sql/cat_tools----stable.sql.in as the update script -- deriving both version numbers so a release needs no edit here. `make update-lint` drives it from a new step in CI's lint job, alongside a step running the checker's own tests. Four changes make that pair clean and keep it meaningful: - sql.mk tags the versioned copy's @generated@ markers with "VERSIONED FILE!", and one of those markers sits inside __cat_tools.create_function()'s dollar-quoted body where no comment strip can reach it. Preprocessing now collapses both forms to the same text; left alone it made every release's copy of that function differ from the base file's. - Statements defining an object in the __cat_tools schema are exempt. That schema is dropped before the install script ends, so nothing in it can leave an updated database short an object. Calls to it are still checked. - A changed CREATE TYPE ... AS ENUM is paired with its old form and checked label by label against ALTER TYPE ... ADD VALUE. One added label is exactly the small, easy-to-miss diff this exists to catch, and it can never match by copy. - A waiver that matches nothing now fails rather than being reported and ignored: a waiver that outlives its reason is how a check like this rots. The header documents what this cannot see, led by the class that matters most: it compares statement text, so a divergence real in the catalog but invisible in the SQL -- pg_init_privs rows that ALTER DEFAULT PRIVILEGES never snapshots, as in defect D1 of https://github.com/Postgres-Extensions/cat_tools/issues/93 -- is out of reach of any text check. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 11 ++ CLAUDE.md | 7 + Makefile | 13 ++ bin/test/textfirst.t | 102 ++++++++++- bin/update_lint_textfirst | 363 ++++++++++++++++++++++++++++---------- 5 files changed, 391 insertions(+), 105 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9e0d9a2..b07e782 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -404,6 +404,17 @@ jobs: # wrapper that swallows that exit code or calls a different entry point # would defeat this safety net. run: make lint + - name: Test bin/update_lint_textfirst + # Run before the check below, so a bug in the checker is reported as a + # bug in the checker rather than as a finding against the SQL. + # Test::Harness is core Perl, so `prove` needs no setup. + run: prove bin/test/textfirst.t + - 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/CLAUDE.md b/CLAUDE.md index bbe9480..65cd85f 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 accumulator + 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..572c148 100644 --- a/Makefile +++ b/Makefile @@ -105,3 +105,16 @@ 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 diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t index 272a12b..350be24 100644 --- a/bin/test/textfirst.t +++ b/bin/test/textfirst.t @@ -1,13 +1,16 @@ #!/usr/bin/env perl # -# Prototype-sized test set for bin/update_lint_textfirst: one case per idea the -# sketch is trying to demonstrate, not coverage. Run from the repo root: +# One case per rule bin/update_lint_textfirst implements, plus the two things +# only the real tree can prove: that the current development pair is clean, and +# that the ALTER DEFAULT PRIVILEGES rule reproduces the historical bug it was +# written for. 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 => 12; +use Test::More tests => 27; use File::Temp qw(tempdir); my $PROG = 'bin/update_lint_textfirst'; @@ -60,7 +63,7 @@ SQL 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, unmatched 0\)/, '... and is counted as matched'; + like $out, qr/added 1 \(matched 1,/, '... and is counted as matched'; } { @@ -73,6 +76,73 @@ SQL like $out, qr/CREATE TABLE t \(a int\)/, '... and is named in the finding'; } +# --- enum labels ------------------------------------------------------------ + +{ + 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 => "ALTER TYPE e ADD VALUE 'c';\n", + ); + is $rc, 0, 'an added enum label covered by ALTER TYPE ... ADD VALUE is clean'; + like $out, qr/enum 1,/, '... and is counted as an enum pairing, not a copy'; +} + +{ + # BEFORE/AFTER is how a label lands anywhere but the end, and says nothing + # about whether the label is present. + my ($rc) = run_trio( + old => "CREATE TYPE e AS ENUM ('a', 'c');\n", + new => "CREATE TYPE e AS ENUM ('a', 'b', 'c');\n", + update => "ALTER TYPE e ADD VALUE 'b' BEFORE 'c';\n", + ); + is $rc, 0, 'an ADD VALUE with a BEFORE clause still counts'; +} + +{ + 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 => "ALTER TYPE e ADD VALUE 'b';\n", + ); + is $rc, 1, 'an added enum label with no ALTER TYPE fails'; + like $out, qr/enum e gained label 'c'/, '... naming the label, not the whole type'; +} + +{ + my ($rc, $out) = run_trio( + old => "CREATE TYPE e AS ENUM ('a', 'b');\n", + new => "CREATE TYPE e AS ENUM ('a');\n", + update => "SELECT 1;\n", + ); + is $rc, 1, 'a removed enum label fails'; + like $out, qr/cannot be removed by an update script/, '... saying no update can do it'; +} + +# --- 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 ------------------------------------------------------- { @@ -92,16 +162,30 @@ SQL new => "SELECT 1;\n", update => "-- update-lint: ok /nothing matches this/ stale\n", ); - is $rc, 0, 'an unused waiver does not fail'; - like $out, qr{UNUSED WAIVER /nothing matches this/}, '... but is reported so it cannot rot'; + is $rc, 1, 'a waiver that matches nothing fails'; + like $out, qr{stale waiver /nothing matches this/}, '... and says which one'; } -# --- ALTER DEFAULT PRIVILEGES, on the real historical bug ------------------- +# --- against the real tree -------------------------------------------------- SKIP: { - skip 'run from the repo root', 2 unless -e 'sql/cat_tools--0.2.1.sql.in'; + 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 the accumulator 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'; - my $out = qx{$^X $PROG --versions 0.2.1 0.2.2 2>&1}; + $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 diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index 899dd3c..1001c88 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -1,110 +1,163 @@ #!/usr/bin/env perl # -# update_lint_textfirst - INCOMPLETE PROTOTYPE. Do not merge, do not wire into -# CI. It exists only to show, concretely, what a text-first update-script -# checker looks like next to the object-model one in -# https://github.com/Postgres-Extensions/cat_tools/pull/90. Several real cases -# are stubbed, and it has known false positives (both listed below). +# update_lint_textfirst - catch an install-script change whose update script +# forgot to keep up, statically: no database, no SQL parser, seconds not +# minutes. # -# The problem is the same one bin/update_lint states: a PR that changes an -# extension's SQL must also extend sql/----.sql.in so an -# existing install reaches the same objects, and nothing fails when that is -# forgotten until a runtime check that needs a database and minutes of CI. +# 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. # -# The principle here is different, and it is the whole point of the sketch: -# never build a model of what an object IS. Compare STATEMENT TEXT. +# 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 accumulator every SQL-touching +# PR extends. Both version numbers are derived -- the current one from +# .control's default_version, the previous one from the single accumulator +# file 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/removed statement must appear, normalized, SOMEWHERE in the +# 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 unmatched is reported; exit non-zero. +# 5. Anything left over is a finding; exit non-zero. # # That works because update scripts are overwhelmingly copy-paste from the -# install script: measured on this tree's own history, 85% of added statements -# match verbatim. The interesting content of the tool is therefore not the -# matcher, it is the list of cases where a copy is impossible -- and the escape -# hatch for the rest. +# install script. Aim it at the small, subtle diffs -- a changed function body, +# one added enum label, a changed GRANT. 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. # -# USAGE: -# bin/update_lint_textfirst --versions OLD NEW -# bin/update_lint_textfirst OLD_INSTALL NEW_INSTALL UPDATE_SCRIPT -# Options: --sql-dir DIR (default "sql"), --ext NAME (default "cat_tools"). -# Exit: 0 clean, 1 gaps found, 2 usage error. +# 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. # -# ESCAPE HATCH (implemented; it is central to the design) +# ENUM LABELS # -# A waiver comment in the UPDATE script: +# The one case worth real code, because it is exactly the small diff this +# exists to catch and it can never match by copy: the install script states +# the final label list in CREATE TYPE ... AS ENUM, while the update script +# must say ALTER TYPE ... ADD VALUE for each label the previous version +# lacked. A changed CREATE TYPE whose type also existed in OLD is paired with +# its old form and checked label by label. Label ORDER is not checked -- see +# the limitations below. # -# -- update-lint: ok /REGEX/ reason +# SCAFFOLDING # -# Any finding whose text matches REGEX is suppressed, and the reason is -# printed instead. Reason is mandatory. Waivers that match nothing are -# reported, so they cannot rot silently. The design intent is that a release -# carries a handful of these on genuine exceptions -- not one per statement. -# They live in the update script because that is the file being reviewed and -# the file the exception is a property of. +# 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. # -# IMPLEMENTED GAP: ALTER DEFAULT PRIVILEGES +# 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. -# This is a real bug in this tree's history, which is the acceptance test: -# --versions 0.2.1 0.2.2 flags five enum types created in 0.2.0/0.2.1 that +# 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. Only the TYPES category is implemented. # -# STUBBED -- these are the other cases where the update script legitimately -# cannot hold a copy of the install statement, and this prototype does not -# handle any of them (each is a false positive today, waivable by hand): +# ESCAPE HATCH +# +# A waiver comment in the UPDATE script: # -# - Enum values. Install says CREATE TYPE ... AS ENUM ('a','b','c'); the -# update must say ALTER TYPE ... ADD VALUE 'c'. Needs a label-set diff. -# - New columns. Install states the final CREATE TABLE column list; the -# update needs ALTER TABLE ... ADD COLUMN. -# - Changed view column list. Install uses CREATE OR REPLACE VIEW; the update -# must DROP VIEW + CREATE VIEW, because REPLACE cannot drop a column. -# - Removals. A DROP never appears in an install script, so a removed -# statement can only ever be matched by inference, never by copy. Removed -# statements are counted and listed but NOT failed on. +# -- update-lint: ok /REGEX/ reason # -# KNOWN FALSE POSITIVE SOURCES +# 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. A waiver that matches nothing FAILS rather +# than being quietly ignored, because a waiver that outlives its reason is how +# a check like this rots -- and because the statement it was written for may +# have changed underneath it into something that now needs review. # -# - Scaffolding bound to old names. The update script's private copy of -# __cat_tools.create_function() calls a differently-named helper than the -# install's copy, so the text legitimately differs. This fires on the -# current dev pair today. -# - Any hand-reformatting. Two helper calls rewritten as literal CREATE OR -# REPLACE FUNCTION make 0.2.2->0.2.3 report 2 of 2 added unmatched, even -# though the update is correct. Text-first cannot tell that from a real -# omission; the escape hatch is the only answer. -# - Function overloads, argument reordering, and any semantics-preserving -# edit are all invisible to text matching in the same way. +# 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 is a deliberate choice to stay small, not an oversight +# +# - 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. +# - 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. +# - Enum label ORDER is not checked, only the set. ALTER TYPE ... ADD VALUE +# takes an optional BEFORE/AFTER, so verifying the resulting sort order +# means modelling insert positions -- more machinery 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 # --------------------------------------------------------------------------- -# sql.mk rewrites the bare @generated@ marker into a -- comment on its way to -# the generated .sql, and resolves the version-conditional `-- SED:` markers. -# Doing the identical substitutions here is what makes them inert without -# special-casing them in the scanner. As in bin/update_lint, 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. +# 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\@/-- GENERATED FILE! DO NOT EDIT!/g; + $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; } @@ -221,7 +274,62 @@ sub normalized_blob { } # --------------------------------------------------------------------------- -# Waivers +# 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) +# --------------------------------------------------------------------------- + +# Returns (type name, [labels]) for a CREATE TYPE ... AS ENUM, else nothing. +# Comments are already gone by normalization time, so every remaining +# single-quoted run inside the parens is a label. +sub enum_labels { + my ($norm) = @_; + return unless $norm =~ /\ACREATE\s+TYPE\s+(\S+)\s+AS\s+ENUM\s*\((.*)\)\z/is; + my ($name, $body) = ($1, $2); + my @labels = $body =~ /'((?:[^']|'')*)'/g; + return ($name, \@labels); +} + +# Returns undef when $norm is not an enum whose type also existed in OLD (so +# the plain copy rule applies), otherwise the list of reasons it is not +# covered -- empty when it is. +sub enum_gaps { + my ($norm, $old_enum, $blob) = @_; + + my ($name, $new_labels) = enum_labels($norm); + return undef unless defined $name; + my $old_labels = $old_enum->{ lc $name } or return undef; + + my %new_has = map { $_ => 1 } @$new_labels; + my @dropped = grep { !$new_has{$_} } @$old_labels; + # No update script can remove an enum label, so say so rather than pretend + # some ALTER TYPE would have covered it. + return ["enum $name dropped label(s) " . join(', ', map {"'$_'"} @dropped) + . '; an enum label cannot be removed by an update script'] + if @dropped; + + my %old_has = map { $_ => 1 } @$old_labels; + return [ map { "enum $name gained label '$_'; update script needs " + . "ALTER TYPE $name ADD VALUE '$_'" } + grep { $blob !~ /ALTER\s+TYPE\s+\Q$name\E\s+ADD\s+VALUE\s+ + (?:IF\s+NOT\s+EXISTS\s+)? '\Q$_\E'/xi } + grep { !$old_has{$_} } @$new_labels ]; +} + +# --------------------------------------------------------------------------- +# Waivers (see header) # --------------------------------------------------------------------------- sub parse_waivers { @@ -248,12 +356,10 @@ sub waive { } # --------------------------------------------------------------------------- -# ALTER DEFAULT PRIVILEGES +# ALTER DEFAULT PRIVILEGES (see header) # -# The one gap this prototype implements. See the header for why a copy of the -# ADP statement in the update script does not cover objects that predate it. -# Only the TYPES category is handled; TABLES/FUNCTIONS/SEQUENCES/SCHEMAS would -# each need their own "what creates one of these" pattern and are not written. +# Only the TYPES category is written. TABLES/FUNCTIONS/SEQUENCES/SCHEMAS would +# each need their own "what creates one of these" pattern. # --------------------------------------------------------------------------- sub adp_gaps { @@ -284,12 +390,51 @@ sub adp_gaps { } # --------------------------------------------------------------------------- -# Main +# Which files to compare # --------------------------------------------------------------------------- -sub usage_error { print STDERR "$PROG: $_[0]\n"; exit 2 } +# 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; +} -my ($sql_dir, $ext, @versions, @paths) = ('sql', 'cat_tools'); +# 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 accumulator update script +# targeting the current version. +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') } @@ -299,17 +444,17 @@ while (@ARGV) { 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; - # A missing update script is EMPTY, not skipped: skipping would pass - # silently on exactly the omission this check exists to catch. - for my $stem ("$ext--$o", "$ext--$n", "$ext--$o--$n") { - my $p = "$sql_dir/$stem.sql.in"; - $p = "$sql_dir/$stem.sql" unless -e $p; - push @paths, -e $p ? $p : '/dev/null'; - } + @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; @@ -319,6 +464,12 @@ my @new = split_statements($new_text); my $blob = normalized_blob($upd_text); my @waivers = parse_waivers($upd_text); +my %old_enum; +for my $st (@old) { + my ($name, $labels) = enum_labels($st->{norm}); + $old_enum{ lc $name } = $labels if defined $name; +} + my %old_count; $old_count{ $_->{norm} }++ for @old; my %new_count; @@ -333,30 +484,48 @@ push @added, $_ for grep { $budget{ $_->{norm} }-- > 0 } @new; push @removed, $_ for grep { $budget{ $_->{norm} }-- > 0 } @old; my (@fail, @waived); -my $matched = 0; +my ($matched, $scaffolding, $enum_ok) = (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 (my $w = waive(\@waivers, $st->{norm})) { - push @waived, "$paths[1]:$st->{line}: $w->{why}"; + if (is_scaffolding($st->{norm})) { $scaffolding++; next } + + if (my $gaps = enum_gaps($st->{norm}, \%old_enum, $blob)) { + $enum_ok++ unless @$gaps; + $finding->($where, $_, $_) for @$gaps; next; } - push @fail, "$paths[1]:$st->{line}: added statement has no copy in $paths[2]\n" - . ' ' . abbrev($st->{norm}); -} -for my $g (adp_gaps(\@old, \@new, $blob)) { - if (my $w = waive(\@waivers, $g)) { push @waived, "ADP: $w->{why}"; next } - push @fail, "$paths[2]: $g"; + $finding->($where, $st->{norm}, + "added statement has no copy in $paths[2]\n " . abbrev($st->{norm})); } -sub abbrev { my $s = shift; length($s) > 140 ? substr($s, 0, 137) . '...' : $s } +$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, unmatched %d), removed %d (not checked -- see header)\n", - scalar @added, $matched, scalar @added - $matched, scalar @removed; +printf " added %d (matched %d, scaffolding %d, enum %d, unmatched %d)\n", + scalar @added, $matched, $scaffolding, $enum_ok, + scalar @added - $matched - $scaffolding - $enum_ok; +printf " removed %d (not checked -- see header)\n", scalar @removed; print " waived $_\n" for @waived; -print " UNUSED WAIVER /$_->{src}/ -- $_->{why}\n" for grep { !$_->{used} } @waivers; if (@fail) { print "\n$_\n" for @fail; @@ -365,3 +534,5 @@ if (@fail) { } print " OK\n"; exit 0; + +# vi: expandtab ts=4 sw=4 From 87dbb470c203c4beb21a7041a4221e581d8cce4a Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 11 Sep 2026 18:12:17 -0500 Subject: [PATCH 3/7] Make a malformed waiver a hard error rather than a silent no-op A line opening with `-- update-lint` that does not parse as `-- update-lint: ok /REGEX/ reason` was dropped without a word, so a missing reason or a mistyped keyword left the author looking at a finding they believed they had already waived, with nothing pointing at the typo. Both forms now fail with the file, line number and offending text. Co-Authored-By: Claude Opus 5 (1M context) --- bin/test/textfirst.t | 18 +++++++++++++++++- bin/update_lint_textfirst | 30 ++++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t index 350be24..c718113 100644 --- a/bin/test/textfirst.t +++ b/bin/test/textfirst.t @@ -10,7 +10,7 @@ use strict; use warnings; -use Test::More tests => 27; +use Test::More tests => 31; use File::Temp qw(tempdir); my $PROG = 'bin/update_lint_textfirst'; @@ -166,6 +166,22 @@ SQL like $out, qr{stale waiver /nothing matches this/}, '... and says which one'; } +# 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'; +} + # --- against the real tree -------------------------------------------------- SKIP: { diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index 1001c88..bc55fa6 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -90,10 +90,15 @@ # 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. A waiver that matches nothing FAILS rather -# than being quietly ignored, because a waiver that outlives its reason is how -# a check like this rots -- and because the statement it was written for may -# have changed underneath it into something that now needs review. +# 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 @@ -333,13 +338,22 @@ sub enum_gaps { # --------------------------------------------------------------------------- sub parse_waivers { - my ($text) = @_; + my ($text, $path) = @_; my @w; + my $lineno = 0; for my $line (split /\n/, $text) { - next unless $line =~ m{--\s*update-lint:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z}; + $lineno++; + next unless $line =~ m{--\s*update-lint\b(.*)\z}; + my $rest = $1; + # Anything that opens with the marker but does not parse is an error, + # never a no-op. Silently dropping it hands the author a finding they + # believe they already waived, with nothing pointing at the typo. + 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("bad regex in waiver: /$re/") unless $qr; + usage_error("$path:$lineno: bad regex in waiver: /$re/") unless $qr; push @w, { re => $qr, src => $re, why => $why, used => 0 }; } return @w; @@ -462,7 +476,7 @@ 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); +my @waivers = parse_waivers($upd_text, $paths[2]); my %old_enum; for my $st (@old) { From 52f2d9411e1009f6319ab6fb679d9aba8566ebeb Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 11 Sep 2026 18:14:08 -0500 Subject: [PATCH 4/7] Trim a waiver comment that restated the header verbatim Co-Authored-By: Claude Opus 5 (1M context) --- bin/update_lint_textfirst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index bc55fa6..cc9a543 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -345,9 +345,8 @@ sub parse_waivers { $lineno++; next unless $line =~ m{--\s*update-lint\b(.*)\z}; my $rest = $1; - # Anything that opens with the marker but does not parse is an error, - # never a no-op. Silently dropping it hands the author a finding they - # believe they already waived, with nothing pointing at the typo. + # 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). 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}; From 9f807c8c7250f6d7681573337c84a29e9da60d38 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 11 Sep 2026 18:30:07 -0500 Subject: [PATCH 5/7] Drop enum label checking; the pgTAP suite verifies enums against the catalog Pairing a changed CREATE TYPE ... AS ENUM against ALTER TYPE ... ADD VALUE was the wrong mechanism for a case that is already covered better. The suite runs the same expected output against a fresh install, an updated database and a pg_upgraded one, so an install and an update script that disagree about an enum's labels fail one of those runs against the real catalog -- stronger than any assertion about statement text, and it needs nothing from this script. A changed enum on a type the old version already had is therefore exempt rather than waived: declaring it out of scope and then failing on it every time would have taught people to reach for a waiver. A brand-new enum type stays under the ordinary copy rule, since it copies into the update script verbatim. The limitations section now leads with the rule that decides what belongs there: ask what the right way to verify a given case is, and treat awkwardness under text comparison as evidence the case belongs to a different mechanism rather than to a cleverer parser. Co-Authored-By: Claude Opus 5 (1M context) --- bin/test/textfirst.t | 45 ++++++------------- bin/update_lint_textfirst | 93 ++++++++++++++------------------------- 2 files changed, 48 insertions(+), 90 deletions(-) diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t index c718113..500a293 100644 --- a/bin/test/textfirst.t +++ b/bin/test/textfirst.t @@ -10,7 +10,7 @@ use strict; use warnings; -use Test::More tests => 31; +use Test::More tests => 28; use File::Temp qw(tempdir); my $PROG = 'bin/update_lint_textfirst'; @@ -76,47 +76,30 @@ SQL like $out, qr/CREATE TABLE t \(a int\)/, '... and is named in the finding'; } -# --- enum labels ------------------------------------------------------------ +# --- 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 => "ALTER TYPE e ADD VALUE 'c';\n", - ); - is $rc, 0, 'an added enum label covered by ALTER TYPE ... ADD VALUE is clean'; - like $out, qr/enum 1,/, '... and is counted as an enum pairing, not a copy'; -} - -{ - # BEFORE/AFTER is how a label lands anywhere but the end, and says nothing - # about whether the label is present. - my ($rc) = run_trio( - old => "CREATE TYPE e AS ENUM ('a', 'c');\n", - new => "CREATE TYPE e AS ENUM ('a', 'b', 'c');\n", - update => "ALTER TYPE e ADD VALUE 'b' BEFORE 'c';\n", - ); - is $rc, 0, 'an ADD VALUE with a BEFORE clause still counts'; -} - -{ - 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 => "ALTER TYPE e ADD VALUE 'b';\n", + update => "SELECT 1;\n", ); - is $rc, 1, 'an added enum label with no ALTER TYPE fails'; - like $out, qr/enum e gained label 'c'/, '... naming the label, not the whole type'; + 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 => "CREATE TYPE e AS ENUM ('a', 'b');\n", - new => "CREATE TYPE e AS ENUM ('a');\n", - update => "SELECT 1;\n", + old => "SELECT 1;\n", + new => "SELECT 1;\nCREATE TYPE e AS ENUM ('a');\n", + update => "SELECT 2;\n", ); - is $rc, 1, 'a removed enum label fails'; - like $out, qr/cannot be removed by an update script/, '... saying no update can do it'; + 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 ------------------------------------------------------------ diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index cc9a543..e7fc64a 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -43,22 +43,19 @@ # # That works because update scripts are overwhelmingly copy-paste from the # install script. Aim it at the small, subtle diffs -- a changed function body, -# one added enum label, a changed GRANT. 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. +# 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 +# ENUM LABELS -- out of scope, checked properly elsewhere # -# The one case worth real code, because it is exactly the small diff this -# exists to catch and it can never match by copy: the install script states -# the final label list in CREATE TYPE ... AS ENUM, while the update script -# must say ALTER TYPE ... ADD VALUE for each label the previous version -# lacked. A changed CREATE TYPE whose type also existed in OLD is paired with -# its old form and checked label by label. Label ORDER is not checked -- see -# the limitations below. +# 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 # @@ -104,7 +101,14 @@ # 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 is a deliberate choice to stay small, not an oversight +# 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 @@ -115,12 +119,18 @@ # 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. -# - Enum label ORDER is not checked, only the set. ALTER TYPE ... ADD VALUE -# takes an optional BEFORE/AFTER, so verifying the resulting sort order -# means modelling insert positions -- more machinery 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 @@ -296,41 +306,9 @@ sub is_scaffolding { # Enum labels (see header) # --------------------------------------------------------------------------- -# Returns (type name, [labels]) for a CREATE TYPE ... AS ENUM, else nothing. -# Comments are already gone by normalization time, so every remaining -# single-quoted run inside the parens is a label. -sub enum_labels { +sub enum_type_name { my ($norm) = @_; - return unless $norm =~ /\ACREATE\s+TYPE\s+(\S+)\s+AS\s+ENUM\s*\((.*)\)\z/is; - my ($name, $body) = ($1, $2); - my @labels = $body =~ /'((?:[^']|'')*)'/g; - return ($name, \@labels); -} - -# Returns undef when $norm is not an enum whose type also existed in OLD (so -# the plain copy rule applies), otherwise the list of reasons it is not -# covered -- empty when it is. -sub enum_gaps { - my ($norm, $old_enum, $blob) = @_; - - my ($name, $new_labels) = enum_labels($norm); - return undef unless defined $name; - my $old_labels = $old_enum->{ lc $name } or return undef; - - my %new_has = map { $_ => 1 } @$new_labels; - my @dropped = grep { !$new_has{$_} } @$old_labels; - # No update script can remove an enum label, so say so rather than pretend - # some ALTER TYPE would have covered it. - return ["enum $name dropped label(s) " . join(', ', map {"'$_'"} @dropped) - . '; an enum label cannot be removed by an update script'] - if @dropped; - - my %old_has = map { $_ => 1 } @$old_labels; - return [ map { "enum $name gained label '$_'; update script needs " - . "ALTER TYPE $name ADD VALUE '$_'" } - grep { $blob !~ /ALTER\s+TYPE\s+\Q$name\E\s+ADD\s+VALUE\s+ - (?:IF\s+NOT\s+EXISTS\s+)? '\Q$_\E'/xi } - grep { !$old_has{$_} } @$new_labels ]; + return $norm =~ /\ACREATE\s+TYPE\s+(\S+)\s+AS\s+ENUM\b/i ? lc $1 : undef; } # --------------------------------------------------------------------------- @@ -479,8 +457,8 @@ my @waivers = parse_waivers($upd_text, $paths[2]); my %old_enum; for my $st (@old) { - my ($name, $labels) = enum_labels($st->{norm}); - $old_enum{ lc $name } = $labels if defined $name; + my $name = enum_type_name($st->{norm}); + $old_enum{$name} = 1 if defined $name; } my %old_count; @@ -497,7 +475,7 @@ push @added, $_ for grep { $budget{ $_->{norm} }-- > 0 } @new; push @removed, $_ for grep { $budget{ $_->{norm} }-- > 0 } @old; my (@fail, @waived); -my ($matched, $scaffolding, $enum_ok) = (0, 0, 0); +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 @@ -514,11 +492,8 @@ for my $st (@added) { if (index($blob, $st->{norm}) >= 0) { $matched++; next } if (is_scaffolding($st->{norm})) { $scaffolding++; next } - if (my $gaps = enum_gaps($st->{norm}, \%old_enum, $blob)) { - $enum_ok++ unless @$gaps; - $finding->($where, $_, $_) for @$gaps; - 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})); @@ -535,8 +510,8 @@ push @fail, "$paths[2]: stale waiver /$_->{src}/ matches nothing -- $_->{why}" 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_ok, - scalar @added - $matched - $scaffolding - $enum_ok; + 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; From de8d51d2bea6c5c972de927c064ed64c6e397109 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Fri, 11 Sep 2026 18:50:09 -0500 Subject: [PATCH 6/7] Let a waiver regex contain a slash; document two declined cases Whitespace now closes the waiver regex rather than the first `/`, so a regex holding an operator, a character class or a path no longer truncates at it and silently waives something wider than written. Two cases are documented rather than implemented, both because the right way to verify them is not a cleverer text parser: - The ALTER DEFAULT PRIVILEGES rule recognizes only the `IN SCHEMA ... ON TYPES` shape. It reproduces a known bug shape and is not a general ADP analysis. - Waivers are read line by line rather than through the scanner, so the marker is live inside a string or a dollar-quoted body. Waivers are file-scoped, so one found in an odd place still behaves as written. Co-Authored-By: Claude Opus 5 (1M context) --- bin/test/textfirst.t | 25 +++++++++++++++++++------ bin/update_lint_textfirst | 19 +++++++++++++++++-- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/bin/test/textfirst.t b/bin/test/textfirst.t index 500a293..58236cd 100644 --- a/bin/test/textfirst.t +++ b/bin/test/textfirst.t @@ -1,16 +1,17 @@ #!/usr/bin/env perl # -# One case per rule bin/update_lint_textfirst implements, plus the two things -# only the real tree can prove: that the current development pair is clean, and -# that the ALTER DEFAULT PRIVILEGES rule reproduces the historical bug it was -# written for. Kept deliberately small -- a checker whose test suite dwarfs it -# has stopped being the cheap option. Run from the repo root: +# 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 => 28; +use Test::More tests => 30; use File::Temp qw(tempdir); my $PROG = 'bin/update_lint_textfirst'; @@ -149,6 +150,18 @@ SQL 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 ( diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index e7fc64a..26fc46d 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -76,7 +76,14 @@ # 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. Only the TYPES category is implemented. +# never got GRANT USAGE. +# +# Narrow on purpose: only `IN SCHEMA GRANT

ON TYPES TO ` is +# recognized. A FOR ROLE or a schema-less global ADP raises nothing, and +# TABLES/FUNCTIONS/SEQUENCES would each need their own "what creates one of +# these" pattern. This rule 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 # @@ -131,6 +138,10 @@ # - 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. # - 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 @@ -325,9 +336,13 @@ sub parse_waivers { 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}; + 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; From 93db23eab0901f3c82f35349ea2a5a9979de7723 Mon Sep 17 00:00:00 2001 From: jnasbyupgrade Date: Sun, 13 Sep 2026 15:27:55 -0500 Subject: [PATCH 7/7] Report unreadable ALTER DEFAULT PRIVILEGES forms; run bin/ tests via make An ADP shape the rule was never taught is the case the rule exists for, so skipping it silently was the failure it was written to prevent. Only the one understood form stays checked; anything else is reported and can be waived. `make test-bin` runs everything in bin/test/, ahead of the targets that use bin/ -- a broken tool should not surface as a broken extension. CI calls the target rather than naming one .t, so a new test file needs no CI edit. The sql.mk preprocessing the checker reimplements is now marked DUPLICATED AT on both sides. "Accumulator" drops out of the vocabulary: these are update scripts. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 9 +++---- CLAUDE.md | 4 +-- Makefile | 9 +++++++ bin/test/textfirst.t | 20 ++++++++++++-- bin/update_lint_textfirst | 55 ++++++++++++++++++++++++++++----------- sql.mk | 6 +++++ 6 files changed, 79 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b07e782..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,11 +408,6 @@ jobs: # wrapper that swallows that exit code or calls a different entry point # would defeat this safety net. run: make lint - - name: Test bin/update_lint_textfirst - # Run before the check below, so a bug in the checker is reported as a - # bug in the checker rather than as a finding against the SQL. - # Test::Harness is core Perl, so `prove` needs no setup. - run: prove bin/test/textfirst.t - 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. diff --git a/CLAUDE.md b/CLAUDE.md index 65cd85f..0a37c70 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,8 +55,8 @@ 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 accumulator - update script `sql/cat_tools----stable.sql.in`**, so an +- **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 diff --git a/Makefile b/Makefile index 572c148..ad5f367 100644 --- a/Makefile +++ b/Makefile @@ -118,3 +118,12 @@ include lint.mk .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/bin/test/textfirst.t b/bin/test/textfirst.t index 58236cd..ea40123 100644 --- a/bin/test/textfirst.t +++ b/bin/test/textfirst.t @@ -11,7 +11,7 @@ use strict; use warnings; -use Test::More tests => 30; +use Test::More tests => 32; use File::Temp qw(tempdir); my $PROG = 'bin/update_lint_textfirst'; @@ -178,6 +178,22 @@ for my $bad ( 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: { @@ -188,7 +204,7 @@ SKIP: { 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 the accumulator update 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 diff --git a/bin/update_lint_textfirst b/bin/update_lint_textfirst index 26fc46d..f686382 100755 --- a/bin/update_lint_textfirst +++ b/bin/update_lint_textfirst @@ -19,10 +19,10 @@ # # 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 accumulator every SQL-touching -# PR extends. Both version numbers are derived -- the current one from -# .control's default_version, the previous one from the single accumulator -# file named after it -- so a release needs no edit here. Released pairs are +# 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. # @@ -79,11 +79,13 @@ # never got GRANT USAGE. # # Narrow on purpose: only `IN SCHEMA GRANT

ON TYPES TO ` is -# recognized. A FOR ROLE or a schema-less global ADP raises nothing, and -# TABLES/FUNCTIONS/SEQUENCES would each need their own "what creates one of -# these" pattern. This rule 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. +# 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 # @@ -141,7 +143,9 @@ # - 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. +# 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 @@ -168,6 +172,9 @@ 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, @@ -365,7 +372,8 @@ sub waive { # 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. +# each need their own "what creates one of these" pattern, so any other form is +# reported as unrecognized instead. # --------------------------------------------------------------------------- sub adp_gaps { @@ -375,11 +383,24 @@ sub adp_gaps { for my $st (@$new) { next if $in_old{ $st->{norm} }; # already in force before OLD's objects were made - next unless $st->{norm} =~ /\A + 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; + /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) { @@ -422,8 +443,12 @@ sub install_script { return first_existing("$sql_dir/$ext--$v.sql.in", "$sql_dir/$ext--$v.sql"); } -# The current cycle's pair, derived from the one accumulator update script -# targeting the current version. +# 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 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