Skip to content

Add additional index to merge_request for project/status/created_at

What does this MR do?

Based on research looking into spikes in performance for Projects::MergeRequestController#index, existing indices don't seem specific enough. #325605 (closed) has more details on my poking around, but here are the relevant bits for this MR.

Problem query

SELECT "merge_requests".* FROM 
"merge_requests" WHERE "merge_requests"."target_project_id" 
= $1 AND ("merge_requests"."state_id" 
IN ($2)) ORDER BY "merge_requests"."created_at" 
DESC, "merge_requests"."id" DESC 
LIMIT $3 OFFSET $4

Index

CREATE INDEX index_merge_requests_on_target_project_id_and_created_at_and_state_id_and_id 
  ON merge_requests 
  USING btree (target_project_id, created_at, state_id, id);

In testing, it is hard to replicate the issue, since for most projects, the most recent MRs are open (state_id = 1) so the benefit is tiny, if anything:

Before

Limit  (cost=0.57..754.16 rows=20 width=756) (actual time=27.256..117.726 rows=20 loops=1)
   Buffers: shared hit=1 read=68 dirtied=21
   I/O Timings: read=112.662
   ->  Index Scan using index_merge_requests_on_target_project_id_and_created_at_and_id on public.merge_requests  (cost=0.57..73325.00 rows=1946 width=756) (actual time=27.254..117.699 rows=20 loops=1)
         Index Cond: (merge_requests.target_project_id = 278964)
         Filter: (merge_requests.state_id = 1)
         Rows Removed by Filter: 2
         Buffers: shared hit=1 read=68 dirtied=21
         I/O Timings: read=112.662


Time: 123.235 ms
  - planning: 5.428 ms
  - execution: 117.807 ms
    - I/O read: 112.662 ms
    - I/O write: N/A

Shared buffers:
  - hits: 1 (~8.00 KiB) from the buffer pool
  - reads: 68 (~544.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 21 (~168.00 KiB)
  - writes: 0

After

 Limit  (cost=0.57..25.77 rows=20 width=756) (actual time=0.351..0.545 rows=20 loops=1)
   Buffers: shared hit=23 read=4
   I/O Timings: read=0.261
   ->  Index Scan using idx_mrs_on_target_id_and_created_at_and_state_id on public.merge_requests  (cost=0.57..2480.16 rows=1968 width=756) (actual time=0.349..0.539 rows=20 loops=1)
         Index Cond: ((merge_requests.target_project_id = 278964) AND (merge_requests.state_id = 1))
         Buffers: shared hit=23 read=4


Time: 5.577 ms
  - planning: 4.969 ms
  - execution: 0.608 ms
    - I/O read: 0.261 ms
    - I/O write: N/A

Shared buffers:
  - hits: 23 (~184.00 KiB) from the buffer pool
  - reads: 4 (~32.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

However, if I flip the ORDERED BY direction to ASC to mimic the behavior of having to scan far to find results, we see before behavior that mimics the performance of the query when it is misbehaving.

Migration

Up

== 20210323182846 AddProjectStatusDateIndexToMergeRequests: migrating =========
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:merge_requests, [:target_project_id, :state_id, :created_at, :id], {:name=>"idx_mrs_on_target_id_and_created_at_and_state_id", :algorithm=>:concurrently})
   -> 0.0239s
-- execute("SET statement_timeout TO 0")
   -> 0.0013s
-- add_index(:merge_requests, [:target_project_id, :state_id, :created_at, :id], {:name=>"idx_mrs_on_target_id_and_created_at_and_state_id", :algorithm=>:concurrently})
   -> 0.0071s
-- execute("RESET ALL")
   -> 0.0015s
== 20210323182846 AddProjectStatusDateIndexToMergeRequests: migrated (0.0356s)

Down

== 20210323182846 AddProjectStatusDateIndexToMergeRequests: reverting =========
-- transaction_open?()
   -> 0.0000s
-- indexes(:merge_requests)
   -> 0.0161s
-- execute("SET statement_timeout TO 0")
   -> 0.0008s
-- remove_index(:merge_requests, {:algorithm=>:concurrently, :name=>"idx_mrs_on_target_id_and_created_at_and_state_id"})
   -> 0.0051s
-- execute("RESET ALL")
   -> 0.0010s
== 20210323182846 AddProjectStatusDateIndexToMergeRequests: reverted (0.0242s)

Related to #325605 (closed)

Screenshots (strongly suggested)

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

Related to #325605 (closed)

Edited by Adam Hegyi

Merge request reports