chore(db): drop dead columns, redundant indexes, and unique constraint - #597
Closed
junhoyeo wants to merge 3 commits into
Closed
chore(db): drop dead columns, redundant indexes, and unique constraint#597junhoyeo wants to merge 3 commits into
junhoyeo wants to merge 3 commits into
Conversation
Migration 0011 drops five pieces of confirmed-dead schema surface: - daily_breakdown.provider_breakdown: declared in schema.ts but never read or written anywhere in src/. Pure allocated bytes per row. - daily_breakdown.model_breakdown: written on every submit, but the only SELECT (users/[username]/route.ts:110) discards the value. Storage + serialization churn with no consumer. - submissions.status + idx_submissions_status: column is only ever written as 'verified' on insert; zero WHERE filters anywhere. The index serves no query. - users.is_admin: column is set/fetched but no admin gate exists anywhere in the codebase. A user with is_admin=true had zero additional privileges. If admin features ship later, reintroduce WITH an actual gate. Code changes follow the schema: remove the field from drizzle table defs, drop the buildModelBreakdown helper (only caller was the submit write path now also gone), and stop emitting / reading the dropped fields from session/auth/profile/submit endpoints. Tests: removed 21 isAdmin mock-object entries and the test that inlined buildModelBreakdown's algorithm. Also fixed a pre-existing leaderboard.test.ts failure caused by #522 adding customFrom / customTo positional args without updating the expectation. Verification: tsc 0 errors, 251/251 vitest, end-to-end POST /api/submit against the post-drop schema returns 200 and writes a daily_breakdown row successfully. Constraint: do not regress the active submit / profile / devices paths Constraint: depends on #593's 0010_submit_count_safety being present Rejected: drop submissions.idx_submissions_user_id + idx_submissions_total_tokens + idx_submissions_date_range as well | needs a fresh pg_stat_user_indexes audit on prod first; the index cleanup is a separate analysis-driven PR Rejected: drop submissions_user_hash_unique constraint (functionally redundant with submissions_user_id_unique) | low-blast-radius but worth its own PR with a freshness check on the submit-idempotency code path Confidence: high Scope-risk: moderate (touches submit/route.ts, the critical write path) Directive: do NOT reintroduce users.is_admin without also wiring an admin gate; the previous setup misled readers into thinking access control existed Not-tested: ad-hoc SQL queries that may have been reading the dropped jsonb columns on prod
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Extends 0011_drop_dead_columns with the index-cleanup (group B) and redundant-unique-constraint (group C) drops that were originally proposed as follow-up PRs, after a fresh integrity audit on the local DB confirmed each drop is safe. Index drops: - idx_submissions_user_id: redundant left-prefix of idx_submissions_leaderboard (user_id, total_tokens, total_cost, created_at). Per #389's prod stats audit, 214 scans on this vs 3.27M on the leaderboard composite. - idx_submissions_total_tokens: 0 prod scans (#389 audit). All reads use SUM() per-user GROUP BY which is served by the leaderboard composite. - idx_submissions_date_range: 0 prod scans (#389 audit). No filter ever queries these columns; leaderboard period filters use daily_breakdown.date which has its own index. - idx_users_username, idx_sessions_token, idx_api_tokens_token, idx_device_codes_device_code, idx_device_codes_user_code: Postgres auto-creates a btree for every UNIQUE constraint; these explicit btrees were 1:1 duplicates of *_unique indexes and only added INSERT/UPDATE write amplification. Index adds (FK coverage): - idx_device_codes_user_id: cascade-delete of a user did a seq scan of device_codes. - idx_group_members_invited_by: cascade-delete of an inviter did a seq scan of group_members. - idx_group_invites_invited_by: same, for group_invites. Constraint drop: - submissions_user_hash_unique (user_id, submission_hash) was functionally subsumed by submissions_user_id_unique (user_id). The stronger constraint already enforces one row per user, so any (user_id, submission_hash) collision is rejected at the user_id level. The submission_hash column itself stays; /api/submit still reads it for idempotency comparison. Verification on clean local DB: - All 11 prior migrations (0000-0010) plus this 0011 apply successfully from an empty schema. - pg_constraint audit: all 14 FKs in the schema now have covering indexes (was 11 before; +3 from this PR). - Index audit: all 9 dead/redundant indexes confirmed gone; the 3 new FK-coverage indexes confirmed present. - bunx tsc --noEmit: 0 errors. - bunx vitest run __tests__: 251/251 pass. - End-to-end POST /api/submit with a real api_tokens row: HTTP 200, daily_breakdown row written, submission totals updated. - Seed script reload: 3 users, 6 devices, 84 daily rows, 1 group with 3 members — all counts consistent. Constraint: each index drop must have either a prod stats scan-count justification or be a literal duplicate of an existing unique-constraint index Constraint: do not regress FK cascade-delete behavior Rejected: drop idx_submissions_created_at | not redundant — leaderboard composite has created_at as the 4th column; a plain created_at filter still benefits from a dedicated index Rejected: drop idx_users_github_id | github OAuth login does a lookup by github_id on every sign-in; this index is hot Confidence: high Scope-risk: moderate (drops are irreversible; index recreation in prod requires CREATE INDEX CONCURRENTLY if scans turn out to be needed) Directive: do NOT re-add an explicit btree on any column that already has a UNIQUE constraint — postgres creates the covering index automatically Not-tested: prod pg_stat_user_indexes scan counts as of merge day (the audit data is ~6 weeks old per #389)
junhoyeo
changed the title
The 2026-05-25 prod audit (pg_stat_user_indexes) changed the picture for the duplicate-of-unique-constraint indexes I planned to drop: - idx_sessions_token: 89,560 scans (sessions_token_unique: 0) - idx_users_username: 30,080 scans (users_username_unique: 0) - idx_api_tokens_token: 27,126 scans (api_tokens_token_unique: 0) - idx_device_codes_device_code: 153 scans - idx_device_codes_user_code: 3 scans Postgres' planner consistently picks the explicit non-unique index over the unique-constraint sibling, so these are NOT no-op drops — the unique-constraint indexes show 0 scans in prod because they aren't being chosen. Removing the explicit ones would invalidate cached plans and force a re-plan onto the unique-constraint indexes; safe in theory but a non-zero risk window. Removed those five DROP INDEX statements from migration 0011 and restored the corresponding index() declarations to schema.ts with a comment explaining why the explicit non-unique siblings stay. Two drops that prod stats initially questioned but the investigation subagents confirmed safe (no app caller, scans are write-side enforcement / vacuum): - idx_submissions_status: 323 scans — vacuum cycles + historical scan count from before commit acccd44 removed the column from the drizzle model. No app WHERE filter exists. - submissions_user_hash_unique: 388 scans — pure constraint-check btree probes on every INSERT/UPDATE that writes submission_hash; no read query references the column. Both still get dropped. Constraint: do not invalidate hot cached query plans on prod without a controlled re-plan window Rejected: drop all 5 unique-duplicate btrees | the planner uses them, sessions_token alone is 89k scans Confidence: high Scope-risk: narrow (only removed planned drops; nothing else changed) Directive: do NOT remove the 5 explicit btrees on prod without first running ANALYZE on the table and verifying the planner falls back to the unique-constraint index Not-tested: cold-plan latency after dropping any of these in a future PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drops five pieces of dead schema surface flagged in the post-#593 audit (#593 (comment)):
daily_breakdown.provider_breakdown jsonbschema.ts, zero reads, zero writes anywhere insrc/. Pure allocated bytes on every daily row.daily_breakdown.model_breakdown jsonbsubmit/route.tson every submission but the only SELECT (users/[username]/route.ts:110) discards the value. Net result: storage + JSON serialization churn with no consumer.submissions.status varchar(20)+idx_submissions_status'verified'on insert (submit/route.ts:205). Zero WHERE filters anywhere in the codebase. The index serves no query.users.is_admin booleanSessionUser, but no admin gate exists anywhere in the codebase. A user withis_admin=truehad zero additional privileges. If admin features ever ship, reintroduce the column WITH an actual gate.Plus one pre-existing test fixup that was failing on
mainbefore this PR:__tests__/api/leaderboard.test.tswas checking 5 positional args togetLeaderboardDatabut the route now passes 7 (customFrom/customTofrom #522 were never added to the expectation).Migration
0011_drop_dead_columns.sql— five DROPs, each gated withIF EXISTSso the migration is safe to replay against any environment regardless of whether the column was previously removed out-of-band.Depends on #593's
0010_submit_count_safetybeing present (this PR is branched offfeat/devices-ui-from-pr389). Land #593 first, then this rebases cleanly onto main.Code changes
src/lib/db/schema.ts: remove the four columns and the dead index from the schema definitions.src/lib/db/helpers.ts: deletebuildModelBreakdown(only caller was the submit flow's model-breakdown writes, now also gone).src/app/api/submit/route.ts: drop thestatus: "verified"insert value, removemodelBreakdownfrom both the in-memorytoInsert/toUpdatetypes and the SQL VALUES list / UPDATE SET /AS batch(...)column list.src/app/api/users/[username]/route.ts: stop SELECT-ingdailyBreakdown.modelBreakdown(it was never read from the result).src/lib/auth/session.ts: removeisAdminfrom theSessionUserinterface and from bothgetSession()/getSessionFromHeader()returns.src/lib/auth/personalTokens.ts: removeisAdminfromAuthenticatedPersonalToken, the SELECT, and the returned record.Test changes
All
isAdmin: false/isAdmin: "users.isAdmin"mock object entries removed across 11 test files (mechanical — they were only there because the production type required the field). Removed theshould build modelBreakdown from clients with multiple modelstest insubmit.test.tssince it was inlining the algorithm of the deleted helper rather than testing observable behavior.Verification
Local stack (
postgres:16onlocalhost:5432, all migrations 0000→0011 applied, seeded withscripts/seed-dev.ts):\d submissionsno longer showsstatus;\d daily_breakdownno longer showsprovider_breakdownormodel_breakdown;\d usersno longer showsis_admin.idx_submissions_statusis gone from\di.bunx tsc --noEmit→ 0 errorsbunx vitest run __tests__→ 251 passed, 0 failed (was 250/1 before the leaderboard test fix)GET /api/users/seed-dev-alice→ 200 JSON, totals match seeded data, noisAdminfield anywhereGET /api/users/seed-dev-alice/devices→ 200 (devices endpoint from feat(devices): add per-device read+rename APIs and fix submit_count drift #593 unaffected)POST /api/submitwith that token + a fresh daily contribution → HTTP 200,submissionIdreturned,daily_breakdownrow inserted withtokens=1000,cost=0.0500,source_breakdownjsonb populated, no errors despite the droppedmodel_breakdowncolumn being absent from the INSERT.Caveats
provider_breakdown/model_breakdownfor ad-hoc analytics on prod, those queries break. From the codebase audit there are no such callers, but worth a heads-up before merge.users.is_adminwas already a useless capability (no gates), so dropping it doesn't change observed behavior for anyone. If admin features are planned, the right pattern is a separateadmin_userstable or a role enum onusers, not a boolean column that the auth layer ignores.submissions/daily_breakdown. TheDROP INDEXis the only meaningful lock.Test plan
0011_drop_dead_columnsapplies cleanly on staging (after feat(devices): add per-device read+rename APIs and fix submit_count drift #593's0010_submit_count_safetyhas landed)\dt,\d submissions, profile + devices endpoints, one real submitSummary by cubic
Dropped dead DB columns, removed unused submission indexes, added FK coverage indexes, and deleted a redundant unique constraint. Cleaned up submit/auth/profile code and tests to match the new schema; intentionally kept planner-preferred duplicate indexes.
Refactors
users.is_admin,submissions.status, anddaily_breakdown.{provider_breakdown, model_breakdown}references; deletedbuildModelBreakdown; stopped writingstatus: "verified"on submit.isAdminfrom mocks, deleted the modelBreakdown unit test, and fixedleaderboardexpectations to include the 6th/7th args asundefined.Migration
daily_breakdown.provider_breakdown,daily_breakdown.model_breakdown,submissions.status(+idx_submissions_status),users.is_admin.idx_submissions_user_id,idx_submissions_total_tokens,idx_submissions_date_range.idx_users_username,idx_sessions_token,idx_api_tokens_token,idx_device_codes_device_code,idx_device_codes_user_code.idx_device_codes_user_id,idx_group_members_invited_by,idx_group_invites_invited_by.submissions_user_hash_unique. All DROPs useIF EXISTS; apply after0010_submit_count_safety.Written for commit 810f8cf. Summary will update on new commits. Review in cubic