Add users/groups zone+created indices (validated subset of #3347) - #4063
Add users/groups zone+created indices (validated subset of #3347)#4063duanemay wants to merge 1 commit into
Conversation
Individually evaluates all 5 index candidates proposed in #3347 against a seeded dataset (~50k users / ~20k groups / ~150k group_membership / 500 external_group_mapping rows) on both MySQL 8 and PostgreSQL 17, using Gatling load tests plus EXPLAIN/EXPLAIN ANALYZE plan inspection. Uses V4_115 because V4_114 is already claimed by another in-flight PR. Winners (included here, both dialects): - idx_users_zone_id_created ON users (identity_zone_id, created) - idx_groups_zone_id_created ON groups (identity_zone_id, created) Both convert a full table scan + sort into an index-only lookup for GET /Users and GET /Groups's default zone-scoped, created-ordered listing query: - MySQL: EXPLAIN Extra goes from "Using where; Using filesort" to nothing; measured GET /Users min response time 280ms -> 163ms, p99 3250ms -> 1828ms under a modest sustained load. - PostgreSQL: EXPLAIN ANALYZE goes from Seq Scan + top-N heapsort Sort to a pure Index Scan; raw query execution time 51.5ms -> 3.2ms (users) and 13.7ms -> 1.9ms (groups). End-to-end HTTP latency didn't move much at this test's concurrency because Postgres's planner already handles ORDER BY ... LIMIT via an efficient top-N heapsort even on a full scan -- but the same structural gap exists (groups had zero usable index for a zone-scoped listing at all before this), so it's included for parity and to protect larger/busier deployments and non-default work_mem settings where that heapsort optimization doesn't apply as cheaply. Not included (dropped after evaluation): - idx_group_membership_id_zone_id (group_id, identity_zone_id) and idx_group_membership_zone_member_group (identity_zone_id, member_id, group_id): neither MySQL nor Postgres's optimizer preferred these over the existing single-column indexes; measured floor response time was unchanged. identity_zone_id on group_membership is fully derived from group_id (backfilled from `groups.identity_zone_id` via group_id in V4_0_6__Add_Identity_Zone_Id_To_Tables.sql) -- and UAA already tried an identity_zone_id-leading composite key on this exact table in V2_4_1__Zonify_Group_Memberships.sql, then reverted it in V2_5_4__Zonify_Groups.sql ("remove zone id from the group_membership table - it is derived from group_id"), on all three dialects. That revert already settled this years ago. - idx_external_group_zone_id_created (group_id, identity_zone_id) on external_group_mapping: EXPLAIN confirms a real win for its query (group_id-based internal SAML/LDAP group resolution), but it's not reachable via any public REST endpoint to load-test, and the table is tiny by default -- left out of this PR pending a deployment that actually has enough external group mappings to matter; can be revisited separately. Before this PR, PostgreSQL and MySQL were already at parity: neither had any of the 5 candidate indices. PR #3347 itself only wrote real SQL for MySQL, leaving postgresql/hsqldb as `-- NOOP` -- if merged as-is it would have created a new MySQL-only disparity. This PR adds real SQL for both MySQL and PostgreSQL instead.
There was a problem hiding this comment.
🟢 Approval recommended
The migration scripts are minimal, syntactically consistent with existing patterns in the repo, and cleanly add the intended indexes for MySQL/PostgreSQL without introducing behavioral changes.
Pull request overview
Adds zone-scoped created-ordering composite indexes to improve the default listing queries for GET /Users and GET /Groups, keeping MySQL and PostgreSQL aligned via a shared Flyway version (V4_115) while leaving HSQLDB as a no-op migration.
Changes:
- Add
(identity_zone_id, created)index onusersfor MySQL and PostgreSQL. - Add
(identity_zone_id, created)index ongroupsfor MySQL and PostgreSQL. - Introduce a matching
V4_115migration file for HSQLDB (NOOP) to keep Flyway versioning consistent across dialect directories.
File summaries
| File | Description |
|---|---|
| server/src/main/resources/org/cloudfoundry/identity/uaa/db/postgresql/V4_115__Add_idx_users_and_groups_zone_id_created.sql | Adds Postgres composite indexes for zone-scoped, created-ordered listings (transactional, IF NOT EXISTS). |
| server/src/main/resources/org/cloudfoundry/identity/uaa/db/mysql/V4_115__Add_idx_users_and_groups_zone_id_created.sql | Adds MySQL composite indexes for zone-scoped, created-ordered listings. |
| server/src/main/resources/org/cloudfoundry/identity/uaa/db/hsqldb/V4_115__Add_idx_users_and_groups_zone_id_created.sql | HSQLDB migration placeholder (-- NOOP) for version alignment. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The HSQLDB migration is a NOOP and should add the equivalent indexes to keep cross-dialect migrations consistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| @@ -0,0 +1 @@ | |||
| -- NOOP No newline at end of file | |||
Summary
Individually evaluates all 5 index candidates proposed in #3347 against a seeded dataset (~50k users / ~20k groups / ~150k
group_membership/ 500external_group_mappingrows) on both MySQL 8 and PostgreSQL 17, using Gatling load tests plusEXPLAIN/EXPLAIN ANALYZEplan inspection.Uses
V4_115becauseV4_114is already claimed by another in-flight PR.Included (both dialects, this PR):
idx_users_zone_id_created ON users (identity_zone_id, created)idx_groups_zone_id_created ON groups (identity_zone_id, created)Both convert a full table scan + sort into an index-only lookup for
GET /UsersandGET /Groups's default zone-scoped,created-ordered listing query:EXPLAINExtragoes fromUsing where; Using filesortto nothing; measuredGET /Usersmin response time 280ms → 163ms, p99 3250ms → 1828ms under a modest sustained load.EXPLAIN ANALYZEgoes fromSeq Scan+ top-N heapsortSortto a pureIndex Scan; raw query execution time 51.5ms → 3.2ms (users) and 13.7ms → 1.9ms (groups). End-to-end HTTP latency didn't move much at this test's concurrency because Postgres's planner already handlesORDER BY ... LIMITvia an efficient top-N heapsort even on a full scan — but the same structural gap exists (groupshad zero usable index for a zone-scoped listing at all before this), so it's included for parity and to protect larger/busier deployments and non-defaultwork_memsettings where that heapsort optimization doesn't apply as cheaply.Before this PR, PostgreSQL and MySQL were already at parity: neither had any of the 5 candidate indices. #3347 itself only wrote real SQL for MySQL, leaving
postgresql/hsqldbas-- NOOP— if merged as-is it would have created a new MySQL-only disparity. This PR adds real SQL for both MySQL and PostgreSQL instead.Not included (dropped after evaluation):
idx_group_membership_id_zone_id (group_id, identity_zone_id)andidx_group_membership_zone_member_group (identity_zone_id, member_id, group_id): neither MySQL's nor Postgres's optimizer preferred these over the existing single-column indexes; measured floor response time was unchanged.identity_zone_idongroup_membershipis fully derived fromgroup_id(backfilled fromgroups.identity_zone_idviagroup_idinV4_0_6__Add_Identity_Zone_Id_To_Tables.sql) — and UAA already tried anidentity_zone_id-leading composite key on this exact table inV2_4_1__Zonify_Group_Memberships.sql, then reverted it inV2_5_4__Zonify_Groups.sql("remove zone id from the group_membership table - it is derived from group_id"), on all three dialects. That revert already settled this years ago.idx_external_group_zone_id_created (group_id, identity_zone_id)onexternal_group_mapping:EXPLAINconfirms a real win for its query (group_id-based internal SAML/LDAP group resolution), but it's not reachable via any public REST endpoint to load-test, and the table is tiny by default — left out of this PR pending a deployment that actually has enough external group mappings to matter; can be revisited separately.Closes/supersedes the MySQL-only, unreviewed portion of #3347 with an individually-validated, both-dialect subset.
Test plan
EXPLAIN/EXPLAIN ANALYZEplans before/after each candidate indexGET /Users,GET /Groups,GET /Groups/{id}/members,GET /Users/{id}before/after each candidate, individually and combined