schemadiff: render the canonical model back to a desired schema file

[Kiran01bm]

Summary

Adds schemadiff.Render: the canonical Model rendered back into a desired-state schema file (one CREATE TABLE plus the model's CREATE INDEX statements). This is the building block for live-schema export — pointing pg-sprite at an existing table and getting a declarative baseline file that diffs to zero against it. The pull CLI command lands in a follow-up PR.

First slice of live-schema export (pull): Render turns an introspected Model into a declarative file that ParseDesired provably admits and that round-trips through IntrospectDesired to an identical model with an empty diff. Serial columns render back to their pseudo-type; any other sequence-backed default fails closed. The pull CLI follows separately.

What

• pkg/schemadiff/render.go — Render(Model) (string, error), reusing the existing columnDef renderer for columns and the server-decompiled Def text for constraints and indexes. All identifiers go through pgx.Identifier.Sanitize().
• The renderer proves its own output admissible by parsing it through statement.ParseDesired before returning — anything a desired file refuses (foreign keys) surfaces as that gate's typed error, so admission rules stay in one place.
• Serial columns render back to serial/bigserial/smallserial when the default is the canonical owned-sequence form; any other sequence-backed default fails closed with ErrUnrenderableDefault (a rendered file could never recreate the sequence it references on the scratch schema).
• Unit tests pin exact rendered text and the refusal cases; integration tests prove the round-trip contract: introspect → render → parse → materialize on scratch → identical model, empty diff — including identity, generated, defaulted, and serial columns, check/unique constraints, and partial/DESC indexes.

Why

Onboarding an existing database to declarative schema management needs a trustworthy baseline: a file whose only proof of correctness is that the engine itself diffs it to zero against the live table. Rendering from the introspected model (server-decompiled types, defaults, constraint and index text) rather than from any AST keeps the execute-and-introspect principle: PostgreSQL remains the canonicalizer on both sides of the round trip.

Before (no export path):
  live table ──Introspect──▶ Model ──▶ (dead end)

After:
  live table ──Introspect──▶ Model ──Render──▶ desired file
                               ▲                    │
                               │              ParseDesired
                          equal, diff = 0           │
                               │                    ▼
                             Model ◀──IntrospectDesired (scratch, rolled back)

[chatgpt-codex-connector]

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

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by his agent. Reviewed at head a4326b7, in a worktree, with every claim tested against a live PostgreSQL: I ran Introspect → Render → ParseDesired → IntrospectDesired → Diff by hand over fixtures the test suite doesn't cover, to find inputs where the round-trip contract holds and the rendered file still misdescribes the table.

Verdict: the round-trip oracle is the right design and the implementation is clean and readable — but both of the proofs Render relies on are structurally blind to omission, and I found four facts that silently disappear while Diff still reports zero changes. One of them is a fact the code's own comment claims to verify. Nothing here is reachable from a CLI today (no pull command yet), so nothing is a live safety gate — but findings 1 and 2 should land before the pull CLI exposes Render to an operator, because the artifact's entire value is that it can be trusted.

Findings

1. Neither ParseDesired admissibility nor diff-to-zero can detect a dropped fact, so four of them get through. The two proofs are blind in the same direction by construction: ParseDesired can only reject what is present in the text — omitting a clause makes the output strictly more admissible — and Diff compares Model to Model, so any fact the model does not carry is absent from both sides and cancels out. I ran four fixtures through the full round trip; every one rendered successfully with models equal = true and diff changes = 0:

live table	rendered as	fact lost
PARTITION BY RANGE (created_at) (relkind p)	plain CREATE TABLE	the partition key — the baseline declares a different kind of object
id bigint DEFAULT nextval('events_id_seq') on a standalone sequence	id bigserial	sequence ownership (finding 2)
name text COLLATE "C"	name text	the collation — changes sort order, and unique/index semantics
CREATE UNLOGGED TABLE	permanent table	crash-safety and replication behavior

The partitioned case is the one I'd fix first: Introspect deliberately admits relkind = 'p' (introspect.go:71), partitioned parents are ordinary in production, and the resulting file would create a plain table if anyone applied it. And this is not a limitation of the declarative grammar — I checked, and ParseDesired happily accepts PARTITION BY, COLLATE and UNLOGGED, and IntrospectDesired materializes all three. The loss is entirely in Model, which is pre-existing and symmetric (harmless) for diff, where both sides drop the same fact. Render is what makes it asymmetric: the output is handed to a human as a description of their live table, so a fact the model can't see becomes a false statement rather than a wash. The fix that matches the package's fail-closed instinct is a positive describability gate in Render — refuse when the target carries something the model provably cannot represent (relkind = 'p', relpersistence <> 'p', any non-default attcollation) — rather than leaning on two proofs that cannot see omissions.

2. serialType says it verifies sequence ownership and actually verifies only the sequence's name — and diff.go already names this exact hazard as unsafe. The doc comment reads "It requires exactly what the serial shorthand produces: an integer-family type, NOT NULL, and a default of nextval on the owned sequence named <table>_<column>_seq." The check is string equality on the default expression; nothing consults pg_depend. Column.SequenceDefault cannot help, because its introspection subquery (introspect.go:96-104) doesn't filter dep.deptype — its own doc says it means "a serial column or a hand-written nextval default." The reachable scenario is the reason standalone sequences exist in the first place — one sequence shared by two tables, which happens to be named after one of them:

CREATE SEQUENCE probe.events_id_seq;                                    -- deptype 'n', owned by nobody
CREATE TABLE probe.events (id bigint NOT NULL DEFAULT nextval('probe.events_id_seq'::regclass), ...);
CREATE TABLE probe.orders (id bigint NOT NULL DEFAULT nextval('probe.events_id_seq'::regclass), ...);

events  ->  RENDERED as  "id" bigserial NOT NULL          # silently declares an OWNED sequence
orders  ->  REFUSED: ... sequence-backed default cannot be rendered as a desired schema

Two tables backed by the same sequence get opposite treatment, and the one that slips through is converted from sharing a sequence to owning a private one — so pulling baselines for both and applying them elsewhere yields two independent sequences and silently breaks the shared-ID invariant the sequence existed to provide. The refusal on orders is itself the evidence that the design intends to catch this; only the name check lets events past. Worth noting the codebase already argued this exact point from the other direction — diff.go:230-236 refuses serial adoption partly because it could "silently bind to an unrelated live sequence of the same name." That is precisely what serialType does. The discriminator is one supported catalog call, which I verified distinguishes all three of my fixtures correctly:

SELECT pg_get_serial_sequence('probe.events', 'id');  -- NULL      (standalone)
SELECT pg_get_serial_sequence('probe.orders', 'id');  -- NULL      (standalone)
SELECT pg_get_serial_sequence('probe.owned',  'id');  -- probe.owned_id_seq

3. Render and pg-sprite fmt disagree on the canonical form of a schema file, on every line. The rendered baseline is the artifact a user checks into git, and the project already ships a canonicalizer for exactly that file. They produce different text from the same input:

-CREATE TABLE "events" (
-    "id" bigint NOT NULL,
-    "name" character varying(50) NOT NULL,
-    CONSTRAINT "events_pkey" PRIMARY KEY (id)
-);
-
+CREATE TABLE events (id bigint NOT NULL, name varchar(50) NOT NULL, CONSTRAINT events_pkey PRIMARY KEY (id));
 CREATE INDEX events_name_idx ON events USING btree (name);

fmt collapses to one line per statement, drops quoting where it isn't required (it does preserve "Order Items"), and prefers type aliases (varchar(50)), while Render is multi-line, always-quoted, and uses the server's format_type names. fmt is idempotent on its own output, so this isn't instability — it is two different definitions of canonical, and running the obvious next command after a pull rewrites the whole file. Whichever way it's resolved, resolving it now is much cheaper than after baselines exist in people's repos; Render's layout is by far the better artifact for review, which argues for fmt moving rather than Render.

4. (nit) A zero-column table renders an empty line between the parentheses. CREATE TABLE t () is legal PostgreSQL, and Render produces CREATE TABLE "nocols" (\n\n); — which ParseDesired accepts and which round-trips to an empty diff, so it is only cosmetic. Skipping the join when defs is empty renders () instead.

Action items

1. (Finding 1) Add an explicit describability gate to Render that refuses a table carrying facts the model cannot represent — partitioned parents (relkind = 'p'), non-permanent tables, and non-default column collations — with typed errors like ErrUnrenderableDefault, so the fail-closed guarantee doesn't depend on two proofs that cannot see omissions. Land this before the pull CLI exposes Render.
2. (Finding 2) Gate serialType on real ownership via pg_get_serial_sequence (or a deptype = 'a' pg_depend edge surfaced as a new OwnedSequence field), and add the standalone-same-name-sequence fixture as an integration test — the current unit test's "shared sequence" case uses a differently named sequence, which is why this passes today. Either fix the doc comment's "owned" claim or make it true.
3. (Finding 3) Decide which canonical form wins for a schema file and make Render and fmt agree, before pulled baselines land in repositories.
4. (optional) (Finding 4) Render () for a zero-column table.

Verified (tried to break, couldn't)

Everything the model does carry survives the round trip, and I pushed on the parts most likely to be subtly wrong. Identifier handling is genuinely safe: every rendered name goes through pgx.Identifier.Sanitize(), and the mixed-case/whitespace/reserved-word fixture ("Order Items", "Item ID", "select") round-trips to an identical model — that test is well chosen, since plain lowercase fixtures would pass even with raw interpolation. Determinism is real, which matters enormously for a file destined for git: columns come back in attnum order and constraints and indexes are ORDER BY name in the introspection queries, so repeated pulls of an unchanged table produce byte-identical output. The admissibility proof is genuinely load-bearing where it can see the problem — a live foreign key surfaces statement.ErrForeignKey from ParseDesired rather than emitting a file the front door would reject, and I confirmed the typed error propagates through the wrap so a caller can branch on it. Index definitions are correctly schema-relative on both sides (pg_get_indexdef always qualifies; introspectIndexes strips it via statement.Qualify(def, "")), so the rendered CREATE INDEX lines match the model they came from. The three serial mappings are each pinned by their own subtest, so a typo in one map entry fails rather than shipping a wrong pseudo-type, and the nullable and non-integer refusals are correct. Reusing columnDef for the non-sequence path is the right call — identity, generated, default and NOT NULL all render from the same code the diff path uses, so the two can't drift. The PR adds three files and deletes nothing, so there is no test-deletion or weakened-assertion surface. go build ./..., go vet ./pkg/schemadiff/... and go test ./pkg/schemadiff/... all pass locally at head; all 12 checks are green across PostgreSQL 14 through 18. No leaks. One process note: the only comment on this PR is the Codex reviewer reporting it is out of usage quota, so no automated review actually ran here.

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

[aparajon]

🤖 Second pass, same head (a4326b7), through the two lenses @aparajon asks pg-sprite changes to be judged on: how easily an outside team adopts this, and the seam an orchestrator embedding the engine consumes. Correctness findings are in the comment above; nothing here blocks.

Lens 1 — OSS adoption

This is the feature that decides whether anyone with an existing database can adopt declarative schema management at all, and it deserves to be framed that way. Every team pg-sprite wants already has tables. The first thing they must do is produce a file that describes what they already have — and today that means hand-writing CREATE TABLE statements against a live schema they only half remember, then discovering the drift when the first diff comes back non-empty. That is where adoption dies, and it dies silently: nobody files an issue saying "I gave up while writing the baseline." pull removes the step entirely, and the reason to be excited about this slice specifically is that it doesn't ask anyone to trust a hand-written translator — the file's correctness is checked by the engine that will consume it. Naming the round-trip as the oracle rather than bolting on a golden-file test suite is the right instinct and it is what makes the feature credible.

The single highest-leverage addition: make the baseline say what it left out. Right now the artifact is silent about its own coverage, and per finding 1 of the correctness comment there are real facts it cannot carry. A human reading a pulled file has no way to know whether it is the whole truth about their table — and the failure mode is the worst kind, because everything looks complete. A short generated header would fix this permanently and cost almost nothing:

-- Generated by pg-sprite from public.events. Verified: diffs to zero against
-- the source table. Not represented: <partitioning | collation | ...>.

That turns the omissions from a trap into a checklist, and it also solves the trust question an evaluator asks in the first ten minutes ("how do I know this is right?") by putting the proof in the artifact instead of in the docs. Once a describability gate exists this header is mostly the empty case, which is exactly when it is most reassuring.

The papercut that will hit in the first five minutes is the fmt divergence (correctness finding 3). The obvious first workflow is pull > schema.sql, then pg-sprite fmt schema.sql because the tool advertises a canonicalizer — and that rewrites every line of the file just produced. Worth resolving before baselines exist in people's repositories rather than after, and worth resolving toward Render's multi-line layout: a baseline for a wide table is a file humans will read in code review for years, and one statement per line is not that file. This is also the moment to notice that a fmt --check CI gate is a very natural thing for an adopting team to add, and it would currently fail on freshly pulled files.

What I'd want in the follow-up PR beyond the CLI wiring: the docs framing. A docs/pull.md that says plainly "point pg-sprite at an existing table, get a declarative file, and the proof it is correct is that the engine diffs it to zero" is the most persuasive page the project could publish for the adoption audience — more than any feature list, because it answers the only question that blocks the first step. And a README line, since pull is the entry point to everything else pg-sprite does.

Lens 2 — the seam an orchestrator consumes

Exporting Render from pkg/schemadiff rather than burying it in the CLI is the right call and consistent with where the project has been heading — diffplan.Plan and now migrate.Run are both libraries with the CLI as an adapter, and baseline generation belongs in the same tier. An orchestrator onboarding a database wants exactly this call: hand it an introspected model, get a file to commit on the user's behalf. The signature is right too — Render(Model) (string, error) is pure, needs no pool, and composes with Introspect however the caller already holds a connection.

The error taxonomy is the part that needs to grow with the gates, and it is easier to get right now than later. ErrUnrenderableDefault is a good typed sentinel and the foreign-key case correctly propagates statement.ErrForeignKey through the wrap, so an embedder can already distinguish "this table has a default I can't express" from "this table has a shape the declarative front door refuses." Both of those map to different messages an orchestrator would show a user. The refusals finding 1 asks for — partitioned parent, non-default collation, unlogged — should each arrive as their own sentinel rather than folded into a generic error, because an orchestrator's whole job at that point is telling a team which unsupported thing to deal with before onboarding. A single ErrUnrenderable covering four causes would push them back to string-matching the message.

One seam question the follow-up should answer deliberately: what does an orchestrator do with a table it cannot render? For a CLI the answer is easy — print the refusal and let a human decide. For an automated onboarding flow over dozens of tables, refusing one table shouldn't abort the batch, and the useful output is "here are the 40 baselines, and here are the 3 tables that need a human, with the reason for each." That is a shape decision about the layer above Render (a per-table result rather than a fail-fast loop), and it is worth deciding before the CLI hard-codes the fail-fast version. Render itself is correctly scoped to one table; this is about what wraps it.

Small note in favor of the current design, since it is the sort of thing that gets refactored away later: proving admissibility by parsing the output through ParseDesired before returning — rather than reasoning about what the grammar accepts — keeps the admission rules in exactly one place and means the renderer cannot drift from the front door as the grammar evolves. It is a little unusual to see a renderer re-parse its own output, and the doc comment explains why well. Worth keeping that comment intact through any future refactor; it is the kind of rationale that reads like belt-and-braces until the one time it saves you.

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

[aparajon]

🤖 Approving: the round-trip oracle is the right design, the implementation is clean, and nothing here is reachable from a CLI yet. Two correctness items should land before the pull CLI exposes Render to an operator — a describability gate for facts the model cannot carry (partitioning, collation, unlogged), and real sequence-ownership verification in serialType. Details in the two comments above.

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

[Kiran01bm]

Review response from Kiran's (@Kiran01bm) code review assessment agent (Amp / Claude Opus 4.5)

Summary: both load-bearing correctness findings (describability gate, sequence ownership) plus the zero-column nit are fixed in the stacked follow-up #55; the fmt/Render canonical-form decision and the pull-CLI adoption items (provenance header, docs framing, batch seam) are tracked as internal follow-ups to land with or before the pull CLI wiring.

#	Concern	Status	Explanation
A1	Render's two proofs cannot see omission: partitioned, unlogged, collated, and standalone-sequence facts silently vanish from the baseline	fixed	#55 adds a positive describability gate — partitioned parents/partitions and FK-involved tables were already refused there; unlogged tables (ErrUnrenderableUnlogged) and explicit column collations (ErrUnrenderableCollation) now refuse too, and Diff fails closed on persistence and collation deltas so none of the four diffs to zero. Integration tests cover each fixture from the review.
A2	serialType claims ownership but checks only the sequence name — a shared standalone sequence named <table>_<column>_seq renders as serial, silently privatizing it	fixed	#55 adds Column.SequenceOwned, introspected from the pg_depend OWNED BY edge (deptype 'a' — the same fact pg_get_serial_sequence reads), and serialType requires it. The review's two-tables-one-sequence fixture is now an integration test: both tables refuse; a genuine bigserial still renders.
A4	Zero-column table renders an empty line between the parentheses	fixed	#55 renders CREATE TABLE "nocols" ();, with a unit test.
A3	Render and fmt disagree on the canonical form of a schema file — fmt after a pull rewrites every line	deferred	Tracked as an internal follow-up, merged with the existing fmt pretty-printing item: we agree fmt should move to Render's multi-line reviewable layout, and it lands before pulled baselines are promoted as a workflow.
L1	Pulled baselines should carry a generated provenance header stating what the model does not represent	deferred	Tracked as an internal follow-up to land with the pull CLI wiring; it is blocked on comment preservation (ParseDesired/fmt refuse commented input today), which is itself a tracked item.
L3	docs/pull.md framing ("declarative file, proven by diff-to-zero") plus a README entry-point line	deferred	Agreed; belongs to the pull CLI follow-up PR where the command exists to document. Tracked as an internal follow-up.
L4	Refusals should each arrive as their own typed sentinel, not a folded generic error	fixed	Already the shape: ErrUnrenderablePartition, ErrUnrenderableForeignKey (plus propagated statement.ErrForeignKey), and now ErrUnrenderableUnlogged / ErrUnrenderableCollation — an orchestrator can branch on each cause without string-matching.
L5	Batch onboarding wants per-table results, not a fail-fast loop — decide before the CLI hard-codes fail-fast	deferred	Agreed and recorded as a shape decision for the pull CLI follow-up; Render stays single-table, the per-table result type lives in the layer above it.
L6	Keep the re-parse-own-output rationale comment through future refactors	no action	Acknowledged — the comment stays.