planner: refuse the relabel route for type shapes it cannot fully read

[Kiran01bm]

An ALTER COLUMN ... TYPE whose live or target type the planner cannot read in full now takes the rewrite route, instead of being classified as a binary-coercible catalog relabel.

Why

The relabel check compares a parse of the live type's format_type rendering with a parse of the statement's target type, and both parses lose information. The live side ignores everything after the closing parenthesis, so timestamp(3) with time zone reads as timestamp(3), and character varying(50)[] as varchar(50). The target side keeps only the last name part and the modifiers, so it drops [], a schema qualifier, and COLLATE. A change PostgreSQL carries out as a table rewrite under ACCESS EXCLUSIVE, or one it cannot cast at all, could therefore be routed native as a brief relabel.

What

• parseTypeText returns the empty shape, which is never coercible, for an array rendering, text after the modifiers, or modifiers it cannot parse.
• statement.Op gains NewTypeOpaque for ALTER COLUMN TYPE. It is set when the target has array bounds, a schema qualifier other than pg_catalog, or a COLLATE clause, and the planner routes such a change to copy-and-swap with type-rewrite.
• The accepted relabels are unchanged: widening varchar, varchar to text, widening numeric precision, and their pg_catalog. and SQL-standard spellings. One case now takes the rewrite route where it was a native no-op: retyping a timestamp(3) column to timestamp(3), whose live rendering also carries trailing text (without time zone).
• Tests:
  • TestClassifyTypeChangeRewritesWhatTheModelCannotSee has eight named planner cases, all native on main.
  • TestClassifyTypeChangeAcceptsPgCatalogQualifiedTarget guards the qualified spellings.
  • New TestParseOpsShapes rows cover the target shapes.
  • TestMigrateDryRunRoutesTimestamptzToTimestampAsRewrite runs a dry run against a real server, so the live type is the server's own rendering.

Before / after

The example is a timestamptz(3) column retyped to timestamp(3), which PostgreSQL does by rewriting the table. The statement and the live rendering are the same on both sides. What changes is that the planner stops reading the rendering only up to the parenthesis.

 Table:     CREATE TABLE s.t (id int PRIMARY KEY, ts timestamptz(3))
 Statement: ALTER TABLE s.t ALTER COLUMN ts TYPE timestamp(3)

 Before (main)                                  After (this PR)
 ─────────────                                  ───────────────
 live type from the server:                     live type from the server:
   "timestamp(3) with time zone"                  "timestamp(3) with time zone"
        │                                              │
        ▼                                              ▼
 read up to ")": timestamp, modifier 3          text after ")": a shape it cannot read
 target:          timestamp, modifier 3         target:          timestamp, modifier 3
        │                                              │
        ▼                                              ▼
 same family and modifiers, so the planner      an unreadable shape is never a relabel,
 calls it a no-op relabel                       so the planner calls it a type rewrite
        │                                              │
        ▼                                              ▼
 dry run: route native, action execute,         dry run: route copy-and-swap, and no
 it would run the ALTER as written,             native ALTER is offered to run
 rewriting s.t under ACCESS EXCLUSIVE

