feat(postgres): add ADD COLUMN synthesis to the statement parser seam

[Kiran01bm]

Add a pkg/ddl seam that synthesizes ALTER TABLE … ADD COLUMN from the desired CREATE TABLE statement, for PostgreSQL startup schema convergence.

Why

verifyPostgresSchemaShape currently fails startup when a storage table is missing a column — it detects drift but cannot converge it. Converging safely requires deriving the exact ADD COLUMN from the embedded desired schema rather than hand-maintaining a parallel copy of every column's DDL. This PR adds that pure synthesis seam; a follow-up PR flips the tripwire outcomes to converge under an advisory lock.

What

• SynthesizeAddColumn(createTableDDL, columnName) on the StatementParser interface, reached through ParserForDialect like every other dialect-sensitive operation. The PostgreSQL implementation parses the CREATE TABLE with pg_query, lifts the column's ColumnDef — type and column-level constraints — into an AlterTableStmt AST, and deparses back to SQL. Table-level constraints (PRIMARY KEY (…), UNIQUE (…), CHECK (…)) are not part of a column declaration and are not carried; the output is pg_query's normalized rendering, not a textual slice of the input. The MySQL-family parser returns a not-supported error (its bootstrapper diffs schemas with Spirit instead).
• Unit tests covering plain columns, NOT NULL + DEFAULT, typmods, function defaults, schema-qualified and quoted identifiers, COLLATE, IDENTITY, generated STORED columns, arrays, STORAGE, COMPRESSION, the table-level-constraint scope, and every error path.
• A corpus test that iterates every CREATE TABLE in the real embedded PostgreSQL schema files and asserts, via parse-tree equality, that the ColumnDef carried by each synthesized ALTER is node-for-node identical to the declaration — no clause can be silently dropped. This proves round-trip faithfulness only: a NOT NULL column without a DEFAULT synthesizes exactly as declared even though PostgreSQL rejects that ALTER on a populated table. Applicability is the caller's judgment; the follow-up converger PR adds the classifier that makes it.
• columnName matches the parser-folded column name (the values CreateTableColumns returns), documented on the seam.

No production caller yet — no behavior change.

Before / after

Before                                  After
┌──────────────────────────────┐       ┌──────────────────────────────┐
│ embedded CREATE TABLE files  │       │ embedded CREATE TABLE files  │
│          │                   │       │          │                   │
│          ▼                   │       │          ▼                   │
│ verify shape ── missing ──▶  │       │ ParserForDialect(postgres)   │
│              column = fatal  │       │   .SynthesizeAddColumn       │
│                              │       │ CREATE TABLE ──▶ ALTER TABLE │
│ (no way to derive the        │       │ ColumnDef lift + deparse     │
│  ADD COLUMN statement)       │       │ (converger lands next PR)    │
└──────────────────────────────┘       └──────────────────────────────┘

[Copilot]

Pull request overview

This PR extends the pkg/ddl PostgreSQL parser seam with a pure helper that can synthesize an ALTER TABLE … ADD COLUMN statement for a specific column by parsing a desired CREATE TABLE statement and lifting the column’s ColumnDef into an AlterTableStmt. This supports upcoming PostgreSQL startup schema convergence work without duplicating column DDL in code.

Changes:

• Added SynthesizePostgresAddColumn(createTableDDL, columnName) to synthesize ADD COLUMN from a parsed PostgreSQL CREATE TABLE AST and deparse it back to SQL.
• Added unit tests for common column forms (defaults, NOT NULL, typmods, quoted/schema-qualified identifiers) and all major error paths.
• Added a corpus-style test that iterates embedded PostgreSQL schema files and verifies every column can be synthesized and re-parsed.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/ddl/postgres_parser.go	Adds SynthesizePostgresAddColumn which parses a single CREATE TABLE, finds a target ColumnDef, constructs an AlterTableStmt, and deparses to SQL.
pkg/ddl/postgres_parser_test.go	Adds unit + embedded-schema corpus tests validating synthesis output and error handling, plus round-trip parsing of synthesized DDL.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

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

[morgo]

🤖 Approved on Morgan's behalf by his AI agent.

Pure addition — one new function plus its tests, no call sites anywhere in the tree yet, 37/37 green. Blast radius today is zero, which is what makes this stampable.

The construction is sound: len(stmts) != 1 and the non-CreateStmt type assertion are both guarded with clear errors, a missing column reports by name, and reusing createNode.CreateStmt.GetRelation() for the AlterTableStmt means schema-qualified names carry through rather than being reassembled. Deriving the ADD COLUMN from the embedded desired schema instead of hand-maintaining a parallel copy of every column's DDL is clearly the right call — the parallel copy is the version that silently rots.

One thing to carry into the follow-up PR, not this one. The synthesized Def is the ColumnDef node verbatim from the CREATE TABLE, so it carries that column's constraints with it. Constraints that are trivially valid at table-creation time are not all valid as an ALTER TABLE ... ADD COLUMN against a populated table:

