CI: priority lanes for rebase-cascade pushes, escalate on stack promotion

.github/workflows/claude-code-review.yml

@@ -27,12 +27,88 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  claude-review:
-    # jnasbyupgrade's own PRs only, and skip drafts (don't spend API/CI on
-    # unfinished PRs).
+  # Cheap gate that skips the paid review when this push's PATCH is
+  # identical to the last one reviewed for this PR - a pure gh-stack rebase
+  # cascade changes only the base commit, not the PR's own diff, so there's
+  # nothing new to review. Restricted to `synchronize`: `opened`/`reopened`/
+  # `ready_for_review` always run the review regardless of patch-id, since
+  # draft->ready is itself a meaningful trigger even if nothing about the
+  # diff needed to change, and comparing against a stale cached patch-id
+  # there would otherwise wrongly suppress it. Runs before the (slow) "wait
+  # for CI" gate in claude-review below, so a rebase-only push never enters
+  # that wait at all.
+  content-check:
+    runs-on: ubuntu-latest
     if: >-
       github.event.pull_request.draft == false &&
       github.event.pull_request.user.login == 'jnasbyupgrade'
+    outputs:
+      content_changed: ${{ steps.diff.outputs.changed }}
+    steps:
+      # Default (non-overridden) checkout for a pull_request_target event
+      # is the BASE branch - deliberate here: bin/patch_id_hash must be on
+      # disk to run, and an older PR predating this script (checked out at
+      # its own head, as claude-review's later step does) wouldn't have it.
+      # The script itself diffs base...head via git plumbing on commit
+      # objects, not the working tree, so which ref is checked out doesn't
+      # affect the computed hash - only whether the SCRIPT is present.
+      - name: Check out the base branch
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+
+      - name: Fetch the PR head commit
+        # fetch-depth 0 above pulls this repo's full history but not a
+        # fork's (or an as-yet-unmerged) ref - fetch head.sha explicitly so
+        # it's reachable for the diff below.
+        run: git fetch origin ${{ github.event.pull_request.head.sha }}
+
+      - name: Restore this PR's previously observed content patch-id
+        id: patchid-restore
+        if: github.event.action == 'synchronize'
+        uses: actions/cache/restore@v4
+        with:
+          path: /tmp/prev_patch_id
+          # A key guaranteed not to already exist, so this always falls
+          # through to restore-keys (prefix match, picks the MOST RECENT
+          # save) instead of ever hitting its own key directly. Same cache
+          # namespace as ci.yml's `changes` job - both are answering the
+          # identical "did this PR's patch change" question, so whichever
+          # workflow runs first primes the cache for the other.
+          key: unused-${{ github.run_id }}
+          restore-keys: |
+            ci-patchid-pr-${{ github.event.pull_request.number }}-
+
+      - name: Compare against the last observed patch-id
+        id: diff
+        run: |
+          NEW=$(bin/patch_id_hash "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}")
+          echo "content patch-id: $NEW"
+          if [ "${{ github.event.action }}" = "synchronize" ] && \
+             [ -f /tmp/prev_patch_id ] && \
+             [ "$(cat /tmp/prev_patch_id)" = "$NEW" ]; then
+            echo "changed=false" >> "$GITHUB_OUTPUT"
+          else
+            echo "changed=true" >> "$GITHUB_OUTPUT"
+          fi
+          echo "$NEW" > /tmp/prev_patch_id
+
+      - name: Save this push's content patch-id for the next push to compare against
+        if: always()
+        uses: actions/cache/save@v4
+        with:
+          path: /tmp/prev_patch_id
+          key: ci-patchid-pr-${{ github.event.pull_request.number }}-${{ github.run_id }}
+
+  claude-review:
+    needs: [content-check]
+    # jnasbyupgrade's own PRs only, skip drafts (don't spend API/CI on
+    # unfinished PRs), and skip a pure rebase-cascade push (see
+    # content-check above - nothing new to review).
+    if: >-
+      github.event.pull_request.draft == false &&
+      github.event.pull_request.user.login == 'jnasbyupgrade' &&
+      needs.content-check.outputs.content_changed == 'true'
     runs-on: ubuntu-latest
     timeout-minutes: 60
     permissions:

Makefile

@@ -42,10 +42,10 @@ TEST_SCHEMA ?=
 export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_schema=$(TEST_SCHEMA)
 
 # Every TEST_SCHEMA value the suite is tested against. A single source so