The other planner cases that change the same way: timetz(3) to time(3), a live array retyped to a scalar, a scalar retyped to an array, an array retyped to an array, a target in a user schema (myschema.text), and a target with COLLATE "C".

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-17T06:31:18.972881Z	72b1002	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[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 1/2 of 02f3096: 0 blocking, 4 non-blocking.

Adversarial correctness pass over the full diff at 02f3096 (the merge of main at ceed81b into the branch), reading classifyTypeChange, parseTypeText, targetTypeShape, binaryCoercible, Op.Describe and the introspection that feeds Facts, not just the hunks. The second comment covers the OSS-adoption and integration lenses.

The change itself is correct. A target with a COLLATE clause now reads as an unreadable shape (planner.go#L556), so classifyTypeChange sends it to copy-and-swap with type-rewrite. The flag is set from def.GetCollClause() != nil (ops.go#L408), and the direction is fail-closed: a false positive costs a slower route, a false negative would run an index rebuild as a "relabel". No entry in docs/invariants.md governs the type-change classifier directly; the nearest is OC-1 (fail closed on uncertainty), which this upholds.

Worth knowing before merging: on current main only one of the four new planner cases is new behavior. The timestamptz, timetz and trailing-text cases already pass at ceed81b (parseTypeText already folds the text after the modifiers into the name there); I ran the new planner test file against the base and only target with a collation fails. Those three are regression guards, which is fine, but see finding 3.

Non-blocking

1. The live column's own collation is not read, so a collated column retyped without COLLATE still routes native. This is the same class of loss the PR closes on the target side, from the other direction, and it is pre-existing rather than a regression. ALTER COLUMN ... TYPE without a COLLATE clause resets the column to the new type's default collation, so a column declared COLLATE "C" that is widened varchar(50) to varchar(100) silently changes collation and rebuilds every index on it under ACCESS EXCLUSIVE. Facts.ColumnTypes carries only format_type, which omits collation, so parseTypeText sees a plain varchar(50) and the planner proves binary-coercible. Measured on postgres:16 at the head: dry run says route=native, disposition=execute, ExecSQL=[ALTER TABLE ... ALTER COLUMN c TYPE varchar(100)], and running that statement moved the collation from "C" to "default" and the index filenode from 16390 to 16391 (table filenode unchanged). schemadiff already introspects Column.Collation (introspect.go#L382-L389) and FactsFrom (planner.go#L278) receives that model, so the fix is small: carry the non-default collation into Facts and have classifyTypeChange take the rewrite route when the live column has one. A Facts built without it behaves as today, which matches the documented "missing facts" contract. The README and docs/limitations.md call explicit column collations out of the declarative model, but migrate --alter / dry run take any statement against any live table, so the gap is reachable. I applied that fix locally and the test below went green; I reverted it.

Test that fails on 02f3096 and passes with the fix

// A column with an explicit collation that is retyped without a COLLATE
// clause takes the type's default collation, so every index on it is rebuilt
// under ACCESS EXCLUSIVE and its sort order changes. That is not a relabel.
func TestMigrateDryRunRoutesCollatedColumnRetypeAsRewrite(t *testing.T) {
	url := testutil.StartPostgres(t)
	pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url})
	require.NoError(t, err)
	defer pool.Close()
	schema := testutil.NewSchema(t, pool)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(`CREATE TABLE %s.t (id int PRIMARY KEY, c varchar(50) COLLATE "C")`, schema))
	require.NoError(t, err)
	_, err = pool.Exec(t.Context(), fmt.Sprintf(`CREATE INDEX t_c ON %s.t (c)`, schema))
	require.NoError(t, err)

	report := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN c TYPE varchar(100)", schema))
	require.Len(t, report.Statements, 1)
	assert.Equal(t, planner.RouteCopyAndSwap, report.Statements[0].Route)
}

--- FAIL: TestMigrateDryRunRoutesCollatedColumnRetypeAsRewrite (2.51s) with expected: "copy-and-swap" / actual: "native" on 02f3096; passes with ColumnCollations carried through FactsFrom and checked in classifyTypeChange.

