From d7b7ee8626876dcc60355006bc9ea072dc61535d Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 7 Sep 2026 16:33:40 +1000 Subject: [PATCH 1/2] build(deps): pin pg-sprite to released v0.3.1 Adopts the invalid-index outcome split shipped in v0.3.0: pre-existing is replaced by abandoned, build-in-flight, builder-unobservable, other-table and not-droppable verdicts, and the two the executor marks permanent are routed to a refusal instead of an operational retry. v0.3.1 adds the typed create-shape causes and owned-relation read-back the PostgreSQL engine adopts in follow-on changes. --- e2e/consumermodule/go.mod | 2 +- e2e/consumermodule/go.sum | 4 +- go.mod | 2 +- go.sum | 4 +- pkg/engine/postgres/apply.go | 107 ++++++++++++------ pkg/engine/postgres/apply_test.go | 92 +++++++++++++-- .../postgres/postgres_integration_test.go | 50 ++++++-- 7 files changed, 205 insertions(+), 56 deletions(-) diff --git a/e2e/consumermodule/go.mod b/e2e/consumermodule/go.mod index 23e55f92f..b7fbc1a53 100644 --- a/e2e/consumermodule/go.mod +++ b/e2e/consumermodule/go.mod @@ -37,7 +37,7 @@ require ( github.com/aws/smithy-go v1.27.7 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6 // indirect - github.com/block/pg-sprite v0.2.0 // indirect + github.com/block/pg-sprite v0.3.1 // indirect github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c // indirect github.com/bradleyfalzon/ghinstallation/v2 v2.18.0 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect diff --git a/e2e/consumermodule/go.sum b/e2e/consumermodule/go.sum index 4c4ad53a4..aca876a09 100644 --- a/e2e/consumermodule/go.sum +++ b/e2e/consumermodule/go.sum @@ -50,8 +50,8 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6 h1:GvubwsqXHanJkhBotCs4XdEmSnwzhHQe7DVGrn+NFok= github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6/go.mod h1:KEo73lbxXs9cFlq+x3Z35UqGg3MTxAPfjDOR/ob/iik= -github.com/block/pg-sprite v0.2.0 h1:H6w/MNJf1rc7XtdVEI0Sq63I2+MkiifPgkS3qfZ9Rz8= -github.com/block/pg-sprite v0.2.0/go.mod h1:vZxHdTMrCOPAYgswveB7PSjOaOuRgnDLGRw6WoOizRg= +github.com/block/pg-sprite v0.3.1 h1:l2w3aAFql+RvKnXoE7aHh1xx0Umj6pRsnjhsSUeEB0g= +github.com/block/pg-sprite v0.3.1/go.mod h1:vZxHdTMrCOPAYgswveB7PSjOaOuRgnDLGRw6WoOizRg= github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c h1:Gdd1vWs0UKLlvq84+4wMveyls7QkFqhQDpF21KR2cIA= github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c/go.mod h1:Lg97/e4zr2X3AQXRUrDVAQZOVqFDZ09px503h4v/Yss= github.com/block/vitess v0.0.0-20260907005807-88d15fda31ea h1:t9VROoN/aCzwwh7Ap90TLijZz3rZHZ6HUgWymC+yVS8= diff --git a/go.mod b/go.mod index 2e5a7ae39..1076a2a84 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.44.5 github.com/aws/aws-sdk-go-v2/service/sts v1.43.3 github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6 - github.com/block/pg-sprite v0.2.0 + github.com/block/pg-sprite v0.3.1 github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c github.com/bradleyfalzon/ghinstallation/v2 v2.18.0 github.com/charmbracelet/bubbles v1.0.0 diff --git a/go.sum b/go.sum index c34e9eb82..7432a93ca 100644 --- a/go.sum +++ b/go.sum @@ -117,8 +117,8 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6 h1:GvubwsqXHanJkhBotCs4XdEmSnwzhHQe7DVGrn+NFok= github.com/block/mysql v0.0.0-20260906224346-ee0a93fe50d6/go.mod h1:KEo73lbxXs9cFlq+x3Z35UqGg3MTxAPfjDOR/ob/iik= -github.com/block/pg-sprite v0.2.0 h1:H6w/MNJf1rc7XtdVEI0Sq63I2+MkiifPgkS3qfZ9Rz8= -github.com/block/pg-sprite v0.2.0/go.mod h1:vZxHdTMrCOPAYgswveB7PSjOaOuRgnDLGRw6WoOizRg= +github.com/block/pg-sprite v0.3.1 h1:l2w3aAFql+RvKnXoE7aHh1xx0Umj6pRsnjhsSUeEB0g= +github.com/block/pg-sprite v0.3.1/go.mod h1:vZxHdTMrCOPAYgswveB7PSjOaOuRgnDLGRw6WoOizRg= github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c h1:Gdd1vWs0UKLlvq84+4wMveyls7QkFqhQDpF21KR2cIA= github.com/block/spirit v0.17.1-0.20260907005557-10804bbe247c/go.mod h1:Lg97/e4zr2X3AQXRUrDVAQZOVqFDZ09px503h4v/Yss= github.com/block/vitess v0.0.0-20260907005807-88d15fda31ea h1:t9VROoN/aCzwwh7Ap90TLijZz3rZHZ6HUgWymC+yVS8= diff --git a/pkg/engine/postgres/apply.go b/pkg/engine/postgres/apply.go index d6940257b..59c376a65 100644 --- a/pkg/engine/postgres/apply.go +++ b/pkg/engine/postgres/apply.go @@ -177,17 +177,21 @@ func (e *Engine) runOptimisticApply(ctx context.Context, conn targetConn, change } var invalidErr *executor.InvalidIndexError - if errors.As(err, &invalidErr) { - // An invalid index — pre-existing or a build's own unrecovered - // leftover — is operational: an operator clears it and a retry can - // succeed. Checked before the refusal and budget arms because the - // verdict wraps the build failure that produced it (a budget- - // cancelled build leaves its own invalid index), and that inner - // cause must not be read as the outcome — the index the operator - // clears is. The detail is built from the typed identifiers and - // verdict code, never the wrapped build or cleanup errors, which - // may carry raw server text; the full cause lands in the server - // log below it. + if errors.As(err, &invalidErr) && !invalidErr.Code().Permanent() { + // An invalid index an operator can clear — a build's own leftover, + // abandoned debris, another backend's build to wait out, or a + // builder this role cannot observe — is operational: once it is + // cleared a retry can succeed. Checked before the refusal and budget + // arms because the verdict wraps the build failure that produced it + // (a budget-cancelled build leaves its own invalid index), and that + // inner cause must not be read as the outcome — the index the + // operator clears is. The permanent verdicts (the name is occupied + // on another table, or by an index the server will not drop + // concurrently) fall through to classifyRefusal: retrying unchanged + // reproduces them. The detail is built from the typed identifiers + // and verdict code, never the wrapped build or cleanup errors, which + // may carry raw server text; the full cause lands in the server log + // below it. logger.Error("PostgreSQL concurrent index build left or found an invalid index", "namespace", change.namespace, "table", change.table, "index_schema", invalidErr.Schema, "index", invalidErr.Index, "error", err) @@ -256,10 +260,13 @@ func classifyApplyFailure(err error, table string) applyFailure { // invalidIndexDetail renders the operator-facing next step for an // invalid-index verdict, matching pg-sprite's own ownership standard: a drop -// is named only when the entry is proven this build's own leftover. A -// pre-existing invalid entry may be another actor's still-running build, and -// an unproven verdict may sit on a healthy index — both get investigation -// steps, never a statement to run. Only the typed identifiers are +// is named only where the executor proved the entry is a failed build's +// debris on the target table — this build's own leftover, or an abandoned +// entry with no builder. A build still in flight says wait; an entry on +// another table, one the server will not drop concurrently, one whose +// builder this role cannot see, and an unproven verdict get investigation +// steps, never a statement to run — the index under the name may be healthy +// or may be exactly what it is meant to be. Only the typed identifiers are // interpolated, never the wrapped build or cleanup errors, which may carry // raw server text. func invalidIndexDetail(invalidErr *executor.InvalidIndexError) string { @@ -268,8 +275,16 @@ func invalidIndexDetail(invalidErr *executor.InvalidIndexError) string { switch invalidErr.Code() { case executor.CodeInvalidIndexOwnLeftover: advice = fmt.Sprintf("this build left its own invalid index %s on the target; drop the invalid index, then retry", name) - case executor.CodeInvalidIndexPreexisting: - advice = fmt.Sprintf("an invalid index %s already occupies the name on the target and may be another actor's build still in progress; check pg_stat_activity before any recovery, then retry", name) + case executor.CodeInvalidIndexAbandoned: + advice = fmt.Sprintf("an abandoned invalid index %s occupies the name on the target table with no backend building it; confirm it is still invalid with no builder, drop the invalid index, then retry", name) + case executor.CodeInvalidIndexBuildInFlight: + advice = fmt.Sprintf("an invalid index %s occupies the name and backend %d is still building it; wait for that build to finish or fail, then retry", name, invalidErr.BuilderPID) + case executor.CodeInvalidIndexBuilderUnobservable: + advice = fmt.Sprintf("an invalid index %s occupies the name on the target table and the engine role cannot observe whether a backend is building it; check pg_stat_progress_create_index with a role granted pg_read_all_stats before any recovery, then retry", name) + case executor.CodeInvalidIndexOtherTable: + advice = fmt.Sprintf("an invalid index %s already occupies the name on a different table%s; this change cannot claim it — rename the index in the schema file and re-plan, or clear the entry through that table's own change", name, invalidIndexTableSuffix(invalidErr)) + case executor.CodeInvalidIndexNotDroppable: + advice = fmt.Sprintf("an invalid index %s occupies the name and is a partitioned table's index, an index partition, or a constraint's index rather than a failed build's leftover; an operator must resolve it on the target, or rename the index in the schema file and re-plan", name) default: // CodeInvalidIndexUnproven and any future verdict fail safe with // investigation steps: the index under the name may be healthy. @@ -278,6 +293,16 @@ func invalidIndexDetail(invalidErr *executor.InvalidIndexError) string { return sanitizeReasonText(advice) } +// invalidIndexTableSuffix names the table the invalid index sits on when the +// catalog inspection saw it; the verdict carries no table when the state +// could not be inspected. +func invalidIndexTableSuffix(invalidErr *executor.InvalidIndexError) string { + if invalidErr.Table == "" { + return "" + } + return fmt.Sprintf(" (%q)", invalidErr.Table) +} + // refusal is a typed apply outcome that retrying cannot fix: the schema // change, the target table, or role provisioning must change first. type refusal struct { @@ -349,15 +374,21 @@ func refusalForCause(err error, table string) *refusal { } return &refusal{reason: "insufficient-privileges", detail: detail} } - // An invalid-index verdict is operational even when the build failure it - // wraps would classify as a refusal on its own — a budget-cancelled - // concurrent build leaves its own invalid index, and the index the - // operator clears is the outcome, not the inner budget exhaustion. - // Declined before the budget arm so the nested cause can never shadow - // the verdict. + // An invalid-index verdict is decided by its own code, never by the build + // failure it wraps — a budget-cancelled concurrent build leaves its own + // invalid index, and the index the operator clears is the outcome, not + // the inner budget exhaustion. Decided before the budget arm so the + // nested cause can never shadow the verdict. Only the permanent members + // of the family refuse: the name is occupied on another table, or by an + // index the server will not drop concurrently, so retrying unchanged + // reproduces the verdict. Every other member is operational. var invalidErr *executor.InvalidIndexError if errors.As(err, &invalidErr) { - return nil + r, _ := refusalForOutcome(invalidErr.Code(), table) + if r != nil { + r.detail = invalidIndexDetail(invalidErr) + } + return r } var budgetErr *executor.BudgetError if errors.As(err, &budgetErr) && budgetErr.Cause == executor.CauseStatement { @@ -444,14 +475,26 @@ func refusalForOutcome(code executor.Code, table string) (*refusal, bool) { // apply fails closed until an operator has inspected the target. return &refusal{reason: "engine-invariant-violation", detail: fmt.Sprintf("the engine's safety invariants did not hold while changing table %q; inspect the target and server logs before re-running", table)}, true - case executor.CodeBudgetLockExceeded, executor.CodeCancelledExternally, - executor.CodeInvalidIndexOwnLeftover, executor.CodeInvalidIndexPreexisting, - executor.CodeInvalidIndexUnproven, executor.CodePoolTooSmall, - executor.CodeExecutionFailed: - // Operational outcomes: a bounded lock race, an external stop, an - // invalid-index state an operator clears, engine pool sizing, or a - // failure outside the typed set. A retry can succeed once - // conditions change, so none is a permanent refusal. + case executor.CodeInvalidIndexOtherTable, executor.CodeInvalidIndexNotDroppable: + // The permanent members of the invalid-index family: the requested + // name is held by an entry this change can never clear — an invalid + // index on a different table, or one the server will not drop + // concurrently (a partitioned table's index, an index partition, a + // constraint's index). Retrying unchanged reproduces the verdict. + // The typed-verdict path replaces this detail with the code's own + // advice; this mapping keeps the vocabulary total. + return &refusal{reason: "invalid-index-occupied", + detail: fmt.Sprintf("an invalid index already occupies a name the change to %q needs and is not a failed build's leftover; rename the index in the schema file and re-plan, or resolve the entry on the target", table)}, true + case executor.CodeBudgetLockExceeded, executor.CodeCancelledByCaller, + executor.CodeCancelledExternally, executor.CodeInvalidIndexOwnLeftover, + executor.CodeInvalidIndexAbandoned, executor.CodeInvalidIndexBuildInFlight, + executor.CodeInvalidIndexBuilderUnobservable, executor.CodeInvalidIndexUnproven, + executor.CodePoolTooSmall, executor.CodeExecutionFailed: + // Operational outcomes: a bounded lock race, the caller's own + // context ending or an external stop, an invalid-index state an + // operator clears or waits out, engine pool sizing, or a failure + // outside the typed set. A retry can succeed once conditions + // change, so none is a permanent refusal. return nil, true } return nil, false diff --git a/pkg/engine/postgres/apply_test.go b/pkg/engine/postgres/apply_test.go index cbf0177e1..04bed0a2e 100644 --- a/pkg/engine/postgres/apply_test.go +++ b/pkg/engine/postgres/apply_test.go @@ -116,6 +116,42 @@ func TestClassifyRefusal(t *testing.T) { name: "external cancellation is operational", err: fmt.Errorf("execute: %w", executor.ErrCancelledExternally), }, + { + name: "the caller's own cancellation is operational", + err: fmt.Errorf("execute: %w", executor.ErrCancelledByCaller), + }, + { + name: "invalid index on another table is a refusal that renders the typed advice", + err: fmt.Errorf("execute: %w", &executor.InvalidIndexError{ + Schema: "public", + Index: "users_ref_idx", + Table: "shipments", + Cleanup: executor.ErrInvalidIndexOnOtherTable, + }), + wantReason: "invalid-index-occupied", + wantDetail: []string{`"public"."users_ref_idx"`, `"shipments"`, "re-plan"}, + wantNotDetail: []string{"drop the invalid index"}, + }, + { + name: "non-droppable invalid index is a refusal even when it wraps a statement-budget cause", + err: fmt.Errorf("execute: %w", &executor.InvalidIndexError{ + Schema: "public", + Index: "users_pkey", + Table: "users", + Build: &executor.BudgetError{Cause: executor.CauseStatement, Budget: time.Second}, + Cleanup: executor.ErrInvalidIndexNotDroppable, + }), + wantReason: "invalid-index-occupied", + wantDetail: []string{`"public"."users_pkey"`, "constraint's index", "operator must resolve"}, + wantNotDetail: []string{"budget", "drop the invalid index"}, + }, + { + name: "abandoned invalid index is operational", + err: fmt.Errorf("execute: %w", &executor.InvalidIndexError{ + Schema: "public", Index: "users_ref_idx", Table: "users", + Cleanup: executor.ErrAbandonedInvalidIndex, + }), + }, { name: "partitioned-parent admission refusal renders the typed sentence", err: fmt.Errorf("admit statement for partitioned PostgreSQL table %q: %w", "users", @@ -268,11 +304,14 @@ func TestRetryPathFitsUnderApplyCeiling(t *testing.T) { } // TestInvalidIndexDetailMatchesVerdictOwnership pins the advice ladder to -// the verdict code: a drop is named only for the build's own proven -// leftover; a pre-existing entry gets an in-progress-build check; an -// unproven verdict gets catalog inspection because the index may be healthy. -// Every branch names the index and none renders the wrapped build or -// cleanup errors, which may carry raw server text. +// the verdict code: a drop is named only where the executor proved the entry +// is a failed build's debris on the target table — this build's own leftover +// or an abandoned entry; a build in flight says wait and names the builder; +// an entry on another table, one the server will not drop concurrently, one +// whose builder the role cannot see, and an unproven verdict get +// investigation steps because the index may be healthy or intentional. Every +// branch names the index and none renders the wrapped build or cleanup +// errors, which may carry raw server text. func TestInvalidIndexDetailMatchesVerdictOwnership(t *testing.T) { rawServerText := errors.New("ERROR: deadline exceeded at host db-internal-1.example.com") tests := []struct { @@ -289,12 +328,47 @@ func TestInvalidIndexDetailMatchesVerdictOwnership(t *testing.T) { wantNotDetail: []string{"db-internal-1"}, }, { - name: "pre-existing entry gets an in-progress-build check, never a drop", - err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", - Build: rawServerText, Cleanup: executor.ErrPreexistingInvalidIndex}, - wantDetail: []string{`"public"."big_ref_idx"`, "another actor's build", "pg_stat_activity"}, + name: "abandoned entry names the drop after a re-check", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", Table: "orders", + Cleanup: executor.ErrAbandonedInvalidIndex}, + wantDetail: []string{`"public"."big_ref_idx"`, "abandoned", "no backend building it", "drop the invalid index", "retry"}, + wantNotDetail: []string{"db-internal-1"}, + }, + { + name: "build in flight says wait and names the builder, never a drop", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", Table: "orders", + BuilderPID: 4242, Cleanup: executor.ErrInvalidIndexBuildInFlight}, + wantDetail: []string{`"public"."big_ref_idx"`, "backend 4242", "still building it", "wait"}, + wantNotDetail: []string{"drop the invalid index", "db-internal-1"}, + }, + { + name: "unobservable builder gets a privileged progress check, never a drop", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", Table: "orders", + Cleanup: executor.ErrInvalidIndexBuilderUnobservable}, + wantDetail: []string{`"public"."big_ref_idx"`, "cannot observe", "pg_stat_progress_create_index", "pg_read_all_stats"}, wantNotDetail: []string{"drop the invalid index", "db-internal-1"}, }, + { + name: "entry on another table names that table and a re-plan, never a drop", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", Table: "shipments", + Cleanup: executor.ErrInvalidIndexOnOtherTable}, + wantDetail: []string{`"public"."big_ref_idx"`, "different table", `"shipments"`, "re-plan"}, + wantNotDetail: []string{"drop the invalid index", "retry", "db-internal-1"}, + }, + { + name: "entry on another table with no inspected table name still re-plans", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", + Cleanup: executor.ErrInvalidIndexOnOtherTable}, + wantDetail: []string{`"public"."big_ref_idx"`, "different table;", "re-plan"}, + wantNotDetail: []string{"drop the invalid index", `("")`}, + }, + { + name: "non-droppable entry is left to an operator, never a drop", + err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", Table: "orders", + Cleanup: executor.ErrInvalidIndexNotDroppable}, + wantDetail: []string{`"public"."big_ref_idx"`, "constraint's index", "operator must resolve", "re-plan"}, + wantNotDetail: []string{"drop the invalid index", "retry", "db-internal-1"}, + }, { name: "unproven verdict gets catalog inspection, never a drop", err: &executor.InvalidIndexError{Schema: "public", Index: "big_ref_idx", diff --git a/pkg/engine/postgres/postgres_integration_test.go b/pkg/engine/postgres/postgres_integration_test.go index b083aa3aa..27e9c0302 100644 --- a/pkg/engine/postgres/postgres_integration_test.go +++ b/pkg/engine/postgres/postgres_integration_test.go @@ -951,13 +951,13 @@ func TestEngineApplyPartitionedParentConcurrentIndexRefusal(t *testing.T) { assert.Contains(t, progress.ErrorMessage, "cannot build parent-level indexes concurrently") } -// TestEngineApplyConcurrentIndexPreexistingInvalidRetryable proves an -// invalid index already occupying the target name fails the build as a -// retryable operational failure whose detail names the index and the -// investigation step — the entry may be another actor's build still in -// progress, so the advice is to check for one before any recovery, never a -// statement to run. -func TestEngineApplyConcurrentIndexPreexistingInvalidRetryable(t *testing.T) { +// TestEngineApplyConcurrentIndexAbandonedInvalidRetryable proves an +// abandoned invalid index already occupying the target name — on the target +// table, with no backend building it — fails the build as a retryable +// operational failure whose detail names the index and the recovery: an +// operator confirms the entry is still invalid and unowned, drops it, and the +// drive offers the retry. +func TestEngineApplyConcurrentIndexAbandonedInvalidRetryable(t *testing.T) { dsn, db := testutil.StartPostgres(t, "invalid_index_test") _, err := db.ExecContext(t.Context(), "CREATE TABLE public.orders (id bigint PRIMARY KEY, ref text)") require.NoError(t, err) @@ -976,8 +976,40 @@ func TestEngineApplyConcurrentIndexPreexistingInvalidRetryable(t *testing.T) { assert.Equal(t, "failed", progress.Metadata["phase"]) assert.True(t, progress.Retryable, "an operator can clear the invalid index; the drive must offer the retry") assert.Contains(t, progress.ErrorMessage, "orders_ref_idx") - assert.Contains(t, progress.ErrorMessage, "another actor's build") - assert.Contains(t, progress.ErrorMessage, "pg_stat_activity") + assert.Contains(t, progress.ErrorMessage, "abandoned invalid index") + assert.Contains(t, progress.ErrorMessage, "no backend building it") + assert.Contains(t, progress.ErrorMessage, "drop the invalid index") +} + +// TestEngineApplyConcurrentIndexOnOtherTableRefused proves an invalid index +// holding the requested name on a different table in the schema is a +// permanent refusal, not a retry: this change can never clear another table's +// entry, so the detail names both the index and the table it sits on and +// sends the author back to the schema file. +func TestEngineApplyConcurrentIndexOnOtherTableRefused(t *testing.T) { + dsn, db := testutil.StartPostgres(t, "invalid_index_other_table_test") + _, err := db.ExecContext(t.Context(), "CREATE TABLE public.orders (id bigint PRIMARY KEY, ref text)") + require.NoError(t, err) + _, err = db.ExecContext(t.Context(), "CREATE TABLE public.shipments (id bigint PRIMARY KEY, ref text)") + require.NoError(t, err) + _, err = db.ExecContext(t.Context(), "CREATE INDEX orders_ref_idx ON public.shipments (ref)") + require.NoError(t, err) + _, err = db.ExecContext(t.Context(), + "UPDATE pg_index SET indisvalid = false WHERE indexrelid = 'public.orders_ref_idx'::regclass") + require.NoError(t, err) + + eng := New() + _, err = eng.Apply(t.Context(), applyRequest(dsn, "orders", + "CREATE INDEX CONCURRENTLY orders_ref_idx ON public.orders (ref)")) + require.NoError(t, err) + progress := awaitPostgresProgress(t, eng, "orders") + assert.Equal(t, engine.StateFailed, progress.State) + assert.Equal(t, "refused", progress.Metadata["phase"]) + assert.False(t, progress.Retryable, "another table's invalid index is permanent until the plan or target changes") + assert.Contains(t, progress.ErrorMessage, "orders_ref_idx") + assert.Contains(t, progress.ErrorMessage, `"shipments"`) + assert.Contains(t, progress.ErrorMessage, "re-plan") + assert.NotContains(t, progress.ErrorMessage, "drop the invalid index") } // applyRequest builds a single-statement apply request with the same identity From ad6dd2a9b32c72576663080ebecfa8070d6b58fc Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 7 Sep 2026 20:49:45 +1000 Subject: [PATCH 2/2] fix(postgres): refuse pool-too-small and pin dispositions to the engine's permanence pg-sprite marks PoolTooSmall permanent: the pool is sized by the target DSN, so a retry is refused at admission the same way. The totality test now asserts each executor code's disposition against Code.Permanent(), with the one deliberate exception named, so a future vocabulary change fails the build instead of quietly landing in the wrong arm. --- docs/postgresql.md | 17 +++++++--- pkg/engine/postgres/apply.go | 15 ++++++--- pkg/engine/postgres/apply_test.go | 33 +++++++++++++++++++ .../postgres/postgres_integration_test.go | 29 ++++++++++++++++ 4 files changed, 85 insertions(+), 9 deletions(-) diff --git a/docs/postgresql.md b/docs/postgresql.md index ffbd74227..832e98752 100644 --- a/docs/postgresql.md +++ b/docs/postgresql.md @@ -284,11 +284,18 @@ change or that depend on the target: refusal. Exhausting the lock budget is retryable after contention clears. - A concurrent index build runs under its own 4-minute budget. A build that leaves an invalid index behind — including one cancelled by that budget — or - finds one already under the requested name fails as a retryable operational - failure naming the index and the recovery step; the invalid index, not the - cause that produced it, is the outcome an operator acts on. Only a budget - exhaustion that provably left nothing is a permanent refusal. A parent-level - index build on a partitioned table is refused permanently. + finds one already under the requested name that an operator can clear (a + failed build's leftover, an abandoned entry, or another backend's build to + wait out) fails as a retryable operational failure naming the index and the + recovery step; the invalid index, not the cause that produced it, is the + outcome an operator acts on. An invalid index under the requested name that + this change can never clear — one on a different table, or one that backs a + constraint or belongs to a partitioned table — is refused permanently with + the same naming, as is a budget exhaustion that provably left nothing. A + parent-level index build on a partitioned table is refused permanently, and + so is a target connection pool too small to hold the build's sessions at + once, since the pool is sized by the target DSN and a retry sees the same + pool. - Other operational failures are recorded as retryable when no create-set prefix committed and expose a sanitized message; connection and server details remain in server logs. A create-set failure after the table commits diff --git a/pkg/engine/postgres/apply.go b/pkg/engine/postgres/apply.go index cac927edb..d3201a982 100644 --- a/pkg/engine/postgres/apply.go +++ b/pkg/engine/postgres/apply.go @@ -541,16 +541,23 @@ func refusalForOutcome(code executor.Code, table string) (*refusal, bool) { return &refusal{reason: "invalid-index-occupied", cause: fmt.Sprintf("an invalid index already occupies a name the change to %q needs and is not a failed build's leftover", table), remedy: "rename the index in the schema file and re-plan, or resolve the entry on the target"}, true + case executor.CodePoolTooSmall: + // The pool is sized by the target DSN, so every retry against the + // same configuration is refused at admission the same way; only an + // operator raising the pool ceiling changes the outcome. + return &refusal{reason: "pool-too-small", + cause: fmt.Sprintf("the target's connection pool cannot hold every session the change to %q needs at once", table), + remedy: "raise the pool size on the target DSN, then re-run"}, true case executor.CodeBudgetLockExceeded, executor.CodeCancelledByCaller, executor.CodeCancelledExternally, executor.CodeInvalidIndexOwnLeftover, executor.CodeInvalidIndexAbandoned, executor.CodeInvalidIndexBuildInFlight, executor.CodeInvalidIndexBuilderUnobservable, executor.CodeInvalidIndexUnproven, - executor.CodePoolTooSmall, executor.CodeExecutionFailed: + executor.CodeExecutionFailed: // Operational outcomes: a bounded lock race, the caller's own // context ending or an external stop, an invalid-index state an - // operator clears or waits out, engine pool sizing, or a failure - // outside the typed set. A retry can succeed once conditions - // change, so none is a permanent refusal. + // operator clears or waits out, or a failure outside the typed set. + // A retry can succeed once conditions change, so none is a permanent + // refusal. return nil, true } return nil, false diff --git a/pkg/engine/postgres/apply_test.go b/pkg/engine/postgres/apply_test.go index 443c66b61..9e6b27572 100644 --- a/pkg/engine/postgres/apply_test.go +++ b/pkg/engine/postgres/apply_test.go @@ -193,6 +193,12 @@ func TestClassifyRefusal(t *testing.T) { err: fmt.Errorf("preflight: %w", preflight.ErrNotTable), wantReason: "not-a-table", }, + { + name: "pool too small for the build's sessions is a refusal that names the pool", + err: fmt.Errorf("admit concurrent build: %w", executor.ErrPoolTooSmall), + wantReason: "pool-too-small", + wantDetail: []string{"connection pool", `"users"`, "raise the pool size"}, + }, { name: "untyped error is operational", err: errors.New("dial tcp: connection refused"), @@ -458,7 +464,26 @@ func TestProgressResultReportsCreateSequenceLength(t *testing.T) { // retryable tail. Every refusal also carries a cause: it is the clause the // composed detail opens with, and a remedy alone would publish a detail that // starts mid-sentence. +// +// The disposition is also pinned to the executor's own verdict: a code +// pg-sprite marks permanent refuses here, and a code it does not marks +// operational, unless the exception table names the code and states why +// SchemaBot's apply policy departs from the engine's floor. Departing in the +// other direction — retrying a code the engine calls permanent — is never +// sanctioned, because the drive would re-run a verdict that cannot change +// until its attempt ceiling ends it. func TestRefusalForOutcomeTotalOverExecutorCodes(t *testing.T) { + // refusedThoughNotPermanent lists the codes SchemaBot refuses although + // pg-sprite leaves them retryable, with the reason for each. The + // engine's Permanent doc names this direction of disagreement as an + // adapter's own retry policy. + refusedThoughNotPermanent := map[executor.Code]string{ + // The statement budget is sized as SchemaBot's native-safety lease: + // a statement that needs longer is not native-safe under this + // policy, so re-running it unchanged would only spend the lease + // again. + executor.CodeBudgetStatementExceeded: "the statement budget is the native-safety lease", + } for _, code := range executor.Codes() { t.Run(string(code), func(t *testing.T) { r, known := refusalForOutcome(code, "users") @@ -467,6 +492,14 @@ func TestRefusalForOutcomeTotalOverExecutorCodes(t *testing.T) { assert.NotEmpty(t, r.reason, "refusal for %q has no reason", code) assert.NotEmpty(t, r.cause, "refusal for %q has no cause", code) } + refused := r != nil + if why, exempt := refusedThoughNotPermanent[code]; exempt { + assert.False(t, code.Permanent(), "exception for %q is stale: the engine now marks it permanent", code) + assert.True(t, refused, "code %q is listed as a policy refusal (%s) but is operational", code, why) + return + } + assert.Equal(t, code.Permanent(), refused, + "disposition for %q disagrees with the engine's permanence verdict", code) }) } } diff --git a/pkg/engine/postgres/postgres_integration_test.go b/pkg/engine/postgres/postgres_integration_test.go index 301c162a8..e6fee053f 100644 --- a/pkg/engine/postgres/postgres_integration_test.go +++ b/pkg/engine/postgres/postgres_integration_test.go @@ -1013,6 +1013,35 @@ func TestEngineApplyConcurrentIndexOnOtherTableRefused(t *testing.T) { assert.NotContains(t, progress.ErrorMessage, "drop the invalid index") } +// TestEngineApplyConcurrentIndexBackingConstraintRefused proves an invalid +// index holding the requested name on the target table but backing a +// constraint is a permanent refusal, not a retry: the server will not drop a +// constraint's index concurrently, so no recovery this change can run clears +// the name. The detail names the index, leaves it to an operator, and never +// tells anyone to drop it. +func TestEngineApplyConcurrentIndexBackingConstraintRefused(t *testing.T) { + dsn, db := testutil.StartPostgres(t, "invalid_index_constraint_test") + _, err := db.ExecContext(t.Context(), + "CREATE TABLE public.orders (id bigint PRIMARY KEY, ref text, CONSTRAINT orders_ref_idx UNIQUE (ref))") + require.NoError(t, err) + _, err = db.ExecContext(t.Context(), + "UPDATE pg_index SET indisvalid = false WHERE indexrelid = 'public.orders_ref_idx'::regclass") + require.NoError(t, err) + + eng := New() + _, err = eng.Apply(t.Context(), applyRequest(dsn, "orders", + "CREATE INDEX CONCURRENTLY orders_ref_idx ON public.orders (ref)")) + require.NoError(t, err) + progress := awaitPostgresProgress(t, eng, "orders") + assert.Equal(t, engine.StateFailed, progress.State) + assert.Equal(t, "refused", progress.Metadata["phase"]) + assert.False(t, progress.Retryable, "a constraint's invalid index is permanent until an operator resolves it or the plan changes") + assert.Contains(t, progress.ErrorMessage, "orders_ref_idx") + assert.Contains(t, progress.ErrorMessage, "constraint's index") + assert.Contains(t, progress.ErrorMessage, "an operator must resolve it") + assert.NotContains(t, progress.ErrorMessage, "drop the invalid index") +} + // applyRequest builds a single-statement apply request with the same identity // shape the drive layer uses: the task identifier stamped into // ResumeState.MigrationContext keys the engine's progress to this apply.