diff --git a/README.md b/README.md index 0c1dfe9..ae51d66 100644 --- a/README.md +++ b/README.md @@ -118,10 +118,11 @@ refusal — never a silently wrong or incomplete result: [runbook](docs/invalid-index-recovery.md) applies. - **The accepted-blocking passthrough covers index maintenance only** — `migrate --accept-blocking SCHEMA.TABLE` runs an otherwise-refused plain - `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` deliberately under the - engine's bounded lock and statement budgets, with the verdict marked - `executed-without-online-safety` (exit 3). Every other refusal — including - a blocking index build on a partitioned parent — still exits 2. Design: + `DROP INDEX` (not `CASCADE`), `REINDEX INDEX`, or `REINDEX TABLE` + deliberately under the engine's bounded lock and statement budgets, with + the verdict marked `executed-without-online-safety` (exit 3). Every other + refusal — including a blocking index build on a partitioned parent — still + exits 2. Design: [docs/lock-budgeted-passthrough.md](docs/lock-budgeted-passthrough.md). - **Non-table objects** — views, standalone sequences, enums, domains, extensions, functions, triggers — are outside the declarative model, diff --git a/docs/capabilities.md b/docs/capabilities.md index 341cdbb..de26aa5 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -237,7 +237,7 @@ or the only online form is a direct statement the operator runs). | Operation | Status | Engine path | Online-safety problem? | Behavior and why | | --- | --- | --- | --- | --- | | `CREATE [UNIQUE] INDEX` on a plain table — including partial, expression, covering (`INCLUDE`), GIN/GiST/BRIN | ✅ | native, safer sequence | Yes | Executed as (or rewritten to) `CREATE INDEX CONCURRENTLY`, with validity verification, typed invalid-index outcomes, and a proven recovery for abandoned leftovers (`RebuildAbandonedIndex`, or `DropAbandonedIndex` to remove the leftover without rebuilding; both library-only; [runbook](invalid-index-recovery.md)) | -| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity` | +| `DROP INDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity` | | `REINDEX` | ❌ | — | Yes — the online form is a direct `CONCURRENTLY` statement that `migrate` does not drive | `migrate` never runs this statement online-safely: the plain form is refused and names `REINDEX ... CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A plain `REINDEX INDEX` or `REINDEX TABLE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0). `REINDEX SCHEMA`, `DATABASE`, and `SYSTEM` stay refused. A desired schema cannot express a reindex, so `diff` never produces one | | Index build on a **partitioned parent** | 🟡 | native, planned flow | Yes | PostgreSQL has no parent-level `CONCURRENTLY`; the blocking form is refused by policy (`--force` does not bypass it, and `--accept-blocking` does not reach it yet). The partition-aware flow — `CREATE INDEX ON ONLY` → per-partition CIC → `ATTACH PARTITION`, with crash-resume per leaf — is planned | | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | ❌ | — | Yes — unsolvable today | PostgreSQL does not support adopting an index on a partitioned parent in any supported version; refused before execution | @@ -366,7 +366,8 @@ unmistakably marked as executed without an online-safety guarantee 0). The value must name the table whose lock you accept — the owning table of the index — resolved from the catalog, and `--statement-timeout` must be spelled on the same command line. Only changes the engine understands but cannot run *safely* are eligible: today a -single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE`. Refusals for +single-relation plain `DROP INDEX` (never `CASCADE`, which also drops dependent objects such +as a foreign key on another table), `REINDEX INDEX`, or `REINDEX TABLE`. Refusals for unrecognized SQL never are, and the refused statement's `blocking_passthrough_eligible` field on the plan report says whether a refusal qualifies. An exhausted lock budget is still a refusal (exit 2, nothing ran); a statement cancelled by its budget is a failure diff --git a/docs/invariants.md b/docs/invariants.md index be0bfd7..ac55903 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -646,10 +646,19 @@ relation (a table where the statement needs an index, or the reverse) is a usage no verdict — each its own typed error — and nothing executes. The acknowledgement is never inferred from the statement text. The resolution and the execution are separate statements on the pool, so a concurrent re-pointing of the index name between them is outside what this -invariant guarantees; the design records the window. *Enforced:* `pkg/migrate` +invariant guarantees; the design records the window. The statement's scope must also be +exactly that table: a `DROP INDEX ... CASCADE` also drops the objects that depend on the index, +such as a foreign key on another table, so it is not a single-relation statement and the +executor refuses it (`ErrUnsupportedAcceptedBlocking`) before opening a transaction. +*Enforced:* `pkg/migrate` (`acceptBlocking`, `lockedTable`; `ErrAcceptBlockingMismatch`, `ErrAcceptBlockingRelationNotFound`, `ErrAcceptBlockingWrongRelationKind`), -`TestRunAcceptBlockingRejectsFalseAcknowledgements`, `TestRelationKindMatches`. +`pkg/executor` (`acceptedBlockingIneligibility`), `pkg/statement` (`ParseOne` classifies a +cascading drop as `IndexTargetOther`), +`TestRunAcceptBlockingRejectsFalseAcknowledgements`, `TestRelationKindMatches`, +`TestAcceptedBlockingAdmission`, `TestExecuteAcceptedBlockingRefusesCascadeDrop`, +`TestExecuteAcceptedBlockingPlainDropOfReferencedIndexIsRefusedByServer`, +`TestExecuteAcceptedBlockingRestrictDropOfReferencedIndexIsRefusedByServer`. *Source:* [lock-budgeted passthrough](lock-budgeted-passthrough.md#flag-and-front-door). ### AB-4 — Acceptance applies only to an eligible in-process refusal, and to nothing else @@ -657,13 +666,16 @@ invariant guarantees; the design records the window. *Enforced:* `pkg/migrate` The acknowledgement changes the outcome of exactly one thing: a gate refusal whose in-process proof the eligibility registry marks eligible (RF-8). Every other refusal — an ineligible gate refusal, a planner refusal, a size-guard or preflight refusal — stands exactly as it -would without the flag, and a statement the gate admits runs its normal path. The +would without the flag — including a `DROP INDEX ... CASCADE`, whose refusal site is the +ineligible one because its scope exceeds the acknowledged table — and a statement the gate +admits runs its normal path. The acknowledgement is not `--force`: the two cannot be combined, and a dry run reports the refusal as it stands rather than the accepted execution. The front door requires the statement budget to be stated on the same command line, because the accepted outage bound is a decision, not a default. *Enforced:* `migrate.AcceptedRefusal`, `MigrateCmd.Validate`, -`TestAcceptedRefusal`, `TestAcceptBlockingFlagGrammar`, -`TestMigrateAcceptBlockingLeavesIneligibleRefusalsAlone`. *Source:* +`migrate.gateRefusal` (the refusal site), `TestAcceptedRefusal`, `TestAcceptBlockingFlagGrammar`, +`TestMigrateAcceptBlockingLeavesIneligibleRefusalsAlone`, +`TestMigrateAcceptBlockingRefusesCascadeAndKeepsDependentForeignKey`. *Source:* [lock-budgeted passthrough](lock-budgeted-passthrough.md#flag-and-front-door). ### AB-5 — The accepted outcome is distinguishable from online-safe execution at every surface diff --git a/docs/lock-budgeted-passthrough.md b/docs/lock-budgeted-passthrough.md index 9e41dd7..96e6deb 100644 --- a/docs/lock-budgeted-passthrough.md +++ b/docs/lock-budgeted-passthrough.md @@ -168,6 +168,14 @@ and the site distinguishes the single-relation forms — one `DROP INDEX`, `REIN `REINDEX TABLE` — from the forms that name several relations or none: `DROP INDEX a, b`, `REINDEX SCHEMA`, `REINDEX DATABASE`, and `REINDEX SYSTEM`. Those multi-relation forms are ineligible in v1 because one flag value cannot acknowledge several tables' locks. +`DROP INDEX ... CASCADE` is on the ineligible side too, although it names one index: it also +drops the objects that depend on the index — a foreign key on another table that references a +unique index, for one — which locks and changes tables the acknowledgement never named. The +parse boundary classifies it as a statement whose scope is wider than one relation, so the +registry refuses it, and the executor's shape check refuses it again for a library caller +that reaches `ExecuteAcceptedBlocking` directly. A plain drop or an explicit `RESTRICT` (the +default behavior) stays eligible, and is the safe retry: if anything still depends on the +index, PostgreSQL refuses the drop with SQLSTATE `2BP01` and changes nothing. Execution is exposed only by the imperative `migrate` front door. Declarative `diff` still classifies and reports eligibility, but it cannot accept the flag or execute this path. A @@ -195,7 +203,7 @@ The v1 set is: | Class | Typed refusal shape | v1 | Rationale | | --- | --- | --- | --- | -| `by-design` | `index-statement` at the plain single-relation `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` site, with the concurrent safer idiom | eligible | This is the core maintenance-window case: the submitted form is understood, the safer idiom is known, one table's lock is being accepted, and bounding `ACCESS EXCLUSIVE` acquisition prevents lock-queue pile-up. The multi-relation sites (`DROP INDEX a, b`, `REINDEX SCHEMA`, `DATABASE`, `SYSTEM`) stay ineligible. `REINDEX` on a partitioned table or index is eligible by shape but cannot run inside the engine-owned transaction; the executor reports it as `unsupported-accepted-blocking` (see [Engine-owned session and budgets](#engine-owned-session-and-budgets)). | +| `by-design` | `index-statement` at the plain single-relation `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` site, with the concurrent safer idiom | eligible | This is the core maintenance-window case: the submitted form is understood, the safer idiom is known, one table's lock is being accepted, and bounding `ACCESS EXCLUSIVE` acquisition prevents lock-queue pile-up. The multi-relation sites (`DROP INDEX a, b`, `REINDEX SCHEMA`, `DATABASE`, `SYSTEM`) and `DROP INDEX ... CASCADE`, whose dependent drops reach tables beyond the acknowledged one, stay ineligible. `REINDEX` on a partitioned table or index is eligible by shape but cannot run inside the engine-owned transaction; the executor reports it as `unsupported-accepted-blocking` (see [Engine-owned session and budgets](#engine-owned-session-and-budgets)). | | `capability-boundary` | `unsupported-partitioned-parent` with cause `parent-blocking-index-build` | eligible | The missing partition-aware flow does not make the plain PostgreSQL statement unknown. Its principal pre-execution hazard is acquiring the parent lock, which `lock_timeout` bounds. | | `capability-boundary` | `not-native-safe-rewrite-required` | ineligible | A rewrite holds its strong lock for the full rewrite. Bounding acquisition alone does not bound the outage, and v1 must not imply otherwise. | | `capability-boundary` | `backend-unavailable` and all other missing routes or backends | ineligible | A missing execution backend is not permission to substitute an unrelated blocking implementation. | diff --git a/docs/plan-report.md b/docs/plan-report.md index bae76dd..21fca9f 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -219,7 +219,7 @@ statement of each class and its consumer action; this table is a copy of its row |---|---| | `capability-boundary` | The engine cannot do this yet, or has no implemented safe route. Wait for the capability or escalate. | | `no-online-safety-problem` | There is nothing for an online schema-change engine to make safe; another tool class owns the work, or the operator runs it directly. Hand the statement to the named `owner`. | -| `by-design` | pg-sprite permanently refuses this form — PostgreSQL offers no online mechanism in any supported version, or a safer idiom exists and the verdict names it. Use the safer idiom; for a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE`, `migrate --accept-blocking SCHEMA.TABLE` runs the blocking form under bounded budgets and exits 3; otherwise run it outside pg-sprite in a maintenance window. | +| `by-design` | pg-sprite permanently refuses this form — PostgreSQL offers no online mechanism in any supported version, or a safer idiom exists and the verdict names it. Use the safer idiom; for a single-relation plain `DROP INDEX` (not `CASCADE`), `REINDEX INDEX`, or `REINDEX TABLE`, `migrate --accept-blocking SCHEMA.TABLE` runs the blocking form under bounded budgets and exits 3; otherwise run it outside pg-sprite in a maintenance window. | | `environmental` | The change is supportable, but not here, now, or as this role: a size policy, exhausted budget, privilege, server version, stale plan, or catalog collision stopped it. Retry, provision, upgrade, re-plan, or escalate. | | `invariant-violation` | pg-sprite refused because its own input or state is incoherent — a report this build cannot have produced. Report it with the verdict; do not retry, wait, or route elsewhere. | diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index 8c0eb11..58303af 100644 --- a/docs/refusal-classes.md +++ b/docs/refusal-classes.md @@ -20,7 +20,7 @@ The refusal verdict carries `class`, with exactly five values: | --- | --- | --- | | `capability-boundary` | The engine cannot do this yet, or has no implemented safe route. This is a T2 capability. | Wait for the capability or escalate. | | `no-online-safety-problem` | There is nothing for an online schema-change engine to make safe; another tool class owns the work, or the operator runs it directly. | Hand the statement to the named owner. | -| `by-design` | pg-sprite permanently refuses this form — because PostgreSQL offers no online mechanism in any supported version, or because a safer idiom exists; the verdict names the idiom or the deliberate path. This is [RF-5](invariants.md#refusals-and-preflight-rf) seen from the consumer's side. | Use the safer idiom. For a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE`, `migrate --accept-blocking SCHEMA.TABLE` runs the blocking form under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)); otherwise run it outside pg-sprite in a maintenance window. | +| `by-design` | pg-sprite permanently refuses this form — because PostgreSQL offers no online mechanism in any supported version, or because a safer idiom exists; the verdict names the idiom or the deliberate path. This is [RF-5](invariants.md#refusals-and-preflight-rf) seen from the consumer's side. | Use the safer idiom. For a single-relation plain `DROP INDEX` (not `CASCADE`), `REINDEX INDEX`, or `REINDEX TABLE`, `migrate --accept-blocking SCHEMA.TABLE` runs the blocking form under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)); otherwise run it outside pg-sprite in a maintenance window. | | `environmental` | The change is supportable, but not here, now, or as this role: a size policy, exhausted budget, privilege, server version, stale plan, or catalog collision stopped it. | Retry, provision, upgrade, re-plan, or escalate. | | `invariant-violation` | pg-sprite refused because its own input or state is incoherent — a report this build cannot have produced. This is a bug in pg-sprite, not a boundary of it; it is the refusal-verdict face of the fail-closed `ErrInvariantViolation` rule in [SAFETY.md](../SAFETY.md#rules-inside-the-core). | Report it with the verdict; do not retry, wait, or route elsewhere. | @@ -76,8 +76,8 @@ routed to provisioning and never to the data change runner. | `unsupported-statement` | Imperative or create admission sentinel `ErrIfNotExistsUnsupported`; create admission sentinel `ErrDuplicateCreateName` (the sentinel forms of the `if-not-exists` and `duplicate-name` causes below) | `by-design` | — | | `unsupported-statement` | Create admission sentinel `ErrPartitionOfUnsupported` (the sentinel form of the `partition-of` cause below) | `capability-boundary` | — | | `unsupported-statement` | `planRefusal` in `pkg/migrate/desired.go`: a statement whose disposition this build does not know, or a plan whose aggregate disposition no statement carries | `invariant-violation` | — | -| `index-statement` | Plain `DROP INDEX` or `REINDEX`, for which the verdict names the concurrent form | `by-design` | — | -| `index-statement` | An already-concurrent maintenance statement that pg-sprite need not wrap | `no-online-safety-problem` | `direct-operator` | +| `index-statement` | Plain `DROP INDEX` or `REINDEX`, for which the verdict names the concurrent form; also a `DROP INDEX CONCURRENTLY` that PostgreSQL rejects (with `CASCADE`, or naming several indexes) | `by-design` | — | +| `index-statement` | An already-concurrent maintenance statement that pg-sprite need not wrap and PostgreSQL accepts | `no-online-safety-problem` | `direct-operator` | | `not-native-safe-table-too-large` | The size policy prevents a blind bounded attempt | `environmental` | — | | `insufficient-privileges` | The connected role lacks a required grant | `environmental` | — | | `not-native-safe-budget-exceeded` | The lock or statement budget cancels an attempt | `environmental` | — | diff --git a/internal/cli/accept_blocking_integration_test.go b/internal/cli/accept_blocking_integration_test.go index 2424044..af4943c 100644 --- a/internal/cli/accept_blocking_integration_test.go +++ b/internal/cli/accept_blocking_integration_test.go @@ -86,12 +86,62 @@ func TestMigrateAcceptBlockingMismatchExecutesNothing(t *testing.T) { assert.True(t, exists, "nothing must have executed") } +// DROP INDEX ... CASCADE also drops the objects that depend on the index. +// Here orders.user_email references the unique index on users, so the +// cascade would drop that foreign key and lock and change orders — a table +// the acknowledgement of users never named. The statement is refused as it +// would be without the flag, and the foreign key stays. +func TestMigrateAcceptBlockingRefusesCascadeAndKeepsDependentForeignKey(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 %[1]s.users ( + id int PRIMARY KEY, + email text NOT NULL + ); + CREATE UNIQUE INDEX users_email_key ON %[1]s.users (email); + CREATE TABLE %[1]s.orders ( + id int PRIMARY KEY, + user_email text REFERENCES %[1]s.users (email) + )`, schema)) + require.NoError(t, err) + + cmd := newMigrateCmd(url, fmt.Sprintf("DROP INDEX %s.users_email_key CASCADE", schema)) + cmd.AcceptBlocking = schema + ".users" + cmd.JSON = true + var out strings.Builder + err = cmd.run(t.Context(), &out) + require.ErrorIs(t, err, verdict.ErrRefused) + require.NotErrorIs(t, err, verdict.ErrAcceptedBlocking) + + var v verdict.Verdict + require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonIndexStatement, v.Reason) + assert.False(t, v.BlockingPassthrough) + + var indexes, foreignKeys int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_indexes WHERE schemaname = $1 AND indexname = 'users_email_key'`, + schema).Scan(&indexes)) + assert.Equal(t, 1, indexes, "the unique index must still exist") + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_constraint + WHERE contype = 'f' AND conrelid = to_regclass($1)`, + schema+".orders").Scan(&foreignKeys)) + assert.Equal(t, 1, foreignKeys, "the foreign key on the unacknowledged table must still exist") +} + // The acknowledgement applies only to the eligible refusals. A gate -// refusal outside the set — an already-concurrent form, an unsupported -// kind — still refuses before dialing, flag or no flag. +// refusal outside the set — an already-concurrent form, a cascading drop, an +// unsupported kind — still refuses before dialing, flag or no flag. func TestMigrateAcceptBlockingLeavesIneligibleRefusalsAlone(t *testing.T) { for _, alter := range []string{ "DROP INDEX CONCURRENTLY public.users_email_idx", + "DROP INDEX public.users_email_idx CASCADE", "REINDEX SCHEMA public", "ALTER INDEX public.users_email_idx SET (fillfactor = 90)", } { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 9e0769f..1b5fc86 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -112,7 +112,7 @@ type MigrateCmd struct { IndexBuildTimeout time.Duration `help:"Overall bound (statement_timeout) for one concurrent index build step; expect large tables to need a generous value." default:"30m"` ValidateTimeout time.Duration `help:"Overall bound (statement_timeout) for one VALIDATE CONSTRAINT step; expect large tables to need a generous value." default:"30m"` Force string `help:"Run the submitted form as-is, overriding a safer-sequence substitution or a rewrite-required/backend-unavailable refusal. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly. The forced run is still parsed, preflighted, size-guarded, and budget-bounded; planner refusals (no known safe path) and unsupported statement kinds cannot be forced." placeholder:"SCHEMA.TABLE"` - AcceptBlocking string `help:"Run one refused blocking index statement as-is, without online safety: a plain single-relation DROP INDEX, REINDEX INDEX, or REINDEX TABLE. The value is the typed acknowledgement: it must name the schema-qualified table whose lock you accept (the index's owning table) exactly. The statement runs in an engine-owned transaction under --lock-timeout and an explicitly supplied --statement-timeout, which must be longer than --lock-timeout; an exhausted lock budget refuses (exit 2), a cancelled or failed statement fails (exit 1), a commit exits 3. Every other refusal stands. Cannot be combined with --force or --dry-run." placeholder:"SCHEMA.TABLE"` + AcceptBlocking string `help:"Run one refused blocking index statement as-is, without online safety: a plain single-relation DROP INDEX (not CASCADE), REINDEX INDEX, or REINDEX TABLE. The value is the typed acknowledgement: it must name the schema-qualified table whose lock you accept (the index's owning table) exactly. The statement runs in an engine-owned transaction under --lock-timeout and an explicitly supplied --statement-timeout, which must be longer than --lock-timeout; an exhausted lock budget refuses (exit 2), a cancelled or failed statement fails (exit 1), a commit exits 3. Every other refusal stands. Cannot be combined with --force or --dry-run." placeholder:"SCHEMA.TABLE"` LockAttempts int `help:"Maximum bounded attempts when native DDL exceeds lock_timeout; 1 disables retry." default:"3"` LockBackoff time.Duration `help:"Initial exponential backoff between lock-timeout attempts." default:"100ms"` LockBackoffMax time.Duration `help:"Maximum exponential backoff between lock-timeout attempts." default:"1s"` diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index f70c418..f618587 100644 --- a/pkg/capabilities/capabilities.yaml +++ b/pkg/capabilities/capabilities.yaml @@ -241,7 +241,7 @@ rows: migrate: refused diff: supported refusal_reason: index-statement - reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`" + reason_notes: "`migrate` never runs this statement online-safely: the plain form is refused and names `DROP INDEX CONCURRENTLY`, and the concurrent form is refused as already safe — run it directly against the database. A single-index plain `DROP INDEX` without `CASCADE` can be run as-is under bounded budgets with `--accept-blocking SCHEMA.TABLE` (exit 3, never 0); `--accept-blocking` checks the replica identity under the table's lock and refuses the drop as `unsupported-statement`, naming the identity index. `diff` reports an index to remove as a **destructive** `DROP INDEX` and hints to run it through `migrate --alter`, which ends in this `index-statement` refusal — run `DROP INDEX CONCURRENTLY` directly instead. `diff` refuses dropping the table's replica identity index, or one a partition's is attached under, with `replica-identity`" - id: reindex area: "indexes" operation: "`REINDEX`" diff --git a/pkg/executor/accepted_blocking.go b/pkg/executor/accepted_blocking.go index d121dae..782e371 100644 --- a/pkg/executor/accepted_blocking.go +++ b/pkg/executor/accepted_blocking.go @@ -115,8 +115,8 @@ func executeAcceptedBlocking(ctx context.Context, pool *pgxpool.Pool, sql string if err != nil { return rep, fmt.Errorf("%w: %w", ErrUnsupportedAcceptedBlocking, err) } - if !acceptedBlockingShape(st) { - return rep, ErrUnsupportedAcceptedBlocking + if reason := acceptedBlockingIneligibility(st); reason != "" { + return rep, fmt.Errorf("%w: %s", ErrUnsupportedAcceptedBlocking, reason) } rep = BlockingReport{SQL: sql, LockTimeout: b.LockTimeout, StatementTimeout: b.StatementTimeout} start := clock.Now() @@ -203,20 +203,41 @@ func refuseReplicaIdentityDrop(ctx context.Context, tx pgx.Tx, rel statement.Ind return nil } -func acceptedBlockingShape(st statement.Statement) bool { +// acceptedBlockingIneligibility returns why a statement is outside the +// accepted-blocking shape, or "" when it is eligible. +func acceptedBlockingIneligibility(st statement.Statement) string { if st.Concurrent() { - return false + return "the concurrent form cannot run inside the engine-owned transaction; run it directly against the database" } switch st.Kind() { case statement.KindCreateIndex: - return true + return "" case statement.KindDropIndex, statement.KindReindex: - return st.IndexTarget() == statement.IndexTargetSingleRelation + // INV: AB-3 — only a statement whose whole scope is the one + // acknowledged table runs here; a DROP INDEX ... CASCADE also drops + // dependent objects on other tables and parses as another target. + return indexScopeIneligibility(st) default: - return false + return "it is not an index statement" } } +// indexScopeIneligibility names why a DROP INDEX or REINDEX statement's scope +// is wider than the one acknowledged table, or returns "" when it is not. +func indexScopeIneligibility(st statement.Statement) string { + if st.IndexTarget() == statement.IndexTargetSingleRelation { + return "" + } + if st.Cascades() { + return "a cascading drop also drops the objects that depend on the index, which the acknowledged table's lock does not cover; " + + "resubmit without CASCADE, and PostgreSQL then refuses the drop if anything depends on the index" + } + if st.Kind() == statement.KindDropIndex { + return "it names several indexes; submit one DROP INDEX per index" + } + return "its scope is wider than one relation; name one index or table" +} + // acceptedBlockingLockError maps a failed table-lock request. The DDL has // not been submitted, so no outcome here is unknown: a lock_timeout or a // statement cancellation both mean the lock was not granted within the diff --git a/pkg/executor/accepted_blocking_integration_test.go b/pkg/executor/accepted_blocking_integration_test.go index 52dc9ba..b384421 100644 --- a/pkg/executor/accepted_blocking_integration_test.go +++ b/pkg/executor/accepted_blocking_integration_test.go @@ -9,6 +9,7 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -86,6 +87,82 @@ func TestExecuteAcceptedBlockingRefusesReindexOnPartitionedRelation(t *testing.T assert.True(t, valid) } +// DROP INDEX ... CASCADE drops the objects that depend on the index. Here the +// foreign key on u references the unique index on t, so the cascade would +// lock and change u, which the lock on t does not cover. The executor +// refuses the shape before it opens a transaction, so the index and the +// foreign key both stay. +func TestExecuteAcceptedBlockingRefusesCascadeDrop(t *testing.T) { + pool, schema := newReferencedIndexFixture(t) + b := executor.BlockingBudget{LockTimeout: time.Second, StatementTimeout: 10 * time.Second} + + _, err := executor.ExecuteAcceptedBlocking(t.Context(), pool, + fmt.Sprintf("DROP INDEX %s.i CASCADE", schema), pgx.Identifier{schema, "t"}, b) + require.ErrorIs(t, err, executor.ErrUnsupportedAcceptedBlocking) + assert.True(t, executor.OutcomeCode(err).Permanent()) + assertReferencedIndexKept(t, pool, schema) +} + +// A plain DROP INDEX is eligible for the accepted-blocking path, and it is +// safe there because PostgreSQL defaults to RESTRICT: the foreign key on u +// depends on the unique index on t, so the server refuses the drop with +// dependent_objects_still_exist and changes nothing on either table. +func TestExecuteAcceptedBlockingPlainDropOfReferencedIndexIsRefusedByServer(t *testing.T) { + pool, schema := newReferencedIndexFixture(t) + b := executor.BlockingBudget{LockTimeout: time.Second, StatementTimeout: 10 * time.Second} + + _, err := executor.ExecuteAcceptedBlocking(t.Context(), pool, + fmt.Sprintf("DROP INDEX %s.i", schema), pgx.Identifier{schema, "t"}, b) + assertDependentObjectsRefusal(t, err) + assertReferencedIndexKept(t, pool, schema) +} + +// An explicit RESTRICT is the same eligible drop spelled out: the server +// refuses it while the foreign key on u depends on the index. +func TestExecuteAcceptedBlockingRestrictDropOfReferencedIndexIsRefusedByServer(t *testing.T) { + pool, schema := newReferencedIndexFixture(t) + b := executor.BlockingBudget{LockTimeout: time.Second, StatementTimeout: 10 * time.Second} + + _, err := executor.ExecuteAcceptedBlocking(t.Context(), pool, + fmt.Sprintf("DROP INDEX %s.i RESTRICT", schema), pgx.Identifier{schema, "t"}, b) + assertDependentObjectsRefusal(t, err) + assertReferencedIndexKept(t, pool, schema) +} + +// newReferencedIndexFixture creates a unique index on t that the foreign key +// on u depends on. +func newReferencedIndexFixture(t *testing.T) (*pgxpool.Pool, string) { + t.Helper() + pool, schema := newPool(t) + _, err := pool.Exec(t.Context(), fmt.Sprintf(` + CREATE TABLE %[1]s.t (id int NOT NULL); + CREATE UNIQUE INDEX i ON %[1]s.t (id); + CREATE TABLE %[1]s.u ( + t_id int REFERENCES %[1]s.t (id) + )`, schema)) + require.NoError(t, err) + return pool, schema +} + +func assertDependentObjectsRefusal(t *testing.T, err error) { + t.Helper() + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "2BP01", pgErr.Code, "dependent_objects_still_exist: the drop was not CASCADE") +} + +func assertReferencedIndexKept(t *testing.T, pool *pgxpool.Pool, schema string) { + t.Helper() + exists, valid := indexState(t, pool, schema, "i") + assert.True(t, exists) + assert.True(t, valid) + var foreignKeys int + require.NoError(t, pool.QueryRow(t.Context(), + `SELECT count(*) FROM pg_constraint WHERE contype = 'f' AND conrelid = to_regclass($1)`, + schema+".u").Scan(&foreignKeys)) + assert.Equal(t, 1, foreignKeys, "the foreign key on u must still exist") +} + func TestExecuteAcceptedBlockingAppliesBothBoundsOnExecutingSession(t *testing.T) { pool, schema := newPool(t) _, err := pool.Exec(t.Context(), fmt.Sprintf(` diff --git a/pkg/executor/accepted_blocking_internal_test.go b/pkg/executor/accepted_blocking_internal_test.go index f92109e..d941a06 100644 --- a/pkg/executor/accepted_blocking_internal_test.go +++ b/pkg/executor/accepted_blocking_internal_test.go @@ -94,6 +94,11 @@ func TestAcceptedBlockingAdmission(t *testing.T) { {sql: "REINDEX INDEX app.i", want: true}, {sql: "REINDEX TABLE app.t", want: true}, {sql: "CREATE INDEX i ON app.t (id)", want: true}, + {sql: "DROP INDEX app.i RESTRICT", want: true}, + // CASCADE drops the objects that depend on the index — a foreign key + // on another table, for one — which the one acknowledged table's lock + // does not cover. + {sql: "DROP INDEX app.i CASCADE"}, {sql: "DROP INDEX CONCURRENTLY app.i"}, {sql: "DROP INDEX app.i, app.j"}, {sql: "REINDEX SCHEMA app"}, @@ -103,7 +108,8 @@ func TestAcceptedBlockingAdmission(t *testing.T) { t.Run(tt.sql, func(t *testing.T) { st, err := statement.ParseOne(tt.sql) require.NoError(t, err) - assert.Equal(t, tt.want, acceptedBlockingShape(st)) + reason := acceptedBlockingIneligibility(st) + assert.Equal(t, tt.want, reason == "", "ineligibility reason: %q", reason) }) } } diff --git a/pkg/migrate/accept_blocking_test.go b/pkg/migrate/accept_blocking_test.go index 38da983..de47d58 100644 --- a/pkg/migrate/accept_blocking_test.go +++ b/pkg/migrate/accept_blocking_test.go @@ -26,9 +26,10 @@ func TestAcceptedRefusal(t *testing.T) { } for name, sql := range map[string]string{ - "drop index": "DROP INDEX app.orders_created_idx", - "reindex index": "REINDEX INDEX app.orders_created_idx", - "reindex table": "REINDEX TABLE app.orders", + "drop index": "DROP INDEX app.orders_created_idx", + "drop index restrict": "DROP INDEX app.orders_created_idx RESTRICT", + "reindex index": "REINDEX INDEX app.orders_created_idx", + "reindex table": "REINDEX TABLE app.orders", } { t.Run("accepts "+name, func(t *testing.T) { proof, ok := AcceptedRefusal("app.orders", gate(t, sql)) @@ -42,6 +43,7 @@ func TestAcceptedRefusal(t *testing.T) { "drop index concurrently": "DROP INDEX CONCURRENTLY app.orders_created_idx", "reindex table concurrently": "REINDEX TABLE CONCURRENTLY app.orders", "drop of several indexes": "DROP INDEX app.a, app.b", + "drop index cascade": "DROP INDEX app.orders_created_idx CASCADE", "reindex schema": "REINDEX SCHEMA app", "create table": "CREATE TABLE app.orders (id int)", "drop table": "DROP TABLE app.orders", diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 33f5604..97607c4 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -41,6 +41,17 @@ func TestGate(t *testing.T) { assert.Equal(t, "DROP INDEX CONCURRENTLY", v.SaferIdiom) }) + t.Run("a cascading drop is refused at the ineligible site with no concurrent idiom", func(t *testing.T) { + v, refused := Gate(parse(t, "DROP INDEX billing.i CASCADE")) + require.True(t, refused) + assert.Equal(t, verdict.ReasonIndexStatement, v.Reason) + proof, err := v.Refusal() + require.NoError(t, err) + assert.Equal(t, verdict.RefusalSiteIndexOther, proof.Site()) + assert.Empty(t, v.SaferIdiom, + "DROP INDEX CONCURRENTLY does not accept CASCADE, so it is not an equivalent to suggest") + }) + t.Run("already-concurrent maintenance carries no safer idiom", func(t *testing.T) { v, refused := Gate(parse(t, "DROP INDEX CONCURRENTLY billing.i")) require.True(t, refused) @@ -65,6 +76,22 @@ func TestGate(t *testing.T) { }) } +// PostgreSQL rejects DROP INDEX CONCURRENTLY ... CASCADE, so the refusal must +// not describe it as the safe concurrent idiom to run directly. +func TestGateConcurrentCascadeIsNotCalledSafe(t *testing.T) { + for _, sql := range []string{ + "DROP INDEX CONCURRENTLY billing.i CASCADE", + "DROP INDEX CONCURRENTLY billing.i, billing.j", + } { + st, err := statement.ParseOne(sql) + require.NoError(t, err) + v, refused := Gate(st) + require.True(t, refused, sql) + assert.Equal(t, verdict.ClassByDesign, v.Class, "%s: PostgreSQL rejects it, so it is not the safe concurrent idiom", sql) + assert.Empty(t, v.Owner, "%s: no direct operator is pointed at a statement PostgreSQL rejects", sql) + } +} + func TestRunRejectsUnrunnableOptions(t *testing.T) { st, err := statement.ParseOne("ALTER TABLE billing.invoices ADD COLUMN age int") require.NoError(t, err) diff --git a/pkg/migrate/refusal_registry.go b/pkg/migrate/refusal_registry.go index e5a8f54..3234c8c 100644 --- a/pkg/migrate/refusal_registry.go +++ b/pkg/migrate/refusal_registry.go @@ -111,9 +111,17 @@ func gateRefusal(kind statement.Kind, concurrent bool, target statement.IndexTar case statement.KindAlterTable, statement.KindCreateIndex: return verdict.Refusal{}, false case statement.KindDropIndex, statement.KindReindex: - if concurrent { + // A concurrent form is the safe idiom to run directly only when + // PostgreSQL accepts it: DROP INDEX CONCURRENTLY names exactly one + // index and takes no CASCADE, so a wider concurrent drop is refused + // as the plain one would be rather than pointed at a direct run the + // server rejects. + if concurrent && (kind == statement.KindReindex || target == statement.IndexTargetSingleRelation) { return verdict.NoOnlineSafetyProblem(verdict.ReasonIndexStatement, verdict.OwnerDirectOperator), true } + // INV: AB-4 — only a statement whose whole scope is one relation is + // at the eligible site; a cascading drop is not, so the + // acknowledgement cannot reach it. site := verdict.RefusalSiteIndexOther if target == statement.IndexTargetSingleRelation { site = verdict.RefusalSiteIndexSingleRelation diff --git a/pkg/migrate/verdicts.go b/pkg/migrate/verdicts.go index 42848fc..7fb020a 100644 --- a/pkg/migrate/verdicts.go +++ b/pkg/migrate/verdicts.go @@ -46,8 +46,19 @@ func Gate(st statement.Statement) (verdict.Verdict, bool) { // forms carry no safer idiom: suggesting the statement the user submitted // would confuse a human once and send a resubmitting automation into a loop. func indexAdvice(st statement.Statement) (detail, saferIdiom string) { + // Cascade comes first: PostgreSQL rejects DROP INDEX CONCURRENTLY ... + // CASCADE, so that form is not the safe concurrent idiom. + if st.Cascades() { + return "DROP INDEX ... CASCADE also drops the objects that depend on the index, which can lock and change " + + "tables other than the index's own; DROP INDEX CONCURRENTLY does not accept CASCADE — " + + "drop each dependent object explicitly, then the index, or resubmit without CASCADE; " + + "PostgreSQL then refuses the drop if anything depends on the index", "" + } + if st.Concurrent() && st.Kind() == statement.KindDropIndex && st.IndexTarget() != statement.IndexTargetSingleRelation { + return "DROP INDEX CONCURRENTLY drops exactly one index; submit one statement per index and run each directly against the database", "" + } if st.Concurrent() { - return "this is already the safe concurrent idiom; pg-sprite does not drive this maintenance form yet — run it directly against the database", "" + return "this is already the safe concurrent idiom; pg-sprite does not drive this maintenance form — run it directly against the database", "" } switch st.Kind() { case statement.KindDropIndex: diff --git a/pkg/statement/statement.go b/pkg/statement/statement.go index 77e75ae..c2d3252 100644 --- a/pkg/statement/statement.go +++ b/pkg/statement/statement.go @@ -91,21 +91,29 @@ type Statement struct { concurrent bool indexTarget IndexTarget indexRelation IndexRelation + cascades bool buildsIndex bool } -// IndexTarget identifies whether an index-maintenance statement names one -// relation. The gate uses this parse-only fact to keep forms requiring more -// than one lock acknowledgement out of accepted-blocking eligibility. +// IndexTarget identifies whether an index-maintenance statement's scope +// stays within one relation. IndexTargetOther means the scope is wider than +// one relation, for any reason — several indexes, a schema or database +// scope, or a cascading drop (see Cascades()) — so it is not a count of the +// relations the statement names. The gate uses this parse-only fact to keep +// forms requiring more than one lock acknowledgement out of +// accepted-blocking eligibility. type IndexTarget int const ( // IndexTargetNone is carried by statements outside index maintenance. IndexTargetNone IndexTarget = iota - // IndexTargetSingleRelation covers one DROP INDEX and REINDEX INDEX or TABLE. + // IndexTargetSingleRelation covers one DROP INDEX (without CASCADE) and + // REINDEX INDEX or TABLE. IndexTargetSingleRelation - // IndexTargetOther covers multi-index DROP and REINDEX scopes that do not - // name one relation: SCHEMA, DATABASE, and SYSTEM. + // IndexTargetOther covers the DROP and REINDEX scopes that do not stay + // within one relation: several indexes in one DROP INDEX, DROP INDEX with + // CASCADE (which also drops dependent objects), and REINDEX SCHEMA, + // DATABASE, and SYSTEM. IndexTargetOther ) @@ -178,10 +186,16 @@ func (s Statement) Concurrent() bool { return s.concurrent } // IndexTarget returns the typed target shape of an index-maintenance statement. func (s Statement) IndexTarget() IndexTarget { return s.indexTarget } -// IndexRelation returns the relation a single-relation index-maintenance -// statement names; the zero value for every other statement. +// IndexRelation returns the relation an IndexTargetSingleRelation statement +// names; the zero value for every other statement, including an +// IndexTargetOther statement that names one index but whose scope is wider, +// such as DROP INDEX ... CASCADE (see Cascades()). func (s Statement) IndexRelation() IndexRelation { return s.indexRelation } +// Cascades reports whether a DROP INDEX used CASCADE, which also drops the +// objects that depend on the index. It is always false for other statements. +func (s Statement) Cascades() bool { return s.cascades } + // BuildsIndex reports whether executing the statement creates a new index: // every CREATE INDEX, and the ALTER TABLE shapes that build one as a side // effect — ADD CONSTRAINT UNIQUE / PRIMARY KEY / EXCLUDE without USING @@ -288,8 +302,11 @@ func ParseOne(sql string) (Statement, error) { if node.GetDropStmt().GetRemoveType() == pganalyze.ObjectType_OBJECT_INDEX { st.kind = KindDropIndex st.concurrent = node.GetDropStmt().GetConcurrent() + st.cascades = node.GetDropStmt().GetBehavior() == pganalyze.DropBehavior_DROP_CASCADE objects := node.GetDropStmt().GetObjects() - if len(objects) == 1 { + // CASCADE also drops the objects that depend on the named index, + // so its scope is wider than the one relation it names. + if len(objects) == 1 && !st.cascades { st.indexTarget = IndexTargetSingleRelation st.indexRelation = droppedIndex(objects[0]) } else { diff --git a/pkg/statement/statement_test.go b/pkg/statement/statement_test.go index ef6379c..eed07fc 100644 --- a/pkg/statement/statement_test.go +++ b/pkg/statement/statement_test.go @@ -113,6 +113,20 @@ func TestParseOneKinds(t *testing.T) { sql: "DROP INDEX idx_a, idx_b", want: Statement{kind: KindDropIndex, indexTarget: IndexTargetOther}, }, + { + name: "drop index restrict is the default behavior spelled out", + sql: "DROP INDEX app.idx_users_email RESTRICT", + want: Statement{kind: KindDropIndex, indexTarget: IndexTargetSingleRelation, + indexRelation: IndexRelation{Schema: "app", Name: "idx_users_email", Kind: IndexRelationIndex}}, + }, + { + // CASCADE also drops the objects that depend on the index, such as a + // foreign key on another table, so the statement's scope is not the + // one relation it names. + name: "drop index cascade names no single relation", + sql: "DROP INDEX app.idx_users_email CASCADE", + want: Statement{kind: KindDropIndex, indexTarget: IndexTargetOther, cascades: true}, + }, { name: "drop index concurrently", sql: "DROP INDEX CONCURRENTLY idx_users_email", diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index e2cdb4d..69dffd6 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -238,8 +238,8 @@ const ( // RefusalSiteIndexSingleRelation is the plain one-relation DROP INDEX, // REINDEX INDEX, or REINDEX TABLE gate site. RefusalSiteIndexSingleRelation RefusalSite = "index-statement-single-relation" - // RefusalSiteIndexOther is a multi-relation DROP INDEX or a REINDEX scope - // that does not identify one relation. + // RefusalSiteIndexOther is a multi-relation DROP INDEX, a DROP INDEX with + // CASCADE, or a REINDEX scope that does not identify one relation. RefusalSiteIndexOther RefusalSite = "index-statement-other" )