Skip to content

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

2 main queries with many rows from user_ids_notificable_on

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

Availability and Testing

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
Edited by Sincheol (David) Kim

Merge request reports