Phase 2.3-2.4: classifier and router seam

[Kiran01bm]

Summary

Phase 2.3–2.4 — the classifier and the router seam. Stacked on kiran01bm/phase-2-1-2-2-diff.

What

• pkg/statement typed per-operation descriptors and advisory safer-SQL rewrites (CREATE INDEX → CONCURRENTLY, ADD CONSTRAINT → NOT VALID + VALIDATE, …).
• pkg/planner: classifies each operation native / copy-and-swap / refuse with typed reasons; migrate --dry-run renders the classified plan.
• pkg/router: assigns classified statements to backends; copy-and-swap reports unavailable until that backend lands.

Why

The classifier is PostgreSQL's missing ALGORITHM=/LOCK= declaration — the safety decision in one pure, testable place — and the router is the single seam where migration policy will live, so Phase 3 executors plug in without touching the planner.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Review requested by Armand and performed by his agent — same two lenses used across this stack (#4, #5, #6, #3): pg-sprite as an OSS-first, best-in-class Postgres DDL tool, and pg-sprite as a clean integration target for an orchestrator. Reviewed at head 16c42c0. An adversarial correctness pass is posted separately.

This is the PR the rest of the engine hangs off, and the decomposition is right: pkg/statement extracts facts (one Op per ALTER subcommand, unrecognized shapes returned as data rather than errors), pkg/planner decides what PostgreSQL would do, pkg/router decides what this build can do about it. Keeping those three questions in three packages is what will let the reference table grow without the routing policy rotting. Routing copy-and-swap to unavailable instead of silently succeeding, and refusing an unknown route as version skew rather than guessing a backend, are both the fail-closed instinct the safety partition asks for. diff and migrate --dry-run sharing one classify-and-route pipeline means the declarative and imperative front doors cannot drift.

OSS lens

1. route: native is carrying two very different meanings. Today it means both "PostgreSQL does this online exactly as you submitted it" (online-idiom, metadata-only, fast-default) and "PostgreSQL can do this, but the form you submitted takes ACCESS EXCLUSIVE and scans — here is a rewrite, which we may or may not have been able to construct" (safer-idiom). A consumer branching on route alone, or an operator skimming -- native (…), reads those identically. For a tool whose headline promise is that it will not surprise you, this is the highest-value distinction to draw before external users start depending on the output — either a separate route/disposition for "native only via the safer sequence", or a rule that safer-idiom without a constructed SaferSQL cannot be execute. The adversarial comment shows both ways that gap currently bites.

2. The reference table deserves to be published as data, not just encoded in a switch. The operation → route → reason mapping is the most valuable single artifact in this repo for a Postgres person: online-DDL knowledge is currently scattered across blog posts of wildly varying vintage and version accuracy, and a tested, versioned table is a real contribution to the ecosystem. Consider generating a docs table from the classifier (or pg-sprite explain --all), stamped with the PostgreSQL versions CI verified it against. It also gives you a natural place to cite why each row is what it is.

3. The table's version-sensitivity isn't expressed anywhere. Several rules encoded here are version-dependent — DETACH PARTITION CONCURRENTLY is PG 14+, SET NOT NULL accepting a validated CHECK as proof is PG 12+, non-rewriting ADD COLUMN … DEFAULT is PG 11+. The CI matrix (14–18) means every rule currently holds across the supported range, which is exactly why now is the cheap moment to state the floor in the package doc. Without it, the first rule that is version-dependent will be added without anyone noticing the classifier has no way to express it.

Integration lens

4. ExecSQL needs an execution contract, not just an order. It is a flat []string, but the steps carry unstated requirements: CREATE INDEX CONCURRENTLY cannot run inside a transaction block, and the four-step SET NOT NULL sequence is only correct if the steps run in order. The text renderer already knows this and handles it beautifully — safer sequences are emitted as comments, never substituted into the executable script, with the reason in the doc comment. The machine-readable seam an orchestrator actually consumes doesn't carry that knowledge. A per-step shape ({sql, in_transaction: false}) or a documented invariant on the field would keep the next consumer from wrapping the list in a transaction and discovering the rule at runtime.

