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