Skip to content

Add index to todos the improve query performance

Adam Hegyi requested to merge optimize-frequently-called-todos-query into master

What does this MR do and why?

This MR adds an extra (async) index to support the query that updates the todo's status from pending to done. I found this query in the postgres checkup report: https://gitlab.com/gitlab-com/gl-infra/reliability/-/snippets/2401701

SELECT "todos"."id"
FROM "todos"
WHERE "todos"."user_id" = 3482158
  AND ("todos"."state" IN ('pending'))
  AND "todos"."project_id" = 278964
  AND "todos"."target_id" = 174214523
  AND "todos"."target_type" = 'MergeRequest'
  AND "todos"."state" != 'done'

Query plan: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/11816/commands/42163

  • Note 1: the "todos"."state" != 'done' part does not make much sense here. It's added by the Todo#batch_update method.
  • Note 2: the query is mainly called from UpdateMergeRequestsWorker. The query is also called when a new comment is added (with mention).

Uses a BITMAP AND with two sub-lookups:

  • Reads all comments by target_type and target_id: bad when MR/issue has many comment
  • Reads all pending comments by user: bad when user has many pending todos

The query is not particularly slow but it's called frequently on the primary (on each MR push, 11 calls/s). The only way I see to optimize this query is by adding an index which would reduce the uncached query time significantly. Caching/reducing the calls seems too complicated.

Improved query plan: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/11816/commands/42244

Migration

Up:

main: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: migrating ==========
main: -- index_exists?(:todos, [:user_id, :project_id, :target_type, :target_id, :state, :id], {:name=>"index_on_todos_user_project_target_and_state", :algorithm=>:concurrently})
main:    -> 0.0068s
main: -- add_index_options(:todos, [:user_id, :project_id, :target_type, :target_id, :state, :id], {:name=>"index_on_todos_user_project_target_and_state", :algorithm=>:concurrently})
main:    -> 0.0002s
main: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: migrated (0.0231s) =

ci: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: migrating ==========
ci: -- index_exists?(:todos, [:user_id, :project_id, :target_type, :target_id, :state, :id], {:name=>"index_on_todos_user_project_target_and_state", :algorithm=>:concurrently})
ci:    -> 0.0079s
ci: -- add_index_options(:todos, [:user_id, :project_id, :target_type, :target_id, :state, :id], {:name=>"index_on_todos_user_project_target_and_state", :algorithm=>:concurrently})
ci:    -> 0.0002s
ci: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: migrated (0.0133s) =

Down

ci: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: reverting ==========
ci: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: reverted (0.0158s) =

main: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: reverting ==========
main: == 20220905112710 AddAsyncIndexToTodosToCoverPendingQuery: reverted (0.0167s) =

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Adam Hegyi

Merge request reports

Loading