[Kiran01bm]

The destructive-change refusal of an accepted DROP INDEX now carries the connected server's live usage evidence for the index, as a typed index_usage field on the verdict and a line in its text.

Stacked on #276; retarget to main once it merges.

Why

#276 refuses a DROP INDEX under --accept-blocking until --allow-destructive names the table, and its detail asks the operator to confirm the index is unused. pg-sprite already holds a connection to the writer, so it can show the first piece of that evidence instead of only describing the query to run. It stays evidence, not a gate: one server's counters cannot see a replica's reads, so the decision remains the operator's.

What

• preflight.ReadIndexUsage(ctx, pool, indexOID): one bounded, read-only, lock-free read of the index's idx_scan, last_idx_scan (PostgreSQL 16+), uniqueness, the database's stats_reset, track_counts, and the server clock, from one statistics snapshot. Keyed on the OID the owning-table lookup already resolved.
• verdict.IndexUsage (index_usage): idx_scan (always present, so a 0 is visible), last_idx_scan and stats_reset (omitted when the server recorded none), read_at, unique, track_counts. The text form gains a usage: line, and the refusal detail states the count and its window.
• If the read fails, nothing executes and the error is returned.
• Docs: limitations, passthrough design, SchemaBot integration, table-usage, SAFETY, RF-10, testing map. The demo asserts index_usage.idx_scan on the refusal.

Before / after

The same refused drop of an index the application scanned 1,284 times since the last reset. Before, the operator is told how to check; after, the check's answer for this server is in the refusal.

pg-sprite migrate --alter 'DROP INDEX app.orders_created_idx' --accept-blocking app.orders --statement-timeout 10m --json

Before                                         After

refused: destructive-change, exit 2            refused: destructive-change, exit 2
detail: "confirm idx_scan is 0 on the          detail: "this server has scanned the index 1284
         primary and every replica ..."                 times since stats_reset at 2026-09-01T00:00:00Z,
                                                        last at 2026-10-28T09:12:44Z; confirm it is 0
                                                        on every replica too ..."
(no usage field)                               "index_usage": {
                                                 "idx_scan": 1284,
                                                 "last_idx_scan": "2026-10-28T09:12:44Z",
                                                 "stats_reset": "2026-09-01T00:00:00Z",
                                                 "read_at": "2026-10-28T09:30:00Z",
                                                 "unique": false,
                                                 "track_counts": true
                                               }

On PostgreSQL 14 and 15 last_idx_scan is absent; with track_counts off the text says the count proves nothing.

🤖 Generated with Amp (Claude Opus 4.6)

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-11-02T03:06:19.452612Z We couldn’t complete this review. Comment @codex review to try again.	83f16f3	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Review 1/2 of 83f16f3: 1 blocking, 2 non-blocking.

