Skip to content

perf(groups): scale membership summaries - #772

Open
dr-hoseyn wants to merge 1 commit into
PasarGuard:devfrom
dr-hoseyn:perf/group-membership-reverse-indexes
Open

perf(groups): scale membership summaries#772
dr-hoseyn wants to merge 1 commit into
PasarGuard:devfrom
dr-hoseyn:perf/group-membership-reverse-indexes

Conversation

@dr-hoseyn

@dr-hoseyn dr-hoseyn commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • calculate Group.total_users in SQL for the group-list query instead of hydrating every related User
  • add covering reverse indexes for group-centric user/inbound membership lookups and bulk deletes
  • build and drop the PostgreSQL indexes concurrently to keep membership writes available during upgrades
  • preserve create/update/detail response behavior and add regression coverage

Closes #771

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Refactor / cleanup
  • Documentation
  • Tests / CI

Checklist

  • I tested the change locally or explained why it cannot be tested.
  • I added or updated tests for behavior changes.
  • I updated documentation, translations, or examples if needed. (Not applicable: API schema is unchanged.)
  • I checked database migrations when models or schema changed.
  • I did not include secrets, tokens, private keys, or unrelated changes.

Testing

  • uv run ruff check . — passed
  • uv run ruff format --check app/db/models.py app/db/crud/group.py tests/test_group_membership_indexes.py tests/api/test_group.py — passed
  • uv run pytest -q tests/test_group_membership_indexes.py tests/api/test_group.py::test_groups_get_counts_users_without_loading_user_rows — 3 passed
  • relevant group/bulk API suite — 18 passed
  • uv run alembic check on SQLite — no new upgrade operations
  • empty SQLite database upgrade to head, index introspection, and one-revision downgrade — passed
  • PostgreSQL offline SQL compilation — emits COMMIT, CREATE INDEX CONCURRENTLY, then BEGIN; downgrade emits DROP INDEX CONCURRENTLY
  • MySQL and SQLite migration SQL compilation — passed

Synthetic SQLite benchmark (500,000 memberships / 100 groups):

  • current full user hydration: 1,014.19 ms and 74.43 MiB traced Python allocations
  • SQL summaries without reverse index: 2,196.56 ms median (association scan)
  • SQL summaries with covering reverse index: 18.016 ms median and 0.0098 MiB peak (covering-index search)

The benchmark is directional and is not presented as a production guarantee.

Screenshots

Not applicable (backend/database-only change).

Notes for reviewers

  • The response schema and route behavior are unchanged.
  • PostgreSQL concurrent index creation must run outside a transaction; the migration uses Alembic's autocommit_block() for this path.
  • MySQL/MariaDB may create an implicit foreign-key index. The explicit composite definition gives the schema a deterministic covering index order for group-centric access; InnoDB can use it for foreign-key enforcement as well.
  • SQLite uses normal index DDL, matching its typical single-node deployment model.

Summary by CodeRabbit

  • Improvements

    • Group listings now display accurate member totals without loading individual member records.
    • Group retrieval is more efficient, especially for groups with many members.
    • Reverse membership lookups are faster through improved database indexing.
  • Bug Fixes

    • Corrected group member-count handling when group details are loaded incrementally.
  • Tests

    • Added coverage to verify member totals, relationship loading behavior, and membership index configuration.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 44d9abe6-9bde-4f87-aa20-14c246b2f4e8

📥 Commits

Reviewing files that changed from the base of the PR and between 3331421 and eddcb96.

📒 Files selected for processing (5)
  • app/db/crud/group.py
  • app/db/migrations/versions/a4d8c7e91b32_add_reverse_group_membership_indexes.py
  • app/db/models.py
  • tests/api/test_group.py
  • tests/test_group_membership_indexes.py

Walkthrough

Group summaries now populate total_users with a correlated SQL count and keep Group.users unloaded. Reverse composite indexes support group-centric association access. PostgreSQL migrations use concurrent index operations, with tests covering counts, loading behavior, and index definitions.

Changes

Group membership scaling

Layer / File(s) Summary
Reverse membership indexes
app/db/models.py, app/db/migrations/versions/..., tests/test_group_membership_indexes.py
Association tables define reverse composite indexes. The migration creates and removes them with PostgreSQL-specific concurrent operations. Tests verify index names and column order.
SQL-populated group totals
app/db/models.py, app/db/crud/group.py, tests/api/test_group.py
Group.total_users is a query expression populated by a correlated count. Group retrieval eagerly loads inbounds without materializing users. Existing user-loading paths assign the loaded count. Tests verify the count and unloaded relationship.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to eddcb

The change preserves existing response behavior and includes regression coverage and migration validation; no actionable merge-blocking risk remains.

Sequence Diagram(s)

sequenceDiagram
  participant GroupListAPI
  participant get_group
  participant SQLDatabase
  GroupListAPI->>get_group: Request group summaries
  get_group->>SQLDatabase: Count user associations and fetch groups
  SQLDatabase-->>get_group: Return groups with total_users
  get_group-->>GroupListAPI: Return summaries without Group.users
Loading

Suggested reviewers: immohammad20000, m03ed, x0sina

Poem

I’m a rabbit who counts without rows in my den,
Reverse indexes make the paths quick again.
Groups bring their totals, while users stay asleep,
Migrations build safely, and tests guard the heap.
Hop, hop—clean summaries leap!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the primary change: improving the scalability of group membership summaries.
Linked Issues check ✅ Passed The changes implement the SQL counts, unloaded relationships, reverse indexes, concurrent PostgreSQL migration operations, and regression tests required by issue #771.
Out of Scope Changes check ✅ Passed All code and test changes directly support scaling group membership summaries and the requirements in issue #771.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dr-hoseyn

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant