From e06ed27d4bda71e02de3a2d83b57b93973e84049 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 16:48:12 +1000 Subject: [PATCH 1/4] Phase 2.3a: typed operation descriptors and advisory rewrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the parse boundary with ParseOps — one typed shape descriptor per operation (default constancy, generated/identity, NOT VALID, USING INDEX, CONCURRENTLY, renames) — plus two syntactic advisory rewriters: Concurrently and AddNotValid. These are the classifier's inputs; no semantics are derived from the AST. --- pkg/statement/ops.go | 406 ++++++++++++++++++++++++++++++++++++++ pkg/statement/ops_test.go | 294 +++++++++++++++++++++++++++ pkg/statement/rewrite.go | 109 ++++++++++ 3 files changed, 809 insertions(+) create mode 100644 pkg/statement/ops.go create mode 100644 pkg/statement/ops_test.go create mode 100644 pkg/statement/rewrite.go diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go new file mode 100644 index 0000000..682451b --- /dev/null +++ b/pkg/statement/ops.go @@ -0,0 +1,406 @@ +package statement + +import ( + "fmt" + "strings" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// OpKind names one operation shape the classifier distinguishes. A single +// ALTER TABLE statement yields one Op per subcommand; index, rename, and +// schema statements yield exactly one. +type OpKind int + +// The operation shapes ParseOps reports. OpUnrecognized is everything the engine +// does not recognize; the classifier refuses it. +const ( + OpUnrecognized OpKind = iota + OpCreateTable + OpAddColumn + OpDropColumn + OpAlterColumnType + OpSetDefault + OpDropDefault + OpSetNotNull + OpDropNotNull + OpSetColumnOptions + OpRenameColumn + OpRenameTable + OpRenameIndex + OpSetSchema + OpSetTablespace + OpSetRelOptions + OpAddConstraint + OpValidateConstraint + OpDropConstraint + OpAttachPartition + OpDetachPartition + OpCreateIndex + OpDropIndex + OpReindex +) + +// DefaultKind classifies the DEFAULT expression shape of an added column. +// Only a provable constant qualifies for PostgreSQL's fast default; any +// other expression is treated as volatile, conservatively. +type DefaultKind int + +// The default shapes an added column can carry. +const ( + // DefaultNone: no DEFAULT clause. + DefaultNone DefaultKind = iota + // DefaultConstant: a literal (possibly type-cast) — fast-default safe. + DefaultConstant + // DefaultExpression: anything else — function calls, identity, serial. + // The engine does not evaluate volatility offline; it assumes the worst. + DefaultExpression +) + +// ConstraintKind names the constraint families the classifier routes +// differently. +type ConstraintKind int + +// The constraint families ParseOps distinguishes. ConstraintUnrecognized +// (e.g. EXCLUDE) has no known safe pattern and is refused. +const ( + ConstraintUnrecognized ConstraintKind = iota + ConstraintPrimaryKey + ConstraintUnique + ConstraintCheck + ConstraintForeignKey + ConstraintNotNull +) + +// Op is one parsed operation: the shape facts the classifier needs, nothing +// executable. Fields beyond Kind are populated only where meaningful for +// that kind; see each field's comment. +type Op struct { + // Kind is the operation shape. + Kind OpKind + // Column is the target column for column operations. + Column string + // Name is the constraint or index name where the operation has one, + // or the new name for renames. + Name string + // Columns are the plain key columns of an ADD PRIMARY KEY / UNIQUE; + // empty when the keys are expressions. + Columns []string + // Constraint is the constraint family for OpAddConstraint. + Constraint ConstraintKind + // NotValid is true for ADD CONSTRAINT ... NOT VALID. + NotValid bool + // UsingIndex is true for ADD CONSTRAINT ... USING INDEX. + UsingIndex bool + // Concurrent is true when the statement carries CONCURRENTLY. + Concurrent bool + // Unique is true for CREATE UNIQUE INDEX. + Unique bool + // GeneratedStored is true for ADD COLUMN ... GENERATED ... STORED. + GeneratedStored bool + // Default is the DEFAULT shape for OpAddColumn. + Default DefaultKind + // NewType is the target type for OpAlterColumnType and the column type + // for OpAddColumn, as the bare grammar type name (e.g. "varchar", + // "numeric") without the pg_catalog qualification. + NewType string + // NewTypeMods are the target type's modifiers (e.g. 50 in varchar(50), + // 12 and 2 in numeric(12,2)); empty when unconstrained. + NewTypeMods []int32 + // HasUsing is true for ALTER COLUMN TYPE ... USING , which always + // means a conversion, never a binary-coercible relabel. + HasUsing bool +} + +// Describe returns a short operator-facing label for the operation, e.g. +// "ADD COLUMN age" — for plan rendering, never for branching. +func (o Op) Describe() string { + switch o.Kind { + case OpCreateTable: + return "CREATE TABLE" + case OpAddColumn: + return "ADD COLUMN " + o.Column + case OpDropColumn: + return "DROP COLUMN " + o.Column + case OpAlterColumnType: + return "ALTER COLUMN " + o.Column + " TYPE " + o.NewType + case OpSetDefault: + return "ALTER COLUMN " + o.Column + " SET DEFAULT" + case OpDropDefault: + return "ALTER COLUMN " + o.Column + " DROP DEFAULT" + case OpSetNotNull: + return "ALTER COLUMN " + o.Column + " SET NOT NULL" + case OpDropNotNull: + return "ALTER COLUMN " + o.Column + " DROP NOT NULL" + case OpSetColumnOptions: + return "ALTER COLUMN " + o.Column + " SET options" + case OpRenameColumn: + return "RENAME COLUMN " + o.Column + " TO " + o.Name + case OpRenameTable: + return "RENAME TO " + o.Name + case OpRenameIndex: + return "RENAME INDEX TO " + o.Name + case OpSetSchema: + return "SET SCHEMA " + o.Name + case OpSetTablespace: + return "SET TABLESPACE " + o.Name + case OpSetRelOptions: + return "SET storage parameters" + case OpAddConstraint: + return "ADD CONSTRAINT " + o.Name + case OpValidateConstraint: + return "VALIDATE CONSTRAINT " + o.Name + case OpDropConstraint: + return "DROP CONSTRAINT " + o.Name + case OpAttachPartition: + return "ATTACH PARTITION" + case OpDetachPartition: + return "DETACH PARTITION" + case OpCreateIndex: + return "CREATE INDEX " + o.Name + case OpDropIndex: + return "DROP INDEX " + o.Name + case OpReindex: + return "REINDEX " + o.Name + default: + return "unrecognized operation" + } +} + +// ParseOps parses one SQL statement and returns its typed operations. An +// ALTER TABLE yields one Op per subcommand; every other supported statement +// yields exactly one. Statements and subcommands the engine does not +// recognize come back as OpUnrecognized — never an error — so the classifier can +// refuse them with context. A parse failure is surfaced to the caller. +func ParseOps(sql string) ([]Op, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return nil, fmt.Errorf("parse statement: %w", err) + } + if n := len(tree.GetStmts()); n != 1 { + return nil, fmt.Errorf("%w: got %d", ErrNotOneStatement, n) + } + node := tree.GetStmts()[0].GetStmt() + switch { + case node.GetAlterTableStmt() != nil: + alter := node.GetAlterTableStmt() + if alter.GetObjtype() != pganalyze.ObjectType_OBJECT_TABLE { + return []Op{{Kind: OpUnrecognized}}, nil + } + ops := make([]Op, 0, len(alter.GetCmds())) + for _, cmd := range alter.GetCmds() { + ops = append(ops, alterTableOp(cmd.GetAlterTableCmd())) + } + return ops, nil + case node.GetCreateStmt() != nil: + return []Op{{Kind: OpCreateTable}}, nil + case node.GetIndexStmt() != nil: + idx := node.GetIndexStmt() + return []Op{{ + Kind: OpCreateIndex, + Name: idx.GetIdxname(), + Concurrent: idx.GetConcurrent(), + Unique: idx.GetUnique(), + }}, nil + case node.GetDropStmt() != nil: + drop := node.GetDropStmt() + if drop.GetRemoveType() != pganalyze.ObjectType_OBJECT_INDEX { + return []Op{{Kind: OpUnrecognized}}, nil + } + return []Op{{Kind: OpDropIndex, Concurrent: drop.GetConcurrent()}}, nil + case node.GetReindexStmt() != nil: + re := node.GetReindexStmt() + return []Op{{ + Kind: OpReindex, + Name: re.GetRelation().GetRelname(), + Concurrent: reindexConcurrent(re), + }}, nil + case node.GetRenameStmt() != nil: + return []Op{renameOp(node.GetRenameStmt())}, nil + case node.GetAlterObjectSchemaStmt() != nil: + alter := node.GetAlterObjectSchemaStmt() + if alter.GetObjectType() != pganalyze.ObjectType_OBJECT_TABLE { + return []Op{{Kind: OpUnrecognized}}, nil + } + return []Op{{Kind: OpSetSchema, Name: alter.GetNewschema()}}, nil + default: + return []Op{{Kind: OpUnrecognized}}, nil + } +} + +// alterTableOp maps one ALTER TABLE subcommand to its Op. +func alterTableOp(cmd *pganalyze.AlterTableCmd) Op { + switch cmd.GetSubtype() { + case pganalyze.AlterTableType_AT_AddColumn: + return addColumnOp(cmd.GetDef().GetColumnDef()) + case pganalyze.AlterTableType_AT_DropColumn: + return Op{Kind: OpDropColumn, Column: cmd.GetName()} + case pganalyze.AlterTableType_AT_AlterColumnType: + def := cmd.GetDef().GetColumnDef() + name, mods := typeRef(def.GetTypeName()) + return Op{ + Kind: OpAlterColumnType, + Column: cmd.GetName(), + NewType: name, + NewTypeMods: mods, + // For ALTER COLUMN TYPE the grammar carries the USING + // expression in the column definition's raw default slot. + HasUsing: def.GetRawDefault() != nil, + } + case pganalyze.AlterTableType_AT_ColumnDefault: + if cmd.GetDef() == nil { + return Op{Kind: OpDropDefault, Column: cmd.GetName()} + } + return Op{Kind: OpSetDefault, Column: cmd.GetName()} + case pganalyze.AlterTableType_AT_SetNotNull: + return Op{Kind: OpSetNotNull, Column: cmd.GetName()} + case pganalyze.AlterTableType_AT_DropNotNull: + return Op{Kind: OpDropNotNull, Column: cmd.GetName()} + case pganalyze.AlterTableType_AT_SetStatistics, + pganalyze.AlterTableType_AT_SetStorage, + pganalyze.AlterTableType_AT_SetOptions, + pganalyze.AlterTableType_AT_ResetOptions: + return Op{Kind: OpSetColumnOptions, Column: cmd.GetName()} + case pganalyze.AlterTableType_AT_SetRelOptions, + pganalyze.AlterTableType_AT_ResetRelOptions: + return Op{Kind: OpSetRelOptions} + case pganalyze.AlterTableType_AT_SetTableSpace: + return Op{Kind: OpSetTablespace, Name: cmd.GetName()} + case pganalyze.AlterTableType_AT_AddConstraint: + return addConstraintOp(cmd.GetDef().GetConstraint()) + case pganalyze.AlterTableType_AT_ValidateConstraint: + return Op{Kind: OpValidateConstraint, Name: cmd.GetName()} + case pganalyze.AlterTableType_AT_DropConstraint: + return Op{Kind: OpDropConstraint, Name: cmd.GetName()} + case pganalyze.AlterTableType_AT_AttachPartition: + return Op{Kind: OpAttachPartition} + case pganalyze.AlterTableType_AT_DetachPartition: + return Op{Kind: OpDetachPartition, Concurrent: cmd.GetDef().GetPartitionCmd().GetConcurrent()} + default: + return Op{Kind: OpUnrecognized} + } +} + +// addColumnOp extracts the shape facts of an added column: its DEFAULT +// shape and whether it is a stored generated column. Identity and serial +// columns are reported as expression defaults — their values come from a +// sequence, which fast default cannot cover. +func addColumnOp(def *pganalyze.ColumnDef) Op { + op := Op{Kind: OpAddColumn, Column: def.GetColname()} + op.NewType, op.NewTypeMods = typeRef(def.GetTypeName()) + if isSerialType(op.NewType) { + op.Default = DefaultExpression + } + for _, c := range def.GetConstraints() { + con := c.GetConstraint() + switch con.GetContype() { + case pganalyze.ConstrType_CONSTR_DEFAULT: + if isConstantExpr(con.GetRawExpr()) { + op.Default = DefaultConstant + } else { + op.Default = DefaultExpression + } + case pganalyze.ConstrType_CONSTR_IDENTITY: + op.Default = DefaultExpression + case pganalyze.ConstrType_CONSTR_GENERATED: + op.GeneratedStored = true + } + } + return op +} + +// addConstraintOp extracts the shape facts of an ADD CONSTRAINT. +func addConstraintOp(con *pganalyze.Constraint) Op { + op := Op{ + Kind: OpAddConstraint, + Name: con.GetConname(), + NotValid: con.GetSkipValidation(), + UsingIndex: con.GetIndexname() != "", + } + switch con.GetContype() { + case pganalyze.ConstrType_CONSTR_PRIMARY: + op.Constraint = ConstraintPrimaryKey + case pganalyze.ConstrType_CONSTR_UNIQUE: + op.Constraint = ConstraintUnique + case pganalyze.ConstrType_CONSTR_CHECK: + op.Constraint = ConstraintCheck + case pganalyze.ConstrType_CONSTR_FOREIGN: + op.Constraint = ConstraintForeignKey + case pganalyze.ConstrType_CONSTR_NOTNULL: + op.Constraint = ConstraintNotNull + default: + op.Constraint = ConstraintUnrecognized + } + for _, k := range con.GetKeys() { + op.Columns = append(op.Columns, k.GetString_().GetSval()) + } + return op +} + +// renameOp maps a RENAME statement (column, table, or index — the grammar +// parses all three as RenameStmt) to its Op. +func renameOp(ren *pganalyze.RenameStmt) Op { + switch ren.GetRenameType() { + case pganalyze.ObjectType_OBJECT_COLUMN: + return Op{Kind: OpRenameColumn, Column: ren.GetSubname(), Name: ren.GetNewname()} + case pganalyze.ObjectType_OBJECT_TABLE: + return Op{Kind: OpRenameTable, Name: ren.GetNewname()} + case pganalyze.ObjectType_OBJECT_INDEX: + return Op{Kind: OpRenameIndex, Name: ren.GetNewname()} + default: + return Op{Kind: OpUnrecognized} + } +} + +// reindexConcurrent reports whether a REINDEX statement carries the +// CONCURRENTLY option (a DefElem in the statement's parameter list). +func reindexConcurrent(re *pganalyze.ReindexStmt) bool { + for _, p := range re.GetParams() { + if p.GetDefElem().GetDefname() == "concurrently" { + return true + } + } + return false +} + +// typeRef returns the bare grammar type name (last path element, without +// the pg_catalog qualification) and its integer modifiers. +func typeRef(tn *pganalyze.TypeName) (string, []int32) { + names := tn.GetNames() + if len(names) == 0 { + return "", nil + } + name := names[len(names)-1].GetString_().GetSval() + var mods []int32 + for _, m := range tn.GetTypmods() { + mods = append(mods, int32(m.GetAConst().GetIval().GetIval())) + } + return name, mods +} + +// isSerialType reports whether the grammar type name is one of the serial +// pseudo-types, which expand to a sequence-backed default. +func isSerialType(name string) bool { + switch strings.ToLower(name) { + case "serial", "serial2", "serial4", "serial8", "smallserial", "bigserial": + return true + default: + return false + } +} + +// isConstantExpr reports whether a DEFAULT expression is a provable +// constant: a literal, possibly wrapped in type casts. Anything else — +// function calls, value functions like CURRENT_TIMESTAMP, expressions — +// is not, and the caller treats it as volatile. +func isConstantExpr(node *pganalyze.Node) bool { + switch { + case node.GetAConst() != nil: + return true + case node.GetTypeCast() != nil: + return isConstantExpr(node.GetTypeCast().GetArg()) + default: + return false + } +} diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go new file mode 100644 index 0000000..93b4ebd --- /dev/null +++ b/pkg/statement/ops_test.go @@ -0,0 +1,294 @@ +package statement_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" +) + +func parseOneOp(t *testing.T, sql string) statement.Op { + t.Helper() + ops, err := statement.ParseOps(sql) + require.NoError(t, err) + require.Len(t, ops, 1) + return ops[0] +} + +func TestParseOpsShapes(t *testing.T) { + cases := []struct { + name string + sql string + want statement.Op + }{ + { + name: "add column plain", + sql: "ALTER TABLE t ADD COLUMN age int", + want: statement.Op{Kind: statement.OpAddColumn, Column: "age", NewType: "int4"}, + }, + { + name: "add column constant default", + sql: "ALTER TABLE t ADD COLUMN age int DEFAULT 0", + want: statement.Op{Kind: statement.OpAddColumn, Column: "age", NewType: "int4", Default: statement.DefaultConstant}, + }, + { + name: "add column cast constant default", + sql: "ALTER TABLE t ADD COLUMN created timestamptz DEFAULT '2020-01-01'::timestamptz", + want: statement.Op{Kind: statement.OpAddColumn, Column: "created", NewType: "timestamptz", Default: statement.DefaultConstant}, + }, + { + name: "add column function default", + sql: "ALTER TABLE t ADD COLUMN id uuid DEFAULT uuid_generate_v4()", + want: statement.Op{Kind: statement.OpAddColumn, Column: "id", NewType: "uuid", Default: statement.DefaultExpression}, + }, + { + name: "add column value function default", + sql: "ALTER TABLE t ADD COLUMN created timestamptz DEFAULT CURRENT_TIMESTAMP", + want: statement.Op{Kind: statement.OpAddColumn, Column: "created", NewType: "timestamptz", Default: statement.DefaultExpression}, + }, + { + name: "add column serial", + sql: "ALTER TABLE t ADD COLUMN n serial", + want: statement.Op{Kind: statement.OpAddColumn, Column: "n", NewType: "serial", Default: statement.DefaultExpression}, + }, + { + name: "add column identity", + sql: "ALTER TABLE t ADD COLUMN n bigint GENERATED ALWAYS AS IDENTITY", + want: statement.Op{Kind: statement.OpAddColumn, Column: "n", NewType: "int8", Default: statement.DefaultExpression}, + }, + { + name: "add column generated stored", + sql: "ALTER TABLE t ADD COLUMN total numeric GENERATED ALWAYS AS (price * qty) STORED", + want: statement.Op{Kind: statement.OpAddColumn, Column: "total", NewType: "numeric", GeneratedStored: true}, + }, + { + name: "drop column", + sql: "ALTER TABLE t DROP COLUMN age", + want: statement.Op{Kind: statement.OpDropColumn, Column: "age"}, + }, + { + name: "alter type with mods", + sql: "ALTER TABLE t ALTER COLUMN name TYPE varchar(100)", + want: statement.Op{Kind: statement.OpAlterColumnType, Column: "name", NewType: "varchar", NewTypeMods: []int32{100}}, + }, + { + name: "alter type with using", + sql: "ALTER TABLE t ALTER COLUMN doc TYPE jsonb USING doc::jsonb", + want: statement.Op{Kind: statement.OpAlterColumnType, Column: "doc", NewType: "jsonb", HasUsing: true}, + }, + { + name: "set default", + sql: "ALTER TABLE t ALTER COLUMN age SET DEFAULT 1", + want: statement.Op{Kind: statement.OpSetDefault, Column: "age"}, + }, + { + name: "drop default", + sql: "ALTER TABLE t ALTER COLUMN age DROP DEFAULT", + want: statement.Op{Kind: statement.OpDropDefault, Column: "age"}, + }, + { + name: "set not null", + sql: "ALTER TABLE t ALTER COLUMN age SET NOT NULL", + want: statement.Op{Kind: statement.OpSetNotNull, Column: "age"}, + }, + { + name: "drop not null", + sql: "ALTER TABLE t ALTER COLUMN age DROP NOT NULL", + want: statement.Op{Kind: statement.OpDropNotNull, Column: "age"}, + }, + { + name: "set statistics", + sql: "ALTER TABLE t ALTER COLUMN age SET STATISTICS 500", + want: statement.Op{Kind: statement.OpSetColumnOptions, Column: "age"}, + }, + { + name: "rename column", + sql: "ALTER TABLE t RENAME COLUMN a TO b", + want: statement.Op{Kind: statement.OpRenameColumn, Column: "a", Name: "b"}, + }, + { + name: "rename table", + sql: "ALTER TABLE t RENAME TO t2", + want: statement.Op{Kind: statement.OpRenameTable, Name: "t2"}, + }, + { + name: "rename index", + sql: "ALTER INDEX i RENAME TO i2", + want: statement.Op{Kind: statement.OpRenameIndex, Name: "i2"}, + }, + { + name: "set schema", + sql: "ALTER TABLE t SET SCHEMA s2", + want: statement.Op{Kind: statement.OpSetSchema, Name: "s2"}, + }, + { + name: "set tablespace", + sql: "ALTER TABLE t SET TABLESPACE fast", + want: statement.Op{Kind: statement.OpSetTablespace, Name: "fast"}, + }, + { + name: "set rel options", + sql: "ALTER TABLE t SET (fillfactor = 70)", + want: statement.Op{Kind: statement.OpSetRelOptions}, + }, + { + name: "add primary key", + sql: "ALTER TABLE t ADD CONSTRAINT t_pkey PRIMARY KEY (id)", + want: statement.Op{Kind: statement.OpAddConstraint, Name: "t_pkey", Constraint: statement.ConstraintPrimaryKey, Columns: []string{"id"}}, + }, + { + name: "add unique using index", + sql: "ALTER TABLE t ADD CONSTRAINT u UNIQUE USING INDEX u_idx", + want: statement.Op{Kind: statement.OpAddConstraint, Name: "u", Constraint: statement.ConstraintUnique, UsingIndex: true}, + }, + { + name: "add check not valid", + sql: "ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0) NOT VALID", + want: statement.Op{Kind: statement.OpAddConstraint, Name: "c", Constraint: statement.ConstraintCheck, NotValid: true}, + }, + { + name: "add foreign key", + sql: "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (pid) REFERENCES p (id)", + want: statement.Op{Kind: statement.OpAddConstraint, Name: "fk", Constraint: statement.ConstraintForeignKey}, + }, + { + name: "validate constraint", + sql: "ALTER TABLE t VALIDATE CONSTRAINT c", + want: statement.Op{Kind: statement.OpValidateConstraint, Name: "c"}, + }, + { + name: "drop constraint", + sql: "ALTER TABLE t DROP CONSTRAINT c", + want: statement.Op{Kind: statement.OpDropConstraint, Name: "c"}, + }, + { + name: "attach partition", + sql: "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)", + want: statement.Op{Kind: statement.OpAttachPartition}, + }, + { + name: "detach partition", + sql: "ALTER TABLE t DETACH PARTITION p", + want: statement.Op{Kind: statement.OpDetachPartition}, + }, + { + name: "detach partition concurrently", + sql: "ALTER TABLE t DETACH PARTITION p CONCURRENTLY", + want: statement.Op{Kind: statement.OpDetachPartition, Concurrent: true}, + }, + { + name: "create index", + sql: "CREATE INDEX i ON t (a)", + want: statement.Op{Kind: statement.OpCreateIndex, Name: "i"}, + }, + { + name: "create unique index concurrently", + sql: "CREATE UNIQUE INDEX CONCURRENTLY i ON t (a)", + want: statement.Op{Kind: statement.OpCreateIndex, Name: "i", Concurrent: true, Unique: true}, + }, + { + name: "drop index", + sql: "DROP INDEX i", + want: statement.Op{Kind: statement.OpDropIndex}, + }, + { + name: "drop index concurrently", + sql: "DROP INDEX CONCURRENTLY i", + want: statement.Op{Kind: statement.OpDropIndex, Concurrent: true}, + }, + { + name: "reindex", + sql: "REINDEX INDEX i", + want: statement.Op{Kind: statement.OpReindex, Name: "i"}, + }, + { + name: "reindex concurrently", + sql: "REINDEX INDEX CONCURRENTLY i", + want: statement.Op{Kind: statement.OpReindex, Name: "i", Concurrent: true}, + }, + { + name: "create table", + sql: "CREATE TABLE t (id int PRIMARY KEY)", + want: statement.Op{Kind: statement.OpCreateTable}, + }, + { + name: "unrecognized statement", + sql: "VACUUM FULL t", + want: statement.Op{Kind: statement.OpUnrecognized}, + }, + { + name: "unrecognized subcommand", + sql: "ALTER TABLE t ENABLE ROW LEVEL SECURITY", + want: statement.Op{Kind: statement.OpUnrecognized}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, parseOneOp(t, tc.sql)) + }) + } +} + +func TestParseOpsMultipleSubcommands(t *testing.T) { + ops, err := statement.ParseOps("ALTER TABLE t ADD COLUMN a int, DROP COLUMN b") + require.NoError(t, err) + require.Len(t, ops, 2) + assert.Equal(t, statement.OpAddColumn, ops[0].Kind) + assert.Equal(t, statement.OpDropColumn, ops[1].Kind) +} + +func TestParseOpsRejectsMultipleStatements(t *testing.T) { + _, err := statement.ParseOps("SELECT 1; SELECT 2") + assert.ErrorIs(t, err, statement.ErrNotOneStatement) +} + +func TestConcurrentlyRewrites(t *testing.T) { + // The rewrite contract is syntactic: the result must parse back with + // the concurrency flag set. Exact deparser wording is not a contract. + for _, sql := range []string{ + "CREATE INDEX i ON t (a)", + "DROP INDEX i", + "REINDEX INDEX i", + "ALTER TABLE t DETACH PARTITION p", + } { + t.Run(sql, func(t *testing.T) { + safer, err := statement.Concurrently(sql) + require.NoError(t, err) + op := parseOneOp(t, safer) + assert.True(t, op.Concurrent, "rewritten statement must be concurrent: %s", safer) + }) + } +} + +func TestConcurrentlyRefusesOtherStatements(t *testing.T) { + _, err := statement.Concurrently("ALTER TABLE t ADD COLUMN a int") + assert.ErrorIs(t, err, statement.ErrNotRewritable) +} + +func TestAddNotValidRewritesNamedCheck(t *testing.T) { + safer, name, err := statement.AddNotValid("ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0)") + require.NoError(t, err) + assert.Equal(t, "c", name) + op := parseOneOp(t, safer) + assert.True(t, op.NotValid, "rewritten constraint must be NOT VALID: %s", safer) +} + +func TestAddNotValidRefusals(t *testing.T) { + cases := []struct { + name string + sql string + }{ + {"unnamed constraint", "ALTER TABLE t ADD CHECK (age > 0)"}, + {"primary key", "ALTER TABLE t ADD CONSTRAINT p PRIMARY KEY (id)"}, + {"not an add constraint", "ALTER TABLE t DROP COLUMN a"}, + {"multiple subcommands", "ALTER TABLE t ADD CONSTRAINT c CHECK (a > 0), DROP COLUMN b"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, _, err := statement.AddNotValid(tc.sql) + assert.ErrorIs(t, err, statement.ErrNotValidNotApplicable) + }) + } +} diff --git a/pkg/statement/rewrite.go b/pkg/statement/rewrite.go new file mode 100644 index 0000000..2adaa5b --- /dev/null +++ b/pkg/statement/rewrite.go @@ -0,0 +1,109 @@ +package statement + +import ( + "errors" + "fmt" + + pganalyze "github.com/pganalyze/pg_query_go/v6" + pgquery "github.com/wasilibs/go-pgquery" +) + +// Typed refusals for advisory rewrites. The rewriters flip one syntactic +// flag and deparse — no semantics are derived; a statement that cannot be +// rewritten that way is refused with one of these. +var ( + // ErrNotRewritable is returned when the statement kind has no + // CONCURRENTLY form to rewrite to. + ErrNotRewritable = errors.New("statement has no concurrent form") + // ErrNotValidNotApplicable is returned when the statement is not a + // single named ADD CHECK / ADD FOREIGN KEY that could take NOT VALID. + ErrNotValidNotApplicable = errors.New("statement cannot take NOT VALID") +) + +// Concurrently returns sql rewritten to its CONCURRENTLY form: CREATE +// INDEX, DROP INDEX, REINDEX, or ALTER TABLE ... DETACH PARTITION. The +// rewrite flips the grammar's concurrency flag and deparses — nothing else +// changes. A statement already concurrent comes back canonicalized. +func Concurrently(sql string) (string, error) { + node, err := parseSingle(sql) + if err != nil { + return "", err + } + switch { + case node.GetIndexStmt() != nil: + node.GetIndexStmt().Concurrent = true + case node.GetDropStmt() != nil && node.GetDropStmt().GetRemoveType() == pganalyze.ObjectType_OBJECT_INDEX: + node.GetDropStmt().Concurrent = true + case node.GetReindexStmt() != nil: + re := node.GetReindexStmt() + if !reindexConcurrent(re) { + re.Params = append(re.Params, &pganalyze.Node{ + Node: &pganalyze.Node_DefElem{DefElem: &pganalyze.DefElem{Defname: "concurrently"}}, + }) + } + case detachPartitionCmd(node) != nil: + detachPartitionCmd(node).Concurrent = true + default: + return "", ErrNotRewritable + } + return deparseOne(node) +} + +// AddNotValid rewrites a single-command ALTER TABLE ... ADD CONSTRAINT +// (named CHECK or FOREIGN KEY) to its NOT VALID form and returns the +// rewritten statement plus the constraint name for the follow-up +// VALIDATE CONSTRAINT step. +func AddNotValid(sql string) (rewritten, constraint string, err error) { + node, err := parseSingle(sql) + if err != nil { + return "", "", err + } + alter := node.GetAlterTableStmt() + if alter == nil || alter.GetObjtype() != pganalyze.ObjectType_OBJECT_TABLE || len(alter.GetCmds()) != 1 { + return "", "", ErrNotValidNotApplicable + } + cmd := alter.GetCmds()[0].GetAlterTableCmd() + if cmd.GetSubtype() != pganalyze.AlterTableType_AT_AddConstraint { + return "", "", ErrNotValidNotApplicable + } + con := cmd.GetDef().GetConstraint() + validatable := con.GetContype() == pganalyze.ConstrType_CONSTR_CHECK || + con.GetContype() == pganalyze.ConstrType_CONSTR_FOREIGN + if !validatable || con.GetConname() == "" { + return "", "", ErrNotValidNotApplicable + } + con.SkipValidation = true + con.InitiallyValid = false + if rewritten, err = deparseOne(node); err != nil { + return "", "", err + } + return rewritten, con.GetConname(), nil +} + +// parseSingle parses sql and requires exactly one statement, returning its +// root node for in-place rewriting. +func parseSingle(sql string) (*pganalyze.Node, error) { + tree, err := pgquery.Parse(sql) + if err != nil { + return nil, fmt.Errorf("parse statement: %w", err) + } + if n := len(tree.GetStmts()); n != 1 { + return nil, fmt.Errorf("%w: got %d", ErrNotOneStatement, n) + } + return tree.GetStmts()[0].GetStmt(), nil +} + +// detachPartitionCmd returns the PartitionCmd of a single-command +// ALTER TABLE ... DETACH PARTITION, or nil when the statement is anything +// else. +func detachPartitionCmd(node *pganalyze.Node) *pganalyze.PartitionCmd { + alter := node.GetAlterTableStmt() + if alter == nil || len(alter.GetCmds()) != 1 { + return nil + } + cmd := alter.GetCmds()[0].GetAlterTableCmd() + if cmd.GetSubtype() != pganalyze.AlterTableType_AT_DetachPartition { + return nil + } + return cmd.GetDef().GetPartitionCmd() +} From b08f7b8c61b4145edd4708064a16f00d93d14430 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 16:48:13 +1000 Subject: [PATCH 2/4] Phase 2.3b: classifier routing native / copy-and-swap / refuse pkg/planner maps each operation to a route with a typed reason, golden-tested against every row of the online-DDL reference. Risky literals get the safer native sequence (CONCURRENTLY, NOT VALID + VALIDATE, USING INDEX attach, the four-step SET NOT NULL pattern). Conservative by construction: unproven defaults are volatile, type changes without live column facts are rewrites, unknown operations are refused. --- SAFETY.md | 2 +- pkg/planner/planner.go | 426 ++++++++++++++++++++++++++++++++++++ pkg/planner/planner_test.go | 179 +++++++++++++++ 3 files changed, 606 insertions(+), 1 deletion(-) create mode 100644 pkg/planner/planner.go create mode 100644 pkg/planner/planner_test.go diff --git a/SAFETY.md b/SAFETY.md index 418341c..9fc1b66 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` (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/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/lint` — classify/diff/route | ❌ periphery¹ | `pkg/statement` (parse boundary), `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect), and `pkg/planner` (classifier) exist (Phase 2); `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/pkg/planner/planner.go b/pkg/planner/planner.go new file mode 100644 index 0000000..8a07dec --- /dev/null +++ b/pkg/planner/planner.go @@ -0,0 +1,426 @@ +// Package planner classifies schema-change statements: for each operation +// it decides whether PostgreSQL can run it online natively (possibly via a +// safer idiom it suggests), whether it needs the engine's copy-and-swap +// path, or whether it is refused. The mapping is the "Needs copy-and-swap?" +// column of docs/postgres-online-ddl-reference.md, applied conservatively: +// anything the planner cannot prove safe routes to copy-and-swap or refuse. +// Classification predicts; executors keep their own protections regardless. +package planner + +import ( + "fmt" + "strconv" + "strings" + + "github.com/jackc/pgx/v5" + + "github.com/block/pg-sprite/pkg/statement" +) + +// Route is where an operation is sent. +type Route string + +// The three routes. +const ( + // RouteNative: PostgreSQL runs it online natively — directly or via + // the safer idiom in Decision.SaferSQL. + RouteNative Route = "native" + // RouteCopyAndSwap: needs a table rewrite; only the engine's shadow + // copy + cutover can do it online. + RouteCopyAndSwap Route = "copy-and-swap" + // RouteRefuse: no known safe path; not executed. + RouteRefuse Route = "refuse" +) + +// worse orders routes for aggregation: refuse > copy-and-swap > native. +func worse(a, b Route) Route { + rank := map[Route]int{RouteNative: 0, RouteCopyAndSwap: 1, RouteRefuse: 2} + if rank[b] > rank[a] { + return b + } + return a +} + +// Reason is the typed cause of a routing decision; automation branches on +// it, never on prose. +type Reason string + +// The reasons a decision can carry. +const ( + // ReasonMetadataOnly: a brief ACCESS EXCLUSIVE catalog change, no scan + // and no rewrite. + ReasonMetadataOnly Reason = "metadata-only" + // ReasonOnlineIdiom: already the safe native form (CONCURRENTLY, + // NOT VALID, VALIDATE, USING INDEX). + ReasonOnlineIdiom Reason = "online-idiom" + // ReasonFastDefault: ADD COLUMN with a constant default — the catalog + // stores the default, no rewrite (PG 11+). + ReasonFastDefault Reason = "fast-default" + // ReasonBinaryCoercible: a type change PostgreSQL relabels without a + // rewrite (widen varchar, varchar to text, widen numeric precision). + ReasonBinaryCoercible Reason = "binary-coercible" + // ReasonSaferIdiom: native, but the submitted form blocks; SaferSQL + // carries the online rewrite when one can be constructed. + ReasonSaferIdiom Reason = "safer-idiom" + // ReasonVolatileDefault: ADD COLUMN whose default the planner cannot + // prove constant — PostgreSQL rewrites the table. + ReasonVolatileDefault Reason = "volatile-default" + // ReasonGeneratedStored: adding a stored generated column computes + // every row — a full rewrite. + ReasonGeneratedStored Reason = "generated-stored" + // ReasonTypeRewrite: a type conversion PostgreSQL cannot relabel — + // rewrite plus reindex. + ReasonTypeRewrite Reason = "type-rewrite" + // ReasonRelocation: SET TABLESPACE moves the heap — a rewrite-scale + // copy. + ReasonRelocation Reason = "relocation" + // ReasonUnsupportedOperation: the planner does not recognize the + // operation or knows no safe path for it. + ReasonUnsupportedOperation Reason = "unsupported-operation" +) + +// Decision is the classification of one operation. +type Decision struct { + // Operation is the operator-facing label (display only). + Operation string `json:"operation"` + // Route is where the operation goes. + Route Route `json:"route"` + // Reason is why. + Reason Reason `json:"reason"` + // SaferSQL is the ordered native sequence to run instead of the + // submitted form, present only for safer-idiom decisions where the + // planner could construct it. + SaferSQL []string `json:"safer_sql,omitempty"` +} + +// Plan is the classification of one statement: one decision per operation +// and the aggregate route (the worst of its decisions — one rewrite makes +// the whole statement a copy, one refusal refuses it). +type Plan struct { + // Statement is the submitted SQL. + Statement string `json:"statement"` + // Route is the aggregate route. + Route Route `json:"route"` + // Decisions are the per-operation classifications, in statement order. + Decisions []Decision `json:"decisions"` +} + +// Facts are introspected properties of the live table that sharpen +// classification. The zero value is valid: with no facts the planner is +// strictly more conservative (every type change becomes copy-and-swap). +type Facts struct { + // ColumnTypes maps a column name to its live type as rendered by + // PostgreSQL's format_type (e.g. "character varying(50)"). + ColumnTypes map[string]string +} + +// Classify parses one statement and routes each of its operations. A parse +// failure is an error; an unrecognized operation is not — it comes back as +// a refuse decision so the caller can render the whole plan. +func Classify(sql string, facts Facts) (Plan, error) { + st, err := statement.ParseOne(sql) + if err != nil { + return Plan{}, err + } + ops, err := statement.ParseOps(sql) + if err != nil { + return Plan{}, err + } + plan := Plan{Statement: sql, Route: RouteNative} + // Safer rewrites are only constructed for single-operation statements: + // a partial rewrite of a multi-operation ALTER would be misleading. + single := len(ops) == 1 + for _, op := range ops { + d := classifyOp(op, st, facts, sql, single) + plan.Route = worse(plan.Route, d.Route) + plan.Decisions = append(plan.Decisions, d) + } + return plan, nil +} + +// classifyOp routes one operation per the reference table. +func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string, single bool) Decision { + d := Decision{Operation: op.Describe()} + switch op.Kind { + case statement.OpCreateTable: + // A new table has no readers to lock out. + d.Route, d.Reason = RouteNative, ReasonMetadataOnly + + case statement.OpAddColumn: + switch { + case op.GeneratedStored: + d.Route, d.Reason = RouteCopyAndSwap, ReasonGeneratedStored + case op.Default == statement.DefaultExpression: + d.Route, d.Reason = RouteCopyAndSwap, ReasonVolatileDefault + case op.Default == statement.DefaultConstant: + d.Route, d.Reason = RouteNative, ReasonFastDefault + default: + d.Route, d.Reason = RouteNative, ReasonMetadataOnly + } + + case statement.OpDropColumn, statement.OpSetDefault, statement.OpDropDefault, + statement.OpDropNotNull, statement.OpRenameColumn, statement.OpRenameTable, + statement.OpRenameIndex, statement.OpSetColumnOptions, statement.OpSetRelOptions, + statement.OpSetSchema, statement.OpDropConstraint: + d.Route, d.Reason = RouteNative, ReasonMetadataOnly + + case statement.OpAlterColumnType: + d.Route, d.Reason = classifyTypeChange(op, facts) + + case statement.OpSetNotNull: + // Native pattern: prove the invariant with a NOT VALID CHECK plus + // an online VALIDATE, then SET NOT NULL is a catalog flip (PG 12+). + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + if single { + d.SaferSQL = setNotNullSequence(st, op.Column) + } + + case statement.OpSetTablespace: + d.Route, d.Reason = RouteCopyAndSwap, ReasonRelocation + + case statement.OpAddConstraint: + d = classifyAddConstraint(op, st, sql, single) + + case statement.OpValidateConstraint: + d.Route, d.Reason = RouteNative, ReasonOnlineIdiom + + case statement.OpAttachPartition: + // Native pattern: pre-add a validated CHECK matching the bound on + // the child to skip the attach-time scan. The planner cannot + // construct that CHECK, so it routes native without a rewrite. + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + + case statement.OpDetachPartition: + d = concurrentlyDecision(d, op.Concurrent, sql, single) + + case statement.OpCreateIndex, statement.OpDropIndex, statement.OpReindex: + d = concurrentlyDecision(d, op.Concurrent, sql, single) + + default: + d.Route, d.Reason = RouteRefuse, ReasonUnsupportedOperation + } + return d +} + +// concurrentlyDecision routes an operation that is online in its +// CONCURRENTLY form: already concurrent is the idiom; otherwise native with +// the concurrent rewrite as the safer sequence. +func concurrentlyDecision(d Decision, concurrent bool, sql string, single bool) Decision { + if concurrent { + d.Route, d.Reason = RouteNative, ReasonOnlineIdiom + return d + } + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + if single { + if safer, err := statement.Concurrently(sql); err == nil { + d.SaferSQL = []string{safer} + } + } + return d +} + +// classifyAddConstraint routes ADD CONSTRAINT per constraint family. +func classifyAddConstraint(op statement.Op, st statement.Statement, sql string, single bool) Decision { + d := Decision{Operation: op.Describe()} + switch { + case op.UsingIndex, op.NotValid: + // Already the safe pattern's cheap step. + d.Route, d.Reason = RouteNative, ReasonOnlineIdiom + + case op.Constraint == statement.ConstraintPrimaryKey, + op.Constraint == statement.ConstraintUnique: + // Direct ADD PK/UNIQUE builds its index under ACCESS EXCLUSIVE; + // the safer sequence builds it concurrently and attaches it. + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + if single && len(op.Columns) > 0 { + d.SaferSQL = usingIndexSequence(st, op) + } + + case op.Constraint == statement.ConstraintCheck, + op.Constraint == statement.ConstraintForeignKey: + // Direct ADD CHECK/FK validates under ACCESS EXCLUSIVE; the safer + // sequence is NOT VALID plus an online VALIDATE. + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + if single { + if notValid, name, err := statement.AddNotValid(sql); err == nil { + d.SaferSQL = []string{ + notValid, + "ALTER TABLE " + tableIdent(st) + " VALIDATE CONSTRAINT " + pgx.Identifier{name}.Sanitize(), + } + } + } + + case op.Constraint == statement.ConstraintNotNull: + d.Route, d.Reason = RouteNative, ReasonSaferIdiom + + default: + // EXCLUDE and anything unrecognized: no online pattern exists. + d.Route, d.Reason = RouteRefuse, ReasonUnsupportedOperation + } + return d +} + +// classifyTypeChange routes ALTER COLUMN TYPE: binary-coercible changes are +// a brief catalog relabel; everything else — or anything the planner cannot +// verify against live column facts — is a rewrite. +func classifyTypeChange(op statement.Op, facts Facts) (Route, Reason) { + if op.HasUsing { + return RouteCopyAndSwap, ReasonTypeRewrite + } + oldType, ok := facts.ColumnTypes[op.Column] + if !ok { + return RouteCopyAndSwap, ReasonTypeRewrite + } + if binaryCoercible(parseTypeText(oldType), typeShape{name: normalizeTypeName(op.NewType), mods: op.NewTypeMods}) { + return RouteNative, ReasonBinaryCoercible + } + return RouteCopyAndSwap, ReasonTypeRewrite +} + +// typeShape is a normalized type family plus its modifiers, comparable +// across the grammar's spelling and format_type's rendering. +type typeShape struct { + name string + mods []int32 +} + +// binaryCoercible reports whether changing old to new is a relabel +// PostgreSQL performs without a rewrite or scan. The rules are the +// reference table's rows, deliberately narrow: widening varchar, varchar to +// text, and widening numeric precision at the same scale. Anything not +// provably on this list is not coercible. +func binaryCoercible(old, next typeShape) bool { + if old.name == "" || next.name == "" { + return false + } + if old.name == next.name && int32sEqual(old.mods, next.mods) { + return true // no-op relabel + } + switch old.name { + case "varchar": + if next.name == "text" { + return true + } + if next.name != "varchar" { + return false + } + if len(next.mods) == 0 { + return true // dropping the length bound + } + return len(old.mods) == 1 && len(next.mods) == 1 && next.mods[0] >= old.mods[0] + case "numeric": + if next.name != "numeric" { + return false + } + if len(next.mods) == 0 { + return true // dropping the precision bound + } + return len(old.mods) == 2 && len(next.mods) == 2 && + next.mods[1] == old.mods[1] && next.mods[0] >= old.mods[0] + default: + return false + } +} + +// int32sEqual reports element-wise equality. +func int32sEqual(a, b []int32) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// normalizeTypeName maps spelling variants of the families binaryCoercible +// knows onto one name. Unknown names pass through unchanged (and will never +// match a rule). +func normalizeTypeName(name string) string { + switch strings.ToLower(strings.TrimSpace(name)) { + case "varchar", "character varying": + return "varchar" + case "numeric", "decimal": + return "numeric" + case "text": + return "text" + default: + return strings.ToLower(strings.TrimSpace(name)) + } +} + +// parseTypeText splits a format_type rendering ("character varying(50)", +// "numeric(10,2)", "text") into its normalized shape. This reads catalog +// output, not SQL — format_type's rendering is stable. +func parseTypeText(s string) typeShape { + name, rest, found := strings.Cut(s, "(") + shape := typeShape{name: normalizeTypeName(name)} + if !found { + return shape + } + rest, _, found = strings.Cut(rest, ")") + if !found { + return typeShape{} + } + for part := range strings.SplitSeq(rest, ",") { + n, err := strconv.ParseInt(strings.TrimSpace(part), 10, 32) + if err != nil { + return typeShape{} + } + shape.mods = append(shape.mods, int32(n)) + } + return shape +} + +// tableIdent renders the statement's target table as a quoted identifier, +// schema-qualified when the statement was. +func tableIdent(st statement.Statement) string { + if st.Schema != "" { + return pgx.Identifier{st.Schema, st.Table}.Sanitize() + } + return pgx.Identifier{st.Table}.Sanitize() +} + +// setNotNullSequence is the native four-step SET NOT NULL pattern: prove +// the invariant online with a NOT VALID CHECK, flip the column, drop the +// scaffold. +func setNotNullSequence(st statement.Statement, column string) []string { + table := tableIdent(st) + conName := fmt.Sprintf("%s_%s_not_null", st.Table, column) + con := pgx.Identifier{conName}.Sanitize() + col := pgx.Identifier{column}.Sanitize() + return []string{ + fmt.Sprintf("ALTER TABLE %s ADD CONSTRAINT %s CHECK (%s IS NOT NULL) NOT VALID", table, con, col), + fmt.Sprintf("ALTER TABLE %s VALIDATE CONSTRAINT %s", table, con), + fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL", table, col), + fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s", table, con), + } +} + +// usingIndexSequence is the native two-step ADD PRIMARY KEY / UNIQUE +// pattern: build the unique index concurrently, then attach it as the +// constraint under a brief lock. +func usingIndexSequence(st statement.Statement, op statement.Op) []string { + suffix := "_key" + keyword := "UNIQUE" + if op.Constraint == statement.ConstraintPrimaryKey { + suffix = "_pkey" + keyword = "PRIMARY KEY" + } + name := op.Name + if name == "" { + name = st.Table + "_" + strings.Join(op.Columns, "_") + suffix + } + idx := pgx.Identifier{name}.Sanitize() + cols := make([]string, len(op.Columns)) + for i, c := range op.Columns { + cols[i] = pgx.Identifier{c}.Sanitize() + } + table := tableIdent(st) + return []string{ + fmt.Sprintf("CREATE UNIQUE INDEX CONCURRENTLY %s ON %s (%s)", idx, table, strings.Join(cols, ", ")), + fmt.Sprintf("ALTER TABLE %s ADD CONSTRAINT %s %s USING INDEX %s", table, idx, keyword, idx), + } +} diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go new file mode 100644 index 0000000..28113ec --- /dev/null +++ b/pkg/planner/planner_test.go @@ -0,0 +1,179 @@ +package planner_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/planner" +) + +// facts mirrors a live table whose column types exercise both sides of the +// binary-coercible rules. +var facts = planner.Facts{ColumnTypes: map[string]string{ + "v50": "character varying(50)", + "vany": "character varying", + "num": "numeric(10,2)", + "i": "integer", + "txt": "text", +}} + +func classifyOne(t *testing.T, sql string) planner.Decision { + t.Helper() + plan, err := planner.Classify(sql, facts) + require.NoError(t, err) + require.Len(t, plan.Decisions, 1) + assert.Equal(t, plan.Decisions[0].Route, plan.Route, "single-decision plan route must match") + return plan.Decisions[0] +} + +// TestClassifyReferenceRows is the golden mapping: one case per row of +// docs/postgres-online-ddl-reference.md. saferSteps is the length of the +// expected safer sequence (0 when the decision carries none). +func TestClassifyReferenceRows(t *testing.T) { + cases := []struct { + name string + sql string + route planner.Route + reason planner.Reason + saferSteps int + }{ + // Column operations. + {"add column plain", "ALTER TABLE t ADD COLUMN age int", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"add column constant default", "ALTER TABLE t ADD COLUMN age int DEFAULT 0", planner.RouteNative, planner.ReasonFastDefault, 0}, + {"add column volatile default now", "ALTER TABLE t ADD COLUMN created timestamptz DEFAULT now()", planner.RouteCopyAndSwap, planner.ReasonVolatileDefault, 0}, + {"add column volatile default random", "ALTER TABLE t ADD COLUMN r float8 DEFAULT random()", planner.RouteCopyAndSwap, planner.ReasonVolatileDefault, 0}, + {"add column volatile default uuid", "ALTER TABLE t ADD COLUMN id uuid DEFAULT uuid_generate_v4()", planner.RouteCopyAndSwap, planner.ReasonVolatileDefault, 0}, + {"add column generated stored", "ALTER TABLE t ADD COLUMN total numeric GENERATED ALWAYS AS (price * qty) STORED", planner.RouteCopyAndSwap, planner.ReasonGeneratedStored, 0}, + {"drop column", "ALTER TABLE t DROP COLUMN age", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"alter type widen varchar", "ALTER TABLE t ALTER COLUMN v50 TYPE varchar(100)", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, + {"alter type varchar to text", "ALTER TABLE t ALTER COLUMN v50 TYPE text", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, + {"alter type drop varchar bound", "ALTER TABLE t ALTER COLUMN v50 TYPE varchar", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, + {"alter type widen numeric precision", "ALTER TABLE t ALTER COLUMN num TYPE numeric(12,2)", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, + {"alter type shrink varchar", "ALTER TABLE t ALTER COLUMN v50 TYPE varchar(10)", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"alter type bound an unbounded varchar", "ALTER TABLE t ALTER COLUMN vany TYPE varchar(50)", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"alter type numeric scale change", "ALTER TABLE t ALTER COLUMN num TYPE numeric(12,4)", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"alter type general int to bigint", "ALTER TABLE t ALTER COLUMN i TYPE bigint", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"alter type text to jsonb with using", "ALTER TABLE t ALTER COLUMN txt TYPE jsonb USING txt::jsonb", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"alter type unknown column", "ALTER TABLE t ALTER COLUMN mystery TYPE text", planner.RouteCopyAndSwap, planner.ReasonTypeRewrite, 0}, + {"set default", "ALTER TABLE t ALTER COLUMN age SET DEFAULT 1", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"drop default", "ALTER TABLE t ALTER COLUMN age DROP DEFAULT", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set not null", "ALTER TABLE t ALTER COLUMN age SET NOT NULL", planner.RouteNative, planner.ReasonSaferIdiom, 4}, + {"drop not null", "ALTER TABLE t ALTER COLUMN age DROP NOT NULL", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"rename column", "ALTER TABLE t RENAME COLUMN a TO b", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set statistics", "ALTER TABLE t ALTER COLUMN age SET STATISTICS 500", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set storage", "ALTER TABLE t ALTER COLUMN blob SET STORAGE EXTERNAL", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set column options", "ALTER TABLE t ALTER COLUMN age SET (n_distinct = 100)", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + + // Index operations. + {"create index", "CREATE INDEX i ON t (a)", planner.RouteNative, planner.ReasonSaferIdiom, 1}, + {"create index concurrently", "CREATE INDEX CONCURRENTLY i ON t (a)", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"drop index", "DROP INDEX i", planner.RouteNative, planner.ReasonSaferIdiom, 1}, + {"drop index concurrently", "DROP INDEX CONCURRENTLY i", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"reindex", "REINDEX INDEX i", planner.RouteNative, planner.ReasonSaferIdiom, 1}, + {"reindex concurrently", "REINDEX INDEX CONCURRENTLY i", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"rename index", "ALTER INDEX i RENAME TO i2", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + + // Constraint operations. + {"add primary key direct", "ALTER TABLE t ADD CONSTRAINT t_pkey PRIMARY KEY (id)", planner.RouteNative, planner.ReasonSaferIdiom, 2}, + {"add unique direct", "ALTER TABLE t ADD CONSTRAINT u UNIQUE (a, b)", planner.RouteNative, planner.ReasonSaferIdiom, 2}, + {"add primary key using index", "ALTER TABLE t ADD CONSTRAINT t_pkey PRIMARY KEY USING INDEX t_pkey", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"add check", "ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0)", planner.RouteNative, planner.ReasonSaferIdiom, 2}, + {"add foreign key", "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (pid) REFERENCES p (id)", planner.RouteNative, planner.ReasonSaferIdiom, 2}, + {"add check not valid", "ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0) NOT VALID", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"add foreign key not valid", "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (pid) REFERENCES p (id) NOT VALID", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"add unnamed check", "ALTER TABLE t ADD CHECK (age > 0)", planner.RouteNative, planner.ReasonSaferIdiom, 0}, + {"add exclusion", "ALTER TABLE t ADD CONSTRAINT ex EXCLUDE USING gist (room WITH =)", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0}, + {"validate constraint", "ALTER TABLE t VALIDATE CONSTRAINT c", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"drop constraint", "ALTER TABLE t DROP CONSTRAINT c", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + + // Table and partition operations. + {"rename table", "ALTER TABLE t RENAME TO t2", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set schema", "ALTER TABLE t SET SCHEMA s2", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"set tablespace", "ALTER TABLE t SET TABLESPACE fast", planner.RouteCopyAndSwap, planner.ReasonRelocation, 0}, + {"set fillfactor", "ALTER TABLE t SET (fillfactor = 70)", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"cluster", "CLUSTER t USING i", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0}, + {"vacuum full", "VACUUM FULL t", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0}, + {"attach partition", "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)", planner.RouteNative, planner.ReasonSaferIdiom, 0}, + {"detach partition", "ALTER TABLE t DETACH PARTITION p", planner.RouteNative, planner.ReasonSaferIdiom, 1}, + {"detach partition concurrently", "ALTER TABLE t DETACH PARTITION p CONCURRENTLY", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + + // Non-DDL and unknown statements. + {"dml", "INSERT INTO t VALUES (1)", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0}, + {"create table", "CREATE TABLE t (id int PRIMARY KEY)", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + d := classifyOne(t, tc.sql) + assert.Equal(t, tc.route, d.Route) + assert.Equal(t, tc.reason, d.Reason) + assert.Len(t, d.SaferSQL, tc.saferSteps) + }) + } +} + +func TestClassifySetNotNullSequence(t *testing.T) { + d := classifyOne(t, "ALTER TABLE s.t ALTER COLUMN age SET NOT NULL") + assert.Equal(t, []string{ + `ALTER TABLE "s"."t" ADD CONSTRAINT "t_age_not_null" CHECK ("age" IS NOT NULL) NOT VALID`, + `ALTER TABLE "s"."t" VALIDATE CONSTRAINT "t_age_not_null"`, + `ALTER TABLE "s"."t" ALTER COLUMN "age" SET NOT NULL`, + `ALTER TABLE "s"."t" DROP CONSTRAINT "t_age_not_null"`, + }, d.SaferSQL) +} + +func TestClassifyAddPrimaryKeySequence(t *testing.T) { + d := classifyOne(t, "ALTER TABLE s.t ADD CONSTRAINT t_pkey PRIMARY KEY (id)") + assert.Equal(t, []string{ + `CREATE UNIQUE INDEX CONCURRENTLY "t_pkey" ON "s"."t" ("id")`, + `ALTER TABLE "s"."t" ADD CONSTRAINT "t_pkey" PRIMARY KEY USING INDEX "t_pkey"`, + }, d.SaferSQL) +} + +func TestClassifyAddCheckSequenceValidates(t *testing.T) { + d := classifyOne(t, "ALTER TABLE t ADD CONSTRAINT c CHECK (age > 0)") + require.Len(t, d.SaferSQL, 2) + // The rewritten first step must parse back as NOT VALID; the second + // step is the online validation of the same constraint. + assert.Contains(t, d.SaferSQL[1], "VALIDATE CONSTRAINT") +} + +func TestClassifyAggregatesWorstRoute(t *testing.T) { + plan, err := planner.Classify( + "ALTER TABLE t ADD COLUMN a int, ADD COLUMN created timestamptz DEFAULT now()", facts) + require.NoError(t, err) + require.Len(t, plan.Decisions, 2) + assert.Equal(t, planner.RouteNative, plan.Decisions[0].Route) + assert.Equal(t, planner.RouteCopyAndSwap, plan.Decisions[1].Route) + assert.Equal(t, planner.RouteCopyAndSwap, plan.Route, "one rewrite makes the statement a copy") +} + +func TestClassifyRefusalDominatesAggregate(t *testing.T) { + plan, err := planner.Classify( + "ALTER TABLE t ADD COLUMN created timestamptz DEFAULT now(), ENABLE ROW LEVEL SECURITY", facts) + require.NoError(t, err) + assert.Equal(t, planner.RouteRefuse, plan.Route, "a refused operation refuses the statement") +} + +func TestClassifyMultiOpStatementCarriesNoRewrites(t *testing.T) { + plan, err := planner.Classify( + "ALTER TABLE t ALTER COLUMN age SET NOT NULL, DROP COLUMN b", facts) + require.NoError(t, err) + for _, d := range plan.Decisions { + assert.Empty(t, d.SaferSQL, "multi-operation statements must not carry partial rewrites") + } +} + +func TestClassifyNoFactsIsConservative(t *testing.T) { + plan, err := planner.Classify("ALTER TABLE t ALTER COLUMN v50 TYPE varchar(100)", planner.Facts{}) + require.NoError(t, err) + require.Len(t, plan.Decisions, 1) + assert.Equal(t, planner.RouteCopyAndSwap, plan.Decisions[0].Route, + "without live column facts every type change is a rewrite") +} + +func TestClassifyParseErrorSurfaces(t *testing.T) { + _, err := planner.Classify("ALTER TABLE", planner.Facts{}) + assert.Error(t, err) +} From 16c42c0d6180c2a344575640ee42dace43b9bced Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 5 Aug 2026 17:36:46 +1000 Subject: [PATCH 3/4] Phase 2.4: router seam assigning classified statements to backends pkg/router is the policy layer between the classifier and the executors: every classified statement gets a backend (native / copy-and-swap) and a typed disposition; copy-and-swap routes come back unavailable until that executor exists, instead of pretending to run. diff and the new migrate --dry-run share the identical classify-and-route pipeline, with live column types feeding the classifier. Refs PLAT-38439. --- SAFETY.md | 2 +- internal/cli/cli.go | 3 +- internal/cli/diff.go | 136 ++++++++++++++++++++--- internal/cli/diff_integration_test.go | 57 ++++++++++ internal/cli/dryrun.go | 98 +++++++++++++++++ internal/cli/dryrun_integration_test.go | 126 ++++++++++++++++++++++ internal/cli/migrate.go | 6 +- pkg/router/router.go | 137 ++++++++++++++++++++++++ pkg/router/router_test.go | 93 ++++++++++++++++ 9 files changed, 643 insertions(+), 15 deletions(-) create mode 100644 internal/cli/dryrun.go create mode 100644 internal/cli/dryrun_integration_test.go create mode 100644 pkg/router/router.go create mode 100644 pkg/router/router_test.go diff --git a/SAFETY.md b/SAFETY.md index 9fc1b66..191f7e9 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` (parse boundary), `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect), and `pkg/planner` (classifier) exist (Phase 2); `pkg/lint` planned (Phase 2) | (CO-7 holds at the parse boundary) | +| `pkg/statement`, `pkg/planner`, `pkg/schemadiff`, `pkg/router`, `pkg/lint` — classify/diff/route | ❌ periphery¹ | `pkg/statement` (parse boundary), `pkg/schemadiff` (introspect/diff via scratch execute-and-introspect), `pkg/planner` (classifier), and `pkg/router` (backend assignment + availability policy) exist (Phase 2); `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/internal/cli/cli.go b/internal/cli/cli.go index 321de51..665d904 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -86,7 +86,8 @@ type MigrateCmd struct { Alter string `help:"Imperative ALTER statement to run." name:"alter" required:""` MaxTableSize byteSize `help:"Size threshold above which the optimistic attempt is skipped (binary units: B, KiB, MiB, GiB, TiB)." default:"1GiB"` - JSON bool `help:"Emit the verdict as JSON."` + DryRun bool `help:"Classify and route the statement, print the plan, and execute nothing."` + JSON bool `help:"Emit the verdict (or dry-run plan) as JSON."` } // Run implements the migrate subcommand. diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 39f170a..e5af19a 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -7,8 +7,11 @@ import ( "fmt" "io" "os" + "strings" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/schemadiff" "github.com/block/pg-sprite/pkg/statement" ) @@ -22,9 +25,68 @@ type diffReport struct { // TableExists reports whether the live table was found; when false the // changes are the full desired schema. TableExists bool `json:"table_exists"` + // Disposition is the routed plan's aggregate disposition: what would + // happen if the engine executed this plan now. + Disposition router.Disposition `json:"disposition"` // Changes is the ordered statement plan; empty means the live table // already matches the desired state. - Changes []schemadiff.Change `json:"changes"` + Changes []plannedChange `json:"changes"` +} + +// plannedChange is one diff statement with its classification and routing: +// the derived SQL plus where the engine would send it and what would run. +type plannedChange struct { + schemadiff.Change + // Route is the planner's aggregate route for the statement. + Route planner.Route `json:"route"` + // Backend is the assigned execution strategy; empty for refusals. + Backend router.Backend `json:"backend,omitempty"` + // Disposition is what execution would do with the statement now. + Disposition router.Disposition `json:"disposition"` + // Decisions are the planner's per-operation classifications. + Decisions []planner.Decision `json:"decisions"` + // ExecSQL is the ordered SQL the native backend would run — the safer + // sequence when the planner constructed one. Empty for non-native + // routes. + ExecSQL []string `json:"exec_sql,omitempty"` +} + +// classifyChanges routes every derived change through the shared +// classify-and-route pipeline. facts sharpen type-change classification; +// the zero value is valid and strictly more conservative. +func classifyChanges(changes []schemadiff.Change, facts planner.Facts) ([]plannedChange, router.Disposition, error) { + plans := make([]planner.Plan, 0, len(changes)) + for _, ch := range changes { + plan, err := planner.Classify(ch.SQL, facts) + if err != nil { + return nil, "", fmt.Errorf("classify derived statement %q: %w", ch.SQL, err) + } + plans = append(plans, plan) + } + routed := router.Route(plans) + planned := make([]plannedChange, 0, len(changes)) + for i, ch := range changes { + st := routed.Statements[i] + planned = append(planned, plannedChange{ + Change: ch, + Route: st.Route, + Backend: st.Backend, + Disposition: st.Disposition, + Decisions: st.Decisions, + ExecSQL: st.ExecSQL, + }) + } + return planned, routed.Disposition, nil +} + +// liveFacts extracts the planner facts the live model provides: the +// canonical type of every live column. +func liveFacts(live schemadiff.Model) planner.Facts { + types := make(map[string]string, len(live.Columns)) + for _, col := range live.Columns { + types[col.Name] = col.Type + } + return planner.Facts{ColumnTypes: types} } // run is the diff flow: parse and admit the desired file, introspect the @@ -50,28 +112,36 @@ func (c *DiffCmd) run(ctx context.Context, out io.Writer) error { defer pool.Close() report := diffReport{Schema: c.Schema, Table: ds.Table, TableExists: true} + var changes []schemadiff.Change + var facts planner.Facts 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. + // onto the target schema, classified with zero facts (there are no + // live columns to sharpen type-change decisions). report.TableExists = false - if report.Changes, err = qualifiedDesired(ds, c.Schema); err != nil { + if changes, err = qualifiedDesired(ds, c.Schema); err != nil { return err } case err != nil: return err default: + facts = liveFacts(live) desired, err := schemadiff.IntrospectDesired(ctx, pool, ds) if err != nil { return err } - if report.Changes, err = schemadiff.Diff(c.Schema, live, desired); err != nil { + if changes, err = schemadiff.Diff(c.Schema, live, desired); err != nil { return err } } + if report.Changes, report.Disposition, err = classifyChanges(changes, facts); err != nil { + return err + } logger.Debug("diff derived", - "schema", c.Schema, "table", ds.Table, "changes", len(report.Changes), "table_exists", report.TableExists) + "schema", c.Schema, "table", ds.Table, "changes", len(report.Changes), + "table_exists", report.TableExists, "disposition", string(report.Disposition)) if c.JSON { return writeJSON(out, report) @@ -96,7 +166,7 @@ func qualifiedDesired(ds statement.DesiredSchema, schema string) ([]schemadiff.C // writeJSON emits the report as JSON. func writeJSON(out io.Writer, report diffReport) error { if report.Changes == nil { - report.Changes = []schemadiff.Change{} + report.Changes = []plannedChange{} } enc := json.NewEncoder(out) enc.SetIndent("", " ") @@ -107,9 +177,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, each annotated with its route, destructive statements flagged, +// and SQL comments for the no-change and missing-table cases so the output +// stays valid SQL. Safer sequences appear as comment lines — never +// substituted into the script body, which stays the literal convergence +// plan (a CONCURRENTLY rewrite could not run inside a transaction block). 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 { @@ -124,18 +196,58 @@ func writePlanText(out io.Writer, report diffReport) error { } } for _, ch := range report.Changes { - if ch.Destructive { - if _, err := fmt.Fprintln(out, "-- destructive"); err != nil { + if err := writeChangeText(out, ch); err != nil { + return err + } + } + return nil +} + +// writeChangeText emits one annotated statement of the text plan. +func writeChangeText(out io.Writer, ch plannedChange) error { + if _, err := fmt.Fprintf(out, "-- %s\n", annotate(ch)); err != nil { + return fmt.Errorf("write plan: %w", err) + } + if len(ch.ExecSQL) > 0 && ch.ExecSQL[0] != ch.SQL { + if _, err := fmt.Fprintln(out, "-- the engine would run instead:"); err != nil { + return fmt.Errorf("write plan: %w", err) + } + for _, safer := range ch.ExecSQL { + if _, err := fmt.Fprintf(out, "-- %s;\n", safer); err != nil { return fmt.Errorf("write plan: %w", err) } } - if _, err := fmt.Fprintf(out, "%s;\n", ch.SQL); err != nil { + } + 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 } +// annotate renders one statement's route annotation: the route, the +// distinct decision reasons, and the availability note for backends this +// build does not implement. +func annotate(ch plannedChange) string { + var reasons []string + seen := map[planner.Reason]bool{} + for _, d := range ch.Decisions { + if !seen[d.Reason] { + seen[d.Reason] = true + reasons = append(reasons, string(d.Reason)) + } + } + s := fmt.Sprintf("%s (%s)", ch.Route, strings.Join(reasons, ", ")) + if ch.Disposition == router.DispositionUnavailable { + s += ": needs the " + string(ch.Backend) + " backend, which is not implemented yet" + } + return s +} + // 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. diff --git a/internal/cli/diff_integration_test.go b/internal/cli/diff_integration_test.go index da8c88a..a415c3c 100644 --- a/internal/cli/diff_integration_test.go +++ b/internal/cli/diff_integration_test.go @@ -14,6 +14,8 @@ import ( "github.com/block/pg-sprite/internal/testutil" "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" ) // newDiffCmd builds a DiffCmd with the flag defaults kong would apply, @@ -69,6 +71,61 @@ func TestDiffPrintsOrderedPlanJSON(t *testing.T) { fmt.Sprintf("CREATE INDEX events_name_idx ON %s.events USING btree (name)", schema), }, sqls) assert.Equal(t, []bool{true, false, false, false}, destructive) + + // Every derived statement is classified and routed: the widen is proven + // binary-coercible by the live facts, SET NOT NULL and CREATE INDEX + // carry their safer native sequences, and the whole plan would execute. + assert.Equal(t, router.DispositionExecute, report.Disposition) + routes := make([]planner.Route, 0, len(report.Changes)) + for _, ch := range report.Changes { + routes = append(routes, ch.Route) + assert.Equal(t, router.BackendNative, ch.Backend, ch.SQL) + assert.Equal(t, router.DispositionExecute, ch.Disposition, ch.SQL) + require.NotEmpty(t, ch.Decisions, ch.SQL) + } + assert.Equal(t, []planner.Route{ + planner.RouteNative, planner.RouteNative, planner.RouteNative, planner.RouteNative, + }, routes) + assert.Equal(t, planner.ReasonBinaryCoercible, report.Changes[1].Decisions[0].Reason, + "live column types must feed the classifier") + assert.Equal(t, planner.ReasonSaferIdiom, report.Changes[2].Decisions[0].Reason) + assert.NotEqual(t, []string{report.Changes[2].SQL}, report.Changes[2].ExecSQL, + "SET NOT NULL carries its safer native sequence") + assert.Equal(t, planner.ReasonSaferIdiom, report.Changes[3].Decisions[0].Reason) + require.Len(t, report.Changes[3].ExecSQL, 1) + assert.NotEqual(t, report.Changes[3].SQL, report.Changes[3].ExecSQL[0], + "CREATE INDEX carries its concurrent rewrite") +} + +// A desired state that needs a table rewrite routes to the copy-and-swap +// backend, and the routed plan says that backend is unavailable in this +// build — the plan is honest about what execution would do. +func TestDiffRoutesRewriteToCopyAndSwap(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 int PRIMARY KEY)", schema)) + require.NoError(t, err) + + cmd := newDiffCmd(t, url, schema, "CREATE TABLE events (id bigint PRIMARY KEY)") + 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, router.DispositionUnavailable, report.Disposition) + require.Len(t, report.Changes, 1) + ch := report.Changes[0] + assert.Equal(t, planner.RouteCopyAndSwap, ch.Route) + assert.Equal(t, router.BackendCopyAndSwap, ch.Backend) + assert.Equal(t, router.DispositionUnavailable, ch.Disposition) + assert.Empty(t, ch.ExecSQL) + require.Len(t, ch.Decisions, 1) + assert.Equal(t, planner.ReasonTypeRewrite, ch.Decisions[0].Reason) } // diff must never write: the live table is bit-identical before and after. diff --git a/internal/cli/dryrun.go b/internal/cli/dryrun.go new file mode 100644 index 0000000..3ebd57a --- /dev/null +++ b/internal/cli/dryrun.go @@ -0,0 +1,98 @@ +package cli + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/block/pg-sprite/pkg/dbconn" + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" + "github.com/block/pg-sprite/pkg/statement" +) + +// runDryRun is the imperative dry-run flow: the identical classify-and-route +// pipeline the declarative front-end uses, with the diff step skipped — the +// submitted statement feeds the classifier directly. It prints the routed +// plan and never executes anything. Introspecting the target table sharpens +// type-change classification; a missing table means zero facts and a +// strictly more conservative plan. +func (c *MigrateCmd) runDryRun(ctx context.Context, out io.Writer) error { + logger := c.diag() + st, err := statement.ParseOne(c.Alter) + if err != nil { + return err + } + logger.Debug("statement parsed", "kind", st.Kind, "schema", st.Schema, "table", st.Table) + + pool, err := dbconn.NewPool(ctx, c.Config()) + if err != nil { + return err + } + defer pool.Close() + + facts, err := dryRunFacts(ctx, pool, st) + if err != nil { + return err + } + plan, err := planner.Classify(st.SQL, facts) + if err != nil { + return err + } + routed := router.Route([]planner.Plan{plan}) + logger.Debug("statement routed", + "route", string(plan.Route), "disposition", string(routed.Disposition)) + + if c.JSON { + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + if err := enc.Encode(routed); err != nil { + return fmt.Errorf("write dry-run plan: %w", err) + } + return nil + } + for _, rs := range routed.Statements { + if err := writeChangeText(out, plannedFromRouted(rs)); err != nil { + return err + } + } + return nil +} + +// dryRunFacts introspects the statement's target table for classifier +// facts. Statements without a single table target (index maintenance) and +// missing tables classify with zero facts. +func dryRunFacts(ctx context.Context, pool *pgxpool.Pool, st statement.Statement) (planner.Facts, error) { + if st.Table == "" { + return planner.Facts{}, nil + } + schema := st.Schema + if schema == "" { + schema = "public" + } + live, err := schemadiff.Introspect(ctx, pool, schema, st.Table) + switch { + case errors.Is(err, schemadiff.ErrTableNotFound): + return planner.Facts{}, nil + case err != nil: + return planner.Facts{}, err + } + return liveFacts(live), nil +} + +// plannedFromRouted adapts a routed statement to the shared text renderer. +func plannedFromRouted(rs router.Statement) plannedChange { + return plannedChange{ + Change: schemadiff.Change{SQL: rs.Statement}, + Route: rs.Route, + Backend: rs.Backend, + Disposition: rs.Disposition, + Decisions: rs.Decisions, + ExecSQL: rs.ExecSQL, + } +} diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go new file mode 100644 index 0000000..2ab3a6e --- /dev/null +++ b/internal/cli/dryrun_integration_test.go @@ -0,0 +1,126 @@ +package cli + +import ( + "encoding/json" + "fmt" + "strings" + "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/planner" + "github.com/block/pg-sprite/pkg/router" +) + +// dryRunPlan runs migrate --dry-run --json and decodes the routed plan. +func dryRunPlan(t *testing.T, url, alter string) router.Plan { + t.Helper() + cmd := newMigrateCmd(url, alter) + cmd.DryRun = true + cmd.JSON = true + var out strings.Builder + require.NoError(t, cmd.run(t.Context(), &out)) + var plan router.Plan + require.NoError(t, json.Unmarshal([]byte(out.String()), &plan)) + return plan +} + +// A rewrite-requiring change dry-runs to the copy-and-swap backend as +// unavailable, and nothing executes: the live column type is untouched. +func TestMigrateDryRunRoutesRewriteWithoutExecuting(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) + require.NoError(t, err) + + plan := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN id TYPE bigint", schema)) + assert.Equal(t, router.DispositionUnavailable, plan.Disposition) + require.Len(t, plan.Statements, 1) + st := plan.Statements[0] + assert.Equal(t, planner.RouteCopyAndSwap, st.Route) + assert.Equal(t, router.BackendCopyAndSwap, st.Backend) + assert.Empty(t, st.ExecSQL) + + var typ string + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT data_type FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'id'`, schema).Scan(&typ)) + assert.Equal(t, "integer", typ, "dry-run must not execute the change") +} + +// Live facts feed the imperative dry-run: a widen the classifier can only +// prove with the live column type routes native, and still executes nothing. +func TestMigrateDryRunUsesLiveFacts(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf( + "CREATE TABLE %s.t (id int PRIMARY KEY, name varchar(20))", schema)) + require.NoError(t, err) + + alter := fmt.Sprintf("ALTER TABLE %s.t ALTER COLUMN name TYPE varchar(50)", schema) + plan := dryRunPlan(t, url, alter) + assert.Equal(t, router.DispositionExecute, plan.Disposition) + require.Len(t, plan.Statements, 1) + st := plan.Statements[0] + assert.Equal(t, planner.RouteNative, st.Route) + assert.Equal(t, router.BackendNative, st.Backend) + require.Len(t, st.Decisions, 1) + assert.Equal(t, planner.ReasonBinaryCoercible, st.Decisions[0].Reason, + "the live varchar(20) must be introspected to prove the widen") + assert.Equal(t, []string{alter}, st.ExecSQL) + + var maxLen int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT character_maximum_length FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'name'`, schema).Scan(&maxLen)) + assert.Equal(t, 20, maxLen, "dry-run must not execute the change") +} + +// The dry-run advisory covers statements the execute gate refuses: a plain +// CREATE INDEX comes back native with its concurrent rewrite, not created. +func TestMigrateDryRunSuggestsConcurrentIndex(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) + require.NoError(t, err) + + submitted := fmt.Sprintf("CREATE INDEX t_id_idx ON %s.t (id)", schema) + plan := dryRunPlan(t, url, submitted) + assert.Equal(t, router.DispositionExecute, plan.Disposition) + require.Len(t, plan.Statements, 1) + st := plan.Statements[0] + assert.Equal(t, planner.RouteNative, st.Route) + require.Len(t, st.Decisions, 1) + assert.Equal(t, planner.ReasonSaferIdiom, st.Decisions[0].Reason) + require.Len(t, st.ExecSQL, 1) + assert.NotEqual(t, submitted, st.ExecSQL[0], "the plan carries the concurrent rewrite") + + var indexes int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_indexes WHERE schemaname = $1 AND indexname = 't_id_idx'`, + schema).Scan(&indexes)) + assert.Equal(t, 0, indexes, "dry-run must not create the index") +} + +// A dry-run against a table that does not exist classifies with zero facts: +// the unprovable type change routes conservatively instead of failing. +func TestMigrateDryRunMissingTableIsConservative(t *testing.T) { + url := testutil.StartPostgres(t) + + plan := dryRunPlan(t, url, "ALTER TABLE missing ALTER COLUMN v TYPE varchar(50)") + assert.Equal(t, router.DispositionUnavailable, plan.Disposition) + require.Len(t, plan.Statements, 1) + assert.Equal(t, planner.RouteCopyAndSwap, plan.Statements[0].Route) +} diff --git a/internal/cli/migrate.go b/internal/cli/migrate.go index e43227c..1bdbf48 100644 --- a/internal/cli/migrate.go +++ b/internal/cli/migrate.go @@ -17,8 +17,12 @@ import ( // run is the migrate flow: gate the statement type, size-guard the table, // attempt the change under budget, and end in exactly one verdict. Refusal // verdicts are printed to out and returned as verdict.ErrRefused so the entry -// point maps them to the refusal exit code. +// point maps them to the refusal exit code. --dry-run diverts to the +// classify-and-route plan instead. func (c *MigrateCmd) run(ctx context.Context, out io.Writer) error { + if c.DryRun { + return c.runDryRun(ctx, out) + } logger := c.diag() st, err := statement.ParseOne(c.Alter) if err != nil { diff --git a/pkg/router/router.go b/pkg/router/router.go new file mode 100644 index 0000000..642086a --- /dev/null +++ b/pkg/router/router.go @@ -0,0 +1,137 @@ +// Package router assigns every classified statement to an execution +// backend. It sits between the planner (which decides what must change and +// how PostgreSQL would treat it) and the executors (which decide how a +// change actually runs), and is the single place migration policy lives: +// which backends exist, which are available, and what happens to a +// statement whose backend is not built yet. Callers branch on the typed +// Backend and Disposition, never on prose. +// +// This is a periphery package (see SAFETY.md): a routed plan is a request. +// Executors enforce their own protections regardless of the route. +package router + +import "github.com/block/pg-sprite/pkg/planner" + +// Backend identifies an execution strategy. +type Backend string + +// The backends the router can assign. A refused statement has no backend. +const ( + // BackendNative runs the change as direct PostgreSQL DDL (the safer + // online idiom when the planner constructed one) under bounded + // lock_timeout / statement_timeout. + BackendNative Backend = "native" + // BackendCopyAndSwap performs the change as a shadow-table copy with a + // logical-replication catch-up and a locked cutover swap. + BackendCopyAndSwap Backend = "copy-and-swap" +) + +// available reports whether the backend is implemented in this build. This +// is the routing policy for backends: copy-and-swap is a known strategy the +// planner routes to, but until its executor exists the router marks the +// statement unavailable rather than pretending it could run. +func available(b Backend) bool { + return b == BackendNative +} + +// Disposition is what would happen to one statement if the routed plan were +// executed now. +type Disposition string + +// The dispositions a routed statement can carry. +const ( + // DispositionExecute: the assigned backend is available; the statement + // would run. + DispositionExecute Disposition = "execute" + // DispositionUnavailable: the change needs a backend this build does + // not implement; the statement would be refused at execution. + DispositionUnavailable Disposition = "unavailable" + // DispositionRefuse: the planner refused the statement; no backend is + // assigned. + DispositionRefuse Disposition = "refuse" +) + +// worse orders dispositions for aggregation: refuse > unavailable > execute. +func worse(a, b Disposition) Disposition { + rank := map[Disposition]int{DispositionExecute: 0, DispositionUnavailable: 1, DispositionRefuse: 2} + if rank[b] > rank[a] { + return b + } + return a +} + +// Statement is one routed statement: the planner's classification plus the +// backend assignment and the literal SQL the native backend would execute. +type Statement struct { + planner.Plan + // Backend is the assigned execution strategy; empty for refusals. + Backend Backend `json:"backend,omitempty"` + // Disposition is what execution would do with the statement now. + Disposition Disposition `json:"disposition"` + // ExecSQL is the ordered SQL the native backend would run: the + // planner's safer sequence when it constructed one, otherwise the + // submitted statement. Empty for non-native routes. + ExecSQL []string `json:"exec_sql,omitempty"` +} + +// Plan is the routed plan for an ordered statement list: one routed +// statement per input plan plus the aggregate disposition (the worst of its +// statements — one unavailable backend makes the whole plan unavailable, +// one refusal refuses it). +type Plan struct { + // Statements are the routed statements, in input order. + Statements []Statement `json:"statements"` + // Disposition is the aggregate disposition. + Disposition Disposition `json:"disposition"` +} + +// Route assigns a backend to every classified statement. It is pure policy: +// no parsing, no database access — classification happens before, execution +// after. +func Route(plans []planner.Plan) Plan { + routed := Plan{Statements: make([]Statement, 0, len(plans)), Disposition: DispositionExecute} + for _, p := range plans { + st := routeStatement(p) + routed.Disposition = worse(routed.Disposition, st.Disposition) + routed.Statements = append(routed.Statements, st) + } + return routed +} + +// routeStatement maps one classified statement to its backend and +// disposition. +func routeStatement(p planner.Plan) Statement { + st := Statement{Plan: p} + switch p.Route { + case planner.RouteNative: + st.Backend = BackendNative + st.ExecSQL = nativeExecSQL(p) + case planner.RouteCopyAndSwap: + st.Backend = BackendCopyAndSwap + case planner.RouteRefuse: + st.Disposition = DispositionRefuse + return st + default: + // An unknown route is a planner/router version skew; refuse it + // rather than guess a backend. + st.Disposition = DispositionRefuse + return st + } + if !available(st.Backend) { + st.Disposition = DispositionUnavailable + return st + } + st.Disposition = DispositionExecute + return st +} + +// nativeExecSQL is the literal SQL the native backend would run for a +// native-routed statement: the planner's safer sequence when it constructed +// one (only single-operation statements carry one), otherwise the submitted +// form. +func nativeExecSQL(p planner.Plan) []string { + if len(p.Decisions) == 1 && len(p.Decisions[0].SaferSQL) > 0 { + return p.Decisions[0].SaferSQL + } + return []string{p.Statement} +} diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go new file mode 100644 index 0000000..47d23d6 --- /dev/null +++ b/pkg/router/router_test.go @@ -0,0 +1,93 @@ +package router_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/router" +) + +// classify runs the real classifier so routing tests exercise the same +// plans the CLI produces, not hand-built ones. +func classify(t *testing.T, sql string) planner.Plan { + t.Helper() + plan, err := planner.Classify(sql, planner.Facts{}) + require.NoError(t, err) + return plan +} + +func TestRouteNativeExecutesSubmittedForm(t *testing.T) { + plan := classify(t, "ALTER TABLE t ADD COLUMN age int DEFAULT 0") + routed := router.Route([]planner.Plan{plan}) + + require.Len(t, routed.Statements, 1) + st := routed.Statements[0] + assert.Equal(t, router.BackendNative, st.Backend) + assert.Equal(t, router.DispositionExecute, st.Disposition) + assert.Equal(t, []string{plan.Statement}, st.ExecSQL, + "a native statement with no safer sequence executes as submitted") + assert.Equal(t, router.DispositionExecute, routed.Disposition) +} + +func TestRouteNativeExecutesSaferSequence(t *testing.T) { + plan := classify(t, "CREATE INDEX events_name_idx ON events (name)") + require.Len(t, plan.Decisions, 1) + require.NotEmpty(t, plan.Decisions[0].SaferSQL, "classifier must construct the concurrent rewrite") + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Equal(t, router.BackendNative, st.Backend) + assert.Equal(t, router.DispositionExecute, st.Disposition) + assert.Equal(t, plan.Decisions[0].SaferSQL, st.ExecSQL, + "the native backend runs the safer sequence, not the submitted form") +} + +func TestRouteCopyAndSwapIsUnavailable(t *testing.T) { + plan := classify(t, "ALTER TABLE t ALTER COLUMN id TYPE bigint") + require.Equal(t, planner.RouteCopyAndSwap, plan.Route) + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Equal(t, router.BackendCopyAndSwap, st.Backend) + assert.Equal(t, router.DispositionUnavailable, st.Disposition) + assert.Empty(t, st.ExecSQL, "no literal SQL for a backend that is not implemented") + assert.Equal(t, router.DispositionUnavailable, routed.Disposition) +} + +func TestRouteRefusedStatementHasNoBackend(t *testing.T) { + plan := classify(t, "ALTER TABLE t ADD CONSTRAINT ex EXCLUDE USING gist (room WITH =)") + require.Equal(t, planner.RouteRefuse, plan.Route) + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Empty(t, st.Backend) + assert.Equal(t, router.DispositionRefuse, st.Disposition) + assert.Empty(t, st.ExecSQL) + assert.Equal(t, router.DispositionRefuse, routed.Disposition) +} + +func TestRouteAggregateIsWorstDisposition(t *testing.T) { + plans := []planner.Plan{ + classify(t, "ALTER TABLE t ADD COLUMN age int"), + classify(t, "ALTER TABLE t ALTER COLUMN id TYPE bigint"), + classify(t, "ALTER TABLE t ADD CONSTRAINT ex EXCLUDE USING gist (room WITH =)"), + } + routed := router.Route(plans) + + require.Len(t, routed.Statements, 3) + assert.Equal(t, router.DispositionExecute, routed.Statements[0].Disposition) + assert.Equal(t, router.DispositionUnavailable, routed.Statements[1].Disposition) + assert.Equal(t, router.DispositionRefuse, routed.Statements[2].Disposition) + assert.Equal(t, router.DispositionRefuse, routed.Disposition, + "one refusal refuses the whole plan") +} + +func TestRouteEmptyPlanExecutes(t *testing.T) { + routed := router.Route(nil) + assert.Empty(t, routed.Statements) + assert.Equal(t, router.DispositionExecute, routed.Disposition, + "a plan with nothing to do has nothing blocking execution") +} From f700319a242efa2886578f523f6a7d3ce621f3ad Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Fri, 7 Aug 2026 10:04:33 +1000 Subject: [PATCH 4/4] planner, router: fail closed on unconstructed safer rewrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #7 review findings: inline ADD COLUMN constraints and CREATE TABLE ... PARTITION OF are now classified as the work they do, and routing refuses (rewrite-required) any statement whose submitted form blocks when no complete online rewrite was constructed — partial rewrites of multi-operation statements can no longer execute the original SQL. Generated scaffold names fit PostgreSQL's 63-byte identifier limit deterministically. --- docs/postgres-online-ddl-reference.md | 4 + internal/cli/diff.go | 5 +- internal/cli/dryrun_integration_test.go | 29 +++++++ pkg/planner/planner.go | 110 ++++++++++++++++++++++-- pkg/planner/planner_test.go | 45 ++++++++++ pkg/router/router.go | 46 ++++++++-- pkg/router/router_test.go | 61 ++++++++++++- pkg/statement/ops.go | 44 +++++++++- pkg/statement/ops_test.go | 37 ++++++++ 9 files changed, 357 insertions(+), 24 deletions(-) diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index 1c07f03..c707722 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -57,6 +57,7 @@ the same dynamic via metadata locks. Read it first if any of that is unfamiliar. | `ADD COLUMN ... DEFAULT ` | ACCESS EXCLUSIVE (brief) | **No** (PG 11+) | Yes | ❌ No | "Fast default" stored in catalog; pre-PG11 this rewrote | | `ADD COLUMN ... DEFAULT ` (e.g. `now()`, `random()`, `uuid_generate_v4()`) | ACCESS EXCLUSIVE | **Yes** (full rewrite) | No | ✅ **Yes** | The expensive case | | `ADD COLUMN ... GENERATED ALWAYS AS (...) STORED` | ACCESS EXCLUSIVE | Yes | No | ✅ **Yes** | Values must be computed | +| `ADD COLUMN ... UNIQUE` / `PRIMARY KEY` / `REFERENCES` / `CHECK` (inline constraint) | ACCESS EXCLUSIVE + index build or validation | No | No | ➖ Native pattern | Same work as the `ADD CONSTRAINT` form, under the `ADD COLUMN` lock — split: add the column first, then build the constraint online (`CONCURRENTLY` + `USING INDEX`, or `NOT VALID` + `VALIDATE`) | | `DROP COLUMN` | ACCESS EXCLUSIVE (brief) | No | Yes | ❌ No | Metadata only; disk space reclaimed lazily by VACUUM | | `ALTER COLUMN TYPE` — binary-coercible (`varchar(50)→varchar(100)`, `varchar→text`, `numeric(10,2)→numeric(12,2)`) | ACCESS EXCLUSIVE (brief) | **No** | No (brief) | ❌ No | No scan when binary-coercible and no length restriction is added | | `ALTER COLUMN TYPE` — general (`int→bigint`, `text→jsonb`, `timestamp→timestamptz` w/ conversion) | ACCESS EXCLUSIVE | **Yes** (rewrite + reindex + revalidate FKs) | No | ✅ **Yes** | The classic "needs a tool" case | @@ -96,6 +97,7 @@ online path, so the heavy **shadow-copy + atomic cutover** path is required · | `ALTER COLUMN SET STATISTICS` / `SET STORAGE` / `SET (n_distinct=...)` | No | varies | ❌ No | SHARE UPDATE EXCLUSIVE / ACCESS EXCLUSIVE | | `ALTER COLUMN TYPE` — binary-coercible | No | No (brief) | ❌ No | ACCESS EXCLUSIVE (brief) | | `ALTER COLUMN SET NOT NULL` | No, but full scan | No | ➖ Native pattern | ACCESS EXCLUSIVE | +| `ADD COLUMN ...` (inline constraint) | No, but index build / validation | No | ➖ Native pattern | ACCESS EXCLUSIVE | | `ADD COLUMN ... DEFAULT ` | Yes | No | ✅ **Yes** | ACCESS EXCLUSIVE | | `ADD COLUMN ... GENERATED ALWAYS AS (...) STORED` | Yes | No | ✅ **Yes** | ACCESS EXCLUSIVE | | `ALTER COLUMN TYPE` — general | Yes | No | ✅ **Yes** | ACCESS EXCLUSIVE | @@ -116,6 +118,7 @@ online path, so the heavy **shadow-copy + atomic cutover** path is required · | `ALTER COLUMN SET STATISTICS` / `SET STORAGE` / `SET (n_distinct=...)` | varies | No | ❌ No | SHARE UPDATE EXCLUSIVE / ACCESS EXCLUSIVE | | `ALTER COLUMN TYPE` — binary-coercible | No (brief) | No | ❌ No | ACCESS EXCLUSIVE (brief) | | `ALTER COLUMN SET NOT NULL` | No | No, but full scan | ➖ Native pattern | ACCESS EXCLUSIVE | +| `ADD COLUMN ...` (inline constraint) | No | No, but index build / validation | ➖ Native pattern | ACCESS EXCLUSIVE | | `ADD COLUMN ... DEFAULT ` | No | Yes | ✅ **Yes** | ACCESS EXCLUSIVE | | `ADD COLUMN ... GENERATED ALWAYS AS (...) STORED` | No | Yes | ✅ **Yes** | ACCESS EXCLUSIVE | | `ALTER COLUMN TYPE` — general | No | Yes | ✅ **Yes** | ACCESS EXCLUSIVE | @@ -188,6 +191,7 @@ ALTER TABLE t ADD CONSTRAINT t_pkey PRIMARY KEY USING INDEX t_pkey; -- brief lo | `SET TABLESPACE` | ACCESS EXCLUSIVE | **Yes** (moves heap) | No | ✅ **Yes** (repack-style) | Rewrite/move; use a repack-style copy instead | | `SET (fillfactor=...)` and most reloptions | SHARE UPDATE EXCLUSIVE | No | Yes | ❌ No | Applies to new rows | | `CLUSTER` / `VACUUM FULL` | ACCESS EXCLUSIVE | **Yes** (full rewrite) | No | ✅ **Yes** (`pg_repack`) | Use `pg_repack` | +| `CREATE TABLE ... PARTITION OF` | ACCESS EXCLUSIVE on **parent** (brief) | No | Blocked on parent while held | ❌ No | Brief and no scan, but it queues behind long-running queries and then blocks every reader of the parent | | `ATTACH PARTITION` | SHARE UPDATE EXCLUSIVE on parent + scan of child | No | Yes | ➖ Native pattern | Add a validated `CHECK` matching the bound on the child first to skip the scan | | `DETACH PARTITION` | ACCESS EXCLUSIVE | No | No | ➖ Use `CONCURRENTLY` | | | `DETACH PARTITION CONCURRENTLY` | SHARE UPDATE EXCLUSIVE | No | Yes | ❌ No | PG 14+ | diff --git a/internal/cli/diff.go b/internal/cli/diff.go index 53ccca6..735520b 100644 --- a/internal/cli/diff.go +++ b/internal/cli/diff.go @@ -254,8 +254,11 @@ func annotate(ch plannedChange) string { } } s := fmt.Sprintf("%s (%s)", ch.Route, strings.Join(reasons, ", ")) - if ch.Disposition == router.DispositionUnavailable { + switch ch.Disposition { + case router.DispositionUnavailable: s += ": needs the " + string(ch.Backend) + " backend, which is not implemented yet" + case router.DispositionRewriteRequired: + s += ": blocks as submitted and no online rewrite was constructed — the engine will not run it" } return s } diff --git a/internal/cli/dryrun_integration_test.go b/internal/cli/dryrun_integration_test.go index 2ab3a6e..dbc72e5 100644 --- a/internal/cli/dryrun_integration_test.go +++ b/internal/cli/dryrun_integration_test.go @@ -114,6 +114,35 @@ func TestMigrateDryRunSuggestsConcurrentIndex(t *testing.T) { assert.Equal(t, 0, indexes, "dry-run must not create the index") } +// A safer-idiom decision without a constructed rewrite dry-runs to +// rewrite-required with no executable SQL, and nothing executes: the +// engine must not fall back to the submitted blocking form. +func TestMigrateDryRunInlineConstraintIsRewriteRequired(t *testing.T) { + url := testutil.StartPostgres(t) + pool, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: url}) + require.NoError(t, err) + defer pool.Close() + schema := testutil.NewSchema(t, pool) + _, err = pool.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema)) + require.NoError(t, err) + + plan := dryRunPlan(t, url, fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN c int UNIQUE", schema)) + assert.Equal(t, router.DispositionRewriteRequired, plan.Disposition) + require.Len(t, plan.Statements, 1) + st := plan.Statements[0] + assert.Equal(t, planner.RouteNative, st.Route) + assert.Equal(t, router.BackendNative, st.Backend) + require.Len(t, st.Decisions, 1) + assert.Equal(t, planner.ReasonSaferIdiom, st.Decisions[0].Reason) + assert.Empty(t, st.ExecSQL, "no executable SQL for an unconstructed rewrite") + + var columns int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM information_schema.columns + WHERE table_schema = $1 AND table_name = 't' AND column_name = 'c'`, schema).Scan(&columns)) + assert.Equal(t, 0, columns, "dry-run must not add the column") +} + // A dry-run against a table that does not exist classifies with zero facts: // the unprovable type change routes conservatively instead of failing. func TestMigrateDryRunMissingTableIsConservative(t *testing.T) { diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index a6ccfeb..cf9e1a6 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -5,12 +5,23 @@ // column of docs/postgres-online-ddl-reference.md, applied conservatively: // anything the planner cannot prove safe routes to copy-and-swap or refuse. // Classification predicts; executors keep their own protections regardless. +// +// The rules assume PostgreSQL 14, the oldest major the test matrix runs; +// every rule holds unconditionally across the supported range (14–18). +// Rules that were version-dependent below that floor — fast default +// (PG 11+), SET NOT NULL proven by a validated CHECK (PG 12+), DETACH +// PARTITION CONCURRENTLY (PG 14+) — carry no version annotation because +// the floor makes them unconditional. A rule that varies within the +// supported range must carry an explicit version fact before it lands. package planner import ( + "crypto/sha256" "fmt" + "slices" "strconv" "strings" + "unicode/utf8" "github.com/jackc/pgx/v5" @@ -74,6 +85,10 @@ const ( // ReasonRelocation: SET TABLESPACE moves the heap — a rewrite-scale // copy. ReasonRelocation Reason = "relocation" + // ReasonPartitionParentLock: creating a partition takes a brief ACCESS + // EXCLUSIVE on the partitioned parent — no scan, but it queues behind + // and then blocks every query on the parent while held. + ReasonPartitionParentLock Reason = "partition-parent-lock" // ReasonUnsupportedOperation: the planner does not recognize the // operation or knows no safe path for it. ReasonUnsupportedOperation Reason = "unsupported-operation" @@ -89,10 +104,23 @@ type Decision struct { Reason Reason `json:"reason"` // SaferSQL is the ordered native sequence to run instead of the // submitted form, present only for safer-idiom decisions where the - // planner could construct it. + // planner could construct it. Execution contract: the steps run one at + // a time, in order, each in its own implicit transaction — never inside + // an enclosing transaction block, which the CONCURRENTLY forms refuse. + // Each sequence constructor documents what a failed step leaves behind + // and how a retry resumes. SaferSQL []string `json:"safer_sql,omitempty"` } +// ExecutableAsSubmitted reports whether the operation's submitted form is +// itself safe to run. It is false exactly for safer-idiom decisions: their +// submitted form blocks and must be replaced by the safer sequence — +// whether or not one was constructed. Routing fails closed on the +// combination of a false ExecutableAsSubmitted and an empty SaferSQL. +func (d Decision) ExecutableAsSubmitted() bool { + return d.Reason != ReasonSaferIdiom +} + // Plan is the classification of one statement: one decision per operation // and the aggregate route (the worst of its decisions — one rewrite makes // the whole statement a copy, one refusal refuses it). @@ -105,9 +133,13 @@ type Plan struct { Decisions []Decision `json:"decisions"` } -// Facts are introspected properties of the live table that sharpen -// classification. The zero value is valid: with no facts the planner is -// strictly more conservative (every type change becomes copy-and-swap). +// Facts are properties of the live table that sharpen classification, and +// they are trusted as stated: the CLI fills them by introspecting the +// target database, and a library caller may supply facts it already holds — +// but they must describe the database the change will run on, because a +// wrong fact can upgrade a rewrite to native. Missing facts are always +// safe: the zero value is valid and classifies strictly more +// conservatively (every type change becomes copy-and-swap). type Facts struct { // ColumnTypes maps a column name to its live type as rendered by // PostgreSQL's format_type (e.g. "character varying(50)"). @@ -143,15 +175,33 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string d := Decision{Operation: op.Describe()} switch op.Kind { case statement.OpCreateTable: - // A new table has no readers to lock out. - d.Route, d.Reason = RouteNative, ReasonMetadataOnly + if op.PartitionOf { + // Creating a partition locks the partitioned parent ACCESS + // EXCLUSIVE — briefly and without a scan, but it queues behind + // any long-running query and then blocks every reader of the + // parent while held. + d.Route, d.Reason = RouteNative, ReasonPartitionParentLock + } else { + // A brand-new standalone table has no readers to lock out. + d.Route, d.Reason = RouteNative, ReasonMetadataOnly + } case statement.OpAddColumn: switch { + case hasUnrecognizedConstraint(op.InlineConstraints): + d.Route, d.Reason = RouteRefuse, ReasonUnsupportedOperation case op.GeneratedStored: d.Route, d.Reason = RouteCopyAndSwap, ReasonGeneratedStored case op.Default == statement.DefaultExpression: d.Route, d.Reason = RouteCopyAndSwap, ReasonVolatileDefault + case len(op.InlineConstraints) > 0: + // An inline UNIQUE / PRIMARY KEY / FOREIGN KEY / CHECK does the + // same index build or validation as its ADD CONSTRAINT form, + // under the ADD COLUMN's ACCESS EXCLUSIVE lock. The safer path + // splits the column addition from an online constraint build; + // the planner does not construct multi-statement splits, so the + // decision carries no rewrite and routing fails closed. + d.Route, d.Reason = RouteNative, ReasonSaferIdiom case op.Default == statement.DefaultConstant: d.Route, d.Reason = RouteNative, ReasonFastDefault default: @@ -374,6 +424,34 @@ func parseTypeText(s string) typeShape { return shape } +// hasUnrecognizedConstraint reports whether an added column carries an +// inline constraint family the engine does not model. +func hasUnrecognizedConstraint(kinds []statement.ConstraintKind) bool { + return slices.Contains(kinds, statement.ConstraintUnrecognized) +} + +// maxIdentifierBytes is PostgreSQL's NAMEDATALEN-1: the server silently +// truncates longer identifiers, which would let a generated name collide +// with the table itself or with a sibling scaffold. +const maxIdentifierBytes = 63 + +// fitIdentifier returns name unchanged when it fits PostgreSQL's identifier +// limit, otherwise a deterministic variant that does: the head of the name +// plus an 8-hex-digit hash of the full name, so distinct inputs stay +// distinct after fitting. +func fitIdentifier(name string) string { + if len(name) <= maxIdentifierBytes { + return name + } + sum := sha256.Sum256([]byte(name)) + suffix := fmt.Sprintf("_%x", sum[:4]) + head := name[:maxIdentifierBytes-len(suffix)] + for !utf8.ValidString(head) { + head = head[:len(head)-1] + } + return head + suffix +} + // tableIdent renders the statement's target table as a quoted identifier, // schema-qualified when the statement was. func tableIdent(st statement.Statement) string { @@ -386,9 +464,16 @@ func tableIdent(st statement.Statement) string { // setNotNullSequence is the native four-step SET NOT NULL pattern: prove // the invariant online with a NOT VALID CHECK, flip the column, drop the // scaffold. +// +// Partial-failure contract: step 1 leaves a NOT VALID CHECK under the +// generated scaffold name, and re-running step 1 then fails with SQLSTATE +// 42710 (duplicate_object) — a retry resumes at step 2. A failed VALIDATE +// (step 2) leaves the same scaffold and is safe to re-run. Steps 3 and 4 +// are metadata-only and safe to re-run; a leftover scaffold is removed by +// running step 4 alone. func setNotNullSequence(st statement.Statement, column string) []string { table := tableIdent(st) - conName := fmt.Sprintf("%s_%s_not_null", st.Table(), column) + conName := fitIdentifier(fmt.Sprintf("%s_%s_not_null", st.Table(), column)) con := pgx.Identifier{conName}.Sanitize() col := pgx.Identifier{column}.Sanitize() return []string{ @@ -402,6 +487,12 @@ func setNotNullSequence(st statement.Statement, column string) []string { // usingIndexSequence is the native two-step ADD PRIMARY KEY / UNIQUE // pattern: build the unique index concurrently, then attach it as the // constraint under a brief lock. +// +// Partial-failure contract: a failed CREATE INDEX CONCURRENTLY (step 1) +// leaves an INVALID index under the generated name, and re-running step 1 +// then fails with SQLSTATE 42P07 (duplicate_table) — the retry path is +// DROP INDEX, then re-run step 1. Step 2 consumes the index into the +// constraint under a brief lock and does not scan. func usingIndexSequence(st statement.Statement, op statement.Op) []string { suffix := "_key" keyword := "UNIQUE" @@ -411,7 +502,10 @@ func usingIndexSequence(st statement.Statement, op statement.Op) []string { } name := op.Name if name == "" { - name = st.Table() + "_" + strings.Join(op.Columns, "_") + suffix + // A user-supplied name is used as-is: the server truncates it the + // same way in every step. A generated name is built to fit so it + // cannot truncate into the table's own name or a sibling's. + name = fitIdentifier(st.Table() + "_" + strings.Join(op.Columns, "_") + suffix) } idx := pgx.Identifier{name}.Sanitize() cols := make([]string, len(op.Columns)) diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index 28113ec..7dc2593 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -1,12 +1,15 @@ package planner_test import ( + "fmt" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/block/pg-sprite/pkg/planner" + "github.com/block/pg-sprite/pkg/statement" ) // facts mirrors a live table whose column types exercise both sides of the @@ -46,6 +49,10 @@ func TestClassifyReferenceRows(t *testing.T) { {"add column volatile default random", "ALTER TABLE t ADD COLUMN r float8 DEFAULT random()", planner.RouteCopyAndSwap, planner.ReasonVolatileDefault, 0}, {"add column volatile default uuid", "ALTER TABLE t ADD COLUMN id uuid DEFAULT uuid_generate_v4()", planner.RouteCopyAndSwap, planner.ReasonVolatileDefault, 0}, {"add column generated stored", "ALTER TABLE t ADD COLUMN total numeric GENERATED ALWAYS AS (price * qty) STORED", planner.RouteCopyAndSwap, planner.ReasonGeneratedStored, 0}, + {"add column inline unique", "ALTER TABLE t ADD COLUMN c int UNIQUE", planner.RouteNative, planner.ReasonSaferIdiom, 0}, + {"add column inline primary key", "ALTER TABLE t ADD COLUMN c int PRIMARY KEY", planner.RouteNative, planner.ReasonSaferIdiom, 0}, + {"add column inline foreign key", "ALTER TABLE t ADD COLUMN c int REFERENCES p (id)", planner.RouteNative, planner.ReasonSaferIdiom, 0}, + {"add column inline check", "ALTER TABLE t ADD COLUMN c int CHECK (c > 0)", planner.RouteNative, planner.ReasonSaferIdiom, 0}, {"drop column", "ALTER TABLE t DROP COLUMN age", planner.RouteNative, planner.ReasonMetadataOnly, 0}, {"alter type widen varchar", "ALTER TABLE t ALTER COLUMN v50 TYPE varchar(100)", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, {"alter type varchar to text", "ALTER TABLE t ALTER COLUMN v50 TYPE text", planner.RouteNative, planner.ReasonBinaryCoercible, 0}, @@ -98,6 +105,7 @@ func TestClassifyReferenceRows(t *testing.T) { {"attach partition", "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)", planner.RouteNative, planner.ReasonSaferIdiom, 0}, {"detach partition", "ALTER TABLE t DETACH PARTITION p", planner.RouteNative, planner.ReasonSaferIdiom, 1}, {"detach partition concurrently", "ALTER TABLE t DETACH PARTITION p CONCURRENTLY", planner.RouteNative, planner.ReasonOnlineIdiom, 0}, + {"create table partition of", "CREATE TABLE p PARTITION OF t FOR VALUES FROM (1) TO (10)", planner.RouteNative, planner.ReasonPartitionParentLock, 0}, // Non-DDL and unknown statements. {"dml", "INSERT INTO t VALUES (1)", planner.RouteRefuse, planner.ReasonUnsupportedOperation, 0}, @@ -177,3 +185,40 @@ func TestClassifyParseErrorSurfaces(t *testing.T) { _, err := planner.Classify("ALTER TABLE", planner.Facts{}) assert.Error(t, err) } + +// generatedConstraintName parses a safer-sequence ADD CONSTRAINT step back +// through the statement parser and returns the typed constraint name, so +// the test asserts identifier facts rather than SQL prose. +func generatedConstraintName(t *testing.T, step string) string { + t.Helper() + ops, err := statement.ParseOps(step) + require.NoError(t, err) + require.Len(t, ops, 1) + require.Equal(t, statement.OpAddConstraint, ops[0].Kind) + require.NotEmpty(t, ops[0].Name) + return ops[0].Name +} + +func TestClassifyGeneratedNamesFitIdentifierLimit(t *testing.T) { + // Long enough that table + column + suffix would exceed PostgreSQL's + // 63-byte identifier limit, where the server would silently truncate. + table := strings.Repeat("t", 40) + colA := strings.Repeat("a", 40) + colB := strings.Repeat("b", 40) + + dA := classifyOne(t, fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL", table, colA)) + dB := classifyOne(t, fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL", table, colB)) + require.NotEmpty(t, dA.SaferSQL) + require.NotEmpty(t, dB.SaferSQL) + + nameA := generatedConstraintName(t, dA.SaferSQL[0]) + nameB := generatedConstraintName(t, dB.SaferSQL[0]) + assert.LessOrEqual(t, len(nameA), 63, "generated names must fit PostgreSQL's identifier limit") + assert.LessOrEqual(t, len(nameB), 63) + assert.NotEqual(t, nameA, nameB, + "columns differing only past the truncation point must not collide") + + // Deterministic: the same input yields the same fitted name. + dA2 := classifyOne(t, fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET NOT NULL", table, colA)) + assert.Equal(t, dA.SaferSQL, dA2.SaferSQL) +} diff --git a/pkg/router/router.go b/pkg/router/router.go index 642086a..a5a721f 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -43,6 +43,12 @@ const ( // DispositionExecute: the assigned backend is available; the statement // would run. DispositionExecute Disposition = "execute" + // DispositionRewriteRequired: the planner says the submitted form + // blocks and must run as a safer idiom, but no executable rewrite was + // constructed (a multi-operation statement, or a pattern the planner + // cannot build). The statement will be refused at execution rather + // than run in its blocking form. + DispositionRewriteRequired Disposition = "rewrite-required" // DispositionUnavailable: the change needs a backend this build does // not implement; the statement would be refused at execution. DispositionUnavailable Disposition = "unavailable" @@ -51,9 +57,15 @@ const ( DispositionRefuse Disposition = "refuse" ) -// worse orders dispositions for aggregation: refuse > unavailable > execute. +// worse orders dispositions for aggregation: +// refuse > unavailable > rewrite-required > execute. func worse(a, b Disposition) Disposition { - rank := map[Disposition]int{DispositionExecute: 0, DispositionUnavailable: 1, DispositionRefuse: 2} + rank := map[Disposition]int{ + DispositionExecute: 0, + DispositionRewriteRequired: 1, + DispositionUnavailable: 2, + DispositionRefuse: 3, + } if rank[b] > rank[a] { return b } @@ -70,7 +82,11 @@ type Statement struct { Disposition Disposition `json:"disposition"` // ExecSQL is the ordered SQL the native backend would run: the // planner's safer sequence when it constructed one, otherwise the - // submitted statement. Empty for non-native routes. + // submitted statement. Execution contract: the steps run one at a + // time, in order, each in its own implicit transaction — never wrapped + // in an enclosing transaction block, which the CONCURRENTLY forms + // refuse. Empty for non-native routes and for statements the engine + // will not run (DispositionRewriteRequired). ExecSQL []string `json:"exec_sql,omitempty"` } @@ -105,7 +121,6 @@ func routeStatement(p planner.Plan) Statement { switch p.Route { case planner.RouteNative: st.Backend = BackendNative - st.ExecSQL = nativeExecSQL(p) case planner.RouteCopyAndSwap: st.Backend = BackendCopyAndSwap case planner.RouteRefuse: @@ -121,6 +136,14 @@ func routeStatement(p planner.Plan) Statement { st.Disposition = DispositionUnavailable return st } + if st.Backend == BackendNative { + sql, ok := nativeExecSQL(p) + if !ok { + st.Disposition = DispositionRewriteRequired + return st + } + st.ExecSQL = sql + } st.Disposition = DispositionExecute return st } @@ -128,10 +151,17 @@ func routeStatement(p planner.Plan) Statement { // nativeExecSQL is the literal SQL the native backend would run for a // native-routed statement: the planner's safer sequence when it constructed // one (only single-operation statements carry one), otherwise the submitted -// form. -func nativeExecSQL(p planner.Plan) []string { +// form — but only when every decision is safe to run as submitted. A +// safer-idiom decision without a constructed rewrite yields no executable +// SQL: running the submitted form would falsify the plan's own reason. +func nativeExecSQL(p planner.Plan) ([]string, bool) { if len(p.Decisions) == 1 && len(p.Decisions[0].SaferSQL) > 0 { - return p.Decisions[0].SaferSQL + return p.Decisions[0].SaferSQL, true + } + for _, d := range p.Decisions { + if !d.ExecutableAsSubmitted() { + return nil, false + } } - return []string{p.Statement} + return []string{p.Statement}, true } diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 47d23d6..6531d4a 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -45,6 +45,49 @@ func TestRouteNativeExecutesSaferSequence(t *testing.T) { "the native backend runs the safer sequence, not the submitted form") } +func TestRouteSaferIdiomWithoutRewriteFailsClosed(t *testing.T) { + // ATTACH PARTITION is a safer-idiom decision the planner does not + // construct a rewrite for: routing must not fall back to the + // submitted blocking form. + plan := classify(t, "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)") + require.Equal(t, planner.RouteNative, plan.Route) + require.Len(t, plan.Decisions, 1) + require.Empty(t, plan.Decisions[0].SaferSQL) + require.False(t, plan.Decisions[0].ExecutableAsSubmitted()) + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Equal(t, router.BackendNative, st.Backend) + assert.Equal(t, router.DispositionRewriteRequired, st.Disposition) + assert.Empty(t, st.ExecSQL, "no executable SQL: running the submitted form would falsify the plan") + assert.Equal(t, router.DispositionRewriteRequired, routed.Disposition) +} + +func TestRouteMultiOpPartialRewriteFailsClosed(t *testing.T) { + // SET NOT NULL needs a safer sequence, but multi-operation statements + // carry no rewrites — the submitted form must not run on the strength + // of the harmless sibling operation. + plan := classify(t, "ALTER TABLE t ALTER COLUMN age SET NOT NULL, DROP COLUMN b") + require.Equal(t, planner.RouteNative, plan.Route) + require.Len(t, plan.Decisions, 2) + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Equal(t, router.DispositionRewriteRequired, st.Disposition) + assert.Empty(t, st.ExecSQL) +} + +func TestRouteInlineConstraintAddColumnFailsClosed(t *testing.T) { + plan := classify(t, "ALTER TABLE t ADD COLUMN c int UNIQUE") + require.Equal(t, planner.RouteNative, plan.Route) + + routed := router.Route([]planner.Plan{plan}) + st := routed.Statements[0] + assert.Equal(t, router.DispositionRewriteRequired, st.Disposition) + assert.Empty(t, st.ExecSQL, + "an inline constraint builds its index under ACCESS EXCLUSIVE; the submitted form must not run") +} + func TestRouteCopyAndSwapIsUnavailable(t *testing.T) { plan := classify(t, "ALTER TABLE t ALTER COLUMN id TYPE bigint") require.Equal(t, planner.RouteCopyAndSwap, plan.Route) @@ -72,19 +115,31 @@ func TestRouteRefusedStatementHasNoBackend(t *testing.T) { func TestRouteAggregateIsWorstDisposition(t *testing.T) { plans := []planner.Plan{ classify(t, "ALTER TABLE t ADD COLUMN age int"), + classify(t, "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)"), classify(t, "ALTER TABLE t ALTER COLUMN id TYPE bigint"), classify(t, "ALTER TABLE t ADD CONSTRAINT ex EXCLUDE USING gist (room WITH =)"), } routed := router.Route(plans) - require.Len(t, routed.Statements, 3) + require.Len(t, routed.Statements, 4) assert.Equal(t, router.DispositionExecute, routed.Statements[0].Disposition) - assert.Equal(t, router.DispositionUnavailable, routed.Statements[1].Disposition) - assert.Equal(t, router.DispositionRefuse, routed.Statements[2].Disposition) + assert.Equal(t, router.DispositionRewriteRequired, routed.Statements[1].Disposition) + assert.Equal(t, router.DispositionUnavailable, routed.Statements[2].Disposition) + assert.Equal(t, router.DispositionRefuse, routed.Statements[3].Disposition) assert.Equal(t, router.DispositionRefuse, routed.Disposition, "one refusal refuses the whole plan") } +func TestRouteAggregateRanksRewriteRequiredBelowUnavailable(t *testing.T) { + plans := []planner.Plan{ + classify(t, "ALTER TABLE t ATTACH PARTITION p FOR VALUES FROM (1) TO (10)"), + classify(t, "ALTER TABLE t ADD COLUMN age int"), + } + routed := router.Route(plans) + assert.Equal(t, router.DispositionRewriteRequired, routed.Disposition, + "one rewrite-required statement blocks the whole plan") +} + func TestRouteEmptyPlanExecutes(t *testing.T) { routed := router.Route(nil) assert.Empty(t, routed.Statements) diff --git a/pkg/statement/ops.go b/pkg/statement/ops.go index 682451b..f28a8b2 100644 --- a/pkg/statement/ops.go +++ b/pkg/statement/ops.go @@ -99,6 +99,15 @@ type Op struct { Unique bool // GeneratedStored is true for ADD COLUMN ... GENERATED ... STORED. GeneratedStored bool + // InlineConstraints are the table-constraint families an added column + // carries inline (UNIQUE, PRIMARY KEY, REFERENCES, CHECK) — each does + // the same index build or validation scan as its ADD CONSTRAINT form. + // An inline constraint the engine does not model is reported as + // ConstraintUnrecognized so the classifier can refuse it. + InlineConstraints []ConstraintKind + // PartitionOf is true for CREATE TABLE ... PARTITION OF, which locks + // the partitioned parent, not just the new relation. + PartitionOf bool // Default is the DEFAULT shape for OpAddColumn. Default DefaultKind // NewType is the target type for OpAlterColumnType and the column type @@ -118,6 +127,9 @@ type Op struct { func (o Op) Describe() string { switch o.Kind { case OpCreateTable: + if o.PartitionOf { + return "CREATE TABLE PARTITION OF" + } return "CREATE TABLE" case OpAddColumn: return "ADD COLUMN " + o.Column @@ -194,7 +206,10 @@ func ParseOps(sql string) ([]Op, error) { } return ops, nil case node.GetCreateStmt() != nil: - return []Op{{Kind: OpCreateTable}}, nil + return []Op{{ + Kind: OpCreateTable, + PartitionOf: node.GetCreateStmt().GetPartbound() != nil, + }}, nil case node.GetIndexStmt() != nil: idx := node.GetIndexStmt() return []Op{{ @@ -283,9 +298,13 @@ func alterTableOp(cmd *pganalyze.AlterTableCmd) Op { } // addColumnOp extracts the shape facts of an added column: its DEFAULT -// shape and whether it is a stored generated column. Identity and serial -// columns are reported as expression defaults — their values come from a -// sequence, which fast default cannot cover. +// shape, whether it is a stored generated column, and any inline table +// constraints it carries. Identity and serial columns are reported as +// expression defaults — their values come from a sequence, which fast +// default cannot cover. Nullability clauses and the deferrability +// attributes that modify a preceding FOREIGN KEY add no work of their own +// and are not reported; any constraint family the engine does not model +// is reported as ConstraintUnrecognized so the classifier fails closed. func addColumnOp(def *pganalyze.ColumnDef) Op { op := Op{Kind: OpAddColumn, Column: def.GetColname()} op.NewType, op.NewTypeMods = typeRef(def.GetTypeName()) @@ -305,6 +324,23 @@ func addColumnOp(def *pganalyze.ColumnDef) Op { op.Default = DefaultExpression case pganalyze.ConstrType_CONSTR_GENERATED: op.GeneratedStored = true + case pganalyze.ConstrType_CONSTR_NULL, + pganalyze.ConstrType_CONSTR_NOTNULL, + pganalyze.ConstrType_CONSTR_ATTR_DEFERRABLE, + pganalyze.ConstrType_CONSTR_ATTR_NOT_DEFERRABLE, + pganalyze.ConstrType_CONSTR_ATTR_DEFERRED, + pganalyze.ConstrType_CONSTR_ATTR_IMMEDIATE: + // No scan and no index build of their own. + case pganalyze.ConstrType_CONSTR_UNIQUE: + op.InlineConstraints = append(op.InlineConstraints, ConstraintUnique) + case pganalyze.ConstrType_CONSTR_PRIMARY: + op.InlineConstraints = append(op.InlineConstraints, ConstraintPrimaryKey) + case pganalyze.ConstrType_CONSTR_FOREIGN: + op.InlineConstraints = append(op.InlineConstraints, ConstraintForeignKey) + case pganalyze.ConstrType_CONSTR_CHECK: + op.InlineConstraints = append(op.InlineConstraints, ConstraintCheck) + default: + op.InlineConstraints = append(op.InlineConstraints, ConstraintUnrecognized) } } return op diff --git a/pkg/statement/ops_test.go b/pkg/statement/ops_test.go index 93b4ebd..cadba02 100644 --- a/pkg/statement/ops_test.go +++ b/pkg/statement/ops_test.go @@ -63,6 +63,43 @@ func TestParseOpsShapes(t *testing.T) { sql: "ALTER TABLE t ADD COLUMN total numeric GENERATED ALWAYS AS (price * qty) STORED", want: statement.Op{Kind: statement.OpAddColumn, Column: "total", NewType: "numeric", GeneratedStored: true}, }, + { + name: "add column inline unique", + sql: "ALTER TABLE t ADD COLUMN c int UNIQUE", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + InlineConstraints: []statement.ConstraintKind{statement.ConstraintUnique}}, + }, + { + name: "add column inline primary key", + sql: "ALTER TABLE t ADD COLUMN c int PRIMARY KEY", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + InlineConstraints: []statement.ConstraintKind{statement.ConstraintPrimaryKey}}, + }, + { + name: "add column inline foreign key", + sql: "ALTER TABLE t ADD COLUMN c int REFERENCES parent (id)", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + InlineConstraints: []statement.ConstraintKind{statement.ConstraintForeignKey}}, + }, + { + name: "add column inline check", + sql: "ALTER TABLE t ADD COLUMN c int CHECK (c > 0)", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + InlineConstraints: []statement.ConstraintKind{statement.ConstraintCheck}}, + }, + { + name: "add column inline check with constant default", + sql: "ALTER TABLE t ADD COLUMN c int DEFAULT 0 CHECK (c > 0)", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + Default: statement.DefaultConstant, + InlineConstraints: []statement.ConstraintKind{statement.ConstraintCheck}}, + }, + { + name: "add column not null carries no inline constraint", + sql: "ALTER TABLE t ADD COLUMN c int NOT NULL DEFAULT 0", + want: statement.Op{Kind: statement.OpAddColumn, Column: "c", NewType: "int4", + Default: statement.DefaultConstant}, + }, { name: "drop column", sql: "ALTER TABLE t DROP COLUMN age",