Skip to content

Add a new index to improve query performance

Sincheol (David) Kim requested to merge improve_notification_settings_query_performance into master

What does this MR do?

This MR adds a new multicolumn index and replaces existing index to achive INDEX ONLY queries when querying notification settings.

I've also noticed that there was a redundant index on user_id so I'm removing that one too. We also have index on (user_id, source_id, source_type) so we have two similar index one starting from user_id and another one starting from source_id. I wonder if there is better way to handle this. 🤔

Related code https://gitlab.com/gitlab-org/gitlab/blob/87093b024ba01757c708bbe358ea644241e6febd/app/services/notification_recipients/builder/base.rb#L102

Related to #326045 (closed)

Example queries

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" = 5

BEFORE: 58.132ms - https://explain.depesz.com/s/BBlv

AFTER: 0.211ms - https://explain.depesz.com/s/mfnh

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

BEFORE: 89.414ms - https://explain.depesz.com/s/xyMU

AFTER: 13.221ms - https://explain.depesz.com/s/iaA8

!-- I'm working on rewriting the ruby logic into sql query to avoid returning massive list of user_ids only to feed back into the query later. This is one of those queries I'm working 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" = 5
    )
    OR (
      "notification_settings"."source_id" = 278964
      AND "notification_settings"."source_type" = 'Project'
      AND "notification_settings"."level" = 3
      AND EXISTS(
        SELECT 1
        FROM "notification_settings" n2
        WHERE n2.user_id = "notification_settings"."user_id"
        AND n2."source_id" IS NULL
        AND n2."source_type" IS NULL
        AND n2."level" = 5
      )
    )
    OR (
    "notification_settings"."source_id" = 9970
    AND "notification_settings"."source_type" = 'Namespace'
    AND "notification_settings"."level" = 5
    )
    OR (
      "notification_settings"."source_id" = 9970
      AND "notification_settings"."source_type" = 'Namespace'
      AND "notification_settings"."level" = 3
      AND EXISTS(
        SELECT 1
        FROM "notification_settings" n2
        WHERE n2.user_id = "notification_settings"."user_id"
        AND n2."source_id" IS NULL
        AND n2."source_type" IS NULL
        AND n2."level" = 5
      )
    )
  );

BEFORE: 830.092ms - https://explain.depesz.com/s/938d

AFTER: 311.206ms - https://explain.depesz.com/s/EUxr

Migration

UP

== 20210402005225 AddSourceAndLevelIndexOnNotificationSettings: migrating =====
-- transaction_open?()
  -> 0.0000s
-- index_exists?(:notification_settings, [:source_id, :source_type, :level, :user_id], {:name=>"index_notification_settings_on_source_and_level_and_user", :algorithm=>:concurrently})
  -> 0.0029s
-- execute("SET statement_timeout TO 0")
  -> 0.0006s
-- add_index(:notification_settings, [:source_id, :source_type, :level, :user_id], {:name=>"index_notification_settings_on_source_and_level_and_user", :algorithm=>:concurrently})
  -> 0.0036s
-- execute("RESET ALL")
  -> 0.0005s
-- transaction_open?()
  -> 0.0000s
-- indexes(:notification_settings)
  -> 0.0019s
-- remove_index(:notification_settings, {:algorithm=>:concurrently, :name=>"index_notification_settings_on_source_id_and_source_type"})
  -> 0.0021s
-- transaction_open?()
  -> 0.0000s
-- indexes(:notification_settings)
  -> 0.0016s
-- remove_index(:notification_settings, {:algorithm=>:concurrently, :name=>"index_notification_settings_on_user_id"})
  -> 0.0012s
== 20210402005225 AddSourceAndLevelIndexOnNotificationSettings: migrated (0.0171s)

DOWN

== 20210402005225 AddSourceAndLevelIndexOnNotificationSettings: reverting =====
-- transaction_open?()
   -> 0.0000s
-- indexes(:notification_settings)
   -> 0.0027s
-- execute("SET statement_timeout TO 0")
   -> 0.0007s
-- remove_index(:notification_settings, {:algorithm=>:concurrently, :name=>"index_notification_settings_on_source_and_level_and_user"})
   -> 0.0044s
-- execute("RESET ALL")
   -> 0.0007s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:notification_settings, [:source_id, :source_type], {:name=>"index_notification_settings_on_source_id_and_source_type", :algorithm=>:concurrently})
   -> 0.0013s
-- add_index(:notification_settings, [:source_id, :source_type], {:name=>"index_notification_settings_on_source_id_and_source_type", :algorithm=>:concurrently})
   -> 0.0038s
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:notification_settings, [:user_id], {:name=>"index_notification_settings_on_user_id", :algorithm=>:concurrently})
   -> 0.0016s
-- add_index(:notification_settings, [:user_id], {:name=>"index_notification_settings_on_user_id", :algorithm=>:concurrently})
   -> 0.0036s
== 20210402005225 AddSourceAndLevelIndexOnNotificationSettings: reverted (0.0217s)

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