Skip to content

db: drop unused plain env_builds status index - #3406

Merged
ValentaTomas merged 1 commit into
mainfrom
drop-unused-env-builds-status-index
Jul 27, 2026
Merged

ValentaTomas merged 1 commit into
mainfrom
drop-unused-env-builds-status-index

Conversation

@ValentaTomas

Copy link
Copy Markdown
Member

idx_env_builds_status is effectively unread (single-digit scans over a multi-month usage window): status-filtered queries drive through per-entity joins or the status_group index family, while every build write maintains this copy on one of the hottest-write tables. status_group indexes are untouched. Needs platform sign-off that no seldom-run tooling depends on a bare status scan — hence draft. Fresh, isolated re-roll of the earlier closed draft (#3391).

@cla-bot cla-bot Bot added the cla-signed label Jul 27, 2026
@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Schema change on a hot table: unlikely to affect app queries if usage analysis is correct, but any ad-hoc or tooling that relied on a bare status index scan could regress until platform confirms none do.

Overview
Removes the standalone idx_env_builds_status index on env_builds(status) because production usage shows it is barely read while every build write still has to maintain it on a high-churn table. Status-filtered access is expected to keep using per-entity paths and the separate status_group index family, which this change does not alter. The migration drops the index with DROP INDEX CONCURRENTLY, uses bounded statement_timeout on the up path and restores the migrator default afterward, and documents a down path that clears any invalid partial index before rebuilding concurrently with no timeout.

Reviewed by Cursor Bugbot for commit 6acf9ea. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@ValentaTomas
ValentaTomas marked this pull request as ready for review July 27, 2026 04:12

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Straightforward migration dropping an unused plain index; the down path correctly guards against a stranded INVALID index from an interrupted concurrent build, and statement_timeout handling matches the migrator's documented session defaults.

Extended reasoning...

Overview

Single-file migration that drops idx_env_builds_status (a plain btree on env_builds(status)) via DROP INDEX CONCURRENTLY, with a down migration that rebuilds it. This is a re-roll of a previously closed draft (#3391), with the same author-provided usage analysis (single-digit scans over a multi-month window) justifying removal in favor of the status_group index family, which the change does not touch.

Security risks

None. This is a schema/index-only change with no application logic, no new inputs, and no data exposure implications.

Level of scrutiny

Low-to-medium: index drops on a hot-write table carry operational risk (lock contention, query plan regressions) rather than correctness risk. I checked the current sqlc queries touching env_builds (get_inprogress_builds.sql, get_concurrent_template_builds.sql, get_builds_statuses.sql, etc.) and confirmed none filter on the bare status column directly — they all use status_group or join through active_template_builds/env_build_assignments, consistent with the PR's rationale. I also verified the original index-creation migration (20250506112836_builds_status_index.sql) and the status_group migration (20260210120002_add_status_group_column.sql) to confirm the migration correctly targets the now-redundant index and leaves status_group's index untouched.

Other factors

The up/down statement_timeout handling mirrors the pattern already used elsewhere in this migrations directory, and the down path's extra DROP INDEX CONCURRENTLY IF EXISTS before CREATE INDEX CONCURRENTLY IF NOT EXISTS correctly avoids the known Postgres pitfall where an INVALID index from an interrupted concurrent build would otherwise cause IF NOT EXISTS to silently skip rebuilding. The PR description notes it is still awaiting platform sign-off (hence draft), which is a judgment call outside the scope of this code review, but the migration itself is mechanically correct and low-risk.

@ValentaTomas
ValentaTomas merged commit bc7af4c into main Jul 27, 2026
43 checks passed
@ValentaTomas
ValentaTomas deleted the drop-unused-env-builds-status-index branch July 27, 2026 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants