Skip to content

Remove Select N+1 Query in Project Authorizations Refresh

What does this MR do and why?

This change removes an N+1 issue that occurs from fetching a project record for project authorization record that is inserted. This happens due to this line introduced in !156775 (merged).

https://gitlab.com/gitlab-org/gitlab/-/issues/479316#note_2184745991

Before:

モ bundle exec rspec spec/workers/authorized_projects_worker_spec.rb:31
Run options: include {:focus=>true, :locations=>{"./spec/workers/authorized_projects_worker_spec.rb"=>[31]}}

Test environment set up in 1.072537721 seconds
F

Failures:

  1) AuthorizedProjectsWorker when running the worker prevents N+1 queries
     Failure/Error:
       expect { described_class.new.perform(user.id) }
         .not_to exceed_query_limit(control).with_threshold(inserts + deletes + updates)

       Expected a maximum of 4 (+4) queries, got 15:

       Query Diff:
       -----------
       DELETE FROM "project_authorizations"...
       -- (expected: 0, got: 1)
          WHERE "project_authorizations"."user_id" = 176 AND "project_authorizations"."project_id" = 124

       INSERT INTO "project_authorizations" ("user_id","project_id","access_level","is_unique")...
       -- (expected: 0, got: 1)
          VALUES (176, 122, 30, TRUE), (176, 123, 30, TRUE) ON CONFLICT ("user_id","project_id","access_level") DO NOTHING RETURNING "user_id","project_id","access_level"

       SELECT "projects"."id", "projects"."name", "projects"."path", "projects"."description", "projects"."created_at", "projects"."updated_at", "projects"."creator_id", "projects"."namespace_id", "projects"."last_activity_at", "projects"."import_url", "projects"."visibility_level", "projects"."archived", "projects"."avatar", "projects"."merge_requests_template", "projects"."star_count", "projects"."merge_requests_rebase_enabled", "projects"."import_type", "projects"."import_source", "projects"."approvals_before_merge", "projects"."reset_approvals_on_push", "projects"."merge_requests_ff_only_enabled", "projects"."issues_template", "projects"."mirror", "projects"."mirror_last_update_at", "projects"."mirror_last_successful_update_at", "projects"."mirror_user_id", "projects"."shared_runners_enabled", "projects"."runners_token", "projects"."build_allow_git_fetch", "projects"."build_timeout", "projects"."mirror_trigger_builds", "projects"."pending_delete", "projects"."public_builds", "projects"."last_repository_check_failed", "projects"."last_repository_check_at", "projects"."only_allow_merge_if_pipeline_succeeds", "projects"."has_external_issue_tracker", "projects"."repository_storage", "projects"."repository_read_only", "projects"."request_access_enabled", "projects"."has_external_wiki", "projects"."ci_config_path", "projects"."lfs_enabled", "projects"."description_html", "projects"."only_allow_merge_if_all_discussions_are_resolved", "projects"."repository_size_limit", "projects"."printing_merge_request_link_enabled", "projects"."auto_cancel_pending_pipelines", "projects"."service_desk_enabled", "projects"."cached_markdown_version", "projects"."delete_error", "projects"."last_repository_updated_at", "projects"."disable_overriding_approvers_per_merge_request", "projects"."storage_version", "projects"."resolve_outdated_diff_discussions", "projects"."remote_mirror_available_overridden", "projects"."only_mirror_protected_branches", "projects"."pull_mirror_available_overridden", "projects"."jobs_cache_index", "projects"."external_authorization_classification_label", "projects"."mirror_overwrites_diverged_branches", "projects"."pages_https_only", "projects"."external_webhook_token", "projects"."packages_enabled", "projects"."merge_requests_author_approval", "projects"."pool_repository_id", "projects"."runners_token_encrypted", "projects"."bfg_object_map", "projects"."detected_repository_languages", "projects"."merge_requests_disable_committers_approval", "projects"."require_password_to_approve", "projects"."max_pages_size", "projects"."max_artifacts_size", "projects"."pull_mirror_branch_prefix", "projects"."remove_source_branch_after_merge", "projects"."marked_for_deletion_at", "projects"."marked_for_deletion_by_user_id", "projects"."autoclose_referenced_issues", "projects"."suggestion_commit_message", "projects"."project_namespace_id", "projects"."hidden", "projects"."organization_id" FROM find_projects_by_id(124) AS projects WHERE ("projects"."id" IS NOT NULL) LIMIT 1...
       -- (expected: 0, got: 1)


       SELECT "projects"."id", "projects"."name", "projects"."path", "projects"."description", "projects"."created_at", "projects"."updated_at", "projects"."creator_id", "projects"."namespace_id", "projects"."last_activity_at", "projects"."import_url", "projects"."visibility_level", "projects"."archived", "projects"."avatar", "projects"."merge_requests_template", "projects"."star_count", "projects"."merge_requests_rebase_enabled", "projects"."import_type", "projects"."import_source", "projects"."approvals_before_merge", "projects"."reset_approvals_on_push", "projects"."merge_requests_ff_only_enabled", "projects"."issues_template", "projects"."mirror", "projects"."mirror_last_update_at", "projects"."mirror_last_successful_update_at", "projects"."mirror_user_id", "projects"."shared_runners_enabled", "projects"."runners_token", "projects"."build_allow_git_fetch", "projects"."build_timeout", "projects"."mirror_trigger_builds", "projects"."pending_delete", "projects"."public_builds", "projects"."last_repository_check_failed", "projects"."last_repository_check_at", "projects"."only_allow_merge_if_pipeline_succeeds", "projects"."has_external_issue_tracker", "projects"."repository_storage", "projects"."repository_read_only", "projects"."request_access_enabled", "projects"."has_external_wiki", "projects"."ci_config_path", "projects"."lfs_enabled", "projects"."description_html", "projects"."only_allow_merge_if_all_discussions_are_resolved", "projects"."repository_size_limit", "projects"."printing_merge_request_link_enabled", "projects"."auto_cancel_pending_pipelines", "projects"."service_desk_enabled", "projects"."cached_markdown_version", "projects"."delete_error", "projects"."last_repository_updated_at", "projects"."disable_overriding_approvers_per_merge_request", "projects"."storage_version", "projects"."resolve_outdated_diff_discussions", "projects"."remote_mirror_available_overridden", "projects"."only_mirror_protected_branches", "projects"."pull_mirror_available_overridden", "projects"."jobs_cache_index", "projects"."external_authorization_classification_label", "projects"."mirror_overwrites_diverged_branches", "projects"."pages_https_only", "projects"."external_webhook_token", "projects"."packages_enabled", "projects"."merge_requests_author_approval", "projects"."pool_repository_id", "projects"."runners_token_encrypted", "projects"."bfg_object_map", "projects"."detected_repository_languages", "projects"."merge_requests_disable_committers_approval", "projects"."require_password_to_approve", "projects"."max_pages_size", "projects"."max_artifacts_size", "projects"."pull_mirror_branch_prefix", "projects"."remove_source_branch_after_merge", "projects"."marked_for_deletion_at", "projects"."marked_for_deletion_by_user_id", "projects"."autoclose_referenced_issues", "projects"."suggestion_commit_message", "projects"."project_namespace_id", "projects"."hidden", "projects"."organization_id" FROM find_projects_by_id(122) AS projects WHERE ("projects"."id" IS NOT NULL) LIMIT 1...
       -- (expected: 0, got: 1)


       SELECT "projects"."id", "projects"."name", "projects"."path", "projects"."description", "projects"."created_at", "projects"."updated_at", "projects"."creator_id", "projects"."namespace_id", "projects"."last_activity_at", "projects"."import_url", "projects"."visibility_level", "projects"."archived", "projects"."avatar", "projects"."merge_requests_template", "projects"."star_count", "projects"."merge_requests_rebase_enabled", "projects"."import_type", "projects"."import_source", "projects"."approvals_before_merge", "projects"."reset_approvals_on_push", "projects"."merge_requests_ff_only_enabled", "projects"."issues_template", "projects"."mirror", "projects"."mirror_last_update_at", "projects"."mirror_last_successful_update_at", "projects"."mirror_user_id", "projects"."shared_runners_enabled", "projects"."runners_token", "projects"."build_allow_git_fetch", "projects"."build_timeout", "projects"."mirror_trigger_builds", "projects"."pending_delete", "projects"."public_builds", "projects"."last_repository_check_failed", "projects"."last_repository_check_at", "projects"."only_allow_merge_if_pipeline_succeeds", "projects"."has_external_issue_tracker", "projects"."repository_storage", "projects"."repository_read_only", "projects"."request_access_enabled", "projects"."has_external_wiki", "projects"."ci_config_path", "projects"."lfs_enabled", "projects"."description_html", "projects"."only_allow_merge_if_all_discussions_are_resolved", "projects"."repository_size_limit", "projects"."printing_merge_request_link_enabled", "projects"."auto_cancel_pending_pipelines", "projects"."service_desk_enabled", "projects"."cached_markdown_version", "projects"."delete_error", "projects"."last_repository_updated_at", "projects"."disable_overriding_approvers_per_merge_request", "projects"."storage_version", "projects"."resolve_outdated_diff_discussions", "projects"."remote_mirror_available_overridden", "projects"."only_mirror_protected_branches", "projects"."pull_mirror_available_overridden", "projects"."jobs_cache_index", "projects"."external_authorization_classification_label", "projects"."mirror_overwrites_diverged_branches", "projects"."pages_https_only", "projects"."external_webhook_token", "projects"."packages_enabled", "projects"."merge_requests_author_approval", "projects"."pool_repository_id", "projects"."runners_token_encrypted", "projects"."bfg_object_map", "projects"."detected_repository_languages", "projects"."merge_requests_disable_committers_approval", "projects"."require_password_to_approve", "projects"."max_pages_size", "projects"."max_artifacts_size", "projects"."pull_mirror_branch_prefix", "projects"."remove_source_branch_after_merge", "projects"."marked_for_deletion_at", "projects"."marked_for_deletion_by_user_id", "projects"."autoclose_referenced_issues", "projects"."suggestion_commit_message", "projects"."project_namespace_id", "projects"."hidden", "projects"."organization_id" FROM find_projects_by_id(123) AS projects WHERE ("projects"."id" IS NOT NULL) LIMIT 1...
       -- (expected: 0, got: 1)


       SAVEPOINT active_record_2...
       -- (expected: 0, got: 1)


       SELECT "user_details"."user_id", "user_details"."job_title", "user_details"."bio", "user_details"."webauthn_xid", "user_details"."provisioned_by_group_id", "user_details"."pronouns", "user_details"."pronunciation", "user_details"."registration_objective", "user_details"."phone", "user_details"."linkedin", "user_details"."twitter", "user_details"."skype", "user_details"."website_url", "user_details"."location", "user_details"."organization", "user_details"."password_last_changed_at", "user_details"."discord", "user_details"."enterprise_group_id", "user_details"."enterprise_group_associated_at", "user_details"."email_reset_offered_at", "user_details"."mastodon", "user_details"."project_authorizations_recalculated_at", "user_details"."onboarding_status", "user_details"."bluesky" FROM "user_details"...
       -- (expected: 0, got: 1)
          WHERE "user_details"."user_id" = 176 LIMIT 1

       SELECT "namespaces"."id", "namespaces"."name", "namespaces"."path", "namespaces"."owner_id", "namespaces"."created_at", "namespaces"."updated_at", "namespaces"."type", "namespaces"."description", "namespaces"."avatar", "namespaces"."membership_lock", "namespaces"."share_with_group_lock", "namespaces"."visibility_level", "namespaces"."request_access_enabled", "namespaces"."ldap_sync_status", "namespaces"."ldap_sync_error", "namespaces"."ldap_sync_last_update_at", "namespaces"."ldap_sync_last_successful_update_at", "namespaces"."ldap_sync_last_sync_at", "namespaces"."description_html", "namespaces"."lfs_enabled", "namespaces"."parent_id", "namespaces"."shared_runners_minutes_limit", "namespaces"."repository_size_limit", "namespaces"."require_two_factor_authentication", "namespaces"."two_factor_grace_period", "namespaces"."cached_markdown_version", "namespaces"."project_creation_level", "namespaces"."runners_token", "namespaces"."file_template_project_id", "namespaces"."saml_discovery_token", "namespaces"."runners_token_encrypted", "namespaces"."custom_project_templates_group_id", "namespaces"."auto_devops_enabled", "namespaces"."extra_shared_runners_minutes_limit", "namespaces"."last_ci_minutes_notification_at", "namespaces"."last_ci_minutes_usage_notification_level", "namespaces"."subgroup_creation_level", "namespaces"."max_pages_size", "namespaces"."max_artifacts_size", "namespaces"."mentions_disabled", "namespaces"."default_branch_protection", "namespaces"."max_personal_access_token_lifetime", "namespaces"."push_rule_id", "namespaces"."shared_runners_enabled", "namespaces"."allow_descendants_override_disabled_shared_runners", "namespaces"."traversal_ids", "namespaces"."organization_id" FROM "namespaces"...
       -- (expected: 0, got: 1)
          WHERE "namespaces"."owner_id" = 176 AND "namespaces"."type" = 'User' LIMIT 1

       SELECT 1 AS one FROM "users"...
       -- (expected: 0, got: 1)
          WHERE "users"."username" = 'user1' AND "users"."id" != 176 LIMIT 1

       INSERT INTO "user_details" ("user_id", "project_authorizations_recalculated_at")...
       -- (expected: 0, got: 1)
          VALUES (176, '2024-10-30 19:22:28.875019') RETURNING "user_id"

       RELEASE SAVEPOINT active_record_2...
       -- (expected: 0, got: 1)

     # ./spec/workers/authorized_projects_worker_spec.rb:53:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:483:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/sidekiq_sharding/validator.rb:42:in `enabled'
     # ./spec/spec_helper.rb:482:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:477:in `block (3 levels) in <top (required)>'
     # ./spec/support/sidekiq_middleware.rb:9:in `with_sidekiq_server_middleware'
     # ./spec/spec_helper.rb:468:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:464:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/application_context.rb:92:in `with_raw_context'
     # ./spec/spec_helper.rb:464:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:277:in `block (2 levels) in <top (required)>'
     # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'

Finished in 6.08 seconds (files took 12.57 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/workers/authorized_projects_worker_spec.rb:31 # AuthorizedProjectsWorker when running the worker prevents N+1 queries

After:

モ bundle exec rspec spec/workers/authorized_projects_worker_spec.rb:31
Run options: include {:focus=>true, :locations=>{"./spec/workers/authorized_projects_worker_spec.rb"=>[31]}}

Test environment set up in 1.136426312 seconds
.

Finished in 5.86 seconds (files took 12.56 seconds to load)
1 example, 0 failures

References

Please include cross links to any resources that are relevant to this MR This will give reviewers and future readers helpful context to give an efficient review of the changes introduced.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by mo khan

Merge request reports

Loading