cockroachdb / cockroachdb/cockroach
sql/opt/workloadindexrec: workload_index_recs loses the index-replacement guidance from recommendations
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
**Describe the problem**
A per-statement replacement recommendation carries guidance about the index it supersedes, as a trailing comment on the `CREATE` ([rec.go:406](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/indexrec/rec.go#L406)):
```
CREATE INDEX ON db.public.foo (b) STORING (c) /* After successfully creating the
replacement index, manually run: `ALTER INDEX db.public.foo@foo_b_idx NOT VISIBLE`
and then, after verifying workload performance, manually run: `DROP INDEX
db.public.foo@foo_b_idx` */;
```
That shape is deliberate — [467e40f504f](https://github.com/cockroachdb/cockroach/commit/467e40f504f) moved the `DROP` into a comment because batching `CREATE; DROP` as one command is unsafe when DDL isn't atomic.
`workload_index_recs()` loses this entirely. It parses each recommendation into a `tree.CreateIndex` AST, merges the ASTs through the trie, and re-renders with `index.String()` ([workload_indexrecs.go:209](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/workloadindexrec/workload_indexrecs.go#L209)). The parser discards comments and the AST has nowhere to carry them, so the superseded index's identity is gone by the time the workload-level output is built. Nothing downstream can reconstruct it.
The result is that `workload_index_recs()` tells you to create indexes but never that existing ones are now redundant, so applying its output accumulates indexes indefinitely.
**What we want**
The workload-level output should carry the same *style* of guidance the per-statement recommendation does — a comment on the `CREATE` row naming the index it supersedes:
```
CREATE INDEX ON t (i) STORING (k) /* supersedes t@t_i; after creating, run
`ALTER INDEX t@t_i NOT VISIBLE`, then `DROP INDEX t@t_i` once verified */;
```
Explicitly **not** a separate executable `DROP INDEX` row. That's the shape 467e40f504f moved away from, and it's worse at the workload level than it was per-statement: the output is a flat unordered list, so a `DROP` row is independently applicable and can sort before the `CREATE` that justifies it.
**What's blocking it**
The superseded index identity isn't available to the workload layer in any usable form. It exists in the stored recommendation string, but only as English prose inside a comment, so recovering it means regex-scraping the comment text. `findBestExistingIndexToReplace` already returns the index structurally at [rec.go:342](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/indexrec/rec.go#L342) — persisting that identity (index name or ID) alongside the recommendation would let the workload layer rebuild the guidance without parsing anything.
One design wrinkle: the trie merges creates across statements, so one merged index can supersede several different existing indexes, and a superseded index may be covered by more than one merged leaf. The guidance has to be aggregated per merged index, not copied verbatim from a single source recommendation.
**To Reproduce**
```sql
CREATE TABLE t (i INT, k INT);
CREATE INDEX t_i ON t (i);
SELECT k FROM t WHERE i = 1;
-- The per-statement recommendation carries the guidance:
SELECT index_recommendations FROM crdb_internal.statement_statistics
WHERE metadata->>'query' LIKE 'SELECT k FROM t%';
-- {"replacement : CREATE INDEX ON t (i) STORING (k) /* ... DROP INDEX t@t_i ... */;"}
-- The workload-level output has dropped it:
SELECT * FROM workload_index_recs();
-- CREATE INDEX ON t (i) STORING (k);
```
**Additional data**
There is a `DROP INDEX` emission path in `FindWorkloadRecs` today — the `disMap` at [workload_indexrecs.go:40](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/workloadindexrec/workload_indexrecs.go#L40) and the loop at [:48](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/workloadindexrec/workload_indexrecs.go#L48) — but it is fed only by parsed `*tree.DropIndex` statements ([:134](https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/opt/workloadindexrec/workload_indexrecs.go#L134)), which v25.3+ recommendations never produce. It is dead for current data, and it emits exactly the unsafe shape described above, so it should be removed as part of this rather than revived.
This went unnoticed because `logic_test/workload_indexrecs` injects the pre-v25.3 two-statement form directly into `system.statement_statistics` instead of exercising the real producer, so its drop assertions still pass against a format the product no longer emits.
Jira issue: CRDB-66575
Contributor guide
Research direction
Start with pkg/sql/opt/workloadindexrec/workload_indexrecs.go, especially FindWorkloadRecs, the disMap path, and trie merging, then inspect pkg/sql/opt/indexrec/rec.go around findBestExistingIndexToReplace. Update logic_test/workload_indexrecs to exercise the current recommendation format and verify aggregated superseded-index guidance appears as CREATE comments without executable DROP rows.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100