Improve notification recipients builder db performance [RUN ALL RSPEC] [RUN AS-IF-FOSS]
What does this MR do?
Related to #325469 (comment 538685161)
This MR attempts to reduce the number of unnecessary user_ids listed in the SQL query. Some of them have more than 50K user_ids. Please see one of the example from https://log.gprd.gitlab.net/goto/c47d1a201ecd998b7163f8bda61a6215
Example query
(NEW with users table)
explain SELECT
"users".*
FROM
"users"
WHERE
"users"."id" IN (
SELECT
"notification_settings"."user_id"
FROM
"notification_settings"
WHERE
(
"notification_settings"."source_id" = 278964
AND "notification_settings"."source_type" = 'Project'
OR "notification_settings"."source_type" = 'Namespace'
AND "notification_settings"."source_id" IN (9970)
)
AND (
(
"notification_settings"."level" = 3
AND EXISTS (
SELECT
true
FROM
"notification_settings" "notification_settings_2"
WHERE
"notification_settings_2"."user_id" = "notification_settings"."user_id"
AND "notification_settings_2"."source_id" IS NULL
AND "notification_settings_2"."source_type" IS NULL
AND "notification_settings_2"."level" = 5
)
)
OR "notification_settings"."level" = 5
)
)
Explain: https://explain.depesz.com/s/r5D5
(Without users table)
explain SELECT
"notification_settings"."user_id"
FROM
"notification_settings"
WHERE
(
(
"notification_settings"."level" = 3
AND EXISTS (
SELECT
true
FROM
"notification_settings" "notification_settings_2"
WHERE
"notification_settings_2"."user_id" = "notification_settings"."user_id"
AND "notification_settings_2"."source_id" IS NULL
AND "notification_settings_2"."source_type" IS NULL
AND "notification_settings_2"."level" = 5
)
)
OR "notification_settings"."level" = 5
)
AND (
"notification_settings"."source_id" = 278964
AND "notification_settings"."source_type" = 'Project'
OR "notification_settings"."source_type" = 'Namespace'
AND "notification_settings"."source_id" IN (9970)
)
Explain: 295.284ms - https://explain.depesz.com/s/tnIo
It utilizes the new index which is being introduced in this MR !58895 (merged).
It's still not under 100ms, but I think it's still an improvement since this change essentially replaces 4 queries via user_ids_notificable_on
and another query via settings_with_global_level_of
user_ids_notificable_on
2 main queries with many rows from explain SELECT "notification_settings".user_id
FROM "notification_settings"
WHERE "notification_settings"."source_id" = 278964
AND "notification_settings"."source_type" = 'Project'
AND "notification_settings"."level" = 3
This query is relatively fast at ~10ms, but it returns 16,219 records - https://explain.depesz.com/s/7jmJ
explain SELECT "notification_settings".user_id
FROM "notification_settings"
WHERE "notification_settings"."source_id" = 9970
AND "notification_settings"."source_type" = 'Namespace'
AND "notification_settings"."level" = 3
This query is also relatively fast at ~20ms. but it returns 46,349 records - https://explain.depesz.com/s/HSIr FYI, these are the results after introducing the new index.
These were then passed into settings_with_global_level_of
as user_ids
which performs badly for projects with many notification settings such as gitlab
- https://explain.depesz.com/s/OzsB
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?- [-] I have included a changelog entry.
- [-] I have not included a changelog entry because it doesn't have user facing changes.
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team