From d626da7494e8434763591225b34b7d56e1d94478 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 22:29:50 +1000 Subject: [PATCH 1/6] Phase 2.1: desired-state parse boundary and execute-and-introspect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Admit a declarative desired-state file (one CREATE TABLE plus its CREATE INDEXes) through the real grammar, materialize it in an always-rolled-back transaction-scoped scratch schema on the target, and introspect both live and desired state into one canonical model from server catalogs — semantic truth comes from PostgreSQL, never from AST transformation, per the layered DDL-understanding decision. --- SAFETY.md | 2 +- docs/low-level-design.md | 9 ++ internal/cli/migrate.go | 4 + internal/cli/migrate_integration_test.go | 2 +- pkg/schemadiff/desired.go | 74 +++++++++ pkg/schemadiff/introspect.go | 185 +++++++++++++++++++++++ pkg/schemadiff/schemadiff.go | 79 ++++++++++ pkg/statement/desired.go | 155 +++++++++++++++++++ pkg/statement/desired_test.go | 87 +++++++++++ pkg/statement/statement.go | 8 + pkg/statement/statement_test.go | 2 +- 11 files changed, 604 insertions(+), 3 deletions(-) create mode 100644 pkg/schemadiff/desired.go create mode 100644 pkg/schemadiff/introspect.go create mode 100644 pkg/schemadiff/schemadiff.go create mode 100644 pkg/statement/desired.go create mode 100644 pkg/statement/desired_test.go diff --git a/SAFETY.md b/SAFETY.md index cc56d6a..418341c 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -27,7 +27,7 @@ The invariant registry (invariant IDs referenced below) lives in | `pkg/checkpoint` — durable resume state | ✅ core | planned (Phase 8) | ST-1, ST-2 | | slot lifecycle (in `pkg/decode`) — create, reap, lag ceiling | ✅ core | planned (Phase 8) | ST-3 | | `pkg/migration` — orchestrator, **cutover swap + fidelity gate** | ✅ core | planned (Phase 7) | LK-2, LK-4, ST-5 | -| `pkg/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/lint` — classify/diff/route | ❌ periphery¹ | `pkg/statement` exists (Phase 1: type gate); rest planned (Phase 2) | (CO-7 holds at the parse boundary) | +| `pkg/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/lint` — classify/diff/route | ❌ periphery¹ | `pkg/statement` (parse boundary) and `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect) exist (Phase 2); `pkg/planner`, `pkg/lint` planned (Phase 2) | (CO-7 holds at the parse boundary) | | `pkg/verdict` — structured outcome contract, rendering, exit codes | ❌ periphery | exists (Phase 1) | — | | `internal/cli` — CLI, flags, help, prompts | ❌ periphery | `migrate`/`status` exist (Phase 1); rest stubs | — | | status / progress / advisory rendering, metrics | ❌ periphery | planned | — | diff --git a/docs/low-level-design.md b/docs/low-level-design.md index c89c47d..e27ed12 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -544,6 +544,15 @@ The scratch database is engine-owned and disposable: preflight may reset it (dro contents) at any time. Restricted environments that won't grant `CREATEDB` pre-provision instead. +**Plan-time diffing uses a lighter mechanism.** `pkg/schemadiff` materializes the desired +state inside a single always-rolled-back transaction in the *target* database, in a +randomly named transaction-scoped schema (`pgsprite_scratch_`). This keeps the +same-server semantic-truth property (same version, extensions, and defaults as the live +table) while requiring no `CREATEDB`, no pre-provisioning, and leaving zero footprint — +appropriate because diffing is read-only planning. The durable `pg_sprite_scratch` +database above is required only by the migration path proper (shadow-DDL derivation and +checkpoint fingerprints), where objects must outlive a transaction. + ### Postgres-only preconditions Spirit has no analog for These have **no MySQL counterpart** but are hard requirements for the logical-decoding path: diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index e2eff93..e43227c 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -108,6 +108,10 @@ func gateVerdict(st statement.Statement) (verdict.Verdict, bool) { v.Reason = verdict.ReasonIndexStatement v.Detail = "a plain REINDEX blocks writes; the concurrent rebuild does not" v.SaferIdiom = "REINDEX ... CONCURRENTLY" + case statement.KindCreateTable: + v.Reason = verdict.ReasonUnsupportedStatement + v.Detail = "migrate changes an existing table; to converge a table onto a desired-state CREATE TABLE, use the declarative front-end" + v.SaferIdiom = "pg-sprite diff --desired schema.sql" case statement.KindOther: v.Reason = verdict.ReasonUnsupportedStatement v.Detail = "only ALTER TABLE statements are supported by the optimistic front door" diff --git a/internal/cli/migrate_integration_test.go b/internal/cli/migrate_integration_test.go index 20b2db2..fe2c634 100644 --- a/internal/cli/migrate_integration_test.go +++ b/internal/cli/migrate_integration_test.go @@ -144,7 +144,7 @@ func TestMigrateGateRefusesWithoutDatabase(t *testing.T) { {"drop index", "DROP INDEX i", verdict.ReasonIndexStatement, "DROP INDEX CONCURRENTLY"}, {"reindex", "REINDEX TABLE t", verdict.ReasonIndexStatement, "REINDEX ... CONCURRENTLY"}, {"alter index", "ALTER INDEX i SET (fillfactor = 90)", verdict.ReasonUnsupportedStatement, ""}, - {"create table", "CREATE TABLE t (id int)", verdict.ReasonUnsupportedStatement, ""}, + {"create table", "CREATE TABLE t (id int)", verdict.ReasonUnsupportedStatement, "pg-sprite diff --desired schema.sql"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/schemadiff/desired.go b/pkg/schemadiff/desired.go new file mode 100644 index 0000000..646d80e --- /dev/null +++ b/pkg/schemadiff/desired.go @@ -0,0 +1,74 @@ +package schemadiff + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/block/pg-sprite/pkg/statement" +) + +// IntrospectDesired materializes a desired-state schema on a scratch schema +// and introspects it into the canonical model: execute-and-introspect, the +// decided way the engine understands DDL semantics. The scratch schema is +// created inside a single transaction that is always rolled back — nothing +// the desired file defines ever persists, no CREATEDB privilege is needed, +// and server-version and extension parity with the live table hold by +// construction because it runs on the same database. +func IntrospectDesired(ctx context.Context, db *pgxpool.Pool, desired statement.DesiredSchema) (Model, error) { + scratch, err := scratchSchemaName() + if err != nil { + return Model{}, err + } + tx, err := db.Begin(ctx) + if err != nil { + return Model{}, fmt.Errorf("begin scratch transaction: %w", err) + } + // The scratch transaction is never committed: rollback is the cleanup + // path for success and failure alike, so the redundant-closer exception + // does not apply — this rollback is load-bearing and its error is + // surfaced on the success path below. + defer func() { + _ = tx.Rollback(context.WithoutCancel(ctx)) + }() + + if _, err := tx.Exec(ctx, "CREATE SCHEMA "+pgx.Identifier{scratch}.Sanitize()); err != nil { + return Model{}, fmt.Errorf("create scratch schema: %w", err) + } + // Unqualified desired statements must land on the scratch schema, while + // extension types installed in public stay resolvable. search_path + // cannot use bind parameters; the identifier is sanitized. + setPath := "SET LOCAL search_path = " + pgx.Identifier{scratch}.Sanitize() + ", public" + if _, err := tx.Exec(ctx, setPath); err != nil { + return Model{}, fmt.Errorf("set scratch search_path: %w", err) + } + for _, st := range desired.Statements { + if _, err := tx.Exec(ctx, st.SQL); err != nil { + return Model{}, fmt.Errorf("execute desired statement on scratch schema: %w", err) + } + } + m, err := introspectInTx(ctx, tx, scratch, desired.Table) + if err != nil { + return Model{}, fmt.Errorf("introspect desired state: %w", err) + } + if err := tx.Rollback(ctx); err != nil { + return Model{}, fmt.Errorf("roll back scratch schema: %w", err) + } + return m, nil +} + +// scratchSchemaName returns a collision-resistant scratch schema name. The +// name only has to be unique among concurrent scratch transactions on the +// same database; the schema itself never outlives its transaction. The +// prefix avoids "pg_", which PostgreSQL reserves for system schemas. +func scratchSchemaName() (string, error) { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generate scratch schema name: %w", err) + } + return "pgsprite_scratch_" + hex.EncodeToString(b[:]), nil +} diff --git a/pkg/schemadiff/introspect.go b/pkg/schemadiff/introspect.go new file mode 100644 index 0000000..34d212c --- /dev/null +++ b/pkg/schemadiff/introspect.go @@ -0,0 +1,185 @@ +package schemadiff + +import ( + "context" + "errors" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/block/pg-sprite/pkg/statement" +) + +// ErrTableNotFound is returned when the target table does not exist in the +// requested schema. +var ErrTableNotFound = errors.New("table not found") + +// ErrNotTable is returned when the target exists but is not an ordinary or +// partitioned table (e.g. a view or foreign table). +var ErrNotTable = errors.New("not an ordinary or partitioned table") + +// Introspect reads the live table schema.table into the canonical model. It +// runs inside a read-only transaction whose search_path is set to the target +// schema (then public), so the server's decompilers print definitions +// unqualified — directly comparable with a desired-state model introspected +// the same way. +func Introspect(ctx context.Context, db *pgxpool.Pool, schema, table string) (Model, error) { + tx, err := db.Begin(ctx) + if err != nil { + return Model{}, fmt.Errorf("begin introspection: %w", err) + } + defer func() { + // Redundant safety closer: the transaction is read-only and always + // rolled back below; this only covers early error returns. + _ = tx.Rollback(context.WithoutCancel(ctx)) + }() + m, err := introspectInTx(ctx, tx, schema, table) + if err != nil { + return Model{}, err + } + if err := tx.Rollback(ctx); err != nil { + return Model{}, fmt.Errorf("end introspection: %w", err) + } + return m, nil +} + +// introspectInTx introspects schema.table inside an open transaction. It +// sets the transaction-local search_path so decompiled definitions print +// unqualified, resolves the relation by explicit qualification (never via +// search_path), and reads columns, constraints, and indexes. +func introspectInTx(ctx context.Context, tx pgx.Tx, schema, table string) (Model, error) { + // search_path cannot use bind parameters; identifiers are sanitized. + setPath := "SET LOCAL search_path = " + pgx.Identifier{schema}.Sanitize() + ", public" + if _, err := tx.Exec(ctx, setPath); err != nil { + return Model{}, fmt.Errorf("set introspection search_path: %w", err) + } + + var oid uint32 + var relkind string + err := tx.QueryRow(ctx, ` + SELECT c.oid, c.relkind::text + 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) + if errors.Is(err, pgx.ErrNoRows) { + return Model{}, fmt.Errorf("%s.%s: %w", schema, table, ErrTableNotFound) + } + if err != nil { + return Model{}, fmt.Errorf("resolve table %s.%s: %w", schema, table, err) + } + if relkind != "r" && relkind != "p" { + return Model{}, fmt.Errorf("%s.%s has relkind %q: %w", schema, table, relkind, ErrNotTable) + } + + m := Model{Table: table} + if m.Columns, err = introspectColumns(ctx, tx, oid); err != nil { + return Model{}, fmt.Errorf("introspect columns of %s.%s: %w", schema, table, err) + } + if m.Constraints, err = introspectConstraints(ctx, tx, oid); err != nil { + return Model{}, fmt.Errorf("introspect constraints of %s.%s: %w", schema, table, err) + } + if m.Indexes, err = introspectIndexes(ctx, tx, oid); err != nil { + return Model{}, fmt.Errorf("introspect indexes of %s.%s: %w", schema, table, err) + } + return m, nil +} + +// introspectColumns reads the canonical column list: server-formatted types +// and server-decompiled default/generation expressions, in attribute order. +func introspectColumns(ctx context.Context, tx pgx.Tx, oid uint32) ([]Column, error) { + rows, err := tx.Query(ctx, ` + SELECT a.attname, + format_type(a.atttypid, a.atttypmod), + a.attnotnull, + COALESCE(pg_get_expr(d.adbin, d.adrelid), ''), + a.attidentity::text, + a.attgenerated::text + FROM pg_attribute a + LEFT JOIN pg_attrdef d ON d.adrelid = a.attrelid AND d.adnum = a.attnum + WHERE a.attrelid = $1 AND a.attnum > 0 AND NOT a.attisdropped + ORDER BY a.attnum`, oid) + if err != nil { + return nil, fmt.Errorf("query columns: %w", err) + } + defer rows.Close() + var cols []Column + for rows.Next() { + var c Column + var identity, generated string + if err := rows.Scan(&c.Name, &c.Type, &c.NotNull, &c.Default, &identity, &generated); err != nil { + return nil, fmt.Errorf("scan column: %w", err) + } + c.Identity = Identity(identity) + c.Generated = generated == "s" + cols = append(cols, c) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read columns: %w", err) + } + return cols, nil +} + +// introspectConstraints reads the table's own constraints (primary key, +// unique, check, foreign key, exclusion) as server-decompiled definitions. +// NOT NULL is modeled on the column (pg_attribute.attnotnull), so the PG 18 +// pg_constraint rows for NOT NULL are deliberately excluded to keep the +// model identical across supported majors. +func introspectConstraints(ctx context.Context, tx pgx.Tx, oid uint32) ([]Constraint, error) { + rows, err := tx.Query(ctx, ` + SELECT conname, pg_get_constraintdef(oid) + FROM pg_constraint + WHERE conrelid = $1 AND contype IN ('p','u','c','f','x') AND conislocal + ORDER BY conname`, oid) + if err != nil { + return nil, fmt.Errorf("query constraints: %w", err) + } + defer rows.Close() + var cons []Constraint + for rows.Next() { + var c Constraint + if err := rows.Scan(&c.Name, &c.Def); err != nil { + return nil, fmt.Errorf("scan constraint: %w", err) + } + cons = append(cons, c) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read constraints: %w", err) + } + return cons, 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. +// pg_get_indexdef always schema-qualifies the ON clause, so the +// qualification is stripped to keep the model schema-relative and +// comparable between the live and scratch sides. +func introspectIndexes(ctx context.Context, tx pgx.Tx, oid uint32) ([]Index, error) { + rows, err := tx.Query(ctx, ` + SELECT c.relname, pg_get_indexdef(i.indexrelid) + FROM pg_index i + JOIN pg_class c ON c.oid = i.indexrelid + WHERE i.indrelid = $1 + AND NOT EXISTS (SELECT 1 FROM pg_constraint con WHERE con.conindid = i.indexrelid) + ORDER BY c.relname`, oid) + if err != nil { + return nil, fmt.Errorf("query indexes: %w", err) + } + defer rows.Close() + var idxs []Index + for rows.Next() { + var ix Index + if err := rows.Scan(&ix.Name, &ix.Def); err != nil { + return nil, fmt.Errorf("scan index: %w", err) + } + if ix.Def, err = statement.Qualify(ix.Def, ""); err != nil { + return nil, fmt.Errorf("unqualify index %s: %w", ix.Name, err) + } + idxs = append(idxs, ix) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("read indexes: %w", err) + } + return idxs, nil +} diff --git a/pkg/schemadiff/schemadiff.go b/pkg/schemadiff/schemadiff.go new file mode 100644 index 0000000..1cdfe9e --- /dev/null +++ b/pkg/schemadiff/schemadiff.go @@ -0,0 +1,79 @@ +// Package schemadiff builds the canonical table model both front-ends share +// and diffs two models into an ordered statement list. The model always +// comes from a real PostgreSQL catalog — the live table is introspected +// directly, and a desired-state file is executed on a transaction-scoped +// scratch schema and introspected the same way, then the transaction is +// rolled back (execute-and-introspect; semantics are never derived from the +// AST). Canonical text (types, defaults, constraint and index definitions) +// is whatever the server's own decompilers print, so cosmetic differences +// (type aliases, default formatting, implicit names) never show up as diffs. +// +// This is a periphery package (see SAFETY.md): its output is a plan request, +// and the core executors re-verify their own preconditions. +package schemadiff + +// Identity is a column's identity kind, as pg_attribute.attidentity spells +// it. +type Identity string + +// The identity kinds. +const ( + // IdentityNone means the column is not an identity column. + IdentityNone Identity = "" + // IdentityAlways is GENERATED ALWAYS AS IDENTITY. + IdentityAlways Identity = "a" + // IdentityByDefault is GENERATED BY DEFAULT AS IDENTITY. + IdentityByDefault Identity = "d" +) + +// Column is one column of the canonical model. +type Column struct { + // Name is the column name. + Name string + // Type is the canonical type text (format_type), e.g. "character + // varying(50)" — never an alias like varchar(50). + Type string + // NotNull reports the NOT NULL attribute. + NotNull bool + // Default is the canonical default expression (pg_get_expr), empty when + // none. For a generated column it is the generation expression. + Default string + // Identity is the identity kind, IdentityNone for plain columns. + Identity Identity + // Generated reports GENERATED ALWAYS AS (...) STORED. + Generated bool +} + +// Constraint is one table constraint: its name plus the server-decompiled +// definition (pg_get_constraintdef), e.g. "PRIMARY KEY (id)". +type Constraint struct { + // Name is the constraint name. + Name string + // Def is the canonical definition text. + Def string +} + +// Index is one non-constraint index: its name plus the server-decompiled +// CREATE INDEX statement (pg_get_indexdef), unqualified under the +// introspection search_path. +type Index struct { + // Name is the index name. + Name string + // Def is the canonical CREATE INDEX statement. + Def string +} + +// Model is the canonical, comparison-ready description of one table. It +// carries no schema qualification: the live and desired sides are +// introspected under matching search_path settings so their definitions +// compare textually. +type Model struct { + // Table is the unqualified table name. + Table string + // Columns are the table's columns in attribute order. + Columns []Column + // Constraints are the table constraints, name-sorted. + Constraints []Constraint + // Indexes are the non-constraint indexes, name-sorted. + Indexes []Index +} diff --git a/pkg/statement/desired.go b/pkg/statement/desired.go new file mode 100644 index 0000000..3548dc6 --- /dev/null +++ b/pkg/statement/desired.go @@ -0,0 +1,155 @@ +package statement + +import ( + "errors" + "fmt" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// Typed refusals for desired-state schema files. Each names one rule of the +// declarative front door; the caller branches with errors.Is, never on text. +var ( + // ErrEmptyDesired is returned when the input contains no statements. + ErrEmptyDesired = errors.New("desired schema contains no statements") + // ErrNoCreateTable is returned when the input has no CREATE TABLE. + ErrNoCreateTable = errors.New("desired schema must contain a CREATE TABLE") + // ErrMultipleCreateTables is returned for more than one CREATE TABLE: + // the engine is single-table scoped. + ErrMultipleCreateTables = errors.New("desired schema must contain exactly one CREATE TABLE") + // ErrDisallowedStatement is returned for any statement kind other than + // CREATE TABLE / CREATE INDEX. The desired file is executed verbatim on + // a scratch schema, so only pure schema definition is admitted. + ErrDisallowedStatement = errors.New("statement kind not allowed in a desired schema") + // ErrQualifiedName is returned when a statement schema-qualifies its + // target. Desired files are schema-relative; the live schema comes from + // the caller, and qualification could escape the scratch schema. + ErrQualifiedName = errors.New("desired schema statements must use unqualified names") + // ErrConcurrentIndex is returned for CREATE INDEX CONCURRENTLY, which + // cannot run inside the scratch transaction. + ErrConcurrentIndex = errors.New("CONCURRENTLY cannot be used in a desired schema") + // ErrWrongIndexTarget is returned when an index targets a table other + // than the desired CREATE TABLE. + ErrWrongIndexTarget = errors.New("index must target the desired table") +) + +// DesiredSchema is a validated desired-state schema file: exactly one +// CREATE TABLE plus any number of CREATE INDEX statements on that table. +// Statement SQL is canonical (parsed and deparsed through the PostgreSQL +// grammar), in input order, one statement per entry. +type DesiredSchema struct { + // Table is the unqualified name of the single CREATE TABLE target. + Table string + // Statements are the admitted statements, the CREATE TABLE among them. + Statements []Statement +} + +// ParseDesired parses a desired-state schema file and admits only what the +// declarative front door can execute on a scratch schema: one unqualified +// CREATE TABLE and unqualified, non-concurrent CREATE INDEX statements on +// it. Anything else is refused with a typed error. +func ParseDesired(sql string) (DesiredSchema, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return DesiredSchema{}, fmt.Errorf("parse desired schema: %w", err) + } + if len(tree.GetStmts()) == 0 { + return DesiredSchema{}, ErrEmptyDesired + } + var ds DesiredSchema + for i, raw := range tree.GetStmts() { + st, err := admitDesiredStatement(raw.GetStmt(), ds.Table) + if err != nil { + return DesiredSchema{}, fmt.Errorf("statement %d: %w", i+1, err) + } + if st.Kind == KindCreateTable { + ds.Table = st.Table + } + if st.SQL, err = deparseOne(raw.GetStmt()); err != nil { + return DesiredSchema{}, fmt.Errorf("statement %d: %w", i+1, err) + } + ds.Statements = append(ds.Statements, st) + } + if ds.Table == "" { + return DesiredSchema{}, ErrNoCreateTable + } + for _, st := range ds.Statements { + if st.Kind == KindCreateIndex && st.Table != ds.Table { + return DesiredSchema{}, fmt.Errorf("%w: index on %q, desired table is %q", + ErrWrongIndexTarget, st.Table, ds.Table) + } + } + return ds, nil +} + +// admitDesiredStatement applies the per-statement admission rules and +// returns the statement's kind and target. seenTable is the CREATE TABLE +// target admitted so far, empty when none. +func admitDesiredStatement(node *pganalyze.Node, seenTable string) (Statement, error) { + switch { + case node.GetCreateStmt() != nil: + rel := node.GetCreateStmt().GetRelation() + if rel.GetSchemaname() != "" { + return Statement{}, fmt.Errorf("%w: %s.%s", ErrQualifiedName, rel.GetSchemaname(), rel.GetRelname()) + } + if seenTable != "" { + return Statement{}, ErrMultipleCreateTables + } + return Statement{Kind: KindCreateTable, Table: rel.GetRelname()}, nil + case node.GetIndexStmt() != nil: + idx := node.GetIndexStmt() + if idx.GetConcurrent() { + return Statement{}, ErrConcurrentIndex + } + rel := idx.GetRelation() + if rel.GetSchemaname() != "" { + return Statement{}, fmt.Errorf("%w: %s.%s", ErrQualifiedName, rel.GetSchemaname(), rel.GetRelname()) + } + return Statement{Kind: KindCreateIndex, Table: rel.GetRelname()}, nil + default: + return Statement{}, ErrDisallowedStatement + } +} + +// Qualify returns sql with its target relation qualified by schema; an +// empty schema strips an existing qualification instead. It supports exactly +// one CREATE TABLE or CREATE INDEX statement. This touches qualification +// only — no semantics are ever derived or transformed at the AST level +// (that is the scratch database's job). +func Qualify(sql, schema string) (string, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return "", fmt.Errorf("parse statement to qualify: %w", err) + } + if n := len(tree.GetStmts()); n != 1 { + return "", fmt.Errorf("%w: got %d", ErrNotOneStatement, n) + } + node := tree.GetStmts()[0].GetStmt() + var rel *pganalyze.RangeVar + switch { + case node.GetCreateStmt() != nil: + rel = node.GetCreateStmt().GetRelation() + case node.GetIndexStmt() != nil: + rel = node.GetIndexStmt().GetRelation() + default: + return "", ErrDisallowedStatement + } + if rel == nil { + return "", ErrDisallowedStatement + } + rel.Schemaname = schema + return deparseOne(node) +} + +// deparseOne renders a single parsed statement back to canonical SQL through +// the PostgreSQL deparser. +func deparseOne(node *pganalyze.Node) (string, error) { + out, err := pgquery.Deparse(&pganalyze.ParseResult{ + Stmts: []*pganalyze.RawStmt{{Stmt: node}}, + }) + if err != nil { + return "", fmt.Errorf("deparse statement: %w", err) + } + return out, nil +} diff --git a/pkg/statement/desired_test.go b/pkg/statement/desired_test.go new file mode 100644 index 0000000..c5bb7b3 --- /dev/null +++ b/pkg/statement/desired_test.go @@ -0,0 +1,87 @@ +package statement + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseDesiredAdmitsTableAndIndexes(t *testing.T) { + ds, err := ParseDesired(`create table events ( + id bigint primary key, + name varchar(50) not null +); +create index events_name_idx on events (name);`) + require.NoError(t, err) + + assert.Equal(t, "events", ds.Table) + require.Len(t, ds.Statements, 2) + assert.Equal(t, KindCreateTable, ds.Statements[0].Kind) + assert.Equal(t, "CREATE TABLE events (id bigint PRIMARY KEY, name varchar(50) NOT NULL)", ds.Statements[0].SQL) + assert.Equal(t, KindCreateIndex, ds.Statements[1].Kind) + assert.Equal(t, "CREATE INDEX events_name_idx ON events USING btree (name)", ds.Statements[1].SQL) +} + +func TestParseDesiredRefusals(t *testing.T) { + tests := []struct { + name string + sql string + wantErr error + }{ + {"empty input", "", ErrEmptyDesired}, + {"comment only", "-- nothing here", ErrEmptyDesired}, + {"no create table", "CREATE INDEX i ON t (c)", ErrNoCreateTable}, + {"two create tables", "CREATE TABLE a (id int); CREATE TABLE b (id int)", ErrMultipleCreateTables}, + {"dml", "CREATE TABLE t (id int); DELETE FROM t", ErrDisallowedStatement}, + {"alter table", "CREATE TABLE t (id int); ALTER TABLE t ADD COLUMN c int", ErrDisallowedStatement}, + {"drop", "CREATE TABLE t (id int); DROP TABLE other", ErrDisallowedStatement}, + {"qualified table", "CREATE TABLE prod.t (id int)", ErrQualifiedName}, + {"qualified index", "CREATE TABLE t (id int); CREATE INDEX i ON prod.t (id)", ErrQualifiedName}, + {"concurrent index", "CREATE TABLE t (id int); CREATE INDEX CONCURRENTLY i ON t (id)", ErrConcurrentIndex}, + {"index on another table", "CREATE TABLE t (id int); CREATE INDEX i ON other (id)", ErrWrongIndexTarget}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseDesired(tt.sql) + require.ErrorIs(t, err, tt.wantErr) + }) + } +} + +func TestParseDesiredRefusesInvalidSQL(t *testing.T) { + _, err := ParseDesired("CREATE TABEL t (id int)") + require.Error(t, err) +} + +func TestQualify(t *testing.T) { + got, err := Qualify("CREATE INDEX i ON t USING btree (c)", "s1") + require.NoError(t, err) + assert.Equal(t, "CREATE INDEX i ON s1.t USING btree (c)", got) + + got, err = Qualify("CREATE TABLE t (id int)", "s1") + require.NoError(t, err) + assert.Equal(t, "CREATE TABLE s1.t (id int)", got) +} + +func TestQualifyEmptySchemaStripsQualification(t *testing.T) { + got, err := Qualify("CREATE INDEX i ON s1.t USING btree (c)", "") + require.NoError(t, err) + assert.Equal(t, "CREATE INDEX i ON t USING btree (c)", got) +} + +func TestQualifyRefusesOtherStatements(t *testing.T) { + _, err := Qualify("ALTER TABLE t ADD COLUMN c int", "s1") + require.ErrorIs(t, err, ErrDisallowedStatement) + + _, err = Qualify("CREATE TABLE a (id int); CREATE TABLE b (id int)", "s1") + require.ErrorIs(t, err, ErrNotOneStatement) +} + +func TestParseOneRecognizesCreateTable(t *testing.T) { + st, err := ParseOne("CREATE TABLE prod.events (id int)") + require.NoError(t, err) + assert.Equal(t, KindCreateTable, st.Kind) + assert.Equal(t, "prod", st.Schema) + assert.Equal(t, "events", st.Table) +} diff --git a/pkg/statement/statement.go b/pkg/statement/statement.go index 45b2697..7036649 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -24,6 +24,7 @@ const ( KindCreateIndex KindDropIndex KindReindex + KindCreateTable ) // String returns the human-readable name of the kind. @@ -37,6 +38,8 @@ func (k Kind) String() string { return "DROP INDEX" case KindReindex: return "REINDEX" + case KindCreateTable: + return "CREATE TABLE" default: return "other" } @@ -84,6 +87,11 @@ func ParseOne(sql string) (Statement, error) { st.Kind = KindAlterTable st.Schema = alter.GetRelation().GetSchemaname() st.Table = alter.GetRelation().GetRelname() + case node.GetCreateStmt() != nil: + rel := node.GetCreateStmt().GetRelation() + st.Kind = KindCreateTable + st.Schema = rel.GetSchemaname() + st.Table = rel.GetRelname() case node.GetIndexStmt() != nil: st.Kind = KindCreateIndex case node.GetDropStmt() != nil: diff --git a/pkg/statement/statement_test.go b/pkg/statement/statement_test.go index b61c264..f48ae2c 100644 --- a/pkg/statement/statement_test.go +++ b/pkg/statement/statement_test.go @@ -71,7 +71,7 @@ func TestParseOneKinds(t *testing.T) { { name: "create table", sql: "CREATE TABLE t (id int)", - want: Statement{Kind: KindOther}, + want: Statement{Kind: KindCreateTable, Table: "t"}, }, { name: "dml", From 4fcdefc60e03c48a6d72c1b357ee1c166e5b8423 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 22:30:05 +1000 Subject: [PATCH 2/6] Phase 2.2: declarative diff plus diff and fmt commands Diff live vs desired canonical models into dependency-ordered SQL changes with a destructive marker and typed refusals for unsupported identity/generation changes. `diff` emits an executable text plan or a JSON report; `fmt` canonicalizes offline via the parser/deparser. --- internal/cli/cli.go | 13 +- internal/cli/diff.go | 162 +++++++++++++ internal/cli/diff_integration_test.go | 175 ++++++++++++++ internal/cli/diff_test.go | 42 ++++ pkg/schemadiff/diff.go | 227 ++++++++++++++++++ pkg/schemadiff/diff_test.go | 194 +++++++++++++++ pkg/schemadiff/schemadiff_integration_test.go | 198 +++++++++++++++ 7 files changed, 1007 insertions(+), 4 deletions(-) create mode 100644 internal/cli/diff.go create mode 100644 internal/cli/diff_integration_test.go create mode 100644 internal/cli/diff_test.go create mode 100644 pkg/schemadiff/diff.go create mode 100644 pkg/schemadiff/diff_test.go create mode 100644 pkg/schemadiff/schemadiff_integration_test.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 157a343..321de51 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -92,23 +92,28 @@ type MigrateCmd struct { // Run implements the migrate subcommand. func (c *MigrateCmd) Run() error { return c.run(context.Background(), os.Stdout) } -// DiffCmd derives statements from a desired-state schema (declarative front-end). +// DiffCmd derives statements from a desired-state schema (declarative +// front-end): introspect the live table, materialize the desired state on a +// rolled-back scratch schema, and print the ordered plan without executing +// anything. type DiffCmd struct { DBFlags `embed:""` Desired string `help:"Path to the desired-state CREATE TABLE .sql file." name:"desired" type:"existingfile" required:""` + Schema string `help:"Schema containing the live table." default:"public"` + JSON bool `help:"Emit the plan as JSON."` } // Run implements the diff subcommand. -func (c *DiffCmd) Run() error { return notImplemented("diff") } +func (c *DiffCmd) Run() error { return c.run(context.Background(), os.Stdout) } // FmtCmd canonicalizes a schema file. It is offline — no database flags. type FmtCmd struct { - Path string `arg:"" optional:"" help:"Schema file to format." type:"existingfile"` + Path string `arg:"" optional:"" help:"Schema file to format; stdin when omitted." type:"existingfile"` } // Run implements the fmt subcommand. -func (c *FmtCmd) Run() error { return notImplemented("fmt") } +func (c *FmtCmd) Run() error { return c.runFmt(os.Stdin, os.Stdout) } // LintCmd checks DDL for unsafe or unsupported patterns. type LintCmd struct{} diff --git a/internal/cli/diff.go b/internal/cli/diff.go new file mode 100644 index 0000000..39f170a --- /dev/null +++ b/internal/cli/diff.go @@ -0,0 +1,162 @@ +package cli + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/statement" +) + +// diffReport is the diff command's JSON output contract. +type diffReport struct { + // Schema is the live schema the diff targeted. + Schema string `json:"schema"` + // Table is the desired (and live) table name. + Table string `json:"table"` + // TableExists reports whether the live table was found; when false the + // changes are the full desired schema. + TableExists bool `json:"table_exists"` + // Changes is the ordered statement plan; empty means the live table + // already matches the desired state. + Changes []schemadiff.Change `json:"changes"` +} + +// run is the diff flow: parse and admit the desired file, introspect the +// live table and the desired state (execute-and-introspect on a rolled-back +// scratch schema), and print the ordered plan. Nothing is ever executed +// against the live table. +func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { + logger := c.diag() + raw, err := os.ReadFile(c.Desired) + if err != nil { + return fmt.Errorf("read desired schema: %w", err) + } + ds, err := statement.ParseDesired(string(raw)) + if err != nil { + return err + } + logger.Debug("desired schema parsed", "table", ds.Table, "statements", len(ds.Statements)) + + pool, err := dbconn.NewPool(ctx, c.Config()) + if err != nil { + return err + } + defer pool.Close() + + report := diffReport{Schema: c.Schema, Table: ds.Table, TableExists: true} + live, err := schemadiff.Introspect(ctx, pool, c.Schema, ds.Table) + switch { + case errors.Is(err, schemadiff.ErrTableNotFound): + // No live table: the plan is the desired schema itself, qualified + // onto the target schema. + report.TableExists = false + if report.Changes, err = qualifiedDesired(ds, c.Schema); err != nil { + return err + } + case err != nil: + return err + default: + desired, err := schemadiff.IntrospectDesired(ctx, pool, ds) + if err != nil { + return err + } + if report.Changes, err = schemadiff.Diff(c.Schema, live, desired); err != nil { + return err + } + } + logger.Debug("diff derived", + "schema", c.Schema, "table", ds.Table, "changes", len(report.Changes), "table_exists", report.TableExists) + + if c.JSON { + return writeJSON(out, report) + } + return writePlanText(out, report) +} + +// qualifiedDesired renders the desired statements as the plan for a table +// that does not exist yet, qualified onto the target schema. +func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.Change, error) { + changes := make([]schemadiff.Change, 0, len(ds.Statements)) + for _, st := range ds.Statements { + qualified, err := statement.Qualify(st.SQL, schema) + if err != nil { + return nil, fmt.Errorf("qualify desired statement: %w", err) + } + changes = append(changes, schemadiff.Change{SQL: qualified}) + } + return changes, nil +} + +// writeJSON emits the report as JSON. +func writeJSON(out io.Writer, report diffReport) error { + if report.Changes == nil { + report.Changes = []schemadiff.Change{} + } + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + if err := enc.Encode(report); err != nil { + return fmt.Errorf("write diff report: %w", err) + } + return nil +} + +// writePlanText emits the plan as an executable SQL script: one statement +// per line, destructive statements flagged with a leading comment line, and +// SQL comments for the no-change and missing-table cases so the output stays +// valid SQL. +func writePlanText(out io.Writer, report diffReport) error { + if len(report.Changes) == 0 { + if _, err := fmt.Fprintln(out, "-- no changes: live table matches the desired schema"); err != nil { + return fmt.Errorf("write plan: %w", err) + } + return nil + } + if !report.TableExists { + if _, err := fmt.Fprintf(out, "-- table %s.%s does not exist; the plan is the full desired schema\n", + report.Schema, report.Table); err != nil { + return fmt.Errorf("write plan: %w", err) + } + } + for _, ch := range report.Changes { + if ch.Destructive { + if _, err := fmt.Fprintln(out, "-- destructive"); err != nil { + return fmt.Errorf("write plan: %w", err) + } + } + if _, err := fmt.Fprintf(out, "%s;\n", ch.SQL); err != nil { + return fmt.Errorf("write plan: %w", err) + } + } + return nil +} + +// runFmt canonicalizes a desired-state schema file: every statement is +// parsed through the PostgreSQL grammar, admitted by the same rules as diff, +// and printed back in the deparser's canonical form. Offline — no database. +func (c *FmtCmd) runFmt(in io.Reader, out io.Writer) error { + var src []byte + var err error + if c.Path == "" { + if src, err = io.ReadAll(in); err != nil { + return fmt.Errorf("read schema from stdin: %w", err) + } + } else if src, err = os.ReadFile(c.Path); err != nil { + return fmt.Errorf("read schema file: %w", err) + } + ds, err := statement.ParseDesired(string(src)) + if err != nil { + return err + } + for _, st := range ds.Statements { + if _, err := fmt.Fprintf(out, "%s;\n", st.SQL); err != nil { + return fmt.Errorf("write formatted schema: %w", err) + } + } + return nil +} diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go new file mode 100644 index 0000000..da8c88a --- /dev/null +++ b/internal/cli/diff_integration_test.go @@ -0,0 +1,175 @@ +package cli + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" +) + +// newDiffCmd builds a DiffCmd with the flag defaults kong would apply, +// pointing at a desired-state file written for the test. +func newDiffCmd(t *testing.T, url, schema, desiredSQL string) *DiffCmd { + t.Helper() + path := filepath.Join(t.TempDir(), "schema.sql") + require.NoError(t, os.WriteFile(path, []byte(desiredSQL), 0o600)) + return &DiffCmd{ + DBFlags: DBFlags{ + URL: url, + LockTimeout: 3 * time.Second, + StatementTimeout: 30 * time.Second, + }, + Desired: path, + Schema: schema, + } +} + +func TestDiffPrintsOrderedPlanJSON(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id bigint PRIMARY KEY, name varchar(20), legacy int)", schema)) + require.NoError(t, err) + + cmd := newDiffCmd(t, url, schema, + "CREATE TABLE events (id bigint PRIMARY KEY, name varchar(50) NOT NULL);\n"+ + "CREATE INDEX events_name_idx ON events (name);") + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + var report diffReport + require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + assert.Equal(t, schema, report.Schema) + assert.Equal(t, "events", report.Table) + assert.True(t, report.TableExists) + + var sqls []string + var destructive []bool + for _, ch := range report.Changes { + sqls = append(sqls, ch.SQL) + destructive = append(destructive, ch.Destructive) + } + assert.Equal(t, []string{ + fmt.Sprintf(`ALTER TABLE "%s"."events" DROP COLUMN "legacy"`, schema), + fmt.Sprintf(`ALTER TABLE "%s"."events" ALTER COLUMN "name" TYPE character varying(50)`, schema), + fmt.Sprintf(`ALTER TABLE "%s"."events" ALTER COLUMN "name" SET NOT NULL`, schema), + fmt.Sprintf("CREATE INDEX events_name_idx ON %s.events USING btree (name)", schema), + }, sqls) + assert.Equal(t, []bool{true, false, false, false}, destructive) +} + +// diff must never write: the live table is bit-identical before and after. +func TestDiffNeverWrites(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id bigint PRIMARY KEY, legacy int)", schema)) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "INSERT INTO %s.events SELECT g, g FROM generate_series(1, 10) g", schema)) + require.NoError(t, err) + + cmd := newDiffCmd(t, url, schema, "CREATE TABLE events (id bigint PRIMARY KEY, name text NOT NULL)") + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + var cols int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM information_schema.columns WHERE table_schema = $1 AND table_name = 'events'`, + schema).Scan(&cols)) + assert.Equal(t, 2, cols, "diff must not change the live table") + var rows int + require.NoError(t, pool.QueryRow(t.Context(), + fmt.Sprintf("SELECT count(*) FROM %s.events", schema)).Scan(&rows)) + assert.Equal(t, 10, rows, "diff must not touch data") +} + +func TestDiffNoChangesEmptyPlan(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id bigint PRIMARY KEY, name text NOT NULL)", schema)) + require.NoError(t, err) + + cmd := newDiffCmd(t, url, schema, "CREATE TABLE events (id bigint PRIMARY KEY, name text NOT NULL)") + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + var report diffReport + require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + assert.True(t, report.TableExists) + assert.Empty(t, report.Changes) +} + +func TestDiffMissingTableEmitsFullDesiredSchema(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + + cmd := newDiffCmd(t, url, schema, + "CREATE TABLE events (id bigint PRIMARY KEY);\nCREATE INDEX events_id_idx ON events (id);") + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + var report diffReport + require.NoError(t, json.Unmarshal([]byte(out.String()), &report)) + assert.False(t, report.TableExists) + var sqls []string + for _, ch := range report.Changes { + sqls = append(sqls, ch.SQL) + } + assert.Equal(t, []string{ + fmt.Sprintf("CREATE TABLE %s.events (id bigint PRIMARY KEY)", schema), + fmt.Sprintf("CREATE INDEX events_id_idx ON %s.events USING btree (id)", schema), + }, sqls) +} + +func TestDiffTextPlanIsExecutableSQL(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.events (id bigint PRIMARY KEY, legacy int)", schema)) + require.NoError(t, err) + + cmd := newDiffCmd(t, url, schema, "CREATE TABLE events (id bigint PRIMARY KEY, name text NOT NULL)") + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + + // The text plan is an executable script: running it converges the table. + _, err = pool.Exec(t.Context(), out.String()) + require.NoError(t, err, "text plan must be executable SQL: %s", out.String()) + + cmd2 := newDiffCmd(t, url, schema, "CREATE TABLE events (id bigint PRIMARY KEY, name text NOT NULL)") + cmd2.JSON = true + var out2 strings.Builder + require.NoError(t, cmd2.run(t.Context(), &out2)) + var report diffReport + require.NoError(t, json.Unmarshal([]byte(out2.String()), &report)) + assert.Empty(t, report.Changes, "executing the text plan must converge the table") +} diff --git a/internal/cli/diff_test.go b/internal/cli/diff_test.go new file mode 100644 index 0000000..bcbcf4f --- /dev/null +++ b/internal/cli/diff_test.go @@ -0,0 +1,42 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" +) + +func TestFmtCanonicalizesFromStdin(t *testing.T) { + cmd := &FmtCmd{} + in := strings.NewReader(`create table events ( + id bigint primary key, + name varchar(50) not null +); +create index events_name_idx on events (name);`) + var out strings.Builder + require.NoError(t, cmd.runFmt(in, &out)) + assert.Equal(t, + "CREATE TABLE events (id bigint PRIMARY KEY, name varchar(50) NOT NULL);\n"+ + "CREATE INDEX events_name_idx ON events USING btree (name);\n", + out.String()) +} + +func TestFmtRefusesDisallowedStatements(t *testing.T) { + cmd := &FmtCmd{} + var out strings.Builder + err := cmd.runFmt(strings.NewReader("CREATE TABLE t (id int); DELETE FROM t"), &out) + require.ErrorIs(t, err, statement.ErrDisallowedStatement) + assert.Empty(t, out.String(), "nothing is written when the input is refused") +} + +func TestFmtRefusesInvalidSQL(t *testing.T) { + cmd := &FmtCmd{} + var out strings.Builder + err := cmd.runFmt(strings.NewReader("CREATE TABEL t (id int)"), &out) + require.Error(t, err) + assert.Empty(t, out.String()) +} diff --git a/pkg/schemadiff/diff.go b/pkg/schemadiff/diff.go new file mode 100644 index 0000000..4d60366 --- /dev/null +++ b/pkg/schemadiff/diff.go @@ -0,0 +1,227 @@ +package schemadiff + +import ( + "errors" + "fmt" + + "github.com/jackc/pgx/v5" + + "github.com/block/pg-sprite/pkg/statement" +) + +// 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. +var ErrUnsupportedChange = errors.New("unsupported schema change") + +// ErrDifferentTables is returned when the two models describe different +// tables — a caller bug, refused rather than diffed. +var ErrDifferentTables = errors.New("models describe different tables") + +// Change is one derived statement of the ordered plan. +type Change struct { + // SQL is the literal statement, without a trailing semicolon. + SQL string `json:"sql"` + // Destructive marks statements that discard data or constraints + // (column and constraint drops). Destructive changes are gated by the + // caller, never executed silently. + Destructive bool `json:"destructive,omitempty"` +} + +// Diff derives the ordered statement list that converges live onto desired. +// Order is dependency-correct: drops first (indexes, then constraints, then +// columns), then column adds and alters, then constraint adds, then index +// creates — so an added column exists before an index or constraint that +// references it. Within each bucket the order is deterministic: attribute +// order for columns, name order for constraints and indexes. schema +// qualifies the emitted statements' table references. +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) + } + table := pgx.Identifier{schema, live.Table}.Sanitize() + + liveCols := columnsByName(live.Columns) + desiredCols := columnsByName(desired.Columns) + liveCons := constraintsByName(live.Constraints) + desiredCons := constraintsByName(desired.Constraints) + liveIdx := indexesByName(live.Indexes) + desiredIdx := indexesByName(desired.Indexes) + + var changes []Change + + // Indexes to drop: gone from desired, or changed (dropped here, + // recreated in the create bucket below). + for _, ix := range live.Indexes { + want, ok := desiredIdx[ix.Name] + if !ok || want.Def != ix.Def { + changes = append(changes, Change{ + SQL: "DROP INDEX " + pgx.Identifier{schema, ix.Name}.Sanitize(), + }) + } + } + + // Constraints to drop: gone from desired, or changed (re-added below). + for _, con := range live.Constraints { + want, ok := desiredCons[con.Name] + if !ok || want.Def != con.Def { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " DROP CONSTRAINT " + pgx.Identifier{con.Name}.Sanitize(), + Destructive: true, + }) + } + } + + // Columns to drop. A rename is indistinguishable from drop+add at the + // catalog level, so it surfaces as exactly that — and the drop is + // flagged destructive for the caller to gate. + for _, col := range live.Columns { + if _, ok := desiredCols[col.Name]; !ok { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " DROP COLUMN " + pgx.Identifier{col.Name}.Sanitize(), + Destructive: true, + }) + } + } + + // Columns to add. + for _, col := range desired.Columns { + if _, ok := liveCols[col.Name]; !ok { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ADD COLUMN " + columnDef(col), + }) + } + } + + // Columns present on both sides: type, default, and nullability deltas. + for _, col := range desired.Columns { + liveCol, ok := liveCols[col.Name] + if !ok { + continue + } + alter, err := alterColumnChanges(table, liveCol, col) + if err != nil { + return nil, err + } + changes = append(changes, alter...) + } + + // Constraints to add: new, or re-added after a definition change. + for _, con := range desired.Constraints { + had, ok := liveCons[con.Name] + if !ok || had.Def != con.Def { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ADD CONSTRAINT " + pgx.Identifier{con.Name}.Sanitize() + " " + con.Def, + }) + } + } + + // Indexes to create: new, or recreated after a definition change. The + // desired definition is server-decompiled and unqualified; only the + // schema qualification is injected. + for _, ix := range desired.Indexes { + had, ok := liveIdx[ix.Name] + if !ok || had.Def != ix.Def { + qualified, err := statement.Qualify(ix.Def, schema) + if err != nil { + return nil, fmt.Errorf("qualify index %s: %w", ix.Name, err) + } + changes = append(changes, Change{SQL: qualified}) + } + } + + return changes, nil +} + +// alterColumnChanges derives the in-place column alterations between two +// versions of the same column. Identity and generation cannot be altered in +// place, so a delta there is refused as unsupported. +func alterColumnChanges(table string, live, desired Column) ([]Change, error) { + if live.Identity != desired.Identity { + return nil, fmt.Errorf("%w: column %q identity change", ErrUnsupportedChange, live.Name) + } + if live.Generated != desired.Generated { + return nil, fmt.Errorf("%w: column %q generated change", ErrUnsupportedChange, live.Name) + } + 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) + } + col := pgx.Identifier{live.Name}.Sanitize() + var changes []Change + if live.Type != desired.Type { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " TYPE " + desired.Type, + }) + } + if !desired.Generated && desired.Identity == IdentityNone && live.Default != desired.Default { + if desired.Default == "" { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP DEFAULT", + }) + } else { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET DEFAULT " + desired.Default, + }) + } + } + if live.NotNull != desired.NotNull { + if desired.NotNull { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET NOT NULL", + }) + } else { + changes = append(changes, Change{ + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP NOT NULL", + }) + } + } + return changes, nil +} + +// columnDef renders a canonical column definition for ADD COLUMN. All parts +// are server-canonical: the type from format_type, expressions from +// pg_get_expr. +func columnDef(c Column) string { + def := pgx.Identifier{c.Name}.Sanitize() + " " + c.Type + switch { + case c.Generated: + def += " GENERATED ALWAYS AS (" + c.Default + ") STORED" + case c.Identity == IdentityAlways: + def += " GENERATED ALWAYS AS IDENTITY" + case c.Identity == IdentityByDefault: + def += " GENERATED BY DEFAULT AS IDENTITY" + case c.Default != "": + def += " DEFAULT " + c.Default + } + if c.NotNull { + def += " NOT NULL" + } + return def +} + +// columnsByName indexes columns for lookup during the diff. +func columnsByName(cols []Column) map[string]Column { + m := make(map[string]Column, len(cols)) + for _, c := range cols { + m[c.Name] = c + } + return m +} + +// constraintsByName indexes constraints for lookup during the diff. +func constraintsByName(cons []Constraint) map[string]Constraint { + m := make(map[string]Constraint, len(cons)) + for _, c := range cons { + m[c.Name] = c + } + return m +} + +// indexesByName indexes indexes for lookup during the diff. +func indexesByName(idxs []Index) map[string]Index { + m := make(map[string]Index, len(idxs)) + for _, ix := range idxs { + m[ix.Name] = ix + } + return m +} diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go new file mode 100644 index 0000000..06d137f --- /dev/null +++ b/pkg/schemadiff/diff_test.go @@ -0,0 +1,194 @@ +package schemadiff + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// base returns a small canonical model to mutate per test. +func base() Model { + return Model{ + Table: "events", + Columns: []Column{ + {Name: "id", Type: "bigint", NotNull: true}, + {Name: "name", Type: "character varying(50)", NotNull: true}, + }, + Constraints: []Constraint{ + {Name: "events_pkey", Def: "PRIMARY KEY (id)"}, + }, + Indexes: []Index{ + {Name: "events_name_idx", Def: "CREATE INDEX events_name_idx ON events USING btree (name)"}, + }, + } +} + +func sqls(changes []Change) []string { + out := make([]string, len(changes)) + for i, c := range changes { + out[i] = c.SQL + } + return out +} + +func TestDiffNoChanges(t *testing.T) { + changes, err := Diff("public", base(), base()) + require.NoError(t, err) + assert.Empty(t, changes) +} + +func TestDiffRefusesDifferentTables(t *testing.T) { + other := base() + other.Table = "users" + _, err := Diff("public", base(), other) + require.ErrorIs(t, err, ErrDifferentTables) +} + +func TestDiffAddColumn(t *testing.T) { + desired := base() + desired.Columns = append(desired.Columns, Column{ + Name: "created_at", Type: "timestamp with time zone", NotNull: true, Default: "now()", + }) + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" ADD COLUMN "created_at" timestamp with time zone DEFAULT now() NOT NULL`, + }, sqls(changes)) + assert.False(t, changes[0].Destructive) +} + +func TestDiffDropColumnIsDestructive(t *testing.T) { + desired := base() + desired.Columns = desired.Columns[:1] // drop "name" + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" DROP COLUMN "name"`, + }, sqls(changes)) + assert.True(t, changes[0].Destructive) +} + +func TestDiffColumnAlterations(t *testing.T) { + desired := base() + desired.Columns[1] = Column{Name: "name", Type: "text", NotNull: false, Default: "'unnamed'::text"} + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" ALTER COLUMN "name" TYPE text`, + `ALTER TABLE "public"."events" ALTER COLUMN "name" SET DEFAULT 'unnamed'::text`, + `ALTER TABLE "public"."events" ALTER COLUMN "name" DROP NOT NULL`, + }, sqls(changes)) +} + +func TestDiffDropDefaultAndSetNotNull(t *testing.T) { + live := base() + live.Columns[1].Default = "'x'::character varying" + live.Columns[1].NotNull = false + changes, err := Diff("public", live, base()) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" ALTER COLUMN "name" DROP DEFAULT`, + `ALTER TABLE "public"."events" ALTER COLUMN "name" SET NOT NULL`, + }, sqls(changes)) +} + +func TestDiffConstraintChangeDropsAndReadds(t *testing.T) { + desired := base() + desired.Constraints = []Constraint{ + {Name: "events_pkey", Def: "PRIMARY KEY (id, name)"}, + } + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" DROP CONSTRAINT "events_pkey"`, + `ALTER TABLE "public"."events" ADD CONSTRAINT "events_pkey" PRIMARY KEY (id, name)`, + }, sqls(changes)) + assert.True(t, changes[0].Destructive) + assert.False(t, changes[1].Destructive) +} + +func TestDiffIndexChangeDropsAndRecreatesQualified(t *testing.T) { + desired := base() + desired.Indexes = []Index{ + {Name: "events_name_idx", Def: "CREATE UNIQUE INDEX events_name_idx ON events USING btree (name)"}, + } + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `DROP INDEX "public"."events_name_idx"`, + `CREATE UNIQUE INDEX events_name_idx ON public.events USING btree (name)`, + }, sqls(changes)) +} + +func TestDiffOrderingDropsBeforeAddsBeforeIndexes(t *testing.T) { + live := base() + live.Columns = append(live.Columns, Column{Name: "legacy", Type: "integer"}) + + desired := base() + desired.Columns = append(desired.Columns, Column{Name: "email", Type: "text", NotNull: true}) + desired.Indexes = append(desired.Indexes, Index{ + Name: "events_email_idx", Def: "CREATE INDEX events_email_idx ON events USING btree (email)", + }) + + changes, err := Diff("public", live, desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" DROP COLUMN "legacy"`, + `ALTER TABLE "public"."events" ADD COLUMN "email" text NOT NULL`, + `CREATE INDEX events_email_idx ON public.events USING btree (email)`, + }, sqls(changes)) +} + +func TestDiffRefusesIdentityAndGeneratedChanges(t *testing.T) { + tests := []struct { + name string + mutate func(*Column) + }{ + {"identity change", func(c *Column) { c.Identity = IdentityAlways }}, + {"generated change", func(c *Column) { c.Generated = true; c.Default = "(id + 1)" }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + desired := base() + tt.mutate(&desired.Columns[0]) + _, err := Diff("public", base(), desired) + require.ErrorIs(t, err, ErrUnsupportedChange) + }) + } +} + +func TestDiffRefusesGenerationExpressionChange(t *testing.T) { + live := base() + live.Columns[0] = Column{Name: "id", Type: "bigint", Generated: true, Default: "(1)"} + desired := base() + desired.Columns[0] = Column{Name: "id", Type: "bigint", Generated: true, Default: "(2)"} + _, err := Diff("public", live, desired) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + +func TestDiffIdentityColumnAddRendersIdentity(t *testing.T) { + desired := base() + desired.Columns = append(desired.Columns, + Column{Name: "seq_a", Type: "bigint", NotNull: true, Identity: IdentityAlways}, + Column{Name: "seq_d", Type: "bigint", NotNull: true, Identity: IdentityByDefault}, + ) + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" ADD COLUMN "seq_a" bigint GENERATED ALWAYS AS IDENTITY NOT NULL`, + `ALTER TABLE "public"."events" ADD COLUMN "seq_d" bigint GENERATED BY DEFAULT AS IDENTITY NOT NULL`, + }, sqls(changes)) +} + +func TestDiffGeneratedColumnAddRendersGenerationExpression(t *testing.T) { + desired := base() + desired.Columns = append(desired.Columns, Column{ + Name: "name_upper", Type: "text", Generated: true, Default: "upper((name)::text)", + }) + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."events" ADD COLUMN "name_upper" text GENERATED ALWAYS AS (upper((name)::text)) STORED`, + }, sqls(changes)) +} diff --git a/pkg/schemadiff/schemadiff_integration_test.go b/pkg/schemadiff/schemadiff_integration_test.go new file mode 100644 index 0000000..cf14829 --- /dev/null +++ b/pkg/schemadiff/schemadiff_integration_test.go @@ -0,0 +1,198 @@ +package schemadiff_test + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/statement" +) + +const desiredSQL = ` +CREATE TABLE events ( + id bigint PRIMARY KEY, + name varchar(50) NOT NULL, + payload jsonb DEFAULT '{}'::jsonb, + created_at timestamptz NOT NULL DEFAULT now(), + CONSTRAINT name_not_empty CHECK (length(name) > 0) +); +CREATE INDEX events_created_at_idx ON events (created_at); +CREATE UNIQUE INDEX events_name_key ON events (name); +` + +// The two-oracle test of execute-and-introspect: creating the table live and +// materializing the same file on the scratch schema must introspect to the +// identical canonical model. +func TestIntrospectDesiredMatchesLiveIntrospection(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) + + ds, err := statement.ParseDesired(desiredSQL) + require.NoError(t, err) + for _, st := range ds.Statements { + qualified, err := statement.Qualify(st.SQL, schema) + require.NoError(t, err) + _, err = pool.Exec(t.Context(), qualified) + require.NoError(t, err) + } + + live, err := schemadiff.Introspect(t.Context(), pool, schema, "events") + require.NoError(t, err) + desired, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + + assert.Equal(t, live, desired, + "live introspection and scratch execute-and-introspect must agree on the canonical model") +} + +// Cosmetically different spellings of the same schema must introspect to the +// same canonical model: the server's decompilers are the canonicalizer. +func TestIntrospectCanonicalizesTypeAliases(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.aliased (a int4, b varchar(50), c timestamptz, d bool DEFAULT TRUE)", schema)) + require.NoError(t, err) + + m, err := schemadiff.Introspect(t.Context(), pool, schema, "aliased") + require.NoError(t, err) + require.Len(t, m.Columns, 4) + assert.Equal(t, "integer", m.Columns[0].Type) + assert.Equal(t, "character varying(50)", m.Columns[1].Type) + assert.Equal(t, "timestamp with time zone", m.Columns[2].Type) + assert.Equal(t, "boolean", m.Columns[3].Type) + assert.Equal(t, "true", m.Columns[3].Default) +} + +func TestIntrospectDesiredLeavesNoFootprint(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + + ds, err := statement.ParseDesired(desiredSQL) + require.NoError(t, err) + _, err = schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + + var leftover int + err = pool.QueryRow(t.Context(), + "SELECT count(*) FROM pg_namespace WHERE nspname LIKE 'pgsprite\\_scratch\\_%'").Scan(&leftover) + require.NoError(t, err) + assert.Zero(t, leftover, "the scratch schema must never survive its transaction") +} + +func TestIntrospectTableNotFound(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 = schemadiff.Introspect(t.Context(), pool, schema, "missing") + require.ErrorIs(t, err, schemadiff.ErrTableNotFound) +} + +func TestIntrospectRefusesViews(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 VIEW %s.v AS SELECT 1 AS one", schema)) + require.NoError(t, err) + + _, err = schemadiff.Introspect(t.Context(), pool, schema, "v") + require.ErrorIs(t, err, schemadiff.ErrNotTable) +} + +// A desired statement that is valid grammar but invalid semantics (a type +// that does not exist) must fail at scratch execution — semantic truth comes +// from the server, not the parser. +func TestIntrospectDesiredSurfacesSemanticErrors(t *testing.T) { + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: testutil.StartPostgres(t)}) + require.NoError(t, err) + defer pool.Close() + + ds, err := statement.ParseDesired("CREATE TABLE t (id no_such_type)") + require.NoError(t, err, "the grammar accepts unknown type names; only the server can refuse them") + _, err = schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.Error(t, err) +} + +// The convergence oracle: diff live against desired, execute the plan, and +// the re-diff must be empty. This closes the loop between the diff engine +// and the real server semantics. +func TestDiffConverges(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) + + // The live table starts from an older shape: a column to drop, a column + // to retype, a default to add, an index to replace. + for _, ddl := range []string{ + fmt.Sprintf(`CREATE TABLE %s.events ( + id bigint PRIMARY KEY, + name varchar(20) NOT NULL, + legacy int, + created_at timestamptz NOT NULL + )`, schema), + fmt.Sprintf("CREATE INDEX events_created_at_idx ON %s.events (created_at DESC)", schema), + } { + _, err := pool.Exec(t.Context(), ddl) + require.NoError(t, err) + } + + ds, err := statement.ParseDesired(desiredSQL) + require.NoError(t, err) + + live, err := schemadiff.Introspect(t.Context(), pool, schema, "events") + require.NoError(t, err) + desired, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + changes, err := schemadiff.Diff(schema, live, desired) + require.NoError(t, err) + require.NotEmpty(t, changes) + + for _, ch := range changes { + _, err := pool.Exec(t.Context(), ch.SQL) + require.NoError(t, err, "derived statement must execute: %s", ch.SQL) + } + + after, err := schemadiff.Introspect(t.Context(), pool, schema, "events") + require.NoError(t, err) + rediff, err := schemadiff.Diff(schema, after, desired) + require.NoError(t, err) + assert.Empty(t, rediff, "after executing the plan the live table must match the desired state") +} + +// Identity and generated columns round-trip through both introspection paths. +func TestIntrospectIdentityAndGeneratedColumns(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.gen ( + id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY, + n int NOT NULL, + doubled int GENERATED ALWAYS AS (n * 2) STORED + )`, schema)) + require.NoError(t, err) + + m, err := schemadiff.Introspect(t.Context(), pool, schema, "gen") + require.NoError(t, err) + require.Len(t, m.Columns, 3) + assert.Equal(t, schemadiff.IdentityAlways, m.Columns[0].Identity) + assert.True(t, m.Columns[2].Generated) + assert.Equal(t, "(n * 2)", m.Columns[2].Default) +} From 68814979d84249772b48fbff1752e18cc770e74d Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 4 Aug 2026 22:32:36 +1000 Subject: [PATCH 3/6] ci: pin golangci-lint v2 and name test matrix jobs The unpinned lint action installed a v1 binary built with an older Go than the module targets, so it could not load the v2 config. Matrix jobs now render as "test (PostgreSQL NN)" in the checks UI. --- .github/workflows/ci.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4fc3def..16f01d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,7 +60,11 @@ jobs: - uses: actions/setup-go@v5 with: go-version-file: go.mod - - uses: golangci/golangci-lint-action@v6 + # Pin the same golangci-lint major used locally (v2 config format); + # the action's default binary lags and cannot load a v2 config. + - uses: golangci/golangci-lint-action@v9 + with: + version: v2.12.2 build: needs: changes @@ -79,6 +83,7 @@ jobs: # images — real Aurora engine-version validation is a separate gate that # cannot run in public CI. test: + name: test (PostgreSQL ${{ matrix.pg }}) needs: changes if: needs.changes.outputs.code == 'true' runs-on: ubuntu-latest From f61d263fdbf2569435b2044e0c88b140ca4ae0dd Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 16:06:52 +1000 Subject: [PATCH 4/6] ci: rename workflow to CI and the ci-ok sentinel to all-green Checks now render as CI / lint, CI / test (PostgreSQL NN), and CI / all-green instead of the ci / ci-ok stutter. "all-green" is the context to require once branch protection is wired. --- .github/workflows/ci.yml | 9 +++++---- docs/testing.md | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 16f01d9..16fc3bd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,4 +1,4 @@ -name: ci +name: CI on: push: @@ -100,9 +100,10 @@ jobs: go-version-file: go.mod - run: make test - # Single required status for branch protection. Succeeds when nothing - # failed — including docs-only PRs where the heavy jobs were skipped. - ci-ok: + # Single required status for branch protection ("all-green" is the + # context to require). Succeeds when nothing failed — including + # docs-only PRs where the heavy jobs were skipped. + all-green: if: always() needs: [changes, lint, build, test] runs-on: ubuntu-latest diff --git a/docs/testing.md b/docs/testing.md index 184ed42..c57861d 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -26,8 +26,9 @@ Concretely: - **Real database, race-enabled.** Unit tests run with `-race`; core (`pkg/`) logic is never validated against mocks — integration tests run against real PostgreSQL, across every supported major in CI. -- **The matrix is a gate, not advisory.** The `ci-ok` sentinel requires the - full version matrix; docs-only changes are the only path that skips it. +- **The matrix is a gate, not advisory.** The `all-green` sentinel job + requires the full version matrix; docs-only changes are the only path + that skips it. - **Coverage never regresses.** Deleting or skipping a test to get green is forbidden (same rule as the hooks: no `--no-verify`, no `nolint`). A numeric coverage ratchet on `pkg/` packages is wired into CI once Phase 1 From 14c9dee89ab115023e972a05a2537fc3619d3d4a Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 16:11:46 +1000 Subject: [PATCH 5/6] ci: pin golangci-lint-action to commit SHA Semgrep and zizmor code-scanning gates require third-party actions pinned to a full commit SHA, matching the existing paths-filter pin. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 16fc3bd..d21e296 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,7 +62,7 @@ jobs: go-version-file: go.mod # Pin the same golangci-lint major used locally (v2 config format); # the action's default binary lags and cannot load a v2 config. - - uses: golangci/golangci-lint-action@v9 + - uses: golangci/golangci-lint-action@ba0d7d2ec06a0ea1cb5fa41b2e4a3ab91d21278a # v9.3.0 with: version: v2.12.2 From 4e24b46f4445a3f9159ef53521a70b9a92baab8e Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 7 Aug 2026 09:20:44 +1000 Subject: [PATCH 6/6] Address PR #6 review: FK refusal, serial adoption, change kinds, fmt comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from the two-lens and adversarial reviews at 14c9dee: - refuse REFERENCES at desired-file admission (typed ErrForeignKey) — the scratch transaction cannot faithfully bind an unqualified FK - refuse serial/sequence-backed default adoption (ErrUnsupportedChange) via pg_depend-backed Column.SequenceDefault — the plan would name a sequence that only existed in the rolled-back scratch transaction - classify every Change with a typed Kind; mark DROP INDEX destructive - annotate lock-hazardous statements in the text plan and point the header at migrate as the executing front door - fmt fails closed on commented input (ErrCommentLoss) instead of silently discarding comments - document column-order-by-name convergence and the two-canon boundary --- docs/low-level-design.md | 19 +++- internal/cli/diff.go | 48 +++++++++- internal/cli/diff_integration_test.go | 9 ++ internal/cli/diff_test.go | 14 +++ pkg/schemadiff/diff.go | 92 ++++++++++++++++--- pkg/schemadiff/diff_test.go | 71 ++++++++++++++ pkg/schemadiff/introspect.go | 11 ++- pkg/schemadiff/schemadiff.go | 6 ++ pkg/schemadiff/schemadiff_integration_test.go | 49 ++++++++++ pkg/statement/comments.go | 31 +++++++ pkg/statement/comments_test.go | 31 +++++++ pkg/statement/desired.go | 35 ++++++- pkg/statement/desired_test.go | 3 + pkg/statement/statement.go | 7 ++ 14 files changed, 403 insertions(+), 23 deletions(-) create mode 100644 pkg/statement/comments.go create mode 100644 pkg/statement/comments_test.go diff --git a/docs/low-level-design.md b/docs/low-level-design.md index e27ed12..32bc1c1 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -249,15 +249,28 @@ live schema (introspected) ─┘ │ 4. **Diff** the two models and emit the minimal set of statements: `ADD/DROP/ALTER COLUMN`, `ADD/DROP CONSTRAINT`, `CREATE/DROP INDEX`, default/nullability changes, etc., in a **dependency-correct order** (e.g. add a column before an index that references it). + Columns are compared **by name**: a live table whose columns are ordered differently from + the desired file converges to "no changes". Attribute order carries no semantics in + PostgreSQL and cannot be changed in place, so — unlike some declarative MySQL tooling — + column order is deliberately out of scope for convergence. 5. **Hand the derived statements to the same classifier**, so a declarative change that turns out to be, say, a binary-coercible type widening still takes the native fast path, and only a genuine rewrite triggers a copy. ### Safety rules (inherited philosophy: surprise-free, decisions-not-options) -- **Destructive diffs are gated.** Dropping a column or constraint, or anything that loses - data, requires an explicit confirmation flag — never inferred silently from "it's missing in - the desired file". +- **Destructive diffs are gated.** Dropping a column, constraint, or index — anything that + loses data or a guarantee (a unique index discards the same uniqueness guarantee as a unique + constraint) — requires an explicit confirmation flag — never inferred silently from "it's + missing in the desired file". +- **Unsupported constructs are refused, never guessed.** The desired file admits one + unqualified `CREATE TABLE` plus `CREATE INDEX` statements on it; each rule is a typed error. + Foreign keys are refused at admission — a `REFERENCES` clause cannot be faithfully executed + in the transaction-scoped scratch schema (an unqualified reference resolves against the + scratch search_path, not the target schema), and FK support needs its own design. Changes + the plan cannot express — identity or generation changes on an existing column, adopting a + sequence-backed (serial) default whose sequence only existed in the rolled-back scratch + transaction — are refused as unsupported rather than emitted as an unexecutable plan. - **Renames are ambiguous and are not guessed.** A column present in live but absent in desired plus a new column in desired is, by default, a *drop + add*, not a rename. Rename intent must be stated explicitly (the engine will not heuristically pair columns), mirroring Spirit's diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 46be182..a7e2556 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -88,7 +88,11 @@ func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.C if err != nil { return nil, fmt.Errorf("qualify desired statement: %w", err) } - changes = append(changes, schemadiff.Change{SQL: qualified}) + kind := schemadiff.ChangeCreateTable + if st.Kind() == statement.KindCreateIndex { + kind = schemadiff.ChangeCreateIndex + } + changes = append(changes, schemadiff.Change{SQL: qualified, Kind: kind}) } return changes, nil } @@ -107,9 +111,11 @@ func writeJSON(out io.Writer, report diffReport) error { } // writePlanText emits the plan as an executable SQL script: one statement -// per line, destructive statements flagged with a leading comment line, and -// SQL comments for the no-change and missing-table cases so the output stays -// valid SQL. +// per line, destructive and lock-hazardous statements flagged with leading +// comment lines, and SQL comments for the no-change and missing-table cases +// so the output stays valid SQL. The header points at migrate as the +// executing front door: running this script directly bypasses the gate that +// refuses blocking statements. func writePlanText(out io.Writer, report diffReport) error { if len(report.Changes) == 0 { if _, err := fmt.Fprintln(out, "-- no changes: live table matches the desired schema"); err != nil { @@ -117,6 +123,12 @@ func writePlanText(out io.Writer, report diffReport) error { } return nil } + if _, err := fmt.Fprintln(out, "-- plan derived by pg-sprite diff; execute statements via pg-sprite migrate,"); err != nil { + return fmt.Errorf("write plan: %w", err) + } + if _, err := fmt.Fprintln(out, "-- which refuses blocking forms — running this script directly bypasses that gate"); err != nil { + return fmt.Errorf("write plan: %w", err) + } if !report.TableExists { if _, err := fmt.Fprintf(out, "-- table %s.%s does not exist; the plan is the full desired schema\n", report.Schema, report.Table); err != nil { @@ -129,6 +141,11 @@ func writePlanText(out io.Writer, report diffReport) error { return fmt.Errorf("write plan: %w", err) } } + if hazard := lockHazard(ch.Kind); hazard != "" { + if _, err := fmt.Fprintf(out, "-- %s\n", hazard); err != nil { + return fmt.Errorf("write plan: %w", err) + } + } if _, err := fmt.Fprintf(out, "%s;\n", ch.SQL); err != nil { return fmt.Errorf("write plan: %w", err) } @@ -136,9 +153,29 @@ func writePlanText(out io.Writer, report diffReport) error { return nil } +// lockHazard describes the blocking behavior of a change kind, empty when +// the statement is not expected to block writers. This is presentation for +// the text plan; the machine contract is the kind itself. +func lockHazard(kind schemadiff.ChangeKind) string { + switch kind { + case schemadiff.ChangeAlterType: + return "rewrites the table under ACCESS EXCLUSIVE, blocking reads and writes" + case schemadiff.ChangeSetNotNull: + return "full table scan under ACCESS EXCLUSIVE" + case schemadiff.ChangeAddConstraint: + return "validation scan or index build that blocks writes" + case schemadiff.ChangeCreateIndex: + return "blocks writes for the whole index build" + default: + return "" + } +} + // runFmt canonicalizes a desired-state schema file: every statement is // parsed through the PostgreSQL grammar, admitted by the same rules as diff, // and printed back in the deparser's canonical form. Offline — no database. +// Commented input is refused (statement.ErrCommentLoss): the parser drops +// comments, and a formatter must never silently discard content. func (c *FmtCmd) runFmt(in io.Reader, out io.Writer) error { var src []byte var err error @@ -149,6 +186,9 @@ func (c *FmtCmd) runFmt(in io.Reader, out io.Writer) error { } else if src, err = os.ReadFile(c.Path); err != nil { return fmt.Errorf("read schema file: %w", err) } + if err := statement.CheckNoComments(string(src)); err != nil { + return err + } ds, err := statement.ParseDesired(string(src)) if err != nil { return err diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go index da8c88a..7d1f85c 100644 --- a/internal/cli/diff_integration_test.go +++ b/internal/cli/diff_integration_test.go @@ -14,6 +14,7 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/schemadiff" ) // newDiffCmd builds a DiffCmd with the flag defaults kong would apply, @@ -57,9 +58,11 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { assert.True(t, report.TableExists) var sqls []string + var kinds []schemadiff.ChangeKind var destructive []bool for _, ch := range report.Changes { sqls = append(sqls, ch.SQL) + kinds = append(kinds, ch.Kind) destructive = append(destructive, ch.Destructive) } assert.Equal(t, []string{ @@ -68,6 +71,12 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { fmt.Sprintf(`ALTER TABLE "%s"."events" ALTER COLUMN "name" SET NOT NULL`, schema), fmt.Sprintf("CREATE INDEX events_name_idx ON %s.events USING btree (name)", schema), }, sqls) + assert.Equal(t, []schemadiff.ChangeKind{ + schemadiff.ChangeDropColumn, + schemadiff.ChangeAlterType, + schemadiff.ChangeSetNotNull, + schemadiff.ChangeCreateIndex, + }, kinds) assert.Equal(t, []bool{true, false, false, false}, destructive) } diff --git a/internal/cli/diff_test.go b/internal/cli/diff_test.go index bcbcf4f..35a6bab 100644 --- a/internal/cli/diff_test.go +++ b/internal/cli/diff_test.go @@ -33,6 +33,20 @@ func TestFmtRefusesDisallowedStatements(t *testing.T) { assert.Empty(t, out.String(), "nothing is written when the input is refused") } +func TestFmtRefusesCommentedInput(t *testing.T) { + cmd := &FmtCmd{} + var out strings.Builder + err := cmd.runFmt(strings.NewReader(`-- events: one row per business event +CREATE TABLE events ( + id bigint PRIMARY KEY, + name varchar(50) NOT NULL -- display name +); +-- covering index for the dashboard query +CREATE INDEX events_name_idx ON events (name);`), &out) + require.ErrorIs(t, err, statement.ErrCommentLoss) + assert.Empty(t, out.String(), "a formatter must never emit output that lost content") +} + func TestFmtRefusesInvalidSQL(t *testing.T) { cmd := &FmtCmd{} var out strings.Builder diff --git a/pkg/schemadiff/diff.go b/pkg/schemadiff/diff.go index 4d60366..896d636 100644 --- a/pkg/schemadiff/diff.go +++ b/pkg/schemadiff/diff.go @@ -18,13 +18,50 @@ var ErrUnsupportedChange = errors.New("unsupported schema change") // tables — a caller bug, refused rather than diffed. var ErrDifferentTables = errors.New("models describe different tables") +// ChangeKind classifies a derived statement so a consumer can gate whole +// classes of change (destructive, rewriting, index-building) without +// parsing SQL. +type ChangeKind string + +// The change kinds a plan can contain. +const ( + // ChangeCreateTable creates the table (missing-table plans only). + ChangeCreateTable ChangeKind = "create-table" + // ChangeDropIndex drops an index. + ChangeDropIndex ChangeKind = "drop-index" + // ChangeDropConstraint drops a table constraint. + ChangeDropConstraint ChangeKind = "drop-constraint" + // ChangeDropColumn drops a column. + ChangeDropColumn ChangeKind = "drop-column" + // ChangeAddColumn adds a column. + ChangeAddColumn ChangeKind = "add-column" + // ChangeAlterType changes a column's type. + ChangeAlterType ChangeKind = "alter-type" + // ChangeSetDefault sets or replaces a column default. + ChangeSetDefault ChangeKind = "set-default" + // ChangeDropDefault drops a column default. + ChangeDropDefault ChangeKind = "drop-default" + // ChangeSetNotNull adds the NOT NULL attribute. + ChangeSetNotNull ChangeKind = "set-not-null" + // ChangeDropNotNull removes the NOT NULL attribute. + ChangeDropNotNull ChangeKind = "drop-not-null" + // ChangeAddConstraint adds a table constraint. + ChangeAddConstraint ChangeKind = "add-constraint" + // ChangeCreateIndex creates an index. + ChangeCreateIndex ChangeKind = "create-index" +) + // Change is one derived statement of the ordered plan. type Change struct { // SQL is the literal statement, without a trailing semicolon. SQL string `json:"sql"` - // Destructive marks statements that discard data or constraints - // (column and constraint drops). Destructive changes are gated by the - // caller, never executed silently. + // Kind classifies the statement for consumers that gate by class. + Kind ChangeKind `json:"kind"` + // Destructive marks statements that discard data, constraints, or + // indexes (column, constraint, and index drops — dropping a unique + // index discards the same guarantee as dropping a unique constraint). + // Destructive changes are gated by the caller, never executed + // silently. Destructive bool `json:"destructive,omitempty"` } @@ -34,7 +71,9 @@ type Change struct { // creates — so an added column exists before an index or constraint that // references it. Within each bucket the order is deterministic: attribute // order for columns, name order for constraints and indexes. schema -// qualifies the emitted statements' table references. +// qualifies the emitted statements' table references. Columns are compared +// by name only: attribute order carries no semantics in PostgreSQL and is +// deliberately out of scope for convergence. 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) @@ -56,7 +95,9 @@ func Diff(schema string, live, desired Model) ([]Change, error) { want, ok := desiredIdx[ix.Name] if !ok || want.Def != ix.Def { changes = append(changes, Change{ - SQL: "DROP INDEX " + pgx.Identifier{schema, ix.Name}.Sanitize(), + SQL: "DROP INDEX " + pgx.Identifier{schema, ix.Name}.Sanitize(), + Kind: ChangeDropIndex, + Destructive: true, }) } } @@ -67,6 +108,7 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if !ok || want.Def != con.Def { changes = append(changes, Change{ SQL: "ALTER TABLE " + table + " DROP CONSTRAINT " + pgx.Identifier{con.Name}.Sanitize(), + Kind: ChangeDropConstraint, Destructive: true, }) } @@ -79,16 +121,24 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if _, ok := desiredCols[col.Name]; !ok { changes = append(changes, Change{ SQL: "ALTER TABLE " + table + " DROP COLUMN " + pgx.Identifier{col.Name}.Sanitize(), + Kind: ChangeDropColumn, Destructive: true, }) } } - // Columns to add. + // Columns to add. A sequence-backed default (serial) cannot be added: + // the desired-side sequence existed only inside the rolled-back + // scratch transaction, so the emitted default would reference a + // relation the plan never creates. for _, col := range desired.Columns { if _, ok := liveCols[col.Name]; !ok { + 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) + } changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ADD COLUMN " + columnDef(col), + SQL: "ALTER TABLE " + table + " ADD COLUMN " + columnDef(col), + Kind: ChangeAddColumn, }) } } @@ -111,7 +161,8 @@ func Diff(schema string, live, desired Model) ([]Change, error) { had, ok := liveCons[con.Name] if !ok || had.Def != con.Def { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ADD CONSTRAINT " + pgx.Identifier{con.Name}.Sanitize() + " " + con.Def, + SQL: "ALTER TABLE " + table + " ADD CONSTRAINT " + pgx.Identifier{con.Name}.Sanitize() + " " + con.Def, + Kind: ChangeAddConstraint, }) } } @@ -126,7 +177,7 @@ func Diff(schema string, live, desired Model) ([]Change, error) { if err != nil { return nil, fmt.Errorf("qualify index %s: %w", ix.Name, err) } - changes = append(changes, Change{SQL: qualified}) + changes = append(changes, Change{SQL: qualified, Kind: ChangeCreateIndex}) } } @@ -150,28 +201,41 @@ func alterColumnChanges(table string, live, desired Column) ([]Change, error) { var changes []Change if live.Type != desired.Type { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " TYPE " + desired.Type, + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " TYPE " + desired.Type, + Kind: ChangeAlterType, }) } if !desired.Generated && desired.Identity == IdentityNone && live.Default != desired.Default { + // A sequence-backed desired default (serial adoption) is refused: + // the sequence existed only inside the rolled-back scratch + // transaction, so the emitted SET DEFAULT would reference a + // relation the plan never creates — or worse, silently bind to an + // unrelated live sequence of the same name. + if desired.SequenceDefault { + return nil, fmt.Errorf("%w: column %q would adopt a sequence-backed default (serial); the plan cannot create its sequence", ErrUnsupportedChange, live.Name) + } if desired.Default == "" { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP DEFAULT", + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP DEFAULT", + Kind: ChangeDropDefault, }) } else { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET DEFAULT " + desired.Default, + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET DEFAULT " + desired.Default, + Kind: ChangeSetDefault, }) } } if live.NotNull != desired.NotNull { if desired.NotNull { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET NOT NULL", + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " SET NOT NULL", + Kind: ChangeSetNotNull, }) } else { changes = append(changes, Change{ - SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP NOT NULL", + SQL: "ALTER TABLE " + table + " ALTER COLUMN " + col + " DROP NOT NULL", + Kind: ChangeDropNotNull, }) } } diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go index 06d137f..7617153 100644 --- a/pkg/schemadiff/diff_test.go +++ b/pkg/schemadiff/diff_test.go @@ -69,6 +69,77 @@ func TestDiffDropColumnIsDestructive(t *testing.T) { assert.True(t, changes[0].Destructive) } +func TestDiffDropIndexIsDestructive(t *testing.T) { + desired := base() + desired.Indexes = nil + changes, err := Diff("public", base(), desired) + require.NoError(t, err) + require.Equal(t, []string{ + `DROP INDEX "public"."events_name_idx"`, + }, sqls(changes)) + assert.True(t, changes[0].Destructive) + assert.Equal(t, ChangeDropIndex, changes[0].Kind) +} + +func TestDiffRefusesSequenceDefaultAdoption(t *testing.T) { + desired := base() + desired.Columns[0].Default = "nextval('events_id_seq'::regclass)" + desired.Columns[0].SequenceDefault = true + _, err := Diff("public", base(), desired) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + +func TestDiffRefusesSequenceDefaultOnAddedColumn(t *testing.T) { + desired := base() + desired.Columns = append(desired.Columns, Column{ + Name: "seq_col", Type: "integer", + Default: "nextval('events_seq_col_seq'::regclass)", SequenceDefault: true, + }) + _, err := Diff("public", base(), desired) + require.ErrorIs(t, err, ErrUnsupportedChange) +} + +func TestDiffIdenticalSequenceDefaultsConverge(t *testing.T) { + withSerial := func() Model { + m := base() + m.Columns[0].Default = "nextval('events_id_seq'::regclass)" + m.Columns[0].SequenceDefault = true + return m + } + changes, err := Diff("public", withSerial(), withSerial()) + require.NoError(t, err) + assert.Empty(t, changes) +} + +func TestDiffChangeKinds(t *testing.T) { + live := base() + live.Columns = append(live.Columns, Column{Name: "legacy", Type: "integer"}) + + desired := base() + desired.Columns[1] = Column{Name: "name", Type: "text", NotNull: true} + desired.Columns = append(desired.Columns, Column{Name: "email", Type: "text"}) + desired.Constraints = append(desired.Constraints, Constraint{ + Name: "events_email_key", Def: "UNIQUE (email)", + }) + desired.Indexes = append(desired.Indexes, Index{ + Name: "events_email_idx", Def: "CREATE INDEX events_email_idx ON events USING btree (email)", + }) + + changes, err := Diff("public", live, desired) + require.NoError(t, err) + kinds := make([]ChangeKind, len(changes)) + for i, c := range changes { + kinds[i] = c.Kind + } + assert.Equal(t, []ChangeKind{ + ChangeDropColumn, + ChangeAddColumn, + ChangeAlterType, + ChangeAddConstraint, + ChangeCreateIndex, + }, kinds) +} + func TestDiffColumnAlterations(t *testing.T) { desired := base() desired.Columns[1] = Column{Name: "name", Type: "text", NotNull: false, Default: "'unnamed'::text"} diff --git a/pkg/schemadiff/introspect.go b/pkg/schemadiff/introspect.go index 34d212c..8c6867d 100644 --- a/pkg/schemadiff/introspect.go +++ b/pkg/schemadiff/introspect.go @@ -93,6 +93,15 @@ func introspectColumns(ctx context.Context, tx pgx.Tx, oid uint32) ([]Column, er format_type(a.atttypid, a.atttypmod), a.attnotnull, COALESCE(pg_get_expr(d.adbin, d.adrelid), ''), + COALESCE(( + SELECT true + FROM pg_depend dep + JOIN pg_class s ON s.oid = dep.refobjid AND s.relkind = 'S' + WHERE dep.classid = 'pg_attrdef'::regclass + AND dep.objid = d.oid + AND dep.refclassid = 'pg_class'::regclass + LIMIT 1 + ), false), a.attidentity::text, a.attgenerated::text FROM pg_attribute a @@ -107,7 +116,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, &identity, &generated); err != nil { + if err := rows.Scan(&c.Name, &c.Type, &c.NotNull, &c.Default, &c.SequenceDefault, &identity, &generated); err != nil { return nil, fmt.Errorf("scan column: %w", err) } c.Identity = Identity(identity) diff --git a/pkg/schemadiff/schemadiff.go b/pkg/schemadiff/schemadiff.go index 1cdfe9e..6a47e4c 100644 --- a/pkg/schemadiff/schemadiff.go +++ b/pkg/schemadiff/schemadiff.go @@ -38,6 +38,12 @@ type Column struct { // Default is the canonical default expression (pg_get_expr), empty when // none. For a generated column it is the generation expression. Default string + // SequenceDefault reports that the default expression depends on a + // sequence (per pg_depend) — a serial column or a hand-written nextval + // default. In a desired-state model that sequence exists only inside + // the rolled-back scratch transaction, so no derived plan can + // reference it. + SequenceDefault bool // Identity is the identity kind, IdentityNone for plain columns. Identity Identity // Generated reports GENERATED ALWAYS AS (...) STORED. diff --git a/pkg/schemadiff/schemadiff_integration_test.go b/pkg/schemadiff/schemadiff_integration_test.go index bb33095..0746aff 100644 --- a/pkg/schemadiff/schemadiff_integration_test.go +++ b/pkg/schemadiff/schemadiff_integration_test.go @@ -52,6 +52,55 @@ func TestIntrospectDesiredMatchesLiveIntrospection(t *testing.T) { "live introspection and scratch execute-and-introspect must agree on the canonical model") } +// Converging a plain integer column onto serial would emit a SET DEFAULT +// referencing a sequence that only ever existed inside the rolled-back +// scratch transaction — a plan that cannot execute. The diff refuses it as +// an unsupported change instead. +func TestDiffRefusesSerialAdoption(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.t (id int PRIMARY KEY, v text)", schema)) + require.NoError(t, err) + + ds, err := statement.ParseDesired("CREATE TABLE t (id serial PRIMARY KEY, v text)") + require.NoError(t, err) + live, err := schemadiff.Introspect(t.Context(), pool, schema, "t") + 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) +} + +// A serial table that already matches its desired file must converge to no +// changes: both sides decompile the sequence default identically under +// their introspection search_path. +func TestDiffSerialTableConverges(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.t (id serial PRIMARY KEY, v text)", schema)) + require.NoError(t, err) + + ds, err := statement.ParseDesired("CREATE TABLE t (id serial PRIMARY KEY, v text)") + require.NoError(t, err) + live, err := schemadiff.Introspect(t.Context(), pool, schema, "t") + require.NoError(t, err) + assert.True(t, live.Columns[0].SequenceDefault, "serial column default must be marked sequence-backed") + desired, err := schemadiff.IntrospectDesired(t.Context(), pool, ds) + require.NoError(t, err) + + changes, err := schemadiff.Diff(schema, live, desired) + require.NoError(t, err) + assert.Empty(t, changes) +} + // Cosmetically different spellings of the same schema must introspect to the // same canonical model: the server's decompilers are the canonicalizer. func TestIntrospectCanonicalizesTypeAliases(t *testing.T) { diff --git a/pkg/statement/comments.go b/pkg/statement/comments.go new file mode 100644 index 0000000..f2620f4 --- /dev/null +++ b/pkg/statement/comments.go @@ -0,0 +1,31 @@ +package statement + +import ( + "errors" + "fmt" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// ErrCommentLoss is returned when an operation that reprints SQL through the +// deparser would silently discard comments. The parser drops comments at +// parse time, so a formatter cannot carry them; refusing is the fail-closed +// alternative to destroying documentation in a source-of-truth file. +var ErrCommentLoss = errors.New("input contains comments, which formatting would discard") + +// CheckNoComments scans sql with the PostgreSQL lexer and returns +// ErrCommentLoss when it contains any SQL (--) or C-style (/* */) comment. +// A scan failure is surfaced to the caller, never guessed around. +func CheckNoComments(sql string) error { + scan, err := pgquery.Scan(sql) + if err != nil { + return fmt.Errorf("scan statement: %w", err) + } + for _, tok := range scan.GetTokens() { + if tok.GetToken() == pganalyze.Token_SQL_COMMENT || tok.GetToken() == pganalyze.Token_C_COMMENT { + return ErrCommentLoss + } + } + return nil +} diff --git a/pkg/statement/comments_test.go b/pkg/statement/comments_test.go new file mode 100644 index 0000000..03009d4 --- /dev/null +++ b/pkg/statement/comments_test.go @@ -0,0 +1,31 @@ +package statement + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCheckNoComments(t *testing.T) { + tests := []struct { + name string + sql string + wantErr error + }{ + {"no comments", "CREATE TABLE t (id int)", nil}, + {"line comment", "-- events table\nCREATE TABLE t (id int)", ErrCommentLoss}, + {"inline line comment", "CREATE TABLE t (\n id int -- surrogate key\n)", ErrCommentLoss}, + {"block comment", "/* header */ CREATE TABLE t (id int)", ErrCommentLoss}, + {"comment inside a string literal is not a comment", "CREATE TABLE t (id int DEFAULT length('--'))", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := CheckNoComments(tt.sql) + if tt.wantErr == nil { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, tt.wantErr) + }) + } +} diff --git a/pkg/statement/desired.go b/pkg/statement/desired.go index 3753994..2ba8fbf 100644 --- a/pkg/statement/desired.go +++ b/pkg/statement/desired.go @@ -29,6 +29,14 @@ var ( // ErrConcurrentIndex is returned for CREATE INDEX CONCURRENTLY, which // cannot run inside the scratch transaction. ErrConcurrentIndex = errors.New("CONCURRENTLY cannot be used in a desired schema") + // ErrForeignKey is returned when the CREATE TABLE carries a REFERENCES + // clause. The scratch transaction cannot faithfully execute a foreign + // key: an unqualified reference resolves against the scratch + // search_path, not the target schema, so it either fails or silently + // binds to the wrong table. Foreign-key support needs its own design + // (cross-file ordering, lock behavior, qualification policy); until + // then the admission gate refuses it. + ErrForeignKey = errors.New("foreign keys are not supported in a desired schema") // ErrWrongIndexTarget is returned when an index targets a table other // than the desired CREATE TABLE. ErrWrongIndexTarget = errors.New("index must target the desired table") @@ -89,13 +97,17 @@ func ParseDesired(sql string) (DesiredSchema, error) { func admitDesiredStatement(node *pganalyze.Node, seenTable string) (Statement, error) { switch { case node.GetCreateStmt() != nil: - rel := node.GetCreateStmt().GetRelation() + create := node.GetCreateStmt() + rel := create.GetRelation() if rel.GetSchemaname() != "" { return Statement{}, fmt.Errorf("%w: %s.%s", ErrQualifiedName, rel.GetSchemaname(), rel.GetRelname()) } if seenTable != "" { return Statement{}, ErrMultipleCreateTables } + if err := refuseForeignKeys(create); err != nil { + return Statement{}, err + } return Statement{kind: KindCreateTable, table: rel.GetRelname()}, nil case node.GetIndexStmt() != nil: idx := node.GetIndexStmt() @@ -112,6 +124,27 @@ func admitDesiredStatement(node *pganalyze.Node, seenTable string) (Statement, e } } +// refuseForeignKeys returns ErrForeignKey when the CREATE TABLE carries a +// REFERENCES clause, in either its column-constraint or table-constraint +// form. This inspects constraint kinds only — no semantics are derived. +func refuseForeignKeys(create *pganalyze.CreateStmt) error { + for _, elt := range create.GetTableElts() { + if con := elt.GetConstraint(); con != nil && con.GetContype() == pganalyze.ConstrType_CONSTR_FOREIGN { + return fmt.Errorf("%w: table constraint on %q", ErrForeignKey, create.GetRelation().GetRelname()) + } + col := elt.GetColumnDef() + if col == nil { + continue + } + for _, c := range col.GetConstraints() { + if con := c.GetConstraint(); con != nil && con.GetContype() == pganalyze.ConstrType_CONSTR_FOREIGN { + return fmt.Errorf("%w: column %q", ErrForeignKey, col.GetColname()) + } + } + } + return nil +} + // Qualify returns sql with its target relation qualified by schema; an // empty schema strips an existing qualification instead. It supports exactly // one CREATE TABLE or CREATE INDEX statement. This touches qualification diff --git a/pkg/statement/desired_test.go b/pkg/statement/desired_test.go index 0a33520..d0f0c94 100644 --- a/pkg/statement/desired_test.go +++ b/pkg/statement/desired_test.go @@ -40,6 +40,9 @@ func TestParseDesiredRefusals(t *testing.T) { {"qualified index", "CREATE TABLE t (id int); CREATE INDEX i ON prod.t (id)", ErrQualifiedName}, {"concurrent index", "CREATE TABLE t (id int); CREATE INDEX CONCURRENTLY i ON t (id)", ErrConcurrentIndex}, {"index on another table", "CREATE TABLE t (id int); CREATE INDEX i ON other (id)", ErrWrongIndexTarget}, + {"column foreign key", "CREATE TABLE child (id int PRIMARY KEY, pid int REFERENCES parent(id))", ErrForeignKey}, + {"table foreign key", "CREATE TABLE child (id int PRIMARY KEY, pid int, FOREIGN KEY (pid) REFERENCES parent(id))", ErrForeignKey}, + {"self-referencing foreign key", "CREATE TABLE node (id int PRIMARY KEY, parent_id int REFERENCES node(id))", ErrForeignKey}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/statement/statement.go b/pkg/statement/statement.go index a454e9d..14d69df 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -3,6 +3,13 @@ // front door needs. In Phase 1 that is a statement-type gate only: which kind // of statement this is and, for ALTER TABLE, which table it targets. No // schema model, no classification. +// +// Two canonical forms coexist deliberately: this package's deparser prints +// grammar-canonical SQL (e.g. varchar(50)) for formatting, while +// pkg/schemadiff models carry server-decompiled text (character +// varying(50)) for comparison. The two canons never mix: models only ever +// compare server output against server output, and deparser output must not +// feed a model comparison or a schema fingerprint. package statement import (