[WEB-4823] chore: Add compound indexing for notification fields to improve query performance #7691
Conversation
WalkthroughAdds explicit database indexes to Notification, FileAsset, UserFavorite, and PageLog models via Meta.indexes and introduces a non-atomic migration creating these indexes concurrently across the four tables. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/plane/db/models/notification.py (3)
17-18: Good call adding indexes; consider trigram for entity_name if using icontains/ILIKE
- entity_identifier and entity_name indexes look right.
- If entity_name is filtered with icontains/ILIKE/prefix, add a GIN trigram index; B-Tree won’t help those queries.
Add this (Postgres-only):
class Meta: ... indexes = [ + # Fast icontains/ILIKE on entity_name + GinIndex(fields=["entity_name"], name="notifications_entity_name_trgm", opclasses=["gin_trgm_ops"]), ]Also add the import elsewhere in this file:
from django.contrib.postgres.indexes import GinIndexAnd ensure pg_trgm is enabled via a migration:
from django.contrib.postgres.operations import TrigramExtension
33-33: Unread-focused index can be smaller and fasterIndexing read_at is useful; for unread counts/views, a partial index WHERE read_at IS NULL on (receiver, workspace, -created_at) typically outperforms a full (… , read_at) index and helps pagination.
Example (Meta.indexes):
from django.db.models import Q ... indexes = [ + models.Index( + fields=["receiver", "workspace", "-created_at"], + name="notifications_unread_rx_ws_createdat_idx", + condition=Q(read_at__isnull=True), + ), ]
42-51: Drop redundant indexes and optimize composite for pagination
- Remove
receiver_workspace_idxandreceiver_read_at_idx—the existing(receiver, workspace, read_at)index covers both.- Extend your composite index to support
ORDER BY snoozed_till, -created_at(e.g.fields=["receiver","workspace","read_at","snoozed_till","-created_at"]) for efficient LIMIT pagination.- Prefix names with
notifications_for cross-table uniqueness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py(1 hunks)apps/api/plane/db/models/notification.py(3 hunks)
🔇 Additional comments (1)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py (1)
28-39: Use concurrent index creation (AddIndexConcurrently) and mark the migration non-atomicReplace migrations.AddIndex(...) with AddIndexConcurrently(...) (from django.contrib.postgres.operations) and set atomic = False because CREATE INDEX CONCURRENTLY cannot run inside a transaction. (docs.djangoproject.com, dokk.org)
Apply the original suggested diff (unchanged):
-from django.db import migrations, models +from django.db import migrations, models +from django.contrib.postgres.operations import AddIndexConcurrently @@ class Migration(migrations.Migration): + # Required for CONCURRENTLY + atomic = False @@ - migrations.AddIndex( + AddIndexConcurrently( model_name='notification', index=models.Index(fields=['receiver', 'workspace'], name='receiver_workspace_idx'), ), - migrations.AddIndex( + AddIndexConcurrently( model_name='notification', index=models.Index(fields=['receiver', 'read_at'], name='receiver_read_at_idx'), ), - migrations.AddIndex( + AddIndexConcurrently( model_name='notification', index=models.Index(fields=['receiver', 'workspace', 'read_at'], name='receiver_workspace_read_at_idx'), ),Optional: add an unread-optimized partial index (Postgres-only) — descending ordering with '-' and partial index via condition are supported by Django Index. (docs.djangoproject.com)
Verification notes: rg shows the three index names only in apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py and no existing AddIndexConcurrently usages; repository did not expose a pinned Django version — confirm the project uses PostgreSQL and Django >= 3.0 before applying this change. (dokk.org, docs.djangoproject.com)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py (1)
13-27: Avoid long locks: create these single-field indexes concurrently and make the migration non-atomic.
AlterField(db_index=True)builds indexes inside the transaction and can take ACCESS EXCLUSIVE locks on a largenotificationtable. Switch to concurrent index creation viaSeparateDatabaseAndState+AddIndexConcurrentlyand setatomic = False.Apply:
@@ -from django.db import migrations, models +from django.db import migrations, models +from django.db.models import Q +from django.contrib.postgres.operations import AddIndexConcurrently @@ -class Migration(migrations.Migration): +class Migration(migrations.Migration): + # Required for *Concurrently operations + atomic = False @@ - operations = [ - migrations.AlterField( - model_name='notification', - name='entity_identifier', - field=models.UUIDField(db_index=True, null=True), - ), - migrations.AlterField( - model_name='notification', - name='entity_name', - field=models.CharField(db_index=True, max_length=255), - ), - migrations.AlterField( - model_name='notification', - name='read_at', - field=models.DateTimeField(db_index=True, null=True), - ), - migrations.AddIndex( - model_name='notification', - index=models.Index(fields=['receiver', 'read_at'], name='receiver_read_at_idx'), - ), - ] + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='notification', + name='entity_identifier', + field=models.UUIDField(db_index=True, null=True), + ), + migrations.AlterField( + model_name='notification', + name='entity_name', + field=models.CharField(db_index=True, max_length=255), + ), + migrations.AlterField( + model_name='notification', + name='read_at', + field=models.DateTimeField(db_index=True, null=True), + ), + ], + database_operations=[ + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['entity_identifier'], name='notification_entity_identifier_idx'), + ), + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['entity_name'], name='notification_entity_name_idx'), + ), + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['read_at'], name='notification_read_at_idx'), + ), + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['receiver', 'read_at'], name='receiver_read_at_idx'), + ), + ], + ), + ]
🧹 Nitpick comments (4)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py (4)
25-26: Speed up unread counts: add a partial index on unread rows.If unread =
read_at IS NULLis common, a partial index is smaller and faster.@@ database_operations=[ ... + AddIndexConcurrently( + model_name='notification', + index=models.Index( + fields=['receiver', 'workspace'], + name='receiver_workspace_unread_idx', + condition=Q(read_at__isnull=True), + ), + ),
21-21: Validate entity_name access patterns; consider pattern-ops or trigram index if prefix/ILIKE searches are used.If queries use
ILIKE 'prefix%', a btree withvarchar_pattern_ops(orpg_trgmfor general substring) is more effective than a plain btree. Optional and Postgres-specific.I can propose an alternative index once you confirm query patterns and Postgres usage in prod.
30-30: Index naming nit: consider app/table prefix to avoid cross-table name collisions.E.g.,
db_notification_receiver_read_at_idxinstead ofreceiver_read_at_idx. Optional.
12-32: Audit and refine Notification indexes
- Queries observed only filter by
workspace__slug+receiver_id, byworkspace__slug+receiver_id+read_at__isnull, then byentity_nameon that result set- The composite index on (
receiver,read_at) covers the unread‐filter but cannot help withworkspaceand the standaloneread_atindex has no standalone use—drop it- No filters on
entity_identifierfound—its index appears unusedentity_nameis always filtered alongsideworkspace+receiver—consider a composite index on (workspace_id,receiver_id,entity_name) instead of its single-field index
Verify these query shapes against production workloads and adjust your index set accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py(1 hunks)apps/api/plane/db/models/notification.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/db/models/notification.py
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py
Outdated
Show resolved
Hide resolved
apps/api/plane/db/migrations/0103_alter_notification_entity_identifier_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/api/plane/db/models/favorite.py (2)
18-19: Avoid redundant single-column index on entity_type.Since a composite index on (entity_type, entity_identifier) is added below, the standalone index on entity_type is likely redundant. Keep a single-column index on entity_identifier only if you do queries by identifier alone.
Apply this diff:
- entity_type = models.CharField(max_length=100, db_index=True) + entity_type = models.CharField(max_length=100)
44-49: Tune index for common access pattern: include user and make it partial on active rows.Favorites are typically fetched per user and active (deleted_at IS NULL). Indexing only (entity_type, entity_identifier) may not help those queries.
Apply this diff:
- indexes = [ - models.Index( - fields=["entity_type", "entity_identifier"], - name="user_favorite_entity_idx", - ), - ] + indexes = [ + models.Index( + fields=["user", "entity_type", "entity_identifier"], + name="user_favorite_user_entity_idx", + condition=models.Q(deleted_at__isnull=True), + ), + ]To verify, EXPLAIN your most frequent SELECTs on user_favorites and confirm this index is chosen.
apps/api/plane/db/models/asset.py (2)
64-67: Trim redundant index and validate identifier type.
- Composite index below covers the leftmost column; drop the standalone index on entity_type to reduce write overhead.
- entity_identifier is CharField while other models use UUIDField; if values are UUIDs, consider UUIDField for storage/validation benefits.
Apply this diff to remove the redundant single-column index:
- entity_type = models.CharField(max_length=255, null=True, blank=True, db_index=True) + entity_type = models.CharField(max_length=255, null=True, blank=True)Confirm whether FileAsset.entity_identifier stores mixed formats; if always UUIDs, we can plan a safe data migration to UUIDField.
81-86: Confirm column order matches filter/sort pattern.If queries often filter by entity_identifier (and optionally entity_type), indexing (entity_identifier, entity_type) yields better selectivity. Otherwise current order is fine since you also have a single-column index on entity_identifier.
Optional reorder:
- models.Index( - fields=["entity_type", "entity_identifier"], - name="file_asset_entity_idx", - ), + models.Index( + fields=["entity_identifier", "entity_type"], + name="file_asset_entity_idx", + ),Also consider a partial index excluding soft-deleted/archived rows if most queries do so (condition=Q(is_deleted=False, is_archived=False)).
apps/api/plane/db/models/page.py (2)
102-108: Drop low-selectivity single-column indexes now covered by composites.entity_name/entity_type typically have low cardinality. Since composite indexes below start with these columns, standalone db_index=True on them is unnecessary. Keep the single-column index on entity_identifier if you filter by it alone.
Apply this diff:
- entity_identifier = models.UUIDField(null=True, blank=True, db_index=True) - entity_name = models.CharField( - max_length=30, verbose_name="Transaction Type", db_index=True - ) - entity_type = models.CharField( - max_length=30, verbose_name="Entity Type", null=True, blank=True, db_index=True - ) + entity_identifier = models.UUIDField(null=True, blank=True, db_index=True) + entity_name = models.CharField( + max_length=30, verbose_name="Transaction Type" + ) + entity_type = models.CharField( + max_length=30, verbose_name="Entity Type", null=True, blank=True + )
119-128: Validate composite index order against real queries.If the common filter is by entity_identifier (optionally plus type/name), reversing order to start with entity_identifier can help. Otherwise current design + single-column entity_identifier index is acceptable.
Optional swap example:
- models.Index( - fields=["entity_type", "entity_identifier"], - name="page_log_entity_idx", - ), + models.Index( + fields=["entity_identifier", "entity_type"], + name="page_log_entity_idx", + ), - models.Index( - fields=["entity_name", "entity_identifier"], - name="page_log_entity_identifier_idx", - ), + models.Index( + fields=["entity_identifier", "entity_name"], + name="page_log_entity_identifier_idx", + ),Run EXPLAIN on your hot PageLog queries to confirm.
apps/api/plane/db/migrations/0103_alter_fileasset_entity_identifier_and_more.py (2)
28-37: Reassess single-column indexes on notification.Indexing entity_name and read_at individually may be unnecessary if queries always include receiver. Prefer (receiver, read_at) plus the partial unread index suggested above; drop the standalone read_at index to save space.
If you agree, remove db_index=True from Notification.read_at in the model and replace with the partial index via a follow-up migration.
12-23: Heads-up: AlterField with db_index=True also creates indexes non-concurrently.Where feasible, avoid AlterField solely to add indexes on large tables. Instead, keep the model field without db_index and add explicit concurrent indexes via operations. This prevents write locks.
I can push a revised pair of migrations: (1) add concurrent indexes, (2) then update model fields without triggering new indexes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/api/plane/db/migrations/0103_alter_fileasset_entity_identifier_and_more.py(1 hunks)apps/api/plane/db/models/asset.py(2 hunks)apps/api/plane/db/models/favorite.py(2 hunks)apps/api/plane/db/models/notification.py(3 hunks)apps/api/plane/db/models/page.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/db/models/notification.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
apps/api/plane/db/migrations/0103_alter_fileasset_entity_identifier_and_more.py
Outdated
Show resolved
Hide resolved
apps/api/plane/db/migrations/0103_alter_fileasset_entity_identifier_and_more.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds database indexes to optimize query performance for notification-related operations and entity lookups across multiple models. The changes focus on improving performance for notification filtering, entity identification, and user preference queries.
- Added single-field indexes (
db_index=True) to commonly queried fields across models - Implemented compound indexes for frequently used field combinations
- Included corresponding Django migration to apply these database schema changes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/db/models/notification.py | Added indexes for entity fields and read status to optimize notification queries |
| apps/api/plane/db/models/page.py | Added indexes for entity tracking fields in page logs |
| apps/api/plane/db/models/favorite.py | Added indexes for entity type and identifier lookups in user favorites |
| apps/api/plane/db/models/asset.py | Added indexes for entity fields in file assets |
| apps/api/plane/db/migrations/0103_alter_fileasset_entity_identifier_and_more.py | Django migration implementing all index changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/api/plane/db/models/page.py (1)
117-127: Trim redundant single-column indexes; keep composites that cover them.Postgres can satisfy queries on the leftmost column from a composite index. If you rarely filter by
entity_typeorentity_namealone, consider dropping those single-column indexes to cut bloat and write-amplification. Keepentity_identifier(useful standalone) and the two composites.Example diff to de-dup:
indexes = [ - models.Index(fields=["entity_type"], name="pagelog_entity_type_idx"), models.Index(fields=["entity_identifier"], name="pagelog_entity_id_idx"), - models.Index(fields=["entity_name"], name="pagelog_entity_name_idx"), models.Index( fields=["entity_type", "entity_identifier"], name="pagelog_type_id_idx" ), models.Index( fields=["entity_name", "entity_identifier"], name="pagelog_name_id_idx" ), ]apps/api/plane/db/models/asset.py (1)
79-87: Avoid overlapping indexes unless query patterns demand them.
(entity_type, entity_identifier)covers filters onentity_typealone. If standaloneentity_typelookups aren’t common, drop that single-column index; keepentity_identifierand the composite.indexes = [ - models.Index(fields=["entity_type"], name="asset_entity_type_idx"), models.Index( fields=["entity_identifier"], name="asset_entity_identifier_idx" ), models.Index( fields=["entity_type", "entity_identifier"], name="asset_entity_idx" ), ]apps/api/plane/db/models/favorite.py (1)
44-52: Reduce index overlap on favorites.Same rationale: if you don’t filter by
entity_typealone frequently, the composite can serve leftmost scans. Consider removing the single-columnentity_typeindex.indexes = [ - models.Index(fields=["entity_type"], name="fav_entity_type_idx"), models.Index( fields=["entity_identifier"], name="fav_entity_identifier_idx" ), models.Index( fields=["entity_type", "entity_identifier"], name="fav_entity_idx" ), ]apps/api/plane/db/models/notification.py (1)
42-49: Refine Notification model indexes
Align indexes with filters (read_at__isnull) and ordering (snoozed_till,-created_at) inapps/api/plane/app/views/notification/base.py:
- Drop standalone
read_atindex (no global scans byread_at).- Replace
(receiver, read_at)index with composite on(receiver, snoozed_till, -created_at)to avoid filesort.- Add partial index on
receiverfor unread queries.class Meta: indexes = [ models.Index(fields=["entity_identifier"], name="notif_entity_identifier_idx"), models.Index(fields=["entity_name"], name="notif_entity_name_idx"), - models.Index(fields=["read_at"], name="notif_read_at_idx"), - models.Index(fields=["receiver","read_at"], name="notif_entity_idx"), + # drop if unused globally + # models.Index(fields=["read_at"], name="notif_read_at_idx"), + # support ORDER BY snoozed_till, -created_at per-view + models.Index( + fields=["receiver", "snoozed_till", "-created_at"], + name="notif_receiver_snoozed_created_idx", + ), + # speed up unread counts/filters by receiver + models.Index( + fields=["receiver"], + name="notif_unread_receiver_idx", + condition=models.Q(read_at__isnull=True), + ), ]apps/api/plane/db/migrations/0103_fileasset_asset_entity_type_idx_and_more.py (1)
14-75: Keep migrations in sync, rename indexes for clarity and drop redundant ops; manually verify no naming collisions.
- Confirm model Meta now exactly defines these indexes to prevent automatic “AddIndex” migrations—this matches the current migration.
- Rename
notif_entity_idxtonotif_receiver_read_at_idx; if adopting the 3-column or partial indexes below, update this migration to match.- Remove any AddIndexConcurrently for single-column indexes you drop from the models (e.g., the standalone
notif_read_at_idx).Example diffs (if adopting notification enhancements):
- AddIndexConcurrently( - model_name='notification', - index=models.Index(fields=['receiver', 'read_at'], name='notif_entity_idx'), - ), + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['receiver', 'read_at', '-created_at'], name='notif_receiver_read_at_created_idx'), + ), + AddIndexConcurrently( + model_name='notification', + index=models.Index(fields=['receiver'], name='notif_unread_receiver_idx', condition=models.Q(read_at__isnull=True)), + ), - AddIndexConcurrently( - model_name='notification', - index=models.Index(fields=['read_at'], name='notif_read_at_idx'), - ),Manually confirm no existing DB indexes use these names:
notif_receiver_read_at_created_idx,notif_unread_receiver_idx,notif_entity_idx,notif_read_at_idx.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/api/plane/db/migrations/0103_fileasset_asset_entity_type_idx_and_more.py(1 hunks)apps/api/plane/db/models/asset.py(1 hunks)apps/api/plane/db/models/favorite.py(1 hunks)apps/api/plane/db/models/notification.py(1 hunks)apps/api/plane/db/models/page.py(2 hunks)
🔇 Additional comments (2)
apps/api/plane/db/models/page.py (1)
104-106: No functional change in field; LGTM.Formatting-only update; null/blank semantics unchanged.
apps/api/plane/db/migrations/0103_fileasset_asset_entity_type_idx_and_more.py (1)
8-8: atomic=False is correct for concurrent index creation.Prevents “CREATE INDEX CONCURRENTLY” from running inside a transaction.
Description
Added compound and single-field database indexes to optimize performance filtering.
Indexes have been added for the following table fields:
These indexes will significantly improve performance for notification filters, especially in inbox views, pagination, and unread count computations.
Type of Change
References
[WEB-4823]
Summary by CodeRabbit