• NOT NULL without a DEFAULT fails on any non-empty table (column "x" of relation "y" contains null values). With a default it's fine, and cheap since PG 11.
• PRIMARY KEY / UNIQUE on an existing table with rows can fail on duplicates, and takes a lock while it builds.

Since the stated plan is for a follow-up to "flip the tripwire outcomes to converge under an advisory lock," that follow-up will be feeding this output at live storage tables at startup — where a failure means the pod doesn't come up. Worth deciding there whether to refuse synthesis for the unsafe shapes, or to let it emit and have the converger classify them. Flagging it here mainly so the next reviewer doesn't assume this seam only ever emits DDL that's safe to apply; the function's doc comment currently promises synthesis, not safety, and that's a reasonable contract as long as it's an explicit one.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 9d44c005.

Verdict: the synthesis itself is solid and genuinely inert — nothing here blocks, but the corpus test proves less than it looks like it proves, and that matters for the converger that lands next. I attacked the AST lift with every column shape I could think of and could not make it silently degrade one; the seam is faithful, which is the right property for it to have. The finding is about where the applicability judgment lives, since this PR is the foundation for flipping a startup tripwire from "fail" to "converge".

Findings

1. TestSynthesizePostgresAddColumn_EmbeddedSchema is green for 69 statements that PostgreSQL will reject on a populated table. I ran the corpus over the embedded schema and counted: 69 columns synthesize to ALTER TABLE … ADD COLUMN … NOT NULL with no DEFAULT, which fails with "column contains null values" on any table that already has rows. The same faithfulness lifts PRIMARY KEY, UNIQUE, and REFERENCES … ON DELETE CASCADE into statements that are valid syntax and either fatal or expensive at execution. None of that is wrong for a synthesis seam — deriving the exact declaration is the job, and the motivating case is safe (applies.superseded_by is NOT NULL DEFAULT '', so it synthesizes to something that applies cleanly). The gap is that the corpus test asserts only "parses and round-trips", while its name and the PR's "verifies synthesis succeeds ... for every column of every table" both read as a safety proof. Nothing in this PR names where the "can this actually be applied?" gate lives. #1220 answers that — it adds PostgresAddColumnManualReason right beside this function and refuses the bare NOT NULL case — so the gap is narrower than it first reads, but it is still invisible from here: a contributor who adds a bare NOT NULL column to an embedded schema file sees this corpus test stay green and has nothing in this file pointing at the classifier that will actually judge it. One sentence in the doc comment naming the seam's contract — synthesis is faithful, applicability is decided elsewhere — closes it, and the test's name and the PR's "every column of every table" should say "parses and round-trips" rather than implying more.

2. columnName must already be the parser-folded name, and nothing says so. SynthesizePostgresAddColumn("CREATE TABLE t (Email text)", "Email") returns column "Email" not found, because PostgreSQL folds the unquoted identifier to email before it ever reaches the AST. That is correct behavior, and it pairs exactly with CreateTableColumns, which returns the same folded names — so the intended call site is safe. But the exported doc comment says nothing about it, and "not found" for a column that is visibly right there in the DDL is a rough five minutes for whoever hits it from a hand-written source. One sentence on the contract closes it.

3. (nit) This is the only dialect-named exported function in pkg/ddl. Everything else reaches PostgreSQL behavior through StatementParser / ParserForDialect. It is defensible — MySQL converges through Spirit's diff, so there is no counterpart to put behind an interface method, and the caller necessarily knows its dialect. Worth deciding deliberately now, while there is exactly one, rather than by accretion later.

4. (nit) The corpus test reads statements[0] rather than iterating the CREATE TABLE statements. Harmless today — I checked all 13 files and each holds exactly one CREATE TABLE, so nothing is being skipped. But the PR body's "every column of every table" stops being true the day a schema file gains a second table, and it fails silently rather than loudly.

Action items

1. (Finding 1) Say in the doc comment that this seam is faithful-by-design and that applicability is judged by the classifier converge additive postgresql storage schema drift at startup #1220 adds alongside it, and reword the corpus test's name and the PR's coverage claim to "parses and round-trips" so neither reads as a safety proof.
2. (Finding 2) Document on SynthesizePostgresAddColumn that columnName is matched against the parsed (case-folded) column name, i.e. the values CreateTableColumns returns.
3. (optional) Decide whether the PostgreSQL-specific entry point stays package-level or moves behind the parser seam.
4. (optional) Iterate the CREATE TABLE statements in the corpus test so its coverage claim stays true as schema files grow.