2. Op.Describe() drops the COLLATE clause, so the plan report names the wrong change. formatNewType (ops.go#L299) says every part that can flip a route must render, but the new NewTypeCollated is not rendered. ALTER COLUMN c TYPE text COLLATE "C" and ALTER COLUMN c TYPE text both describe as ALTER COLUMN c TYPE text, and they now route in opposite directions. Describe() is the Decision.Operation string (planner.go#L334, L479), so an operator reading a dry run sees a type-rewrite verdict on what looks like a text-to-text change. Suggested fix: append COLLATE ... in formatNewType (the grammar's collation name, or a bare COLLATE marker if the name is not captured).

Test that fails on 02f3096 and passes with the fix

func TestDescribeKeepsCollation(t *testing.T) {
	plain, err := statement.ParseOps(`ALTER TABLE t ALTER COLUMN c TYPE text`)
	require.NoError(t, err)
	collated, err := statement.ParseOps(`ALTER TABLE t ALTER COLUMN c TYPE text COLLATE "C"`)
	require.NoError(t, err)
	assert.NotEqual(t, plain[0].Describe(), collated[0].Describe())
}

--- FAIL: TestDescribeKeepsCollation (0.49s) with Should not be: "ALTER COLUMN c TYPE text" on 02f3096; passes once formatNewType renders the clause (not applied here, the assertion is the specification).

3. The PR description does not describe the diff. It names statement.Op.NewTypeOpaque, parseTypeText returning the empty shape for arrays and trailing text, "eight named planner cases, all native on main", and a behavior change for timestamp(3) to timestamp(3). None of that is in the head: the field is NewTypeCollated, parseTypeText is unchanged, there are four cases, and three of them already pass on main. The branch evidently absorbed the earlier fixes through its merges of main. A reviewer or release-notes reader trusting the text would look for an exported field that does not exist. Please rewrite the summary around what remains: the COLLATE target, plus guard tests for the shapes main now handles.

4. Smaller items.

• The reason is type-rewrite, but the repo's own reference says a collation-only change has no heap rewrite, only an index rebuild (binary-coercible-type-changes.md, "A collation change alone"). Refusing the relabel route is right; the reason's published text ("PostgreSQL rewrites the whole table") overstates it for this case.
• The comment on targetTypeShape (planner.go#L547-L554) has one reflowed line far longer than its neighbors.
• No test exercises COLLATE together with USING or a multi-operation statement; both reach classifyTypeChange unchanged, so this is coverage only.

Verified

• go test ./pkg/planner/ ./pkg/statement/ green at the head. TestMigrateDryRunRoutesTimestamptzToTimestampAsRewrite green against a real postgres:16.
• The new planner test run against the base worktree (ceed81b): target with a collation FAILS there (native), the other three cases and the pg_catalog test pass. So only the collation case is a behavior change.
• NewTypeCollated has exactly one reader (targetTypeShape) and one writer; Op is not compared with == anywhere (it holds slices), so adding the field is safe.

Mutant	Caught by
drop || op.NewTypeCollated in targetTypeShape	TestClassifyTypeChangeRewritesWhatTheModelCannotSee/target_with_a_collation
NewTypeCollated: false in alterTableOp	planner collation case and TestParseOpsShapes/alter_type_with_a_collation
NewTypeCollated: ... == nil (inverted)	many: TestClassifyReferenceRows widen cases, TestClassifyUnverifiedMarksFactlessTypeChanges, the collation case
omit the live Collation from Facts (the gap in finding 1)	survives: no test has a collated live column

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

[aparajon]

🤖 Review 2/2 of 02f3096: OSS adoption and integration lenses, 0 additional blocking, 3 non-blocking.

The two lenses applied to the diff at 02f3096. The blocking count for the PR stays at 0 (see 1/2).

OSS adoption: would an operator who is new to pg-sprite understand what happened?

1. A collation-only change is reported as a table rewrite. After this PR, ALTER COLUMN c TYPE text COLLATE "C" dry-runs as type-rewrite, and the reason's documentation (postgres-online-ddl-reference.md#type-rewrite, "so PostgreSQL rewrites the whole table under ACCESS EXCLUSIVE") is what the operator reads. The repo's own reference says a collation change alone does not rewrite the heap; it rebuilds the indexes. Combined with 1/2 finding 2, the report line reads ALTER COLUMN c TYPE text with no sign that a collation is involved. Someone new to the tool will assume the verdict is a bug. One sentence in the type-rewrite section naming the collation case, plus rendering the clause in Describe(), would make the output self-explanatory.

2. The classification doc is silent on the new rule. binary-coercible-type-changes.md, "How pg-sprite classifies type changes" lists what the verdict accepts and calls out schema-qualified and quoted names as matching no rule. A target with COLLATE now also matches no rule, and the section is where an adopter looks first. The same section should say that the live column's explicit collation is not yet read (1/2 finding 1), or the fix for it should land with the doc line.

Integration ease: schemabot and other importers

3. The exported surface grows by one field, and the PR text describes a different one. statement.Op is exported, and importers that call statement.ParseOps or planner.Classify directly see NewTypeCollated as new API. Adding a bool is source-compatible for keyed literals and the zero value means "no clause", so importers that build Op values themselves keep the old behavior. That is the right shape. But the description announces NewTypeOpaque, which does not exist, and an importer writing upgrade notes from it would grep for the wrong name. Fixing the description (1/2 finding 3) is the whole remedy.

Two things that are good for integrators and worth keeping: the new gate lives in the planner's single targetTypeShape seam, so every front door (declarative diff, dry run, library Classify) picks it up identically; and nothing in Facts changed, so hand-built Facts values keep compiling. If the live-collation fix from 1/2 is taken, adding an optional ColumnCollations map keeps both properties.

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

[aparajon]

🤖 Stamping with comments: 0 blocking, 4 non-blocking (see the review comments above).

This stamp was left by Claude Code (claude-sonnet-5-5).