diff --git a/docs/plan-report.md b/docs/plan-report.md index 31c0f5d..4b358cf 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -64,7 +64,7 @@ consumer rendering either into a shared surface must clamp and escape them. | `disposition` | string | always | What execution would do with this statement now (see Dispositions). | | `reason` | string | refusals only | Typed refusal cause for this statement: `unsupported-statement` for a planner-level refusal, `unsupported-partitioned-parent` when target facts refuse it. An unknown value must be treated as refused. | | `decisions` | array | always | The planner's per-operation classifications (below). | -| `exec_sql` | array | native route | The ordered SQL the native backend would run — the safer sequence when the planner constructed one. Absent for non-native routes. | +| `exec_sql` | array | native route | The ordered SQL the native backend would run — the safer sequence when the planner constructed one, or the statement as written for a table that does not exist yet (the greenfield create path runs plain builds; see Fingerprint). Absent for non-native routes. | | `execution` | string | with `exec_sql` | The typed execution contract for `exec_sql` (see Execution contracts). A consumer that runs the statements itself branches on this — it is what says the steps must not be wrapped in a transaction block. Present exactly when `exec_sql` is. | | `guidance` | string | `rewrite-required` only | The typed manual path for a rewrite-required refusal, drawn from the [suggest report's Guidance vocabulary](suggest-report.md#guidance-guidance). The engine will not run the statement; this names what to do instead (`split-statement`, `add-column-then-constraint`, …). Present exactly when `disposition` is `rewrite-required`. Explanatory: excluded from the fingerprint. | @@ -101,11 +101,11 @@ consumer rendering either into a shared surface must clamp and escape them. | Value | Meaning | |---|---| -| `metadata-only` | A brief ACCESS EXCLUSIVE catalog change, no scan and no rewrite. | +| `metadata-only` | A brief ACCESS EXCLUSIVE catalog change, no scan and no rewrite. Also the classification of every executable build on a table that does not exist yet (a `diff` greenfield plan): an index on an empty table nobody reads has no rows to scan and no readers to lock out, so a `safer-idiom` decision is reclassified here and `exec_sql` is the statement as written. | | `online-idiom` | Already the safe native form (CONCURRENTLY, NOT VALID, VALIDATE, USING INDEX). | | `fast-default` | ADD COLUMN with a constant default — the catalog stores the default, no rewrite. | | `binary-coercible` | A type change PostgreSQL relabels without a rewrite (widen varchar, varchar to text, widen numeric precision). | -| `safer-idiom` | Native, but the submitted form blocks; `safer_sql` carries the online rewrite when one can be constructed. | +| `safer-idiom` | Native, but the submitted form blocks; `safer_sql` carries the online rewrite when one can be constructed. Never appears on an executable statement of a greenfield plan — see `metadata-only`. | | `app-breaking-rename` | A column or table rename — metadata-only for PostgreSQL, but running application code still referencing the old name breaks the instant it commits. For a column the safe sequence is expand/contract: add the new column, dual-write and backfill, switch reads, then drop the old column as its own reviewed change. For a table, coordinate the rename with the application deploy that adopts the new name. Index renames stay `metadata-only` — SQL never references an index by name. | | `volatile-default` | ADD COLUMN whose default the planner cannot prove constant — PostgreSQL rewrites the table. | | `generated-stored` | Adding a stored generated column computes every row — a full rewrite. | @@ -199,6 +199,19 @@ separator (`0x1E`). Explanatory fields (`decisions`, `kind`, `destructive`) are reworded reason does not change identity, but a rerouted, resequenced, or rewritten plan does. An empty plan has a defined identity (the digest of no input). +For a table that does not exist yet, `exec_sql` is the plain canonical build that the +greenfield create path runs. It never substitutes `CONCURRENTLY` for an index on a table +born in that run, and the fingerprint therefore commits to the plain build. + +Upgrade caveat: earlier `format_version` 2 reports disclosed the planner's online rewrite +(`CREATE INDEX CONCURRENTLY …`) as the greenfield `exec_sql` — a form the create path +refuses and never ran. Correcting the disclosure changed the greenfield fingerprint's +*value* without changing its *definition* (the serialization above is unchanged, so the +version is not bumped): the fingerprint now commits to what actually runs. A consumer +holding a greenfield fingerprint from an earlier report gets one +`plan-fingerprint-mismatch` on its next apply; re-plan and pin the new value. Fingerprints +for tables that already exist are unaffected. + This is a **plan identity, not a schema fingerprint**. The engine's schema-state comparisons only ever compare server-decompiled output against server-decompiled output (see `pkg/statement`); the plan fingerprint never participates in them. diff --git a/docs/postgres-online-ddl-reference.md b/docs/postgres-online-ddl-reference.md index bde4e72..13e8ed5 100644 --- a/docs/postgres-online-ddl-reference.md +++ b/docs/postgres-online-ddl-reference.md @@ -298,6 +298,12 @@ The operation is a brief catalog-only change: it takes at most a short written on any table size, provided the lock can be acquired promptly (pg-sprite runs every session under a bounded `lock_timeout`). +Every executable build on a table that does not exist yet — the `diff` +greenfield case — classifies here too, including index builds that would be +[`safer-idiom`](#safer-idiom) on a live table: an index on an empty table +nobody reads has no rows to scan and no readers to lock out, so the create +path runs the statement as written. + `ACCESS EXCLUSIVE` is the bucket's worst case, not its uniform cost — several forms take only `SHARE UPDATE EXCLUSIVE`, which does not block reads or writes, only competing DDL and vacuum: @@ -370,6 +376,11 @@ block. When no online sequence can be constructed (for example the classification stays `safer-idiom` but the statement is refused with [`rewrite-required`](#rewrite-required) and exits 2. +On a table that does not exist yet (the `diff` greenfield case) there is no +live traffic to protect, so the build is reclassified +[`metadata-only`](#metadata-only) and runs as written — the greenfield create +path does not substitute `CONCURRENTLY`. + ### `app-breaking-rename` | Verdict | Lock | Scan / rewrite | Dry-run exit | diff --git a/docs/safer-sequences.md b/docs/safer-sequences.md index 6f7d2c3..8e3509d 100644 --- a/docs/safer-sequences.md +++ b/docs/safer-sequences.md @@ -103,7 +103,7 @@ the tool itself (see below): | `ADD UNIQUE` (direct) | 2 steps: `CREATE UNIQUE INDEX CONCURRENTLY` → `ADD CONSTRAINT … USING INDEX` (this page's example) | | `ADD PRIMARY KEY` (direct) | The same 2 steps — **on a column already `NOT NULL`**. On a nullable column, adopting the index must also set `NOT NULL`, and PostgreSQL validates that by scanning the heap under `ACCESS EXCLUSIVE` — the blocking work the substitution exists to avoid, and a scan the brief step budget cancels. Reach `NOT NULL` first via the 4-step sequence above, then add the primary key | | `ADD CHECK` / `ADD FOREIGN KEY` (direct) | 2 steps: `ADD CONSTRAINT … NOT VALID` → `VALIDATE CONSTRAINT` — the validation scan runs under a lock that lets reads and writes proceed | -| `CREATE INDEX` (non-concurrent) | 1 statement: the same build with `CONCURRENTLY` | +| `CREATE INDEX` (non-concurrent) | 1 statement: the same build with `CONCURRENTLY` — on a live table only; an index on a table that does not exist yet (`diff` greenfield) is `metadata-only` and runs as written | | `DETACH PARTITION` (non-concurrent) | 1 statement: `DETACH PARTITION … CONCURRENTLY` — shown by `--dry-run` and `suggest`, but **execution refuses this step today**: a cancelled concurrent detach leaves a detach-pending partition state the executor does not own recovering | Statements already in the safe form (`… USING INDEX`, `… NOT VALID`, diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 48e6403..e4da940 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -169,6 +169,11 @@ The greenfield `CREATE TABLE` path is a fixed call order, all inside the apply s 3. `preflight.CheckCreatePrivileges` — mint the `CreationRole` proof for the target schema. 4. `executor.ExecuteCreate` — consume both proofs and run the set. +For the shapes the create path admits, the plan's `exec_sql` is the statement as written — +the plain `CREATE TABLE` and index builds this path runs, with each build's decision +reclassified `metadata-only`; it never substitutes `CONCURRENTLY` for an index on the table +born in that run. + `migrate.RunDesired` runs this sequence itself when the plan is greenfield — the adapter does not assemble it and must not mint either proof separately (a proof minted outside the executing session proves nothing about it). The order decides which refusal wins when both diff --git a/internal/cli/diff_text_test.go b/internal/cli/diff_text_test.go index 45e572e..9442cb7 100644 --- a/internal/cli/diff_text_test.go +++ b/internal/cli/diff_text_test.go @@ -10,6 +10,7 @@ import ( "github.com/block/pg-sprite/pkg/plan" "github.com/block/pg-sprite/pkg/planner" "github.com/block/pg-sprite/pkg/router" + "github.com/block/pg-sprite/pkg/schemadiff" ) // The text rendering is this renderer's own unit test: the layout below is @@ -108,6 +109,43 @@ func TestDiffTextGreenfieldLeadsWithNote(t *testing.T) { assert.NotContains(t, text, "error[table-not-found]") } +// A greenfield index build renders as the metadata-only note the planner +// already gives the CREATE TABLE — never as the safer-idiom warning that +// would tell the reader a table nobody reads yet is about to be locked. +func TestDiffTextGreenfieldIndexRendersMetadataOnly(t *testing.T) { + report := plan.NewReport(plan.SourceDiff) + report.Schema, report.Table, report.ServerVersion = "public", "widgets", "16.10" + report.Disposition = router.DispositionExecute + missing := false + report.TableExists = &missing + sql := `CREATE INDEX "widgets_name_idx" ON "public"."widgets" USING btree ("name")` + report.Statements = append(report.Statements, plan.Statement{ + SQL: sql, + Kind: schemadiff.ChangeCreateIndex, + Route: planner.RouteNative, + Backend: router.BackendNative, + Disposition: router.DispositionExecute, + Decisions: []planner.Decision{{ + Operation: "create index", + Route: planner.RouteNative, + Reason: planner.ReasonMetadataOnly, + }}, + ExecSQL: []string{sql}, + Execution: planner.ExecutionAutocommit, + }) + + var out strings.Builder + require.NoError(t, writeDiffText(&out, palette{}, report)) + text := out.String() + assert.Contains(t, text, "statement 1:\n "+sql+";\n") + assert.Contains(t, text, "note[metadata-only]:\n create index — a brief catalog-only change") + assert.Contains(t, text, "note:\n runs as written\n") + assert.Contains(t, text, "docs:\n "+onlineDDLReferenceURL+"#metadata-only\n") + assert.NotContains(t, text, "warning[safer-idiom]") + assert.NotContains(t, text, "#safer-idiom") + assert.NotContains(t, text, "safer online sequence") +} + // A converged table renders a single plan entry — no statements, no // execution pointers. func TestDiffTextNoChanges(t *testing.T) { diff --git a/pkg/diffplan/diffplan.go b/pkg/diffplan/diffplan.go index 4739f84..eca60ec 100644 --- a/pkg/diffplan/diffplan.go +++ b/pkg/diffplan/diffplan.go @@ -99,7 +99,9 @@ func Plan(ctx context.Context, pool *pgxpool.Pool, req Request) (plan.Report, er if report.Statements, report.Disposition, err = classifyChanges(changes, facts); err != nil { return plan.Report{}, err } - if tableExists { + if !tableExists { + plan.DiscloseGreenfieldExecution(&report) + } else { targetFacts, checkErr := preflight.LookupTargetFacts(ctx, pool, req.Schema, ds.Table()) if checkErr != nil { return plan.Report{}, checkErr diff --git a/pkg/diffplan/diffplan_integration_test.go b/pkg/diffplan/diffplan_integration_test.go index 7ef7b2d..bdec9a9 100644 --- a/pkg/diffplan/diffplan_integration_test.go +++ b/pkg/diffplan/diffplan_integration_test.go @@ -229,6 +229,7 @@ func TestPlanMissingTableEmitsFullDesiredSchema(t *testing.T) { schemadiff.ChangeCreateTable, schemadiff.ChangeCreateIndex, }, kinds) + assertGreenfieldIndexExecution(t, report.Statements[1]) } // A greenfield plan must state execution order even when the desired file @@ -265,4 +266,18 @@ func TestPlanMissingTableOrdersCreateTableFirst(t *testing.T) { schemadiff.ChangeCreateTable, schemadiff.ChangeCreateIndex, }, kinds) + assertGreenfieldIndexExecution(t, report.Statements[1]) +} + +func assertGreenfieldIndexExecution(t *testing.T, got plan.Statement) { + t.Helper() + require.Equal(t, []string{got.SQL}, got.ExecSQL) + assert.NotContains(t, got.ExecSQL[0], "CONCURRENTLY") + assert.Equal(t, planner.ExecutionAutocommit, got.Execution) + require.NotEmpty(t, got.Decisions) + for _, decision := range got.Decisions { + assert.Equal(t, planner.ReasonMetadataOnly, decision.Reason) + assert.Empty(t, decision.SaferSQL) + assert.Empty(t, decision.SaferSQLExecution) + } } diff --git a/pkg/plan/plan.go b/pkg/plan/plan.go index 148237d..98d3ddd 100644 --- a/pkg/plan/plan.go +++ b/pkg/plan/plan.go @@ -151,10 +151,7 @@ func RefuseUnsupportedPartitionedParent(report *Report, refused []bool) { report.Statements[i].Reason = verdict.ReasonUnsupportedPartitionedParent report.Statements[i].ExecSQL = nil report.Statements[i].Execution = "" - for j := range report.Statements[i].Decisions { - report.Statements[i].Decisions[j].SaferSQL = nil - report.Statements[i].Decisions[j].SaferSQLExecution = "" - } + withdrawSaferAdvice(&report.Statements[i]) } if any { report.Disposition = router.DispositionRefuse @@ -162,6 +159,44 @@ func RefuseUnsupportedPartitionedParent(report *Report, refused []bool) { } } +// DiscloseGreenfieldExecution makes executable statements describe the plain, +// bounded builds used for a table born in this run. There is no live traffic +// to protect, so the create path does not substitute the planner's online +// idioms; each safer-idiom decision is reclassified metadata-only — the same +// reason the planner gives the CREATE TABLE itself, because an index built +// on an empty table nobody reads yet has no rows to scan and no readers to +// lock out. Reclassifying keeps the statement in a state router.Route can +// produce: an execute disposition never carries a safer-idiom decision +// without its rewrite. +func DiscloseGreenfieldExecution(report *Report) { + for i := range report.Statements { + st := &report.Statements[i] + if st.Disposition != router.DispositionExecute { + continue + } + st.ExecSQL = []string{st.SQL} + st.Execution = planner.ExecutionAutocommit + withdrawSaferAdvice(st) + for j := range st.Decisions { + if st.Decisions[j].Reason == planner.ReasonSaferIdiom { + st.Decisions[j].Reason = planner.ReasonMetadataOnly + } + } + } +} + +// withdrawSaferAdvice clears the planner's per-decision online rewrite from +// a statement whose execution advice no longer applies — a refusal, or a +// greenfield build that runs as written. Every mutator that withdraws +// execution advice goes through here, so a decision field added later is +// cleared in one place. +func withdrawSaferAdvice(st *Statement) { + for j := range st.Decisions { + st.Decisions[j].SaferSQL = nil + st.Decisions[j].SaferSQLExecution = "" + } +} + // NewReport returns an empty report for source with the contract version // stamped and Statements non-nil, so an empty plan serializes as [] rather // than null. diff --git a/pkg/plan/plan_test.go b/pkg/plan/plan_test.go index 985c134..ef57660 100644 --- a/pkg/plan/plan_test.go +++ b/pkg/plan/plan_test.go @@ -177,6 +177,75 @@ func TestRefuseUnsupportedPartitionedParentWithdrawsExecutionAdvice(t *testing.T assert.Empty(t, r.Statements[0].Decisions[0].SaferSQLExecution) } +func TestDiscloseGreenfieldExecutionUsesPlainSQLForExecutableStatements(t *testing.T) { + concurrent := "CREATE INDEX CONCURRENTLY i ON s.t (c)" + saferIdiom := func() []planner.Decision { + return []planner.Decision{{ + Route: planner.RouteNative, + Reason: planner.ReasonSaferIdiom, + SaferSQL: []string{concurrent}, + SaferSQLExecution: planner.ExecutionAutocommit, + }} + } + r := plan.Report{Statements: []plan.Statement{ + { + SQL: "CREATE INDEX i ON s.t USING btree (c)", + Disposition: router.DispositionExecute, + ExecSQL: []string{concurrent}, + Execution: planner.ExecutionAutocommit, + Decisions: saferIdiom(), + }, + { + SQL: "ALTER TABLE s.t SET UNLOGGED", + Disposition: router.DispositionRefuse, + ExecSQL: []string{concurrent}, + Execution: planner.ExecutionAutocommit, + Decisions: saferIdiom(), + }, + { + SQL: "ALTER TABLE s.t ADD COLUMN nickname text UNIQUE", + Disposition: router.DispositionRewriteRequired, + Decisions: saferIdiom(), + }, + { + SQL: "ALTER TABLE s.t ALTER COLUMN c TYPE bigint", + Disposition: router.DispositionUnavailable, + Decisions: saferIdiom(), + }, + { + SQL: "CREATE TABLE s.t (c int)", + Disposition: router.DispositionExecute, + ExecSQL: []string{"CREATE TABLE s.t (c int)"}, + Execution: planner.ExecutionAutocommit, + Decisions: []planner.Decision{{ + Route: planner.RouteNative, + Reason: planner.ReasonMetadataOnly, + }}, + }, + }} + + plan.DiscloseGreenfieldExecution(&r) + + executable := r.Statements[0] + assert.Equal(t, []string{executable.SQL}, executable.ExecSQL) + assert.Equal(t, planner.ExecutionAutocommit, executable.Execution) + assert.Equal(t, planner.ReasonMetadataOnly, executable.Decisions[0].Reason, + "a safer-idiom build on a table born in this run is reclassified metadata-only") + assert.Empty(t, executable.Decisions[0].SaferSQL) + assert.Empty(t, executable.Decisions[0].SaferSQLExecution) + + for _, untouched := range r.Statements[1:4] { + assert.Equal(t, saferIdiom(), untouched.Decisions, untouched.Disposition) + } + assert.Equal(t, []string{concurrent}, r.Statements[1].ExecSQL) + assert.Equal(t, planner.ExecutionAutocommit, r.Statements[1].Execution) + + createTable := r.Statements[4] + assert.Equal(t, []string{createTable.SQL}, createTable.ExecSQL) + assert.Equal(t, planner.ExecutionAutocommit, createTable.Execution) + assert.Equal(t, planner.ReasonMetadataOnly, createTable.Decisions[0].Reason) +} + // The JSON shape is the adapter-facing contract: exact keys, exact // omissions. A consumer pins format_version 2 against this test. func TestReportJSONShape(t *testing.T) {