5. The safer sequences need a documented partial-failure contract. If VALIDATE CONSTRAINT fails (the data genuinely has NULLs, or the statement hits its budget), a NOT VALID CHECK named <table>_<column>_not_null is left behind on a live table, and a retry of the same sequence now fails at step 1 on the name that already exists. The same applies to a CREATE INDEX CONCURRENTLY that fails and leaves an INVALID index. A driver retrying an interrupted apply is the normal case in a control plane, not the exceptional one — so "what does step N assume about steps 1..N-1, and what is safe to re-run" belongs in the package doc now, while the sequences are still few. (executor: native CREATE INDEX CONCURRENTLY with fail-closed invalid-index recovery #15 looks like where the executor side of this lands.)

6. Facts is one field today and already load-bearing — name its growth path. ColumnTypes alone drives the entire type-change branch, and the fail-closed default (missing fact → copy-and-swap) is exactly right. But nullability, approximate row count, existing indexes, and partitioning status would each sharpen a row of the table, and each needs a decision about who is trusted to supply it — the CLI introspects, but an orchestrator may want to pass facts it already holds. Worth saying in the doc comment whether Facts is "whatever the caller knows" or "only what pg-sprite introspected", because the safety argument differs.

Verified solid

The refusal wall holds under pressure: I swept twenty statement shapes an operator might plausibly submit, and every genuinely non-online operation the engine doesn't model — CREATE TABLE … AS SELECT, SET UNLOGGED/SET LOGGED, INHERIT, OWNER TO, ENABLE ROW LEVEL SECURITY, DROP EXPRESSION, CLUSTER, VACUUM FULL, TRUNCATE, ALTER TYPE … ADD VALUE — came back refuse (unsupported-operation). That is the property that matters most in a classifier and it is genuinely intact. isConstantExpr accepting only AConst and TypeCast is the right kind of conservative: now() and uuid_generate_v4() route to copy-and-swap rather than being reasoned about. classifyTypeChange failing closed when the column type is unknown or a USING clause is present means the dangerous direction requires evidence, not the safe one. binaryCoercible's narrow allowlist checks out against a real server — I tested all five rules by watching relfilenode, including the numeric-precision widening I expected to be wrong, and PostgreSQL rewrites none of them. Schema qualification survives every safer-sequence construction. #5's RENAME/SET SCHEMA misclassification is fixed here. CGO_ENABLED=0 go build ./... still passes at this head.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Adversarial correctness review requested by Armand and performed by his agent — separate from the two-lens pass. Method: attack the classifier and the router, then verify every candidate finding against a real PostgreSQL at head 16c42c0 (checkout + testcontainers). All four findings below reproduced live; reproduction tests are in the collapsed blocks at the end.

Findings, most severe first

1. ADD COLUMN with an inline table constraint is classified metadata-only and executed as submitted. addColumnOp inspects only CONSTR_DEFAULT, CONSTR_IDENTITY, and CONSTR_GENERATED; CONSTR_UNIQUE, CONSTR_PRIMARY, CONSTR_FOREIGN, and CONSTR_CHECK fall straight through the switch, so the Op reports "plain add column, no default" and the planner takes the default: arm:

ALTER TABLE t ADD COLUMN c int                        route=native reason=metadata-only disposition=execute
ALTER TABLE t ADD COLUMN c int UNIQUE                 route=native reason=metadata-only disposition=execute
ALTER TABLE t ADD COLUMN c int PRIMARY KEY            route=native reason=metadata-only disposition=execute
ALTER TABLE t ADD COLUMN c int REFERENCES parent(id)  route=native reason=metadata-only disposition=execute
ALTER TABLE t ADD COLUMN c int CHECK (c > 0)          route=native reason=metadata-only disposition=execute

Against a real server, ADD COLUMN c bigint UNIQUE on a 200k-row table builds a full index while holding ACCESS EXCLUSIVE:

lock held: t          ShareLock
lock held: t          AccessExclusiveLock
lock held: t_c_key    AccessExclusiveLock
index built by the 'metadata-only' statement: t_c_key

That is the exact operation ADD CONSTRAINT … UNIQUE is correctly routed safer-idiom for, reachable through a spelling the classifier doesn't look at. The PRIMARY KEY form is the same index build; REFERENCES additionally takes SHARE ROW EXCLUSIVE on the parent table. Worth noting the test suite covers every DEFAULT shape (constant, now(), random(), uuid_generate_v4(), CURRENT_TIMESTAMP, serial, identity, generated-stored) and no inline table constraint at all — the gap is in the fact extractor, so no amount of planner testing would surface it. Fix shape: have addColumnOp surface the inline constraint and route it through the same path classifyAddConstraint already handles well.

2. reason: safer-idiom with no constructed safer SQL silently executes the blocking form. nativeExecSQL falls back to p.Statement whenever SaferSQL is absent, and writeChangeText suppresses the "the engine would run instead" block when ExecSQL[0] == ch.SQL. The result is an annotation asserting a safer idiom exists, immediately followed by the unsafe statement, with nothing marking the difference. Two triggers, both reproduced:

-- native (safer-idiom)
ALTER TABLE t ALTER COLUMN a SET NOT NULL, ALTER COLUMN b SET NOT NULL;

-- native (safer-idiom)
ALTER TABLE t ATTACH PARTITION t_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01');

The first is the single := len(ops) == 1 gate: add a second subcommand and the safer sequence disappears while the reason stays. The second is by design — the code comment says plainly that the planner cannot construct the ATTACH CHECK — but the disposition is still execute and the emitted SQL still scans the whole child table under ACCESS EXCLUSIVE on the parent. In JSON the same shape appears as "reason":"safer-idiom" with no safer_sql key and "disposition":"execute", so a consumer has to infer the hazard from an absent field. This is the concrete form of OSS-lens item 1: safer-idiom without SaferSQL is the one combination that should never reach execute unannounced.

3. Generated constraint and index names collide with the table itself once PostgreSQL truncates them to 63 bytes. setNotNullSequence and usingIndexSequence build names by concatenation (<table>_<column>_not_null, <table>_<cols>_key/_pkey) and sanitize after, with no length check. On a table whose own name is at the 63-byte limit, every generated name truncates back to exactly the table name — so the safer sequence for ADD UNIQUE is unexecutable at step 1:

CREATE UNIQUE INDEX CONCURRENTLY "orders_line_items_fulfillment_attempts_reconci...
    err=ERROR: relation "orders_line_items_fulfillment_attempts_reconciliation_snapshots" already exists (SQLSTATE 42P07)
ALTER TABLE "t_8198_1"."orders_line_items_fulfillment_attempts_reconciliation_sn...
    err=ERROR: "orders_line_items_fulfillment_attempts_reconciliation_snapshots" is not an index (SQLSTATE 42809)

Short of that limit the failure is a collision between siblings rather than with the table: two SET NOT NULL scaffolds on the same long-named table generate 102- and 97-byte names that truncate to the same 63 bytes, and the second ADD CONSTRAINT fails 42710 … already exists. Either way the advertised safer path fails with an error that points at the table name rather than at name generation, and the operator's likely reading is "pg-sprite is broken", not "shorten your identifiers". Names should be built to fit — a deterministic hash suffix when the concatenation would exceed 63 bytes is the usual answer — and ideally checked against existing relations before being emitted.

4. CREATE TABLE … PARTITION OF is metadata-only on a justification that does not hold for it. The branch is commented "A new table has no readers to lock out", which is true of CREATE TABLE and false of the partition form: it takes ACCESS EXCLUSIVE on the parent, i.e. on every reader of every partition.

lock held: p          AccessExclusiveLock
lock held: p_2026     AccessExclusiveLock

The route itself is defensible — it is brief and scans nothing — so this is milder than the others. But metadata-only is the annotation an operator uses to decide they can run something during peak traffic, and on a hot partitioned table an ACCESS EXCLUSIVE on the parent queued behind one long-running SELECT stalls reads across the whole table. A distinct reason that names the parent lock would keep the table honest, and the comment should stop claiming there are no readers.

Probed and held

Attacks that failed, and one that changed my mind. binaryCoercible is correct — I expected the numeric rule to be wrong, since "widening precision at the same scale" reads like something PostgreSQL would still verify; I tested all five allowlisted transformations by comparing pg_relation_filenode across the ALTER, and PostgreSQL rewrites none of them (varchar(50)→varchar(100), →text, →varchar, numeric(10,2)→numeric(20,2), →numeric). The safer PRIMARY KEY sequence stays online on a nullable column — I expected ADD CONSTRAINT … PRIMARY KEY USING INDEX to force a full scan to prove NOT NULL; on 300k rows it completed in 6ms with no sequential scan and the column correctly flipped, so the unique index is enough proof for PostgreSQL. Schema qualification survives every safer-sequence construction (ALTER TABLE billing.invoices … produces "billing"."invoices" throughout, with the index landing in the table's schema). The refusal wall holds across twenty probed shapes — CREATE TABLE … AS SELECT, SET UNLOGGED/SET LOGGED, INHERIT, OWNER TO, ENABLE ROW LEVEL SECURITY, DROP EXPRESSION, CLUSTER, VACUUM FULL, TRUNCATE, and ALTER TYPE … ADD VALUE all refuse. The text renderer never substitutes safer SQL into the executable script body, because a CONCURRENTLY rewrite could not run in a transaction — deliberate, documented, and the right call.

Reproduction tests

Findings 1 and 2 — pkg/planner/adv_test.go: classification and routing (no database needed)

package planner_test

import (
	"encoding/json"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/pkg/planner"
	"github.com/block/pg-sprite/pkg/router"
)

// What does the classifier say about an ADD COLUMN that carries an inline
// table constraint (UNIQUE / PRIMARY KEY / REFERENCES / CHECK)? Each of
// these does substantially more than add a catalog entry.
func TestAdvAddColumnInlineConstraints(t *testing.T) {
	for _, sql := range []string{
		`ALTER TABLE t ADD COLUMN c int`,
		`ALTER TABLE t ADD COLUMN c int UNIQUE`,
		`ALTER TABLE t ADD COLUMN c int PRIMARY KEY`,
		`ALTER TABLE t ADD COLUMN c int REFERENCES parent(id)`,
		`ALTER TABLE t ADD COLUMN c int CHECK (c > 0)`,
	} {
		p, err := planner.Classify(sql, planner.Facts{})
		require.NoError(t, err)
		routed := router.Route([]planner.Plan{p})
		t.Logf("%-56s route=%-13s reason=%-16s disposition=%s exec=%v",
			sql, p.Route, p.Decisions[0].Reason, routed.Statements[0].Disposition, routed.Statements[0].ExecSQL)
	}
}

// A multi-operation ALTER: the `single` gate suppresses safer-SQL
// construction. What does the router then hand the native backend?
func TestAdvMultiOpSuppressesSaferSQL(t *testing.T) {
	for _, sql := range []string{
		`ALTER TABLE t ALTER COLUMN a SET NOT NULL`,
		`ALTER TABLE t ALTER COLUMN a SET NOT NULL, ALTER COLUMN b SET NOT NULL`,
		`ALTER TABLE t ATTACH PARTITION t_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01')`,
		`CREATE INDEX i ON t (a)`,
	} {
		p, err := planner.Classify(sql, planner.Facts{})
		require.NoError(t, err)
		routed := router.Route([]planner.Plan{p})
		b, _ := json.Marshal(routed.Statements[0])
		t.Logf("%s\n  -> %s", sql, b)
	}
}

The rendered-plan half of finding 2, in internal/cli:

package cli

import (
	"strings"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/pkg/planner"
	"github.com/block/pg-sprite/pkg/router"
	"github.com/block/pg-sprite/pkg/schemadiff"
)

// What the operator reads for statements the planner routes native with a
// safer idiom it could not construct.
func TestAdvPlanTextNoSaferSQL(t *testing.T) {
	for _, sql := range []string{
		`ALTER TABLE t ALTER COLUMN a SET NOT NULL, ALTER COLUMN b SET NOT NULL`,
		`ALTER TABLE t ATTACH PARTITION t_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01')`,
		`ALTER TABLE t ADD COLUMN c int UNIQUE`,
	} {
		p, err := planner.Classify(sql, planner.Facts{})
		require.NoError(t, err)
		rs := router.Route([]planner.Plan{p}).Statements[0]
		var out strings.Builder
		require.NoError(t, writeChangeText(&out, plannedChange{
			Change: schemadiff.Change{SQL: sql}, Route: rs.Route, Backend: rs.Backend,
			Disposition: rs.Disposition, Decisions: rs.Decisions, ExecSQL: rs.ExecSQL,
		}))
		t.Logf("\n%s", out.String())
	}
}

Observed:

-- native (safer-idiom)
ALTER TABLE t ALTER COLUMN a SET NOT NULL, ALTER COLUMN b SET NOT NULL;

-- native (safer-idiom)
ALTER TABLE t ATTACH PARTITION t_2026 FOR VALUES FROM ('2026-01-01') TO ('2027-01-01');

-- native (metadata-only)
ALTER TABLE t ADD COLUMN c int UNIQUE;

Findings 1, 3 and 4 — pkg/planner/adv_integration_test.go: locks, index build, name truncation, partition parent lock

package planner_test

import (
	"fmt"
	"testing"

	"github.com/stretchr/testify/require"

	"github.com/block/pg-sprite/internal/testutil"
	"github.com/block/pg-sprite/pkg/dbconn"
	"github.com/block/pg-sprite/pkg/planner"
)

// ADD COLUMN ... UNIQUE is classified metadata-only. Does PostgreSQL treat
// it as a catalog change, or does it build an index over the whole table
// under ACCESS EXCLUSIVE?
func TestAdvAddColumnUniqueIsNotMetadataOnly(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.t (id bigint PRIMARY KEY, v text)", schema))
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"INSERT INTO %s.t SELECT g, repeat('x', 100) FROM generate_series(1, 200000) g", schema))
	require.NoError(t, err)

	conn, err := pool.Acquire(t.Context())
	require.NoError(t, err)
	defer conn.Release()
	_, err = conn.Exec(t.Context(), "BEGIN")
	require.NoError(t, err)
	_, err = conn.Exec(t.Context(), fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN c bigint UNIQUE", schema))
	require.NoError(t, err)

	rows, err := conn.Query(t.Context(), `
		SELECT c.relname, l.mode FROM pg_locks l
		JOIN pg_class c ON c.oid = l.relation
		WHERE l.pid = pg_backend_pid() AND l.locktype = 'relation'
		  AND c.relnamespace = $1::regnamespace
		ORDER BY c.relname`, schema)
	require.NoError(t, err)
	for rows.Next() {
		var rel, mode string
		require.NoError(t, rows.Scan(&rel, &mode))
		t.Logf("lock held: %-24s %s", rel, mode)
	}
	rows.Close()

	var idx string
	var pages int
	require.NoError(t, conn.QueryRow(t.Context(), `
		SELECT c.relname, c.relpages FROM pg_class c
		WHERE c.relnamespace = $1::regnamespace AND c.relkind = 'i' AND c.relname LIKE 't_c%'`,
		schema).Scan(&idx, &pages))
	t.Logf("index built by the 'metadata-only' statement: %s", idx)
	_, err = conn.Exec(t.Context(), "ROLLBACK")
	require.NoError(t, err)
}

// usingIndexSequence names constraints and indexes by concatenation, and
// those names are permanent. Two unique constraints on a long-named table:
// do the planner's names survive PostgreSQL's 63-byte identifier limit?
func TestAdvUniqueNameTruncationCollision(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	const tbl = "orders_line_items_fulfillment_attempts_reconciliation_snapshots"
	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		`CREATE TABLE %s.%s (customer_reference_identifier bigint, tenant_partition_key bigint)`, schema, tbl))
	require.NoError(t, err)

	for _, cols := range []string{"customer_reference_identifier", "tenant_partition_key"} {
		p, err := planner.Classify(fmt.Sprintf("ALTER TABLE %s.%s ADD UNIQUE (%s)", schema, tbl, cols), planner.Facts{})
		require.NoError(t, err)
		for _, s := range p.Decisions[0].SaferSQL {
			_, execErr := pool.Exec(t.Context(), s)
			t.Logf("%.80s...\n    err=%v", s, execErr)
		}
	}
}