-# test-schema-all and CI (once collapsed - see the "why not a CI matrix"
-# note below) can't silently drift onto different sets. See the TEST_SCHEMA
-# comment above for why exercising more than one value here is meaningful
-# (search_path exclusion), not just "install into schema A vs schema B".
+# test-schema-all/test-update-schema-all and CI can't silently drift onto
+# different sets. See the TEST_SCHEMA comment above for why exercising more
+# than one value here is meaningful (search_path exclusion), not just
+# "install into schema A vs schema B".
 TEST_SCHEMA_VALUES = "" Quoted
 
 # TEST_SCHEMA is deliberately NOT a CI matrix dimension: unlike PostgreSQL
@@ -69,3 +69,82 @@ test-schema-all:
 		echo "=== TEST_SCHEMA=$$schema ==="; \
 		$(MAKE) test TEST_SCHEMA="$$schema" || exit 1; \
 	done
+
+# TEST_LOAD_SOURCE selects how test/install/load.sql installs count_nulls
+# for the WHOLE test run:
+#   - fresh (default): CREATE EXTENSION count_nulls (current version).
+#   - update: CREATE EXTENSION at the oldest version we still ship a full
+#     install script for (0.9.6), then ALTER EXTENSION UPDATE to current -
+#     committed, since test/install runs outside any per-test rolled-back
+#     transaction (see pgxntool/README.asc's Update & Upgrade (U&U) Testing
+#     section for why the commit matters).
+#   - existing: count_nulls is already installed (a real `pg_upgrade` run,
+#     external to this invocation) - test/install only asserts it's present
+#     and current, it does not drop/create/update anything. Meant to be run
+#     with CONTRIB_TESTDB=<db> EXTRA_REGRESS_OPTS=--use-existing against a
+#     real database, not via a make wrapper here.
+#
+# "update" (this) is extension-level (ALTER EXTENSION UPDATE); "upgrade" is
+# cluster-level (pg_upgrade) - 'existing' is how that axis is exercised.
+#
+# Propagated the same way as TEST_SCHEMA: via the count_nulls.test_load_mode
+# GUC, exported unconditionally through PGOPTIONS, read without missing_ok.
+TEST_LOAD_SOURCE ?= fresh
+ifeq ($(filter $(TEST_LOAD_SOURCE),fresh update existing),)
+$(error TEST_LOAD_SOURCE must be 'fresh', 'update' or 'existing', got '$(TEST_LOAD_SOURCE)')
+endif
+export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_load_mode=$(TEST_LOAD_SOURCE)
+
+# TEST_EXISTING_DEPLOY: in 'existing' mode (TEST_LOAD_SOURCE=existing), how
+# was the extension actually deployed onto the cluster before this run
+# started? Unlike TEST_LOAD_SOURCE/TEST_SCHEMA above, this does not select or
+# change any install behavior - it only tells test/install/load.sql's
+# existing-mode assertion where to cross-check "what does the cluster
+# consider count_nulls's current version" against the actually-installed
+# extversion:
+#   - filesystem (default): a real .control file is on disk (a real `make
+#     install`, or a pg_upgrade'd cluster carrying one over) - cross-check
+#     against pg_available_extensions.default_version, which reads .control
+#     files directly off disk.
+#   - pgtle: count_nulls was registered purely through pg_tle's
+#     database-backed catalog (see the pg-tle-test CI job), never touching
+#     the filesystem. pg_available_extensions does NOT see pg_tle
+#     registrations at all - it only ever reads .control files off disk - so
+#     its default_version comes back NULL for a pg_tle-only extension even
+#     though a version-less CREATE EXTENSION resolves correctly through
+#     pg_tle. pg_tle ships its own separate, non-integrated analog instead:
+#     pgtle.available_extensions(), a C function whose own doc comment in
+#     pg_tle's tleextension.c says "The system view pg_available_extensions
+#     provides a user interface to this SRF" - i.e. pg_tle's SRF is modeled
+#     on pg_available_extensions, but pg_tle never hooks or populates the
+#     real view itself. In this mode, cross-check against pg_tle's SRF
+#     instead.
+#
+# Deliberately the SAME env var name bin/test_existing already reads (to
+# decide whether to sandbox `make test`'s install step via a scratch
+# DESTDIR) - it's already exported to any `make` invocation bin/test_existing
+# spawns as a child process (and explicitly passed through on run_suite's
+# `make test` command line too), so no extra plumbing is needed to get it
+# here.
+TEST_EXISTING_DEPLOY ?= filesystem
+ifeq ($(filter $(TEST_EXISTING_DEPLOY),filesystem pgtle),)
+$(error TEST_EXISTING_DEPLOY must be 'filesystem' or 'pgtle', got '$(TEST_EXISTING_DEPLOY)')
+endif
+export PGOPTIONS := $(PGOPTIONS) -c count_nulls.test_existing_deploy=$(TEST_EXISTING_DEPLOY)
+
+# Convenience wrapper: `make test-update` == `make test TEST_LOAD_SOURCE=update`.
+# Must recurse (a fresh $(MAKE)) rather than depend on `test`, so the
+# parse-time TEST_LOAD_SOURCE conditional above re-evaluates with update set.
+.PHONY: test-update
+test-update:
+	$(MAKE) test TEST_LOAD_SOURCE=update
+
+# Same TEST_SCHEMA loop as test-schema-all, but in update mode - used by the
+# test CI job's update leg instead of crossing TEST_SCHEMA into ITS matrix
+# too, same reasoning as test-schema-all above.
+.PHONY: test-update-schema-all
+test-update-schema-all:
+	@for schema in $(TEST_SCHEMA_VALUES); do \
+		echo "=== TEST_SCHEMA=$$schema (update) ==="; \
+		$(MAKE) test TEST_LOAD_SOURCE=update TEST_SCHEMA="$$schema" || exit 1; \
+	done