Verified (tried to break, couldn't)

I could not find a column shape that lifts lossily: identity columns, GENERATED ALWAYS AS (…) STORED, COLLATE, CHECK, REFERENCES … ON DELETE CASCADE, UNIQUE, PRIMARY KEY, array types, bigserial, and typmods all reappear in the ALTER with every constraint intact, which is the property that makes the AST lift better than string surgery; table-level shapes are equally clean — IF NOT EXISTS, TEMP, UNLOGGED, PARTITION BY, INHERITS, a trailing semicolon and a leading comment all parse and produce the right statement. Schema qualification and quoted mixed-case identifiers survive deparse byte-exact. Table-level constraints cannot be mistaken for columns, since only ColumnDef nodes are scanned. Every error path returns a real wrapped error rather than a silent zero value, and empty or whitespace-only input reports "got 0" instead of dereferencing a missing statement — no fallback string-splitting anywhere, so the real-parser requirement holds. The "no production caller yet" claim checks out: the only reference to the function in non-test code is its own definition. No test functions were deleted and no assertions weakened in the diff. Build and the pkg/ddl suite pass locally at head, and CI is 20/20 green.

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

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

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

[morgo]

🤖 Re-approving on Morgan's behalf at 615b4f1d (automated review).

The branch was force-pushed past my earlier approval — the old SHA is unreachable, so I re-checked rather than assuming a clean rebase.

The new commit moves ADD COLUMN synthesis behind the parser seam (+25/-0 in parser.go, +7/-3 in postgres_parser.go), which is the right placement: it keeps the synthesis reachable through StatementParser rather than as a PostgreSQL-only entry point, consistent with how CostScalesWithTableSize and CreateIndex sit on the same interface. Still +246/-0 overall, confined to pkg/ddl, no production callers changed. CI green.

Nothing further from me.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/1212, follow-up commit

All four findings fixed: the seam's contract (faithful by design, parser-folded column names) is now stated in its docs, the entry point moved behind StatementParser, and the corpus test claims only what it proves. Severity-ordered; the two optional action items are rows 3 and 4.

#	Concern	Status
1	The corpus test is green for 69 NOT NULL-without-DEFAULT columns PostgreSQL would reject on a populated table; its name and the PR's "verifies synthesis succeeds for every column" read as a safety proof when it only proves round-trip faithfulness	fixed — the StatementParser.SynthesizeAddColumn doc now states the seam is faithful by design and that applicability (whether the DDL is safe to run unattended) is the caller's judgment, made by the classifier #1220 adds; the corpus test is renamed TestPostgresParserSynthesizeAddColumn_EmbeddedSchemaRoundTrips and the PR body's coverage claim now says round-trip faithfulness only
2	columnName must already be the parser-folded name ("Email" → column "Email" not found), and nothing says so	fixed — the doc now states columnName is matched against the parser-folded column name, i.e. the values CreateTableColumns returns
3	(nit) Only dialect-named exported function in pkg/ddl; everything else reaches PostgreSQL behavior through ParserForDialect	fixed — moved behind the seam as StatementParser.SynthesizeAddColumn; the MySQL-family parser returns a not-supported error, matching the CreateIndex precedent
4	(nit) Corpus test reads statements[0], so a schema file gaining a second table would be silently skipped	fixed — the test iterates every statement p.Classify reports as StatementCreateTable and requires at least one per file, so a file with no CREATE TABLE fails loudly

[morgo]

🤖 Re-approving on Morgan's behalf (automated review) — the head moved past my last approval at 615b4f1d, so re-verifying rather than letting the stale stamp stand.

The move is a main merge (88fd597d) and nothing else: the PR's own diff is still exactly +246/-0 across the same four pkg/ddl files, and the two content commits (9d44c005, 615b4f1d) are unchanged. So this is a re-check of the merge, not of the change.

The merge is worth checking rather than waving through, because #1237 landed on main at 19:59Z and touches the same two files — pkg/ddl/parser.go and pkg/ddl/postgres_parser.go. A merge that quietly dropped one side's additions would produce a green-looking branch that silently lost CostScalesWithTableSize. It didn't:

• Both methods are present in the StatementParser interface with their doc comments intact, and both parsers implement both — SynthesizeAddColumn at parser.go:173 / postgres_parser.go:125, CostScalesWithTableSize at parser.go:185 / postgres_parser.go:195, with #1237's alterSpecScalesWithTableSize, alterCmdScalesWithTableSize, addColumnScalesWithTableSize and isConstantExpr all still there.
• The PR diff shows -0 against a merge base that now includes #1237, which is the mechanical confirmation that no main content was removed.
• mergeStateStatus: CLEAN, and all 37 checks pass — including Build and all four lint variants (default, e2e, integration, consumer-module), which is what proves the two additive interface methods didn't strand an implementation under some other build tag.

Re-confirming the safety posture that made this stampable in the first place, since it survives the merge unchanged: the MySQL side is an explicit refusal, not a silent path —

func (tidbStatementParser) SynthesizeAddColumn(string, string) (string, error) {
	return "", fmt.Errorf("ADD COLUMN synthesis is not supported by the MySQL statement parser")
}

— and the PR still wires no callers, so the live MySQL production path is untouched by construction rather than by argument.

My earlier findings stand as written and none of them are affected by the merge. Nothing new to raise.