// CREATE TABLE ... PARTITION OF is classified metadata-only on the grounds
// that "a new table has no readers to lock out". Which relations does it
// actually lock?
func TestAdvCreatePartitionOfLocks(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	_, err = pool.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.p (id bigint, d date) PARTITION BY RANGE (d)", schema))
	require.NoError(t, err)

	conn, err := pool.Acquire(t.Context())
	require.NoError(t, err)
	defer conn.Release()
	_, err = conn.Exec(t.Context(), "BEGIN")
	require.NoError(t, err)
	_, err = conn.Exec(t.Context(), fmt.Sprintf(
		"CREATE TABLE %s.p_2026 PARTITION OF %s.p FOR VALUES FROM ('2026-01-01') TO ('2027-01-01')",
		schema, schema))
	require.NoError(t, err)

	rows, err := conn.Query(t.Context(), `
		SELECT c.relname, l.mode FROM pg_locks l JOIN pg_class c ON c.oid = l.relation
		WHERE l.pid = pg_backend_pid() AND l.locktype = 'relation'
		  AND c.relnamespace = $1::regnamespace ORDER BY c.relname, l.mode`, schema)
	require.NoError(t, err)
	for rows.Next() {
		var rel, mode string
		require.NoError(t, rows.Scan(&rel, &mode))
		t.Logf("lock held: %-10s %s", rel, mode)
	}
	rows.Close()
	_, err = conn.Exec(t.Context(), "ROLLBACK")
	require.NoError(t, err)
}

