From 838bc2ad20e870ad8518864fe10d3aef9e365bd0 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 28 Oct 2026 14:19:35 +1100 Subject: [PATCH] planner: decide destructiveness for every operation shape explicitly Both destructive gates rest on destructiveOp with no core backstop, so a shape added to the parser must not default to non-destructive unexamined. --- docs/invariants.md | 5 ++- pkg/planner/destructive.go | 59 +++++++++++++++++++++++++++++++++ pkg/planner/destructive_test.go | 44 ++++++++++++++++++++++++ pkg/planner/planner.go | 19 ----------- 4 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 pkg/planner/destructive.go create mode 100644 pkg/planner/destructive_test.go diff --git a/docs/invariants.md b/docs/invariants.md index 63218c22..8cd6d91d 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -1406,7 +1406,10 @@ Each refusal is a preflight **error with a stated reason** — never a warning, *Enforced:* `pkg/migrate` (`run`'s destructive gate through `destructiveOperations` and `destructiveChangeVerdict`, `checkAllowDestructiveAck`, `AdmitDestructive` for the dry run, `auditAllowDestructive`; `ErrAllowDestructiveMismatch`), `RunDesired`'s admission - (`ErrAllowDestructiveNotSupported`), `pkg/planner` (`destructiveOp`), + (`ErrAllowDestructiveNotSupported`), `pkg/planner` (`destructiveOp`, which decides every + shape in `statement.OpKinds()` explicitly and treats an unclassified one as destructive; + `TestEveryOpKindIsClassifiedForDestructiveness`, + `TestAnUnclassifiedOpKindIsTreatedAsDestructive`), `TestRunRefusesAnUnacknowledgedDropColumn`, `TestRunRefusesAnUnacknowledgedDropConstraint`, `TestRunRefusesAnUnacknowledgedDropNotNull`, `TestRunExecutesAnAcknowledgedDropColumn`, `TestRunRejectsAMismatchedDestructiveAcknowledgement`, diff --git a/pkg/planner/destructive.go b/pkg/planner/destructive.go new file mode 100644 index 00000000..802b847d --- /dev/null +++ b/pkg/planner/destructive.go @@ -0,0 +1,59 @@ +package planner + +import "github.com/block/pg-sprite/pkg/statement" + +// destructiveOp reports whether an operation shape discards live +// structure. A drop is destructive regardless of how it routes: a dropped +// column discards data, a dropped constraint discards a guarantee the +// schema was providing — and dropping NOT NULL discards exactly the same +// guarantee its constraint form would, so it is marked identically — and a +// dropped index discards a structure that is expensive to rebuild (and, +// for a unique index, the uniqueness guarantee). Dropping a DEFAULT is +// deliberately not destructive: a default guarantees nothing about +// existing rows and is recreated by a metadata-only statement. +// +// Both front doors' destructive gates rest on this answer, and the core +// has no backstop for it (SAFETY.md), so a shape nobody has classified is +// treated as destructive: it is refused without the acknowledgement rather +// than run unguarded. +func destructiveOp(kind statement.OpKind) bool { + destructive, _ := classifyDestructive(kind) + return destructive +} + +// classifyDestructive is the explicit decision for every shape in +// statement.OpKinds(). classified is false only for a shape this switch +// does not name, which TestEveryOpKindIsClassifiedForDestructiveness +// rejects, so a shape added to the parser cannot ship without a decision. +func classifyDestructive(kind statement.OpKind) (destructive, classified bool) { + switch kind { + case statement.OpDropColumn, + statement.OpDropConstraint, + statement.OpDropIndex, + statement.OpDropNotNull: + return true, true + case statement.OpUnrecognized, + statement.OpCreateTable, + statement.OpAddColumn, + statement.OpAlterColumnType, + statement.OpSetDefault, + statement.OpDropDefault, + statement.OpSetNotNull, + statement.OpSetColumnOptions, + statement.OpRenameColumn, + statement.OpRenameTable, + statement.OpRenameIndex, + statement.OpSetSchema, + statement.OpSetTablespace, + statement.OpSetRelOptions, + statement.OpAddConstraint, + statement.OpValidateConstraint, + statement.OpAttachPartition, + statement.OpDetachPartition, + statement.OpCreateIndex, + statement.OpReindex: + return false, true + default: + return true, false + } +} diff --git a/pkg/planner/destructive_test.go b/pkg/planner/destructive_test.go new file mode 100644 index 00000000..05d7a313 --- /dev/null +++ b/pkg/planner/destructive_test.go @@ -0,0 +1,44 @@ +package planner + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/statement" +) + +// Every operation shape carries an explicit destructiveness decision, and +// exactly the four drops are destructive. A shape added to +// statement.OpKinds() without a case in classifyDestructive fails here +// instead of reaching the destructive gates with an unexamined answer. +func TestEveryOpKindIsClassifiedForDestructiveness(t *testing.T) { + kinds := statement.OpKinds() + require.NotEmpty(t, kinds) + + var destructive []statement.OpKind + for _, k := range kinds { + d, classified := classifyDestructive(k) + require.True(t, classified, "op kind %d (%q) has no destructiveness decision", + k, statement.Op{Kind: k}.Describe()) + if d { + destructive = append(destructive, k) + } + } + assert.ElementsMatch(t, []statement.OpKind{ + statement.OpDropColumn, + statement.OpDropConstraint, + statement.OpDropIndex, + statement.OpDropNotNull, + }, destructive) +} + +// A shape outside the classified set is treated as destructive, so the +// gates refuse it without the acknowledgement rather than let it run. +func TestAnUnclassifiedOpKindIsTreatedAsDestructive(t *testing.T) { + unclassified := statement.OpKind(len(statement.OpKinds())) + _, classified := classifyDestructive(unclassified) + require.False(t, classified, "the probe must be outside the classified set") + assert.True(t, destructiveOp(unclassified)) +} diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 99eeec33..d37f1561 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -558,25 +558,6 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string return d } -// destructiveOp reports whether an operation shape discards live -// structure. A drop is destructive regardless of how it routes: a dropped -// column discards data, a dropped constraint discards a guarantee the -// schema was providing — and dropping NOT NULL discards exactly the same -// guarantee its constraint form would, so it is marked identically — and a -// dropped index discards a structure that is expensive to rebuild (and, -// for a unique index, the uniqueness guarantee). Dropping a DEFAULT is -// deliberately not destructive: a default guarantees nothing about -// existing rows and is recreated by a metadata-only statement. -func destructiveOp(kind statement.OpKind) bool { - switch kind { - case statement.OpDropColumn, statement.OpDropConstraint, statement.OpDropIndex, - statement.OpDropNotNull: - return true - default: - return false - } -} - // 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. The rewrite trades the