I read the whole diff at 83f16f3 (a merge of main into the branch; the PR's own change is pkg/preflight/index_usage.go, the index_usage verdict field and its text line, acceptBlocking and lockedTable in pkg/migrate, and the docs), the table-usage read it is modelled on, and lockedTable's relkind handling. I ran the new tests against PostgreSQL 17 (all pass, pkg/verdict, internal/cli, the new preflight test, the DROP INDEX acceptance test), mutation-tested the production lines, and drove the built binary against a live server for the cases below. CI is green on all of 14 through 18.

The change is well scoped: the read is one bounded, read-only transaction keyed on the index OID the owning-table lookup already resolved, a failed read returns an error with nothing executed (fail closed), the refusal itself is untouched, and the evidence is reported rather than judged. The one defect is that the evidence is wrong for a kind of index the same code path explicitly accepts.

Invariants: extends RF-10 (the refusal still stands and still names the acknowledgement; it now also carries evidence), upholds CO-9 (the new query is pg_catalog-qualified throughout), upholds LK-2 and AB-2 (the read takes its own lock_timeout and statement_timeout, takes no lock on the index or table, and executes nothing).

Blocking

1. A partitioned index reports idx_scan: 0 however heavily its partitions are scanned. lockedTable accepts a partitioned index (relkind I) for DROP INDEX, and the usage read goes straight to pg_stat_all_indexes through a LEFT JOIN with COALESCE(s.idx_scan, 0) (index_usage.go#L49, #L56). That view lists only indexes of plain tables, TOAST tables, and materialized views; a partitioned parent index has no row, and the scans live on the partitions' own indexes. The join finds nothing, the COALESCE turns the absence into a clean zero, and nothing in the output says it is not a real count.

Reproduced on PostgreSQL 17 with the built binary: a table partitioned by range with CREATE INDEX ev_k_idx ON app.ev (k), two index lookups on k so the partition's index shows idx_scan = 2. Then:

pg-sprite migrate --alter 'DROP INDEX app.ev_k_idx' --accept-blocking app.ev --statement-timeout 10m --json

refused: destructive-change
detail: "... this server has scanned the index 0 times since its statistics began (no reset recorded); ..."
"index_usage": { "idx_scan": 0, "read_at": "...", "unique": false, "track_counts": true }

track_counts is true and unique is false, so none of the caveats the PR does model fire; the operator is shown "scanned 0 times" for an index that was just used. That is the evidence the PR adds so the operator can decide, and it is biased toward "safe to drop", on a refusal whose only next step is the --allow-destructive rerun. Before this PR the refusal made no claim about usage, so this is a new confidently wrong answer on a data-loss gate, which is why it blocks even though the gate itself is unchanged (RF-10 still refuses; the evidence it hangs on the refusal misleads).

Suggested fix: for relkind I, sum idx_scan (and take the max last_idx_scan) over the leaf indexes of pg_partition_tree(indexrelid); a plain index keeps the existing join (pg_partition_tree returns no rows for a non-partitioned index, so a blanket replacement breaks the existing test, which I confirmed). The alternative that needs no new query is to refuse to report usage for a partitioned index and say so in the text. Either way a missing statistics row must not be rendered as a zero: read the stats row without COALESCE and treat a NULL for a plain index as an error or as "no statistics recorded".

Test case: fails on 83f16f3

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"
)

// The scans of a partitioned table's index are recorded on the partitions'
// indexes, so the usage read of the parent index must report them.
func TestReadIndexUsageCountsScansOfAPartitionedIndexsPartitions(t *testing.T) {
	f := newUsageFixture(t, testutil.StartPostgres(t))
	f.exec(t, fmt.Sprintf(`
		CREATE TABLE %[1]s.events (id int, ts timestamptz NOT NULL, k int) 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 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;
		ANALYZE %[1]s.events`, f.schema))
	f.exec(t, "SET enable_seqscan = off")
	for range 3 {
		f.exec(t, fmt.Sprintf("SELECT id FROM %s.events WHERE k = 5", f.schema))
	}
	f.exec(t, "RESET enable_seqscan")
	f.publish(t)

	parent := f.indexOID(t, "events_k_idx")
	var u preflight.IndexUsage
	require.Eventually(t, func() bool {
		var err error
		u, err = preflight.ReadIndexUsage(t.Context(), f.pool, parent)
		require.NoError(t, err)
		return u.IdxScans > 0
	}, statsVisible, 100*time.Millisecond, "the partition's scans must reach the parent index's usage")
	assert.EqualValues(t, 3, u.IdxScans)
}

--- FAIL: TestReadIndexUsageCountsScansOfAPartitionedIndexsPartitions (10.15s) on 83f16f3 ("Condition never satisfied"). With the COALESCE(s.idx_scan, 0) select item replaced by COALESCE((SELECT sum(ls.idx_scan) FROM pg_partition_tree(i.indexrelid) pt JOIN pg_stat_all_indexes ls ON ls.indexrelid = pt.relid WHERE pt.isleaf), 0) (pg_catalog-qualified) this test passes, and TestReadIndexUsageCountsEachIndexsOwnScans then fails, as above: that prototype is not the fix, only proof the read can see the partition scans. Drop it in pkg/preflight (it reuses the existing fixture).

Non-blocking

2. The window the detail states can be longer than the window the count covers. describeIndexUsage says "scanned the index N times since stats_reset at T", and the following sentence asks the operator to confirm the window covers the application's full cycle. The count starts at the later of the database's stats_reset and the index's own creation (or its last single-object counter reset), and only the first is read. Reproduced: pg_stat_reset(), ten seconds later CREATE INDEX, then the refusal reads "scanned the index 0 times since stats_reset at 03:13:24Z" for an index created at 03:13:3x. With a database reset months ago and an index built last week (the usual state after an index swap by CREATE INDEX CONCURRENTLY), the operator is told the zero covers months. It is evidence the operator weighs, so non-blocking, but the claim is false as written. A one-clause caveat ("or since the index was created, which the server does not record") keeps the sentence true; REINDEX CONCURRENTLY kept the count on 17 in my run, so I am not claiming that path.

3. Most of the new behavior is not pinned by a test. The mutation table below shows the evidence sentence, every caveat in it, and the error path surviving. The acceptance test asserts Detail contains idx_scan, which the static text already provides, so removing describeIndexUsage from the detail changes nothing it checks. Needed: a unit test over describeIndexUsage (it is pure; five branches: no reset, reset, last scan, unique, track_counts off), one assertion that the refusal detail contains the scan count, and a failing-read test (close the pool or use an unknown OID) that asserts the error and that the index still exists. The no SET LOCAL budgets row below was caught only because deleting the if leaves budgets unused and the package no longer compiles; a behavioral check on the read's bounds is still absent, as are checks that it is read-only and snapshot-pinned.

Verified

• Ran on 83f16f3: go test ./pkg/verdict/ ./internal/cli/, TestReadIndexUsageCountsEachIndexsOwnScans, TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex: pass on PostgreSQL 17. Per-version behavior (last scan on 16+) rests on the CI matrix, all green.
• The read targets the OID lockedTable resolved; if the index is dropped between the two, the read returns errIndexUsageIndexNotPresent and nothing executes. Fail closed.
• Plain index, unique index, and an index created after a reset behave as documented (idx_scan, unique, window text) in the built binary.
• Mutation testing of the production lines (each reverted; worktree clean afterwards):

Mutant	Caught by
idx_scan always 0	TestReadIndexUsageCountsEachIndexsOwnScans
scan-time query never used	same test (PostgreSQL 16+ only)
unique always false	same test
track_counts always true	survived
statistics snapshot pin removed	survived (race-only, expected)
SET LOCAL budgets removed	compile error only (unused variable), not a behavioral catch
transaction ReadOnly to ReadWrite	survived
read error swallowed (zero usage used instead)	survived
verdict copy drops IdxScans	survived
verdict copy drops Unique	survived
verdict copy drops TrackCounts	TestRunAcceptBlockingNeedsTheDestructiveAcknowledgementToDropAnIndex
detail omits the usage sentence	survived
detail omits track_counts caveat	survived
detail omits unique note	survived
detail ignores stats_reset	survived
detail omits last scan	survived
idx_scan JSON gains omitempty	TestIndexUsageJSONKeepsAZeroCountAndOmitsUnrecordedTimes
Verdict.String drops the usage line	same test
Summary drops track_counts off	same test
CLI text drops the usage line	TestVerdictTextColorWrapsLabelsOnly

This review was generated by Claude Code (claude-sonnet-5-5).

[aparajon]

🤖 Review 2/2 of 83f16f3: OSS adoption and SchemaBot integration lenses, 0 blocking, 3 non-blocking.

This is the second of two comments; the correctness pass, with its one blocking finding about partitioned indexes, is the one above. The seam an adapter binds to moves only additively: a new exported preflight.ReadIndexUsage and preflight.IndexUsage, a new verdict.IndexUsage and Verdict.IndexUsage pointer field, and an optional index_usage JSON object on the destructive-change refusal of an accepted DROP INDEX. No flag, option, refusal reason, or cause is added or renamed, so a standalone user sees one more line in the refusal and an orchestrator that ignores unknown JSON fields sees nothing.

Blocking

None.

Non-blocking

1. read_at, last_idx_scan, and stats_reset come out in the client's local offset, not UTC. The verdict copies the time.Time values pgx scanned straight into the struct (allow_destructive.go#L151-L157), and encoding/json writes them in whatever time.Local the process has. Observed from the built binary on a host at +08:00: "read_at": "2026-11-02T11:13:17.653672+08:00", while the same refusal's detail renders the same instants in UTC with a Z (describeIndexUsage and Summary both call .UTC()), and the PR's before/after example shows Z. A consumer parsing RFC 3339 is fine either way, but an adapter that stores or diffs the strings, or a golden file in an importer's tests, sees a different value depending on which host ran pg-sprite. These are the only timestamps in the verdict contract, so this sets the convention: normalizing to UTC when the verdict is built (or in a MarshalJSON) makes the field deterministic.

2. The track_counts caveat is narrower than the table-usage read the docs say it matches. ReadTableUsage also reads the saved per-role and per-database track_counts settings (readUsageTrackCountsSettings), because a role whose session has counting off contributes no scans however busy it is; the index read reports only the reading session's own value (index_usage.go#L54), yet table-usage.md#L13 says the index read runs "under the same limits". So the application role can have counting off while the refusal shows track_counts: true and a clean zero. Reusing the table read's settings check, or narrowing the docs sentence to what the code does, closes the gap; the first also keeps the two reads from drifting apart.

3. A failed read changes the outcome class of a refusal, and the error is untyped. On a statistics-read failure the refusal (exit 2, destructive-change) becomes a plain error (exit 1) from acceptBlocking. That is the fail-closed choice, and nothing runs, so it is safe. For an orchestrator, though, a statement it would have shown as "refused, here is the acknowledgement to supply" now surfaces as an opaque failure that is indistinguishable from a connection fault, and the PR body's "stays evidence, not a gate" is not quite true of the error path: a statistics outage turns the evidence into a gate on seeing the refusal at all. If the destructive-change refusal should survive an unreadable statistic, return the refusal with index_usage omitted and a detail clause saying usage could not be read; if failing is intended, say so in schemabot-integration.md so an adapter maps it deliberately. Separately, the PR description still says "Stacked on #276; retarget to main" although #276 has merged and the base is main.

Verified

• Exported surface checked against main: additions only, no existing symbol, field, JSON key, or refusal cause changes, and the new JSON object is omitempty, so every other verdict is byte-identical.
• Docs updated in the places the change touches (limitations, passthrough, integration, table-usage, testing, SAFETY, README, RF-10 entry); the README, passthrough, and integration text claims "one server's view, replicas keep their own counters", which the code and the verdict comments say the same way.
• The demo asserts only that index_usage.idx_scan is a number, so it does not pin a value; fine for a smoke check.

This review was generated by Claude Code (claude-sonnet-5-5).

[aparajon]

🤖 Stamping with comments: 1 blocking, 5 non-blocking (see the two review comments above).

This stamp was left by Claude Code (claude-sonnet-5-5).

[aparajon]

🤖 Delta review of d063f92 (since 83f16f3): 0 blocking, 1 non-blocking.

The head moved by one commit on top of 83f16f3 (no rebase; compare shows ahead_by: 1, behind_by: 0), d063f92, which answers the blocking finding and the other points from my two earlier comments.

The blocking finding is resolved

The partitioned index no longer reads as a clean zero. index_usage.go now walks pg_inherits from the index OID (a recursive CTE, relispartition children only, no lock on any partition), keeps the leaf indexes (relkind i), and sums idx_scan and takes the max last_idx_scan over them. A plain index is its own single leaf, so the existing path keeps its meaning. The old COALESCE(s.idx_scan, 0) is gone from the per-index read: the query also returns how many leaves exist and how many have a statistics row, and ReadIndexUsage returns errIndexUsageStatisticsMissing when they differ, so an absent row is an error (fail closed) rather than a zero. A partitioned index with no partitions is a true zero over zero leaves, and says so.

Evidence, run on the head:

• My test from the first review (TestReadIndexUsageCountsScansOfAPartitionedIndexsPartitions, copied into pkg/preflight) failed on 83f16f3 and passes on d063f92 (PostgreSQL 17).
• The PR's own TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes covers two levels (a leaf under a sub-partitioned partition, whose own index is partitioned and not counted), asserts 5 scans over 2 leaves, the leaf's own count, and Partitions; it passes on PostgreSQL 17 and 14, as do ...OfAPartitionedIndexWithNoPartitions, ...RefusesAnOIDThatIsNotAnIndex, ...SeesTrackCountsTurnedOffForARole, and the original ...CountsEachIndexsOwnScans.
• The built binary, on PostgreSQL 17, against a range-partitioned table with a partition-level index scanned twice: DROP INDEX app.ev_k_idx --accept-blocking app.ev now refuses with "idx_scan": 2, "partitions": 1, a last-scan time, and the detail "scanned the index through its partition indexes (1) 2 times"; the index still exists afterwards (nothing executed).

The other earlier points

• Window wording (my non-blocking 2): the detail now says the count runs since the reset "or since the index was created, if later (the server does not record when)", and the IdxScans and StatsReset comments match. Addressed.
• Pinning (my non-blocking 3): new unit tests TestDescribeIndexUsage (five branches) and TestIndexUsageEvidenceIsUTCAndCountsPartitions, plus the track_counts and non-index-OID integration tests. Addressed for the evidence sentence and every caveat; the failed-read path is still not pinned (see below).
• UTC (second comment, 1): indexUsageEvidence converts read_at, last_idx_scan, and stats_reset to UTC. With TZ=Asia/Singapore the built binary prints ...Z timestamps. Addressed.
• track_counts narrower than the table read (second comment, 2): the saved per-role and per-database check is factored out of the table read as readUsageTrackCountsOffBySetting and used by both, surfaced as track_counts_off_by_setting plus a detail clause. Addressed, and the two reads can no longer drift.
• Error class (second comment, 3): schemabot-integration.md now tells an adapter that a failed read returns the error with a zero verdict and nothing executed, and to route it as a retryable failure, not a refusal. Addressed by documenting the intended behavior.

Non-blocking

1. The new missing-statistics branch is not pinned. I removed the statsRows != leaves check (index_usage.go#L152) and every test still passed. That branch is the fail-closed guard that replaced the old COALESCE, so it is the line a future edit could quietly turn back into "no row means zero". It is hard to provoke from a live server (the statistics view lists every index of a table, TOAST table, or materialized view), so a test may need to split the leaf-versus-row comparison into a small pure helper and assert it directly, the same way describeIndexUsage is tested. The same goes for the read-error path in acceptBlocking, which the first review also noted.

Invariants

Extends RF-10 (the refusal's evidence is now correct for a partitioned index), upholds CO-9 (every function, operator, relation, and type in the new CTE is pg_catalog-qualified; only COALESCE, UNION ALL, and WITH RECURSIVE are syntax), and upholds LK-2 and AB-2 (the read is still one bounded read-only transaction that takes no lock on the index, table, or any partition, and executes nothing). The RF-10 entry's test list in docs/invariants.md names the new test.

Verified

• Ran on d063f92: go build ./..., go vet of the three touched packages, go test ./pkg/verdict/ ./internal/cli/, pkg/preflight TestReadIndexUsage* (PostgreSQL 17, and 14 for the query without last_idx_scan), pkg/migrate TestIndexUsageEvidenceIsUTCAndCountsPartitions, TestDescribeIndexUsage, and the two DROP INDEX acceptance tests. All pass.
• Mutation testing of the new production lines (each reverted; worktree clean afterwards):

Mutant	Caught by
sum replaced by max over the leaves	TestReadIndexUsageSumsAPartitionedIndexsLeafIndexes
leaf filter (relkind = 'i') removed	the same test
partitioned flag always false	the same test
Partitions never set	the same test
saved-setting read dropped	TestReadIndexUsageSeesTrackCountsTurnedOffForARole
read_at not converted to UTC	TestIndexUsageEvidenceIsUTCAndCountsPartitions
verdict copy drops Partitions	the same test
verdict copy drops TrackCountsOffBySetting	the same test
detail drops the created-later caveat	TestDescribeIndexUsage
Summary drops the partitions clause	pkg/verdict tests
Summary drops the saved-setting clause	pkg/verdict tests
statsRows != leaves check removed	survived (finding 1)

This review was generated by Claude Code (claude-sonnet-5-5).

[aparajon]

🤖 Stamping with comments: 0 blocking, 1 non-blocking (see the delta review comment above). The earlier blocking finding on partitioned indexes is resolved.

This stamp was left by Claude Code (claude-sonnet-5-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/277, follow-up commit

All three comments are answered. The blocking partitioned-index finding and every non-blocking finding are fixed: the first two comments' findings in d063f92, and the delta review's missing-statistics check in the follow-up commit. One part, the acceptBlocking read-error path, gets a reply instead of a test.

#	Finding	Status	Explanation
C1-F1	A partitioned index reports idx_scan: 0 however heavily its partitions are scanned	fixed (in d063f92)	The read walks the index's partition tree through pg_inherits, without locking any partition. It sums idx_scan, takes the latest last_idx_scan over the leaf indexes at any depth, and reports partitions. The delta review confirmed it on PostgreSQL 17 and 14 and with the built binary.
C3-F1	The fail-closed missing-statistics check is not pinned	fixed	The leaf-versus-row comparison is now a pure helper, checkLeafStatisticsListed, and unit tests cover an unlisted leaf (refused) and every leaf listed, including zero leaves. Removing the check now fails a test. The test is named in the RF-10 entry.
C3-F1	The read-error path in acceptBlocking is not pinned	reply	No test can reach it without a mock. The index usage read takes no lock and only reads statistics views every role can read, so a live server gives it no way to fail between the table lookup and the read. The core forbids mocked-DB tests. The path wraps the error, says nothing was executed, and the adapter doc routes it as a retryable failure.
C1-F2	The stated window can be longer than the one the count covers	fixed (in d063f92)	The detail says the count runs since stats_reset "or since the index was created, if later (the server does not record when)". The verdict and the doc say the same.
C1-F3	Most of the new behavior is not pinned by a test	fixed (in d063f92)	Added a renderer test over every branch of describeIndexUsage, a test of the verdict copy, acceptance-test assertions on the new fields, and integration tests for a non-index OID and a saved track_counts = off.
C2-F1	read_at, last_idx_scan and stats_reset come out in the host's local offset	fixed (in d063f92)	The verdict's evidence is converted to UTC when it is built.
C2-F2	The track_counts caveat is narrower than the table read's	fixed (in d063f92)	The index read reuses the table read's saved-settings check and reports track_counts_off_by_setting.
C2-F3	A failed read turns a refusal into an untyped error	fixed (in d063f92)	Failing closed is intended. schemabot-integration.md tells adapters to route it as a retryable failure, not as a refusal.
Reviews and each comment's Verified section	Approvals; CO-9, LK-2, AB-2 upheld	no action	—

Source: block/pg-sprite#277, review comments 6568238539 and 6568239449 at head 83f16f3a, and review comment 6568831592 at head d063f924; reviews 5731750454 and 5732001309; fixes in d063f92 and the follow-up commit.