Probed and held — pkg/planner/adv2_integration_test.go: binary-coercibility and the PRIMARY KEY sequence (both refuted my hypotheses)

// binaryCoercible claims varchar widening and numeric precision widening
// need no rewrite. Does PostgreSQL agree? A rewrite changes the relfilenode.
func TestAdvBinaryCoercibleRewriteCheck(t *testing.T) {
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)})
	require.NoError(t, err)
	t.Cleanup(pool.Close)
	schema := testutil.NewSchema(t, pool)

	cases := []struct{ from, to string }{
		{"varchar(50)", "varchar(100)"},
		{"varchar(50)", "text"},
		{"varchar(50)", "varchar"},
		{"numeric(10,2)", "numeric(20,2)"},
		{"numeric(10,2)", "numeric"},
	}
	for i, c := range cases {
		tbl := fmt.Sprintf("%s.t%d", schema, i)
		_, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s (c %s)", tbl, c.from))
		require.NoError(t, err)
		var before, after uint32
		require.NoError(t, pool.QueryRow(t.Context(),
			fmt.Sprintf("SELECT pg_relation_filenode('%s')", tbl)).Scan(&before))
		_, err = pool.Exec(t.Context(), fmt.Sprintf("ALTER TABLE %s ALTER COLUMN c TYPE %s", tbl, c.to))
		require.NoError(t, err)
		require.NoError(t, pool.QueryRow(t.Context(),
			fmt.Sprintf("SELECT pg_relation_filenode('%s')", tbl)).Scan(&after))
		t.Logf("%-14s -> %-14s rewrite=%v", c.from, c.to, before != after)
	}
}

Observed — every allowlisted rule is genuinely rewrite-free:

varchar(50)    -> varchar(100)   rewrite=false
varchar(50)    -> text           rewrite=false
varchar(50)    -> varchar        rewrite=false
numeric(10,2)  -> numeric(20,2)  rewrite=false
numeric(10,2)  -> numeric        rewrite=false

And the PRIMARY KEY sequence on a nullable column over 300k rows:

planner safer SQL: CREATE UNIQUE INDEX CONCURRENTLY "t_id_pkey" ON "t" ("id")
planner safer SQL: ALTER TABLE "t" ADD CONSTRAINT "t_id_pkey" PRIMARY KEY USING INDEX "t_id_pkey"
ADD CONSTRAINT ... USING INDEX: elapsed=6.021084ms seq_scans 1 -> 1, column now NOT NULL=true

This review was generated by Claude Code (claude-fable-5). All four findings were reproduced against a live PostgreSQL 16 using the tests above.

[aparajon]

🤖 Approving on Armand's behalf. My two-lens review and adversarial correctness pass are posted above — the findings there are for follow-up, not fix-before-merge blockers.

This approval was submitted by Claude Code (claude-fable-5) at Armand's direction.