From 8f7ef16d37ae77eb7a673c54fba690917092ef03 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 29 Oct 2026 10:03:59 +1100 Subject: [PATCH 1/4] migrate: require --allow-destructive for an accepted DROP INDEX --accept-blocking accepts the lock, not the loss of the index. A dropped index the application still reads through is a common mistake, so the refusal lists the checks to make before acknowledging it. --- README.md | 8 ++- demo/tour.sh | 47 +++++++++++---- docs/capabilities.md | 9 ++- docs/cli-output-examples.md | 9 ++- docs/invariants.md | 13 ++-- docs/limitations.md | 4 +- docs/lock-budgeted-passthrough.md | 8 +++ docs/refusal-classes.md | 2 +- docs/schemabot-integration.md | 4 +- .../cli/accept_blocking_integration_test.go | 7 ++- internal/cli/allow_destructive_test.go | 12 ++-- internal/cli/cli.go | 13 ++-- internal/cli/docs_test.go | 1 + pkg/capabilities/capabilities.yaml | 2 +- pkg/migrate/accept_blocking.go | 16 +++++ .../accept_blocking_integration_test.go | 59 ++++++++++++++++++- pkg/migrate/allow_destructive.go | 29 +++++++-- .../allow_destructive_integration_test.go | 4 +- pkg/migrate/allow_destructive_test.go | 15 ----- pkg/migrate/migrate.go | 28 ++++----- 20 files changed, 207 insertions(+), 83 deletions(-) diff --git a/README.md b/README.md index 7641db99..29e82a02 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,10 @@ refusal — never a silently wrong or incomplete result: `--allow-destructive SCHEMA.TABLE` names the table; the acknowledged run is audited and its verdict carries `allowed_destructive: true`. A drop that adds the same column back gets the guidance to change its type in place, - which keeps its values. + which keeps its values. A `DROP INDEX` run under `--accept-blocking` needs + `--allow-destructive` too: accepting the lock is not accepting the loss, + and the refusal lists the checks (`idx_scan` on every replica, foreign + keys, uniqueness) that catch an index the application still uses. - **Destructive desired-state plans are refused as a whole** — `migrate --desired` creates missing tables and converges supported changes, but never infers permission to discard live structure. A new primary key or unique constraint over a live @@ -150,7 +153,8 @@ refusal — never a silently wrong or incomplete result: `migrate --accept-blocking SCHEMA.TABLE` runs an otherwise-refused plain `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 + the verdict marked `executed-without-online-safety` (exit 3). A `DROP INDEX` + also needs `--allow-destructive` naming the same table. 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). diff --git a/demo/tour.sh b/demo/tour.sh index ffea6e8a..b19e3eba 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -203,31 +203,55 @@ execute_refused() { fi } -# execute_accepted table sql +# execute_accepted table allowed_destructive sql # # Runs a gate-refused blocking index statement under the --accept-blocking -# acknowledgement and requires the without-online-safety verdict: its own +# acknowledgement plus --allow-destructive (which a DROP INDEX needs and a +# REINDEX ignores), and requires the without-online-safety verdict: its own # exit code 3 (never 0, which stays exclusive to online-safe execution), -# the passthrough marker, and the table the acknowledgement named. The -# statement budget is spelled on the command line because the flag refuses -# to inherit the default as the accepted outage bound. +# the passthrough marker, whether the verdict records an acknowledged loss, +# and the table the acknowledgements named. The statement budget is spelled +# on the command line because the flag refuses to inherit the default as +# the accepted outage bound. execute_accepted() { - local table="$1" sql="$2" out status=0 - step "execute (accept blocking on $table): $sql" + local table="$1" allowed="$2" sql="$3" out status=0 + step "execute (accept blocking and the loss on $table): $sql" if [ "$CHECK" = 1 ]; then out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql" \ - --accept-blocking "$table" --statement-timeout 30s) || status=$? + --accept-blocking "$table" --allow-destructive "$table" --statement-timeout 30s) || status=$? assert_eq "accepted exit of [$sql]" 3 "$status" assert_eq "outcome of [$sql]" "executed-without-online-safety" "$(jq -r '.outcome' <<<"$out")" assert_eq "blocking_passthrough of [$sql]" true "$(jq -r '.blocking_passthrough' <<<"$out")" + assert_eq "allowed_destructive of [$sql]" "$allowed" "$(jq -r '.allowed_destructive // false' <<<"$out")" assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" assert_eq "statement_timeout of [$sql]" 30s "$(jq -r '.statement_timeout' <<<"$out")" else - "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --statement-timeout 30s \ + "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --allow-destructive "$table" \ + --statement-timeout 30s \ || echo "(exit $? — expected 3: committed without an online-safety guarantee)" fi } +# execute_accepted_needs_loss table sql +# +# The same DROP INDEX under --accept-blocking alone: accepting the lock is +# not accepting the loss, so it is refused (exit 2, destructive-change) and +# nothing runs. +execute_accepted_needs_loss() { + local table="$1" sql="$2" out status=0 + step "execute (accept blocking only on $table): $sql" + if [ "$CHECK" = 1 ]; then + out=$("$PGS" migrate --url "$PG_DSN" --json --alter "$sql" \ + --accept-blocking "$table" --statement-timeout 30s) || status=$? + assert_eq "refusal exit of [$sql]" 2 "$status" + assert_eq "reason of [$sql]" "destructive-change" "$(jq -r '.reason' <<<"$out")" + assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" + else + "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --statement-timeout 30s \ + || echo "(refused, exit $? — expected 2: a dropped index also needs --allow-destructive)" + fi +} + # execute_accept_mismatch wrong_table sql # # The acknowledgement names the table the statement locks; naming another @@ -486,8 +510,9 @@ run_exec() { # runs first so the index is still there to drop afterwards. execute_refused index-statement "DROP INDEX orders_total_idx" execute_accept_mismatch public.users "DROP INDEX orders_total_idx" - execute_accepted public.orders "DROP INDEX orders_total_idx" - execute_accepted public.orders "REINDEX TABLE orders" + execute_accepted_needs_loss public.orders "DROP INDEX orders_total_idx" + execute_accepted public.orders true "DROP INDEX orders_total_idx" + execute_accepted public.orders false "REINDEX TABLE orders" # A drop discards the column's values, so it is refused until the # acknowledgement names the table, then runs natively. refused_destructive "ALTER TABLE users DROP COLUMN status" diff --git a/docs/capabilities.md b/docs/capabilities.md index 68328175..25985f0c 100644 --- a/docs/capabilities.md +++ b/docs/capabilities.md @@ -118,7 +118,8 @@ them is a limitation: silently run the blocking form. Index maintenance (`DROP INDEX`, `REINDEX`) is the second case: the online form is a plain `CONCURRENTLY` statement, which `migrate` refuses to wrap and names so you run it directly; for a single-relation plain form, - `--accept-blocking` runs it under budgets and exits 3. + `--accept-blocking` runs it under budgets and exits 3 (a `DROP INDEX` also needs + `--allow-destructive`). Every matrix table carries an **"Online-safety problem?"** column: "Yes" means there is a readers-and-writers problem for an online engine to solve (pg-sprite solves it, plans to, @@ -240,7 +241,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` 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`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE` | +| `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` plus `--allow-destructive SCHEMA.TABLE` for the loss (exit 3, never 0; without the second acknowledgement it is refused with `destructive-change`); `--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`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE` | | `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 | | `ALTER INDEX ... RENAME TO` | ✅ | native, as-is | Yes | Metadata-only: application SQL names tables and columns, not indexes. `migrate` resolves the index's owning table from the catalog, preflights and privilege-checks that table, and runs the rename under the brief budgets after locking the table in `SHARE UPDATE EXCLUSIVE` and re-checking that the name is still a plain index of it, then confirms before the commit that the index now carrying the new name is the one it checked, since the rename locks only the index and another session can move the name meanwhile (else the permanent `renamed-index-drift` code, nothing renamed — `ALTER INDEX ... RENAME` would also rename a table). An index that backs a primary-key, unique, or exclusion constraint renames the constraint, which `ON CONFLICT ON CONSTRAINT` names, so it is refused as `rewrite-required` (`app-breaking-rename`, guidance `coordinated-constraint-rename`); `--force` does not apply to an index rename. Offline `lint` cannot tell the two kinds of index apart and reports neither. A desired file cannot express a rename | | 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 | @@ -371,7 +372,9 @@ unmistakably marked as executed without an online-safety guarantee 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` (never `CASCADE`, which also drops dependent objects such -as a foreign key on another table), `REINDEX INDEX`, or `REINDEX TABLE`. Refusals for +as a foreign key on another table), `REINDEX INDEX`, or `REINDEX TABLE`. A `DROP INDEX` +also discards the index, so it needs `--allow-destructive` naming the same table; without +it the drop is refused with `destructive-change` before anything runs. 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/cli-output-examples.md b/docs/cli-output-examples.md index 6d2eae7d..97fc310d 100644 --- a/docs/cli-output-examples.md +++ b/docs/cli-output-examples.md @@ -264,7 +264,10 @@ An operator-accepted blocking refusal has a distinct marked outcome and exit takes the schema-qualified table whose lock the operator accepts, must match the index's owning table exactly (a mismatch executes nothing and exits 1), and requires an explicit `--statement-timeout`; it cannot be combined with -`--force` or `--dry-run`. The text rendering marks the outcome as a warning: +`--force` or `--dry-run`. A `DROP INDEX` also discards the index, so it needs +`--allow-destructive` naming the same table; without it the drop is refused +with `destructive-change` (exit 2) and nothing runs. The text rendering marks +the outcome as a warning: ```text executed without online safety (accepted blocking refusal) @@ -273,10 +276,11 @@ executed without online safety (accepted blocking refusal) statement: DROP INDEX public.users_email_idx safer: DROP INDEX CONCURRENTLY budgets: lock 3s, statement 10m + allowed: the statement's data loss was acknowledged (allow-destructive) ``` ```console -$ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blocking public.users --lock-timeout 3s --statement-timeout 10m --json +$ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blocking public.users --allow-destructive public.users --lock-timeout 3s --statement-timeout 10m --json { "outcome": "executed-without-online-safety", "reason": "index-statement", @@ -284,6 +288,7 @@ $ pg-sprite migrate --alter 'DROP INDEX public.users_email_idx' --accept-blockin "statement": "DROP INDEX public.users_email_idx", "table": "public.users", "safer_idiom": "DROP INDEX CONCURRENTLY", + "allowed_destructive": true, "blocking_passthrough": true, "lock_timeout": "3s", "statement_timeout": "10m" diff --git a/docs/invariants.md b/docs/invariants.md index 63218c22..dfb8c263 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -851,7 +851,8 @@ would without the flag — including a `DROP INDEX ... CASCADE`, whose refusal s 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 +refusal as it stands rather than the accepted execution. It accepts the lock only: an +accepted `DROP INDEX` also needs the destructive acknowledgement (RF-10). 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`, `migrate.gateRefusal` (the refusal site), `TestAcceptedRefusal`, `TestAcceptBlockingFlagGrammar`, @@ -1400,8 +1401,11 @@ Each refusal is a preflight **error with a stated reason** — never a warning, other gate, and an acknowledged run is audited at warn level and carries `allowed_destructive: true` on its verdict. The declarative path takes no acknowledgement and refuses a destructive plan whole. A planner refusal runs nothing and keeps its own - reason. A `DROP INDEX` stays with the statement-kind gate and `--accept-blocking`, which - the destructive acknowledgement does not reach. Outage-risk statements are covered by + reason. A `DROP INDEX` — unique or not — is gate-refused first; when `--accept-blocking` + accepts that refusal, the drop still runs only if `--allow-destructive` names the same + owning table, and otherwise it is refused with `destructive-change` before anything + executes, the detail listing the checks that catch an index the application still uses. + Accepting the lock is not accepting the loss. Outage-risk statements are covered by RF-5 and AB-1..AB-5 (`--force`, `--accept-blocking`) and by the LK-2 budgets. *Enforced:* `pkg/migrate` (`run`'s destructive gate through `destructiveOperations` and `destructiveChangeVerdict`, `checkAllowDestructiveAck`, `AdmitDestructive` for the dry @@ -1412,7 +1416,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `TestRunRejectsAMismatchedDestructiveAcknowledgement`, `TestRunRefusesAnUnacknowledgedDropColumnRoutedToCopyAndSwap`, `TestRunNeedsBothForceAndTheDestructiveAcknowledgement`, - `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, `TestAdmitDestructive`, + `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, + `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, `TestAdmitDestructive`, `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, `demo/tour.sh` (`refused_destructive`). *Source:* [design principles](design-principles.md#declarative-review-first-workflow) diff --git a/docs/limitations.md b/docs/limitations.md index 4fb40385..d617d6d7 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -10,12 +10,12 @@ not escape hatches: | Change | Current behavior | | --- | --- | -| `DROP COLUMN`, `DROP CONSTRAINT`, and `ALTER COLUMN ... DROP NOT NULL` on `migrate --alter` | Refused with `destructive-change` (class `by-design`, exit 2) before any route runs the statement — native, safer sequence, `--force`, or copy-and-swap — and `--dry-run` ends in the same refusal verdict: each discards data or a guarantee the schema was providing, and pg-sprite does not infer that the loss is intended. `--allow-destructive SCHEMA.TABLE` (`Options.AllowDestructive` as a library caller) names the resolved table and runs the statement through every other gate as usual; its verdict carries `allowed_destructive: true` and the run logs one warning naming the table. A mismatched acknowledgement executes nothing and exits 1. It acknowledges only the loss: a forced type rewrite that also drops a column needs `--force` too. It cannot be combined with `--accept-blocking` or `--desired`, and it does not reach `DROP INDEX`, which keeps its own refusal below. A statement that drops a column and adds one of the same name back is refused with the `alter-column-type-in-place` guidance: `ALTER COLUMN ... TYPE ... USING ...` keeps the values the drop would discard. A planner refusal of a drop (`replica-identity`, `dependent-view`, `cascading-drop`) keeps its own reason, since no acknowledgement lifts it. | +| `DROP COLUMN`, `DROP CONSTRAINT`, and `ALTER COLUMN ... DROP NOT NULL` on `migrate --alter` | Refused with `destructive-change` (class `by-design`, exit 2) before any route runs the statement — native, safer sequence, `--force`, or copy-and-swap — and `--dry-run` ends in the same refusal verdict: each discards data or a guarantee the schema was providing, and pg-sprite does not infer that the loss is intended. `--allow-destructive SCHEMA.TABLE` (`Options.AllowDestructive` as a library caller) names the resolved table and runs the statement through every other gate as usual; its verdict carries `allowed_destructive: true` and the run logs one warning naming the table. A mismatched acknowledgement executes nothing and exits 1. It acknowledges only the loss: a forced type rewrite that also drops a column needs `--force` too. It cannot be combined with `--desired`. It does not lift a `DROP INDEX` refusal on its own; it is the second acknowledgement a `DROP INDEX` under `--accept-blocking` needs (below). A statement that drops a column and adds one of the same name back is refused with the `alter-column-type-in-place` guidance: `ALTER COLUMN ... TYPE ... USING ...` keeps the values the drop would discard. A planner refusal of a drop (`replica-identity`, `dependent-view`, `cascading-drop`) keeps its own reason, since no acknowledgement lifts it. | | Index builds on a partitioned parent | PostgreSQL cannot build an index concurrently at the parent level. PostgreSQL supports a plain blocking build, but pg-sprite refuses it by policy because it takes `ACCESS EXCLUSIVE`; `--force` does not bypass this decision. `migrate --accept-blocking` does not reach this refusal yet either — it covers the single-relation `DROP INDEX` / `REINDEX` refusals — so an operator who chooses a maintenance-window blocking build runs it outside the CLI today; a library caller can run it through `executor.ExecuteAcceptedBlocking`, which marks the verdict as executed without online safety ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). The partition-aware `CREATE INDEX ON ONLY` → per-partition `CREATE INDEX CONCURRENTLY` → `ATTACH PARTITION` flow is planned but not yet implemented. | | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | | `ADD PRIMARY KEY` / `ADD UNIQUE` with a storage parameter the online sequence cannot render | The online sequence builds the key's index with `CREATE UNIQUE INDEX CONCURRENTLY` and carries the key's `WITH (...)` parameters onto it, each name quoted and each value a quoted literal. A parameter it cannot render faithfully, one whose value is not a plain word, number, or string, contains a backslash, or whose name is namespaced, would be dropped or changed, so no sequence is built. Running the statement as written builds the index under `ACCESS EXCLUSIVE`, so it is refused as `rewrite-required` with the `unique-index-then-constraint` guidance. Build the index concurrently with the parameter, then attach it with `ADD CONSTRAINT ... USING INDEX`. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | -| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | +| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). A `DROP INDEX`, unique or not, also needs `--allow-destructive` naming the same table: without it the drop is refused with `destructive-change` (exit 2) before anything runs, and the detail lists the checks to make first — `idx_scan` is 0 on the primary and every replica over a full application cycle, no foreign key relies on the index, and nothing needs a unique index's guarantee. `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | | `RENAME COLUMN` and `RENAME TO` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the catalog flip is instant, but every running application instance that still queries the old name breaks the moment it commits, and no safer sequence can be built from the statement alone because the dual-write and the read switch live in the application. The plan report, `suggest`, and the verdict carry the manual path as typed guidance — `expand-contract-column` for a column (add the new column, dual-write and backfill, switch reads, drop the old column as its own reviewed change) and `coordinated-table-rename` for a table (coordinate the rename with the deploy that adopts the new name, or keep the old name readable through a view until every instance has moved) — and `lint` reports the rename as an error. `--force SCHEMA.TABLE` runs the submitted rename as a blind bounded attempt under the usual budgets and size guard, for the operator who has coordinated the deploy; on a table above `--max-table-size` (default 1 GiB) the forced rename is refused as `not-native-safe-table-too-large` until that threshold is raised. | | `ALTER INDEX ... RENAME TO` on an index that backs a primary-key, unique, or exclusion constraint | Refused with `not-native-safe-rewrite-required`, class `by-design`, guidance `coordinated-constraint-rename`, nothing executed: PostgreSQL renames the constraint with its index, and `ON CONFLICT ON CONSTRAINT` and error handling that name the old constraint break the moment it commits. `--force` does not apply to an index rename. A plain index rename runs. The refusal needs the catalog, so offline `lint` and `suggest` do not report it. | | `ATTACH PARTITION` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the engine does not yet construct the validated `CHECK` that lets the attach skip its scan. Pre-prove the bound yourself, then run the attach directly. `--force SCHEMA.TABLE` runs the blocking `ATTACH PARTITION` as submitted under bounded budgets, taking the blocking lock. | diff --git a/docs/lock-budgeted-passthrough.md b/docs/lock-budgeted-passthrough.md index 96e6debd..387a71a8 100644 --- a/docs/lock-budgeted-passthrough.md +++ b/docs/lock-budgeted-passthrough.md @@ -133,10 +133,18 @@ a bare boolean: pg-sprite migrate \ --alter 'DROP INDEX app.orders_created_at_idx' \ --accept-blocking app.orders \ + --allow-destructive app.orders \ --lock-timeout 3s \ --statement-timeout 10m ``` +A `DROP INDEX` needs both acknowledgements, each naming the owning table: `--accept-blocking` +accepts the lock, and `--allow-destructive` accepts losing the index (RF-10). The second is +checked after the owning table resolves and before anything executes; without it the drop is +refused with `destructive-change`, and the detail lists the checks that catch an index the +application still uses. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only +`--accept-blocking`. + The value names the table whose lock the operator accepts, because that is the relation the blocking form makes unavailable: a plain `DROP INDEX` takes `ACCESS EXCLUSIVE` on the index's table, and a parent index build locks the parent. The partitioned-parent statements and diff --git a/docs/refusal-classes.md b/docs/refusal-classes.md index cd833aa1..fcd0d265 100644 --- a/docs/refusal-classes.md +++ b/docs/refusal-classes.md @@ -84,7 +84,7 @@ routed to provisioning and never to the data change runner. | `not-native-safe-rewrite-required` | The submitted form blocks the table and the planner cannot construct the required safer sequence | `capability-boundary` | — | | `not-native-safe-rewrite-required` | A `RENAME COLUMN` or `RENAME TO` (`app-breaking-rename`): no SQL exists to substitute, and the deliberate path is the `guidance` rollout and then the statement under `--force` | `by-design` | — | | `backend-unavailable` | The plan routes to copy-and-swap for an operation the engine does not carry yet | `capability-boundary` | — | -| `destructive-change` | Neither front door infers permission to discard live data or structure: desired-state admission refuses a destructive plan whole, and `migrate --alter` refuses a statement that drops a column, constraint, or `NOT NULL` until `--allow-destructive` names the table (RF-10) | `by-design` | — | +| `destructive-change` | Neither front door infers permission to discard live data or structure: desired-state admission refuses a destructive plan whole, and `migrate --alter` refuses a statement that drops a column, constraint, or `NOT NULL` — and a `DROP INDEX` under `--accept-blocking` — until `--allow-destructive` names the table (RF-10) | `by-design` | — | | `plan-fingerprint-mismatch` | The recomputed plan is not the reviewed plan | `environmental` | — | | `create-collision` | A target or claimed name is occupied at apply time by a relation or a standalone type | `environmental` | — | | `declarative-foreign-key` | A desired-state plan adds or drops a foreign key, or creates a table that declares one; desired-state execution does not run foreign-key changes yet | `capability-boundary` | — | diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 1075dbac..4b1f46a0 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -631,7 +631,9 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: acknowledgement is `migrate.ErrAllowDestructiveMismatch` with a zero verdict and nothing executed. An acknowledged run's verdict carries `allowed_destructive: true`, the machine-readable audit record to store beside the operator's consent. The acknowledgement - covers only the loss: a statement that also needs `Force` still needs it. `RunDesired` + covers only the loss: a statement that also needs `Force` still needs it, and a `DROP INDEX` + run with `Options.AcceptBlocking` needs both, each naming the index's owning table. + `RunDesired` rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a destructive plan whole, so a declarative drop the operator approves runs as its own imperative statement. diff --git a/internal/cli/accept_blocking_integration_test.go b/internal/cli/accept_blocking_integration_test.go index af4943c9..9dec9657 100644 --- a/internal/cli/accept_blocking_integration_test.go +++ b/internal/cli/accept_blocking_integration_test.go @@ -15,8 +15,9 @@ import ( "github.com/block/pg-sprite/pkg/verdict" ) -// --accept-blocking turns the gate refusal of a plain DROP INDEX into a -// bounded blocking execution: the command dials, runs the statement as +// --accept-blocking, with --allow-destructive for the dropped index, turns +// the gate refusal of a plain DROP INDEX into a bounded blocking execution: +// the command dials, runs the statement as // submitted, prints the without-online-safety verdict, and returns the // sentinel the entry point maps to its own exit code — never the refusal's // and never the plain success's. @@ -36,6 +37,7 @@ func TestMigrateAcceptBlockingRunsDropIndex(t *testing.T) { cmd := newMigrateCmd(url, fmt.Sprintf("DROP INDEX %s.users_email_idx", schema)) cmd.AcceptBlocking = schema + ".users" + cmd.AllowDestructive = schema + ".users" cmd.JSON = true var out strings.Builder err = cmd.run(t.Context(), &out) @@ -46,6 +48,7 @@ func TestMigrateAcceptBlockingRunsDropIndex(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(out.String()), &v)) assert.Equal(t, verdict.OutcomeExecutedWithoutOnlineSafety, v.Outcome) assert.True(t, v.BlockingPassthrough) + assert.True(t, v.AllowedDestructive) assert.Equal(t, schema+".users", v.Table) assert.Equal(t, verdict.ReasonIndexStatement, v.Reason) assert.Equal(t, "3s", v.LockTimeout) diff --git a/internal/cli/allow_destructive_test.go b/internal/cli/allow_destructive_test.go index 1518c40d..12dd2171 100644 --- a/internal/cli/allow_destructive_test.go +++ b/internal/cli/allow_destructive_test.go @@ -10,9 +10,9 @@ import ( // --allow-destructive acknowledges data loss, a risk none of the other // acknowledgements covers, so the grammar combines it with --force, -// --dry-run, and --restart, and refuses it beside --accept-blocking, the -// separate policy path for refused index statements, and beside --desired, -// which refuses destructive plans whole. +// --dry-run, --restart, and --accept-blocking (an accepted DROP INDEX +// needs both), and refuses it beside --desired, which refuses destructive +// plans whole. func TestAllowDestructiveFlagGrammar(t *testing.T) { parse := func(args ...string) error { c := New("test") @@ -43,10 +43,8 @@ func TestAllowDestructiveFlagGrammar(t *testing.T) { require.NoError(t, parse("--restart", "public.orders")) }) - t.Run("rejects --accept-blocking", func(t *testing.T) { - err := parse("--accept-blocking", "public.orders", "--statement-timeout", "10m") - require.Error(t, err) - assert.Contains(t, err.Error(), "--allow-destructive cannot be combined with --accept-blocking") + t.Run("combines with --accept-blocking", func(t *testing.T) { + require.NoError(t, parse("--accept-blocking", "public.orders", "--statement-timeout", "10m")) }) t.Run("rejects --desired", func(t *testing.T) { diff --git a/internal/cli/cli.go b/internal/cli/cli.go index b64862f0..f7fa9e43 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -112,8 +112,8 @@ 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 (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"` - AllowDestructive string `help:"Run a statement that discards live data or structure: DROP COLUMN, DROP CONSTRAINT, or ALTER COLUMN ... DROP NOT NULL. Without it such a statement is refused (exit 2, reason destructive-change) before anything runs, on every route it could take. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly. It acknowledges only the loss: the statement is still classified, routed, preflighted, size-guarded, and budget-bounded, and a statement that also needs --force needs both. With --dry-run the plan is reported as the acknowledged run would take it. Cannot be combined with --accept-blocking or --desired." name:"allow-destructive" 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. A DROP INDEX also needs --allow-destructive naming the same table, or it is refused (exit 2, reason destructive-change). Cannot be combined with --force or --dry-run." placeholder:"SCHEMA.TABLE"` + AllowDestructive string `help:"Run a statement that discards live data or structure: DROP COLUMN, DROP CONSTRAINT, ALTER COLUMN ... DROP NOT NULL, or a DROP INDEX run under --accept-blocking. Without it such a statement is refused (exit 2, reason destructive-change) before anything runs, on every route it could take. The value is the typed acknowledgement: it must name the resolved schema-qualified target table exactly (for a DROP INDEX, the index's owning table). It acknowledges only the loss: the statement is still classified, routed, preflighted, size-guarded, and budget-bounded, and a statement that also needs --force or --accept-blocking needs both. With --dry-run the plan is reported as the acknowledged run would take it. Cannot be combined with --desired." name:"allow-destructive" 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"` @@ -144,9 +144,9 @@ type MigrateCmd struct { // copy-and-swap. --allow-destructive acknowledges data loss, a different // risk from the one --force accepts, so the two combine; a dry run with it // reports the plan the acknowledged run would take, and a restart re-runs -// the same statement, so both combine too. It cannot ride with -// --accept-blocking, a separate policy path the statement-kind gate takes -// before the destructive gate is reached. +// the same statement, so both combine too. It also combines with +// --accept-blocking, which an accepted DROP INDEX needs both of: one +// accepts the lock, the other the loss. func (c *MigrateCmd) Validate(kctx *kong.Context) error { if err := c.validateInput(); err != nil { return err @@ -160,9 +160,6 @@ func (c *MigrateCmd) Validate(kctx *kong.Context) error { if c.DryRun && c.AcceptBlocking != "" { return errors.New("--accept-blocking cannot be combined with --dry-run: the dry run reports the refusal as it stands") } - if c.AllowDestructive != "" && c.AcceptBlocking != "" { - return errors.New("--allow-destructive cannot be combined with --accept-blocking: accepted blocking is a separate policy path for refused index statements, which the destructive acknowledgement does not govern") - } if c.Force != "" && c.AcceptBlocking != "" { return errors.New("--accept-blocking cannot be combined with --force: each acknowledges a different decision, and a statement reaches one") } diff --git a/internal/cli/docs_test.go b/internal/cli/docs_test.go index 6e9578ee..54c81745 100644 --- a/internal/cli/docs_test.go +++ b/internal/cli/docs_test.go @@ -100,6 +100,7 @@ func TestCLIOutputExamplesMatchPipelineOutput(t *testing.T) { verdict.ByDesign(verdict.ReasonIndexStatement).WithSite(verdict.RefusalSiteIndexSingleRelation), 3*time.Second, 10*time.Minute) require.NoError(t, err) + acceptedBlocking.AllowedDestructive = true rewriteRequired := alterReport(t, "ALTER TABLE users ADD COLUMN nickname text UNIQUE", "public", "users", usersFacts()) copyAndSwapRouted := alterReport(t, "ALTER TABLE users ALTER COLUMN id TYPE text", diff --git a/pkg/capabilities/capabilities.yaml b/pkg/capabilities/capabilities.yaml index 068ffda1..2ebba8e0 100644 --- a/pkg/capabilities/capabilities.yaml +++ b/pkg/capabilities/capabilities.yaml @@ -242,7 +242,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` 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`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE`" + 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` plus `--allow-destructive SCHEMA.TABLE` for the loss (exit 3, never 0; without the second acknowledgement it is refused with `destructive-change`); `--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`. `plan` and `lint` refuse `DROP INDEX … CASCADE` with `cascading-drop`, since `DROP INDEX CONCURRENTLY` does not accept `CASCADE`" - id: reindex area: "indexes" operation: "`REINDEX`" diff --git a/pkg/migrate/accept_blocking.go b/pkg/migrate/accept_blocking.go index 4dbb4647..ad1883be 100644 --- a/pkg/migrate/accept_blocking.go +++ b/pkg/migrate/accept_blocking.go @@ -71,6 +71,21 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem return verdict.Verdict{}, fmt.Errorf("%w: %s locks %q, got %q; nothing was executed", ErrAcceptBlockingMismatch, st.Kind(), table, opts.AcceptBlocking) } + if opts.AllowDestructive != "" { + if err := checkAllowDestructiveAck(table, opts.AllowDestructive); err != nil { + return verdict.Verdict{}, err + } + } + // INV: RF-10 — accepting the lock is not accepting the loss: a dropped + // index runs only when the destructive acknowledgement also names its + // table. + dropsIndex := st.Kind() == statement.KindDropIndex + if dropsIndex { + if opts.AllowDestructive == "" { + return dropIndexDestructiveVerdict(st, table), nil + } + auditAllowDestructive(opts.audit(), st, table) + } budget := executor.BlockingBudget{ LockTimeout: opts.Budget.Brief.LockTimeout, StatementTimeout: opts.Budget.Brief.StatementTimeout, @@ -131,6 +146,7 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem return verdict.Verdict{}, fmt.Errorf("%w: %w", executor.ErrInvariantViolation, err) } v.Table = table + v.AllowedDestructive = dropsIndex v.Detail = fmt.Sprintf("the operator accepted the blocking form: it ran as-is under budgets (lock %s, statement %s) "+ "with no online-safety guarantee; %s was locked for the statement's duration", budget.LockTimeout, budget.StatementTimeout, table) diff --git a/pkg/migrate/accept_blocking_integration_test.go b/pkg/migrate/accept_blocking_integration_test.go index bd096e16..0ed8f957 100644 --- a/pkg/migrate/accept_blocking_integration_test.go +++ b/pkg/migrate/accept_blocking_integration_test.go @@ -19,10 +19,13 @@ import ( // acceptBlockingOptions is the runnable policy with the acknowledgement // set and the brief budgets tightened so a contended or slow statement -// resolves in well under a second. +// resolves in well under a second. The destructive acknowledgement names +// the same table, because an accepted DROP INDEX needs both; it changes +// nothing for a REINDEX, which discards nothing. func acceptBlockingOptions(table string) migrate.Options { opts := runOptions() opts.AcceptBlocking = table + opts.AllowDestructive = table opts.Budget.Brief = executor.Budget{LockTimeout: 100 * time.Millisecond, StatementTimeout: 5 * time.Second} return opts } @@ -80,6 +83,7 @@ func TestRunAcceptBlockingExecutesEligibleRefusals(t *testing.T) { acceptBlockingOptions(schema+".orders")) require.NoError(t, err) assertAccepted(t, v, schema) + assert.True(t, v.AllowedDestructive, "a dropped index is a destructive change the verdict records") assert.False(t, indexExists(t, pool, schema, "orders_created_idx"), "the accepted DROP INDEX must have committed") }) @@ -91,6 +95,7 @@ func TestRunAcceptBlockingExecutesEligibleRefusals(t *testing.T) { acceptBlockingOptions(schema+".orders")) require.NoError(t, err) assertAccepted(t, v, schema) + assert.False(t, v.AllowedDestructive, "a REINDEX discards nothing") assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "REINDEX keeps the index") }) @@ -207,6 +212,58 @@ func TestRunAcceptBlockingRejectsFalseAcknowledgements(t *testing.T) { }) } +// Accepting the lock is not accepting the loss. A DROP INDEX under +// --accept-blocking alone is refused as a destructive change before +// anything runs, with the checks that catch an index the application still +// uses; a destructive acknowledgement naming another table is a usage +// error. The index here is a plain, non-unique one: the gate covers every +// index drop, not only the unique ones that also carry a guarantee. +func TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex(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.orders ( + id int PRIMARY KEY, + created_at timestamptz + ); + CREATE TABLE %[1]s.customers (id int PRIMARY KEY); + CREATE INDEX orders_created_idx ON %[1]s.orders (created_at)`, schema)) + require.NoError(t, err) + drop := parseOne(t, fmt.Sprintf("DROP INDEX %s.orders_created_idx", schema)) + + t.Run("refused without the destructive acknowledgement", func(t *testing.T) { + opts := acceptBlockingOptions(schema + ".orders") + opts.AllowDestructive = "" + + v, err := migrate.Run(t.Context(), pool, drop, opts) + + require.NoError(t, err) + assert.Equal(t, verdict.OutcomeRefused, v.Outcome) + assert.Equal(t, verdict.ReasonDestructiveChange, v.Reason) + assert.Equal(t, verdict.ClassByDesign, v.Class) + assert.Equal(t, schema+".orders", v.Table, "the refusal names the owning table the acknowledgement must spell") + assert.False(t, v.BlockingPassthrough) + assert.False(t, v.AllowedDestructive) + assert.Contains(t, v.Detail, "idx_scan") + assert.Contains(t, v.Detail, "--allow-destructive "+schema+".orders") + assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "the refused drop must execute nothing") + }) + + t.Run("a destructive acknowledgement naming another table executes nothing", func(t *testing.T) { + opts := acceptBlockingOptions(schema + ".orders") + opts.AllowDestructive = schema + ".customers" + + v, err := migrate.Run(t.Context(), pool, drop, opts) + + require.ErrorIs(t, err, migrate.ErrAllowDestructiveMismatch) + assert.Equal(t, verdict.Verdict{}, v) + assert.True(t, indexExists(t, pool, schema, "orders_created_idx")) + }) +} + // The caller's context ending while the accepted statement runs is an // outcome the client cannot establish: the statement was submitted and // the server decides whether its transaction rolled back. The verdict says diff --git a/pkg/migrate/allow_destructive.go b/pkg/migrate/allow_destructive.go index eadf2ba7..9edef635 100644 --- a/pkg/migrate/allow_destructive.go +++ b/pkg/migrate/allow_destructive.go @@ -128,12 +128,29 @@ func AdmitDestructive(st statement.Statement, schema string, rs router.Statement return v, true, nil } -// auditAllowDestructive records the acknowledged data loss before anything -// executes. The record is warn-level and unconditional, as the force audit -// is, and the verdict's AllowedDestructive field is its machine-readable -// twin. -func auditAllowDestructive(audit *slog.Logger, st statement.Statement) { +// dropIndexDestructiveVerdict is the refusal for an accepted blocking DROP +// INDEX without the destructive acknowledgement. Every index drop counts, +// not only a unique one: the common mistake is dropping an index the +// application still reads through, so the detail lists the checks that +// catch it before naming the acknowledgement. table is the index's owning +// table, resolved from the catalog. +func dropIndexDestructiveVerdict(st statement.Statement, table string) verdict.Verdict { + detail := "DROP INDEX discards an index that queries, foreign-key checks, or a uniqueness guarantee may " + + "still rely on, and undoing it is a full index build; before dropping, confirm that " + + "pg_stat_user_indexes.idx_scan for the index is 0 on the primary and on every replica (each instance " + + "keeps its own counters) over a window since stats_reset that covers the application's full cycle, " + + "that no foreign key's referencing columns rely on it, and, for a unique index, that nothing still " + + "needs the uniqueness it enforces — then rerun with --allow-destructive " + table + v := verdict.Verdict{Statement: st.SQL(), Table: table, Detail: detail} + return v.WithRefusal(destructiveChangeRefusal()) +} + +// auditAllowDestructive records the acknowledged data loss on table before +// anything executes. The record is warn-level and unconditional, as the +// force audit is, and the verdict's AllowedDestructive field is its +// machine-readable twin. +func auditAllowDestructive(audit *slog.Logger, st statement.Statement, table string) { audit.Warn("destructive change acknowledged; the statement will discard live data or structure", - "table", qualified(st), + "table", table, "kind", st.Kind().String()) } diff --git a/pkg/migrate/allow_destructive_integration_test.go b/pkg/migrate/allow_destructive_integration_test.go index 26fb7373..d2724149 100644 --- a/pkg/migrate/allow_destructive_integration_test.go +++ b/pkg/migrate/allow_destructive_integration_test.go @@ -369,9 +369,9 @@ func TestRunGuidesADropAndReAddToAnInPlaceTypeChange(t *testing.T) { assert.EqualValues(t, 500*501/2, total, "every qty value is still there") } -// The destructive acknowledgement does not reach a DROP INDEX: the +// The destructive acknowledgement alone does not lift a DROP INDEX: the // statement-kind gate refuses it first in favor of DROP INDEX -// CONCURRENTLY, and only --accept-blocking governs that refusal. +// CONCURRENTLY, and only --accept-blocking accepts that refusal. func TestRunAllowDestructiveDoesNotLiftADropIndexRefusal(t *testing.T) { f := newDestructiveFixture(t) opts := runOptions() diff --git a/pkg/migrate/allow_destructive_test.go b/pkg/migrate/allow_destructive_test.go index d6ce689e..bbf0908f 100644 --- a/pkg/migrate/allow_destructive_test.go +++ b/pkg/migrate/allow_destructive_test.go @@ -26,21 +26,6 @@ func routeOne(t *testing.T, sql string) (statement.Statement, router.Statement) return st, router.Route([]planner.Plan{classified}).Statements[0] } -// The destructive acknowledgement and accepted blocking are separate -// policy paths; both at once is rejected before any database work. -func TestRunRejectsAllowDestructiveWithAcceptBlocking(t *testing.T) { - st, err := statement.ParseOne("DROP INDEX app.orders_created_idx") - require.NoError(t, err) - opts := DefaultOptions() - opts.AllowDestructive = "app.orders" - opts.AcceptBlocking = "app.orders" - - v, err := Run(t.Context(), nil, st, opts) - require.Error(t, err) - assert.Contains(t, err.Error(), "AllowDestructive") - assert.Equal(t, verdict.Verdict{}, v) -} - // Desired-state execution refuses a destructive plan whole, so the // acknowledgement has nothing to acknowledge there and is rejected as a // typed sentinel before any database work. diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 845fecb2..9d2a54ba 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -82,19 +82,19 @@ type Options struct { // from the catalog — exactly; empty means every refusal stands. // Ineligible refusals stand regardless. Force and AcceptBlocking are // mutually exclusive: a statement reaches one routing decision, and - // one acknowledgement names it. + // one acknowledgement names it. An accepted DROP INDEX also needs + // AllowDestructive: accepting the lock is not accepting the loss. AcceptBlocking string // AllowDestructive is the typed acknowledgement that the statement may - // discard live data or structure: a dropped column, constraint, or NOT - // NULL. Without it such a statement is refused before anything runs, - // with reason destructive-change, on every route it could take. It must - // name the resolved schema-qualified target table exactly; empty means + // discard live data or structure: a dropped column, constraint, NOT + // NULL, or index. Without it such a statement is refused before + // anything runs, with reason destructive-change, on every route it + // could take. It must name the resolved schema-qualified target table + // exactly — for a DROP INDEX, the index's owning table; empty means // destructive statements are refused. It acknowledges only the loss: // the statement still meets every other gate, so a statement that also - // needs Force needs both. It cannot be combined with AcceptBlocking, - // whose statements the statement-kind gate refuses before this one, and - // [RunDesired] rejects it. + // needs Force or AcceptBlocking needs both. [RunDesired] rejects it. AllowDestructive string // MaxTableSizeBytes is the threshold above which a blind bounded @@ -182,9 +182,6 @@ func (o Options) validate() error { if o.Force != "" && o.AcceptBlocking != "" { return errors.New("migrate: Options.Force and Options.AcceptBlocking are mutually exclusive; a statement reaches one routing decision and one acknowledgement names it") } - if o.AllowDestructive != "" && o.AcceptBlocking != "" { - return errors.New("migrate: Options.AllowDestructive and Options.AcceptBlocking are mutually exclusive; accepted blocking is a separate policy path the destructive acknowledgement does not govern") - } if o.CopySwap.Restart != "" && (o.Force != "" || o.AcceptBlocking != "") { return errors.New("migrate: Options.CopySwap.Restart acknowledges a copy-and-swap run starting over, which Options.Force and Options.AcceptBlocking both route away from; name one acknowledgement") } @@ -224,9 +221,10 @@ func (o Options) retry() executor.RetryPolicy { // checkpoint row the next Run over the same statement resumes from. // // A statement that discards live data or structure — a dropped column, -// constraint, or NOT NULL — is refused with reason destructive-change -// before any route runs it, unless Options.AllowDestructive names its -// table; an acknowledged run's verdict carries AllowedDestructive. +// constraint, NOT NULL, or (on the accepted blocking path) index — is +// refused with reason destructive-change before any route runs it, unless +// Options.AllowDestructive names its table; an acknowledged run's verdict +// carries AllowedDestructive. // // A bare table name resolves on the pool's own search_path, so two pools // with different paths can address different tables from the same @@ -319,7 +317,7 @@ func run(ctx context.Context, pool *pgxpool.Pool, st statement.Statement, opts O return destructiveChangeVerdict(st, destructive) } allowedDestructive = true - auditAllowDestructive(opts.audit(), st) + auditAllowDestructive(opts.audit(), st, qualified(st)) } if opts.CopySwap.Restart != "" { auditRestart(opts.audit(), st) From b84271d643a5ee55c83c0fb29d3e63e1c5c84335 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Thu, 29 Oct 2026 10:34:42 +1100 Subject: [PATCH 2/4] migrate: show the index's live usage when refusing a DROP INDEX The refusal asks the operator to confirm the index is unused; it now carries the connected server's idx_scan, stats_reset, last scan and uniqueness as evidence, as a typed index_usage field. --- README.md | 3 +- SAFETY.md | 2 +- demo/tour.sh | 1 + docs/invariants.md | 6 +- docs/limitations.md | 2 +- docs/lock-budgeted-passthrough.md | 7 +- docs/schemabot-integration.md | 4 +- docs/table-usage.md | 5 +- docs/testing.md | 1 + internal/cli/color_test.go | 1 + internal/cli/verdict_text.go | 3 + pkg/migrate/accept_blocking.go | 15 ++- .../accept_blocking_integration_test.go | 7 ++ pkg/migrate/allow_destructive.go | 54 +++++++-- pkg/preflight/index_usage.go | 111 ++++++++++++++++++ pkg/preflight/index_usage_integration_test.go | 85 ++++++++++++++ pkg/verdict/verdict.go | 47 ++++++++ pkg/verdict/verdict_test.go | 31 +++++ 18 files changed, 364 insertions(+), 21 deletions(-) create mode 100644 pkg/preflight/index_usage.go create mode 100644 pkg/preflight/index_usage_integration_test.go diff --git a/README.md b/README.md index 29e82a02..58ca8e8d 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,8 @@ refusal — never a silently wrong or incomplete result: which keeps its values. A `DROP INDEX` run under `--accept-blocking` needs `--allow-destructive` too: accepting the lock is not accepting the loss, and the refusal lists the checks (`idx_scan` on every replica, foreign - keys, uniqueness) that catch an index the application still uses. + keys, uniqueness) that catch an index the application still uses, with + the connected server's own scan count for the index in `index_usage`. - **Destructive desired-state plans are refused as a whole** — `migrate --desired` creates missing tables and converges supported changes, but never infers permission to discard live structure. A new primary key or unique constraint over a live diff --git a/SAFETY.md b/SAFETY.md index f707b186..9d594b52 100644 --- a/SAFETY.md +++ b/SAFETY.md @@ -18,7 +18,7 @@ The invariant registry (invariant IDs referenced below) lives in | Package | Core? | Status | Invariants enforced | | --- | --- | --- | --- | | `pkg/dbconn` — pool defaults, terminate-blockers, retries, RDS TLS, advisory table lock, replication connection | ✅ core | table-lock primitive exists; `ConnectReplication` dials one replication-mode connection with the pool's TLS, timeouts, and `BeforeConnect` hook; wiring into executing modes planned | LK-1 primitive; LK-2 primitives; CO-9 (session hook and `LocalSearchPath`) | -| `pkg/preflight` — precondition verifier, refusals | ✅ core | exists; copy-and-swap target proof (shape and dependents) and cluster/volume environment check gate the copy-and-swap route before any shadow object is created; table usage evidence (`ReadTableUsage`, `TableUsage.Judge`) exists with no caller yet | ST-6, RF-1..RF-5, RF-11, CO-9 (every proof read is `pg_catalog`-qualified) | +| `pkg/preflight` — precondition verifier, refusals | ✅ core | exists; copy-and-swap target proof (shape and dependents) and cluster/volume environment check gate the copy-and-swap route before any shadow object is created; table usage evidence (`ReadTableUsage`, `TableUsage.Judge`) exists with no caller yet; index usage evidence (`ReadIndexUsage`) is reported on a refused `DROP INDEX` | ST-6, RF-1..RF-5, RF-11, CO-9 (every proof read is `pg_catalog`-qualified) | | `pkg/executor` — bounded optimistic attempt; native concurrent index build with invalid-index recovery; native sequence executor for the safer idioms | ✅ core | exists (Phase 1: attempt-under-budget; Phase 3.1: concurrent index build; Phase 3.2: sequence executor) | LK-2 (attempt bound + the CONCURRENTLY wait-policy exception), CO-9 (qualified proof reads), ST-9 (create owner verified, never repaired) | | `pkg/checksum` — chunk verifier, divergence policy, repair, continuous checker | ✅ core | the chunk `Verifier` exists (one read-only `REPEATABLE READ` transaction per chunk in which the chunk is cut and both digests read, so all three share a snapshot and the cut is bounded, every column cast to the shadow's type on both sides, the same guard as the copier — owner role, the copy statement's `search_path` (the catalog alone, or the shadow build's path when the proof carries conversions), `ACCESS SHARE` on both relations, lock confirmation, relation-OID check — and a `Report` of mismatched chunks up to the landed watermark); `Check` runs the pass under an explicit `DivergencePolicy` (zero value refused; `ParseDivergencePolicy` for configuration, which names `abort` or `repair` and refuses `recopy`), aborts with the shadow untouched or recopies every differing chunk with the copier's chunk statement in one guarded transaction — every chunk's rows deleted before any are put back, so a unique value the source moved between two chunks lands — then rereads each chunk under `repair`, or commits without the reread under `recopy` (the reconcile mode's policy, for a source written while the pass runs; a recopy that collides with a shadow row because the source moved a unique value from a clean chunk into a differing one is rolled back and the pass compares and recopies again, a bounded number of times, counting each collision), returning the committed repairs with any refusal, and mints `CleanWatermark` and `VerifiedShadow` (private constructors) only from a pass that found nothing and repaired nothing; `CheckLive` is the pass for a source written while the catch-up applies the stream — a differing chunk is reread once the catch-up has confirmed past the insert position it was read at and judged against the earlier read (settled, hot and reread again up to a bound, or a confirmed divergence), an unsettled chunk is an `UnresolvedError` that mints nothing, a live repair holds the catch-up's flushes off the shadow in Go for as long as it recopies (`LiveOptions.Flushes`, a `FlushGate` the applier's `Catchup` satisfies, required under `repair`) with `SHARE MODE` on the shadow against every other writer, resumes them once the recopy has committed, and settles each repaired chunk the same way, and `recopy` is refused; the pass is the progress tracker's `WorkSource` while it runs (chunks compared, rows hashed, chunks mismatched, chunks repaired, chunks reread, all from memory); the continuous checker that runs `CheckLive` on a loop is planned | CO-1, CO-2, CO-3, CO-9, LK-1, LK-2 | | `pkg/copier` — shadow-table chunked copy | ✅ core | contract types and the keyset `Chunker` exist (row-count chunks over the proven key, first chunk open below and last open above, a cut frontier for the applier's discard rule, time-targeted sizing) and the parallel `Copier` (one bounded never-overwriting insert per chunk under the table lock session — each column the proof's conversions name selected as its `USING` expression, every other bare — each in a guarded transaction — owner role, the copy statement's `search_path` (the catalog alone, or the shadow build's path when the copy carries conversions), `ACCESS SHARE` on both relations, lock confirmation, relation-OID check — frontier-ordered in-flight registry, a resume that first clears the shadow above the watermark, `Position.Classify` for the applier, and the copy step's `progress.WorkSource` — rows from committed chunks, the source's catalog row count, both tables' measured sizes; every cut and measurement in a bounded read-only transaction with the catalog alone on its `search_path`) exist; every refusal is a `*RefusalError` carrying a typed `RefusalCause` from the closed set in [docs/refusal-classes.md](docs/refusal-classes.md#copier-refusals-keyed-on-copierrefusalcause) | CO-4 (chunk coverage, copy SQL shape, in-flight registry), CO-9, LK-1, LK-2 (every statement on the caller's pool bounds itself), LK-3 | diff --git a/demo/tour.sh b/demo/tour.sh index b19e3eba..e1cb1ded 100755 --- a/demo/tour.sh +++ b/demo/tour.sh @@ -246,6 +246,7 @@ execute_accepted_needs_loss() { assert_eq "refusal exit of [$sql]" 2 "$status" assert_eq "reason of [$sql]" "destructive-change" "$(jq -r '.reason' <<<"$out")" assert_eq "table of [$sql]" "$table" "$(jq -r '.table' <<<"$out")" + assert_eq "index_usage.idx_scan is a number in [$sql]" number "$(jq -r '.index_usage.idx_scan | type' <<<"$out")" else "$PGS" migrate --url "$PG_DSN" --alter "$sql" --accept-blocking "$table" --statement-timeout 30s \ || echo "(refused, exit $? — expected 2: a dropped index also needs --allow-destructive)" diff --git a/docs/invariants.md b/docs/invariants.md index dfb8c263..3378185f 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -1404,7 +1404,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, reason. A `DROP INDEX` — unique or not — is gate-refused first; when `--accept-blocking` accepts that refusal, the drop still runs only if `--allow-destructive` names the same owning table, and otherwise it is refused with `destructive-change` before anything - executes, the detail listing the checks that catch an index the application still uses. + executes, the detail listing the checks that catch an index the application still uses and + the verdict's `index_usage` carrying the connected server's live evidence for the index. Accepting the lock is not accepting the loss. Outage-risk statements are covered by RF-5 and AB-1..AB-5 (`--force`, `--accept-blocking`) and by the LK-2 budgets. *Enforced:* `pkg/migrate` (`run`'s destructive gate through `destructiveOperations` and @@ -1417,7 +1418,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `TestRunRefusesAnUnacknowledgedDropColumnRoutedToCopyAndSwap`, `TestRunNeedsBothForceAndTheDestructiveAcknowledgement`, `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, - `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, `TestAdmitDestructive`, + `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, + `TestReadIndexUsageCountsEachIndexsOwnScans`, `TestAdmitDestructive`, `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, `demo/tour.sh` (`refused_destructive`). *Source:* [design principles](design-principles.md#declarative-review-first-workflow) diff --git a/docs/limitations.md b/docs/limitations.md index d617d6d7..2496d69a 100644 --- a/docs/limitations.md +++ b/docs/limitations.md @@ -15,7 +15,7 @@ not escape hatches: | `ADD CONSTRAINT ... USING INDEX` on a partitioned parent | PostgreSQL does not support adopting an existing index on a partitioned parent in any supported version. pg-sprite refuses before execution. | | `ADD PRIMARY KEY` / `ADD UNIQUE` with a storage parameter the online sequence cannot render | The online sequence builds the key's index with `CREATE UNIQUE INDEX CONCURRENTLY` and carries the key's `WITH (...)` parameters onto it, each name quoted and each value a quoted literal. A parameter it cannot render faithfully, one whose value is not a plain word, number, or string, contains a backslash, or whose name is namespaced, would be dropped or changed, so no sequence is built. Running the statement as written builds the index under `ACCESS EXCLUSIVE`, so it is refused as `rewrite-required` with the `unique-index-then-constraint` guidance. Build the index concurrently with the parameter, then attach it with `ADD CONSTRAINT ... USING INDEX`. | | `ADD FOREIGN KEY ... NOT VALID` on a partitioned parent | PostgreSQL does not support this before version 18, so pg-sprite refuses it on versions 14–17. It is supported on version 18 and later. | -| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). A `DROP INDEX`, unique or not, also needs `--allow-destructive` naming the same table: without it the drop is refused with `destructive-change` (exit 2) before anything runs, and the detail lists the checks to make first — `idx_scan` is 0 on the primary and every replica over a full application cycle, no foreign key relies on the index, and nothing needs a unique index's guarantee. `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | +| `DROP INDEX` and `REINDEX`, plain or `CONCURRENTLY` | `migrate` never runs index maintenance online-safely. The plain form is refused with `index-statement` and names its `CONCURRENTLY` idiom; the concurrent form is refused as already safe, so run it directly against the database. `migrate --accept-blocking SCHEMA.TABLE` runs a single-relation plain `DROP INDEX`, `REINDEX INDEX`, or `REINDEX TABLE` as-is under bounded budgets and exits 3 ([lock-budgeted-passthrough.md](lock-budgeted-passthrough.md)). A `DROP INDEX`, unique or not, also needs `--allow-destructive` naming the same table: without it the drop is refused with `destructive-change` (exit 2) before anything runs, and the detail lists the checks to make first — `idx_scan` is 0 on the primary and every replica over a full application cycle, no foreign key relies on the index, and nothing needs a unique index's guarantee. The refusal's `index_usage` carries the connected server's own evidence — `idx_scan`, `stats_reset`, `last_idx_scan` (PostgreSQL 16+), uniqueness, and whether `track_counts` was on — which covers that one server only, never its replicas. `diff`'s hint to run a removed index through `migrate --alter` ends in this refusal; run `DROP INDEX CONCURRENTLY` directly. | | `RENAME COLUMN` and `RENAME TO` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the catalog flip is instant, but every running application instance that still queries the old name breaks the moment it commits, and no safer sequence can be built from the statement alone because the dual-write and the read switch live in the application. The plan report, `suggest`, and the verdict carry the manual path as typed guidance — `expand-contract-column` for a column (add the new column, dual-write and backfill, switch reads, drop the old column as its own reviewed change) and `coordinated-table-rename` for a table (coordinate the rename with the deploy that adopts the new name, or keep the old name readable through a view until every instance has moved) — and `lint` reports the rename as an error. `--force SCHEMA.TABLE` runs the submitted rename as a blind bounded attempt under the usual budgets and size guard, for the operator who has coordinated the deploy; on a table above `--max-table-size` (default 1 GiB) the forced rename is refused as `not-native-safe-table-too-large` until that threshold is raised. | | `ALTER INDEX ... RENAME TO` on an index that backs a primary-key, unique, or exclusion constraint | Refused with `not-native-safe-rewrite-required`, class `by-design`, guidance `coordinated-constraint-rename`, nothing executed: PostgreSQL renames the constraint with its index, and `ON CONFLICT ON CONSTRAINT` and error handling that name the old constraint break the moment it commits. `--force` does not apply to an index rename. A plain index rename runs. The refusal needs the catalog, so offline `lint` and `suggest` do not report it. | | `ATTACH PARTITION` | Without `--force`, refused with `not-native-safe-rewrite-required`, nothing executed: the engine does not yet construct the validated `CHECK` that lets the attach skip its scan. Pre-prove the bound yourself, then run the attach directly. `--force SCHEMA.TABLE` runs the blocking `ATTACH PARTITION` as submitted under bounded budgets, taking the blocking lock. | diff --git a/docs/lock-budgeted-passthrough.md b/docs/lock-budgeted-passthrough.md index 387a71a8..1abb5730 100644 --- a/docs/lock-budgeted-passthrough.md +++ b/docs/lock-budgeted-passthrough.md @@ -142,7 +142,12 @@ A `DROP INDEX` needs both acknowledgements, each naming the owning table: `--acc accepts the lock, and `--allow-destructive` accepts losing the index (RF-10). The second is checked after the owning table resolves and before anything executes; without it the drop is refused with `destructive-change`, and the detail lists the checks that catch an index the -application still uses. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only +application still uses. The refusal also carries `index_usage`, the connected server's live +evidence for the index (`preflight.ReadIndexUsage`, keyed on the index OID the owning-table +lookup resolved): its `idx_scan` since `stats_reset`, `last_idx_scan` on PostgreSQL 16 and +later, whether it is unique, and whether `track_counts` was on. It is one server's view — +replicas keep their own counters — so it is evidence for the operator, not a gate. The read +takes no lock; if it fails, nothing executes and the error is returned. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only `--accept-blocking`. The value names the table whose lock the operator accepts, because that is the relation the diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 4b1f46a0..078ec204 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -632,7 +632,9 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: executed. An acknowledged run's verdict carries `allowed_destructive: true`, the machine-readable audit record to store beside the operator's consent. The acknowledgement covers only the loss: a statement that also needs `Force` still needs it, and a `DROP INDEX` - run with `Options.AcceptBlocking` needs both, each naming the index's owning table. + run with `Options.AcceptBlocking` needs both, each naming the index's owning table; its + refusal carries `index_usage` (the writer's `idx_scan`, `stats_reset`, `last_idx_scan`, + uniqueness, `track_counts`) to show the operator before asking for that consent. `RunDesired` rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a destructive plan whole, so a declarative drop the operator approves runs as its own diff --git a/docs/table-usage.md b/docs/table-usage.md index d5c4aa83..e79a8ff7 100644 --- a/docs/table-usage.md +++ b/docs/table-usage.md @@ -8,7 +8,10 @@ those statistics can prove. This page explains what the check reads, how it deci `DROP TABLE` is still refused by both front doors today ([capabilities](capabilities.md#deliberately-operator-owned)). The check described here is a read-only primitive, `preflight.ReadTableUsage` and `TableUsage.Judge`, that a later -drop path will consult. Nothing calls it yet. +drop path will consult. Nothing calls it yet. Its index-level sibling, +`preflight.ReadIndexUsage`, reads one index's `idx_scan`, `last_idx_scan`, and uniqueness +under the same limits and is reported, never judged, on a refused `DROP INDEX` +([lock-budgeted passthrough](lock-budgeted-passthrough.md#flag-and-front-door)). ## Read this first: what the check can see diff --git a/docs/testing.md b/docs/testing.md index cf54d89f..2841977b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -409,6 +409,7 @@ same package cover the rest of it. | Library front door (`diffplan.Plan`): ordered routed plan, missing-table, no-op, copy-and-swap refusal, never-writes, deterministic fingerprint | [diffplan unit](../pkg/diffplan/diffplan_test.go), [diffplan integration](../pkg/diffplan/diffplan_integration_test.go) | | Bounded optimistic native attempt and table preflight | [pkg/executor](../pkg/executor/optimistic_integration_test.go), [pkg/preflight](../pkg/preflight/preflight_integration_test.go) | | Table usage evidence (`ReadTableUsage`, `Judge`): window edges, recent scans and lock holders, PostgreSQL 14/15 unknown, resets never idle, one statistics snapshot per read, `track_counts` turned off by a saved setting, partitioned tables judged across every leaf partition | [pkg/preflight](../pkg/preflight/table_usage_integration_test.go), [coverage](../pkg/preflight/table_usage_coverage_integration_test.go), [partitions](../pkg/preflight/table_usage_partitions_integration_test.go), [verdict (unit)](../pkg/preflight/table_usage_verdict_test.go), [partition aggregation (unit)](../pkg/preflight/table_usage_partitions_test.go) | +| Index usage evidence (`ReadIndexUsage`): each index's own scan count, uniqueness, last scan time on PostgreSQL 16+ | [pkg/preflight](../pkg/preflight/index_usage_integration_test.go) | | Safer-sequence runner: autocommit steps, committed-prefix semantics on a mid-sequence failure | [pkg/executor](../pkg/executor/sequence_integration_test.go) | | Concurrent index build: caller-owned session, invalid-index detection into a typed state, and the proven recovery (`RebuildAbandonedIndex`, `DropAbandonedIndex`) | [pkg/executor](../pkg/executor/recover_integration_test.go) | | Greenfield `CREATE TABLE` path: shape refusals, claimed-name collisions, post-commit name verification | [pkg/executor](../pkg/executor/create_integration_test.go) | diff --git a/internal/cli/color_test.go b/internal/cli/color_test.go index f25fff55..31b67e9a 100644 --- a/internal/cli/color_test.go +++ b/internal/cli/color_test.go @@ -186,6 +186,7 @@ func fullVerdict(t *testing.T) verdict.Verdict { LockTimeout: "3s", StatementTimeout: "10m", StatisticsRefresh: &verdict.StatisticsRefresh{Columns: []string{"c"}, Pending: `ANALYZE "t"`}, + IndexUsage: &verdict.IndexUsage{IdxScans: 7, Unique: true, TrackCounts: true}, } rv := reflect.ValueOf(v) for i := range rv.NumField() { diff --git a/internal/cli/verdict_text.go b/internal/cli/verdict_text.go index dc2f48e4..add062f4 100644 --- a/internal/cli/verdict_text.go +++ b/internal/cli/verdict_text.go @@ -48,6 +48,9 @@ func writeVerdictText(out io.Writer, pal palette, v verdict.Verdict) error { if v.Forced { fmt.Fprintf(&b, "\n %s the submitted form ran as-is (force acknowledged)", pal.bold("forced:")) } + if v.IndexUsage != nil { + fmt.Fprintf(&b, "\n %s %s", pal.bold("usage:"), v.IndexUsage.Summary()) + } if v.AllowedDestructive { fmt.Fprintf(&b, "\n %s the statement's data loss was acknowledged (allow-destructive)", pal.bold("allowed:")) } diff --git a/pkg/migrate/accept_blocking.go b/pkg/migrate/accept_blocking.go index ad1883be..4f4fdcb9 100644 --- a/pkg/migrate/accept_blocking.go +++ b/pkg/migrate/accept_blocking.go @@ -11,6 +11,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "github.com/block/pg-sprite/pkg/executor" + "github.com/block/pg-sprite/pkg/preflight" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/verdict" ) @@ -82,7 +83,11 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem dropsIndex := st.Kind() == statement.KindDropIndex if dropsIndex { if opts.AllowDestructive == "" { - return dropIndexDestructiveVerdict(st, table), nil + usage, err := preflight.ReadIndexUsage(ctx, pool, locked.relationOID) + if err != nil { + return verdict.Verdict{}, fmt.Errorf("read usage of the index to drop on %s: %w; nothing was executed", table, err) + } + return dropIndexDestructiveVerdict(st, table, usage), nil } auditAllowDestructive(opts.audit(), st, table) } @@ -154,10 +159,12 @@ func acceptBlocking(ctx context.Context, pool *pgxpool.Pool, st statement.Statem } // acceptedTable is the table an accepted statement locks, as the catalog -// resolved it, with the relkind of that table. +// resolved it, with the relkind of that table and the OID of the relation +// the statement names (the index, or the table itself). type acceptedTable struct { schema, name string relkind string + relationOID uint32 } // qualified renders the table as the acknowledgement must spell it. @@ -198,7 +205,7 @@ func lockedTable(ctx context.Context, pool *pgxpool.Pool, rel statement.IndexRel // against. to_regclass itself still resolves the bare name on that path, // exactly as the statement will. const q = ` - SELECT c.relkind::pg_catalog.text, + SELECT c.oid, c.relkind::pg_catalog.text, COALESCE(tn.nspname, n.nspname), COALESCE(t.relname, c.relname), COALESCE(t.relkind, c.relkind)::pg_catalog.text @@ -211,7 +218,7 @@ func lockedTable(ctx context.Context, pool *pgxpool.Pool, rel statement.IndexRel name := regclassName(rel) var relkind string var table acceptedTable - err := pool.QueryRow(ctx, q, name).Scan(&relkind, &table.schema, &table.name, &table.relkind) + err := pool.QueryRow(ctx, q, name).Scan(&table.relationOID, &relkind, &table.schema, &table.name, &table.relkind) if errors.Is(err, pgx.ErrNoRows) { return acceptedTable{}, fmt.Errorf("%w: %s is not visible on the session search_path; nothing was executed", ErrAcceptBlockingRelationNotFound, name) diff --git a/pkg/migrate/accept_blocking_integration_test.go b/pkg/migrate/accept_blocking_integration_test.go index 0ed8f957..4b5c18c0 100644 --- a/pkg/migrate/accept_blocking_integration_test.go +++ b/pkg/migrate/accept_blocking_integration_test.go @@ -249,6 +249,13 @@ func TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex(t *tes assert.False(t, v.AllowedDestructive) assert.Contains(t, v.Detail, "idx_scan") assert.Contains(t, v.Detail, "--allow-destructive "+schema+".orders") + // The refusal carries this server's live evidence for the index it + // refused to drop: a fresh, non-unique index nothing has scanned. + require.NotNil(t, v.IndexUsage) + assert.EqualValues(t, 0, v.IndexUsage.IdxScans) + assert.False(t, v.IndexUsage.Unique) + assert.True(t, v.IndexUsage.TrackCounts) + assert.False(t, v.IndexUsage.ReadAt.IsZero()) assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "the refused drop must execute nothing") }) diff --git a/pkg/migrate/allow_destructive.go b/pkg/migrate/allow_destructive.go index 9edef635..06ad6d94 100644 --- a/pkg/migrate/allow_destructive.go +++ b/pkg/migrate/allow_destructive.go @@ -5,7 +5,9 @@ import ( "fmt" "log/slog" "strings" + "time" + "github.com/block/pg-sprite/pkg/preflight" "github.com/block/pg-sprite/pkg/router" "github.com/block/pg-sprite/pkg/statement" "github.com/block/pg-sprite/pkg/suggest" @@ -132,19 +134,53 @@ func AdmitDestructive(st statement.Statement, schema string, rs router.Statement // INDEX without the destructive acknowledgement. Every index drop counts, // not only a unique one: the common mistake is dropping an index the // application still reads through, so the detail lists the checks that -// catch it before naming the acknowledgement. table is the index's owning -// table, resolved from the catalog. -func dropIndexDestructiveVerdict(st statement.Statement, table string) verdict.Verdict { +// catch it before naming the acknowledgement, and carries this server's +// live usage of the index as the first piece of that evidence. table is the +// index's owning table, resolved from the catalog. +func dropIndexDestructiveVerdict(st statement.Statement, table string, usage preflight.IndexUsage) verdict.Verdict { detail := "DROP INDEX discards an index that queries, foreign-key checks, or a uniqueness guarantee may " + - "still rely on, and undoing it is a full index build; before dropping, confirm that " + - "pg_stat_user_indexes.idx_scan for the index is 0 on the primary and on every replica (each instance " + - "keeps its own counters) over a window since stats_reset that covers the application's full cycle, " + - "that no foreign key's referencing columns rely on it, and, for a unique index, that nothing still " + - "needs the uniqueness it enforces — then rerun with --allow-destructive " + table - v := verdict.Verdict{Statement: st.SQL(), Table: table, Detail: detail} + "still rely on, and undoing it is a full index build; " + describeIndexUsage(usage) + + "; before dropping, confirm that pg_stat_user_indexes.idx_scan for the index is 0 on every replica " + + "too (each instance keeps its own counters) over a window since stats_reset that covers the " + + "application's full cycle, that no foreign key's referencing columns rely on it, and, for a unique " + + "index, that nothing still needs the uniqueness it enforces — then rerun with --allow-destructive " + table + v := verdict.Verdict{ + Statement: st.SQL(), + Table: table, + Detail: detail, + IndexUsage: &verdict.IndexUsage{ + IdxScans: usage.IdxScans, + LastIdxScan: usage.LastIdxScan, + StatsReset: usage.StatsReset, + ReadAt: usage.ReadAt, + Unique: usage.Unique, + TrackCounts: usage.TrackCounts, + }, + } return v.WithRefusal(destructiveChangeRefusal()) } +// describeIndexUsage renders the connected server's evidence for the +// refusal detail: the scan count, since when it counts, the last scan when +// the server records one, and the caveats that make a zero meaningless. +func describeIndexUsage(u preflight.IndexUsage) string { + since := "since its statistics began (no reset recorded)" + if u.StatsReset != nil { + since = "since stats_reset at " + u.StatsReset.UTC().Format(time.RFC3339) + } + s := fmt.Sprintf("this server has scanned the index %d times %s", u.IdxScans, since) + if u.LastIdxScan != nil { + s += ", last at " + u.LastIdxScan.UTC().Format(time.RFC3339) + } + if u.Unique { + s += "; it is a unique index" + } + if !u.TrackCounts { + s += "; track_counts is off, so this server counted no scans and the count proves nothing" + } + return s +} + // auditAllowDestructive records the acknowledged data loss on table before // anything executes. The record is warn-level and unconditional, as the // force audit is, and the verdict's AllowedDestructive field is its diff --git a/pkg/preflight/index_usage.go b/pkg/preflight/index_usage.go new file mode 100644 index 00000000..cc14429b --- /dev/null +++ b/pkg/preflight/index_usage.go @@ -0,0 +1,111 @@ +package preflight + +import ( + "context" + "errors" + "fmt" + "strconv" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// IndexUsage is the evidence of how an index has been used, read from the +// cumulative statistics of the one server the pool is connected to. Like +// TableUsage it describes that server alone: scans served by a streaming +// replica or an Aurora reader are counted there, not here. It is built only +// by ReadIndexUsage. +type IndexUsage struct { + // IdxScans is the cumulative count of scans of the index since + // StatsReset (pg_stat_all_indexes.idx_scan). + IdxScans int64 + // LastIdxScan is when the server last recorded a scan of the index; nil + // when it recorded none since its statistics began, and always on + // PostgreSQL 14 and 15, which do not record scan times. + LastIdxScan *time.Time + // StatsReset is when the database's statistics were last reset; nil + // when the server reports none, so the start of counting is unknown. + StatsReset *time.Time + // ReadAt is the server's now() for the read. + ReadAt time.Time + // Unique reports whether the index enforces uniqueness. + Unique bool + // TrackCounts is the reading session's track_counts; with it off the + // server counts no scans at all. + TrackCounts bool +} + +// The index usage read: the index's scan count and uniqueness, the +// database's statistics reset, the server's clock, and track_counts, from +// one statistics snapshot. indexUsageWithScanTimeQuery adds last_idx_scan, +// which PostgreSQL 16 introduced. Neither reads the relation itself, so +// neither takes a lock on the index or its table. +// +// INV: CO-9 — every function, operator, relation, and type is +// pg_catalog-qualified. +const ( + indexUsageHead = ` + SELECT COALESCE(s.idx_scan, 0), ` + indexUsageTail = `, + (SELECT d.stats_reset FROM pg_catalog.pg_stat_database d + WHERE d.datname OPERATOR(pg_catalog.=) pg_catalog.current_database()), + pg_catalog.now(), i.indisunique, + pg_catalog.current_setting('track_counts')::pg_catalog.bool + FROM pg_catalog.pg_index i + LEFT JOIN pg_catalog.pg_stat_all_indexes s ON s.indexrelid OPERATOR(pg_catalog.=) i.indexrelid + WHERE i.indexrelid OPERATOR(pg_catalog.=) $1::pg_catalog.oid` + indexUsageQuery = indexUsageHead + "NULL::pg_catalog.timestamptz" + indexUsageTail + indexUsageWithScanTimeQuery = indexUsageHead + "s.last_idx_scan" + indexUsageTail +) + +// errIndexUsageIndexNotPresent means the index the caller resolved was +// dropped before its usage was read. +var errIndexUsageIndexNotPresent = errors.New("the index no longer exists") + +// ReadIndexUsage reads the usage evidence of the index with the given OID +// on the server the pool is connected to, in one read-only transaction +// bounded by its own lock and statement timeouts, with every statistic from +// one snapshot. Keying on the OID reads the index the caller resolved, not +// whatever holds its name now. It changes nothing. +func ReadIndexUsage(ctx context.Context, pool *pgxpool.Pool, indexOID uint32) (IndexUsage, error) { + tx, err := pool.BeginTx(ctx, pgx.TxOptions{AccessMode: pgx.ReadOnly}) + if err != nil { + return IndexUsage{}, fmt.Errorf("begin index usage read %d: %w", indexOID, err) + } + defer func() { + // Redundant safety closer: after a successful Commit this returns + // the guaranteed ErrTxClosed. + _ = tx.Rollback(context.WithoutCancel(ctx)) + }() + // INV: LK-2 — the read bounds itself rather than trusting the pool's + // session defaults. + budgets := "SET LOCAL lock_timeout = " + strconv.FormatInt(usageReadLockTimeout.Milliseconds(), 10) + + "; SET LOCAL statement_timeout = " + strconv.FormatInt(usageReadStatementTimeout.Milliseconds(), 10) + if _, err := tx.Exec(ctx, budgets); err != nil { + return IndexUsage{}, fmt.Errorf("set index usage read budgets: %w", err) + } + if err := pinUsageStatsSnapshot(ctx, tx); err != nil { + return IndexUsage{}, err + } + var versionNum int + if err := tx.QueryRow(ctx, "SELECT pg_catalog.current_setting('server_version_num')::pg_catalog.int4").Scan(&versionNum); err != nil { + return IndexUsage{}, fmt.Errorf("read server version for the index usage read: %w", err) + } + q := indexUsageQuery + if versionNum >= scanTimesSinceVersionNum { + q = indexUsageWithScanTimeQuery + } + var u IndexUsage + err = tx.QueryRow(ctx, q, indexOID).Scan(&u.IdxScans, &u.LastIdxScan, &u.StatsReset, &u.ReadAt, &u.Unique, &u.TrackCounts) + if errors.Is(err, pgx.ErrNoRows) { + return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, errIndexUsageIndexNotPresent) + } + if err != nil { + return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, err) + } + if err := tx.Commit(ctx); err != nil { + return IndexUsage{}, fmt.Errorf("commit index usage read %d: %w", indexOID, err) + } + return u, nil +} diff --git a/pkg/preflight/index_usage_integration_test.go b/pkg/preflight/index_usage_integration_test.go new file mode 100644 index 00000000..17c7a18a --- /dev/null +++ b/pkg/preflight/index_usage_integration_test.go @@ -0,0 +1,85 @@ +package preflight_test + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/preflight" +) + +// indexOID resolves a fixture index to the OID ReadIndexUsage is keyed on. +func (f usageFixture) indexOID(t *testing.T, index string) uint32 { + t.Helper() + var oid uint32 + require.NoError(t, f.pool.QueryRow(t.Context(), + "SELECT to_regclass($1)::oid", f.schema+"."+index).Scan(&oid)) + return oid +} + +// ReadIndexUsage reports each index's own scan count, not the table's: of +// two indexes on one table, the one the test scans three times reads 3 and +// the unique one nothing scans reads 0. It reads the uniqueness of each, +// and, on PostgreSQL 16 and later, when the scanned one was last used. +func TestReadIndexUsageCountsEachIndexsOwnScans(t *testing.T) { + f := newUsageFixture(t, testutil.StartPostgres(t)) + f.exec(t, fmt.Sprintf(` + CREATE TABLE %[1]s.orders ( + id int PRIMARY KEY, + sku text NOT NULL, + created_at timestamptz NOT NULL + ); + INSERT INTO %[1]s.orders + SELECT g, 'sku-' || g, '2026-01-01'::timestamptz + g * interval '1 minute' + FROM generate_series(1, 1000) g; + CREATE INDEX orders_created_idx ON %[1]s.orders (created_at); + CREATE UNIQUE INDEX orders_sku_key ON %[1]s.orders (sku); + ANALYZE %[1]s.orders`, f.schema)) + + // Seq scans are disabled so each lookup scans orders_created_idx, the + // only index on created_at. + f.exec(t, "SET enable_seqscan = off") + for range 3 { + f.exec(t, fmt.Sprintf(` + SELECT id + FROM %s.orders + WHERE created_at = '2026-01-01 00:10:00+00'`, f.schema)) + } + f.exec(t, "RESET enable_seqscan") + f.publish(t) + + scanned := f.indexOID(t, "orders_created_idx") + var u preflight.IndexUsage + var readErr error + require.Eventually(t, func() bool { + if f.major < 15 { + if _, readErr = f.conn.Exec(t.Context(), "SELECT 1"); readErr != nil { + return true + } + } + u, readErr = preflight.ReadIndexUsage(t.Context(), f.pool, scanned) + return readErr != nil || u.IdxScans == 3 + }, statsVisible, 100*time.Millisecond, "the server must publish 3 scans of orders_created_idx") + require.NoError(t, readErr) + + assert.EqualValues(t, 3, u.IdxScans) + assert.False(t, u.Unique) + assert.True(t, u.TrackCounts) + assert.False(t, u.ReadAt.IsZero()) + if f.major >= 16 { + require.NotNil(t, u.LastIdxScan, "PostgreSQL 16 and later record when the index was last scanned") + assert.False(t, u.LastIdxScan.After(u.ReadAt), "the last scan cannot be later than the read") + } else { + assert.Nil(t, u.LastIdxScan, "PostgreSQL 14 and 15 record no scan times") + } + + unused, err := preflight.ReadIndexUsage(t.Context(), f.pool, f.indexOID(t, "orders_sku_key")) + require.NoError(t, err) + assert.EqualValues(t, 0, unused.IdxScans) + assert.True(t, unused.Unique) + assert.Nil(t, unused.LastIdxScan) +} diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index eaaa3142..aa3747f1 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -581,6 +581,50 @@ type Verdict struct { // StatisticsRefresh reports the ANALYZE a committed type change owes // the planner; nil when the statement retyped no column. StatisticsRefresh *StatisticsRefresh `json:"statistics_refresh,omitempty"` + // IndexUsage is the live usage evidence of the index a destructive-change + // refusal of an accepted DROP INDEX would have dropped; nil for every + // other verdict. + IndexUsage *IndexUsage `json:"index_usage,omitempty"` +} + +// IndexUsage is how the server pg-sprite is connected to has seen an index +// used, read when a DROP INDEX is refused for want of the destructive +// acknowledgement. It describes that one server only: every replica keeps +// its own counters, so an index this server never scans may still serve a +// replica's reads. +type IndexUsage struct { + // IdxScans is the cumulative count of scans of the index since + // StatsReset. + IdxScans int64 `json:"idx_scan"` + // LastIdxScan is when the server last recorded a scan; absent when it + // recorded none, and always on PostgreSQL 14 and 15. + LastIdxScan *time.Time `json:"last_idx_scan,omitempty"` + // StatsReset is when counting began; absent when the server reports no + // reset, so the start of the window is unknown. + StatsReset *time.Time `json:"stats_reset,omitempty"` + // ReadAt is the server's clock when the evidence was read. + ReadAt time.Time `json:"read_at"` + // Unique reports whether the index enforces uniqueness, a guarantee the + // drop would also discard. + Unique bool `json:"unique"` + // TrackCounts is false when the server was counting no scans, so a zero + // IdxScans proves nothing. + TrackCounts bool `json:"track_counts"` +} + +// Summary renders the evidence on one line for the verdict's text form. +func (u IndexUsage) Summary() string { + s := fmt.Sprintf("%d index scans on this server (replicas not counted)", u.IdxScans) + if u.LastIdxScan != nil { + s += ", last " + u.LastIdxScan.UTC().Format(time.RFC3339) + } + if u.Unique { + s += ", unique" + } + if !u.TrackCounts { + s += ", track_counts off" + } + return s } // StatisticsRefresh is the outcome of re-analyzing a table after a @@ -654,6 +698,9 @@ func (v Verdict) String() string { if v.Forced { b.WriteString("\n forced: the submitted form ran as-is (force acknowledged)") } + if v.IndexUsage != nil { + fmt.Fprintf(&b, "\n usage: %s", v.IndexUsage.Summary()) + } if v.AllowedDestructive { b.WriteString("\n allowed: the statement's data loss was acknowledged (allow-destructive)") } diff --git a/pkg/verdict/verdict_test.go b/pkg/verdict/verdict_test.go index 0ad98e21..d5d8279b 100644 --- a/pkg/verdict/verdict_test.go +++ b/pkg/verdict/verdict_test.go @@ -425,3 +425,34 @@ func TestStringIncludesAttemptsWhenSet(t *testing.T) { v.Attempts = 0 assert.NotContains(t, v.String(), "attempts") } + +// index_usage is part of the verdict contract an orchestrator reads: a zero +// scan count is present, not omitted, because 0 is the evidence the +// operator is asked to confirm, while the times the server did not record +// are absent rather than a zero time. +func TestIndexUsageJSONKeepsAZeroCountAndOmitsUnrecordedTimes(t *testing.T) { + readAt := time.Date(2026, 10, 28, 9, 30, 0, 0, time.UTC) + v := Verdict{ + Statement: "DROP INDEX app.orders_created_idx", + Table: "app.orders", + IndexUsage: &IndexUsage{ReadAt: readAt, Unique: true, TrackCounts: true}, + }.WithRefusal(ByDesign(ReasonDestructiveChange)) + + js, err := v.JSON() + require.NoError(t, err) + var got map[string]any + require.NoError(t, json.Unmarshal([]byte(js), &got)) + assert.Equal(t, map[string]any{ + "idx_scan": float64(0), + "read_at": "2026-10-28T09:30:00Z", + "unique": true, + "track_counts": true, + }, got["index_usage"]) + + lastScan := readAt.Add(-time.Hour) + v.IndexUsage.IdxScans = 42 + v.IndexUsage.LastIdxScan = &lastScan + v.IndexUsage.TrackCounts = false + assert.Contains(t, v.String(), + "usage: 42 index scans on this server (replicas not counted), last 2026-10-28T08:30:00Z, unique, track_counts off") +} From d063f924b1b37a55c984a080747d6e0d8334b877 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 2 Nov 2026 14:49:02 +1100 Subject: [PATCH 3/4] preflight: sum a partitioned index's usage over its leaf indexes A partitioned index has no statistics row, so its usage read as a clean zero. Also reports saved track_counts settings, states the creation-time window caveat, and emits the evidence in UTC. --- docs/invariants.md | 3 +- docs/lock-budgeted-passthrough.md | 12 +- docs/schemabot-integration.md | 8 +- docs/table-usage.md | 3 +- docs/testing.md | 2 +- .../accept_blocking_integration_test.go | 3 + pkg/migrate/allow_destructive.go | 54 +++++-- pkg/migrate/index_usage_evidence_test.go | 91 +++++++++++ pkg/preflight/index_usage.go | 81 ++++++++-- ...index_usage_partitions_integration_test.go | 147 ++++++++++++++++++ pkg/preflight/table_usage.go | 4 +- pkg/preflight/table_usage_coverage.go | 11 +- pkg/verdict/verdict.go | 24 ++- pkg/verdict/verdict_test.go | 20 ++- 14 files changed, 412 insertions(+), 51 deletions(-) create mode 100644 pkg/migrate/index_usage_evidence_test.go create mode 100644 pkg/preflight/index_usage_partitions_integration_test.go diff --git a/docs/invariants.md b/docs/invariants.md index 63cf8d02..4a9b5390 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -1431,7 +1431,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `TestRunNeedsBothForceAndTheDestructiveAcknowledgement`, `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, - `TestReadIndexUsageCountsEachIndexsOwnScans`, `TestAdmitDestructive`, + `TestReadIndexUsageCountsEachIndexsOwnScans`, + `TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes`, `TestAdmitDestructive`, `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, `demo/tour.sh` (`refused_destructive`). *Source:* [design principles](design-principles.md#declarative-review-first-workflow) diff --git a/docs/lock-budgeted-passthrough.md b/docs/lock-budgeted-passthrough.md index 1abb5730..c9dbdbd1 100644 --- a/docs/lock-budgeted-passthrough.md +++ b/docs/lock-budgeted-passthrough.md @@ -144,10 +144,14 @@ checked after the owning table resolves and before anything executes; without it refused with `destructive-change`, and the detail lists the checks that catch an index the application still uses. The refusal also carries `index_usage`, the connected server's live evidence for the index (`preflight.ReadIndexUsage`, keyed on the index OID the owning-table -lookup resolved): its `idx_scan` since `stats_reset`, `last_idx_scan` on PostgreSQL 16 and -later, whether it is unique, and whether `track_counts` was on. It is one server's view — -replicas keep their own counters — so it is evidence for the operator, not a gate. The read -takes no lock; if it fails, nothing executes and the error is returned. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only +lookup resolved): its `idx_scan` since `stats_reset` (or since the index was created, if +later, which the server does not record), `last_idx_scan` on PostgreSQL 16 and later, whether +it is unique, whether `track_counts` was on, and whether a saved role or database setting +turns it off. A partitioned index has no statistics of its own, so its `idx_scan` is the sum +over its leaf indexes and `partitions` counts them. It is one server's view — +replicas keep their own counters — so it is evidence for the operator, not a gate. Its times +are UTC. The read takes no lock; if it fails, including when the server lists no statistics +for a leaf index, nothing executes and the error is returned instead of the refusal. `REINDEX INDEX` and `REINDEX TABLE` discard nothing and need only `--accept-blocking`. The value names the table whose lock the operator accepts, because that is the relation the diff --git a/docs/schemabot-integration.md b/docs/schemabot-integration.md index 03e6f8b2..22bec4f1 100644 --- a/docs/schemabot-integration.md +++ b/docs/schemabot-integration.md @@ -635,8 +635,12 @@ and PostgreSQL `lock_timeout`). Contract points for the PostgreSQL adapter: machine-readable audit record to store beside the operator's consent. The acknowledgement covers only the loss: a statement that also needs `Force` still needs it, and a `DROP INDEX` run with `Options.AcceptBlocking` needs both, each naming the index's owning table; its - refusal carries `index_usage` (the writer's `idx_scan`, `stats_reset`, `last_idx_scan`, - uniqueness, `track_counts`) to show the operator before asking for that consent. + refusal carries `index_usage` (the writer's `idx_scan`, summed over the leaf indexes of a + partitioned index with `partitions` counting them, `stats_reset`, `last_idx_scan`, + uniqueness, `track_counts`, `track_counts_off_by_setting`; times in UTC) to show the + operator before asking for that consent. If that read fails, `Run` returns the error with a + zero verdict and nothing executed rather than the refusal: route it as a retryable + failure, like a connection fault, not as a refusal. `RunDesired` rejects `AllowDestructive` (`ErrAllowDestructiveNotSupported`) and keeps refusing a destructive plan whole, so a declarative drop the operator approves runs as its own diff --git a/docs/table-usage.md b/docs/table-usage.md index e79a8ff7..6ae28248 100644 --- a/docs/table-usage.md +++ b/docs/table-usage.md @@ -10,7 +10,8 @@ those statistics can prove. This page explains what the check reads, how it deci a read-only primitive, `preflight.ReadTableUsage` and `TableUsage.Judge`, that a later drop path will consult. Nothing calls it yet. Its index-level sibling, `preflight.ReadIndexUsage`, reads one index's `idx_scan`, `last_idx_scan`, and uniqueness -under the same limits and is reported, never judged, on a refused `DROP INDEX` +(summed over the leaf indexes of a partitioned index) under the same limits, including the +saved `track_counts` settings check, and is reported, never judged, on a refused `DROP INDEX` ([lock-budgeted passthrough](lock-budgeted-passthrough.md#flag-and-front-door)). ## Read this first: what the check can see diff --git a/docs/testing.md b/docs/testing.md index 2841977b..a91f2ee2 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -409,7 +409,7 @@ same package cover the rest of it. | Library front door (`diffplan.Plan`): ordered routed plan, missing-table, no-op, copy-and-swap refusal, never-writes, deterministic fingerprint | [diffplan unit](../pkg/diffplan/diffplan_test.go), [diffplan integration](../pkg/diffplan/diffplan_integration_test.go) | | Bounded optimistic native attempt and table preflight | [pkg/executor](../pkg/executor/optimistic_integration_test.go), [pkg/preflight](../pkg/preflight/preflight_integration_test.go) | | Table usage evidence (`ReadTableUsage`, `Judge`): window edges, recent scans and lock holders, PostgreSQL 14/15 unknown, resets never idle, one statistics snapshot per read, `track_counts` turned off by a saved setting, partitioned tables judged across every leaf partition | [pkg/preflight](../pkg/preflight/table_usage_integration_test.go), [coverage](../pkg/preflight/table_usage_coverage_integration_test.go), [partitions](../pkg/preflight/table_usage_partitions_integration_test.go), [verdict (unit)](../pkg/preflight/table_usage_verdict_test.go), [partition aggregation (unit)](../pkg/preflight/table_usage_partitions_test.go) | -| Index usage evidence (`ReadIndexUsage`): each index's own scan count, uniqueness, last scan time on PostgreSQL 16+ | [pkg/preflight](../pkg/preflight/index_usage_integration_test.go) | +| Index usage evidence (`ReadIndexUsage`): each index's own scan count, uniqueness, last scan time on PostgreSQL 16+, a partitioned index summed over its leaf indexes at any depth, `track_counts` turned off by a saved setting, a non-index OID refused | [pkg/preflight](../pkg/preflight/index_usage_integration_test.go), [partitions](../pkg/preflight/index_usage_partitions_integration_test.go), [evidence and detail (unit)](../pkg/migrate/index_usage_evidence_test.go) | | Safer-sequence runner: autocommit steps, committed-prefix semantics on a mid-sequence failure | [pkg/executor](../pkg/executor/sequence_integration_test.go) | | Concurrent index build: caller-owned session, invalid-index detection into a typed state, and the proven recovery (`RebuildAbandonedIndex`, `DropAbandonedIndex`) | [pkg/executor](../pkg/executor/recover_integration_test.go) | | Greenfield `CREATE TABLE` path: shape refusals, claimed-name collisions, post-commit name verification | [pkg/executor](../pkg/executor/create_integration_test.go) | diff --git a/pkg/migrate/accept_blocking_integration_test.go b/pkg/migrate/accept_blocking_integration_test.go index 4d9cba53..a7b1918d 100644 --- a/pkg/migrate/accept_blocking_integration_test.go +++ b/pkg/migrate/accept_blocking_integration_test.go @@ -274,7 +274,10 @@ func TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex(t *tes assert.EqualValues(t, 0, v.IndexUsage.IdxScans) assert.False(t, v.IndexUsage.Unique) assert.True(t, v.IndexUsage.TrackCounts) + assert.False(t, v.IndexUsage.TrackCountsOffBySetting) + assert.Nil(t, v.IndexUsage.Partitions, "a plain index has no leaf count") assert.False(t, v.IndexUsage.ReadAt.IsZero()) + assert.Equal(t, time.UTC, v.IndexUsage.ReadAt.Location()) assert.True(t, indexExists(t, pool, schema, "orders_created_idx"), "the refused drop must execute nothing") }) diff --git a/pkg/migrate/allow_destructive.go b/pkg/migrate/allow_destructive.go index 06ad6d94..0b50ad07 100644 --- a/pkg/migrate/allow_destructive.go +++ b/pkg/migrate/allow_destructive.go @@ -145,30 +145,57 @@ func dropIndexDestructiveVerdict(st statement.Statement, table string, usage pre "application's full cycle, that no foreign key's referencing columns rely on it, and, for a unique " + "index, that nothing still needs the uniqueness it enforces — then rerun with --allow-destructive " + table v := verdict.Verdict{ - Statement: st.SQL(), - Table: table, - Detail: detail, - IndexUsage: &verdict.IndexUsage{ - IdxScans: usage.IdxScans, - LastIdxScan: usage.LastIdxScan, - StatsReset: usage.StatsReset, - ReadAt: usage.ReadAt, - Unique: usage.Unique, - TrackCounts: usage.TrackCounts, - }, + Statement: st.SQL(), + Table: table, + Detail: detail, + IndexUsage: indexUsageEvidence(usage), } return v.WithRefusal(destructiveChangeRefusal()) } +// indexUsageEvidence copies the read into the verdict with every instant in +// UTC, so the JSON is the same whichever host's time zone ran pg-sprite. +func indexUsageEvidence(u preflight.IndexUsage) *verdict.IndexUsage { + e := &verdict.IndexUsage{ + IdxScans: u.IdxScans, + LastIdxScan: utcTime(u.LastIdxScan), + StatsReset: utcTime(u.StatsReset), + ReadAt: u.ReadAt.UTC(), + Unique: u.Unique, + TrackCounts: u.TrackCounts, + TrackCountsOffBySetting: u.TrackCountsOffBySetting, + } + if u.Partitioned { + partitions := u.Partitions + e.Partitions = &partitions + } + return e +} + +func utcTime(t *time.Time) *time.Time { + if t == nil { + return nil + } + utc := t.UTC() + return &utc +} + // describeIndexUsage renders the connected server's evidence for the // refusal detail: the scan count, since when it counts, the last scan when // the server records one, and the caveats that make a zero meaningless. +// The server does not record when an index was created, and an index +// counts only from then, so the window is stated as the later of the two. func describeIndexUsage(u preflight.IndexUsage) string { since := "since its statistics began (no reset recorded)" if u.StatsReset != nil { since = "since stats_reset at " + u.StatsReset.UTC().Format(time.RFC3339) } - s := fmt.Sprintf("this server has scanned the index %d times %s", u.IdxScans, since) + since += " or since the index was created, if later (the server does not record when)" + object := "the index" + if u.Partitioned { + object = fmt.Sprintf("the index through its partition indexes (%d)", u.Partitions) + } + s := fmt.Sprintf("this server has scanned %s %d times %s", object, u.IdxScans, since) if u.LastIdxScan != nil { s += ", last at " + u.LastIdxScan.UTC().Format(time.RFC3339) } @@ -178,6 +205,9 @@ func describeIndexUsage(u preflight.IndexUsage) string { if !u.TrackCounts { s += "; track_counts is off, so this server counted no scans and the count proves nothing" } + if u.TrackCountsOffBySetting { + s += "; an ALTER ROLE or ALTER DATABASE setting turns track_counts off, so scans by those sessions are not counted" + } return s } diff --git a/pkg/migrate/index_usage_evidence_test.go b/pkg/migrate/index_usage_evidence_test.go new file mode 100644 index 00000000..c4fea3fe --- /dev/null +++ b/pkg/migrate/index_usage_evidence_test.go @@ -0,0 +1,91 @@ +package migrate + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/pkg/preflight" +) + +// The evidence's instants are UTC whatever zone pgx scanned them in, so the +// verdict's JSON does not depend on the host's time zone, and a partitioned +// index carries its leaf count while a plain one carries none. +func TestIndexUsageEvidenceIsUTCAndCountsPartitions(t *testing.T) { + east := time.FixedZone("UTC+8", 8*60*60) + readAt := time.Date(2026, 11, 2, 11, 13, 17, 0, east) + reset := readAt.Add(-48 * time.Hour) + last := readAt.Add(-time.Hour) + + e := indexUsageEvidence(preflight.IndexUsage{ + IdxScans: 5, ReadAt: readAt, StatsReset: &reset, LastIdxScan: &last, + Partitioned: true, Partitions: 2, TrackCounts: true, TrackCountsOffBySetting: true, + }) + + assert.Equal(t, time.UTC, e.ReadAt.Location()) + assert.True(t, e.ReadAt.Equal(readAt)) + require.NotNil(t, e.StatsReset) + assert.Equal(t, time.UTC, e.StatsReset.Location()) + require.NotNil(t, e.LastIdxScan) + assert.Equal(t, time.UTC, e.LastIdxScan.Location()) + require.NotNil(t, e.Partitions) + assert.Equal(t, 2, *e.Partitions) + assert.EqualValues(t, 5, e.IdxScans) + assert.True(t, e.TrackCountsOffBySetting) + + plain := indexUsageEvidence(preflight.IndexUsage{ReadAt: readAt, TrackCounts: true}) + assert.Nil(t, plain.Partitions) + assert.Nil(t, plain.StatsReset) + assert.Nil(t, plain.LastIdxScan) +} + +// describeIndexUsage is the detail's renderer: each piece of evidence and +// each caveat appears exactly when its field says so. +func TestDescribeIndexUsage(t *testing.T) { + reset := time.Date(2026, 9, 1, 0, 0, 0, 0, time.UTC) + last := time.Date(2026, 11, 1, 8, 30, 0, 0, time.UTC) + const created = " or since the index was created, if later (the server does not record when)" + + cases := []struct { + name string + usage preflight.IndexUsage + want string + }{ + { + name: "no reset recorded", + usage: preflight.IndexUsage{TrackCounts: true}, + want: "this server has scanned the index 0 times since its statistics began (no reset recorded)" + created, + }, + { + name: "reset and last scan", + usage: preflight.IndexUsage{IdxScans: 7, StatsReset: &reset, LastIdxScan: &last, TrackCounts: true}, + want: "this server has scanned the index 7 times since stats_reset at 2026-09-01T00:00:00Z" + created + + ", last at 2026-11-01T08:30:00Z", + }, + { + name: "partitioned and unique", + usage: preflight.IndexUsage{IdxScans: 5, Partitioned: true, Partitions: 2, Unique: true, TrackCounts: true}, + want: "this server has scanned the index through its partition indexes (2) 5 times since its statistics began (no reset recorded)" + + created + "; it is a unique index", + }, + { + name: "track_counts off", + usage: preflight.IndexUsage{}, + want: "this server has scanned the index 0 times since its statistics began (no reset recorded)" + created + + "; track_counts is off, so this server counted no scans and the count proves nothing", + }, + { + name: "track_counts off by a saved setting", + usage: preflight.IndexUsage{TrackCounts: true, TrackCountsOffBySetting: true}, + want: "this server has scanned the index 0 times since its statistics began (no reset recorded)" + created + + "; an ALTER ROLE or ALTER DATABASE setting turns track_counts off, so scans by those sessions are not counted", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, describeIndexUsage(tc.usage)) + }) + } +} diff --git a/pkg/preflight/index_usage.go b/pkg/preflight/index_usage.go index cc14429b..3fbf3f34 100644 --- a/pkg/preflight/index_usage.go +++ b/pkg/preflight/index_usage.go @@ -17,43 +17,82 @@ import ( // replica or an Aurora reader are counted there, not here. It is built only // by ReadIndexUsage. type IndexUsage struct { - // IdxScans is the cumulative count of scans of the index since - // StatsReset (pg_stat_all_indexes.idx_scan). + // IdxScans is the cumulative count of scans of the index + // (pg_stat_all_indexes.idx_scan). For a partitioned index it is the sum + // over its leaf indexes, where the server records the scans; the + // partitioned index itself has no statistics. IdxScans int64 - // LastIdxScan is when the server last recorded a scan of the index; nil - // when it recorded none since its statistics began, and always on - // PostgreSQL 14 and 15, which do not record scan times. + // LastIdxScan is when the server last recorded a scan of the index, or + // of any of a partitioned index's leaf indexes; nil when it recorded + // none since its statistics began, and always on PostgreSQL 14 and 15, + // which do not record scan times. LastIdxScan *time.Time // StatsReset is when the database's statistics were last reset; nil - // when the server reports none, so the start of counting is unknown. + // when the server reports none. An index created later has counted only + // since its creation, which the server does not record. StatsReset *time.Time // ReadAt is the server's now() for the read. ReadAt time.Time // Unique reports whether the index enforces uniqueness. Unique bool + // Partitioned reports whether the index is a partitioned index, whose + // scans are counted on its leaf indexes. + Partitioned bool + // Partitions is the number of leaf indexes of a partitioned index, at + // any depth; 0 for an index that is not partitioned. + Partitions int // TrackCounts is the reading session's track_counts; with it off the // server counts no scans at all. TrackCounts bool + // TrackCountsOffBySetting reports that a saved ALTER ROLE or ALTER + // DATABASE setting turns track_counts off for some role or for this + // database, so the scans of those sessions are not counted even when + // the reading session's TrackCounts is on. + TrackCountsOffBySetting bool } -// The index usage read: the index's scan count and uniqueness, the +// The index usage read: the scan counts of the index's leaf indexes (the +// index itself unless it is partitioned), its uniqueness and relkind, the // database's statistics reset, the server's clock, and track_counts, from // one statistics snapshot. indexUsageWithScanTimeQuery adds last_idx_scan, -// which PostgreSQL 16 introduced. Neither reads the relation itself, so -// neither takes a lock on the index or its table. +// which PostgreSQL 16 introduced. The tree walks pg_inherits rather than +// calling pg_partition_tree, which locks every partition. Nothing here +// reads a relation itself, so the read takes no lock on the index or its +// table. statsRows counts the leaves the statistics view lists, so a +// missing row is told apart from a zero count. // // INV: CO-9 — every function, operator, relation, and type is // pg_catalog-qualified. const ( indexUsageHead = ` - SELECT COALESCE(s.idx_scan, 0), ` - indexUsageTail = `, + WITH RECURSIVE tree(relid) AS ( + SELECT $1::pg_catalog.oid + UNION ALL + SELECT h.inhrelid + FROM tree t + JOIN pg_catalog.pg_inherits h ON h.inhparent OPERATOR(pg_catalog.=) t.relid + JOIN pg_catalog.pg_class child ON child.oid OPERATOR(pg_catalog.=) h.inhrelid + WHERE child.relispartition + ), + leaves AS ( + SELECT s.indexrelid AS stats_row, s.idx_scan, ` + indexUsageTail = ` AS last_idx_scan + FROM tree t + JOIN pg_catalog.pg_class c ON c.oid OPERATOR(pg_catalog.=) t.relid + LEFT JOIN pg_catalog.pg_stat_all_indexes s ON s.indexrelid OPERATOR(pg_catalog.=) t.relid + WHERE c.relkind OPERATOR(pg_catalog.=) 'i' + ) + SELECT (SELECT pg_catalog.count(*) FROM leaves), + (SELECT pg_catalog.count(l.stats_row) FROM leaves l), + (SELECT COALESCE(pg_catalog.sum(l.idx_scan), 0)::pg_catalog.int8 FROM leaves l), + (SELECT pg_catalog.max(l.last_idx_scan) FROM leaves l), (SELECT d.stats_reset FROM pg_catalog.pg_stat_database d WHERE d.datname OPERATOR(pg_catalog.=) pg_catalog.current_database()), pg_catalog.now(), i.indisunique, + ic.relkind OPERATOR(pg_catalog.=) 'I', pg_catalog.current_setting('track_counts')::pg_catalog.bool FROM pg_catalog.pg_index i - LEFT JOIN pg_catalog.pg_stat_all_indexes s ON s.indexrelid OPERATOR(pg_catalog.=) i.indexrelid + JOIN pg_catalog.pg_class ic ON ic.oid OPERATOR(pg_catalog.=) i.indexrelid WHERE i.indexrelid OPERATOR(pg_catalog.=) $1::pg_catalog.oid` indexUsageQuery = indexUsageHead + "NULL::pg_catalog.timestamptz" + indexUsageTail indexUsageWithScanTimeQuery = indexUsageHead + "s.last_idx_scan" + indexUsageTail @@ -63,6 +102,10 @@ const ( // dropped before its usage was read. var errIndexUsageIndexNotPresent = errors.New("the index no longer exists") +// errIndexUsageStatisticsMissing means the statistics view lists no row for +// a leaf index, so its scans are unknown rather than zero. +var errIndexUsageStatisticsMissing = errors.New("the server reports no statistics for the index") + // ReadIndexUsage reads the usage evidence of the index with the given OID // on the server the pool is connected to, in one read-only transaction // bounded by its own lock and statement timeouts, with every statistic from @@ -97,13 +140,25 @@ func ReadIndexUsage(ctx context.Context, pool *pgxpool.Pool, indexOID uint32) (I q = indexUsageWithScanTimeQuery } var u IndexUsage - err = tx.QueryRow(ctx, q, indexOID).Scan(&u.IdxScans, &u.LastIdxScan, &u.StatsReset, &u.ReadAt, &u.Unique, &u.TrackCounts) + var leaves, statsRows int + err = tx.QueryRow(ctx, q, indexOID).Scan(&leaves, &statsRows, &u.IdxScans, &u.LastIdxScan, + &u.StatsReset, &u.ReadAt, &u.Unique, &u.Partitioned, &u.TrackCounts) if errors.Is(err, pgx.ErrNoRows) { return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, errIndexUsageIndexNotPresent) } if err != nil { return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, err) } + if statsRows != leaves { + return IndexUsage{}, fmt.Errorf("read index usage %d: %w (%d of %d leaf indexes listed)", + indexOID, errIndexUsageStatisticsMissing, statsRows, leaves) + } + if u.Partitioned { + u.Partitions = leaves + } + if u.TrackCountsOffBySetting, err = readUsageTrackCountsOffBySetting(ctx, tx); err != nil { + return IndexUsage{}, err + } if err := tx.Commit(ctx); err != nil { return IndexUsage{}, fmt.Errorf("commit index usage read %d: %w", indexOID, err) } diff --git a/pkg/preflight/index_usage_partitions_integration_test.go b/pkg/preflight/index_usage_partitions_integration_test.go new file mode 100644 index 00000000..1e49927b --- /dev/null +++ b/pkg/preflight/index_usage_partitions_integration_test.go @@ -0,0 +1,147 @@ +package preflight_test + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/block/pg-sprite/internal/testutil" + "github.com/block/pg-sprite/pkg/preflight" +) + +// A partitioned index has no statistics of its own: the server records its +// scans on the partitions' indexes. ReadIndexUsage sums them over every +// leaf index at any depth, here 3 lookups in one partition and 2 in a +// sub-partition's partition, and reports the partitioned index's leaves. +func TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes(t *testing.T) { + f := newUsageFixture(t, testutil.StartPostgres(t)) + f.exec(t, fmt.Sprintf(` + CREATE TABLE %[1]s.events ( + id int NOT NULL, + ts timestamptz NOT NULL, + k int NOT NULL + ) PARTITION BY RANGE (ts); + CREATE TABLE %[1]s.events_2026 PARTITION OF %[1]s.events + FOR VALUES FROM ('2026-01-01') TO ('2027-01-01'); + CREATE TABLE %[1]s.events_2027 PARTITION OF %[1]s.events + FOR VALUES FROM ('2027-01-01') TO ('2028-01-01') + PARTITION BY RANGE (ts); + CREATE TABLE %[1]s.events_2027_h1 PARTITION OF %[1]s.events_2027 + FOR VALUES FROM ('2027-01-01') TO ('2027-07-01'); + CREATE INDEX events_k_idx ON %[1]s.events (k); + INSERT INTO %[1]s.events SELECT g, '2026-03-01', g FROM generate_series(1, 2000) g; + INSERT INTO %[1]s.events SELECT g, '2027-03-01', g FROM generate_series(1, 2000) g; + ANALYZE %[1]s.events`, f.schema)) + + // Seq scans are disabled so each lookup scans the partition's index. + f.exec(t, "SET enable_seqscan = off") + for range 3 { + f.exec(t, fmt.Sprintf(` + SELECT id + FROM %s.events + WHERE ts = '2026-03-01' AND k = 5`, f.schema)) + } + for range 2 { + f.exec(t, fmt.Sprintf(` + SELECT id + FROM %s.events + WHERE ts = '2027-03-01' AND k = 5`, f.schema)) + } + f.exec(t, "RESET enable_seqscan") + f.publish(t) + + parent := f.indexOID(t, "events_k_idx") + var u preflight.IndexUsage + var readErr error + require.Eventually(t, func() bool { + if f.major < 15 { + if _, readErr = f.conn.Exec(t.Context(), "SELECT 1"); readErr != nil { + return true + } + } + u, readErr = preflight.ReadIndexUsage(t.Context(), f.pool, parent) + return readErr != nil || u.IdxScans == 5 + }, statsVisible, 100*time.Millisecond, "the partitions' 5 scans must reach the partitioned index's usage") + require.NoError(t, readErr) + + assert.EqualValues(t, 5, u.IdxScans) + assert.True(t, u.Partitioned) + assert.Equal(t, 2, u.Partitions, "events_2026's index and events_2027_h1's; events_2027's own is partitioned, not a leaf") + if f.major >= 16 { + assert.NotNil(t, u.LastIdxScan, "the latest leaf scan time") + } + + leaf, err := preflight.ReadIndexUsage(t.Context(), f.pool, f.indexOID(t, "events_2026_k_idx")) + require.NoError(t, err) + assert.EqualValues(t, 3, leaf.IdxScans, "a leaf index reports its own scans") + assert.False(t, leaf.Partitioned) + assert.Zero(t, leaf.Partitions) +} + +// A partitioned index on a table with no partitions has nothing to scan: +// its count is a true zero over zero leaves, not a missing row read as 0. +func TestReadIndexUsageOfAPartitionedIndexWithNoPartitions(t *testing.T) { + f := newUsageFixture(t, testutil.StartPostgres(t)) + f.exec(t, fmt.Sprintf(` + CREATE TABLE %[1]s.events ( + ts timestamptz NOT NULL, + k int NOT NULL + ) PARTITION BY RANGE (ts); + CREATE INDEX events_k_idx ON %[1]s.events (k)`, f.schema)) + + u, err := preflight.ReadIndexUsage(t.Context(), f.pool, f.indexOID(t, "events_k_idx")) + require.NoError(t, err) + assert.True(t, u.Partitioned) + assert.Zero(t, u.Partitions) + assert.Zero(t, u.IdxScans) +} + +// An OID that names no index is refused, not reported as an unused index. +func TestReadIndexUsageRefusesAnOIDThatIsNotAnIndex(t *testing.T) { + f := newUsageFixture(t, testutil.StartPostgres(t)) + f.exec(t, fmt.Sprintf(` + CREATE TABLE %s.orders ( + id int PRIMARY KEY + )`, f.schema)) + + _, err := preflight.ReadIndexUsage(t.Context(), f.pool, f.indexOID(t, "orders")) + require.Error(t, err) +} + +// A role whose saved settings turn track_counts off scans an index without +// being counted, so the reading session's own track_counts does not prove +// the count complete; the read reports the saved setting. +func TestReadIndexUsageSeesTrackCountsTurnedOffForARole(t *testing.T) { + f := newUsageFixture(t, testutil.StartPostgres(t)) + f.exec(t, fmt.Sprintf(` + CREATE TABLE %[1]s.orders ( + id int PRIMARY KEY + )`, f.schema)) + var database string + require.NoError(t, f.conn.QueryRow(t.Context(), "SELECT current_database()").Scan(&database)) + // Roles are cluster-wide; the setting is scoped to this test's own + // database so no other test's read sees it. + role := pgx.Identifier{f.schema + "_untracked"}.Sanitize() + f.exec(t, "CREATE ROLE "+role) + t.Cleanup(func() { + _, err := f.pool.Exec(context.WithoutCancel(t.Context()), "DROP ROLE "+role) + assert.NoError(t, err) + }) + pkey := f.indexOID(t, "orders_pkey") + + f.exec(t, fmt.Sprintf(`ALTER ROLE %s IN DATABASE %s SET track_counts = on`, role, pgx.Identifier{database}.Sanitize())) + u, err := preflight.ReadIndexUsage(t.Context(), f.pool, pkey) + require.NoError(t, err) + assert.False(t, u.TrackCountsOffBySetting, "a saved setting that leaves track_counts on") + + f.exec(t, fmt.Sprintf(`ALTER ROLE %s IN DATABASE %s SET track_counts = off`, role, pgx.Identifier{database}.Sanitize())) + u, err = preflight.ReadIndexUsage(t.Context(), f.pool, pkey) + require.NoError(t, err) + assert.True(t, u.TrackCounts, "the reading session still counts") + assert.True(t, u.TrackCountsOffBySetting) +} diff --git a/pkg/preflight/table_usage.go b/pkg/preflight/table_usage.go index c372a6d7..28640fd2 100644 --- a/pkg/preflight/table_usage.go +++ b/pkg/preflight/table_usage.go @@ -254,9 +254,11 @@ func ReadTableUsage(ctx context.Context, pool *pgxpool.Pool, schema, table strin if err := readUsageServerFacts(ctx, tx, &u); err != nil { return TableUsage{}, err } - if err := readUsageTrackCountsSettings(ctx, tx, &u); err != nil { + offBySetting, err := readUsageTrackCountsOffBySetting(ctx, tx) + if err != nil { return TableUsage{}, err } + u.trackCountsOffBySetting = offBySetting tree, err := readUsageRelations(ctx, tx, &u) if err != nil { return TableUsage{}, err diff --git a/pkg/preflight/table_usage_coverage.go b/pkg/preflight/table_usage_coverage.go index ebb2329c..23429034 100644 --- a/pkg/preflight/table_usage_coverage.go +++ b/pkg/preflight/table_usage_coverage.go @@ -35,13 +35,13 @@ func pinUsageStatsSnapshot(ctx context.Context, tx pgx.Tx) error { return nil } -// readUsageTrackCountsSettings records on u whether a saved per-role or +// readUsageTrackCountsOffBySetting reports whether a saved per-role or // per-database setting turns track_counts off for some role or for this // database. The reading session's own value already reflects the server's // configuration and the reading role's settings; this catches the // settings of every other role. Each saved value was validated as a // boolean when it was set, so the cast cannot fail. -func readUsageTrackCountsSettings(ctx context.Context, tx pgx.Tx, u *TableUsage) error { +func readUsageTrackCountsOffBySetting(ctx context.Context, tx pgx.Tx) (bool, error) { // INV: CO-9 — every function, operator, relation, and type is // pg_catalog-qualified. const q = ` @@ -55,12 +55,11 @@ func readUsageTrackCountsSettings(ctx context.Context, tx pgx.Tx, u *TableUsage) WHERE pg_catalog.split_part(setting.entry, '=', 1) OPERATOR(pg_catalog.=) 'track_counts'` rows, err := tx.Query(ctx, q) if err != nil { - return fmt.Errorf("read track_counts settings: %w", err) + return false, fmt.Errorf("read track_counts settings: %w", err) } values, err := pgx.CollectRows(rows, pgx.RowTo[bool]) if err != nil { - return fmt.Errorf("read track_counts settings: %w", err) + return false, fmt.Errorf("read track_counts settings: %w", err) } - u.trackCountsOffBySetting = slices.Contains(values, false) - return nil + return slices.Contains(values, false), nil } diff --git a/pkg/verdict/verdict.go b/pkg/verdict/verdict.go index 54caac94..7f4de39c 100644 --- a/pkg/verdict/verdict.go +++ b/pkg/verdict/verdict.go @@ -603,27 +603,38 @@ type Verdict struct { // replica's reads. type IndexUsage struct { // IdxScans is the cumulative count of scans of the index since - // StatsReset. + // StatsReset, or since the index was created if that is later; for a + // partitioned index, the sum over its leaf indexes. IdxScans int64 `json:"idx_scan"` // LastIdxScan is when the server last recorded a scan; absent when it - // recorded none, and always on PostgreSQL 14 and 15. + // recorded none, and always on PostgreSQL 14 and 15. In UTC. LastIdxScan *time.Time `json:"last_idx_scan,omitempty"` - // StatsReset is when counting began; absent when the server reports no - // reset, so the start of the window is unknown. + // StatsReset is when the database's statistics were last reset; absent + // when the server reports no reset. In UTC. StatsReset *time.Time `json:"stats_reset,omitempty"` - // ReadAt is the server's clock when the evidence was read. + // ReadAt is the server's clock when the evidence was read. In UTC. ReadAt time.Time `json:"read_at"` // Unique reports whether the index enforces uniqueness, a guarantee the // drop would also discard. Unique bool `json:"unique"` + // Partitions is the number of leaf indexes of a partitioned index, whose + // scans IdxScans sums; absent for an index that is not partitioned. + Partitions *int `json:"partitions,omitempty"` // TrackCounts is false when the server was counting no scans, so a zero // IdxScans proves nothing. TrackCounts bool `json:"track_counts"` + // TrackCountsOffBySetting is true when a saved ALTER ROLE or ALTER + // DATABASE setting turns track_counts off, so the scans of those + // sessions are missing from IdxScans. + TrackCountsOffBySetting bool `json:"track_counts_off_by_setting"` } // Summary renders the evidence on one line for the verdict's text form. func (u IndexUsage) Summary() string { s := fmt.Sprintf("%d index scans on this server (replicas not counted)", u.IdxScans) + if u.Partitions != nil { + s += fmt.Sprintf(" through its partition indexes (%d)", *u.Partitions) + } if u.LastIdxScan != nil { s += ", last " + u.LastIdxScan.UTC().Format(time.RFC3339) } @@ -633,6 +644,9 @@ func (u IndexUsage) Summary() string { if !u.TrackCounts { s += ", track_counts off" } + if u.TrackCountsOffBySetting { + s += ", track_counts off for some roles" + } return s } diff --git a/pkg/verdict/verdict_test.go b/pkg/verdict/verdict_test.go index d5d8279b..9ee8acd7 100644 --- a/pkg/verdict/verdict_test.go +++ b/pkg/verdict/verdict_test.go @@ -443,16 +443,26 @@ func TestIndexUsageJSONKeepsAZeroCountAndOmitsUnrecordedTimes(t *testing.T) { var got map[string]any require.NoError(t, json.Unmarshal([]byte(js), &got)) assert.Equal(t, map[string]any{ - "idx_scan": float64(0), - "read_at": "2026-10-28T09:30:00Z", - "unique": true, - "track_counts": true, + "idx_scan": float64(0), + "read_at": "2026-10-28T09:30:00Z", + "unique": true, + "track_counts": true, + "track_counts_off_by_setting": false, }, got["index_usage"]) lastScan := readAt.Add(-time.Hour) + partitions := 2 v.IndexUsage.IdxScans = 42 v.IndexUsage.LastIdxScan = &lastScan + v.IndexUsage.Partitions = &partitions v.IndexUsage.TrackCounts = false + v.IndexUsage.TrackCountsOffBySetting = true assert.Contains(t, v.String(), - "usage: 42 index scans on this server (replicas not counted), last 2026-10-28T08:30:00Z, unique, track_counts off") + "usage: 42 index scans on this server (replicas not counted) through its partition indexes (2), last 2026-10-28T08:30:00Z, unique, track_counts off, track_counts off for some roles") + + js, err = v.JSON() + require.NoError(t, err) + got = nil + require.NoError(t, json.Unmarshal([]byte(js), &got)) + assert.Equal(t, float64(2), got["index_usage"].(map[string]any)["partitions"], "a partitioned index's leaf count") } From a05c4409d6e11dd1bd282393aeb85a869ce9c7a5 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 2 Nov 2026 22:49:58 +1100 Subject: [PATCH 4/4] preflight: pin the index usage read's missing-statistics check A live server lists every leaf index, so the fail-closed check is tested as a pure helper rather than through ReadIndexUsage. --- docs/invariants.md | 3 ++- pkg/preflight/index_usage.go | 15 ++++++++++++--- pkg/preflight/index_usage_test.go | 23 +++++++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 pkg/preflight/index_usage_test.go diff --git a/docs/invariants.md b/docs/invariants.md index 4a9b5390..1c4c9c8f 100644 --- a/docs/invariants.md +++ b/docs/invariants.md @@ -1432,7 +1432,8 @@ Each refusal is a preflight **error with a stated reason** — never a warning, `TestRunAllowDestructiveDoesNotLiftADropIndexRefusal`, `TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex`, `TestReadIndexUsageCountsEachIndexsOwnScans`, - `TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes`, `TestAdmitDestructive`, + `TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes`, + `TestCheckLeafStatisticsListedRefusesAnUnlistedLeaf`, `TestAdmitDestructive`, `TestMigrateDryRunReportsTheDestructiveRefusal`, `TestAllowDestructiveFlagGrammar`, `demo/tour.sh` (`refused_destructive`). *Source:* [design principles](design-principles.md#declarative-review-first-workflow) diff --git a/pkg/preflight/index_usage.go b/pkg/preflight/index_usage.go index 3fbf3f34..ef23419a 100644 --- a/pkg/preflight/index_usage.go +++ b/pkg/preflight/index_usage.go @@ -106,6 +106,16 @@ var errIndexUsageIndexNotPresent = errors.New("the index no longer exists") // a leaf index, so its scans are unknown rather than zero. var errIndexUsageStatisticsMissing = errors.New("the server reports no statistics for the index") +// checkLeafStatisticsListed fails closed when the statistics view lists +// fewer rows than there are leaf indexes: an unlisted leaf's scans are +// unknown, and counting them as zero would make a used index look idle. +func checkLeafStatisticsListed(leaves, statsRows int) error { + if statsRows != leaves { + return fmt.Errorf("%w (%d of %d leaf indexes listed)", errIndexUsageStatisticsMissing, statsRows, leaves) + } + return nil +} + // ReadIndexUsage reads the usage evidence of the index with the given OID // on the server the pool is connected to, in one read-only transaction // bounded by its own lock and statement timeouts, with every statistic from @@ -149,9 +159,8 @@ func ReadIndexUsage(ctx context.Context, pool *pgxpool.Pool, indexOID uint32) (I if err != nil { return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, err) } - if statsRows != leaves { - return IndexUsage{}, fmt.Errorf("read index usage %d: %w (%d of %d leaf indexes listed)", - indexOID, errIndexUsageStatisticsMissing, statsRows, leaves) + if err := checkLeafStatisticsListed(leaves, statsRows); err != nil { + return IndexUsage{}, fmt.Errorf("read index usage %d: %w", indexOID, err) } if u.Partitioned { u.Partitions = leaves diff --git a/pkg/preflight/index_usage_test.go b/pkg/preflight/index_usage_test.go new file mode 100644 index 00000000..0469ab2a --- /dev/null +++ b/pkg/preflight/index_usage_test.go @@ -0,0 +1,23 @@ +package preflight + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// A leaf index the statistics view does not list has unknown scans, so the +// read fails closed rather than counting it as zero. A live server lists +// every index of a table, which is why this is checked here and not through +// ReadIndexUsage. +func TestCheckLeafStatisticsListedRefusesAnUnlistedLeaf(t *testing.T) { + assert.ErrorIs(t, checkLeafStatisticsListed(2, 1), errIndexUsageStatisticsMissing) + assert.ErrorIs(t, checkLeafStatisticsListed(1, 0), errIndexUsageStatisticsMissing) +} + +// Every leaf listed, including a partitioned index with no partitions (zero +// leaves, zero rows), is a complete read. +func TestCheckLeafStatisticsListedAcceptsEveryLeafListed(t *testing.T) { + assert.NoError(t, checkLeafStatisticsListed(2, 2)) + assert.NoError(t, checkLeafStatisticsListed(0, 0)) +}