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
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
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
Related to #325605 (closed)