Skip to content

Draft: Milestones::DestroyService: reduce SQL queries count [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Aleksei Lipniagov requested to merge 21090-reduce-sql-queries-count into master

What does this MR do?

Reduce the amount of SQL queries Milestones::DestroyService produces.

Fixes #21090 (closed)

⚠ domain expert review needed

It is rather radical change:

  • We don't execute Issues::UpdateService on each related issue, as we pass skip_milestone_email=true, and the underlying action is a no-op (need domain expert check), but still produces a lot of bogus DB queries
  • We don't execute MergeRequests::UpdateService on each related merge request, as we pass skip_milestone_email=true, and the underlying action is a no-op (need domain expert check), but still produces a lot of bogus DB queries
  • There is no need to nullify target_id in each of the connected Event, and I haven't found any Event-related callbacks also

The gain is decreasing the number of SQL queries from thousands to < 10, which could significantly reduce the primary DB pressure, especially during spikes.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

You could check the amount of queries execute using the QueryRecorder.

Run: QUERY_RECORDER_DEBUG=1 bin/spring rspec spec/services/milestones/destroy_service_spec.rb:25.

It is worth updating the spec, so it would create multiple issues and merge requests to make the underlying problem visible:

For example:

    it 'deletes milestone id from issuables' do
      10.times do |i|
        issue = create(:issue, project: project, milestone: milestone)
        merge_request = create(:merge_request, source_project: project, source_branch: "branch-#{i}", milestone: milestone)
      end

      control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
        service.execute(milestone)
      end
      binding.pry

      expect(issue.reload.milestone).to be_nil
      expect(merge_request.reload.milestone).to be_nil
    end

This records 912 queries already (and this already uses update_all for events here):

[1] pry(#<RSpec::ExampleGroups::MilestonesDestroyService::Execute>)> control.count
=> 912

After the change: it's only 9 for the same setup and test that produced 912 (above ^)

The additional spec (with QueryRecorder) fails on the master:

Failures:

  1) Milestones::DestroyService#execute avoids N+1 database queries
     Failure/Error: expect { service.execute(milestone) }.not_to exceed_query_limit(control_count)

       Expected a maximum of 103 queries, got 913:

Security

N/A

Related to #21090 (closed)

Edited by Aleksei Lipniagov

Merge request reports