Skip to content

Commit bba6eb3

Browse files
authored
Add db indexes to user table (#3067)
Add composite indexes based on existing queries/usage, ensuring partial index prefixes are useful too. - `is_active` filtering is set in the default `User` manager - most of our user queries are per `organization` - multiple cases filter by `username` or `email` (most notably schedule related queries, given the low-level backend ical representation) Also rework how users are fetched from DB when getting users from schedules ical representation (which was particularly slow when regex filtering by required permission). Related to grafana/oncall-private#2163
1 parent a898835 commit bba6eb3

File tree

4 files changed

+42
-12
lines changed

4 files changed

+42
-12
lines changed

CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1616

1717
- Fix regression in public actions endpoint handling user field by @mderynck ([#3053](https://github.com/grafana/oncall/pull/3053))
1818

19+
### Changed
20+
21+
- Rework how users are fetched from DB when getting users from schedules ical representation ([#3067](https://github.com/grafana/oncall/pull/3067))
22+
1923
## v1.3.38 (2023-09-19)
2024

2125
### Fixed

engine/apps/schedules/ical_utils.py

+13-12
Original file line numberDiff line numberDiff line change
@@ -67,11 +67,10 @@
6767
def users_in_ical(
6868
usernames_from_ical: typing.List[str],
6969
organization: "Organization",
70-
) -> "UserQuerySet":
70+
) -> typing.List["User"]:
7171
"""
7272
This method returns a sequence of `User` objects, filtered by users whose username, or case-insensitive e-mail,
73-
is present in `usernames_from_ical`. If `include_viewers` is set to `True`, users are further filtered down
74-
based on their granted permissions.
73+
is present in `usernames_from_ical`.
7574
7675
Parameters
7776
----------
@@ -80,24 +79,26 @@ def users_in_ical(
8079
organization : apps.user_management.models.organization.Organization
8180
The organization in question
8281
"""
83-
from apps.user_management.models import User
82+
required_permission = RBACPermission.Permissions.SCHEDULES_WRITE
8483

8584
emails_from_ical = [username.lower() for username in usernames_from_ical]
8685

87-
# users_found_in_ical = organization.users
8886
users_found_in_ical = organization.users.filter(
89-
**User.build_permissions_query(RBACPermission.Permissions.SCHEDULES_WRITE, organization)
90-
)
91-
92-
users_found_in_ical = users_found_in_ical.filter(
9387
(Q(username__in=usernames_from_ical) | Q(email__lower__in=emails_from_ical))
9488
).distinct()
9589

96-
return users_found_in_ical
90+
if organization.is_rbac_permissions_enabled:
91+
# it is more efficient to check permissions on the subset of users filtered above
92+
# than performing a regex query for the required permission
93+
users_found_in_ical = [u for u in users_found_in_ical if {"action": required_permission.value} in u.permissions]
94+
else:
95+
users_found_in_ical = users_found_in_ical.filter(role__lte=required_permission.fallback_role.value)
96+
97+
return list(users_found_in_ical)
9798

9899

99100
@timed_lru_cache(timeout=100)
100-
def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> UserQuerySet:
101+
def memoized_users_in_ical(usernames_from_ical: typing.List[str], organization: "Organization") -> typing.List["User"]:
101102
# using in-memory cache instead of redis to avoid pickling python objects
102103
return users_in_ical(usernames_from_ical, organization)
103104

@@ -354,7 +355,7 @@ def list_users_to_notify_from_ical_for_period(
354355
schedule: "OnCallSchedule",
355356
start_datetime: datetime.datetime,
356357
end_datetime: datetime.datetime,
357-
) -> UserQuerySet:
358+
) -> typing.List["User"]:
358359
users_found_in_ical: typing.Sequence["User"] = []
359360
events = schedule.final_events(start_datetime, end_datetime)
360361
usernames = []
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
# Generated by Django 3.2.20 on 2023-09-26 22:03
2+
3+
from django.db import migrations, models
4+
5+
6+
class Migration(migrations.Migration):
7+
8+
dependencies = [
9+
('user_management', '0014_auto_20230728_0802'),
10+
]
11+
12+
operations = [
13+
migrations.AddIndex(
14+
model_name='user',
15+
index=models.Index(fields=['is_active', 'organization', 'username'], name='user_manage_is_acti_385fc4_idx'),
16+
),
17+
migrations.AddIndex(
18+
model_name='user',
19+
index=models.Index(fields=['is_active', 'organization', 'email'], name='user_manage_is_acti_7e930d_idx'),
20+
),
21+
]

engine/apps/user_management/models/user.py

+4
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,10 @@ class Meta:
169169
# Including is_active to unique_together and setting is_active to None allows to
170170
# have multiple deleted users with the same user_id, but user_id is unique among active users
171171
unique_together = ("user_id", "organization", "is_active")
172+
indexes = [
173+
models.Index(fields=["is_active", "organization", "username"]),
174+
models.Index(fields=["is_active", "organization", "email"]),
175+
]
172176

173177
public_primary_key = models.CharField(
174178
max_length=20,

0 commit comments

Comments
 (0)