fix(usage): add entity_usage index for compute.percentile lookup - #30032
fix(usage): add entity_usage index for compute.percentile lookup#30032sonika-shah wants to merge 2 commits into
Conversation
The 1.13 unique-key reorder to (id, usageDate) sped up usage upserts but left the compute.percentile UPDATE -- which filters by (usageDate, entityType) with no id -- without a usable index. On large catalogs the resulting full-table scan grows with usage history and blows past the client read timeout; the client retry then collides with the still-running statement, surfacing as 'Lock wait timeout exceeded'. Add a supplementary non-unique (usageDate, entityType) index so the percentile query seeks its day/type slice again. It excludes the mutable count columns, so the count-update path optimized by the unique-key reorder is untouched and the upsert-deadlock fix is preserved.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Plain CREATE INDEX takes a write-blocking lock on entity_usage for the full build. Match the CONCURRENTLY pattern used by the other large-table index builds (1.13.0) so usage writes and percentile reads aren't blocked during upgrade; DROP ... IF EXISTS first clears any INVALID index left by an interrupted build. MySQL is unchanged -- ADD INDEX is online by default on MySQL 8.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| -- actively-written table) for the duration of the build. The DROP ... IF EXISTS first clears any | ||
| -- INVALID leftover from a previously interrupted CONCURRENTLY build so this re-runs cleanly. | ||
| DROP INDEX CONCURRENTLY IF EXISTS entity_usage_percentile_idx; | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS entity_usage_percentile_idx ON entity_usage (usageDate, entityType); |
There was a problem hiding this comment.
⚠️ Bug: DROP-first cleanup defeated by per-statement migration tracking
The comment claims the leading DROP INDEX CONCURRENTLY IF EXISTS clears an INVALID leftover so the migration "re-runs cleanly", but the migration framework tracks each statement individually in SERVER_MIGRATION_SQL_LOGS and skips any already-recorded statement (MigrationFile.parseSQLFiles line 83 filters via checkIfQueryPreviouslyRan; MigrationProcessImpl also re-checks). On a first run the DROP succeeds (no-op) and is logged; if the subsequent CREATE INDEX CONCURRENTLY is interrupted it leaves an INVALID index and is NOT logged. On re-run the DROP is skipped (already logged) and CREATE INDEX CONCURRENTLY IF NOT EXISTS sees the index name already exists (validity is not checked by IF NOT EXISTS), so it silently no-ops. The INVALID index is never rebuilt, the planner ignores it, and computePercentile keeps full-scanning — the fix silently fails with no error. Consider handling cleanup outside the tracked statement (e.g. via the Java Migration class that can DROP INVALID indexes before the CREATE), or reindex/validate as part of the same statement.
Was this helpful? React with 👍 / 👎
| DROP INDEX CONCURRENTLY IF EXISTS entity_usage_percentile_idx; | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS entity_usage_percentile_idx ON entity_usage (usageDate, entityType); |
There was a problem hiding this comment.
💡 Bug: CREATE INDEX CONCURRENTLY requires autocommit / no open transaction
Postgres CREATE INDEX CONCURRENTLY and DROP INDEX CONCURRENTLY throw "cannot run inside a transaction block" if executed within a transaction. This works only because the server jdbi connection pool defaults to autoCommit=true (HikariCPDataSourceFactory) and statements run via handle.execute without an explicit transaction. If migrations are ever run through a path/pool that disables autocommit or wraps DDL in a transaction, both statements will fail. Worth confirming the ops/CLI migrate entrypoint uses the same autocommit-enabled connection, since the earlier non-CONCURRENTLY form did not have this constraint.
Was this helpful? React with 👍 / 👎
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
🟡 Playwright Results — all passed (30 flaky)✅ 4524 passed · ❌ 0 failed · 🟡 30 flaky · ⏭️ 96 skipped
🟡 30 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Problem
compute.percentile(POST /api/v1/usage/compute.percentile/{entityType}/{date}) times out consistently on large catalogs after 1.13. The client sees aSocketTimeoutException; the server log shows the real cause —Lock wait timeout exceededon theUsageDAO.computePercentileUPDATE.Root cause
The 1.13 migration in #25751 reordered the
entity_usageunique key from(usageDate, id)→(id, usageDate)to reduce usage-upsert deadlocks. That table has a single index, shared by two different access patterns:insertOrUpdateCount/insertOrReplaceCount) filters byid→ the new(id, usageDate)index serves it well (this path is healthy).computePercentilefilters byusageDate+entityTypewith noid→ after the reorder there is no usable index, so it full-scansentity_usage. The scan grows with usage history, exceeds the client read timeout, and the client's retry then collides with the still-running statement holding row locks →Lock wait timeout exceeded.Only
computePercentileis affected; the write path is unchanged.Fix
Add a supplementary, non-unique
(usageDate, entityType)index onentity_usagein the1.13.2native migration, so the percentile query seeks its day/type slice again instead of scanning the whole table.Audited all statements in
UsageDAOthat touchentity_usage:id-based statements (upserts, deletes,getUsageById,getLatestUsage,getLatestUsageBatch) remain served by the existing(id, usageDate)unique key — the optimizer ignores the new index for them.count1/count7/count30columns, soUPDATEs (which only change counts) never touch it — the count-update path optimized by Fix Metrics collection; reduce no.of metrics; improve slow request lo… #25751 is untouched and the upsert-deadlock fix is preserved.Migrations are guarded/idempotent (MySQL prepared-statement check on
information_schema.statistics; PostgresCREATE INDEX IF NOT EXISTS). Please add the To release label so this cherry-picks into 1.13.x.Follow-up (separate PR)
computePercentileis still O(n²) (correlatedCOUNT(*)subqueries). This index restores the pre-1.13 working behavior; the durable fix is to rewrite it withPERCENT_RANK()window functions (the MySQL 5.7 constraint noted in the DAO comment is stale — tests run on MySQL 8).Greptile Summary
This PR adds a usage lookup index for the percentile endpoint.
entity_usage (usageDate, entityType).id-based usage index path unchanged.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
entity_usagepercentile lookup index.Reviews (3): Last reviewed commit: "fix(usage): build entity_usage percentil..." | Re-trigger Greptile
Context used (3)