fix(list): use JSONB containment for RemoteURL filter (GIN-indexable) (#1220)
## Summary Same shape of bug as #1215's cursor fix, different filter. The RemoteURL filter built its WHERE predicate using `jsonb_array_elements` + `->>` extraction, which the planner can't translate into a GIN search. Replaced with a JSONB containment predicate `@>` that uses `idx_servers_json_remotes`. ## Diagnosis (from the per-phase publish slog #1215 added) Today's first publish-latency alert at 17:08 UTC pointed straight at this: ``` publish complete server_name=dev.storage/mcp version=1.10.1 total_ms=14822 validate_ms=0 lock_ms=238 remotes_ms=10980 version_checks_ms=535 unmark_ms=3014 create_ms=53 ``` `remotes_ms=10,980` on a server with **one** remote URL pointed at `validateNoDuplicateRemoteURLs`, which runs the RemoteURL filter against the database. EXPLAIN ANALYZE on prod confirmed: ``` Filter: (status != 'deleted') AND EXISTS(SubPlan 1) Rows Removed by Filter: 21,092 ← scans the whole table Buffers: shared hit=20,028 Execution Time: 10,005 ms ``` ## Fix `internal/database/postgres.go:99` rewritten: ```sql -- before (planner can't use GIN index for this) EXISTS (SELECT 1 FROM jsonb_array_elements(value->'remotes') AS remote WHERE remote->>'url' = $1) -- after (GIN-indexable) value -> 'remotes' @> jsonb_build_array(jsonb_build_object('url', $1::text)) ``` Semantically equivalent for our schema (`url` is always a string, NOT NULL). ## Local benchmark — 20K rows, prod-shaped JSONB (~1KB/row) | Form | Buffer hits | Index used | Time | |------|------------:|------------|-----:| | OLD | 40,398 | none — full scan | 17.0 ms | | NEW | **294** | **`idx_servers_json_remotes` GIN bitmap** | **1.6 ms** | Maps to prod's 10,005 ms cold-cache observation — same shape of speedup we got from the cursor fix. ## Scope honesty This PR addresses the **publish slowness path** (`validateNoDuplicateRemoteURLs` in the publish transaction). It directly resolves today's 17:08 UTC `Publish Endpoint Latency` alert. It does **not** address: - Today's 17:35–17:40 UTC `Availability dropped below 95%` alert. Per-investigation: only 4 publishes happened in the entire 17:00–17:50 window and zero during the burst minutes — so my initial "concurrent slow publishes starved the pool" hypothesis was wrong. The actual cause was scraper-driven concurrency (4,500+ requests in 5 min) on plain cursor pagination + ILIKE substring search, where individual queries are fast (mean 42 ms) but tail latency under concurrency reaches 10 s server-side and 20–35 s nginx-level due to pool queueing. - The 18:17 UTC `Publish Endpoint Latency` re-fire (same scraper-driven concurrency). A follow-up PR will raise pool size + PG `max_connections` + add explicit PG resource limits to absorb concurrency without queue blowup. That fix has a different shape (config) and a different risk profile (PG restart for `max_connections`); kept separate so this surgical SQL change can ship cleanly. ## Test plan - [x] `go build ./...` clean - [x] `make lint` clean - [x] `go test -race ./internal/database/... ./internal/service/...` green — existing `TestPostgreSQL_ListServers/filter by remote URL` covers semantic correctness - [x] EXPLAIN ANALYZE comparison locally on 20K rows + prod-shaped JSONB - [ ] After merge + deploy: verify on prod via `EXPLAIN ANALYZE`; confirm `remotes_ms` in `publish complete` slog drops to single-digit ms ## Out of scope (separate follow-ups) - pgxpool `MaxConns` / `MinConns` bump + PG `max_connections` raise + PG `resources` limits — addresses the cursor-tail and ILIKE-search concurrency that this PR doesn't touch - Per-IP rate limiting at nginx — defends against scraper retry storms regardless of query speed - `Cache-Control` headers on `/v0/servers` — let nginx absorb scraper repeats - Pre-existing `superfluous WriteHeader` warnings — separate Huma framework issue - ILIKE substring search on `server_name` is unindexable — could be replaced with `pg_trgm` GIN index or full-text search 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a490b6f1ec
commit
54cbe2473c
1 changed files with 9 additions and 1 deletions
|
|
@ -96,7 +96,15 @@ func buildFilterConditions(filter *ServerFilter, argIndex int) ([]string, []any,
|
|||
argIndex++
|
||||
}
|
||||
if filter.RemoteURL != nil {
|
||||
conditions = append(conditions, fmt.Sprintf("EXISTS (SELECT 1 FROM jsonb_array_elements(value->'remotes') AS remote WHERE remote->>'url' = $%d)", argIndex))
|
||||
// Use a JSONB containment predicate so the planner can use the GIN index
|
||||
// idx_servers_json_remotes on (value -> 'remotes'). The previously-used
|
||||
// EXISTS / jsonb_array_elements / ->> form is logically equivalent but
|
||||
// the planner can't translate the per-row array unfolding into a GIN
|
||||
// search — it falls back to scanning every row. validateNoDuplicateRemoteURLs
|
||||
// in the publish path ran this filter and was measured at ~10s on prod's
|
||||
// 21K-row table on 2026-04-28; the containment form is GIN-indexable and
|
||||
// completes in low single-digit ms.
|
||||
conditions = append(conditions, fmt.Sprintf("value -> 'remotes' @> jsonb_build_array(jsonb_build_object('url', $%d::text))", argIndex))
|
||||
args = append(args, *filter.RemoteURL)
|
||||
argIndex++
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue