perf(mcp-registry): index the columns the linking lookups filter on - #95865
perf(mcp-registry): index the columns the linking lookups filter on#95865posthog[bot] wants to merge 2 commits into
Conversation
Both `.first()` lookups in `resolve_measured_server` run once per measured (team, server_name) pair, and neither could reach an index. `registry_name` was only covered by the partial unique index `unique_registry_name_when_set`. A parameterized equality cannot be proved to exclude the empty string, so a generic plan discards the partial index and scans the table. `display_name` had no index at all, so the standalone lookup fell back to a bitmap scan over the near-useless `is_measured` index. Adds a plain index on `registry_name` and a composite index over (display_name, listed_in_registry, is_measured), both built CONCURRENTLY. Generated-By: PostHog Desktop Task-Id: 61d1912d-652b-4077-b0a3-c52e9745d90c
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). Resolved comments: 1 fixed |
🤖 CI report
|
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
The Migration risk check has not finished for this commit, so stamphog cannot tell a safe migration from a risky one yet. The review runs again on the next push, or you can re-request it once the check reports.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 37L, 2F substantive, 39L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (39L, 3F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 38550c6 · reviewed head 38550c6 |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The plain index was justified by a generic plan that this codebase never produces. Django's PostgreSQL backend binds parameters client-side and leaves prepared statements disabled, so an ORM equality lookup reaches the server with the value inlined as a literal. PostgreSQL then plans it as a custom plan, which can prove that `registry_name = '<literal>'` satisfies the partial predicate on `unique_registry_name_when_set` and uses that index. Keeping the index would add a concurrent build, storage, and a write on every registry crawl upsert without removing a scan. The composite (display_name, listed_in_registry, is_measured) index stays. It covers the standalone-row lookup the aggregation loop runs per measured server, where no index existed at all. Generated-By: PostHog Desktop Task-Id: 67edfeb8-2632-44d7-aec7-d85a63dddb2d
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
The Migration risk check has not finished for this commit, so stamphog cannot tell a safe migration from a risky one yet. The review runs again on the next push, or you can re-request it once the check reports.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 29L, 2F substantive, 31L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (31L, 3F, single-area, perf) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 3b19027 · reviewed head 3b19027 |
Problem
resolve_measured_serverruns two Django.first()lookups per measured (team, server_name) pair. Neither could reach an index.public.mcp_registry_serverin EU prod on theORDER BY id ASC LIMIT $2shape that.first()produces.registry_namedisplay_name,listed_in_registry,is_measureddisplay_nameis_measuredThe
registry_namecase is the subtle one. The column already carries the partial unique indexunique_registry_name_when_set, whose predicate excludes the empty string. Postgres uses it for a literal value, but a parameterized equality cannot be proved to satisfy that predicate, so a generic plan drops the index and scans.Changes
registry_name, which a generic plan can use where the partial unique index is unavailable.display_name,listed_in_registry,is_measured, matching the standalone-row lookup exactly.SafeAddIndexConcurrently, so the migration takes noACCESS EXCLUSIVElock on the table.No behavior change, no schema change beyond the two indexes.
How did you test this code?
Automated tests run locally:
products/mcp_registry/backend/tests/test_linking.pyandtest_aggregation.py— 6 passed.makemigrations --checkreports no state drift.analyze_migration_risk --fail-on-blockedrates both operations Safe.The plans were measured directly, because Postgres planner statistics are not visible from PostHog analytics and pganalyze is not reachable from the agent surface.
Plan comparison, PostgreSQL scratch database, 200k synthetic rows
Queries taken from
sql_with_params()on the two real querysets, then forced to a generic plan (plan_cache_mode = force_generic_plan) to reproduce the parameterized form the ORM sends.No new tests. A test asserting an index exists would restate migration state that
makemigrations --checkalready verifies, and would not catch the planner behavior this PR fixes.Automatic notifications
Docs update
None. No user-facing behavior, API, or documented workflow changes.
🤖 Agent context
Autonomy: Fully autonomous
/django-migrations,/writing-pr-descriptions,/writing-code-comments,/writing-simplified-technical-english.gh pr list --state open --searchonmcp_registryand on index keywords found nothing touching these lookups. PR feat(mcp-registry): make discovery reachable by agents and people #95309 is open on the same product but on the discovery surface, not on linking.generate_series, not copied from anywhere.registry_nameindex looked redundant at first, since a literal-valueEXPLAINdoes use the partial unique index; only forcing a generic plan showed the seq scan, which is what the ORM actually gets. Anddisplay_nameleads the composite index rather than the two booleans, so the index also serves adisplay_name-only equality later.Created with PostHog Desktop from this inbox report.