Skip to content

Fix 500 error sorting merge requests with approvers by milestone

Stan Hu requested to merge sh-fix-merge-request-finder-milestone into master

What does this MR do and why?

When adding a Approved By or Approver filter and sorting by milestone, a 500 error will occur because of a SQL error:

PG::GroupingError: ERROR:  column "milestones.due_date" must appear in
the GROUP BY clause or be used in an aggregate function

This happens because the approval query uses a GROUP BY query (https://gitlab.com/gitlab-org/gitlab/-/blob/29479455a951d1daf379471a5579c00b60897a43/app/models/concerns/approvable.rb#L24) to find merge requests that match all approvers. However, this GROUP BY query breaks the SQL query since it's expected that milestones.due_date and milestones.id are present in that clause.

We can solve this problem in a few ways:

  1. Add the required grouping columns to this query (this commit).
  2. Use a CTE to isolate the query for approvers.
  3. Rewrite the approvers query to avoid the GROUP BY altogether.

The second way appears to work, but there's a risk of running into errors due to nested queries. Plus, it does not appear the CTE improves performance.

The third way may also be possible, but it would possibly require some schema changes or other refactoring.

For now, we can solve this problem by manually adding the required grouping columns when approvers or approvals are specified.

Relates to #223062 (closed)

How to set up and validate locally

  1. Go to your MR dashboard on dashboard/merge_requests.
  2. Go to Merged tab.
  3. Add filter Approved By with any user selection, by default this can potentially try to load a lot of matches so you can additionally add Milestone as one more criteria.
  4. Sort by Milestone Due Date
  5. Hit Enter/Return to apply the filter.

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

Queries

Before (error)

SELECT "merge_requests".*
FROM "merge_requests"
INNER JOIN "projects" ON "projects"."id" = "merge_requests"."target_project_id"
INNER JOIN "approvals" ON "approvals"."merge_request_id" = "merge_requests"."id"
INNER JOIN "users" ON "users"."id" = "approvals"."user_id"
LEFT JOIN project_features ON projects.id = project_features.project_id
LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS
         (SELECT 1
          FROM "project_authorizations"
          WHERE "project_authorizations"."user_id" = 64248
            AND (project_authorizations.project_id = projects.id))
       OR projects.visibility_level IN (0,
                                        10,
                                        20))
  AND ("project_features"."merge_requests_access_level" > 0
       OR "project_features"."merge_requests_access_level" IS NULL)
  AND ("merge_requests"."state_id" IN (3))
  AND "projects"."archived" = FALSE
  AND "users"."username" = 'stanhu'
GROUP BY "merge_requests"."id",
HAVING (COUNT(users.id) = 1)
ORDER BY milestones.due_date IS NULL,
                                milestones.id IS NULL,
                                                 milestones.due_date ASC,
                                                 "merge_requests"."id" DESC
LIMIT 20
OFFSET 0

After

SELECT "merge_requests".*
FROM "merge_requests"
INNER JOIN "projects" ON "projects"."id" = "merge_requests"."target_project_id"
INNER JOIN "approvals" ON "approvals"."merge_request_id" = "merge_requests"."id"
INNER JOIN "users" ON "users"."id" = "approvals"."user_id"
LEFT JOIN project_features ON projects.id = project_features.project_id
LEFT OUTER JOIN milestones ON merge_requests.milestone_id = milestones.id
WHERE (EXISTS
         (SELECT 1
          FROM "project_authorizations"
          WHERE "project_authorizations"."user_id" = 64428
            AND (project_authorizations.project_id = projects.id))
       OR projects.visibility_level IN (0,
                                        10,
                                        20))
  AND ("project_features"."merge_requests_access_level" > 0
       OR "project_features"."merge_requests_access_level" IS NULL)
  AND ("merge_requests"."state_id" IN (3))
  AND "projects"."archived" = FALSE
  AND "users"."username" = 'stanhu'
GROUP BY "merge_requests"."id",
         "merge_requests"."id",
         "milestones"."id",
         "milestones"."due_date"
HAVING (COUNT(users.id) = 1)
ORDER BY milestones.due_date IS NULL,
                                milestones.id IS NULL,
                                                 milestones.due_date ASC,
                                                 "merge_requests"."id" DESC
LIMIT 20
OFFSET 0

Query plan (warm cache): https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/12077/commands/42853

Time: 139.380 ms
  - planning: 3.646 ms
  - execution: 135.734 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 94367 (~737.20 MiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0
Edited by Stan Hu

Merge request reports