From 1aba8712040f34bb87e93ecfbba6738e05b648b5 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 20 Aug 2026 18:33:20 +1000 Subject: [PATCH 1/3] schemadiff: refuse rendering partitioned tables and FK-involved tables The single-table model carries no partition topology and no incoming foreign-key relationships, so a rendered baseline of either would look complete while being silently wrong. Fail closed with typed errors instead, and document the declarative-model boundaries (FKs in either direction, partitioned tables, non-table objects) in limitations.md and the README so the v1 capability surface is explicit. --- AGENTS.md | 8 +++ README.md | 19 +++++ docs/limitations.md | 14 ++++ pkg/schemadiff/diff.go | 9 +++ pkg/schemadiff/diff_test.go | 15 ++++ pkg/schemadiff/introspect.go | 44 +++++++++++- pkg/schemadiff/render.go | 25 +++++++ pkg/schemadiff/render_integration_test.go | 84 +++++++++++++++++++++++ pkg/schemadiff/render_test.go | 26 +++++++ pkg/schemadiff/schemadiff.go | 13 ++++ 10 files changed, 254 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 12dce92..e4c5c81 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,6 +135,14 @@ is a smoke tour of the built binary, not a second test suite. - Never reference internal company details (cluster names, hostnames, org names) in code, comments, commits, or PRs — this is a public repo. +## Capability statement + +[docs/limitations.md](docs/limitations.md) and the README's "What pg-sprite does not do yet" +section are the public capability statement. Any PR that adds, removes, or re-scopes a +capability — a planner route, a refusal, a verdict outcome, a declarative-model boundary — +updates both in the same PR, and every release sweeps them against the shipped behavior +before tagging. + ## Logging and observability - **stdout is the product's output; diagnostics go to stderr.** Command results (verdicts, diff --git a/README.md b/README.md index 0af0a34..4f90cb0 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,25 @@ build plan live in [docs/](docs/) — start with [docs/README.md](docs/README.md); the vision — what pg-sprite is and is not — is [docs/vision.md](docs/vision.md). +## What pg-sprite does not do yet + +So expectations are set before you point it at a database — the full list +with per-item behavior is **[docs/limitations.md](docs/limitations.md)**; +wherever an operation meets a boundary below, it fails closed with a typed +refusal — never a silently wrong or incomplete result: + +- **Copy-and-swap** (genuine table rewrites) is not yet available — those + changes refuse rather than fall through to a blocking rewrite. +- **Foreign keys** are out of the declarative model in either direction: + desired files cannot declare them, and export refuses both a table that + carries foreign keys and a table that other tables reference. FK DDL + still works through the statement front door (`NOT VALID` + `VALIDATE`). +- **Partitioned tables** support in-place statement changes, but cannot be + expressed in or exported to desired files. +- **Non-table objects** — views, standalone sequences, enums, domains, + extensions, functions, triggers — are outside the declarative model, + which covers one ordinary table plus its indexes per file. + The codebase is partitioned into a small safety-critical core and a periphery — **[SAFETY.md](SAFETY.md)** says which packages are which and the rules that apply inside the core. Read it before changing anything under diff --git a/docs/limitations.md b/docs/limitations.md index 5a2998c..7d26b47 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -9,3 +9,17 @@ guarantees. These are current capability boundaries, not escape hatches: | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | | Copy-and-swap | The copy-and-swap backend is not yet available. Statements that require it route to `refuse`; pg-sprite never falls through to a blocking rewrite. | + +## Declarative model boundaries + +The declarative front door (desired-state files, `diff`, and schema export) +models **one ordinary table plus its indexes**. These are the boundaries of +that model today. Wherever an operation meets a boundary, it fails closed +with a typed refusal — never a silently wrong or incomplete result: + +| Not modeled | Current behavior | +| --- | --- | +| Foreign keys (either direction) | Unsupported in the declarative model, in both directions. A desired file cannot declare a `REFERENCES` clause (refused at parse), and export refuses both sides of a foreign-key relationship: a table whose definition carries foreign-key constraints surfaces the parse gate's typed error, and a table that other tables reference refuses with its own typed error — a single-table baseline cannot carry incoming foreign-key topology, so rendering one would silently drop the relationship. Foreign-key **DDL is still supported through the statement front door** (`ADD FOREIGN KEY` routes to the online `NOT VALID` + `VALIDATE` sequence). Because a desired file can never declare a foreign key, `diff` on a live table that carries one plans a **destructive** `DROP CONSTRAINT` for it — gated like every destructive change, never auto-executed — and incoming foreign keys are invisible to a single-table diff entirely, so tables participating in foreign-key relationships in either direction should not be managed declaratively yet. | +| Partitioned tables | Partitioned parents and their partitions are introspectable, and the statement front door supports in-place changes on them (see the partitioned-parent rows above), but they cannot be expressed in or exported to a desired file: the model captures the partition key and attachment only to refuse — it does not carry partition bounds or the parent/partition topology. A partitioning mismatch between live and desired is a typed `diff` refusal, never a zero diff. | +| Non-table objects | Views, materialized views, standalone sequences, enums, domains, extensions, functions, and triggers are outside the model. A serial column's owned sequence is the one exception: it round-trips through the `serial` pseudo-types. A column may *use* an unmanaged type (an enum, a domain) — the type text round-trips — but the type's definition is not managed. | +| Multiple tables per file | A desired file is single-table scoped: exactly one `CREATE TABLE` plus `CREATE INDEX` statements on it. Multi-table schemas are managed as one file per table. | diff --git a/pkg/schemadiff/diff.go b/pkg/schemadiff/diff.go index 2d06f4e..652f42b 100644 --- a/pkg/schemadiff/diff.go +++ b/pkg/schemadiff/diff.go @@ -99,6 +99,15 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if live.Table != desired.Table { return nil, fmt.Errorf("%w: %q vs %q", ErrDifferentTables, live.Table, desired.Table) } + // Partitioning is table identity, not an alterable attribute: no ALTER + // can add, remove, or change a partition key or a partition attachment + // in place, so a mismatch fails closed instead of diffing to silence. + if live.PartitionKey != desired.PartitionKey { + return nil, fmt.Errorf("partition key %q vs %q: %w", live.PartitionKey, desired.PartitionKey, ErrUnsupportedChange) + } + if live.IsPartition != desired.IsPartition { + return nil, fmt.Errorf("partition attachment differs between live and desired: %w", ErrUnsupportedChange) + } table := pgx.Identifier{schema, live.Table}.Sanitize() liveCols := columnsByName(live.Columns) diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go index d15dcb8..fadad2a 100644 --- a/pkg/schemadiff/diff_test.go +++ b/pkg/schemadiff/diff_test.go @@ -45,6 +45,21 @@ func TestDiffRefusesDifferentTables(t *testing.T) { require.ErrorIs(t, err, ErrDifferentTables) } +// Partitioning is table identity: no ALTER can change a partition key or a +// partition attachment in place, so a mismatch is a typed refusal — never a +// silent zero diff. +func TestDiffRefusesPartitioningMismatch(t *testing.T) { + partitioned := base() + partitioned.PartitionKey = "RANGE (id)" + _, err := Diff("public", base(), partitioned) + require.ErrorIs(t, err, ErrUnsupportedChange) + + child := base() + child.IsPartition = true + _, err = Diff("public", child, base()) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + func TestDiffAddColumn(t *testing.T) { desired := base() desired.Columns = append(desired.Columns, Column{ diff --git a/pkg/schemadiff/introspect.go b/pkg/schemadiff/introspect.go index 8c6867d..1d6eb25 100644 --- a/pkg/schemadiff/introspect.go +++ b/pkg/schemadiff/introspect.go @@ -57,11 +57,15 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model var oid uint32 var relkind string + var isPartition bool + var partitionKey string err := tx.QueryRow(ctx, ` - SELECT c.oid, c.relkind::text + SELECT c.oid, c.relkind::text, c.relispartition, + COALESCE(pg_get_partkeydef(c.oid), '') FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = $1 AND c.relname = $2`, schema, table).Scan(&oid, &relkind) + WHERE n.nspname = $1 AND c.relname = $2`, schema, table). + Scan(&oid, &relkind, &isPartition, &partitionKey) if errors.Is(err, pgx.ErrNoRows) { return Model{}, fmt.Errorf("%s.%s: %w", schema, table, ErrTableNotFound) } @@ -72,7 +76,7 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model return Model{}, fmt.Errorf("%s.%s has relkind %q: %w", schema, table, relkind, ErrNotTable) } - m := Model{Table: table} + m := Model{Table: table, PartitionKey: partitionKey, IsPartition: isPartition} if m.Columns, err = introspectColumns(ctx, tx, oid); err != nil { return Model{}, fmt.Errorf("introspect columns of %s.%s: %w", schema, table, err) } @@ -82,6 +86,9 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model if m.Indexes, err = introspectIndexes(ctx, tx, oid); err != nil { return Model{}, fmt.Errorf("introspect indexes of %s.%s: %w", schema, table, err) } + if m.ReferencedBy, err = introspectReferencedBy(ctx, tx, oid); err != nil { + return Model{}, fmt.Errorf("introspect incoming foreign keys of %s.%s: %w", schema, table, err) + } return m, nil } @@ -158,6 +165,37 @@ func introspectConstraints(ctx context.Context, tx pgx.Tx, oid uint32) ([]Constr return cons, nil } +// introspectReferencedBy reads the incoming foreign keys: constraints on +// other tables whose referenced relation is this table, as +// "table.constraint" strings. A self-referential foreign key is excluded — +// it is already carried as one of the table's own constraints. These are +// not part of this table's definition; the model carries them only for the +// renderer to refuse on. +func introspectReferencedBy(ctx context.Context, tx pgx.Tx, oid uint32) ([]string, error) { + rows, err := tx.Query(ctx, ` + SELECT c.relname || '.' || con.conname + FROM pg_constraint con + JOIN pg_class c ON c.oid = con.conrelid + WHERE con.confrelid = $1 AND con.contype = 'f' AND con.conrelid <> $1 + ORDER BY c.relname, con.conname`, oid) + if err != nil { + return nil, fmt.Errorf("query incoming foreign keys: %w", err) + } + defer rows.Close() + var refs []string + for rows.Next() { + var ref string + if err := rows.Scan(&ref); err != nil { + return nil, fmt.Errorf("scan incoming foreign key: %w", err) + } + refs = append(refs, ref) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read incoming foreign keys: %w", err) + } + return refs, nil +} + // introspectIndexes reads the non-constraint indexes as server-decompiled // CREATE INDEX statements. Constraint-backed indexes (primary key, unique // constraint, exclusion) are represented by their constraint instead. diff --git a/pkg/schemadiff/render.go b/pkg/schemadiff/render.go index ec193a0..6887fa7 100644 --- a/pkg/schemadiff/render.go +++ b/pkg/schemadiff/render.go @@ -19,6 +19,22 @@ import ( // truncated sequence name, a nullable column — must be resolved by hand. var ErrUnrenderableDefault = errors.New("sequence-backed default cannot be rendered as a desired schema") +// ErrUnrenderablePartition is returned for a partitioned parent or a +// partition. The model does not carry partition bounds or the +// parent/partition topology, so a rendered file would silently lose the +// PARTITION BY clause or the partition attachment — the renderer refuses +// instead of emitting a wrong baseline. +var ErrUnrenderablePartition = errors.New("partitioned tables cannot be rendered as a desired schema") + +// ErrUnrenderableForeignKey is returned when other tables reference this +// one with foreign keys. A desired file cannot declare foreign keys, so +// the single-table model carries no incoming foreign-key topology — a +// rendered baseline would look complete while silently dropping the +// table's relationships. The renderer refuses instead. Outgoing foreign +// keys are refused separately by the desired-file grammar +// (statement.ErrForeignKey). +var ErrUnrenderableForeignKey = errors.New("tables referenced by foreign keys cannot be rendered as a desired schema") + // Render renders the canonical model into a desired-state schema file: one // CREATE TABLE followed by the model's CREATE INDEX statements. The output // is proven admissible by parsing it through statement.ParseDesired before @@ -28,6 +44,15 @@ var ErrUnrenderableDefault = errors.New("sequence-backed default cannot be rende // the table it came from yields no changes — the round-trip contract the // integration tests enforce. func Render(m Model) (string, error) { + if m.PartitionKey != "" { + return "", fmt.Errorf("render table %q: partitioned parent (PARTITION BY %s): %w", m.Table, m.PartitionKey, ErrUnrenderablePartition) + } + if m.IsPartition { + return "", fmt.Errorf("render table %q: partition of a partitioned parent: %w", m.Table, ErrUnrenderablePartition) + } + if len(m.ReferencedBy) != 0 { + return "", fmt.Errorf("render table %q: referenced by foreign keys (%s): %w", m.Table, strings.Join(m.ReferencedBy, ", "), ErrUnrenderableForeignKey) + } defs := make([]string, 0, len(m.Columns)+len(m.Constraints)) for _, c := range m.Columns { def, err := renderColumnDef(m.Table, c) diff --git a/pkg/schemadiff/render_integration_test.go b/pkg/schemadiff/render_integration_test.go index e1539b7..50b1025 100644 --- a/pkg/schemadiff/render_integration_test.go +++ b/pkg/schemadiff/render_integration_test.go @@ -66,6 +66,35 @@ func TestRenderRoundTripsLiveTable(t *testing.T) { assert.Empty(t, changes, "diffing a table against its own rendering must yield no changes") } +// Quoted identifiers round-trip: a table whose name carries whitespace and +// mixed case, columns that are mixed-case and a reserved word, and a +// mixed-case constraint all force every identifier the renderer emits +// through real quoting — a Sanitize call replaced with raw interpolation +// would produce a file this test refuses to parse or materialize. +func TestRenderRoundTripsQuotedIdentifiers(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + for _, ddl := range []string{ + fmt.Sprintf(`CREATE TABLE %s."Order Items" ( + "ID" bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY, + "User ID" bigint NOT NULL, + "select" text, + CONSTRAINT "User Positive" CHECK ("User ID" > 0) + )`, schema), + fmt.Sprintf(`CREATE INDEX "Order Items_User_idx" ON %s."Order Items" ("User ID")`, schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + live, desired, changes := roundTrip(t, pool, schema, "Order Items") + assert.Equal(t, live, desired, "rendered output must introspect back to the identical model") + assert.Empty(t, changes, "diffing a table against its own rendering must yield no changes") +} + // A serial table round-trips through the serial pseudo-type: the rendered // file recreates the owned sequence on the scratch schema and both sides // decompile the default identically. @@ -111,6 +140,52 @@ func TestRenderRoundTripsQuotedIdentifiers(t *testing.T) { assert.Empty(t, changes, "diffing a table against its own rendering must yield no changes") } +// Partitioned parents and their partitions are introspectable (the +// statement front door supports in-place changes on them) but refuse to +// render: the model captures the partition key and attachment exactly so +// the refusal — and the diff guard against a partitioning mismatch — fire +// on real catalogs, never silently. +func TestRenderRefusesLivePartitionedTables(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + for _, ddl := range []string{ + fmt.Sprintf("CREATE TABLE %s.metrics (id bigint NOT NULL, day date NOT NULL) PARTITION BY RANGE (day)", schema), + fmt.Sprintf("CREATE TABLE %s.metrics_p1 PARTITION OF %s.metrics FOR VALUES FROM ('2026-01-01') TO ('2026-02-01')", schema, schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + parent, err := schemadiff.Introspect(t.Context(), pool, schema, "metrics") + require.NoError(t, err) + assert.Equal(t, "RANGE (day)", parent.PartitionKey) + _, err = schemadiff.Render(parent) + require.ErrorIs(t, err, schemadiff.ErrUnrenderablePartition) + + child, err := schemadiff.Introspect(t.Context(), pool, schema, "metrics_p1") + require.NoError(t, err) + assert.True(t, child.IsPartition) + _, err = schemadiff.Render(child) + require.ErrorIs(t, err, schemadiff.ErrUnrenderablePartition) + + // The diff guard closes the partition-blind hole end to end: a desired + // file declaring PARTITION BY against a plain live table must refuse, + // not diff to zero. + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.plain (id bigint NOT NULL, day date NOT NULL)", schema)) + require.NoError(t, err) + ds, err := statement.ParseDesired("CREATE TABLE plain (id bigint NOT NULL, day date NOT NULL) PARTITION BY RANGE (day)") + require.NoError(t, err) + livePlain, err := schemadiff.Introspect(t.Context(), pool, schema, "plain") + require.NoError(t, err) + desiredPartitioned, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + _, err = schemadiff.Diff(schema, livePlain, desiredPartitioned) + require.ErrorIs(t, err, schemadiff.ErrUnsupportedChange) +} + // A live table with a foreign key cannot be rendered: the desired-file // grammar refuses foreign keys, and the renderer surfaces that gate's typed // error rather than emitting a file the front door would reject. @@ -132,4 +207,13 @@ func TestRenderRefusesLiveForeignKey(t *testing.T) { require.NoError(t, err) _, err = schemadiff.Render(live) require.ErrorIs(t, err, statement.ErrForeignKey) + + // The referenced side refuses too: incoming foreign keys are not part + // of this table's own definition, so a rendered baseline of it would + // silently drop the relationship. + referenced, err := schemadiff.Introspect(t.Context(), pool, schema, "users") + require.NoError(t, err) + assert.Equal(t, []string{"orders.orders_user_id_fkey"}, referenced.ReferencedBy) + _, err = schemadiff.Render(referenced) + require.ErrorIs(t, err, schemadiff.ErrUnrenderableForeignKey) } diff --git a/pkg/schemadiff/render_test.go b/pkg/schemadiff/render_test.go index b97adea..3fd2a34 100644 --- a/pkg/schemadiff/render_test.go +++ b/pkg/schemadiff/render_test.go @@ -96,6 +96,21 @@ func TestRenderRefusesUnrenderableSequenceDefaults(t *testing.T) { } } +// Partitioned parents and partitions refuse to render: the model carries no +// partition bounds or topology, so a rendered file would be a silently +// wrong baseline. +func TestRenderRefusesPartitionedTables(t *testing.T) { + parent := base() + parent.PartitionKey = "RANGE (id)" + _, err := Render(parent) + require.ErrorIs(t, err, ErrUnrenderablePartition) + + child := base() + child.IsPartition = true + _, err = Render(child) + require.ErrorIs(t, err, ErrUnrenderablePartition) +} + // The renderer proves its own output admissible through ParseDesired, so a // model carrying what a desired file refuses surfaces that gate's typed // error — a foreign key is the canonical case. @@ -109,3 +124,14 @@ func TestRenderRefusesForeignKey(t *testing.T) { _, err := Render(m) require.ErrorIs(t, err, statement.ErrForeignKey) } + +// A table referenced by other tables' foreign keys refuses to render: the +// single-table model cannot carry incoming foreign-key topology, so a +// rendered baseline would silently drop the table's relationships. +func TestRenderRefusesIncomingForeignKey(t *testing.T) { + m := base() + m.ReferencedBy = []string{"orders.orders_user_id_fkey"} + + _, err := Render(m) + require.ErrorIs(t, err, ErrUnrenderableForeignKey) +} diff --git a/pkg/schemadiff/schemadiff.go b/pkg/schemadiff/schemadiff.go index 6a47e4c..fa2321f 100644 --- a/pkg/schemadiff/schemadiff.go +++ b/pkg/schemadiff/schemadiff.go @@ -76,6 +76,19 @@ type Index struct { type Model struct { // Table is the unqualified table name. Table string + // PartitionKey is the server-decompiled partition key definition + // (pg_get_partkeydef), e.g. "RANGE (created_at)" — empty for a + // non-partitioned table. + PartitionKey string + // IsPartition reports that the table is itself a partition of a + // partitioned parent (pg_class.relispartition). + IsPartition bool + // ReferencedBy lists the incoming foreign keys — constraints on other + // tables that reference this one — as "table.constraint" strings, in + // that order. Incoming foreign keys are not part of this table's own + // definition and cannot be expressed in a desired file, so the model + // carries them only for the renderer to refuse on. + ReferencedBy []string // Columns are the table's columns in attribute order. Columns []Column // Constraints are the table constraints, name-sorted. From fcd7d065df37fd134f5a1b44614ae261bf03c3a4 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 20 Aug 2026 19:51:47 +1000 Subject: [PATCH 2/3] schemadiff: exclude partition clones from ReferencedBy A foreign key on a partitioned referencing table is mirrored onto every partition in pg_constraint; without the conislocal filter (already used by introspectConstraints) ReferencedBy reported one entry per partition clone, and a partitioned self-referential FK leaked its clones past the self-reference exclusion. Tests pin both cases; ErrUnsupportedChange's doc comment now covers its partition-difference call sites. --- pkg/schemadiff/diff.go | 24 +++- pkg/schemadiff/introspect.go | 49 +++++-- pkg/schemadiff/render_integration_test.go | 162 ++++++++++++++++++++++ 3 files changed, 225 insertions(+), 10 deletions(-) diff --git a/pkg/schemadiff/diff.go b/pkg/schemadiff/diff.go index 652f42b..72bf6e8 100644 --- a/pkg/schemadiff/diff.go +++ b/pkg/schemadiff/diff.go @@ -10,8 +10,11 @@ import ( ) // ErrUnsupportedChange is returned when converging live onto desired would -// need a change the engine does not derive (identity or generation changes -// on an existing column). The caller surfaces it; nothing is guessed. +// need a change the engine does not derive: identity, generation, or +// collation changes on an existing column, a persistence (unlogged) +// difference, or a partitioning difference (partition key or partition +// attachment) — table identity that no ALTER converges. The caller +// surfaces it; nothing is guessed. var ErrUnsupportedChange = errors.New("unsupported schema change") // ErrDifferentTables is returned when the two models describe different @@ -108,6 +111,12 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if live.IsPartition != desired.IsPartition { return nil, fmt.Errorf("partition attachment differs between live and desired: %w", ErrUnsupportedChange) } + // Persistence is convergeable only by a full table rewrite (SET LOGGED + // / SET UNLOGGED), which the engine does not derive — a mismatch fails + // closed instead of diffing to silence. + if live.Unlogged != desired.Unlogged { + return nil, fmt.Errorf("table persistence (unlogged) differs between live and desired: %w", ErrUnsupportedChange) + } table := pgx.Identifier{schema, live.Table}.Sanitize() liveCols := columnsByName(live.Columns) @@ -166,6 +175,11 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if col.SequenceDefault { return nil, fmt.Errorf("%w: column %q has a sequence-backed default (serial); the plan cannot create its sequence", ErrUnsupportedChange, col.Name) } + // columnDef carries no COLLATE clause, so emitting the add + // would silently drop the collation — refuse instead. + if col.Collation != "" { + return nil, fmt.Errorf("%w: column %q has an explicit collation (%s); the plan cannot carry it", ErrUnsupportedChange, col.Name, col.Collation) + } changes = append(changes, Change{ SQL: "ALTER TABLE " + table + " ADD COLUMN " + columnDef(col), Kind: ChangeAddColumn, @@ -224,6 +238,12 @@ func alterColumnChanges(table string, live, desired Column) ([]Change, error) { if live.Generated != desired.Generated { return nil, fmt.Errorf("%w: column %q generated change", ErrUnsupportedChange, live.Name) } + // A collation change rewrites the column and rebuilds its indexes — + // the engine does not derive it, so a delta fails closed instead of + // diffing to silence. + if live.Collation != desired.Collation { + return nil, fmt.Errorf("%w: column %q collation change (%q vs %q)", ErrUnsupportedChange, live.Name, live.Collation, desired.Collation) + } if desired.Generated && (live.Type != desired.Type || live.Default != desired.Default) { return nil, fmt.Errorf("%w: column %q generation expression or type change", ErrUnsupportedChange, live.Name) } diff --git a/pkg/schemadiff/introspect.go b/pkg/schemadiff/introspect.go index 1d6eb25..ccb4799 100644 --- a/pkg/schemadiff/introspect.go +++ b/pkg/schemadiff/introspect.go @@ -56,16 +56,16 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model } var oid uint32 - var relkind string + var relkind, persistence string var isPartition bool var partitionKey string err := tx.QueryRow(ctx, ` - SELECT c.oid, c.relkind::text, c.relispartition, + SELECT c.oid, c.relkind::text, c.relpersistence::text, c.relispartition, COALESCE(pg_get_partkeydef(c.oid), '') FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace WHERE n.nspname = $1 AND c.relname = $2`, schema, table). - Scan(&oid, &relkind, &isPartition, &partitionKey) + Scan(&oid, &relkind, &persistence, &isPartition, &partitionKey) if errors.Is(err, pgx.ErrNoRows) { return Model{}, fmt.Errorf("%s.%s: %w", schema, table, ErrTableNotFound) } @@ -76,7 +76,7 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model return Model{}, fmt.Errorf("%s.%s has relkind %q: %w", schema, table, relkind, ErrNotTable) } - m := Model{Table: table, PartitionKey: partitionKey, IsPartition: isPartition} + m := Model{Table: table, PartitionKey: partitionKey, IsPartition: isPartition, Unlogged: persistence == "u"} if m.Columns, err = introspectColumns(ctx, tx, oid); err != nil { return Model{}, fmt.Errorf("introspect columns of %s.%s: %w", schema, table, err) } @@ -94,6 +94,12 @@ func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model // introspectColumns reads the canonical column list: server-formatted types // and server-decompiled default/generation expressions, in attribute order. +// Two dependency facts ride along for each sequence-backed default: that +// the default depends on a sequence at all (the pg_attrdef edge), and that +// the sequence is owned by this exact column (the OWNED BY edge, deptype +// 'a') — the discriminator between a serial column and a hand-written +// nextval on a standalone or shared sequence. An explicit column collation +// is read only when it differs from the type's default collation. func introspectColumns(ctx context.Context, tx pgx.Tx, oid uint32) ([]Column, error) { rows, err := tx.Query(ctx, ` SELECT a.attname, @@ -109,6 +115,29 @@ func introspectColumns(ctx context.Context, tx pgx.Tx, oid uint32) ([]Column, er AND dep.refclassid = 'pg_class'::regclass LIMIT 1 ), false), + COALESCE(( + SELECT true + FROM pg_depend dep + JOIN pg_class s ON s.oid = dep.refobjid AND s.relkind = 'S' + JOIN pg_depend own ON own.classid = 'pg_class'::regclass + AND own.objid = s.oid + AND own.refclassid = 'pg_class'::regclass + AND own.refobjid = a.attrelid + AND own.refobjsubid = a.attnum + AND own.deptype = 'a' + WHERE dep.classid = 'pg_attrdef'::regclass + AND dep.objid = d.oid + AND dep.refclassid = 'pg_class'::regclass + LIMIT 1 + ), false), + COALESCE(( + SELECT quote_ident(cn.nspname) || '.' || quote_ident(col.collname) + FROM pg_collation col + JOIN pg_namespace cn ON cn.oid = col.collnamespace + JOIN pg_type t ON t.oid = a.atttypid + WHERE col.oid = a.attcollation + AND a.attcollation <> t.typcollation + ), ''), a.attidentity::text, a.attgenerated::text FROM pg_attribute a @@ -123,7 +152,7 @@ func introspectColumns(ctx context.Context, tx pgx.Tx, oid uint32) ([]Column, er for rows.Next() { var c Column var identity, generated string - if err := rows.Scan(&c.Name, &c.Type, &c.NotNull, &c.Default, &c.SequenceDefault, &identity, &generated); err != nil { + if err := rows.Scan(&c.Name, &c.Type, &c.NotNull, &c.Default, &c.SequenceDefault, &c.SequenceOwned, &c.Collation, &identity, &generated); err != nil { return nil, fmt.Errorf("scan column: %w", err) } c.Identity = Identity(identity) @@ -168,15 +197,19 @@ func introspectConstraints(ctx context.Context, tx pgx.Tx, oid uint32) ([]Constr // introspectReferencedBy reads the incoming foreign keys: constraints on // other tables whose referenced relation is this table, as // "table.constraint" strings. A self-referential foreign key is excluded — -// it is already carried as one of the table's own constraints. These are -// not part of this table's definition; the model carries them only for the -// renderer to refuse on. +// it is already carried as one of the table's own constraints. Partition +// clones are excluded too (conislocal, as in introspectConstraints): a +// foreign key on a partitioned referencing table is mirrored onto every +// partition, and those mirrors are the same relationship, not independent +// ones. These are not part of this table's definition; the model carries +// them only for the renderer to refuse on. func introspectReferencedBy(ctx context.Context, tx pgx.Tx, oid uint32) ([]string, error) { rows, err := tx.Query(ctx, ` SELECT c.relname || '.' || con.conname FROM pg_constraint con JOIN pg_class c ON c.oid = con.conrelid WHERE con.confrelid = $1 AND con.contype = 'f' AND con.conrelid <> $1 + AND con.conislocal ORDER BY c.relname, con.conname`, oid) if err != nil { return nil, fmt.Errorf("query incoming foreign keys: %w", err) diff --git a/pkg/schemadiff/render_integration_test.go b/pkg/schemadiff/render_integration_test.go index 50b1025..9b20e39 100644 --- a/pkg/schemadiff/render_integration_test.go +++ b/pkg/schemadiff/render_integration_test.go @@ -217,3 +217,165 @@ func TestRenderRefusesLiveForeignKey(t *testing.T) { _, err = schemadiff.Render(referenced) require.ErrorIs(t, err, schemadiff.ErrUnrenderableForeignKey) } + +// A foreign key on a partitioned referencing table is cloned onto every +// partition in pg_constraint; ReferencedBy must carry the one real +// relationship, not a row per partition clone. +func TestIntrospectReferencedByCollapsesPartitionClones(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + for _, ddl := range []string{ + fmt.Sprintf("CREATE TABLE %s.t (id bigint PRIMARY KEY)", schema), + fmt.Sprintf("CREATE TABLE %s.pt (id bigint, day date NOT NULL, t_id bigint REFERENCES %s.t(id)) PARTITION BY RANGE (day)", schema, schema), + fmt.Sprintf("CREATE TABLE %s.pt_1 PARTITION OF %s.pt FOR VALUES FROM ('2026-01-01') TO ('2026-02-01')", schema, schema), + fmt.Sprintf("CREATE TABLE %s.pt_2 PARTITION OF %s.pt FOR VALUES FROM ('2026-02-01') TO ('2026-03-01')", schema, schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + live, err := schemadiff.Introspect(t.Context(), pool, schema, "t") + require.NoError(t, err) + assert.Equal(t, []string{"pt.pt_t_id_fkey"}, live.ReferencedBy) + _, err = schemadiff.Render(live) + require.ErrorIs(t, err, schemadiff.ErrUnrenderableForeignKey) + assert.NotContains(t, err.Error(), "pt_1") +} + +// A self-referential foreign key is the table's own constraint, not an +// incoming reference — ReferencedBy excludes it whether the table is plain +// or partitioned (where every partition carries a clone of the root +// constraint). Rendering still refuses, but on the outgoing foreign key in +// the table's own definition, not on a phantom incoming one. +func TestIntrospectReferencedByExcludesSelfReference(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + for _, ddl := range []string{ + fmt.Sprintf("CREATE TABLE %s.emp (id bigint PRIMARY KEY, parent_id bigint REFERENCES %s.emp(id))", schema, schema), + fmt.Sprintf(`CREATE TABLE %s.spt ( + id bigint, day date NOT NULL, parent_id bigint, parent_day date, + PRIMARY KEY (id, day), + FOREIGN KEY (parent_id, parent_day) REFERENCES %s.spt (id, day) + ) PARTITION BY RANGE (day)`, schema, schema), + fmt.Sprintf("CREATE TABLE %s.spt_1 PARTITION OF %s.spt FOR VALUES FROM ('2026-01-01') TO ('2026-02-01')", schema, schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + emp, err := schemadiff.Introspect(t.Context(), pool, schema, "emp") + require.NoError(t, err) + assert.Empty(t, emp.ReferencedBy) + _, err = schemadiff.Render(emp) + require.ErrorIs(t, err, statement.ErrForeignKey) + require.NotErrorIs(t, err, schemadiff.ErrUnrenderableForeignKey) + + spt, err := schemadiff.Introspect(t.Context(), pool, schema, "spt") + require.NoError(t, err) + assert.Empty(t, spt.ReferencedBy) +} + +// Ownership, not naming, separates a serial column from a hand-written +// nextval default. Two tables share one standalone sequence that happens to +// carry the serial-style name of the first: rendering either as serial +// would silently convert the shared sequence into a private one and break +// the shared-ID invariant, so both refuse. A genuinely owned sequence still +// renders. +func TestRenderRefusesStandaloneSequenceWithSerialName(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + for _, ddl := range []string{ + fmt.Sprintf("CREATE SEQUENCE %s.events_id_seq", schema), + fmt.Sprintf("CREATE TABLE %s.events (id bigint NOT NULL DEFAULT nextval('%s.events_id_seq'::regclass), name text)", schema, schema), + fmt.Sprintf("CREATE TABLE %s.orders (id bigint NOT NULL DEFAULT nextval('%s.events_id_seq'::regclass), name text)", schema, schema), + fmt.Sprintf("CREATE TABLE %s.owned (id bigserial NOT NULL, name text)", schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + for _, table := range []string{"events", "orders"} { + live, err := schemadiff.Introspect(t.Context(), pool, schema, table) + require.NoError(t, err) + require.True(t, live.Columns[0].SequenceDefault) + assert.False(t, live.Columns[0].SequenceOwned, "a standalone sequence is not owned, whatever its name") + _, err = schemadiff.Render(live) + require.ErrorIs(t, err, schemadiff.ErrUnrenderableDefault, "table %s", table) + } + + owned, err := schemadiff.Introspect(t.Context(), pool, schema, "owned") + require.NoError(t, err) + assert.True(t, owned.Columns[0].SequenceOwned) + rendered, err := schemadiff.Render(owned) + require.NoError(t, err) + assert.Contains(t, rendered, `"id" bigserial NOT NULL`) +} + +// An unlogged live table refuses to render, and a persistence mismatch +// between live and desired is a typed diff refusal — never a plain-table +// baseline or a silent zero diff. +func TestRenderRefusesUnloggedLiveTable(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE UNLOGGED TABLE %s.buffer (id bigint NOT NULL, name text)", schema)) + require.NoError(t, err) + + live, err := schemadiff.Introspect(t.Context(), pool, schema, "buffer") + require.NoError(t, err) + assert.True(t, live.Unlogged) + _, err = schemadiff.Render(live) + require.ErrorIs(t, err, schemadiff.ErrUnrenderableUnlogged) + + // The diff guard closes the persistence-blind hole end to end: a + // desired file declaring UNLOGGED against a plain live table must + // refuse, not diff to zero. + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.plainbuf (id bigint NOT NULL, name text)", schema)) + require.NoError(t, err) + ds, err := statement.ParseDesired("CREATE UNLOGGED TABLE plainbuf (id bigint NOT NULL, name text)") + require.NoError(t, err) + desired, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + assert.True(t, desired.Unlogged) + livePlain, err := schemadiff.Introspect(t.Context(), pool, schema, "plainbuf") + require.NoError(t, err) + _, err = schemadiff.Diff(schema, livePlain, desired) + require.ErrorIs(t, err, schemadiff.ErrUnsupportedChange) +} + +// A column with an explicit collation refuses to render, and a collation +// delta between live and desired is a typed diff refusal — never a +// baseline that silently drops the COLLATE clause or a silent zero diff. +func TestRenderRefusesCollatedLiveColumn(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + _, err = pool.Exec(t.Context(), fmt.Sprintf(`CREATE TABLE %s.words (id bigint NOT NULL, word text COLLATE "C")`, schema)) + require.NoError(t, err) + + live, err := schemadiff.Introspect(t.Context(), pool, schema, "words") + require.NoError(t, err) + assert.Equal(t, `pg_catalog."C"`, live.Columns[1].Collation) + _, err = schemadiff.Render(live) + require.ErrorIs(t, err, schemadiff.ErrUnrenderableCollation) + + ds, err := statement.ParseDesired(`CREATE TABLE words (id bigint NOT NULL, word text)`) + require.NoError(t, err) + desired, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + _, err = schemadiff.Diff(schema, live, desired) + require.ErrorIs(t, err, schemadiff.ErrUnsupportedChange) +} From 5e62e4675a7735a62f1ff4a2d3aac3c851a4718b Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 20 Aug 2026 19:59:37 +1000 Subject: [PATCH 3/3] schemadiff: verify sequence ownership; refuse unlogged and collated renders Address the #52 adversarial review: Render's two proofs (ParseDesired admissibility, diff-to-zero) are structurally blind to omission, so the facts the model cannot carry now refuse instead of silently vanishing from the baseline. serialType requires a genuine pg_depend OWNED BY edge (Column.SequenceOwned), not the serial-style sequence name; unlogged tables and explicit column collations refuse on both render and diff with their own typed sentinels; a zero-column table renders (). Also excludes partition clones from ReferencedBy (conislocal), matching introspectConstraints. Amp-Thread-ID: https://ampcode.com/threads/T-01a01341-1d4b-7198-a602-a7ea4b7c4884 Co-authored-by: Amp --- README.md | 3 ++ docs/limitations.md | 4 +- pkg/schemadiff/diff_test.go | 31 ++++++++++++++++ pkg/schemadiff/render.go | 45 ++++++++++++++++++----- pkg/schemadiff/render_integration_test.go | 29 --------------- pkg/schemadiff/render_test.go | 43 ++++++++++++++++++++-- pkg/schemadiff/schemadiff.go | 23 ++++++++++++ 7 files changed, 136 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 4f90cb0..3d1f2d2 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,9 @@ refusal — never a silently wrong or incomplete result: still works through the statement front door (`NOT VALID` + `VALIDATE`). - **Partitioned tables** support in-place statement changes, but cannot be expressed in or exported to desired files. +- **Unlogged tables and explicit column collations** are outside the + declarative model: converging either is a table (or column) rewrite, so + export and diff refuse rather than plan one. - **Non-table objects** — views, standalone sequences, enums, domains, extensions, functions, triggers — are outside the declarative model, which covers one ordinary table plus its indexes per file. diff --git a/docs/limitations.md b/docs/limitations.md index 7d26b47..6cae7b0 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -21,5 +21,7 @@ with a typed refusal — never a silently wrong or incomplete result: | --- | --- | | Foreign keys (either direction) | Unsupported in the declarative model, in both directions. A desired file cannot declare a `REFERENCES` clause (refused at parse), and export refuses both sides of a foreign-key relationship: a table whose definition carries foreign-key constraints surfaces the parse gate's typed error, and a table that other tables reference refuses with its own typed error — a single-table baseline cannot carry incoming foreign-key topology, so rendering one would silently drop the relationship. Foreign-key **DDL is still supported through the statement front door** (`ADD FOREIGN KEY` routes to the online `NOT VALID` + `VALIDATE` sequence). Because a desired file can never declare a foreign key, `diff` on a live table that carries one plans a **destructive** `DROP CONSTRAINT` for it — gated like every destructive change, never auto-executed — and incoming foreign keys are invisible to a single-table diff entirely, so tables participating in foreign-key relationships in either direction should not be managed declaratively yet. | | Partitioned tables | Partitioned parents and their partitions are introspectable, and the statement front door supports in-place changes on them (see the partitioned-parent rows above), but they cannot be expressed in or exported to a desired file: the model captures the partition key and attachment only to refuse — it does not carry partition bounds or the parent/partition topology. A partitioning mismatch between live and desired is a typed `diff` refusal, never a zero diff. | -| Non-table objects | Views, materialized views, standalone sequences, enums, domains, extensions, functions, and triggers are outside the model. A serial column's owned sequence is the one exception: it round-trips through the `serial` pseudo-types. A column may *use* an unmanaged type (an enum, a domain) — the type text round-trips — but the type's definition is not managed. | +| Unlogged tables | Persistence is not managed: converging it (`SET LOGGED` / `SET UNLOGGED`) is a full table rewrite. Export refuses an unlogged table — a plain `CREATE TABLE` baseline would silently change its crash-safety and replication behavior — and a persistence mismatch between live and desired is a typed `diff` refusal, never a zero diff. | +| Column collations | An explicit `COLLATE` on a column is not managed: converging a collation delta rewrites the column and its indexes. Export refuses a collated column — a baseline without the clause would silently change sort order and index semantics — and a collation delta (including on an added column) is a typed `diff` refusal. | +| Non-table objects | Views, materialized views, standalone sequences, enums, domains, extensions, functions, and triggers are outside the model. A serial column's owned sequence is the one exception: it round-trips through the `serial` pseudo-types — and ownership is verified through the catalog (`pg_depend`), so a hand-written `nextval` default on a standalone sequence that merely carries the serial-style name refuses rather than exporting as `serial` and silently privatizing a shared sequence. A column may *use* an unmanaged type (an enum, a domain) — the type text round-trips — but the type's definition is not managed. | | Multiple tables per file | A desired file is single-table scoped: exactly one `CREATE TABLE` plus `CREATE INDEX` statements on it. Multi-table schemas are managed as one file per table. | diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go index fadad2a..3946f67 100644 --- a/pkg/schemadiff/diff_test.go +++ b/pkg/schemadiff/diff_test.go @@ -60,6 +60,37 @@ func TestDiffRefusesPartitioningMismatch(t *testing.T) { require.ErrorIs(t, err, ErrUnsupportedChange) } +// Persistence converges only by a full table rewrite (SET LOGGED / SET +// UNLOGGED), which the engine does not derive — a mismatch is a typed +// refusal, never a silent zero diff. +func TestDiffRefusesPersistenceMismatch(t *testing.T) { + unlogged := base() + unlogged.Unlogged = true + _, err := Diff("public", unlogged, base()) + require.ErrorIs(t, err, ErrUnsupportedChange) + + _, err = Diff("public", base(), unlogged) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + +// A collation delta on an existing column rewrites the column, and an +// added column's COLLATE clause is not carried by the emitted ADD COLUMN — +// both are typed refusals, never a silent zero diff or a silently dropped +// clause. +func TestDiffRefusesCollationChanges(t *testing.T) { + collated := base() + collated.Columns[1].Collation = `"C"` + _, err := Diff("public", base(), collated) + require.ErrorIs(t, err, ErrUnsupportedChange) + + added := base() + added.Columns = append(added.Columns, Column{ + Name: "note", Type: "text", Collation: `"C"`, + }) + _, err = Diff("public", base(), added) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + func TestDiffAddColumn(t *testing.T) { desired := base() desired.Columns = append(desired.Columns, Column{ diff --git a/pkg/schemadiff/render.go b/pkg/schemadiff/render.go index 6887fa7..ffbbbda 100644 --- a/pkg/schemadiff/render.go +++ b/pkg/schemadiff/render.go @@ -35,6 +35,18 @@ var ErrUnrenderablePartition = errors.New("partitioned tables cannot be rendered // (statement.ErrForeignKey). var ErrUnrenderableForeignKey = errors.New("tables referenced by foreign keys cannot be rendered as a desired schema") +// ErrUnrenderableUnlogged is returned for an unlogged table. The +// declarative model does not manage persistence, so a rendered plain +// CREATE TABLE would silently change the table's crash-safety and +// replication behavior — the renderer refuses instead. +var ErrUnrenderableUnlogged = errors.New("unlogged tables cannot be rendered as a desired schema") + +// ErrUnrenderableCollation is returned when a column carries an explicit +// collation. The declarative model does not manage collations, so a +// rendered baseline without the COLLATE clause would silently change sort +// order and index semantics — the renderer refuses instead. +var ErrUnrenderableCollation = errors.New("columns with an explicit collation cannot be rendered as a desired schema") + // Render renders the canonical model into a desired-state schema file: one // CREATE TABLE followed by the model's CREATE INDEX statements. The output // is proven admissible by parsing it through statement.ParseDesired before @@ -53,6 +65,9 @@ func Render(m Model) (string, error) { if len(m.ReferencedBy) != 0 { return "", fmt.Errorf("render table %q: referenced by foreign keys (%s): %w", m.Table, strings.Join(m.ReferencedBy, ", "), ErrUnrenderableForeignKey) } + if m.Unlogged { + return "", fmt.Errorf("render table %q: %w", m.Table, ErrUnrenderableUnlogged) + } defs := make([]string, 0, len(m.Columns)+len(m.Constraints)) for _, c := range m.Columns { def, err := renderColumnDef(m.Table, c) @@ -66,9 +81,12 @@ func Render(m Model) (string, error) { } var b strings.Builder - b.WriteString("CREATE TABLE " + pgx.Identifier{m.Table}.Sanitize() + " (\n") - b.WriteString(strings.Join(defs, ",\n")) - b.WriteString("\n);\n") + b.WriteString("CREATE TABLE " + pgx.Identifier{m.Table}.Sanitize() + " (") + // CREATE TABLE t () is legal; render it without an empty body line. + if len(defs) != 0 { + b.WriteString("\n" + strings.Join(defs, ",\n") + "\n") + } + b.WriteString(");\n") for _, ix := range m.Indexes { b.WriteString("\n" + ix.Def + ";\n") } @@ -83,8 +101,13 @@ func Render(m Model) (string, error) { // renderColumnDef renders one column for CREATE TABLE. A serial column is // rendered back to its pseudo-type so the desired file recreates the owned // sequence on the scratch schema; every other sequence-backed default is -// refused because the sequence it references cannot exist there. +// refused because the sequence it references cannot exist there. An +// explicit column collation is refused: the model carries it only to keep +// the baseline from silently dropping it. func renderColumnDef(table string, c Column) (string, error) { + if c.Collation != "" { + return "", fmt.Errorf("column %q collation %s: %w", c.Name, c.Collation, ErrUnrenderableCollation) + } if c.SequenceDefault { st, ok := serialType(table, c) if !ok { @@ -97,17 +120,21 @@ func renderColumnDef(table string, c Column) (string, error) { // serialType maps a sequence-backed integer column back to the serial // pseudo-type it expands from. It requires exactly what the serial -// shorthand produces: an integer-family type, NOT NULL, and a default of -// nextval on the owned sequence named __seq. The name check -// covers only names that need no quoting inside the nextval literal; -// exotic or truncated sequence names fail closed to ErrUnrenderableDefault. +// shorthand produces: an integer-family type, NOT NULL, a sequence the +// column actually owns (the pg_depend OWNED BY edge — a standalone +// sequence that merely happens to carry the serial-style name is not +// ownership, and rendering it as serial would silently convert a shared +// sequence into a private one), and a default of nextval on the sequence +// named
__seq. The name check covers only names that need +// no quoting inside the nextval literal; exotic or truncated sequence +// names fail closed to ErrUnrenderableDefault. func serialType(table string, c Column) (string, bool) { base, ok := map[string]string{ "smallint": "smallserial", "integer": "serial", "bigint": "bigserial", }[c.Type] - if !ok || !c.NotNull { + if !ok || !c.NotNull || !c.SequenceOwned { return "", false } if c.Default != "nextval('"+table+"_"+c.Name+"_seq'::regclass)" { diff --git a/pkg/schemadiff/render_integration_test.go b/pkg/schemadiff/render_integration_test.go index 9b20e39..a92de29 100644 --- a/pkg/schemadiff/render_integration_test.go +++ b/pkg/schemadiff/render_integration_test.go @@ -66,35 +66,6 @@ func TestRenderRoundTripsLiveTable(t *testing.T) { assert.Empty(t, changes, "diffing a table against its own rendering must yield no changes") } -// Quoted identifiers round-trip: a table whose name carries whitespace and -// mixed case, columns that are mixed-case and a reserved word, and a -// mixed-case constraint all force every identifier the renderer emits -// through real quoting — a Sanitize call replaced with raw interpolation -// would produce a file this test refuses to parse or materialize. -func TestRenderRoundTripsQuotedIdentifiers(t *testing.T) { - pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) - require.NoError(t, err) - defer pool.Close() - schema := testutil.NewSchema(t, pool) - - for _, ddl := range []string{ - fmt.Sprintf(`CREATE TABLE %s."Order Items" ( - "ID" bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY, - "User ID" bigint NOT NULL, - "select" text, - CONSTRAINT "User Positive" CHECK ("User ID" > 0) - )`, schema), - fmt.Sprintf(`CREATE INDEX "Order Items_User_idx" ON %s."Order Items" ("User ID")`, schema), - } { - _, err := pool.Exec(t.Context(), ddl) - require.NoError(t, err) - } - - live, desired, changes := roundTrip(t, pool, schema, "Order Items") - assert.Equal(t, live, desired, "rendered output must introspect back to the identical model") - assert.Empty(t, changes, "diffing a table against its own rendering must yield no changes") -} - // A serial table round-trips through the serial pseudo-type: the rendered // file recreates the owned sequence on the scratch schema and both sides // decompile the default identically. diff --git a/pkg/schemadiff/render_test.go b/pkg/schemadiff/render_test.go index 3fd2a34..9c27860 100644 --- a/pkg/schemadiff/render_test.go +++ b/pkg/schemadiff/render_test.go @@ -56,7 +56,7 @@ func TestRenderSerialColumn(t *testing.T) { m := base() m.Columns[0] = Column{ Name: "id", Type: tt.colType, NotNull: true, - Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, + Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, SequenceOwned: true, } out, err := Render(m) @@ -77,13 +77,21 @@ func TestRenderRefusesUnrenderableSequenceDefaults(t *testing.T) { Name: "id", Type: "bigint", NotNull: true, Default: "nextval('global_id_seq'::regclass)", SequenceDefault: true, }}, + {"standalone sequence with the serial-style name", Column{ + Name: "id", Type: "bigint", NotNull: true, + Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, SequenceOwned: false, + }}, + {"owned sequence with a non-serial name", Column{ + Name: "id", Type: "bigint", NotNull: true, + Default: "nextval('renamed_seq'::regclass)", SequenceDefault: true, SequenceOwned: true, + }}, {"nullable serial", Column{ Name: "id", Type: "bigint", - Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, + Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, SequenceOwned: true, }}, {"non-integer type", Column{ Name: "id", Type: "numeric", NotNull: true, - Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, + Default: "nextval('events_id_seq'::regclass)", SequenceDefault: true, SequenceOwned: true, }}, } for _, tt := range tests { @@ -111,6 +119,35 @@ func TestRenderRefusesPartitionedTables(t *testing.T) { require.ErrorIs(t, err, ErrUnrenderablePartition) } +// A zero-column table is legal PostgreSQL and renders as an empty body, +// not an empty line between the parentheses. +func TestRenderZeroColumnTable(t *testing.T) { + m := Model{Table: "nocols"} + out, err := Render(m) + require.NoError(t, err) + assert.Equal(t, "CREATE TABLE \"nocols\" ();\n", out) +} + +// An unlogged table refuses to render: the model does not manage +// persistence, so a plain CREATE TABLE baseline would silently change the +// table's crash-safety and replication behavior. +func TestRenderRefusesUnloggedTable(t *testing.T) { + m := base() + m.Unlogged = true + _, err := Render(m) + require.ErrorIs(t, err, ErrUnrenderableUnlogged) +} + +// A column with an explicit collation refuses to render: the model does +// not manage collations, so a baseline without the COLLATE clause would +// silently change sort order and index semantics. +func TestRenderRefusesCollatedColumn(t *testing.T) { + m := base() + m.Columns[1].Collation = `"C"` + _, err := Render(m) + require.ErrorIs(t, err, ErrUnrenderableCollation) +} + // The renderer proves its own output admissible through ParseDesired, so a // model carrying what a desired file refuses surfaces that gate's typed // error — a foreign key is the canonical case. diff --git a/pkg/schemadiff/schemadiff.go b/pkg/schemadiff/schemadiff.go index fa2321f..8a550a0 100644 --- a/pkg/schemadiff/schemadiff.go +++ b/pkg/schemadiff/schemadiff.go @@ -44,6 +44,21 @@ type Column struct { // the rolled-back scratch transaction, so no derived plan can // reference it. SequenceDefault bool + // SequenceOwned reports that the sequence behind the default is owned + // by this exact column (a pg_depend OWNED BY edge, deptype 'a') — what + // the serial shorthand produces. It is false for a hand-written + // nextval default on a standalone sequence or another table's + // sequence, however the sequence is named: sharing distinguishes it + // from ownership, not naming. + SequenceOwned bool + // Collation is the column's explicit collation as a schema-qualified, + // quote_ident-quoted name, empty when the column uses its type's + // default collation. The declarative model does not manage collations yet, so + // the model carries it only to refuse: dropping a COLLATE clause from + // a rendered baseline would silently change sort order and index + // semantics, and a collation delta cannot be converged without a + // table rewrite. + Collation string // Identity is the identity kind, IdentityNone for plain columns. Identity Identity // Generated reports GENERATED ALWAYS AS (...) STORED. @@ -83,6 +98,14 @@ type Model struct { // IsPartition reports that the table is itself a partition of a // partitioned parent (pg_class.relispartition). IsPartition bool + // Unlogged reports that the table is unlogged + // (pg_class.relpersistence 'u'). The declarative model does not manage + // persistence yet — converging it (SET LOGGED / SET UNLOGGED) is a + // full table rewrite — so the model carries it only to refuse: + // rendering an unlogged table as a plain CREATE TABLE would silently + // change its crash-safety and replication behavior, and a persistence + // mismatch fails the diff closed instead of diffing to silence. + Unlogged bool // ReferencedBy lists the incoming foreign keys — constraints on other // tables that reference this one — as "table.constraint" strings, in // that order. Incoming foreign keys are not part of this table's own