bin/compare_fresh_vs_update

@@ -0,0 +1,135 @@
+#!/usr/bin/env bash
+#
+# compare_fresh_vs_update - structurally compare every object the count_nulls
+# extension owns between a FRESH install (CREATE EXTENSION at current) and an
+# UPDATED one (CREATE EXTENSION at FROM_VERSION, then ALTER EXTENSION
+# UPDATE), in the same schema. A fixed pgTAP expected-output suite (see
+# TEST_LOAD_SOURCE in the Makefile) only proves the specific behaviors it
+# happens to assert still work - it does not prove an update script left
+# object definitions/comments/ACLs BYTE-FOR-BYTE identical to a fresh
+# install of the same version. This catches divergence classes a fixed
+# suite doesn't already know to test for.
+#
+# Modeled on the manual technique used in Postgres-Extensions/cat_tools#46,
+# which found a real bug this way (a pre-0.2.2 update path left
+# `EXECUTE PROCEDURE` hardcoded in a trigger body that fresh installs had
+# already updated to `EXECUTE FUNCTION`). Unlike that PR, this is committed,
+# reusable tooling rather than a one-off manual diff.
+#
+# Postgres-Extensions/cat_tools#67 proposes generalizing this exact
+# capability - "list every object an extension owns, with each object's
+# actual definition" - into a real, user-facing cat_tools feature, not
+# test-only plumbing tied to one extension's update path. If that lands,
+# this script's object-discovery-and-rendering logic (extension_members()
+# below, and the per-kind comparisons) is a natural candidate to
+# reimplement on top of it rather than staying a bespoke,
+# count_nulls-specific copy.
+#
+# What's compared, per object the extension owns (discovered live via
+# pg_depend - see extension_members(), not a hardcoded object list, so a
+# newly added function is automatically covered without editing this
+# script): pg_get_functiondef() (full definition: schema, args, body,
+# volatility, strictness - everything), its comment (obj_description), and
+# its ACL (proacl). count_nulls currently ships only functions/triggers, no
+# views - the doc this is modeled on also compares pg_get_viewdef/type
+# labels/extension membership for extensions that have those; add a query
+# for the relevant catalog (pg_class for views, pg_type for types, ...) the
+# same way if count_nulls ever grows one.
+#
+# USAGE: bin/compare_fresh_vs_update [SCHEMA] [FROM_VERSION] [EXISTING_DB]
+#   SCHEMA       - schema both installs target (default: unqualified, same
+#                  as TEST_SCHEMA empty - see the Makefile). Both installs
+#                  use the SAME schema, since the point is comparing object
+#                  definitions, not exercising schema-qualification (that's
+#                  TEST_SCHEMA's job in the regular suite).
+#   FROM_VERSION - the update origin (default: 0.9.6, the oldest version
+#                  count_nulls still ships a full install script for).
+#                  Ignored when EXISTING_DB is given - that database's
+#                  history is whatever already produced it.
+#   EXISTING_DB  - compare against this ALREADY-POPULATED database instead
+#                  of creating+updating a scratch one (default: none, create
+#                  our own scratch "updated" database as before). Lets
+#                  callers that produced their update/upgrade result some
+#                  other way - e.g. bin/test_existing's real binary
+#                  pg_upgrade path - reuse this same comparison without this
+#                  script re-deriving that database itself. The caller owns
+#                  EXISTING_DB's lifecycle: it is never created, updated, or
+#                  dropped here, only queried.
+#
+# Exits nonzero (and prints a real diff) on ANY difference. Scratch
+# database(s) this script created itself are dropped on exit regardless of
+# outcome; an EXISTING_DB passed in is left untouched.
+set -euo pipefail
+
+cd "$(dirname "$(readlink -f "$0")")/.."
+
+schema=${1:-}
+from_version=${2:-0.9.6}
+existing_db=${3:-}
+
+fresh_db=compare_fresh_vs_update_fresh
+update_db=${existing_db:-compare_fresh_vs_update_updated}
+fresh_snapshot=$(mktemp)
+update_snapshot=$(mktemp)
+
+cleanup() {
+  dropdb --if-exists "$fresh_db"
+  # Only drop update_db if we created it ourselves - an EXISTING_DB belongs
+  # to the caller (e.g. the real pg_upgraded database bin/test_existing is
+  # still using) and must survive this script running.
+  if [ -z "$existing_db" ]; then
+    dropdb --if-exists "$update_db"
+  fi
+  rm -f "$fresh_snapshot" "$update_snapshot"
+}
+trap cleanup EXIT
+
+# extension_members(): every object pg_depend records as owned by the
+# count_nulls extension (deptype 'e'), restricted to pg_proc for now (see
+# the header comment on extending this). Ordered by name/args so the two
+# snapshots line up for a textual diff regardless of OID assignment order,
+# which differs between a fresh install and an update.
+query() {
+  cat <<'SQL'
+SELECT
+  '-- ' || p.oid::regprocedure::text || E'\n'
+  || pg_get_functiondef(p.oid) || E'\n'
+  || '-- comment: ' || coalesce(obj_description(p.oid, 'pg_proc'), '(none)') || E'\n'
+  || '-- acl: ' || coalesce(p.proacl::text, '(default)') || E'\n'
+FROM pg_depend d
+JOIN pg_extension x ON d.refobjid = x.oid AND x.extname = 'count_nulls'
+JOIN pg_proc p ON d.objid = p.oid AND d.classid = 'pg_proc'::regclass
+WHERE d.deptype = 'e'
+ORDER BY p.proname, p.oid::regprocedure::text;
+SQL
+}
+
+install_in_schema() {
+  local sql=""
+  if [ -n "$schema" ]; then
+    sql="CREATE SCHEMA IF NOT EXISTS \"$schema\"; SET search_path = \"$schema\"; "
+  fi
+  echo "$sql"
+}
+
+createdb "$fresh_db"
+psql -d "$fresh_db" -v ON_ERROR_STOP=1 -c "$(install_in_schema)CREATE EXTENSION count_nulls"
+
+if [ -z "$existing_db" ]; then
+  createdb "$update_db"
+  psql -d "$update_db" -v ON_ERROR_STOP=1 -c "$(install_in_schema)CREATE EXTENSION count_nulls VERSION '$from_version'"
+  psql -d "$update_db" -v ON_ERROR_STOP=1 -c "SET client_min_messages = WARNING; ALTER EXTENSION count_nulls UPDATE"
+fi
+
+psql -d "$fresh_db"  -tA -v ON_ERROR_STOP=1 -c "$(query)" > "$fresh_snapshot"
+psql -d "$update_db" -tA -v ON_ERROR_STOP=1 -c "$(query)" > "$update_snapshot"
+
+source_desc="$from_version->current update"
+[ -n "$existing_db" ] && source_desc="'$existing_db'"
+
+if diff -u "$fresh_snapshot" "$update_snapshot"; then
+  echo "OK: fresh install and $source_desc produce IDENTICAL object definitions/comments/ACLs"
+else
+  echo "FAIL: $source_desc diverges from a fresh install of the same version - see diff above" >&2
+  exit 1
+fi

bin/patch_id_hash

@@ -0,0 +1,13 @@
+#!/bin/sh
+# Emits, on stdout, a single hash representing the CONTENT of the diff
+# between $1 (base) and $2 (head) - via `git patch-id --stable`, not a plain
+# `git diff | sha256sum`. patch-id normalizes away the context-line shifts a
+# rebase causes when the base commit itself changes, so it comes out
+# IDENTICAL across a pure rebase that introduces no new content. That's the
+# signal CI uses to tell "just a gh-stack rebase cascade" apart from "real
+# new commits", and route the former to a low-priority runner lane instead
+# of treating it like fresh work.
+set -eu
+base="$1"
+head="$2"
+git diff "$base...$head" | git patch-id --stable | awk '{print $1}'